[PATCH v3 0/2] drm/panel: Add device link in drm_panel_attach()

[PATCH v3 0/2] drm/panel: Add device link in drm_panel_attach()

[Jyri Sarha]

I dropped this work at the end of the previous merge window, since I
had more urgent thing to do. It looks like topic has been raised
again[1], so I'll respin this now.

The second version of these patches can be found here:
https://lists.freedesktop.org/archives/dri-devel/2018-February/167628.html

Changes since v2:
- Rebased both patches on top of the latest drm-next
- "drm/panel: Remove drm_panel_detach() calls from all panel drives"
 - Updated the kernel doc for drm_panel_detach()
- "drm/panel: Add device_link from panel device to drm device"
 - Fixed the logically wrong second paragraph in the commit message
   according to Eric Anholt's suggestion and added his reviewed by

The first patch could be squashed to second, but kept is separate
since I think it is correct even without the second patch.

With these patches unbinding a panel driver in use does not cause
nasty backtraces and corrupted drm core structures, but instead it
cleanly unbinds the drm master device at the same time.

The only down side (currently[1]) is that the drm device does not reprobe
if the panel driver is bound again, but everything should work if the
drm master driver is bound manually.

Best regards,
Jyri

[1] https://lists.freedesktop.org/archives/dri-devel/2018-April/174083.html
[2] https://lists.freedesktop.org/archives/dri-devel/2018-February/166907.html

Jyri Sarha (2):
  drm/panel: Remove drm_panel_detach() calls from all panel drives
  drm/panel: Add device_link from panel device to drm device

 drivers/gpu/drm/drm_panel.c                          | 14 ++++++++++++++
 drivers/gpu/drm/panel/panel-innolux-p079zca.c        |  1 -
 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c       |  1 -
 drivers/gpu/drm/panel/panel-lvds.c                   |  1 -
 drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c |  1 -
 drivers/gpu/drm/panel/panel-seiko-43wvf1g.c          |  1 -
 drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c      |  1 -
 drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c      |  1 -
 drivers/gpu/drm/panel/panel-simple.c                 |  1 -
 drivers/gpu/drm/panel/panel-sitronix-st7789v.c       |  1 -
 include/drm/drm_panel.h                              |  1 +
 11 files changed, 15 insertions(+), 9 deletions(-)

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

[PATCH v3 1/2] drm/panel: Remove drm_panel_detach() calls from all panel drives

[Jyri Sarha]

Remove all drm_panel_detach() calls from all panel drives update the
kernel doc for drm_panel_detach().

Setting the connector and drm to NULL when the drm panel device is
going away hardly serves any purpose. Usually the the whole memory
structure is freed right after the remove call. However, calling the
detach function from the master drm device, and setting the connector
pointer to NULL, has the logic of marking the panel again as available
for another drm master to attach. The usual situation would be the same
drm master device binding again.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/drm_panel.c                          | 4 ++++
 drivers/gpu/drm/panel/panel-innolux-p079zca.c        | 1 -
 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c       | 1 -
 drivers/gpu/drm/panel/panel-lvds.c                   | 1 -
 drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c | 1 -
 drivers/gpu/drm/panel/panel-seiko-43wvf1g.c          | 1 -
 drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c      | 1 -
 drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c      | 1 -
 drivers/gpu/drm/panel/panel-simple.c                 | 1 -
 drivers/gpu/drm/panel/panel-sitronix-st7789v.c       | 1 -
 10 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 308d442..29d2c74 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -115,6 +115,10 @@ EXPORT_SYMBOL(drm_panel_attach);
  * Detaches a panel from the connector it is attached to. If a panel is not
  * attached to any connector this is effectively a no-op.
  *
+ * NOTE: This function should not be called by the panel device
+ *       itself. It is only for the drm device that called the
+ *       drm_panel_attach().
+ *
  * Return: 0 on success or a negative error code on failure.
  */
 int drm_panel_detach(struct drm_panel *panel)
diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
index 57df39b..bb53e08 100644
--- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
+++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
@@ -292,7 +292,6 @@ static int innolux_panel_remove(struct mipi_dsi_device *dsi)
 		DRM_DEV_ERROR(&dsi->dev, "failed to detach from DSI host: %d\n",
 			      err);
 
-	drm_panel_detach(&innolux->base);
 	innolux_panel_del(innolux);
 
 	return 0;
diff --git a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
index 0a94ab7..99caa78 100644
--- a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
+++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
@@ -500,7 +500,6 @@ static int jdi_panel_remove(struct mipi_dsi_device *dsi)
 		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n",
 			ret);
 
-	drm_panel_detach(&jdi->base);
 	jdi_panel_del(jdi);
 
 	return 0;
