The first version of the series can be found here: https://lists.freedesktop.org/archives/dri-devel/2018-February/166909.html
The only change is the dev_err print in drm_panel_attach() if device_link_add() fails.
The device_link_del() is still there in drm_panel_detach(), despite Lukas Wunner's comment[1]. In the usual (currently all) cases things would work perfectly without the call too, because device_links_driver_cleanup() will eventually remove all orphaned links. However, this would cause an error in the situation where a drm device would like to detach a panel but remain operational, since the drm device would be unbound for no good reason if the detached panel is later unbound.
Anyway, if somebody still comes up with an argument for dropping the device_link_del() from drm_panel_detach(), I'll do it as things will normally work fine either case (drm devices don't normally detach panels and remain operational).
These older comment still apply:
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[2]) 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-February/167047.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 | 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(-)
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 jsarha@ti.com --- 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(-)
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 b5e3994..e8bc356 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 3b0ba9f..5aa736c 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)
On Wed, Feb 28, 2018 at 01:09:29PM +0200, Jyri Sarha wrote:
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 jsarha@ti.com
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(-)
I don't understand the purpose of this patch. I'll grant you that the current implementation of drm_panel_detach() is not very useful, but then you add code to drm_panel_detach() in the next patch and mention in the commit message that panel drivers should be calling the drm_panel_detach() function to remove the link.
This is confusing. Can you clarify?
Thierry
On 28/02/18 20:53, Thierry Reding wrote:
On Wed, Feb 28, 2018 at 01:09:29PM +0200, Jyri Sarha wrote:
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 jsarha@ti.com
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(-)
I don't understand the purpose of this patch. I'll grant you that the current implementation of drm_panel_detach() is not very useful, but then you add code to drm_panel_detach() in the next patch and mention in the commit message that panel drivers should be calling the drm_panel_detach() function to remove the link.
This is confusing. Can you clarify?
When looking at the current implementation it does not make any sense to me to call drm_panel_detach() from the panel driver itself. However, it makes perfect sense calling it from drm driver. Setting panel->connector = NULL marks it free and attachable to other devices, but the panel driver that the passive element in the picture should not go and mark itself available on its own.
But now that I take the steps to make the drm_panel_detach() to be called only from drm device I should at least document it too.
Also in general I think it is hard to come up with a detach implementation that would work from both panel and the drm device.
Best regards, Jyri
On Wed, Feb 28, 2018 at 11:31:53PM +0200, Jyri Sarha wrote:
On 28/02/18 20:53, Thierry Reding wrote:
On Wed, Feb 28, 2018 at 01:09:29PM +0200, Jyri Sarha wrote:
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 jsarha@ti.com
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(-)
I don't understand the purpose of this patch. I'll grant you that the current implementation of drm_panel_detach() is not very useful, but then you add code to drm_panel_detach() in the next patch and mention in the commit message that panel drivers should be calling the drm_panel_detach() function to remove the link.
This is confusing. Can you clarify?
When looking at the current implementation it does not make any sense to me to call drm_panel_detach() from the panel driver itself. However, it makes perfect sense calling it from drm driver. Setting panel->connector = NULL marks it free and attachable to other devices, but the panel driver that the passive element in the picture should not go and mark itself available on its own.
But now that I take the steps to make the drm_panel_detach() to be called only from drm device I should at least document it too.
Also in general I think it is hard to come up with a detach implementation that would work from both panel and the drm device.
I think we first need a series which changes drm_panel_detach to be called by drm drivers (not panel drivers), and have a drm_panel_remove of similar (like we do with bridges) to remove the panel driver.
Then I think this series here makes a lot more sense as a follow-up. Otherwise it's indeed rather confusing. -Daniel
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(+)
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;
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)
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)
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
On Wed, Feb 28, 2018 at 01:09:28PM +0200, Jyri Sarha wrote:
The device_link_del() is still there in drm_panel_detach(), despite Lukas Wunner's comment[1]. In the usual (currently all) cases things would work perfectly without the call too, because device_links_driver_cleanup() will eventually remove all orphaned links. However, this would cause an error in the situation where a drm device would like to detach a panel but remain operational, since the drm device would be unbound for no good reason if the detached panel is later unbound.
Okay, in that case I'd suggest dropping the DL_FLAG_AUTOREMOVE flag and keep the device_link_del(). That gives you the flexibility to detach a panel at runtime and drop the device link, but also have the DRM driver unbound once the panel driver is unbound.
If you have things like optional panels that can be detached without the necessity to unbind the DRM driver, you need something else instead of or on top of device links. Perhaps some kind of notifier block. And perhaps two drm_panel_attach/detach() helpers in the DRM library, one with device link and one with notifier.
As stated in the device links documentation, optional dependencies are "beyond the scope of device links."
Thanks,
Lukas
On 28/02/18 21:47, Lukas Wunner wrote:
On Wed, Feb 28, 2018 at 01:09:28PM +0200, Jyri Sarha wrote:
The device_link_del() is still there in drm_panel_detach(), despite Lukas Wunner's comment[1]. In the usual (currently all) cases things would work perfectly without the call too, because device_links_driver_cleanup() will eventually remove all orphaned links. However, this would cause an error in the situation where a drm device would like to detach a panel but remain operational, since the drm device would be unbound for no good reason if the detached panel is later unbound.
Okay, in that case I'd suggest dropping the DL_FLAG_AUTOREMOVE flag and keep the device_link_del(). That gives you the flexibility to detach a panel at runtime and drop the device link, but also have the DRM driver unbound once the panel driver is unbound.
If you have things like optional panels that can be detached without the necessity to unbind the DRM driver, you need something else instead of or on top of device links. Perhaps some kind of notifier block. And perhaps two drm_panel_attach/detach() helpers in the DRM library, one with device link and one with notifier.
As stated in the device links documentation, optional dependencies are "beyond the scope of device links."
I think the "optional panel" usage pattern is quite unlikely to ever exist, but still it sound wrong to me to leave the links behind when there is an apparent symmetry of detach and attach functions.
The situation would be different if we would get rid of the detach call all together. After all the function in its current form is pretty useless. The only purpose for its existence is marking the panel available for other drm devices to use, which would suggest that the "optional panel"-pattern is supported.
I think my current approach is fine (after removing the DL_FLAG_AUTOREMOVE, I had not really understood its purpose before) if we accept that the device link is there only as a precaution. But I am also fine with removing the drm_panel_detach() function and laeving the DL_FLAG_AUTOREMOVE flag there.
Best regards, Jyri
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Eric Anholt -
Jyri Sarha -
Lukas Wunner -
Thierry Reding