Re: [PATCH] drm/tidss: encoder: convert to devm_drm_bridge_alloc()

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hello Michael,

On Wed, 16 Jul 2025 15:41:07 +0200
Michael Walle <mwalle@kernel.org> wrote:

> Convert the tidss encoder to use devm_drm_bridge_alloc(). Instead of
> allocating the memory by drmm_simple_encoder_alloc() use
> devm_drm_bridge_alloc() and initialize the encoder afterwards.
> 
> Fixes: a7748dd127ea ("drm/bridge: get/put the bridge reference in drm_bridge_add/remove()")
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> ---
>  drivers/gpu/drm/tidss/tidss_encoder.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
> index 95b4aeff2775..81a04f767770 100644
> --- a/drivers/gpu/drm/tidss/tidss_encoder.c
> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
> @@ -90,14 +90,18 @@ int tidss_encoder_create(struct tidss_device *tidss,
>  	struct drm_connector *connector;
>  	int ret;
>  
> -	t_enc = drmm_simple_encoder_alloc(&tidss->ddev, struct tidss_encoder,
> -					  encoder, encoder_type);
> +	t_enc = devm_drm_bridge_alloc(tidss->dev, struct tidss_encoder,
> +				      bridge, &tidss_bridge_funcs);
>  	if (IS_ERR(t_enc))
>  		return PTR_ERR(t_enc);
>  
> +	ret = drm_simple_encoder_init(&tidss->ddev, &t_enc->encoder,
> +				      encoder_type);

Nit: this should be a single line (< 100 chars).

> +	if (ret)
> +		return ret;
> +
>  	t_enc->tidss = tidss;
>  	t_enc->next_bridge = next_bridge;
> -	t_enc->bridge.funcs = &tidss_bridge_funcs;
>  
>  	enc = &t_enc->encoder;
>  	enc->possible_crtcs = possible_crtcs;

This patch switches from drmm_ to devm_, but I haven't checked in
detail whether this introduces a different lifetime. At a very quick
glance however it looks fine.

With that assumption, looks like a correct patch.

However allocating an encoder using a bridge alloc function (while we
used to call an encoder allocation function) looks counter-intuitive.

We had discussed on IRC a different idea, adding a wrapper structure
around the bridge. Quoting your proposal:

  struct tidss_encoder_bridge {
    struct drm_bridge bridge;
    struct tidss_encoder *encoder
  }

and then in the bridge funcs go from drm_bridge to tidss_encoder_brigde
and use the pointer to get the original private struct.

That would be cleaner and more intuitive, but use a bit more memory and
have an additional pointer deref, thus I think we can live with the
patch you just sent, at least for now.

In the long term it may be that more and more components of the DRM
subsystem become dynamically allocated like bridges and panels [0] have
recently become. So at some point it may make sense to add the bridge
wrapper struct and then go back to drmm_simple_encoder_alloc() for
allocation.

[0] https://lore.kernel.org/all/20250331-b4-panel-refcounting-v4-1-dad50c60c6c9@redhat.com/

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com