[PATCH] drm/vc4: Add module dependency on hdmi-codec

[Javier Martinez Canillas]

Hello Maxime,

On 9/2/22 16:41, Maxime Ripard wrote:
> The VC4 HDMI controller driver relies on the HDMI codec ASoC driver. In
> order to set it up properly, in vc4_hdmi_audio_init(), our HDMI driver
> will register a device matching the HDMI codec driver, and then register
> an ASoC card using that codec.
> 
> However, if vc4 is compiled as a module, chances are that the hdmi-codec
> driver will be too. In such a case, the module loader will have a very
> narrow window to load the module between the device registration and the
> card registration.
> 
> If it fails to load the module in time, the card registration will fail
> with EPROBE_DEFER, and we'll abort the audio initialisation,
> unregistering the HDMI codec device in the process.
> 
> The next time the bind callback will be run, it's likely that we end up
> missing that window again, effectively preventing vc4 to probe entirely.
> 
> In order to prevent this, we can create a soft dependency of the vc4
> driver on the HDMI codec one so that we're sure the HDMI codec will be
> loaded before the VC4 module is, and thus we'll never end up in the
> previous situation.
> 
> Fixes: 91e99e113929 ("drm/vc4: hdmi: Register HDMI codec")
> Signed-off-by: Maxime Ripard <maxime at cerno.tech>
> ---
>  drivers/gpu/drm/vc4/vc4_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index ffbbb454c9e8..2027063fdc30 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -490,6 +490,7 @@ module_init(vc4_drm_register);
>  module_exit(vc4_drm_unregister);
>  
>  MODULE_ALIAS("platform:vc4-drm");
> +MODULE_SOFTDEP("pre: snd-soc-hdmi-codec");

While this will solve the issue, I'm not a big fan of the MODULE_SOFTDEP()
macro to be honest. I always have the feeling that's fragile and something
that may not be updated if for example a module name changes in the future.

I wonder if instead the HDMI_CODEC_DRV_NAME platform device should not be
registered earlier and in a place that's not cleaned up in the probe error
path. For example in vc4_drm_register() instead of vc4_hdmi_audio_init().

Granted, that would register the platform device even when HDMI isn't used
which is disadvantage of doing it at vc4_hdmi_bind() time. But still, feel
that is more robust to rely on the Linux device model than module softdeps.

Having said that, I don't have a strong opinion and this patch is correct
and has been in the mailing list for a long time. So feel free to push it
if you think is the correct approach to solve this:

Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat