Re: [PATCH 2/2] drm/panel: Add device_link from panel device to drm device

[Jyri Sarha <jsarha@ti.com>]

Sorry Lukas, for forgetting these comments.

I'll take these comments to this thread as the other thread is a dead
end. I think the drm_panel_attach() (in this patch) is a better place to
add the device link than panel_bridge_attach() (the other patch).

On 21/02/18 09:52, Lukas Wunner wrote:
> On Wed, Feb 21, 2018 at 12:21:50AM +0200, Jyri Sarha wrote:
>> @@ -94,6 +114,8 @@ static void panel_bridge_detach(struct drm_bridge
*bridge)
>>  	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
>>
>>  	drm_panel_detach(panel_bridge->panel);
>> +
>> +	device_link_del(panel_bridge->link);
>
> No, you've set the DL_FLAG_AUTOREMOVE flag, so you'll end up removing
> the link twice, which oopses.
>
> It's either DL_FLAG_AUTOREMOVE or device_link_del(), not both.
>

Oh yes. I'll drop the device_link_del(). Now reading the code more
carefully, the link del from the driver is obviously redundant. However,
it does not cause a crash if the link is only deleted from the consumer
size.

There is still one argument for having the link del in panel detach
however: If some drm device would behave more dynamically, and could
detach a panel on the fly but stay operational still, then it would be
correct to call link del from the drm driver. But if we ever have such a
device, we can solve that issue separately.

>
>> +static int panel_bridge_link_to_master(struct panel_bridge
*panel_bridge)
>> +{
>> +	struct device *mdev = panel_bridge->bridge.dev->dev;
>> +	struct device *pdev = panel_bridge->panel->dev;
>> +	u32 flags = DL_FLAG_AUTOREMOVE;
>> +
>> +	panel_bridge->link = device_link_add(mdev, pdev, flags);
>> +	if (!panel_bridge->link) {
>> +		dev_err(pdev, "failed to link panel %s to %s\n",
>> +			dev_name(pdev), dev_name(mdev));
>
> You're printing two instances of pdev's name in the log message,
> one should be sufficient.
>
Oh yes, I'll drop the panel device name from the format string.

> Also, you've mixed up the order: mdev is the consumer, pdev the
> supplier.
>
Yes, I noticed that myself. The new patch does not have this problem.

> (Bikeshed:  The DL_FLAG_AUTOREMOVE would still fit within 80 chars
> on the line with device_link_add() and the flags variable wouldn't
> have to be declared then.  Your call.)
>

This time I need the flags for having the line under 80 chars.

> Thanks,
>
> Lukas
>
Best regards,
Jyri


On 23/02/18 08:58, Lukas Wunner wrote:
> On Thu, Feb 22, 2018 at 07:46:27PM +0200, Jyri Sarha wrote:
>> Add device_link from panel device (supplier) to drm device (consumer)
>> with DL_FLAG_AUTOREMOVE when drm_panel_attach() is called. Currently
>> the master drm driver is not protected against the attached. The
>> device_link with DL_FLAG_AUTOREMOVE should make sure the drm device is
>> unbound before the panel driver becomes unavailable.
>>
>> The device_link is removed when drm_panel_detach() is called. The
>> drm_panel_detach() should be called by the panel driver it self when
>> it is removed. Otherwise the both driver are racing to delete the same
>> link.
> 
> Unfortunately you haven't addressed any of my comments on the previous
> version of this patch:
> 
> https://www.spinics.net/lists/dri-devel/msg166320.html
> 
> Please respin with my comments addressed.  Thanks,
> 
> Lukas
> 


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel