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

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

Hi Marek, Maxime,

On Mon, 7 Jul 2025 12:12:51 +0200
Marek Szyprowski <m.szyprowski@samsung.com> wrote:

> On 07.07.2025 11:21, Maxime Ripard wrote:
> > On Thu, Jul 03, 2025 at 05:50:32PM +0200, Luca Ceresoli wrote:  
> >> On Tue, 1 Jul 2025 16:27:54 +0200
> >> Maxime Ripard <mripard@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@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
> >> $  
> > Ah, you're right, sorry for the noise.
> >  
> >> So I think this patch should be taken as it fixes a regression. Do you
> >> agree on this?  
> > As far as I know, that commit only exists in drm-misc-next. Also, it
> > should have a Fixes tag.  
> 
> I wasn't sure which commit should be listed as Fixes tag in this case. 
> The mentioned 9c399719cfb9 ("drm: convert many bridge drivers from 
> devm_kzalloc() to devm_drm_bridge_alloc() API")?

Despite being somewhat orthogonal, I think it should be commit
a7748dd127ea ("drm/bridge: get/put the bridge reference in
drm_bridge_add/remove()") just because it is the very first commit in
the drm-misc-next history introducing a get/put pair, and thus
triggering the refcount warning.

I'm applying with that added.

Thanks both for the discussion.

Luca

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