Re: [PATCH 3/8] drm/display: Introduce a DRM display-helper module

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 30.03.22 um 11:23 schrieb Javier Martinez Canillas:
> On 3/22/22 20:27, Thomas Zimmermann wrote:
> > Replace the DP-helper module with a display-helper module. Update
> > all related Kconfig and Makefile rules.
> >
> > Besides the existing code for DisplayPort, the new module will
> > contain helpers for other video-output standards, such as HDMI.
> > Drivers will still be able to select the required video-output
> > helpers. Linking all such code into a single module avoids the
> > proliferation of small kernel modules.
> >
> > Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > ---
>
> [snip]
>
> > +config DRM_DISPLAY_HELPER
> > +	tristate
> > +	depends on DRM
> > +	help
> > +	  DRM helpers for display adapters.
> > +
> >   config DRM_DP_HELPER
> >   	tristate
> >   	depends on DRM
> > +	select DRM_DISPLAY_HELPER
> >   	help
> >   	  DRM helpers for DisplayPort.
>
> I was about to ask why this would still be needed but then re-read the
> commit message that says drivers will still be able to select required
> video-output helpers.
>
> That makes sense since the fact that all helpers will be in the same module
> would be transparent to drivers.
>
> > diff --git a/drivers/gpu/drm/display/Makefile b/drivers/gpu/drm/display/Makefile
> > index 75faffc706b1..90f12e9b4735 100644
> > --- a/drivers/gpu/drm/display/Makefile
> > +++ b/drivers/gpu/drm/display/Makefile
> > @@ -2,8 +2,9 @@
> >   
> >   obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
> >   
> > -drm_dp_helper-y := drm_dp.o drm_dp_dual_mode_helper.o drm_dp_helper_mod.o drm_dp_mst_topology.o
> > -drm_dp_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> > -drm_dp_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
> > +drm_display_helper-y := drm_display_helper_mod.o
> > +drm_display_helper-$(CONFIG_DRM_DP_HELPER) := drm_dp_helper.o drm_dp_dual_mode_helper.o drm_dp_mst_topology.o
> > +drm_display_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> > +drm_display_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
> >   
> > -obj-$(CONFIG_DRM_DP_HELPER) += drm_dp_helper.o
> > +obj-$(CONFIG_DRM_DISPLAY_HELPER) += drm_display_helper.o
>
> The drm_dp_helper.ko module has some parameters and this change will break
> existing kernel cmdline that are using it:
>
> $ modinfo drivers/gpu/drm/dp/drm_dp_helper.ko | grep parm | cut -d : -f2
>             drm_dp_cec_unregister_delay
>             dp_aux_i2c_speed_khz
>             dp_aux_i2c_transfer_size
>
> I don't know whether those are considered a kernel ABI or not though, and
> some already changed when the DP helpers were moved from drm_kms_helper.ko

Good point. I'll mention it in the commit message andcheck the 
documentation as well.

At least, no one complained when these functions moved from kms helpers 
into dp helpers. Moving them again is unfortunate, but I hope that the 
new library will stick.

I somehow expected that HDMI, HDCP et al would require their own 
libraries.  But introducing several new and tiny kernel modules for such 
small helpers wasn't worth it. Hence, there's the display library that 
can collect all such helpers in a single place.

It looks like MIPI DSI could be another candidate to be moved into the 
display library; at least partially. I have go through the codebase to 
see if there are drivers that would benefit from such a change.

Best regards
Thomas

>

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature