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

Message ID d6bb7a8900d9a701ae13a3d0fa2c34b01089038f.1519815150.git.jsarha@ti.com
State New
Headers show
Series
  • drm/panel: Add device link in drm_panel_attach()
Related show

Commit Message

Jyri Sarha Feb. 28, 2018, 11:09 a.m.
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

Eric Anholt Feb. 28, 2018, 6:14 p.m. | #1
Jyri Sarha <jsarha@ti.com> writes:

> 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.


I think this paragraph wants to be:

The device_link is removed when drm_panel_detach() is called. The
drm_panel_detach() should be called by the consumer DRM driver, not the
panel driver, otherwise both drivers are racing to delete the same link.

Other than that, these patches are:

Reviewed-by: Eric Anholt <eric@anholt.net>


(though you'll probably want to wait a bit for Thierry to look at them
too)
Lukas Wunner Feb. 28, 2018, 7:32 p.m. | #2
On Wed, Feb 28, 2018 at 01:09:30PM +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.

AFAICS drm_panel_detach() is always called by the DRM drivers, so it's
confusing that you're saying here it should be called by the panel
drivers.  That would also seem to be asymmetric since drm_panel_attach()
is called by the DRM drivers.


> --- 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;

If you ditch the device_link_del() and use DL_FLAG_AUTOREMOVE only,
you don't need to save the pointer to struct device_link.

Thanks,

Lukas
Jyri Sarha March 1, 2018, 5:53 p.m. | #3
On 28/02/18 20:14, Eric Anholt wrote:
> Jyri Sarha <jsarha@ti.com> writes:
> 
>> 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.
> 
> I think this paragraph wants to be:
> 
> The device_link is removed when drm_panel_detach() is called. The
> drm_panel_detach() should be called by the consumer DRM driver, not the
> panel driver, otherwise both drivers are racing to delete the same link.
> 
> Other than that, these patches are:
> 
> Reviewed-by: Eric Anholt <eric@anholt.net>
> 

Thanks second paragraph was indeed completely wrong. And the first -
especially the second sentence - does not make too much sense either,
but it anyway need rephrasing if I drop DL_FLAG_AUTOREMOVE.

> (though you'll probably want to wait a bit for Thierry to look at them
> too)
>

Patch

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 308d442..afa8337 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 to %s\n",
+			dev_name(connector->dev->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;