Hi Am 29.09.23 um 09:33 schrieb Geert Uytterhoeven:
Hi Thomas, On Fri, Sep 29, 2023 at 9:11 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:Am 28.09.23 um 17:32 schrieb Geert Uytterhoeven: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.To clarify: do you mean keeping the single big drm_modeset_helper_vtables.h, or the split drm_*_helper_vtable.h set?
I meant to keep the single big drm_modeset_helper_vtables.h. It's not nice to look at, but I think we might easily mess up the header dependencies with any changes.
Best regards Thomas
Thanks!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.hGr{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
