[PATCH] Revert "drm/panel: Add device_link from panel device to DRM device"

[PATCH] Revert "drm/panel: Add device_link from panel device to DRM device"

[Linus Walleij]

This reverts commit 0c08754b59da5557532d946599854e6df28edc22.

commit 0c08754b59da
("drm/panel: Add device_link from panel device to DRM device")
creates a circular dependency under these circumstances:

1. The panel depends on dsi-host because it is MIPI-DSI child
   device.
2. dsi-host depends on the drm parent device (connector->dev->dev)
   this should be allowed.
3. drm parent dev (connector->dev->dev) depends on the panel
   after this patch.

This makes the dependency circular and while it appears it
does not affect any in-tree drivers (they do not seem to have
dsi hosts depending on the same parent device) this does not
seem right.

As noted in a response from Andrzej Hajda, the intent is
likely to make the panel dependent on the DRM device
(connector->dev) not its parent. But we have no way of
doing that since the DRM device doesn't contain any
struct device on its own (arguably it should).

Revert this until a proper approach is figured out.

Cc: Jyri Sarha <jsarha@ti.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/drm_panel.c | 10 ----------
 include/drm/drm_panel.h     |  1 -
 2 files changed, 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index b902361dee6e..1d9a9d2fe0e0 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -24,7 +24,6 @@
 #include <linux/err.h>
 #include <linux/module.h>
 
-#include <drm/drm_device.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_panel.h>
 
@@ -105,13 +104,6 @@ int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
 	if (panel->connector)
 		return -EBUSY;
 
-	panel->link = device_link_add(connector->dev->dev, panel->dev, 0);
-	if (!panel->link) {
-		dev_err(panel->dev, "failed to link panel to %s\n",
-			dev_name(connector->dev->dev));
-		return -EINVAL;
-	}
-
 	panel->connector = connector;
 	panel->drm = connector->dev;
 
@@ -133,8 +125,6 @@ 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 582a0ec0aa70..777814755fa6 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -89,7 +89,6 @@ struct drm_panel {
 	struct drm_device *drm;
 	struct drm_connector *connector;
 	struct device *dev;
-	struct device_link *link;
 
 	const struct drm_panel_funcs *funcs;
 
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] Revert "drm/panel: Add device_link from panel device to DRM device"

[Sean Paul]

[actually Cc Eric and Andrzej]

On Thu, Sep 27, 2018 at 02:41:30PM +0200, Linus Walleij wrote:
> This reverts commit 0c08754b59da5557532d946599854e6df28edc22.
> 
> commit 0c08754b59da
> ("drm/panel: Add device_link from panel device to DRM device")
> creates a circular dependency under these circumstances:
> 
> 1. The panel depends on dsi-host because it is MIPI-DSI child
>    device.
> 2. dsi-host depends on the drm parent device (connector->dev->dev)
>    this should be allowed.
> 3. drm parent dev (connector->dev->dev) depends on the panel
>    after this patch.
> 
> This makes the dependency circular and while it appears it
> does not affect any in-tree drivers (they do not seem to have
> dsi hosts depending on the same parent device) this does not
> seem right.

Hey Linus,
I'm trying to wrap my head around this :-)

I just read through the original patch thread. It doesn't seem like this was
introduced to fix a bug? Will reverting this cause regressions on in-tree
drivers?

What's different in your in-development driver that's causing you to hit
this?

Thanks!

Sean

> 
> As noted in a response from Andrzej Hajda, the intent is
> likely to make the panel dependent on the DRM device
> (connector->dev) not its parent. But we have no way of
> doing that since the DRM device doesn't contain any
> struct device on its own (arguably it should).
> 
> Revert this until a proper approach is figured out.
> 
> Cc: Jyri Sarha <jsarha@ti.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpu/drm/drm_panel.c | 10 ----------
>  include/drm/drm_panel.h     |  1 -
>  2 files changed, 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index b902361dee6e..1d9a9d2fe0e0 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -24,7 +24,6 @@
>  #include <linux/err.h>
>  #include <linux/module.h>
>  
> -#include <drm/drm_device.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_panel.h>
>  
> @@ -105,13 +104,6 @@ int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
>  	if (panel->connector)
>  		return -EBUSY;
>  
> -	panel->link = device_link_add(connector->dev->dev, panel->dev, 0);
> -	if (!panel->link) {
> -		dev_err(panel->dev, "failed to link panel to %s\n",
> -			dev_name(connector->dev->dev));
> -		return -EINVAL;
> -	}
> -
>  	panel->connector = connector;
>  	panel->drm = connector->dev;
>  
> @@ -133,8 +125,6 @@ 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 582a0ec0aa70..777814755fa6 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -89,7 +89,6 @@ struct drm_panel {
>  	struct drm_device *drm;
>  	struct drm_connector *connector;
>  	struct device *dev;
> -	struct device_link *link;
>  
>  	const struct drm_panel_funcs *funcs;
>  
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] Revert "drm/panel: Add device_link from panel device to DRM device"

[Andrzej Hajda]

On 27.09.2018 15:50, Sean Paul wrote:
> [actually Cc Eric and Andrzej]
>
> On Thu, Sep 27, 2018 at 02:41:30PM +0200, Linus Walleij wrote:
>> This reverts commit 0c08754b59da5557532d946599854e6df28edc22.
>>
>> commit 0c08754b59da
>> ("drm/panel: Add device_link from panel device to DRM device")
>> creates a circular dependency under these circumstances:
>>
>> 1. The panel depends on dsi-host because it is MIPI-DSI child
>>    device.
>> 2. dsi-host depends on the drm parent device (connector->dev->dev)
>>    this should be allowed.
>> 3. drm parent dev (connector->dev->dev) depends on the panel
>>    after this patch.
>>
>> This makes the dependency circular and while it appears it
>> does not affect any in-tree drivers (they do not seem to have
>> dsi hosts depending on the same parent device) this does not
>> seem right.
> Hey Linus,
> I'm trying to wrap my head around this :-)
>
> I just read through the original patch thread. It doesn't seem like this was
> introduced to fix a bug? Will reverting this cause regressions on in-tree
> drivers?

As I wrote in original patch thread, the original patch breaks platforms
with exynos-dsi encoder.
Quite detailed explanation in the original thread [1], 1st response to
the patch.
Apparently my response was ignored :)

[1]: https://patchwork.freedesktop.org/patch/218878/

Regards
Andrzej

>
> What's different in your in-development driver that's causing you to hit
> this?
>
> Thanks!
>
> Sean
>
>> As noted in a response from Andrzej Hajda, the intent is
>> likely to make the panel dependent on the DRM device
>> (connector->dev) not its parent. But we have no way of
>> doing that since the DRM device doesn't contain any
>> struct device on its own (arguably it should).
>>
>> Revert this until a proper approach is figured out.
>>
>> Cc: Jyri Sarha <jsarha@ti.com>
>> Cc: Eric Anholt <eric@anholt.net>
>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>>  drivers/gpu/drm/drm_panel.c | 10 ----------
>>  include/drm/drm_panel.h     |  1 -
>>  2 files changed, 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
>> index b902361dee6e..1d9a9d2fe0e0 100644
>> --- a/drivers/gpu/drm/drm_panel.c
>> +++ b/drivers/gpu/drm/drm_panel.c
>> @@ -24,7 +24,6 @@
>>  #include <linux/err.h>
>>  #include <linux/module.h>
>>  
>> -#include <drm/drm_device.h>
>>  #include <drm/drm_crtc.h>
>>  #include <drm/drm_panel.h>
>>  
>> @@ -105,13 +104,6 @@ int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
>>  	if (panel->connector)
>>  		return -EBUSY;
>>  
>> -	panel->link = device_link_add(connector->dev->dev, panel->dev, 0);
>> -	if (!panel->link) {
>> -		dev_err(panel->dev, "failed to link panel to %s\n",
>> -			dev_name(connector->dev->dev));
>> -		return -EINVAL;
>> -	}
>> -
>>  	panel->connector = connector;
>>  	panel->drm = connector->dev;
>>  
>> @@ -133,8 +125,6 @@ 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 582a0ec0aa70..777814755fa6 100644
>> --- a/include/drm/drm_panel.h
>> +++ b/include/drm/drm_panel.h
>> @@ -89,7 +89,6 @@ struct drm_panel {
>>  	struct drm_device *drm;
>>  	struct drm_connector *connector;
>>  	struct device *dev;
>> -	struct device_link *link;
>>  
>>  	const struct drm_panel_funcs *funcs;
>>  
>> -- 
>> 2.17.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] Revert "drm/panel: Add device_link from panel device to DRM device"

[Sean Paul]

On Thu, Sep 27, 2018 at 04:25:56PM +0200, Andrzej Hajda wrote:
> On 27.09.2018 15:50, Sean Paul wrote:
> > [actually Cc Eric and Andrzej]
> >
> > On Thu, Sep 27, 2018 at 02:41:30PM +0200, Linus Walleij wrote:
> >> This reverts commit 0c08754b59da5557532d946599854e6df28edc22.
> >>
> >> commit 0c08754b59da
> >> ("drm/panel: Add device_link from panel device to DRM device")
> >> creates a circular dependency under these circumstances:
> >>
> >> 1. The panel depends on dsi-host because it is MIPI-DSI child
> >>    device.
> >> 2. dsi-host depends on the drm parent device (connector->dev->dev)
> >>    this should be allowed.
> >> 3. drm parent dev (connector->dev->dev) depends on the panel
> >>    after this patch.
> >>
> >> This makes the dependency circular and while it appears it
> >> does not affect any in-tree drivers (they do not seem to have
> >> dsi hosts depending on the same parent device) this does not
> >> seem right.
> > Hey Linus,
> > I'm trying to wrap my head around this :-)
> >
> > I just read through the original patch thread. It doesn't seem like this was
> > introduced to fix a bug? Will reverting this cause regressions on in-tree
> > drivers?
> 
> As I wrote in original patch thread, the original patch breaks platforms
> with exynos-dsi encoder.
> Quite detailed explanation in the original thread [1], 1st response to
> the patch.
> Apparently my response was ignored :)

Thanks Andrzej,
I've pushed this to drm-misc-fixes. It'll go out this week or next.

Thanks,

Sean

> 
> [1]: https://patchwork.freedesktop.org/patch/218878/
> 
> Regards
> Andrzej
> 
> >
> > What's different in your in-development driver that's causing you to hit
> > this?
> >
> > Thanks!
> >
> > Sean
> >
> >> As noted in a response from Andrzej Hajda, the intent is
> >> likely to make the panel dependent on the DRM device
> >> (connector->dev) not its parent. But we have no way of
> >> doing that since the DRM device doesn't contain any
> >> struct device on its own (arguably it should).
> >>
> >> Revert this until a proper approach is figured out.
> >>
> >> Cc: Jyri Sarha <jsarha@ti.com>
> >> Cc: Eric Anholt <eric@anholt.net>
> >> Cc: Andrzej Hajda <a.hajda@samsung.com>
> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> >> ---
> >>  drivers/gpu/drm/drm_panel.c | 10 ----------
> >>  include/drm/drm_panel.h     |  1 -
> >>  2 files changed, 11 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> >> index b902361dee6e..1d9a9d2fe0e0 100644
> >> --- a/drivers/gpu/drm/drm_panel.c
> >> +++ b/drivers/gpu/drm/drm_panel.c
> >> @@ -24,7 +24,6 @@
> >>  #include <linux/err.h>
> >>  #include <linux/module.h>
> >>  
> >> -#include <drm/drm_device.h>
> >>  #include <drm/drm_crtc.h>
> >>  #include <drm/drm_panel.h>
> >>  
> >> @@ -105,13 +104,6 @@ int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
> >>  	if (panel->connector)
> >>  		return -EBUSY;
> >>  
> >> -	panel->link = device_link_add(connector->dev->dev, panel->dev, 0);
> >> -	if (!panel->link) {
> >> -		dev_err(panel->dev, "failed to link panel to %s\n",
> >> -			dev_name(connector->dev->dev));
> >> -		return -EINVAL;
> >> -	}
> >> -
> >>  	panel->connector = connector;
> >>  	panel->drm = connector->dev;
> >>  
> >> @@ -133,8 +125,6 @@ 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 582a0ec0aa70..777814755fa6 100644
> >> --- a/include/drm/drm_panel.h
> >> +++ b/include/drm/drm_panel.h
> >> @@ -89,7 +89,6 @@ struct drm_panel {
> >>  	struct drm_device *drm;
> >>  	struct drm_connector *connector;
> >>  	struct device *dev;
> >> -	struct device_link *link;
> >>  
> >>  	const struct drm_panel_funcs *funcs;
> >>  
> >> -- 
> >> 2.17.1
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] Revert "drm/panel: Add device_link from panel device to DRM device"

[Linus Walleij]

On Thu, Sep 27, 2018 at 3:50 PM Sean Paul <sean@poorly.run> wrote:

> What's different in your in-development driver that's causing you to hit
> this?

I think Andrzej answered it well, but I couldn't see what I did
different from the Exynos driver, so I was puzzled that it would
still work, but as it turns out that one breaks too.

As far as I can tell, the proper fix is more complex and require
embedding a struct device into struct drm_device like most
subsystems do and create a link to that device instead of
parent (dev->dev), because the parent device may be
the parent of several subdevices, not just the drm_device,
but also (in my case at least) the DSI devices. A typical case
would be if the DSI host is in the same address range as the
display controller (then it is usually represented by the same
device).

On other platforms the parent device may just be parent for
the drm_device but we can't just assume that.

Yours,
Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel