Re: [PATCH/RFC 3/3] drm: Split drm_modeset_helper_vtables.h

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 28.09.23 um 17:32 schrieb Geert Uytterhoeven:
Hi Thomas,

On Thu, Sep 28, 2023 at 3:59 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
Am 28.09.23 um 14:16 schrieb Geert Uytterhoeven:
<drm/drm_modeset_helper_vtables.h> is the second largest header file in
the DRM subsystem, and declares helpers vtables for various DRM
components.  Several vtables contain methods with the same name, and all
but one vtable do not fit on the screen, making it hard to navigate to
the actual method one is interested in.

Make it easier for the casual reviewer to keep track by splitting
<drm/drm_modeset_helper_vtables.h> in multiple header files, one per DRM
component.

I never liked this header either, but do we need new header files? Each
struct could be appended to the end of the regular header: struct
drm_plane_helper_funcs to drm_plane.h, drm_connector_helper_func to
drm_connector.h and so on.

That would work for me, too.  But perhaps we want to maintain a clear
separation between core and helpers?

Note that moving the contents to *_helper.h would be another option,
drm_crtc_helper.h and drm_plane_helper.h already exist.

I've taken a closer look at the users of the _vtables header. There's code in drm_atomic_helper.c or drm_probe_helper.c that invokes the callback functions.

The drivers fill the pointers with code that often comes from other helper modules. That code is in files like drm_plane_helper.c or drm_crtc_helper.c. There header files are drm_plane_helper.h, etc.

In that context, the _vtables header makes sense, as it separates the callers from the callees. Putting the structs into headers like drm_plane_helper.h would move it to the callee side.

I suggest to leave the header as it is. The fallout to the code base from refactoring seems worse than the current state.

Best regards
Thomas


Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
---
RFC, a future patch could replace inclusion of
<drm/drm_modeset_helper_vtables.h> by inclusion of one or more of the
new files, and reduce compilation time.
---
   include/drm/drm_connector_helper_vtable.h   |  364 +++++
   include/drm/drm_crtc_helper_vtable.h        |  483 ++++++
   include/drm/drm_encoder_helper_vtable.h     |  381 +++++
   include/drm/drm_mode_config_helper_vtable.h |   97 ++
   include/drm/drm_modeset_helper_vtables.h    | 1466 +------------------
   include/drm/drm_plane_helper_vtable.h       |  297 ++++
   6 files changed, 1627 insertions(+), 1461 deletions(-)
   create mode 100644 include/drm/drm_connector_helper_vtable.h
   create mode 100644 include/drm/drm_crtc_helper_vtable.h
   create mode 100644 include/drm/drm_encoder_helper_vtable.h
   create mode 100644 include/drm/drm_mode_config_helper_vtable.h
   create mode 100644 include/drm/drm_plane_helper_vtable.h

Gr{oetje,eeting}s,

                         Geert


--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux