diff mbox

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

Message ID acfd3eb48a180c88e7463798505aadacc004f28f.1519319383.git.jsarha@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jyri Sarha Feb. 22, 2018, 5:46 p.m. UTC
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.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/drm_panel.c | 12 ++++++++++++
 include/drm/drm_panel.h     |  1 +
 2 files changed, 13 insertions(+)

Comments

Lukas Wunner Feb. 23, 2018, 6:58 a.m. UTC | #1
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
Jyri Sarha Feb. 23, 2018, 10:43 a.m. UTC | #2
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
>
Jyri Sarha Feb. 23, 2018, 1:10 p.m. UTC | #3
On 23/02/18 12:43, Jyri Sarha wrote:
> 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.
> 

Actually I am little by little becoming convinced that leaving the
device_link_del() there in drm_panel_detach() would be the right thing
to do. Assuming that my assessment (by reading the code and testing
different approaches) is correct that there is no harm in deleting the
link from the consumer side when the consumer is removed.

I'll left this be over the weekend and see if there are any more
comments by Monday.

>>
>>> +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
>>
> 
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 308d442..d9452e98 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -24,6 +24,7 @@ 
 #include <linux/err.h>
 #include <linux/module.h>
 
+#include <drm/drm_device.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_panel.h>
 
@@ -98,9 +99,18 @@  EXPORT_SYMBOL(drm_panel_remove);
  */
 int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
 {
+	u32 flags = DL_FLAG_AUTOREMOVE;
+
 	if (panel->connector)
 		return -EBUSY;
 
+	panel->link = device_link_add(connector->dev->dev, panel->dev, flags);
+	if (!panel->link) {
+		dev_err(panel->dev, "failed to link panel %s to %s\n",
+			dev_name(connector->dev->dev), dev_name(panel->dev));
+		return -EINVAL;
+	}
+
 	panel->connector = connector;
 	panel->drm = connector->dev;
 
@@ -119,6 +129,8 @@  EXPORT_SYMBOL(drm_panel_attach);
  */
 int drm_panel_detach(struct drm_panel *panel)
 {
+	device_link_del(panel->link);
+
 	panel->connector = NULL;
 	panel->drm = NULL;
 
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 14ac240..26a1b5f 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -89,6 +89,7 @@  struct drm_panel {
 	struct drm_device *drm;
 	struct drm_connector *connector;
 	struct device *dev;
+	struct device_link *link;
 
 	const struct drm_panel_funcs *funcs;