drm/panel: Add device link in drm_panel_attach()

PATCH Superseded Jyri Sarha 2 participants 6 messages Feb 22, 2018 – Feb 23, 2018

Subsystems (4)

DRM drivers DRM misc DRM panel drivers DRM driver for LVDS panels

lore

Share & tools

lore

Jyri Sarha

author

2018-02-22 17:47 UTC #0 lore raw

cover drm/panel: Add device link in drm_panel_attach()

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 unbind 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 drived is bound again, but everything should work if the drm master driver is bound manually. Atleast this the behavior with this series on am335x-evm:

https://lists.freedesktop.org/archives/dri-devel/2018-February/166898.html

[1] 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                          | 12 ++++++++++++
 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, 13 insertions(+), 9 deletions(-)

Signature · 6 lines

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Jyri Sarha

author

2018-02-22 17:47 UTC #1 lore raw

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

Setting the connector and drm to NULL when the drm panel device is going away hardly serves any purpose. Usually the the whole memory stucture is freed right after the remove call.

Signed-off-by Jyri Sarha <[email protected]>

---
 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 -
 9 files changed, 9 deletions(-)

drivers/gpu/drm/panel/panel-innolux-p079zca.c +0 -1

index 6ba9344..517782a 100644
@@ -310,7 +310,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;

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c +0 -1

index 5b2340e..3f6ac64 100644
@@ -502,7 +502,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;

drivers/gpu/drm/panel/panel-lvds.c +0 -1

index b5e3994..e8bc356 100644
@@ -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);

drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c +0 -1

index 74a8061..cb4dfb9 100644
@@ -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;

drivers/gpu/drm/panel/panel-seiko-43wvf1g.c +0 -1

index 71c09ed..75f9253 100644
@@ -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);

drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c +0 -1

index 3cce3ca..dcdf517 100644
@@ -442,7 +442,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;

drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c +0 -1

index 3aeb0bd..5f9d3ed 100644
@@ -352,7 +352,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;

drivers/gpu/drm/panel/panel-simple.c +0 -1

index 3b0ba9f..5aa736c 100644
@@ -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);

drivers/gpu/drm/panel/panel-sitronix-st7789v.c +0 -1

index 358c64e..74284e5 100644
@@ -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)

Signature · 6 lines

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Jyri Sarha

author

2018-02-22 17:47 UTC #2 lore raw

PATCH 2/2 drm/panel: Add device_link from panel device to drm device

In reply to Jyri Sarha

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 <[email protected]>

---
 drivers/gpu/drm/drm_panel.c | 12 ++++++++++++
 include/drm/drm_panel.h     |  1 +
 2 files changed, 13 insertions(+)

drivers/gpu/drm/drm_panel.c +12 -0

index 308d442..d9452e98 100644
@@ -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;

include/drm/drm_panel.h +1 -0

index 14ac240..26a1b5f 100644
@@ -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;

Signature · 6 lines

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Lukas Wunner

2018-02-23 06:58 UTC #3 lore raw

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

Quoted 10 lines · Jyri Sarha

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
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Jyri Sarha

author

2018-02-23 10:43 UTC #4 lore raw

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

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

Quoted 2 lines · Lukas Wunner

Review static void panel_bridge_detach(struct drm_bridge
Quoted 1 line · Jyri Sarha
(quoted diff) +0 -0
@@ -94,6 +114,8 @@ static void panel_bridge_detach(struct drm_bridge

*bridge)

Quoted 11 lines

Review static void panel_bridge_detach(struct drm_bridge · line 118
Quoted 5 lines
(quoted diff) +2 -0
 	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.

Quoted 2 lines

Review static void panel_bridge_detach(struct drm_bridge · line 119
Quoted 2 lines
(quoted diff) +1 -0
+static int panel_bridge_link_to_master(struct panel_bridge

*panel_bridge)

Quoted 4 lines

(quoted diff) +4 -0

+{
+	struct device *mdev = panel_bridge->bridge.dev->dev;
+	struct device *pdev = panel_bridge->panel->dev;
+	u32 flags = DL_FLAG_AUTOREMOVE;

Quoted 9 lines

Review
Quoted 5 lines
(quoted diff) +5 -0
+
+	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.

Quoted 3 lines

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.

Quoted 4 lines

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

Quoted 4 lines

Thanks,
Lukas

Best regards,
Jyri

Quoted 21 lines · Lukas Wunner

Quoted 10 lines · Jyri Sarha
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

Signature · 6 lines

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

Jyri Sarha

author

2018-02-23 13:10 UTC #5 lore raw

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

Quoted 27 lines · Jyri Sarha

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).
Quoted 2 lines · Lukas Wunner
Review static void panel_bridge_detach(struct drm_bridge
Quoted 1 line · Jyri Sarha
(quoted diff) +0 -0
@@ -94,6 +114,8 @@ static void panel_bridge_detach(struct drm_bridge
*bridge)
Quoted 11 lines
Review static void panel_bridge_detach(struct drm_bridge · line 118
Quoted 5 lines
(quoted diff) +2 -0
 	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.

Quoted 6 lines

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.

Quoted 62 lines

Quoted 2 lines
Review static void panel_bridge_detach(struct drm_bridge · line 119
Quoted 2 lines
(quoted diff) +1 -0
+static int panel_bridge_link_to_master(struct panel_bridge
*panel_bridge)
Quoted 4 lines
Quoted 4 lines
(quoted diff) +4 -0
+{
+	struct device *mdev = panel_bridge->bridge.dev->dev;
+	struct device *pdev = panel_bridge->panel->dev;
+	u32 flags = DL_FLAG_AUTOREMOVE;
Quoted 9 lines
Review
Quoted 5 lines
(quoted diff) +5 -0
+
+	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.
Quoted 3 lines
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.
Quoted 4 lines
(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.
Quoted 4 lines
Thanks,
Lukas
Best regards,
Jyri
Quoted 21 lines · Lukas Wunner
Quoted 10 lines · Jyri Sarha
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

Signature · 6 lines

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