Re: [PATCH v2 10/26] drm/bridge: panel: provide an owner .odev device

[Peter Rosin]

On 2018-05-08 08:51, Jyri Sarha wrote:
> On 05/04/18 16:51, Peter Rosin wrote:
>> It gets rid of an #ifdef and the .of_node member is going away.
>>
>> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/bridge/panel.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
>> index 6d99d4a3beb3..f43d77b5ed20 100644
>> --- a/drivers/gpu/drm/bridge/panel.c
>> +++ b/drivers/gpu/drm/bridge/panel.c
>> @@ -169,10 +169,8 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
>>  	panel_bridge->connector_type = connector_type;
>>  	panel_bridge->panel = panel;
>>  
>> +	panel_bridge->bridge.odev = panel->dev;
> 
> I am afraid this approach will eventually conflict with my lately
> accepted patch[1].

I don't see how? The links are refcounted. So, if there is one link
each for the panel and bridge between the drm device and the panel
device that link will simply get two references. If/when the panel
device then goes away, the drm device will be brought down because
of that link (with two references, but that is irrelevant). When
the drm device is brought down, it will (presumably) bring down the
bridge as well (which will fix the refcount as the bridge link is
killed as part of that).

Or have you done some test and seen an actual problem?

> The panel already has a device pointer of its own, and technically the
> bridge element created here is created by the master drm device itself.

Not always, some bridges also call drm_panel_bridge_add for connecting
its output to either a panel or another bridge.

And by that line of argument, the devm_kzalloc in drm_panel_bridge_add
attaches the bridge memory to the wrong device as well. Maybe that
should be fixed? Seems orthogonal though, but it would introduce a
natural struct device in that function to assign to .odev. I think
the device owning the bridge memory should be the same as the .odev
device of the bridge.

> I suggest assigning odev here to NULL or to master drm device itself.

I'd rather not use NULL, since it is nice to be able to rely on the
.odev being there, and WARN if it isn't.

Cheers,
Peter

> Best regards,
> Jyri
> 
>>  	panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs;
>> -#ifdef CONFIG_OF
>> -	panel_bridge->bridge.of_node = panel->dev->of_node;
>> -#endif
>>  
>>  	drm_bridge_add(&panel_bridge->bridge);
>>  
>>
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2018-April/174559.html
>