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

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

[Michael Walle]

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);
+	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;
-- 
2.39.5

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

[Luca Ceresoli]

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

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

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 1366 bytes --]

Hi,

thanks for taking a look!

> 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.

I was doing that until I've realized that meson/meson_encoder_* is
doing it the way this patch does it.

> 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.

I'm fine with changing it to the wrapper struct. It's your/the
maintainers call :)

-michael

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

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

[Luca Ceresoli]

Hello Jyri, Tomi, Michael,

On Thu, 17 Jul 2025 09:49:44 +0200
"Michael Walle" <mwalle@kernel.org> wrote:

> Hi,
> 
> thanks for taking a look!
> 
> > 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.  
> 
> I was doing that until I've realized that meson/meson_encoder_* is
> doing it the way this patch does it.

Which was done by, er, myself. O:-)

To my excuse, meson was using *_encoder_alloc() but rather
devm_kzalloc() + drm_simple_encoder_init(), and the change was
semi-automated via a coccinelle script, so I didn't fully realize that.

> > 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.  
> 
> I'm fine with changing it to the wrapper struct. It's your/the
> maintainers call :)

I think the driver maintainers opinion is more relevant, but in lack of
one I think we can take the patch as is, given it's already written.

Jyri, Tomi?

Luca

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

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

[Tomi Valkeinen]

Hi,

On 17/07/2025 16:56, Luca Ceresoli wrote:
> Hello Jyri, Tomi, Michael,
> 
> On Thu, 17 Jul 2025 09:49:44 +0200
> "Michael Walle" <mwalle@kernel.org> wrote:
> 
>> Hi,
>>
>> thanks for taking a look!
>>
>>> 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.  
>>
>> I was doing that until I've realized that meson/meson_encoder_* is
>> doing it the way this patch does it.
> 
> Which was done by, er, myself. O:-)
> 
> To my excuse, meson was using *_encoder_alloc() but rather
> devm_kzalloc() + drm_simple_encoder_init(), and the change was
> semi-automated via a coccinelle script, so I didn't fully realize that.
> 
>>> 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.  
>>
>> I'm fine with changing it to the wrapper struct. It's your/the
>> maintainers call :)
> 
> I think the driver maintainers opinion is more relevant, but in lack of
> one I think we can take the patch as is, given it's already written.
> 
> Jyri, Tomi?

I think this is fine, even though I do agree the tidss_encoder.c is very
confusing.

I'll pick this up. I think drm-misc-next-fixes is the correct branch here.

 Tomi

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

[Luca Ceresoli]

Hello Tomi,

On Fri, 18 Jul 2025 14:13:03 +0300
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:

> Hi,
> 
> On 17/07/2025 16:56, Luca Ceresoli wrote:
> > Hello Jyri, Tomi, Michael,
> > 
> > On Thu, 17 Jul 2025 09:49:44 +0200
> > "Michael Walle" <mwalle@kernel.org> wrote:
> >   
> >> Hi,
> >>
> >> thanks for taking a look!
> >>  
> >>> 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.    
> >>
> >> I was doing that until I've realized that meson/meson_encoder_* is
> >> doing it the way this patch does it.  
> > 
> > Which was done by, er, myself. O:-)
> > 
> > To my excuse, meson was using *_encoder_alloc() but rather
> > devm_kzalloc() + drm_simple_encoder_init(), and the change was
> > semi-automated via a coccinelle script, so I didn't fully realize that.
> >   
> >>> 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.    
> >>
> >> I'm fine with changing it to the wrapper struct. It's your/the
> >> maintainers call :)  
> > 
> > I think the driver maintainers opinion is more relevant, but in lack of
> > one I think we can take the patch as is, given it's already written.
> > 
> > Jyri, Tomi?  
> 
> I think this is fine, even though I do agree the tidss_encoder.c is very
> confusing.
> 
> I'll pick this up. I think drm-misc-next-fixes is the correct branch here.

Looks like to correct branch to me.

Thanks for handling this!

Luca

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