Re: [PATCH v4 4/4] drm/bridge: imx8qxp-pixel-link: get/put the next bridge

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

Hello Liu,

On Mon Jan 26, 2026 at 9:06 AM CET, Liu Ying wrote:
> Hi Luca,
>
> On Wed, Jan 07, 2026 at 10:56:29AM +0100, Luca Ceresoli wrote:
>> This driver obtains a bridge pointer from of_drm_find_bridge() in the probe
>> function and stores it until driver removal. of_drm_find_bridge() is
>> deprecated. Move to of_drm_find_and_get_bridge() for the bridge to be
>> refcounted and use bridge->next_bridge to put the reference on
>> deallocation.
>>
>> This needs to be handled in various steps:
>>
>>  * the bridge returned of_drm_get_bridge() is stored in the local temporary
>>    variable next_bridge whose scope is the for loop, so a cleanup action is
>>    enough
>>  * the value of next_bridge is copied into selected_bridge, potentially
>>    more than once, so a cleanup action at function scope plus a
>>    drm_bridge_put() in case of reassignment are enough
>>  * on successful return selected_bridge is stored in bridge->next_bridge,
>>    which ensures it is put when the bridge is deallocated
>>
>> Reviewed-by: Maxime Ripard <mripard@kernel.org>
>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Thanks for having found the time to go into the details and provide a
careful review of this series!

>> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
>> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
>> @@ -23,7 +23,6 @@
>>
>>  struct imx8qxp_pixel_link {
>>  	struct drm_bridge bridge;
>> -	struct drm_bridge *next_bridge;
>>  	struct device *dev;
>>  	struct imx_sc_ipc *ipc_handle;
>>  	u8 stream_id;
>> @@ -140,7 +139,7 @@ static int imx8qxp_pixel_link_bridge_attach(struct drm_bridge *bridge,
>>  	}
>>
>>  	return drm_bridge_attach(encoder,
>> -				 pl->next_bridge, bridge,
>> +				 pl->bridge.next_bridge, bridge,
>>  				 DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>  }
>>
>> @@ -260,7 +259,7 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>  {
>>  	struct device_node *np = pl->dev->of_node;
>>  	struct device_node *port;
>> -	struct drm_bridge *selected_bridge = NULL;
>> +	struct drm_bridge *selected_bridge __free(drm_bridge_put) = NULL;
>>  	u32 port_id;
>>  	bool found_port = false;
>>  	int reg;
>> @@ -297,7 +296,8 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>  			continue;
>>  		}
>>
>> -		struct drm_bridge *next_bridge = of_drm_find_bridge(remote);
>> +		struct drm_bridge *next_bridge __free(drm_bridge_put) =
>> +			of_drm_find_and_get_bridge(remote);
>>  		if (!next_bridge)
>>  			return -EPROBE_DEFER;
>>
>> @@ -305,12 +305,14 @@ static int imx8qxp_pixel_link_find_next_bridge(struct imx8qxp_pixel_link *pl)
>>  		 * Select the next bridge with companion PXL2DPI if
>>  		 * present, otherwise default to the first bridge
>>  		 */
>> -		if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi"))
>> -			selected_bridge = next_bridge;
>> +		if (!selected_bridge || of_property_present(remote, "fsl,companion-pxl2dpi")) {
>> +			drm_bridge_put(selected_bridge);
>> +			selected_bridge = drm_bridge_get(next_bridge);
>
> Considering selecting the first bridge without the companion pxl2dpi,
> there would be a superfluous refcount for the selected bridge:
>
> 1) of_drm_find_and_get_bridge: refcount = 1
> 2) drm_bridge_put: noop, since selected_bridge is NULL, refcount = 1
> 3) drm_bridge_get: refcount = 2
> 4) drm_bridge_put(__free): refcount = 1
> 5) drm_bridge_get: for the pl->bridge.next_bridge, refcount = 2

Here you are missing one put. There are two drm_bridge_put(__free), one for
next_bridge and one for selected_bridge. So your list should rather be:

1) next_bridge = of_drm_find_and_get_bridge: refcount = 1
2) drm_bridge_put(selected_bridge): noop, since selected_bridge is NULL, refcount = 1
3) selected_bridge = drm_bridge_get: refcount = 2
4) drm_bridge_put(next_bridge) [__free at loop scope end]: refcount = 1
5) pl->bridge.next_bridge = drm_bridge_get(), refcount = 2
6) drm_bridge_put(selected_bridge) [__free at function scope end]: refcount = 1

The idea is that for each pointer (which is a reference) we get a reference
(refcount++) when the pointer is set and put the reference when that same
pointer goes out of scope or is reset to NULL. "the pointer is set" can be
either of_drm_find_and_get_bridge() or an assignment, as each of these
operations creates another reference (pointer) to the same bridge.

Does it look correct?

> I think the below snippet would be the right thing to do:
> -8<-
> {
> 	...
>
> 	struct drm_bridge *next_bridge __free(drm_bridge_put) =
> 		of_drm_find_and_get_bridge(remote);
>   		if (!next_bridge)
>   			return -EPROBE_DEFER;
>
> 	/*
> 	 * Select the next bridge with companion PXL2DPI if
> 	 * present, otherwise default to the first bridge
> 	 */
> 	if (!selected_bridge)
> 		selected_bridge = drm_bridge_get(next_bridge);
>
> 	if (of_property_present(remote, "fsl,companion-pxl2dpi")) {
> 		if (selected_bridge)
> 			drm_bridge_put(selected_bridge);
>
> 		selected_bridge = drm_bridge_get(next_bridge);
> 	}
> }

Your version of the code looks OK as well so far, but totally equivalent to
what my patch proposes.

Do you think splitting the if() into two if()s is clearer? Would you like
me to change this?

> ...
> pl->bridge.next_bridge = selected_bridge;

Based on the logic above the drm_bridge_get() is still needed here (both
with the single if() or the split if()s) because at function exit the
selected_bridge reference will be put.

Luca

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