diff --git a/drivers/gpu/drm/panel/panel-lvds.c b/drivers/gpu/drm/panel/panel-lvds.c
index 5185819..8a16878 100644
--- a/drivers/gpu/drm/panel/panel-lvds.c
+++ b/drivers/gpu/drm/panel/panel-lvds.c
@@ -282,7 +282,6 @@ static int panel_lvds_remove(struct platform_device *pdev)
 {
 	struct panel_lvds *lvds = dev_get_drvdata(&pdev->dev);
 
-	drm_panel_detach(&lvds->panel);
 	drm_panel_remove(&lvds->panel);
 
 	panel_lvds_disable(&lvds->panel);
diff --git a/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c b/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
index 74a8061..cb4dfb9 100644
--- a/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
+++ b/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
@@ -299,7 +299,6 @@ static int wuxga_nt_panel_remove(struct mipi_dsi_device *dsi)
 	if (ret < 0)
 		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
 
-	drm_panel_detach(&wuxga_nt->base);
 	wuxga_nt_panel_del(wuxga_nt);
 
 	return 0;
diff --git a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
index 71c09ed..75f9253 100644
--- a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
+++ b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
@@ -292,7 +292,6 @@ static int seiko_panel_remove(struct platform_device *pdev)
 {
 	struct seiko_panel *panel = dev_get_drvdata(&pdev->dev);
 
-	drm_panel_detach(&panel->base);
 	drm_panel_remove(&panel->base);
 
 	seiko_panel_disable(&panel->base);
diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
index 6bf8730..02fc0f5 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -418,7 +418,6 @@ static int sharp_panel_remove(struct mipi_dsi_device *dsi)
 	if (err < 0)
 		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
 
-	drm_panel_detach(&sharp->base);
 	sharp_panel_del(sharp);
 
 	return 0;
diff --git a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
index 494aa9b..e5cae00 100644
--- a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
@@ -327,7 +327,6 @@ static int sharp_nt_panel_remove(struct mipi_dsi_device *dsi)
 	if (ret < 0)
 		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
 
-	drm_panel_detach(&sharp_nt->base);
 	sharp_nt_panel_del(sharp_nt);
 
 	return 0;
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index f2d96611..9388acf 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -364,7 +364,6 @@ static int panel_simple_remove(struct device *dev)
 {
 	struct panel_simple *panel = dev_get_drvdata(dev);
 
-	drm_panel_detach(&panel->base);
 	drm_panel_remove(&panel->base);
 
 	panel_simple_disable(&panel->base);
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 358c64e..74284e5 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -419,7 +419,6 @@ static int st7789v_remove(struct spi_device *spi)
 {
 	struct st7789v *ctx = spi_get_drvdata(spi);
 
-	drm_panel_detach(&ctx->panel);
 	drm_panel_remove(&ctx->panel);
 
 	if (ctx->backlight)
-- 
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

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

[Jyri Sarha]

Add device_link from panel device (supplier) to drm device (consumer)
when drm_panel_attach() is called. This patch should protect the
master drm driver if an attached panel driver unbinds while it is in
use. The device_link 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 consumer DRM driver, not the
panel driver, otherwise both drivers are racing to delete the same link.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/drm_panel.c | 10 ++++++++++
 include/drm/drm_panel.h     |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 29d2c74..7474045 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>
 
@@ -101,6 +102,13 @@ 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;
 
@@ -123,6 +131,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;
 
-- 
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

Re: [PATCH v3 1/2] drm/panel: Remove drm_panel_detach() calls from all panel drives

[Daniel Vetter]

On Tue, Apr 24, 2018 at 06:51:07PM +0300, Jyri Sarha wrote:
> Remove all drm_panel_detach() calls from all panel drives update the
> kernel doc for drm_panel_detach().
> 
> Setting the connector and drm to NULL when the drm panel device is
> going away hardly serves any purpose. Usually the the whole memory
> structure is freed right after the remove call. However, calling the
> detach function from the master drm device, and setting the connector
> pointer to NULL, has the logic of marking the panel again as available
> for another drm master to attach. The usual situation would be the same
> drm master device binding again.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>

Yeah this makes tons of sense, no panel driver calls attach either.

> ---
>  drivers/gpu/drm/drm_panel.c                          | 4 ++++
>  drivers/gpu/drm/panel/panel-innolux-p079zca.c        | 1 -
>  drivers/gpu/drm/panel/panel-jdi-lt070me05000.c       | 1 -
>  drivers/gpu/drm/panel/panel-lvds.c                   | 1 -
>  drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c | 1 -
>  drivers/gpu/drm/panel/panel-seiko-43wvf1g.c          | 1 -
>  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c      | 1 -
>  drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c      | 1 -
>  drivers/gpu/drm/panel/panel-simple.c                 | 1 -
>  drivers/gpu/drm/panel/panel-sitronix-st7789v.c       | 1 -
>  10 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index 308d442..29d2c74 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -115,6 +115,10 @@ EXPORT_SYMBOL(drm_panel_attach);
>   * Detaches a panel from the connector it is attached to. If a panel is not
>   * attached to any connector this is effectively a no-op.
>   *
> + * NOTE: This function should not be called by the panel device
> + *       itself. It is only for the drm device that called the
> + *       drm_panel_attach().

Does this result in pretty output when building the docs with

$ make htmldocs

Also might be good to add something like

 * When unloading, the driver should detach from the panel by calling
 * drm_panel_detach().

to the kerneldoc of drm_panel_attach.

Either way:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> + *
>   * Return: 0 on success or a negative error code on failure.
>   */
>  int drm_panel_detach(struct drm_panel *panel)
> diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> index 57df39b..bb53e08 100644
> --- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> +++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> @@ -292,7 +292,6 @@ static int innolux_panel_remove(struct mipi_dsi_device *dsi)
>  		DRM_DEV_ERROR(&dsi->dev, "failed to detach from DSI host: %d\n",
>  			      err);
>  
> -	drm_panel_detach(&innolux->base);
>  	innolux_panel_del(innolux);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
> index 0a94ab7..99caa78 100644
> --- a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
> +++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
> @@ -500,7 +500,6 @@ static int jdi_panel_remove(struct mipi_dsi_device *dsi)
>  		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n",
>  			ret);
>  
> -	drm_panel_detach(&jdi->base);
>  	jdi_panel_del(jdi);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/panel/panel-lvds.c b/drivers/gpu/drm/panel/panel-lvds.c
> index 5185819..8a16878 100644
> --- a/drivers/gpu/drm/panel/panel-lvds.c
> +++ b/drivers/gpu/drm/panel/panel-lvds.c
> @@ -282,7 +282,6 @@ static int panel_lvds_remove(struct platform_device *pdev)
>  {
>  	struct panel_lvds *lvds = dev_get_drvdata(&pdev->dev);
>  
> -	drm_panel_detach(&lvds->panel);
>  	drm_panel_remove(&lvds->panel);
>  
>  	panel_lvds_disable(&lvds->panel);
> diff --git a/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c b/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
> index 74a8061..cb4dfb9 100644
> --- a/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
> +++ b/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
> @@ -299,7 +299,6 @@ static int wuxga_nt_panel_remove(struct mipi_dsi_device *dsi)
>  	if (ret < 0)
>  		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
>  
> -	drm_panel_detach(&wuxga_nt->base);
>  	wuxga_nt_panel_del(wuxga_nt);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
> index 71c09ed..75f9253 100644
> --- a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
> +++ b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
> @@ -292,7 +292,6 @@ static int seiko_panel_remove(struct platform_device *pdev)
>  {
>  	struct seiko_panel *panel = dev_get_drvdata(&pdev->dev);
>  
> -	drm_panel_detach(&panel->base);
>  	drm_panel_remove(&panel->base);
>  
>  	seiko_panel_disable(&panel->base);
> diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> index 6bf8730..02fc0f5 100644
> --- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> +++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> @@ -418,7 +418,6 @@ static int sharp_panel_remove(struct mipi_dsi_device *dsi)
>  	if (err < 0)
>  		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
>  
> -	drm_panel_detach(&sharp->base);
>  	sharp_panel_del(sharp);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
> index 494aa9b..e5cae00 100644
> --- a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
> +++ b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
> @@ -327,7 +327,6 @@ static int sharp_nt_panel_remove(struct mipi_dsi_device *dsi)
>  	if (ret < 0)
>  		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
>  
> -	drm_panel_detach(&sharp_nt->base);
>  	sharp_nt_panel_del(sharp_nt);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index f2d96611..9388acf 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -364,7 +364,6 @@ static int panel_simple_remove(struct device *dev)
>  {
>  	struct panel_simple *panel = dev_get_drvdata(dev);
>  
> -	drm_panel_detach(&panel->base);
>  	drm_panel_remove(&panel->base);
>  
>  	panel_simple_disable(&panel->base);
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index 358c64e..74284e5 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -419,7 +419,6 @@ static int st7789v_remove(struct spi_device *spi)
>  {
>  	struct st7789v *ctx = spi_get_drvdata(spi);
>  
> -	drm_panel_detach(&ctx->panel);
>  	drm_panel_remove(&ctx->panel);
>  
>  	if (ctx->backlight)
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel