[PATCH] drm/bridge: analogix_dp: Use devm_drm_bridge_alloc() API

[Maxime Ripard]

Hi,

On Tue, Jul 08, 2025 at 04:19:15PM +0200, Luca Ceresoli wrote:
> Hello Maxime, all,
> 
> On Mon, 7 Jul 2025 12:59:54 +0200
> Luca Ceresoli <luca.ceresoli at bootlin.com> wrote:
> 
> > On Mon, 7 Jul 2025 11:07:26 +0200
> > Marek Szyprowski <m.szyprowski at samsung.com> wrote:
> > 
> > > On 03.07.2025 17:50, Luca Ceresoli wrote:  
> > > > On Tue, 1 Jul 2025 16:27:54 +0200
> > > > Maxime Ripard <mripard at kernel.org> wrote:
> > > >    
> > > >> On Tue, Jul 01, 2025 at 04:02:19PM +0200, Luca Ceresoli wrote:    
> > > >>> Hello Marek, Maxime,
> > > >>>
> > > >>> thanks Marek for spotting the issue and sending a patch!
> > > >>>
> > > >>> On Mon, 30 Jun 2025 18:44:24 +0200
> > > >>> Maxime Ripard <mripard at kernel.org> wrote:
> > > >>>        
> > > >>>>> @@ -1643,7 +1625,7 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
> > > >>>>>   		return ret;
> > > >>>>>   	}
> > > >>>>>   
> > > >>>>> -	ret = analogix_dp_create_bridge(drm_dev, dp);
> > > >>>>> +	ret = drm_bridge_attach(dp->encoder, &dp->bridge, NULL, 0);
> > > >>>>>   	if (ret) {
> > > >>>>>   		DRM_ERROR("failed to create bridge (%d)\n", ret);
> > > >>>>>   		goto err_unregister_aux;    
> > > >>>> It looks like you don't set bridge->driver_private anymore. Is it on purpose?    
> > > >>> This looks correct to me. In current code, driver_private is used to
> > > >>> hold a pointer to the driver private struct (struct
> > > >>> analogix_dp_device). With devm_drm_bridge_alloc() container_of() is now
> > > >>> enough, no pointer is needed. With the patch applied, driver_private
> > > >>> becomes unused.    
> > > >> Then we should remove it from the structure if it's unused.    
> > > > Makes sense now that struct drm_bridge is meant to be always embedded
> > > > in a driver-private struct. But several drivers are still using it, so
> > > > those would need to be updated beforehand:
> > > >
> > > > $ git grep  -l driver_private -- drivers/gpu/drm/ | wc -l
> > > > 23
> > > > $
> > > >
> > > > So I think this patch should be taken as it fixes a regression. Do you
> > > > agree on this?    
> > > 
> > > Yes, please apply it as a fix :)
> > > 
> > > 
> > > BTW, there are 2 more bridge drivers that need a fix similar to the 
> > > $subject patch:
> > > 
> > > $ git grep "bridge = devm_kzalloc" drivers/gpu
> > > drivers/gpu/drm/sti/sti_hda.c:  bridge = devm_kzalloc(dev, 
> > > sizeof(*bridge), GFP_KERNEL);
> > > drivers/gpu/drm/sti/sti_hdmi.c: bridge = devm_kzalloc(dev, 
> > > sizeof(*bridge), GFP_KERNEL);  
> > 
> > Ouch. My grep logic was probably too clever and missed these obvious
> > ones. I'm taking care of converting these ones later this week as time
> > permits, unless patches are sent before.
> 
> I think I missed those two because I searched for all calls to
> drm_bridge_add(), and converted matching drivers, but these two do now
> call drm_bridge_add() at all.
> 
> I understand this probably works just fine, for non-DT hardware at
> least. However, doesn't this look like wrong, from a DRM bridge API
> point of view?
> 
> Right now I'm preparing patches to convert to devm_drm_bridge_alloc(),
> but what about the following two additions:
> 
>  * add calls to drm_bridge_add/remove() in those drivers

Yep, we definitely need to do that

>  * add a WARN in drm_bridge_attach() if the bridge was not added

We also need to update the documentation to make it more obvious,
because at the moment it's not documented that you need to add a bridge
before attaching it.

And for displaying the warning, I guess we could but I'd like to see the
implementation. If it's anything but trivial, I don't think it's worth it.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 273 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20250709/9d2fddb9/attachment.sig>