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

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

[Jyri Sarha]

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

-- 
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 v2 1/2] drm/panel: Remove drm_panel_detach() calls from all panel drives

[Jyri Sarha]

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)
-- 
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 v2 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)
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;
 
-- 
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 v2 2/2] drm/panel: Add device_link from panel device to drm device

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1003 bytes --]

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)

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1287 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

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

[Lukas Wunner]

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

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

[Lukas Wunner]

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

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

[Jyri Sarha]

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

-- 
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 v2 0/2] drm/panel: Add device link in drm_panel_attach()

[Jyri Sarha]

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


-- 
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 v2 2/2] drm/panel: Add device_link from panel device to drm device

[Jyri Sarha]

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

-- 
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 v2 1/2] drm/panel: Remove drm_panel_detach() calls from all panel drives

[Daniel Vetter]

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