Re: [PATCH] drm: Split out drm_probe_helper.h

On Mon, Jan 21, 2019 at 11:13 PM Sam Ravnborg [off-list ref] wrote:

Hi Daniel et al.

quoted

quoted

Yeah the drm_crtc_helper.h header is a bit the miniature drmP.h for legacy
kms drivers. Just removing it from all the atomic drivers caused lots of
fallout, I expect even more if you entirely remove the includes it has.
Maybe a todo, care to pls create that patch since it's your idea?

The main reason I bailed out initially was that this would create
small changes to several otherwise seldomly touched files.
And then we would later come and remove drmP.h - so lots of
small but incremental changes to the same otherwise seldomly
edited files.
And the job was only partially done.

I will try to experiment with an approach where I clean up the
include/drm/*.h files a little (like suggested above, +delete drmP.h
and maybe a bit more).

Then to try on a driver by driver basis to make it build with a
cleaned set of include files.
I hope that the cleaned up driver can still build without the
cleaned header files so the changes can be submitted piecemal.

Will do so with an eye on the lesser maintained drivers to try it
out to avoid creating too much chrunch for others.

I have now a few patches queued, but the result is not too pretty.
I did the following:

- For all files in include/drm/*.h the set of include files
  were adjusted to the minimum number of files required to make
  them build without any other files included first.

  Created one .c file for each .h file. Then included the .h
  file and adjusted to the minimal set of include files.
  In the process a lot of forwards were added.

- Deleted drmP.h

- Fixed build of a few drivers: sti, tilcdc, gma500, tve200, via

Some observations:

- Killing all the includes not needed in the headers files
  results in a a lot of extra changes.
  Examples:
    drm_modseset_helper_vtables.h is no longer
    included by anyone, so needs to be added in many files

    drm_atomic_state_helper.h is no longer included
    by anyone so likewise needs to be added in many files

- It is very tedious to do this properly.
  The process I followed was:
  - delete / comment out all include files
  - add back the obvious from a quick scan of the code
  - build - fix - build - fix - build - fix ...
  -   next file...

- The result is errorprone as only the allyesconfig + allmodconfig
  variants are tested. But reallife configurations are more diverse.

Current diffstat:
   111 files changed, 771 insertions(+), 401 deletions(-)

This is for the 5 drivers alone and not the header cleanup.
So long story short - this is not good and not the way forward.

I will try to come up with a few improvements to make the
headers files selfcontained, but restricted to the changes that
add forwards/include to avoid the chrunch in all the drivers.

And then post for review a few patches to clean up some headers.
If the cleanup gets a go I will try to persuade the introduction
of these.
This will include, but will not be limited to, the above mentioned
drm_crtc_helper.h header file.

For now too much time was already spent on this, so it is at the
moment pushed back on my TODO list.
This mail serve also as a kind of "where had I left", when/if I
pick this up again.

If there are anyone that knows some tooling that can help in the
process of adjusting the header files I am all ears.

Yeah in the process of splitting up drmP.h we've created a few smaller
such piles of headers. I think in some cases it's just not going to be
worth it to fully split them up, e.g. drm_crtc_helper.h is going to be
a pure legacy helper, only needed by pre-atomic drivers. Splitting
that up doesn't seem to useful to me. Similarly we might want
drm_atomic_helper.h to keep pulling in the other helper headers. So
probably going to be a judgement call on a case-by-case basis.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel