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

Commit Message

Jyri Sarha April 26, 2018, 8:07 a.m. UTC

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 <[e-mail]>
Reviewed-by: Eric Anholt <[e-mail]>
---
 drivers/gpu/drm/drm_panel.c | 10 ++++++++++
 include/drm/drm_panel.h     |  1 +
 2 files changed, 11 insertions(+)

Comments

Andrzej Hajda May 18, 2018, 11:51 a.m. UTC | #1

On 26.04.2018 10:07, Jyri Sarha wrote:
> 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 <[e-mail]>
> Reviewed-by: Eric Anholt <[e-mail]>

Hi Jyri,

This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
using exynos_drm_dsi and dsi panels.
Exynos-DSI properly handles panels unbind - ie references to panel are
properly removed on panels removal and respective drm_connector enters
disconnected state, without destroying whole drm device.
And again on panel driver re-bind drm_panel is properly attached to
exynos-dsi and panel pipeline is working again.
This patch will break this behavior, ie it will destroy whole drm device.

Making device_links for panels optional seems to me the best solution,
what do you think?

The funny thing is that due to bug in device link code, your patch has
no effect on these platforms. So it does not hurt these platform yet,
but the bug will be fixed sooner or later.
The trick in case of exynos-dsi is to use mipi_dsi attach/detach
callbacks to get notifications about drm_panel's
appearance/disappearance, I guess ultimately such callbacks should be a
part of drm_panel
framework.

Below description how all the code works:
Initialization:
1. exynos_dsi (component of exynos_drm) at the end of bind callback
registers mipi_dsi bus.
2. during mipi_dsi bus registration or later (if panel driver was not
yet registered), panel driver is bound ie - its probe callback is called.
3. during panel's probe drm_panel_add is called, and later mipi_dsi_attach.
4. mipi_dsi_attach calls dsi .attach callback of exynos_dsi.
5. exynos_dsi.attach callback looks for drm_panel, calls
drm_panel_attach and changes connector's status to connected.
....
Panel's unbind:
1. device_release_driver calls panel's remove callback.
2. panel's remove callback calls mipi_dsi_detach.
3. mipi_dsi_detach calls exynos_dsi.detach callback.
4. exynos_dsi.detach calls drm_panel_detach and changes connector's
state to disconnected.

Panel's rebind is equivalent of steps 2-4 of Initialization.

I have wrote the code few years ago, and it seemed to me the only safe
way to handle dynamic panel bind/unbind.
For example please note that step 5 of initialization and steps 2-4 of
unbind are executed always under panel's device_lock so it makes it
safe. Otherwise you risk that between panel lookup and panel attach
panel's driver can be unbind and consumer is left with dangling panel's
pointer, so even with your patch most of the drivers are still buggy.

And few words about the bug(?) in device_link code: If you call
device_link_add from probe of the supplier(as it is in this case) its
status is set to DL_STATE_DORMANT, and after successful bind it is
changed to DL_STATE_AVAILABLE, which is incorrect (it should be
DL_STATE_ACTIVE), as a result on supplier's unbind consumer is not unbound.

Regards
Andrzej

> ---
>  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 71e4075..965530a 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>
>  
> @@ -104,6 +105,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;
>  
> @@ -125,6 +133,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 May 18, 2018, 12:54 p.m. UTC | #2

On 18/05/18 14:51, Andrzej Hajda wrote:
> On 26.04.2018 10:07, Jyri Sarha wrote:
>> 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 <[e-mail]>
>> Reviewed-by: Eric Anholt <[e-mail]>
> 
> Hi Jyri,
> 
> This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
> using exynos_drm_dsi and dsi panels.
> Exynos-DSI properly handles panels unbind - ie references to panel are
> properly removed on panels removal and respective drm_connector enters
> disconnected state, without destroying whole drm device.
> And again on panel driver re-bind drm_panel is properly attached to
> exynos-dsi and panel pipeline is working again.
> This patch will break this behavior, ie it will destroy whole drm device.
> 
> Making device_links for panels optional seems to me the best solution,
> what do you think?
> 

Sounds good to me. Unfortunately I have no time to spend on this in the
coming two weeks. If you can come up with a fix, please send patch and
put me in cc.

I am also fine with reverting the patch.

> The funny thing is that due to bug in device link code, your patch has
> no effect on these platforms. So it does not hurt these platform yet,
> but the bug will be fixed sooner or later.
> The trick in case of exynos-dsi is to use mipi_dsi attach/detach
> callbacks to get notifications about drm_panel's
> appearance/disappearance, I guess ultimately such callbacks should be a
> part of drm_panel
> framework.
> 

That sounds like an ultimate solution, especially if there is a default
behaviour that unbinds the master drm device as the most devices do not
need anything else.

Best regards,
Jyri

> Below description how all the code works:
> Initialization:
> 1. exynos_dsi (component of exynos_drm) at the end of bind callback
> registers mipi_dsi bus.
> 2. during mipi_dsi bus registration or later (if panel driver was not
> yet registered), panel driver is bound ie - its probe callback is called.
> 3. during panel's probe drm_panel_add is called, and later mipi_dsi_attach.
> 4. mipi_dsi_attach calls dsi .attach callback of exynos_dsi.
> 5. exynos_dsi.attach callback looks for drm_panel, calls
> drm_panel_attach and changes connector's status to connected.
> ....
> Panel's unbind:
> 1. device_release_driver calls panel's remove callback.
> 2. panel's remove callback calls mipi_dsi_detach.
> 3. mipi_dsi_detach calls exynos_dsi.detach callback.
> 4. exynos_dsi.detach calls drm_panel_detach and changes connector's
> state to disconnected.
> 
> Panel's rebind is equivalent of steps 2-4 of Initialization.
> 
> I have wrote the code few years ago, and it seemed to me the only safe
> way to handle dynamic panel bind/unbind.
> For example please note that step 5 of initialization and steps 2-4 of
> unbind are executed always under panel's device_lock so it makes it
> safe. Otherwise you risk that between panel lookup and panel attach
> panel's driver can be unbind and consumer is left with dangling panel's
> pointer, so even with your patch most of the drivers are still buggy.
> 
> And few words about the bug(?) in device_link code: If you call
> device_link_add from probe of the supplier(as it is in this case) its
> status is set to DL_STATE_DORMANT, and after successful bind it is
> changed to DL_STATE_AVAILABLE, which is incorrect (it should be
> DL_STATE_ACTIVE), as a result on supplier's unbind consumer is not unbound.
> 
> Regards
> Andrzej
> 
>> ---
>>  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 71e4075..965530a 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>
>>  
>> @@ -104,6 +105,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;
>>  
>> @@ -125,6 +133,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;
>>  
> 
>

Peter Rosin May 19, 2018, 4:48 p.m. UTC | #3

On 2018-05-18 13:51, Andrzej Hajda wrote:
> On 26.04.2018 10:07, Jyri Sarha wrote:
>> 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 <[e-mail]>
>> Reviewed-by: Eric Anholt <[e-mail]>
> 
> Hi Jyri,
> 
> This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
> using exynos_drm_dsi and dsi panels.
> Exynos-DSI properly handles panels unbind - ie references to panel are
> properly removed on panels removal and respective drm_connector enters
> disconnected state, without destroying whole drm device.
> And again on panel driver re-bind drm_panel is properly attached to
> exynos-dsi and panel pipeline is working again.
> This patch will break this behavior, ie it will destroy whole drm device.
> 
> Making device_links for panels optional seems to me the best solution,
> what do you think?
> 
> The funny thing is that due to bug in device link code, your patch has
> no effect on these platforms. So it does not hurt these platform yet,
> but the bug will be fixed sooner or later.

Ah, that's a pretty strong hint that we are doing something unusual. So,
I had a deeper look and I think that device-links (with state, i.e. not
DL_FLAG_STATELESS links) are assumed to be created by the .probe function
of either the consumer or the supplier. Then it seems to works as expected.
And that makes some sense too, because a link indicates that there exist
a dependency between the two and that the consumer cannot really exist
without the supplier. This is also what happens for the drm devices
(panel/bridge consumers) Jyri and I are working with; they call panel or
bridge attach during their probe. But this is not the case for exynos,
which calls panel/bridge attach at some later stage, and that obviously
violates the assumption that the device-link consumer cannot exist w/o
the supplier ("obviously" since the drm driver has managed to successfully
probe without the supplier).

So, when the panel/bridge supplier is probed after the consumer is
bound, the link starts out as DL_STATE_DORMANT, and progresses to
DL_STATE_AVAILABLE once the panel/bridge has finished the probe. This is
not what *we* want.

So, one idea I have is to, on panel/bridge attach, verify if the
consumer is in its probe, and only create the link if that is the
case. So, the link would be optional, but it would all be automatic.

The function device_is_bound seems like a good candidate for that
check?

Maybe device_link_add should automatically create a DL_FLAG_STATELESS link
if either consumer or supplier has "proven" (with a successful probe) that
they can exist without the other end? That would also work for us, since
a DL_FLAG_STATELESS link does no harm in our case...

Cheers,
Peter

> The trick in case of exynos-dsi is to use mipi_dsi attach/detach
> callbacks to get notifications about drm_panel's
> appearance/disappearance, I guess ultimately such callbacks should be a
> part of drm_panel
> framework.
> 
> Below description how all the code works:
> Initialization:
> 1. exynos_dsi (component of exynos_drm) at the end of bind callback
> registers mipi_dsi bus.
> 2. during mipi_dsi bus registration or later (if panel driver was not
> yet registered), panel driver is bound ie - its probe callback is called.
> 3. during panel's probe drm_panel_add is called, and later mipi_dsi_attach.
> 4. mipi_dsi_attach calls dsi .attach callback of exynos_dsi.
> 5. exynos_dsi.attach callback looks for drm_panel, calls
> drm_panel_attach and changes connector's status to connected.
> ....
> Panel's unbind:
> 1. device_release_driver calls panel's remove callback.
> 2. panel's remove callback calls mipi_dsi_detach.
> 3. mipi_dsi_detach calls exynos_dsi.detach callback.
> 4. exynos_dsi.detach calls drm_panel_detach and changes connector's
> state to disconnected.
> 
> Panel's rebind is equivalent of steps 2-4 of Initialization.
> 
> I have wrote the code few years ago, and it seemed to me the only safe
> way to handle dynamic panel bind/unbind.
> For example please note that step 5 of initialization and steps 2-4 of
> unbind are executed always under panel's device_lock so it makes it
> safe. Otherwise you risk that between panel lookup and panel attach
> panel's driver can be unbind and consumer is left with dangling panel's
> pointer, so even with your patch most of the drivers are still buggy.
> 
> And few words about the bug(?) in device_link code: If you call
> device_link_add from probe of the supplier(as it is in this case) its
> status is set to DL_STATE_DORMANT, and after successful bind it is
> changed to DL_STATE_AVAILABLE, which is incorrect (it should be
> DL_STATE_ACTIVE), as a result on supplier's unbind consumer is not unbound.
> 
> Regards
> Andrzej
> 
>> ---
>>  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 71e4075..965530a 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>
>>  
>> @@ -104,6 +105,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;
>>  
>> @@ -125,6 +133,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;
>>  
> 
>

Peter Rosin May 19, 2018, 6:01 p.m. UTC | #4

On 2018-05-19 18:48, Peter Rosin wrote:
> On 2018-05-18 13:51, Andrzej Hajda wrote:
>> On 26.04.2018 10:07, Jyri Sarha wrote:
>>> 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 <[e-mail]>
>>> Reviewed-by: Eric Anholt <[e-mail]>
>>
>> Hi Jyri,
>>
>> This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
>> using exynos_drm_dsi and dsi panels.
>> Exynos-DSI properly handles panels unbind - ie references to panel are
>> properly removed on panels removal and respective drm_connector enters
>> disconnected state, without destroying whole drm device.
>> And again on panel driver re-bind drm_panel is properly attached to
>> exynos-dsi and panel pipeline is working again.
>> This patch will break this behavior, ie it will destroy whole drm device.
>>
>> Making device_links for panels optional seems to me the best solution,
>> what do you think?
>>
>> The funny thing is that due to bug in device link code, your patch has
>> no effect on these platforms. So it does not hurt these platform yet,
>> but the bug will be fixed sooner or later.
> 
> Ah, that's a pretty strong hint that we are doing something unusual. So,
> I had a deeper look and I think that device-links (with state, i.e. not
> DL_FLAG_STATELESS links) are assumed to be created by the .probe function
> of either the consumer or the supplier. Then it seems to works as expected.
> And that makes some sense too, because a link indicates that there exist
> a dependency between the two and that the consumer cannot really exist
> without the supplier. This is also what happens for the drm devices
> (panel/bridge consumers) Jyri and I are working with; they call panel or
> bridge attach during their probe. But this is not the case for exynos,
> which calls panel/bridge attach at some later stage, and that obviously
> violates the assumption that the device-link consumer cannot exist w/o
> the supplier ("obviously" since the drm driver has managed to successfully
> probe without the supplier).
> 
> So, when the panel/bridge supplier is probed after the consumer is
> bound, the link starts out as DL_STATE_DORMANT, and progresses to
> DL_STATE_AVAILABLE once the panel/bridge has finished the probe. This is
> not what *we* want.
> 
> So, one idea I have is to, on panel/bridge attach, verify if the
> consumer is in its probe, and only create the link if that is the
> case. So, the link would be optional, but it would all be automatic.
> 
> The function device_is_bound seems like a good candidate for that
> check?
> 
> Maybe device_link_add should automatically create a DL_FLAG_STATELESS link
> if either consumer or supplier has "proven" (with a successful probe) that

s/either consumer or supplier/the consumer/

The supplier should always be able to probe w/o the consumer being present.

> they can exist without the other end? That would also work for us, since
> a DL_FLAG_STATELESS link does no harm in our case...
> 
> Cheers,
> Peter
> 
>> The trick in case of exynos-dsi is to use mipi_dsi attach/detach
>> callbacks to get notifications about drm_panel's
>> appearance/disappearance, I guess ultimately such callbacks should be a
>> part of drm_panel
>> framework.
>>
>> Below description how all the code works:
>> Initialization:
>> 1. exynos_dsi (component of exynos_drm) at the end of bind callback
>> registers mipi_dsi bus.
>> 2. during mipi_dsi bus registration or later (if panel driver was not
>> yet registered), panel driver is bound ie - its probe callback is called.
>> 3. during panel's probe drm_panel_add is called, and later mipi_dsi_attach.
>> 4. mipi_dsi_attach calls dsi .attach callback of exynos_dsi.
>> 5. exynos_dsi.attach callback looks for drm_panel, calls
>> drm_panel_attach and changes connector's status to connected.
>> ....
>> Panel's unbind:
>> 1. device_release_driver calls panel's remove callback.
>> 2. panel's remove callback calls mipi_dsi_detach.
>> 3. mipi_dsi_detach calls exynos_dsi.detach callback.
>> 4. exynos_dsi.detach calls drm_panel_detach and changes connector's
>> state to disconnected.
>>
>> Panel's rebind is equivalent of steps 2-4 of Initialization.
>>
>> I have wrote the code few years ago, and it seemed to me the only safe
>> way to handle dynamic panel bind/unbind.
>> For example please note that step 5 of initialization and steps 2-4 of
>> unbind are executed always under panel's device_lock so it makes it
>> safe. Otherwise you risk that between panel lookup and panel attach
>> panel's driver can be unbind and consumer is left with dangling panel's
>> pointer, so even with your patch most of the drivers are still buggy.
>>
>> And few words about the bug(?) in device_link code: If you call
>> device_link_add from probe of the supplier(as it is in this case) its
>> status is set to DL_STATE_DORMANT, and after successful bind it is
>> changed to DL_STATE_AVAILABLE, which is incorrect (it should be
>> DL_STATE_ACTIVE), as a result on supplier's unbind consumer is not unbound.
>>
>> Regards
>> Andrzej
>>
>>> ---
>>>  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 71e4075..965530a 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>
>>>  
>>> @@ -104,6 +105,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;
>>>  
>>> @@ -125,6 +133,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;
>>>  
>>
>>
>

Andrzej Hajda May 21, 2018, 8:15 a.m. UTC | #5

On 19.05.2018 18:48, Peter Rosin wrote:
> On 2018-05-18 13:51, Andrzej Hajda wrote:
>> On 26.04.2018 10:07, Jyri Sarha wrote:
>>> 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 <[e-mail]>
>>> Reviewed-by: Eric Anholt <[e-mail]>
>> Hi Jyri,
>>
>> This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
>> using exynos_drm_dsi and dsi panels.
>> Exynos-DSI properly handles panels unbind - ie references to panel are
>> properly removed on panels removal and respective drm_connector enters
>> disconnected state, without destroying whole drm device.
>> And again on panel driver re-bind drm_panel is properly attached to
>> exynos-dsi and panel pipeline is working again.
>> This patch will break this behavior, ie it will destroy whole drm device.
>>
>> Making device_links for panels optional seems to me the best solution,
>> what do you think?
>>
>> The funny thing is that due to bug in device link code, your patch has
>> no effect on these platforms. So it does not hurt these platform yet,
>> but the bug will be fixed sooner or later.
> Ah, that's a pretty strong hint that we are doing something unusual. So,
> I had a deeper look and I think that device-links (with state, i.e. not
> DL_FLAG_STATELESS links) are assumed to be created by the .probe function
> of either the consumer or the supplier. Then it seems to works as expected.
> And that makes some sense too, because a link indicates that there exist
> a dependency between the two and that the consumer cannot really exist
> without the supplier. This is also what happens for the drm devices
> (panel/bridge consumers) Jyri and I are working with; they call panel or
> bridge attach during their probe. But this is not the case for exynos,
> which calls panel/bridge attach at some later stage, and that obviously
> violates the assumption that the device-link consumer cannot exist w/o
> the supplier ("obviously" since the drm driver has managed to successfully
> probe without the supplier).
>
> So, when the panel/bridge supplier is probed after the consumer is
> bound, the link starts out as DL_STATE_DORMANT, and progresses to
> DL_STATE_AVAILABLE once the panel/bridge has finished the probe. This is
> not what *we* want.

And this is also incorrect from Documentation PoV:
 * @DL_STATE_DORMANT: None of the supplier/consumer drivers is present.
 * @DL_STATE_AVAILABLE: The supplier driver is present, but the consumer
is not.

>
> So, one idea I have is to, on panel/bridge attach, verify if the
> consumer is in its probe, and only create the link if that is the
> case. So, the link would be optional, but it would all be automatic.

Making it automatic looks tempting, but also error prone. In case of
component framework bind callbacks can be called from probe of any
component, so sometimes it can be called also from exynos-dsi probe,
sometimes not (depending on order of probing, which we cannot rely on).
So in some cases we will end-up with links, sometimes without. Ie
following scenarios are possible in drm_panel_attach:
1. exynos-dsi bound, panel during probe.
2. exynos-dsi during probe, panel during probe.

>
> The function device_is_bound seems like a good candidate for that
> check?
>
> Maybe device_link_add should automatically create a DL_FLAG_STATELESS link
> if either consumer or supplier has "proven" (with a successful probe) that
> they can exist without the other end? That would also work for us, since
> a DL_FLAG_STATELESS link does no harm in our case...

What will be benefit of having stateless device_links, in case of
panels/bridges?

Regards
Andrzej


>
> Cheers,
> Peter
>
>> The trick in case of exynos-dsi is to use mipi_dsi attach/detach
>> callbacks to get notifications about drm_panel's
>> appearance/disappearance, I guess ultimately such callbacks should be a
>> part of drm_panel
>> framework.
>>
>> Below description how all the code works:
>> Initialization:
>> 1. exynos_dsi (component of exynos_drm) at the end of bind callback
>> registers mipi_dsi bus.
>> 2. during mipi_dsi bus registration or later (if panel driver was not
>> yet registered), panel driver is bound ie - its probe callback is called.
>> 3. during panel's probe drm_panel_add is called, and later mipi_dsi_attach.
>> 4. mipi_dsi_attach calls dsi .attach callback of exynos_dsi.
>> 5. exynos_dsi.attach callback looks for drm_panel, calls
>> drm_panel_attach and changes connector's status to connected.
>> ....
>> Panel's unbind:
>> 1. device_release_driver calls panel's remove callback.
>> 2. panel's remove callback calls mipi_dsi_detach.
>> 3. mipi_dsi_detach calls exynos_dsi.detach callback.
>> 4. exynos_dsi.detach calls drm_panel_detach and changes connector's
>> state to disconnected.
>>
>> Panel's rebind is equivalent of steps 2-4 of Initialization.
>>
>> I have wrote the code few years ago, and it seemed to me the only safe
>> way to handle dynamic panel bind/unbind.
>> For example please note that step 5 of initialization and steps 2-4 of
>> unbind are executed always under panel's device_lock so it makes it
>> safe. Otherwise you risk that between panel lookup and panel attach
>> panel's driver can be unbind and consumer is left with dangling panel's
>> pointer, so even with your patch most of the drivers are still buggy.
>>
>> And few words about the bug(?) in device_link code: If you call
>> device_link_add from probe of the supplier(as it is in this case) its
>> status is set to DL_STATE_DORMANT, and after successful bind it is
>> changed to DL_STATE_AVAILABLE, which is incorrect (it should be
>> DL_STATE_ACTIVE), as a result on supplier's unbind consumer is not unbound.
>>
>> Regards
>> Andrzej
>>
>>> ---
>>>  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 71e4075..965530a 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>
>>>  
>>> @@ -104,6 +105,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;
>>>  
>>> @@ -125,6 +133,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;
>>>  
>>
>
>
>

Peter Rosin May 21, 2018, 8:53 a.m. UTC | #6

On 2018-05-21 10:15, Andrzej Hajda wrote:
> On 19.05.2018 18:48, Peter Rosin wrote:
>> On 2018-05-18 13:51, Andrzej Hajda wrote:
>>> On 26.04.2018 10:07, Jyri Sarha wrote:
>>>> 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 <[e-mail]>
>>>> Reviewed-by: Eric Anholt <[e-mail]>
>>> Hi Jyri,
>>>
>>> This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
>>> using exynos_drm_dsi and dsi panels.
>>> Exynos-DSI properly handles panels unbind - ie references to panel are
>>> properly removed on panels removal and respective drm_connector enters
>>> disconnected state, without destroying whole drm device.
>>> And again on panel driver re-bind drm_panel is properly attached to
>>> exynos-dsi and panel pipeline is working again.
>>> This patch will break this behavior, ie it will destroy whole drm device.
>>>
>>> Making device_links for panels optional seems to me the best solution,
>>> what do you think?
>>>
>>> The funny thing is that due to bug in device link code, your patch has
>>> no effect on these platforms. So it does not hurt these platform yet,
>>> but the bug will be fixed sooner or later.
>> Ah, that's a pretty strong hint that we are doing something unusual. So,
>> I had a deeper look and I think that device-links (with state, i.e. not
>> DL_FLAG_STATELESS links) are assumed to be created by the .probe function
>> of either the consumer or the supplier. Then it seems to works as expected.
>> And that makes some sense too, because a link indicates that there exist
>> a dependency between the two and that the consumer cannot really exist
>> without the supplier. This is also what happens for the drm devices
>> (panel/bridge consumers) Jyri and I are working with; they call panel or
>> bridge attach during their probe. But this is not the case for exynos,
>> which calls panel/bridge attach at some later stage, and that obviously
>> violates the assumption that the device-link consumer cannot exist w/o
>> the supplier ("obviously" since the drm driver has managed to successfully
>> probe without the supplier).
>>
>> So, when the panel/bridge supplier is probed after the consumer is
>> bound, the link starts out as DL_STATE_DORMANT, and progresses to
>> DL_STATE_AVAILABLE once the panel/bridge has finished the probe. This is
>> not what *we* want.
> 
> And this is also incorrect from Documentation PoV:
>  * @DL_STATE_DORMANT: None of the supplier/consumer drivers is present.
>  * @DL_STATE_AVAILABLE: The supplier driver is present, but the consumer
> is not.
> 
>>
>> So, one idea I have is to, on panel/bridge attach, verify if the
>> consumer is in its probe, and only create the link if that is the
>> case. So, the link would be optional, but it would all be automatic.
> 
> Making it automatic looks tempting, but also error prone. In case of
> component framework bind callbacks can be called from probe of any
> component, so sometimes it can be called also from exynos-dsi probe,
> sometimes not (depending on order of probing, which we cannot rely on).
> So in some cases we will end-up with links, sometimes without. Ie
> following scenarios are possible in drm_panel_attach:
> 1. exynos-dsi bound, panel during probe.
> 2. exynos-dsi during probe, panel during probe.

2. exynos-dsi during probe, panel bound? Or is this case 3, and 2 happens
when drivers probe in parallel?

Whichever, you are right, I naively thought that the bind happened
after the probe of all devices, but naturally it happens as part of
the last device to register its component, and that normally happens
during its probe.

Sigh. So, scratch that, I guess we need a flag...

> 
>>
>> The function device_is_bound seems like a good candidate for that
>> check?
>>
>> Maybe device_link_add should automatically create a DL_FLAG_STATELESS link
>> if either consumer or supplier has "proven" (with a successful probe) that
>> they can exist without the other end? That would also work for us, since
>> a DL_FLAG_STATELESS link does no harm in our case...
> 
> What will be benefit of having stateless device_links, in case of
> panels/bridges?

To have device suspend/resume happen in a predictable order? But maybe that
has no value whatsoever?

Cheers,
Peter

> Regards
> Andrzej
> 
> 
>>
>> Cheers,
>> Peter
>>
>>> The trick in case of exynos-dsi is to use mipi_dsi attach/detach
>>> callbacks to get notifications about drm_panel's
>>> appearance/disappearance, I guess ultimately such callbacks should be a
>>> part of drm_panel
>>> framework.
>>>
>>> Below description how all the code works:
>>> Initialization:
>>> 1. exynos_dsi (component of exynos_drm) at the end of bind callback
>>> registers mipi_dsi bus.
>>> 2. during mipi_dsi bus registration or later (if panel driver was not
>>> yet registered), panel driver is bound ie - its probe callback is called.
>>> 3. during panel's probe drm_panel_add is called, and later mipi_dsi_attach.
>>> 4. mipi_dsi_attach calls dsi .attach callback of exynos_dsi.
>>> 5. exynos_dsi.attach callback looks for drm_panel, calls
>>> drm_panel_attach and changes connector's status to connected.
>>> ....
>>> Panel's unbind:
>>> 1. device_release_driver calls panel's remove callback.
>>> 2. panel's remove callback calls mipi_dsi_detach.
>>> 3. mipi_dsi_detach calls exynos_dsi.detach callback.
>>> 4. exynos_dsi.detach calls drm_panel_detach and changes connector's
>>> state to disconnected.
>>>
>>> Panel's rebind is equivalent of steps 2-4 of Initialization.
>>>
>>> I have wrote the code few years ago, and it seemed to me the only safe
>>> way to handle dynamic panel bind/unbind.
>>> For example please note that step 5 of initialization and steps 2-4 of
>>> unbind are executed always under panel's device_lock so it makes it
>>> safe. Otherwise you risk that between panel lookup and panel attach
>>> panel's driver can be unbind and consumer is left with dangling panel's
>>> pointer, so even with your patch most of the drivers are still buggy.
>>>
>>> And few words about the bug(?) in device_link code: If you call
>>> device_link_add from probe of the supplier(as it is in this case) its
>>> status is set to DL_STATE_DORMANT, and after successful bind it is
>>> changed to DL_STATE_AVAILABLE, which is incorrect (it should be
>>> DL_STATE_ACTIVE), as a result on supplier's unbind consumer is not unbound.
>>>
>>> Regards
>>> Andrzej
>>>
>>>> ---
>>>>  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 71e4075..965530a 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>
>>>>  
>>>> @@ -104,6 +105,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;
>>>>  
>>>> @@ -125,6 +133,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;
>>>>  
>>>
>>
>>
>>
>

Andrzej Hajda May 21, 2018, 9:21 a.m. UTC | #7

On 21.05.2018 10:53, Peter Rosin wrote:
> On 2018-05-21 10:15, Andrzej Hajda wrote:
>> On 19.05.2018 18:48, Peter Rosin wrote:
>>> On 2018-05-18 13:51, Andrzej Hajda wrote:
>>>> On 26.04.2018 10:07, Jyri Sarha wrote:
>>>>> 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 <[e-mail]>
>>>>> Reviewed-by: Eric Anholt <[e-mail]>
>>>> Hi Jyri,
>>>>
>>>> This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
>>>> using exynos_drm_dsi and dsi panels.
>>>> Exynos-DSI properly handles panels unbind - ie references to panel are
>>>> properly removed on panels removal and respective drm_connector enters
>>>> disconnected state, without destroying whole drm device.
>>>> And again on panel driver re-bind drm_panel is properly attached to
>>>> exynos-dsi and panel pipeline is working again.
>>>> This patch will break this behavior, ie it will destroy whole drm device.
>>>>
>>>> Making device_links for panels optional seems to me the best solution,
>>>> what do you think?
>>>>
>>>> The funny thing is that due to bug in device link code, your patch has
>>>> no effect on these platforms. So it does not hurt these platform yet,
>>>> but the bug will be fixed sooner or later.
>>> Ah, that's a pretty strong hint that we are doing something unusual. So,
>>> I had a deeper look and I think that device-links (with state, i.e. not
>>> DL_FLAG_STATELESS links) are assumed to be created by the .probe function
>>> of either the consumer or the supplier. Then it seems to works as expected.
>>> And that makes some sense too, because a link indicates that there exist
>>> a dependency between the two and that the consumer cannot really exist
>>> without the supplier. This is also what happens for the drm devices
>>> (panel/bridge consumers) Jyri and I are working with; they call panel or
>>> bridge attach during their probe. But this is not the case for exynos,
>>> which calls panel/bridge attach at some later stage, and that obviously
>>> violates the assumption that the device-link consumer cannot exist w/o
>>> the supplier ("obviously" since the drm driver has managed to successfully
>>> probe without the supplier).
>>>
>>> So, when the panel/bridge supplier is probed after the consumer is
>>> bound, the link starts out as DL_STATE_DORMANT, and progresses to
>>> DL_STATE_AVAILABLE once the panel/bridge has finished the probe. This is
>>> not what *we* want.
>> And this is also incorrect from Documentation PoV:
>>  * @DL_STATE_DORMANT: None of the supplier/consumer drivers is present.
>>  * @DL_STATE_AVAILABLE: The supplier driver is present, but the consumer
>> is not.
>>
>>> So, one idea I have is to, on panel/bridge attach, verify if the
>>> consumer is in its probe, and only create the link if that is the
>>> case. So, the link would be optional, but it would all be automatic.
>> Making it automatic looks tempting, but also error prone. In case of
>> component framework bind callbacks can be called from probe of any
>> component, so sometimes it can be called also from exynos-dsi probe,
>> sometimes not (depending on order of probing, which we cannot rely on).
>> So in some cases we will end-up with links, sometimes without. Ie
>> following scenarios are possible in drm_panel_attach:
>> 1. exynos-dsi bound, panel during probe.
>> 2. exynos-dsi during probe, panel during probe.
> 2. exynos-dsi during probe, panel bound? Or is this case 3, and 2 happens
> when drivers probe in parallel?

Panel is always probed not earlier than the end of exynos-dsi bind, so
only scenarios 1 and 2 should be possible.

> Whichever, you are right, I naively thought that the bind happened
> after the probe of all devices, but naturally it happens as part of
> the last device to register its component, and that normally happens
> during its probe.
>
> Sigh. So, scratch that, I guess we need a flag...
>
>>> The function device_is_bound seems like a good candidate for that
>>> check?
>>>
>>> Maybe device_link_add should automatically create a DL_FLAG_STATELESS link
>>> if either consumer or supplier has "proven" (with a successful probe) that
>>> they can exist without the other end? That would also work for us, since
>>> a DL_FLAG_STATELESS link does no harm in our case...
>> What will be benefit of having stateless device_links, in case of
>> panels/bridges?
> To have device suspend/resume happen in a predictable order? But maybe that
> has no value whatsoever?

At the moment panels do not have linux pm code, so I guess their pm is
done entirely by drm framework.
I am not sure about bridges.

Regards
Andrzej

>
> Cheers,
> Peter
>
>> Regards
>> Andrzej
>>
>>
>>> Cheers,
>>> Peter
>>>
>>>> The trick in case of exynos-dsi is to use mipi_dsi attach/detach
>>>> callbacks to get notifications about drm_panel's
>>>> appearance/disappearance, I guess ultimately such callbacks should be a
>>>> part of drm_panel
>>>> framework.
>>>>
>>>> Below description how all the code works:
>>>> Initialization:
>>>> 1. exynos_dsi (component of exynos_drm) at the end of bind callback
>>>> registers mipi_dsi bus.
>>>> 2. during mipi_dsi bus registration or later (if panel driver was not
>>>> yet registered), panel driver is bound ie - its probe callback is called.
>>>> 3. during panel's probe drm_panel_add is called, and later mipi_dsi_attach.
>>>> 4. mipi_dsi_attach calls dsi .attach callback of exynos_dsi.
>>>> 5. exynos_dsi.attach callback looks for drm_panel, calls
>>>> drm_panel_attach and changes connector's status to connected.
>>>> ....
>>>> Panel's unbind:
>>>> 1. device_release_driver calls panel's remove callback.
>>>> 2. panel's remove callback calls mipi_dsi_detach.
>>>> 3. mipi_dsi_detach calls exynos_dsi.detach callback.
>>>> 4. exynos_dsi.detach calls drm_panel_detach and changes connector's
>>>> state to disconnected.
>>>>
>>>> Panel's rebind is equivalent of steps 2-4 of Initialization.
>>>>
>>>> I have wrote the code few years ago, and it seemed to me the only safe
>>>> way to handle dynamic panel bind/unbind.
>>>> For example please note that step 5 of initialization and steps 2-4 of
>>>> unbind are executed always under panel's device_lock so it makes it
>>>> safe. Otherwise you risk that between panel lookup and panel attach
>>>> panel's driver can be unbind and consumer is left with dangling panel's
>>>> pointer, so even with your patch most of the drivers are still buggy.
>>>>
>>>> And few words about the bug(?) in device_link code: If you call
>>>> device_link_add from probe of the supplier(as it is in this case) its
>>>> status is set to DL_STATE_DORMANT, and after successful bind it is
>>>> changed to DL_STATE_AVAILABLE, which is incorrect (it should be
>>>> DL_STATE_ACTIVE), as a result on supplier's unbind consumer is not unbound.
>>>>
>>>> Regards
>>>> Andrzej
>>>>
>>>>> ---
>>>>>  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 71e4075..965530a 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>
>>>>>  
>>>>> @@ -104,6 +105,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;
>>>>>  
>>>>> @@ -125,6 +133,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;
>>>>>  
>>>
>>>
>
>
>

Peter Rosin May 21, 2018, 9:56 p.m. UTC | #8

On 2018-05-21 11:21, Andrzej Hajda wrote:
> On 21.05.2018 10:53, Peter Rosin wrote:
>> On 2018-05-21 10:15, Andrzej Hajda wrote:
>>> On 19.05.2018 18:48, Peter Rosin wrote:
>>>> On 2018-05-18 13:51, Andrzej Hajda wrote:
>>>>> On 26.04.2018 10:07, Jyri Sarha wrote:
>>>>>> 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 <[e-mail]>
>>>>>> Reviewed-by: Eric Anholt <[e-mail]>
>>>>> Hi Jyri,
>>>>>
>>>>> This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
>>>>> using exynos_drm_dsi and dsi panels.
>>>>> Exynos-DSI properly handles panels unbind - ie references to panel are
>>>>> properly removed on panels removal and respective drm_connector enters
>>>>> disconnected state, without destroying whole drm device.
>>>>> And again on panel driver re-bind drm_panel is properly attached to
>>>>> exynos-dsi and panel pipeline is working again.
>>>>> This patch will break this behavior, ie it will destroy whole drm device.
>>>>>
>>>>> Making device_links for panels optional seems to me the best solution,
>>>>> what do you think?
>>>>>
>>>>> The funny thing is that due to bug in device link code, your patch has
>>>>> no effect on these platforms. So it does not hurt these platform yet,
>>>>> but the bug will be fixed sooner or later.
>>>> Ah, that's a pretty strong hint that we are doing something unusual. So,
>>>> I had a deeper look and I think that device-links (with state, i.e. not
>>>> DL_FLAG_STATELESS links) are assumed to be created by the .probe function
>>>> of either the consumer or the supplier. Then it seems to works as expected.
>>>> And that makes some sense too, because a link indicates that there exist
>>>> a dependency between the two and that the consumer cannot really exist
>>>> without the supplier. This is also what happens for the drm devices
>>>> (panel/bridge consumers) Jyri and I are working with; they call panel or
>>>> bridge attach during their probe. But this is not the case for exynos,
>>>> which calls panel/bridge attach at some later stage, and that obviously
>>>> violates the assumption that the device-link consumer cannot exist w/o
>>>> the supplier ("obviously" since the drm driver has managed to successfully
>>>> probe without the supplier).
>>>>
>>>> So, when the panel/bridge supplier is probed after the consumer is
>>>> bound, the link starts out as DL_STATE_DORMANT, and progresses to
>>>> DL_STATE_AVAILABLE once the panel/bridge has finished the probe. This is
>>>> not what *we* want.
>>> And this is also incorrect from Documentation PoV:
>>>  * @DL_STATE_DORMANT: None of the supplier/consumer drivers is present.
>>>  * @DL_STATE_AVAILABLE: The supplier driver is present, but the consumer
>>> is not.
>>>
>>>> So, one idea I have is to, on panel/bridge attach, verify if the
>>>> consumer is in its probe, and only create the link if that is the
>>>> case. So, the link would be optional, but it would all be automatic.
>>> Making it automatic looks tempting, but also error prone. In case of
>>> component framework bind callbacks can be called from probe of any
>>> component, so sometimes it can be called also from exynos-dsi probe,
>>> sometimes not (depending on order of probing, which we cannot rely on).
>>> So in some cases we will end-up with links, sometimes without. Ie
>>> following scenarios are possible in drm_panel_attach:
>>> 1. exynos-dsi bound, panel during probe.
>>> 2. exynos-dsi during probe, panel during probe.
>> 2. exynos-dsi during probe, panel bound? Or is this case 3, and 2 happens
>> when drivers probe in parallel?
> 
> Panel is always probed not earlier than the end of exynos-dsi bind, so
> only scenarios 1 and 2 should be possible.
> 
>> Whichever, you are right, I naively thought that the bind happened
>> after the probe of all devices, but naturally it happens as part of
>> the last device to register its component, and that normally happens
>> during its probe.
>>
>> Sigh. So, scratch that, I guess we need a flag...

I looked into that, and it seems very fragile to get the flag to be
correct for all cases. That road is bound to produce lots of bugs, and
it will be hard to get it right. In short, I would not descend into that
particular rabbit hole.

Can it be assumed that the drm_device of the encoder in drm_bridge_attach
is a master component, if the drm "cluster" is componentized? Are there
currently other ways of handling drm driver binding changes than
components?

If the answers are "yes" and "no", it might be possible to check if
encoder->dev is a master component and only establish the device link if
it isn't. All it would take is to add a component_device_is_master()
query function to drivers/base/component.c

Something like this (untested):

bool component_device_is_master(struct device *dev)
{
	struct master *m;

	mutex_lock(&component_mutex);
	m = __master_find(dev, NULL);
	mutex_unlock(&component_mutex);

	return !!m;
}
EXPORT_SYMBOL_GPL(component_device_is_master);

And then check that before calling device_link_add().

Cheers,
Peter

Andrzej Hajda May 22, 2018, 6:29 a.m. UTC | #9

On 21.05.2018 23:56, Peter Rosin wrote:
> On 2018-05-21 11:21, Andrzej Hajda wrote:
>> On 21.05.2018 10:53, Peter Rosin wrote:
>>> On 2018-05-21 10:15, Andrzej Hajda wrote:
>>>> On 19.05.2018 18:48, Peter Rosin wrote:
>>>>> On 2018-05-18 13:51, Andrzej Hajda wrote:
>>>>>> On 26.04.2018 10:07, Jyri Sarha wrote:
>>>>>>> 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 <[e-mail]>
>>>>>>> Reviewed-by: Eric Anholt <[e-mail]>
>>>>>> Hi Jyri,
>>>>>>
>>>>>> This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
>>>>>> using exynos_drm_dsi and dsi panels.
>>>>>> Exynos-DSI properly handles panels unbind - ie references to panel are
>>>>>> properly removed on panels removal and respective drm_connector enters
>>>>>> disconnected state, without destroying whole drm device.
>>>>>> And again on panel driver re-bind drm_panel is properly attached to
>>>>>> exynos-dsi and panel pipeline is working again.
>>>>>> This patch will break this behavior, ie it will destroy whole drm device.
>>>>>>
>>>>>> Making device_links for panels optional seems to me the best solution,
>>>>>> what do you think?
>>>>>>
>>>>>> The funny thing is that due to bug in device link code, your patch has
>>>>>> no effect on these platforms. So it does not hurt these platform yet,
>>>>>> but the bug will be fixed sooner or later.
>>>>> Ah, that's a pretty strong hint that we are doing something unusual. So,
>>>>> I had a deeper look and I think that device-links (with state, i.e. not
>>>>> DL_FLAG_STATELESS links) are assumed to be created by the .probe function
>>>>> of either the consumer or the supplier. Then it seems to works as expected.
>>>>> And that makes some sense too, because a link indicates that there exist
>>>>> a dependency between the two and that the consumer cannot really exist
>>>>> without the supplier. This is also what happens for the drm devices
>>>>> (panel/bridge consumers) Jyri and I are working with; they call panel or
>>>>> bridge attach during their probe. But this is not the case for exynos,
>>>>> which calls panel/bridge attach at some later stage, and that obviously
>>>>> violates the assumption that the device-link consumer cannot exist w/o
>>>>> the supplier ("obviously" since the drm driver has managed to successfully
>>>>> probe without the supplier).
>>>>>
>>>>> So, when the panel/bridge supplier is probed after the consumer is
>>>>> bound, the link starts out as DL_STATE_DORMANT, and progresses to
>>>>> DL_STATE_AVAILABLE once the panel/bridge has finished the probe. This is
>>>>> not what *we* want.
>>>> And this is also incorrect from Documentation PoV:
>>>>  * @DL_STATE_DORMANT: None of the supplier/consumer drivers is present.
>>>>  * @DL_STATE_AVAILABLE: The supplier driver is present, but the consumer
>>>> is not.
>>>>
>>>>> So, one idea I have is to, on panel/bridge attach, verify if the
>>>>> consumer is in its probe, and only create the link if that is the
>>>>> case. So, the link would be optional, but it would all be automatic.
>>>> Making it automatic looks tempting, but also error prone. In case of
>>>> component framework bind callbacks can be called from probe of any
>>>> component, so sometimes it can be called also from exynos-dsi probe,
>>>> sometimes not (depending on order of probing, which we cannot rely on).
>>>> So in some cases we will end-up with links, sometimes without. Ie
>>>> following scenarios are possible in drm_panel_attach:
>>>> 1. exynos-dsi bound, panel during probe.
>>>> 2. exynos-dsi during probe, panel during probe.
>>> 2. exynos-dsi during probe, panel bound? Or is this case 3, and 2 happens
>>> when drivers probe in parallel?
>> Panel is always probed not earlier than the end of exynos-dsi bind, so
>> only scenarios 1 and 2 should be possible.
>>
>>> Whichever, you are right, I naively thought that the bind happened
>>> after the probe of all devices, but naturally it happens as part of
>>> the last device to register its component, and that normally happens
>>> during its probe.
>>>
>>> Sigh. So, scratch that, I guess we need a flag...
> I looked into that, and it seems very fragile to get the flag to be
> correct for all cases. That road is bound to produce lots of bugs, and
> it will be hard to get it right. In short, I would not descend into that
> particular rabbit hole.
>
> Can it be assumed that the drm_device of the encoder in drm_bridge_attach
> is a master component, if the drm "cluster" is componentized?

I wouldn't call it assumption, I would say rather it is a common practice.

> Are there
> currently other ways of handling drm driver binding changes than
> components?

No, there are drivers which do not use components, but call
drm_panel_attach:
$ for d in drivers/gpu/drm/*/; do git grep -q 'struct component_ops' $d
&& continue; git grep -q drm_panel_attach $d && echo $d; done
drivers/gpu/drm/bridge/
drivers/gpu/drm/fsl-dcu/
drivers/gpu/drm/mxsfb/
drivers/gpu/drm/rcar-du/
drivers/gpu/drm/tegra/

Components are optional.

>
> If the answers are "yes" and "no", it might be possible to check if
> encoder->dev is a master component and only establish the device link if
> it isn't. All it would take is to add a component_device_is_master()
> query function to drivers/base/component.c
> Something like this (untested):
>
> bool component_device_is_master(struct device *dev)
> {
> 	struct master *m;
>
> 	mutex_lock(&component_mutex);
> 	m = __master_find(dev, NULL);
> 	mutex_unlock(&component_mutex);
>
> 	return !!m;
> }
> EXPORT_SYMBOL_GPL(component_device_is_master);
>
> And then check that before calling device_link_add().

Why do not use simpler solution, just create function
drm_panel_attach_without_link and call it explicitly from drivers, or
add additional flag argument to either drm_panel_attach either to
drm_panel structure? Maybe it will not be prettier but will be more
explicit.

Regards
Andrzej


>
> Cheers,
> Peter
>
>
>

Peter Rosin May 22, 2018, 7:36 a.m. UTC | #10

On 2018-05-22 08:29, Andrzej Hajda wrote:
> On 21.05.2018 23:56, Peter Rosin wrote:
>> On 2018-05-21 11:21, Andrzej Hajda wrote:
>>> On 21.05.2018 10:53, Peter Rosin wrote:
>>>> On 2018-05-21 10:15, Andrzej Hajda wrote:
>>>>> On 19.05.2018 18:48, Peter Rosin wrote:
>>>>>> On 2018-05-18 13:51, Andrzej Hajda wrote:
>>>>>>> On 26.04.2018 10:07, Jyri Sarha wrote:
>>>>>>>> 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 <[e-mail]>
>>>>>>>> Reviewed-by: Eric Anholt <[e-mail]>
>>>>>>> Hi Jyri,
>>>>>>>
>>>>>>> This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
>>>>>>> using exynos_drm_dsi and dsi panels.
>>>>>>> Exynos-DSI properly handles panels unbind - ie references to panel are
>>>>>>> properly removed on panels removal and respective drm_connector enters
>>>>>>> disconnected state, without destroying whole drm device.
>>>>>>> And again on panel driver re-bind drm_panel is properly attached to
>>>>>>> exynos-dsi and panel pipeline is working again.
>>>>>>> This patch will break this behavior, ie it will destroy whole drm device.
>>>>>>>
>>>>>>> Making device_links for panels optional seems to me the best solution,
>>>>>>> what do you think?
>>>>>>>
>>>>>>> The funny thing is that due to bug in device link code, your patch has
>>>>>>> no effect on these platforms. So it does not hurt these platform yet,
>>>>>>> but the bug will be fixed sooner or later.
>>>>>> Ah, that's a pretty strong hint that we are doing something unusual. So,
>>>>>> I had a deeper look and I think that device-links (with state, i.e. not
>>>>>> DL_FLAG_STATELESS links) are assumed to be created by the .probe function
>>>>>> of either the consumer or the supplier. Then it seems to works as expected.
>>>>>> And that makes some sense too, because a link indicates that there exist
>>>>>> a dependency between the two and that the consumer cannot really exist
>>>>>> without the supplier. This is also what happens for the drm devices
>>>>>> (panel/bridge consumers) Jyri and I are working with; they call panel or
>>>>>> bridge attach during their probe. But this is not the case for exynos,
>>>>>> which calls panel/bridge attach at some later stage, and that obviously
>>>>>> violates the assumption that the device-link consumer cannot exist w/o
>>>>>> the supplier ("obviously" since the drm driver has managed to successfully
>>>>>> probe without the supplier).
>>>>>>
>>>>>> So, when the panel/bridge supplier is probed after the consumer is
>>>>>> bound, the link starts out as DL_STATE_DORMANT, and progresses to
>>>>>> DL_STATE_AVAILABLE once the panel/bridge has finished the probe. This is
>>>>>> not what *we* want.
>>>>> And this is also incorrect from Documentation PoV:
>>>>>  * @DL_STATE_DORMANT: None of the supplier/consumer drivers is present.
>>>>>  * @DL_STATE_AVAILABLE: The supplier driver is present, but the consumer
>>>>> is not.
>>>>>
>>>>>> So, one idea I have is to, on panel/bridge attach, verify if the
>>>>>> consumer is in its probe, and only create the link if that is the
>>>>>> case. So, the link would be optional, but it would all be automatic.
>>>>> Making it automatic looks tempting, but also error prone. In case of
>>>>> component framework bind callbacks can be called from probe of any
>>>>> component, so sometimes it can be called also from exynos-dsi probe,
>>>>> sometimes not (depending on order of probing, which we cannot rely on).
>>>>> So in some cases we will end-up with links, sometimes without. Ie
>>>>> following scenarios are possible in drm_panel_attach:
>>>>> 1. exynos-dsi bound, panel during probe.
>>>>> 2. exynos-dsi during probe, panel during probe.
>>>> 2. exynos-dsi during probe, panel bound? Or is this case 3, and 2 happens
>>>> when drivers probe in parallel?
>>> Panel is always probed not earlier than the end of exynos-dsi bind, so
>>> only scenarios 1 and 2 should be possible.
>>>
>>>> Whichever, you are right, I naively thought that the bind happened
>>>> after the probe of all devices, but naturally it happens as part of
>>>> the last device to register its component, and that normally happens
>>>> during its probe.
>>>>
>>>> Sigh. So, scratch that, I guess we need a flag...
>> I looked into that, and it seems very fragile to get the flag to be
>> correct for all cases. That road is bound to produce lots of bugs, and
>> it will be hard to get it right. In short, I would not descend into that
>> particular rabbit hole.
>>
>> Can it be assumed that the drm_device of the encoder in drm_bridge_attach
>> is a master component, if the drm "cluster" is componentized?
> 
> I wouldn't call it assumption, I would say rather it is a common practice.
> 
>> Are there
>> currently other ways of handling drm driver binding changes than
>> components?
> 
> No, there are drivers which do not use components, but call
> drm_panel_attach:
> $ for d in drivers/gpu/drm/*/; do git grep -q 'struct component_ops' $d
> && continue; git grep -q drm_panel_attach $d && echo $d; done
> drivers/gpu/drm/bridge/
> drivers/gpu/drm/fsl-dcu/
> drivers/gpu/drm/mxsfb/
> drivers/gpu/drm/rcar-du/
> drivers/gpu/drm/tegra/
> 
> Components are optional.

Yes, of course components are optional. The question was if there are
currently *other* ways (i.e. not the component framework, not device links)
of dealing with disappearing panels/bridges. However, see below, the
question is irrelevant with my below suggestion.

>>
>> If the answers are "yes" and "no", it might be possible to check if
>> encoder->dev is a master component and only establish the device link if
>> it isn't. All it would take is to add a component_device_is_master()
>> query function to drivers/base/component.c
>> Something like this (untested):
>>
>> bool component_device_is_master(struct device *dev)
>> {
>> 	struct master *m;
>>
>> 	mutex_lock(&component_mutex);
>> 	m = __master_find(dev, NULL);
>> 	mutex_unlock(&component_mutex);
>>
>> 	return !!m;
>> }
>> EXPORT_SYMBOL_GPL(component_device_is_master);
>>
>> And then check that before calling device_link_add().
> 
> Why do not use simpler solution, just create function
> drm_panel_attach_without_link and call it explicitly from drivers, or
> add additional flag argument to either drm_panel_attach either to
> drm_panel structure? Maybe it will not be prettier but will be more
> explicit.

Because, if you take bridges into account as well and add a
drm_bridge_attach_without_link, which function do you call from e.g.
drm_simple_display_pipe_attach_bridge()? Sure, you can probably verify
the callers and come to the conclusion that you maybe always want the
link, currently. Same question for the tda998x driver, which function
to use for attaching the bridge would have to depend on how the driver
was bound (as a component or not, yes I know, currently tda998x only
does component, but the whole reason I'm involved is that I want it
to also register as a standalone drm_bridge). Not doing this with some
automatic check simply leads to combinatorial hell.

Maybe a better solution is for the drm driver to record whether it
wants links in its struct drm_device, before calling drm_dev_register?
That's also explicit. drm_panel_attach/drm_bridge_attach could then
easily determine if the link should be made. IMHO, it would also be
quite easy to set this correctly for any given drm_device.

Cheers,
Peter

Peter Rosin May 22, 2018, 8:37 a.m. UTC | #11

On 2018-05-22 09:36, Peter Rosin wrote:
> On 2018-05-22 08:29, Andrzej Hajda wrote:
>> On 21.05.2018 23:56, Peter Rosin wrote:
>>> On 2018-05-21 11:21, Andrzej Hajda wrote:
>>>> On 21.05.2018 10:53, Peter Rosin wrote:
>>>>> On 2018-05-21 10:15, Andrzej Hajda wrote:
>>>>>> On 19.05.2018 18:48, Peter Rosin wrote:
>>>>>>> On 2018-05-18 13:51, Andrzej Hajda wrote:
>>>>>>>> On 26.04.2018 10:07, Jyri Sarha wrote:
>>>>>>>>> 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 <[e-mail]>
>>>>>>>>> Reviewed-by: Eric Anholt <[e-mail]>
>>>>>>>> Hi Jyri,
>>>>>>>>
>>>>>>>> This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
>>>>>>>> using exynos_drm_dsi and dsi panels.
>>>>>>>> Exynos-DSI properly handles panels unbind - ie references to panel are
>>>>>>>> properly removed on panels removal and respective drm_connector enters
>>>>>>>> disconnected state, without destroying whole drm device.
>>>>>>>> And again on panel driver re-bind drm_panel is properly attached to
>>>>>>>> exynos-dsi and panel pipeline is working again.
>>>>>>>> This patch will break this behavior, ie it will destroy whole drm device.
>>>>>>>>
>>>>>>>> Making device_links for panels optional seems to me the best solution,
>>>>>>>> what do you think?
>>>>>>>>
>>>>>>>> The funny thing is that due to bug in device link code, your patch has
>>>>>>>> no effect on these platforms. So it does not hurt these platform yet,
>>>>>>>> but the bug will be fixed sooner or later.
>>>>>>> Ah, that's a pretty strong hint that we are doing something unusual. So,
>>>>>>> I had a deeper look and I think that device-links (with state, i.e. not
>>>>>>> DL_FLAG_STATELESS links) are assumed to be created by the .probe function
>>>>>>> of either the consumer or the supplier. Then it seems to works as expected.
>>>>>>> And that makes some sense too, because a link indicates that there exist
>>>>>>> a dependency between the two and that the consumer cannot really exist
>>>>>>> without the supplier. This is also what happens for the drm devices
>>>>>>> (panel/bridge consumers) Jyri and I are working with; they call panel or
>>>>>>> bridge attach during their probe. But this is not the case for exynos,
>>>>>>> which calls panel/bridge attach at some later stage, and that obviously
>>>>>>> violates the assumption that the device-link consumer cannot exist w/o
>>>>>>> the supplier ("obviously" since the drm driver has managed to successfully
>>>>>>> probe without the supplier).
>>>>>>>
>>>>>>> So, when the panel/bridge supplier is probed after the consumer is
>>>>>>> bound, the link starts out as DL_STATE_DORMANT, and progresses to
>>>>>>> DL_STATE_AVAILABLE once the panel/bridge has finished the probe. This is
>>>>>>> not what *we* want.
>>>>>> And this is also incorrect from Documentation PoV:
>>>>>>  * @DL_STATE_DORMANT: None of the supplier/consumer drivers is present.
>>>>>>  * @DL_STATE_AVAILABLE: The supplier driver is present, but the consumer
>>>>>> is not.
>>>>>>
>>>>>>> So, one idea I have is to, on panel/bridge attach, verify if the
>>>>>>> consumer is in its probe, and only create the link if that is the
>>>>>>> case. So, the link would be optional, but it would all be automatic.
>>>>>> Making it automatic looks tempting, but also error prone. In case of
>>>>>> component framework bind callbacks can be called from probe of any
>>>>>> component, so sometimes it can be called also from exynos-dsi probe,
>>>>>> sometimes not (depending on order of probing, which we cannot rely on).
>>>>>> So in some cases we will end-up with links, sometimes without. Ie
>>>>>> following scenarios are possible in drm_panel_attach:
>>>>>> 1. exynos-dsi bound, panel during probe.
>>>>>> 2. exynos-dsi during probe, panel during probe.
>>>>> 2. exynos-dsi during probe, panel bound? Or is this case 3, and 2 happens
>>>>> when drivers probe in parallel?
>>>> Panel is always probed not earlier than the end of exynos-dsi bind, so
>>>> only scenarios 1 and 2 should be possible.
>>>>
>>>>> Whichever, you are right, I naively thought that the bind happened
>>>>> after the probe of all devices, but naturally it happens as part of
>>>>> the last device to register its component, and that normally happens
>>>>> during its probe.
>>>>>
>>>>> Sigh. So, scratch that, I guess we need a flag...
>>> I looked into that, and it seems very fragile to get the flag to be
>>> correct for all cases. That road is bound to produce lots of bugs, and
>>> it will be hard to get it right. In short, I would not descend into that
>>> particular rabbit hole.
>>>
>>> Can it be assumed that the drm_device of the encoder in drm_bridge_attach
>>> is a master component, if the drm "cluster" is componentized?
>>
>> I wouldn't call it assumption, I would say rather it is a common practice.
>>
>>> Are there
>>> currently other ways of handling drm driver binding changes than
>>> components?
>>
>> No, there are drivers which do not use components, but call
>> drm_panel_attach:
>> $ for d in drivers/gpu/drm/*/; do git grep -q 'struct component_ops' $d
>> && continue; git grep -q drm_panel_attach $d && echo $d; done
>> drivers/gpu/drm/bridge/
>> drivers/gpu/drm/fsl-dcu/
>> drivers/gpu/drm/mxsfb/
>> drivers/gpu/drm/rcar-du/
>> drivers/gpu/drm/tegra/
>>
>> Components are optional.
> 
> Yes, of course components are optional. The question was if there are
> currently *other* ways (i.e. not the component framework, not device links)
> of dealing with disappearing panels/bridges. However, see below, the
> question is irrelevant with my below suggestion.
> 
>>>
>>> If the answers are "yes" and "no", it might be possible to check if
>>> encoder->dev is a master component and only establish the device link if
>>> it isn't. All it would take is to add a component_device_is_master()
>>> query function to drivers/base/component.c
>>> Something like this (untested):
>>>
>>> bool component_device_is_master(struct device *dev)
>>> {
>>> 	struct master *m;
>>>
>>> 	mutex_lock(&component_mutex);
>>> 	m = __master_find(dev, NULL);
>>> 	mutex_unlock(&component_mutex);
>>>
>>> 	return !!m;
>>> }
>>> EXPORT_SYMBOL_GPL(component_device_is_master);
>>>
>>> And then check that before calling device_link_add().
>>
>> Why do not use simpler solution, just create function
>> drm_panel_attach_without_link and call it explicitly from drivers, or
>> add additional flag argument to either drm_panel_attach either to
>> drm_panel structure? Maybe it will not be prettier but will be more
>> explicit.
> 
> Because, if you take bridges into account as well and add a
> drm_bridge_attach_without_link, which function do you call from e.g.
> drm_simple_display_pipe_attach_bridge()? Sure, you can probably verify
> the callers and come to the conclusion that you maybe always want the
> link, currently. Same question for the tda998x driver, which function
> to use for attaching the bridge would have to depend on how the driver
> was bound (as a component or not, yes I know, currently tda998x only
> does component, but the whole reason I'm involved is that I want it
> to also register as a standalone drm_bridge). Not doing this with some
> automatic check simply leads to combinatorial hell.
> 
> Maybe a better solution is for the drm driver to record whether it
> wants links in its struct drm_device, before calling drm_dev_register?
> That's also explicit. drm_panel_attach/drm_bridge_attach could then
> easily determine if the link should be made. IMHO, it would also be
> quite easy to set this correctly for any given drm_device.

While pursuing that idea, I found several examples of drivers that
deal with both component-parts not wanting the device-link and non-
component-parts wanting the device-link. So, a dead end.

Cheers,
Peter

Andrzej Hajda May 22, 2018, 9:45 a.m. UTC | #12

On 22.05.2018 09:36, Peter Rosin wrote:
> On 2018-05-22 08:29, Andrzej Hajda wrote:
>> On 21.05.2018 23:56, Peter Rosin wrote:
>>> On 2018-05-21 11:21, Andrzej Hajda wrote:
>>>> On 21.05.2018 10:53, Peter Rosin wrote:
>>>>> On 2018-05-21 10:15, Andrzej Hajda wrote:
>>>>>> On 19.05.2018 18:48, Peter Rosin wrote:
>>>>>>> On 2018-05-18 13:51, Andrzej Hajda wrote:
>>>>>>>> On 26.04.2018 10:07, Jyri Sarha wrote:
>>>>>>>>> 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 <[e-mail]>
>>>>>>>>> Reviewed-by: Eric Anholt <[e-mail]>
>>>>>>>> Hi Jyri,
>>>>>>>>
>>>>>>>> This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
>>>>>>>> using exynos_drm_dsi and dsi panels.
>>>>>>>> Exynos-DSI properly handles panels unbind - ie references to panel are
>>>>>>>> properly removed on panels removal and respective drm_connector enters
>>>>>>>> disconnected state, without destroying whole drm device.
>>>>>>>> And again on panel driver re-bind drm_panel is properly attached to
>>>>>>>> exynos-dsi and panel pipeline is working again.
>>>>>>>> This patch will break this behavior, ie it will destroy whole drm device.
>>>>>>>>
>>>>>>>> Making device_links for panels optional seems to me the best solution,
>>>>>>>> what do you think?
>>>>>>>>
>>>>>>>> The funny thing is that due to bug in device link code, your patch has
>>>>>>>> no effect on these platforms. So it does not hurt these platform yet,
>>>>>>>> but the bug will be fixed sooner or later.
>>>>>>> Ah, that's a pretty strong hint that we are doing something unusual. So,
>>>>>>> I had a deeper look and I think that device-links (with state, i.e. not
>>>>>>> DL_FLAG_STATELESS links) are assumed to be created by the .probe function
>>>>>>> of either the consumer or the supplier. Then it seems to works as expected.
>>>>>>> And that makes some sense too, because a link indicates that there exist
>>>>>>> a dependency between the two and that the consumer cannot really exist
>>>>>>> without the supplier. This is also what happens for the drm devices
>>>>>>> (panel/bridge consumers) Jyri and I are working with; they call panel or
>>>>>>> bridge attach during their probe. But this is not the case for exynos,
>>>>>>> which calls panel/bridge attach at some later stage, and that obviously
>>>>>>> violates the assumption that the device-link consumer cannot exist w/o
>>>>>>> the supplier ("obviously" since the drm driver has managed to successfully
>>>>>>> probe without the supplier).
>>>>>>>
>>>>>>> So, when the panel/bridge supplier is probed after the consumer is
>>>>>>> bound, the link starts out as DL_STATE_DORMANT, and progresses to
>>>>>>> DL_STATE_AVAILABLE once the panel/bridge has finished the probe. This is
>>>>>>> not what *we* want.
>>>>>> And this is also incorrect from Documentation PoV:
>>>>>>  * @DL_STATE_DORMANT: None of the supplier/consumer drivers is present.
>>>>>>  * @DL_STATE_AVAILABLE: The supplier driver is present, but the consumer
>>>>>> is not.
>>>>>>
>>>>>>> So, one idea I have is to, on panel/bridge attach, verify if the
>>>>>>> consumer is in its probe, and only create the link if that is the
>>>>>>> case. So, the link would be optional, but it would all be automatic.
>>>>>> Making it automatic looks tempting, but also error prone. In case of
>>>>>> component framework bind callbacks can be called from probe of any
>>>>>> component, so sometimes it can be called also from exynos-dsi probe,
>>>>>> sometimes not (depending on order of probing, which we cannot rely on).
>>>>>> So in some cases we will end-up with links, sometimes without. Ie
>>>>>> following scenarios are possible in drm_panel_attach:
>>>>>> 1. exynos-dsi bound, panel during probe.
>>>>>> 2. exynos-dsi during probe, panel during probe.
>>>>> 2. exynos-dsi during probe, panel bound? Or is this case 3, and 2 happens
>>>>> when drivers probe in parallel?
>>>> Panel is always probed not earlier than the end of exynos-dsi bind, so
>>>> only scenarios 1 and 2 should be possible.
>>>>
>>>>> Whichever, you are right, I naively thought that the bind happened
>>>>> after the probe of all devices, but naturally it happens as part of
>>>>> the last device to register its component, and that normally happens
>>>>> during its probe.
>>>>>
>>>>> Sigh. So, scratch that, I guess we need a flag...
>>> I looked into that, and it seems very fragile to get the flag to be
>>> correct for all cases. That road is bound to produce lots of bugs, and
>>> it will be hard to get it right. In short, I would not descend into that
>>> particular rabbit hole.
>>>
>>> Can it be assumed that the drm_device of the encoder in drm_bridge_attach
>>> is a master component, if the drm "cluster" is componentized?
>> I wouldn't call it assumption, I would say rather it is a common practice.
>>
>>> Are there
>>> currently other ways of handling drm driver binding changes than
>>> components?
>> No, there are drivers which do not use components, but call
>> drm_panel_attach:
>> $ for d in drivers/gpu/drm/*/; do git grep -q 'struct component_ops' $d
>> && continue; git grep -q drm_panel_attach $d && echo $d; done
>> drivers/gpu/drm/bridge/
>> drivers/gpu/drm/fsl-dcu/
>> drivers/gpu/drm/mxsfb/
>> drivers/gpu/drm/rcar-du/
>> drivers/gpu/drm/tegra/
>>
>> Components are optional.
> Yes, of course components are optional. The question was if there are
> currently *other* ways (i.e. not the component framework, not device links)
> of dealing with disappearing panels/bridges. However, see below, the
> question is irrelevant with my below suggestion.
>
>>> If the answers are "yes" and "no", it might be possible to check if
>>> encoder->dev is a master component and only establish the device link if
>>> it isn't. All it would take is to add a component_device_is_master()
>>> query function to drivers/base/component.c
>>> Something like this (untested):
>>>
>>> bool component_device_is_master(struct device *dev)
>>> {
>>> 	struct master *m;
>>>
>>> 	mutex_lock(&component_mutex);
>>> 	m = __master_find(dev, NULL);
>>> 	mutex_unlock(&component_mutex);
>>>
>>> 	return !!m;
>>> }
>>> EXPORT_SYMBOL_GPL(component_device_is_master);
>>>
>>> And then check that before calling device_link_add().
>> Why do not use simpler solution, just create function
>> drm_panel_attach_without_link and call it explicitly from drivers, or
>> add additional flag argument to either drm_panel_attach either to
>> drm_panel structure? Maybe it will not be prettier but will be more
>> explicit.
> Because, if you take bridges into account as well and add a
> drm_bridge_attach_without_link, which function do you call from e.g.
> drm_simple_display_pipe_attach_bridge()? Sure, you can probably verify
> the callers and come to the conclusion that you maybe always want the
> link, currently. Same question for the tda998x driver, which function
> to use for attaching the bridge would have to depend on how the driver
> was bound (as a component or not, yes I know, currently tda998x only
> does component, but the whole reason I'm involved is that I want it
> to also register as a standalone drm_bridge). Not doing this with some
> automatic check simply leads to combinatorial hell.

I was focused on panels, which are managed not by drm core, but by
upstream pipeline element (bridge/encoder). For them decision about
using device links should be made by the manager not drm core, I guess.
In case of bridges it is different, bridges are attached by upstream
elements, but then they are tracked/managed/detached only by drm core
(at least this is current practice).  If somebody wants to implement
dynamic bridges this pattern cannot be used, ie bridge should be
attached/detached by upstream element, like in case of panels.

>
> Maybe a better solution is for the drm driver to record whether it
> wants links in its struct drm_device, before calling drm_dev_register?
> That's also explicit. drm_panel_attach/drm_bridge_attach could then
> easily determine if the link should be made. IMHO, it would also be
> quite easy to set this correctly for any given drm_device.

As I said earlier I think decision about link creation should be always
up to element which performs attachment, It only knows what should be
attached, so it is the best candidate to handle dynamic unbind and
re-bind of the downstream element.

Regards
Andrzej

>
> Cheers,
> Peter
>
>
>

Peter Rosin May 22, 2018, 3:03 p.m. UTC | #13

On 2018-05-22 11:45, Andrzej Hajda wrote:
> On 22.05.2018 09:36, Peter Rosin wrote:
>> On 2018-05-22 08:29, Andrzej Hajda wrote:
>>> On 21.05.2018 23:56, Peter Rosin wrote:
>>>> On 2018-05-21 11:21, Andrzej Hajda wrote:
>>>>> On 21.05.2018 10:53, Peter Rosin wrote:
>>>>>> On 2018-05-21 10:15, Andrzej Hajda wrote:
>>>>>>> On 19.05.2018 18:48, Peter Rosin wrote:
>>>>>>>> On 2018-05-18 13:51, Andrzej Hajda wrote:
>>>>>>>>> On 26.04.2018 10:07, Jyri Sarha wrote:
>>>>>>>>>> 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 <[e-mail]>
>>>>>>>>>> Reviewed-by: Eric Anholt <[e-mail]>
>>>>>>>>> Hi Jyri,
>>>>>>>>>
>>>>>>>>> This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
>>>>>>>>> using exynos_drm_dsi and dsi panels.
>>>>>>>>> Exynos-DSI properly handles panels unbind - ie references to panel are
>>>>>>>>> properly removed on panels removal and respective drm_connector enters
>>>>>>>>> disconnected state, without destroying whole drm device.
>>>>>>>>> And again on panel driver re-bind drm_panel is properly attached to
>>>>>>>>> exynos-dsi and panel pipeline is working again.
>>>>>>>>> This patch will break this behavior, ie it will destroy whole drm device.
>>>>>>>>>
>>>>>>>>> Making device_links for panels optional seems to me the best solution,
>>>>>>>>> what do you think?
>>>>>>>>>
>>>>>>>>> The funny thing is that due to bug in device link code, your patch has
>>>>>>>>> no effect on these platforms. So it does not hurt these platform yet,
>>>>>>>>> but the bug will be fixed sooner or later.
>>>>>>>> Ah, that's a pretty strong hint that we are doing something unusual. So,
>>>>>>>> I had a deeper look and I think that device-links (with state, i.e. not
>>>>>>>> DL_FLAG_STATELESS links) are assumed to be created by the .probe function
>>>>>>>> of either the consumer or the supplier. Then it seems to works as expected.
>>>>>>>> And that makes some sense too, because a link indicates that there exist
>>>>>>>> a dependency between the two and that the consumer cannot really exist
>>>>>>>> without the supplier. This is also what happens for the drm devices
>>>>>>>> (panel/bridge consumers) Jyri and I are working with; they call panel or
>>>>>>>> bridge attach during their probe. But this is not the case for exynos,
>>>>>>>> which calls panel/bridge attach at some later stage, and that obviously
>>>>>>>> violates the assumption that the device-link consumer cannot exist w/o
>>>>>>>> the supplier ("obviously" since the drm driver has managed to successfully
>>>>>>>> probe without the supplier).
>>>>>>>>
>>>>>>>> So, when the panel/bridge supplier is probed after the consumer is
>>>>>>>> bound, the link starts out as DL_STATE_DORMANT, and progresses to
>>>>>>>> DL_STATE_AVAILABLE once the panel/bridge has finished the probe. This is
>>>>>>>> not what *we* want.
>>>>>>> And this is also incorrect from Documentation PoV:
>>>>>>>  * @DL_STATE_DORMANT: None of the supplier/consumer drivers is present.
>>>>>>>  * @DL_STATE_AVAILABLE: The supplier driver is present, but the consumer
>>>>>>> is not.
>>>>>>>
>>>>>>>> So, one idea I have is to, on panel/bridge attach, verify if the
>>>>>>>> consumer is in its probe, and only create the link if that is the
>>>>>>>> case. So, the link would be optional, but it would all be automatic.
>>>>>>> Making it automatic looks tempting, but also error prone. In case of
>>>>>>> component framework bind callbacks can be called from probe of any
>>>>>>> component, so sometimes it can be called also from exynos-dsi probe,
>>>>>>> sometimes not (depending on order of probing, which we cannot rely on).
>>>>>>> So in some cases we will end-up with links, sometimes without. Ie
>>>>>>> following scenarios are possible in drm_panel_attach:
>>>>>>> 1. exynos-dsi bound, panel during probe.
>>>>>>> 2. exynos-dsi during probe, panel during probe.
>>>>>> 2. exynos-dsi during probe, panel bound? Or is this case 3, and 2 happens
>>>>>> when drivers probe in parallel?
>>>>> Panel is always probed not earlier than the end of exynos-dsi bind, so
>>>>> only scenarios 1 and 2 should be possible.
>>>>>
>>>>>> Whichever, you are right, I naively thought that the bind happened
>>>>>> after the probe of all devices, but naturally it happens as part of
>>>>>> the last device to register its component, and that normally happens
>>>>>> during its probe.
>>>>>>
>>>>>> Sigh. So, scratch that, I guess we need a flag...
>>>> I looked into that, and it seems very fragile to get the flag to be
>>>> correct for all cases. That road is bound to produce lots of bugs, and
>>>> it will be hard to get it right. In short, I would not descend into that
>>>> particular rabbit hole.
>>>>
>>>> Can it be assumed that the drm_device of the encoder in drm_bridge_attach
>>>> is a master component, if the drm "cluster" is componentized?
>>> I wouldn't call it assumption, I would say rather it is a common practice.
>>>
>>>> Are there
>>>> currently other ways of handling drm driver binding changes than
>>>> components?
>>> No, there are drivers which do not use components, but call
>>> drm_panel_attach:
>>> $ for d in drivers/gpu/drm/*/; do git grep -q 'struct component_ops' $d
>>> && continue; git grep -q drm_panel_attach $d && echo $d; done
>>> drivers/gpu/drm/bridge/
>>> drivers/gpu/drm/fsl-dcu/
>>> drivers/gpu/drm/mxsfb/
>>> drivers/gpu/drm/rcar-du/
>>> drivers/gpu/drm/tegra/
>>>
>>> Components are optional.
>> Yes, of course components are optional. The question was if there are
>> currently *other* ways (i.e. not the component framework, not device links)
>> of dealing with disappearing panels/bridges. However, see below, the
>> question is irrelevant with my below suggestion.
>>
>>>> If the answers are "yes" and "no", it might be possible to check if
>>>> encoder->dev is a master component and only establish the device link if
>>>> it isn't. All it would take is to add a component_device_is_master()
>>>> query function to drivers/base/component.c
>>>> Something like this (untested):
>>>>
>>>> bool component_device_is_master(struct device *dev)
>>>> {
>>>> 	struct master *m;
>>>>
>>>> 	mutex_lock(&component_mutex);
>>>> 	m = __master_find(dev, NULL);
>>>> 	mutex_unlock(&component_mutex);
>>>>
>>>> 	return !!m;
>>>> }
>>>> EXPORT_SYMBOL_GPL(component_device_is_master);
>>>>
>>>> And then check that before calling device_link_add().
>>> Why do not use simpler solution, just create function
>>> drm_panel_attach_without_link and call it explicitly from drivers, or
>>> add additional flag argument to either drm_panel_attach either to
>>> drm_panel structure? Maybe it will not be prettier but will be more
>>> explicit.
>> Because, if you take bridges into account as well and add a
>> drm_bridge_attach_without_link, which function do you call from e.g.
>> drm_simple_display_pipe_attach_bridge()? Sure, you can probably verify
>> the callers and come to the conclusion that you maybe always want the
>> link, currently. Same question for the tda998x driver, which function
>> to use for attaching the bridge would have to depend on how the driver
>> was bound (as a component or not, yes I know, currently tda998x only
>> does component, but the whole reason I'm involved is that I want it
>> to also register as a standalone drm_bridge). Not doing this with some
>> automatic check simply leads to combinatorial hell.
> 
> I was focused on panels, which are managed not by drm core, but by
> upstream pipeline element (bridge/encoder). For them decision about
> using device links should be made by the manager not drm core, I guess.
> In case of bridges it is different, bridges are attached by upstream
> elements, but then they are tracked/managed/detached only by drm core
> (at least this is current practice).  If somebody wants to implement
> dynamic bridges this pattern cannot be used, ie bridge should be
> attached/detached by upstream element, like in case of panels.
> 
>>
>> Maybe a better solution is for the drm driver to record whether it
>> wants links in its struct drm_device, before calling drm_dev_register?
>> That's also explicit. drm_panel_attach/drm_bridge_attach could then
>> easily determine if the link should be made. IMHO, it would also be
>> quite easy to set this correctly for any given drm_device.
> 
> As I said earlier I think decision about link creation should be always
> up to element which performs attachment, It only knows what should be
> attached, so it is the best candidate to handle dynamic unbind and
> re-bind of the downstream element.

But does the attacher in fact know *what* should be attached? And
how? Take e.g. the drm_panel_attach in analogix_dp_bridge_attach, in
analogix_dp_core.c. Should that attach be with or without a
device-link? That function has no knowledge whatsoever about the
requirements for dp->plat_data->panel. That panel could e.g. come
from rockchip_dp_probe, in analogix_dp-rockchip.c, which can be
further traced back to drm_of_find_panel_or_bridge. But what kind of
requirements do that panel have? It might be a dsi panel, in which
case we had better not create a device-link, as you have shown
previously in the thread. Or it might be some little trivial panel,
like panel-lg-lg4573.c, in which case we *really* want the device-
link so that the panel doesn't disappear on us.

Maybe it's easy to see this, if you know the ins and outs of the
code. But I don't see it. And I don't see how this path leads to
maintainable code. I still think the link-or-no-link decision needs
to be in a central place.

Cheers,
Peter

Peter Rosin May 23, 2018, 8:29 a.m. UTC | #14

On 2018-05-22 17:03, Peter Rosin wrote:
> On 2018-05-22 11:45, Andrzej Hajda wrote:
>> On 22.05.2018 09:36, Peter Rosin wrote:
>>> On 2018-05-22 08:29, Andrzej Hajda wrote:
>>>> On 21.05.2018 23:56, Peter Rosin wrote:
>>>>> On 2018-05-21 11:21, Andrzej Hajda wrote:
>>>>>> On 21.05.2018 10:53, Peter Rosin wrote:
>>>>>>> On 2018-05-21 10:15, Andrzej Hajda wrote:
>>>>>>>> On 19.05.2018 18:48, Peter Rosin wrote:
>>>>>>>>> On 2018-05-18 13:51, Andrzej Hajda wrote:
>>>>>>>>>> On 26.04.2018 10:07, Jyri Sarha wrote:
>>>>>>>>>>> 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 <[e-mail]>
>>>>>>>>>>> Reviewed-by: Eric Anholt <[e-mail]>
>>>>>>>>>> Hi Jyri,
>>>>>>>>>>
>>>>>>>>>> This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
>>>>>>>>>> using exynos_drm_dsi and dsi panels.
>>>>>>>>>> Exynos-DSI properly handles panels unbind - ie references to panel are
>>>>>>>>>> properly removed on panels removal and respective drm_connector enters
>>>>>>>>>> disconnected state, without destroying whole drm device.
>>>>>>>>>> And again on panel driver re-bind drm_panel is properly attached to
>>>>>>>>>> exynos-dsi and panel pipeline is working again.
>>>>>>>>>> This patch will break this behavior, ie it will destroy whole drm device.
>>>>>>>>>>
>>>>>>>>>> Making device_links for panels optional seems to me the best solution,
>>>>>>>>>> what do you think?
>>>>>>>>>>
>>>>>>>>>> The funny thing is that due to bug in device link code, your patch has
>>>>>>>>>> no effect on these platforms. So it does not hurt these platform yet,
>>>>>>>>>> but the bug will be fixed sooner or later.
>>>>>>>>> Ah, that's a pretty strong hint that we are doing something unusual. So,
>>>>>>>>> I had a deeper look and I think that device-links (with state, i.e. not
>>>>>>>>> DL_FLAG_STATELESS links) are assumed to be created by the .probe function
>>>>>>>>> of either the consumer or the supplier. Then it seems to works as expected.
>>>>>>>>> And that makes some sense too, because a link indicates that there exist
>>>>>>>>> a dependency between the two and that the consumer cannot really exist
>>>>>>>>> without the supplier. This is also what happens for the drm devices
>>>>>>>>> (panel/bridge consumers) Jyri and I are working with; they call panel or
>>>>>>>>> bridge attach during their probe. But this is not the case for exynos,
>>>>>>>>> which calls panel/bridge attach at some later stage, and that obviously
>>>>>>>>> violates the assumption that the device-link consumer cannot exist w/o
>>>>>>>>> the supplier ("obviously" since the drm driver has managed to successfully
>>>>>>>>> probe without the supplier).
>>>>>>>>>
>>>>>>>>> So, when the panel/bridge supplier is probed after the consumer is
>>>>>>>>> bound, the link starts out as DL_STATE_DORMANT, and progresses to
>>>>>>>>> DL_STATE_AVAILABLE once the panel/bridge has finished the probe. This is
>>>>>>>>> not what *we* want.
>>>>>>>> And this is also incorrect from Documentation PoV:
>>>>>>>>  * @DL_STATE_DORMANT: None of the supplier/consumer drivers is present.
>>>>>>>>  * @DL_STATE_AVAILABLE: The supplier driver is present, but the consumer
>>>>>>>> is not.
>>>>>>>>
>>>>>>>>> So, one idea I have is to, on panel/bridge attach, verify if the
>>>>>>>>> consumer is in its probe, and only create the link if that is the
>>>>>>>>> case. So, the link would be optional, but it would all be automatic.
>>>>>>>> Making it automatic looks tempting, but also error prone. In case of
>>>>>>>> component framework bind callbacks can be called from probe of any
>>>>>>>> component, so sometimes it can be called also from exynos-dsi probe,
>>>>>>>> sometimes not (depending on order of probing, which we cannot rely on).
>>>>>>>> So in some cases we will end-up with links, sometimes without. Ie
>>>>>>>> following scenarios are possible in drm_panel_attach:
>>>>>>>> 1. exynos-dsi bound, panel during probe.
>>>>>>>> 2. exynos-dsi during probe, panel during probe.
>>>>>>> 2. exynos-dsi during probe, panel bound? Or is this case 3, and 2 happens
>>>>>>> when drivers probe in parallel?
>>>>>> Panel is always probed not earlier than the end of exynos-dsi bind, so
>>>>>> only scenarios 1 and 2 should be possible.
>>>>>>
>>>>>>> Whichever, you are right, I naively thought that the bind happened
>>>>>>> after the probe of all devices, but naturally it happens as part of
>>>>>>> the last device to register its component, and that normally happens
>>>>>>> during its probe.
>>>>>>>
>>>>>>> Sigh. So, scratch that, I guess we need a flag...
>>>>> I looked into that, and it seems very fragile to get the flag to be
>>>>> correct for all cases. That road is bound to produce lots of bugs, and
>>>>> it will be hard to get it right. In short, I would not descend into that
>>>>> particular rabbit hole.
>>>>>
>>>>> Can it be assumed that the drm_device of the encoder in drm_bridge_attach
>>>>> is a master component, if the drm "cluster" is componentized?
>>>> I wouldn't call it assumption, I would say rather it is a common practice.
>>>>
>>>>> Are there
>>>>> currently other ways of handling drm driver binding changes than
>>>>> components?
>>>> No, there are drivers which do not use components, but call
>>>> drm_panel_attach:
>>>> $ for d in drivers/gpu/drm/*/; do git grep -q 'struct component_ops' $d
>>>> && continue; git grep -q drm_panel_attach $d && echo $d; done
>>>> drivers/gpu/drm/bridge/
>>>> drivers/gpu/drm/fsl-dcu/
>>>> drivers/gpu/drm/mxsfb/
>>>> drivers/gpu/drm/rcar-du/
>>>> drivers/gpu/drm/tegra/
>>>>
>>>> Components are optional.
>>> Yes, of course components are optional. The question was if there are
>>> currently *other* ways (i.e. not the component framework, not device links)
>>> of dealing with disappearing panels/bridges. However, see below, the
>>> question is irrelevant with my below suggestion.
>>>
>>>>> If the answers are "yes" and "no", it might be possible to check if
>>>>> encoder->dev is a master component and only establish the device link if
>>>>> it isn't. All it would take is to add a component_device_is_master()
>>>>> query function to drivers/base/component.c
>>>>> Something like this (untested):
>>>>>
>>>>> bool component_device_is_master(struct device *dev)
>>>>> {
>>>>> 	struct master *m;
>>>>>
>>>>> 	mutex_lock(&component_mutex);
>>>>> 	m = __master_find(dev, NULL);
>>>>> 	mutex_unlock(&component_mutex);
>>>>>
>>>>> 	return !!m;
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(component_device_is_master);
>>>>>
>>>>> And then check that before calling device_link_add().
>>>> Why do not use simpler solution, just create function
>>>> drm_panel_attach_without_link and call it explicitly from drivers, or
>>>> add additional flag argument to either drm_panel_attach either to
>>>> drm_panel structure? Maybe it will not be prettier but will be more
>>>> explicit.
>>> Because, if you take bridges into account as well and add a
>>> drm_bridge_attach_without_link, which function do you call from e.g.
>>> drm_simple_display_pipe_attach_bridge()? Sure, you can probably verify
>>> the callers and come to the conclusion that you maybe always want the
>>> link, currently. Same question for the tda998x driver, which function
>>> to use for attaching the bridge would have to depend on how the driver
>>> was bound (as a component or not, yes I know, currently tda998x only
>>> does component, but the whole reason I'm involved is that I want it
>>> to also register as a standalone drm_bridge). Not doing this with some
>>> automatic check simply leads to combinatorial hell.
>>
>> I was focused on panels, which are managed not by drm core, but by
>> upstream pipeline element (bridge/encoder). For them decision about
>> using device links should be made by the manager not drm core, I guess.
>> In case of bridges it is different, bridges are attached by upstream
>> elements, but then they are tracked/managed/detached only by drm core
>> (at least this is current practice).  If somebody wants to implement
>> dynamic bridges this pattern cannot be used, ie bridge should be
>> attached/detached by upstream element, like in case of panels.
>>
>>>
>>> Maybe a better solution is for the drm driver to record whether it
>>> wants links in its struct drm_device, before calling drm_dev_register?
>>> That's also explicit. drm_panel_attach/drm_bridge_attach could then
>>> easily determine if the link should be made. IMHO, it would also be
>>> quite easy to set this correctly for any given drm_device.
>>
>> As I said earlier I think decision about link creation should be always
>> up to element which performs attachment, It only knows what should be
>> attached, so it is the best candidate to handle dynamic unbind and
>> re-bind of the downstream element.
> 
> But does the attacher in fact know *what* should be attached? And
> how? Take e.g. the drm_panel_attach in analogix_dp_bridge_attach, in
> analogix_dp_core.c. Should that attach be with or without a
> device-link? That function has no knowledge whatsoever about the
> requirements for dp->plat_data->panel. That panel could e.g. come
> from rockchip_dp_probe, in analogix_dp-rockchip.c, which can be
> further traced back to drm_of_find_panel_or_bridge. But what kind of
> requirements do that panel have? It might be a dsi panel, in which
> case we had better not create a device-link, as you have shown
> previously in the thread. Or it might be some little trivial panel,
> like panel-lg-lg4573.c, in which case we *really* want the device-
> link so that the panel doesn't disappear on us.
> 
> Maybe it's easy to see this, if you know the ins and outs of the
> code. But I don't see it. And I don't see how this path leads to
> maintainable code. I still think the link-or-no-link decision needs
> to be in a central place.

Are not all dsi panels client on a bus, and therefore managed by a
dsi host? And are not all dsi panels attached to a dsi connector?

How about this:

int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
{
	int ret;

	if (panel->connector)
		return -EBUSY;

	if (connector->connector_type != DRM_MODE_CONNECTOR_DSI) {
		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;

	return 0;
}

Andrzej Hajda May 23, 2018, 9:39 a.m. UTC | #15

On 23.05.2018 10:29, Peter Rosin wrote:
> On 2018-05-22 17:03, Peter Rosin wrote:
>> On 2018-05-22 11:45, Andrzej Hajda wrote:
>>> On 22.05.2018 09:36, Peter Rosin wrote:
>>>> On 2018-05-22 08:29, Andrzej Hajda wrote:
>>>>> On 21.05.2018 23:56, Peter Rosin wrote:
>>>>>> On 2018-05-21 11:21, Andrzej Hajda wrote:
>>>>>>> On 21.05.2018 10:53, Peter Rosin wrote:
>>>>>>>> On 2018-05-21 10:15, Andrzej Hajda wrote:
>>>>>>>>> On 19.05.2018 18:48, Peter Rosin wrote:
>>>>>>>>>> On 2018-05-18 13:51, Andrzej Hajda wrote:
>>>>>>>>>>> On 26.04.2018 10:07, Jyri Sarha wrote:
>>>>>>>>>>>> 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 <[e-mail]>
>>>>>>>>>>>> Reviewed-by: Eric Anholt <[e-mail]>
>>>>>>>>>>> Hi Jyri,
>>>>>>>>>>>
>>>>>>>>>>> This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
>>>>>>>>>>> using exynos_drm_dsi and dsi panels.
>>>>>>>>>>> Exynos-DSI properly handles panels unbind - ie references to panel are
>>>>>>>>>>> properly removed on panels removal and respective drm_connector enters
>>>>>>>>>>> disconnected state, without destroying whole drm device.
>>>>>>>>>>> And again on panel driver re-bind drm_panel is properly attached to
>>>>>>>>>>> exynos-dsi and panel pipeline is working again.
>>>>>>>>>>> This patch will break this behavior, ie it will destroy whole drm device.
>>>>>>>>>>>
>>>>>>>>>>> Making device_links for panels optional seems to me the best solution,
>>>>>>>>>>> what do you think?
>>>>>>>>>>>
>>>>>>>>>>> The funny thing is that due to bug in device link code, your patch has
>>>>>>>>>>> no effect on these platforms. So it does not hurt these platform yet,
>>>>>>>>>>> but the bug will be fixed sooner or later.
>>>>>>>>>> Ah, that's a pretty strong hint that we are doing something unusual. So,
>>>>>>>>>> I had a deeper look and I think that device-links (with state, i.e. not
>>>>>>>>>> DL_FLAG_STATELESS links) are assumed to be created by the .probe function
>>>>>>>>>> of either the consumer or the supplier. Then it seems to works as expected.
>>>>>>>>>> And that makes some sense too, because a link indicates that there exist
>>>>>>>>>> a dependency between the two and that the consumer cannot really exist
>>>>>>>>>> without the supplier. This is also what happens for the drm devices
>>>>>>>>>> (panel/bridge consumers) Jyri and I are working with; they call panel or
>>>>>>>>>> bridge attach during their probe. But this is not the case for exynos,
>>>>>>>>>> which calls panel/bridge attach at some later stage, and that obviously
>>>>>>>>>> violates the assumption that the device-link consumer cannot exist w/o
>>>>>>>>>> the supplier ("obviously" since the drm driver has managed to successfully
>>>>>>>>>> probe without the supplier).
>>>>>>>>>>
>>>>>>>>>> So, when the panel/bridge supplier is probed after the consumer is
>>>>>>>>>> bound, the link starts out as DL_STATE_DORMANT, and progresses to
>>>>>>>>>> DL_STATE_AVAILABLE once the panel/bridge has finished the probe. This is
>>>>>>>>>> not what *we* want.
>>>>>>>>> And this is also incorrect from Documentation PoV:
>>>>>>>>>  * @DL_STATE_DORMANT: None of the supplier/consumer drivers is present.
>>>>>>>>>  * @DL_STATE_AVAILABLE: The supplier driver is present, but the consumer
>>>>>>>>> is not.
>>>>>>>>>
>>>>>>>>>> So, one idea I have is to, on panel/bridge attach, verify if the
>>>>>>>>>> consumer is in its probe, and only create the link if that is the
>>>>>>>>>> case. So, the link would be optional, but it would all be automatic.
>>>>>>>>> Making it automatic looks tempting, but also error prone. In case of
>>>>>>>>> component framework bind callbacks can be called from probe of any
>>>>>>>>> component, so sometimes it can be called also from exynos-dsi probe,
>>>>>>>>> sometimes not (depending on order of probing, which we cannot rely on).
>>>>>>>>> So in some cases we will end-up with links, sometimes without. Ie
>>>>>>>>> following scenarios are possible in drm_panel_attach:
>>>>>>>>> 1. exynos-dsi bound, panel during probe.
>>>>>>>>> 2. exynos-dsi during probe, panel during probe.
>>>>>>>> 2. exynos-dsi during probe, panel bound? Or is this case 3, and 2 happens
>>>>>>>> when drivers probe in parallel?
>>>>>>> Panel is always probed not earlier than the end of exynos-dsi bind, so
>>>>>>> only scenarios 1 and 2 should be possible.
>>>>>>>
>>>>>>>> Whichever, you are right, I naively thought that the bind happened
>>>>>>>> after the probe of all devices, but naturally it happens as part of
>>>>>>>> the last device to register its component, and that normally happens
>>>>>>>> during its probe.
>>>>>>>>
>>>>>>>> Sigh. So, scratch that, I guess we need a flag...
>>>>>> I looked into that, and it seems very fragile to get the flag to be
>>>>>> correct for all cases. That road is bound to produce lots of bugs, and
>>>>>> it will be hard to get it right. In short, I would not descend into that
>>>>>> particular rabbit hole.
>>>>>>
>>>>>> Can it be assumed that the drm_device of the encoder in drm_bridge_attach
>>>>>> is a master component, if the drm "cluster" is componentized?
>>>>> I wouldn't call it assumption, I would say rather it is a common practice.
>>>>>
>>>>>> Are there
>>>>>> currently other ways of handling drm driver binding changes than
>>>>>> components?
>>>>> No, there are drivers which do not use components, but call
>>>>> drm_panel_attach:
>>>>> $ for d in drivers/gpu/drm/*/; do git grep -q 'struct component_ops' $d
>>>>> && continue; git grep -q drm_panel_attach $d && echo $d; done
>>>>> drivers/gpu/drm/bridge/
>>>>> drivers/gpu/drm/fsl-dcu/
>>>>> drivers/gpu/drm/mxsfb/
>>>>> drivers/gpu/drm/rcar-du/
>>>>> drivers/gpu/drm/tegra/
>>>>>
>>>>> Components are optional.
>>>> Yes, of course components are optional. The question was if there are
>>>> currently *other* ways (i.e. not the component framework, not device links)
>>>> of dealing with disappearing panels/bridges. However, see below, the
>>>> question is irrelevant with my below suggestion.
>>>>
>>>>>> If the answers are "yes" and "no", it might be possible to check if
>>>>>> encoder->dev is a master component and only establish the device link if
>>>>>> it isn't. All it would take is to add a component_device_is_master()
>>>>>> query function to drivers/base/component.c
>>>>>> Something like this (untested):
>>>>>>
>>>>>> bool component_device_is_master(struct device *dev)
>>>>>> {
>>>>>> 	struct master *m;
>>>>>>
>>>>>> 	mutex_lock(&component_mutex);
>>>>>> 	m = __master_find(dev, NULL);
>>>>>> 	mutex_unlock(&component_mutex);
>>>>>>
>>>>>> 	return !!m;
>>>>>> }
>>>>>> EXPORT_SYMBOL_GPL(component_device_is_master);
>>>>>>
>>>>>> And then check that before calling device_link_add().
>>>>> Why do not use simpler solution, just create function
>>>>> drm_panel_attach_without_link and call it explicitly from drivers, or
>>>>> add additional flag argument to either drm_panel_attach either to
>>>>> drm_panel structure? Maybe it will not be prettier but will be more
>>>>> explicit.
>>>> Because, if you take bridges into account as well and add a
>>>> drm_bridge_attach_without_link, which function do you call from e.g.
>>>> drm_simple_display_pipe_attach_bridge()? Sure, you can probably verify
>>>> the callers and come to the conclusion that you maybe always want the
>>>> link, currently. Same question for the tda998x driver, which function
>>>> to use for attaching the bridge would have to depend on how the driver
>>>> was bound (as a component or not, yes I know, currently tda998x only
>>>> does component, but the whole reason I'm involved is that I want it
>>>> to also register as a standalone drm_bridge). Not doing this with some
>>>> automatic check simply leads to combinatorial hell.
>>> I was focused on panels, which are managed not by drm core, but by
>>> upstream pipeline element (bridge/encoder). For them decision about
>>> using device links should be made by the manager not drm core, I guess.
>>> In case of bridges it is different, bridges are attached by upstream
>>> elements, but then they are tracked/managed/detached only by drm core
>>> (at least this is current practice).  If somebody wants to implement
>>> dynamic bridges this pattern cannot be used, ie bridge should be
>>> attached/detached by upstream element, like in case of panels.
>>>
>>>> Maybe a better solution is for the drm driver to record whether it
>>>> wants links in its struct drm_device, before calling drm_dev_register?
>>>> That's also explicit. drm_panel_attach/drm_bridge_attach could then
>>>> easily determine if the link should be made. IMHO, it would also be
>>>> quite easy to set this correctly for any given drm_device.
>>> As I said earlier I think decision about link creation should be always
>>> up to element which performs attachment, It only knows what should be
>>> attached, so it is the best candidate to handle dynamic unbind and
>>> re-bind of the downstream element.
>> But does the attacher in fact know *what* should be attached? And
>> how? Take e.g. the drm_panel_attach in analogix_dp_bridge_attach, in
>> analogix_dp_core.c. Should that attach be with or without a
>> device-link? 
analogix_dp_core.c is a 'library' used by exynos_dp and rockchip_dp,
neither of these drivers perform dynamic panel management so answer is
simple: device_link will protect them from panel unbind.
>> That function has no knowledge whatsoever about the
>> requirements for dp->plat_data->panel. That panel could e.g. come
>> from rockchip_dp_probe, in analogix_dp-rockchip.c, which can be
>> further traced back to drm_of_find_panel_or_bridge. But what kind of
>> requirements do that panel have? It might be a dsi panel, in which
>> case we had better not create a device-link, as you have shown
>> previously in the thread. Or it might be some little trivial panel,
>> like panel-lg-lg4573.c, in which case we *really* want the device-
>> link so that the panel doesn't disappear on us.

In general it is not a matter of panel, but of panel's consumer, ie
upstream pipe element, for example dsi host.

>>
>> Maybe it's easy to see this, if you know the ins and outs of the
>> code. But I don't see it. And I don't see how this path leads to
>> maintainable code. I still think the link-or-no-link decision needs
>> to be in a central place.
> Are not all dsi panels client on a bus, and therefore managed by a
> dsi host?

Panels are managed by dsi host only if dsi host implements it, and it is
up to dsi host author, it is not mandatory. The only case I am aware of
is exynos-dsi. I guess most developers are not aware of the problem, or
they do not care, or ... whatever. This patch shows it very clearly.

>  And are not all dsi panels attached to a dsi connector?
>
> How about this:
>
> int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
> {
> 	int ret;
>
> 	if (panel->connector)
> 		return -EBUSY;
>
> 	if (connector->connector_type != DRM_MODE_CONNECTOR_DSI) {
> 		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;
>
> 	return 0;
> }

With such approach you do not protect dsi hosts without dynamic panel
management support. As I said earlier: it is matter of panel consumers
(for example dsi host), ie if it implements dynamic panel management, it
should attach panel without creating device_links, as it take care of
disappearing panels. If it does not perform dynamic management it should
rely on default behavior - links should be created, to protect drm
device from using dangling pointers.

Regards
Andrzej

Peter Rosin May 23, 2018, 10:46 a.m. UTC | #16

On 2018-05-23 11:39, Andrzej Hajda wrote:

*snip*

> Panels are managed by dsi host only if dsi host implements it, and it is
> up to dsi host author, it is not mandatory. The only case I am aware of
> is exynos-dsi. I guess most developers are not aware of the problem, or
> they do not care, or ... whatever. This patch shows it very clearly.

*snip*

> With such approach you do not protect dsi hosts without dynamic panel
> management support. As I said earlier: it is matter of panel consumers
> (for example dsi host), ie if it implements dynamic panel management, it
> should attach panel without creating device_links, as it take care of
> disappearing panels. If it does not perform dynamic management it should
> rely on default behavior - links should be created, to protect drm
> device from using dangling pointers.

So, you're saying that the only place that needs to be patched to call
drm_panel_attach_without_link is exynos_dsi_host_attach()?

What about the dw-mipi-dsi driver in rockchip/dw-mipi-dsi.c? It also
calls drm_panel_attach/drm_panel_detach in its dw_mipi_dsi_host_attach
and dw_mipi_dsi_host_detach functions.

Similar thing in bridge/synopsis/dw-mipi-dsi.c, but it calls the
wrappers drm_panel_bridge_add/remove to create an implicit bridge
that in turn handles the panel. So, do we need a to add a
drm_panel_bridge_add_without_link too?

What about tegra/dsi.c? It also calls drm_panel_attach in its
tegra_dsi_host_attach function? But that one doesn't detach the
panel in its tegra_dsi_host_detach. Is that a bug?

Cheers,
Peter

Andrzej Hajda May 23, 2018, 12:38 p.m. UTC | #17

On 23.05.2018 12:46, Peter Rosin wrote:
> On 2018-05-23 11:39, Andrzej Hajda wrote:
>
> *snip*
>
>> Panels are managed by dsi host only if dsi host implements it, and it is
>> up to dsi host author, it is not mandatory. The only case I am aware of
>> is exynos-dsi. I guess most developers are not aware of the problem, or
>> they do not care, or ... whatever. This patch shows it very clearly.
> *snip*
>
>> With such approach you do not protect dsi hosts without dynamic panel
>> management support. As I said earlier: it is matter of panel consumers
>> (for example dsi host), ie if it implements dynamic panel management, it
>> should attach panel without creating device_links, as it take care of
>> disappearing panels. If it does not perform dynamic management it should
>> rely on default behavior - links should be created, to protect drm
>> device from using dangling pointers.
> So, you're saying that the only place that needs to be patched to call
> drm_panel_attach_without_link is exynos_dsi_host_attach()?

It should be at least there, but I suppose only there.

>
> What about the dw-mipi-dsi driver in rockchip/dw-mipi-dsi.c? It also
> calls drm_panel_attach/drm_panel_detach in its dw_mipi_dsi_host_attach
> and dw_mipi_dsi_host_detach functions.

dw_mipi_dsi_host_detach calls only drm_panel_detach, dangling pointer
will stay in dsi->panel, and nothing prevents from using it. I guess it
is just buggy code.

>
> Similar thing in bridge/synopsis/dw-mipi-dsi.c, but it calls the
> wrappers drm_panel_bridge_add/remove to create an implicit bridge
> that in turn handles the panel. So, do we need a to add a
> drm_panel_bridge_add_without_link too?

Again looks like a buggy code, before panel removal it should be turned
off, it should be performed under lock,....

>
> What about tegra/dsi.c? It also calls drm_panel_attach in its
> tegra_dsi_host_attach function? But that one doesn't detach the
> panel in its tegra_dsi_host_detach. Is that a bug?

Also looks like a bug, but this is dual-channel device so I am not so sure.

Regards
Andrzej

Peter Rosin May 23, 2018, 2:34 p.m. UTC | #18

On 2018-05-23 14:38, Andrzej Hajda wrote:
> On 23.05.2018 12:46, Peter Rosin wrote:
>> On 2018-05-23 11:39, Andrzej Hajda wrote:
>>
>> *snip*
>>
>>> Panels are managed by dsi host only if dsi host implements it, and it is
>>> up to dsi host author, it is not mandatory. The only case I am aware of
>>> is exynos-dsi. I guess most developers are not aware of the problem, or
>>> they do not care, or ... whatever. This patch shows it very clearly.
>> *snip*
>>
>>> With such approach you do not protect dsi hosts without dynamic panel
>>> management support. As I said earlier: it is matter of panel consumers
>>> (for example dsi host), ie if it implements dynamic panel management, it
>>> should attach panel without creating device_links, as it take care of
>>> disappearing panels. If it does not perform dynamic management it should
>>> rely on default behavior - links should be created, to protect drm
>>> device from using dangling pointers.
>> So, you're saying that the only place that needs to be patched to call
>> drm_panel_attach_without_link is exynos_dsi_host_attach()?
> 
> It should be at least there, but I suppose only there.
> 
>>
>> What about the dw-mipi-dsi driver in rockchip/dw-mipi-dsi.c? It also
>> calls drm_panel_attach/drm_panel_detach in its dw_mipi_dsi_host_attach
>> and dw_mipi_dsi_host_detach functions.
> 
> dw_mipi_dsi_host_detach calls only drm_panel_detach, dangling pointer
> will stay in dsi->panel, and nothing prevents from using it. I guess it
> is just buggy code.

Yeah, now I see it.

>>
>> Similar thing in bridge/synopsis/dw-mipi-dsi.c, but it calls the
>> wrappers drm_panel_bridge_add/remove to create an implicit bridge
>> that in turn handles the panel. So, do we need a to add a
>> drm_panel_bridge_add_without_link too?
> 
> Again looks like a buggy code, before panel removal it should be turned
> off, it should be performed under lock,....

Yeah, that looks like crap code.

>>
>> What about tegra/dsi.c? It also calls drm_panel_attach in its
>> tegra_dsi_host_attach function? But that one doesn't detach the
>> panel in its tegra_dsi_host_detach. Is that a bug?
> 
> Also looks like a bug, but this is dual-channel device so I am not so sure.

It's very confusing and hard to get a grip on how things are supposed
to be done when everything appears to be riddled with misunderstandings...

While browsing the code base I originally thought many more sites needed
the drm_panel_attach_without_link variant, but that appears to not be the
case. Thanks for your patient explanations!

Cheers,
Peter

Daniel Vetter Aug. 9, 2018, 8:56 a.m. UTC | #19

On Thu, Apr 26, 2018 at 10:07 AM, Jyri Sarha <[e-mail]> wrote:
> 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 <[e-mail]>
> Reviewed-by: Eric Anholt <[e-mail]>

Just noticed this complains when building docs:

./include/drm/drm_panel.h:98: warning: Function parameter or member
'link' not described in 'drm_panel'

Care to fix this? Also would be good to capture some of the
discussions that ensued from this patch in the docs ...

Thanks, Daniel

> ---
>  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 71e4075..965530a 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>
>
> @@ -104,6 +105,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;
>
> @@ -125,6 +133,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
>

Linus Walleij Aug. 30, 2018, 12:32 p.m. UTC | #20

On Thu, Apr 26, 2018 at 10:07 AM Jyri Sarha <[e-mail]> wrote:

> 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 <[e-mail]>
> Reviewed-by: Eric Anholt <[e-mail]>

Hi I have a problem with an in-development driver and this patch.

Do not worry! It is no regression. I just need help to do things right
now.

My panel is a DSI device on a DSI master:

                mcde@a0350000 {
                        status = "okay";

                        dsi@a0351000 {
                                port {
                                        dsi0_out: endpoint {
                                                remote-endpoint = <&panel_in>;
                                        };
                                };

                                panel: display {
                                        compatible = "samsung,s6d16d0";
                                        reg = <0>;
                                        /* VDD1 on the component */
                                        power-supply = <&ab8500_ldo_aux1_reg>;
                                        reset-gpios = <&gpio2 1
GPIO_ACTIVE_LOW>;

                                        port {
                                                panel_in: endpoint {

remote-endpoint = <&dsi0_out>;
                                                };
                                        };
                                };
                        };
                };

So in my DSI host driver .bind() I do:

    drm_encoder_init(drm, encoder, &mcde_dsi_encoder_funcs,
             DRM_MODE_ENCODER_DSI, NULL);
    drm_encoder_helper_add(encoder, &mcde_dsi_encoder_helper_funcs);
    ret = drm_connector_init(encoder->dev, connector,
                 &mcde_dsi_connector_funcs,
                 DRM_MODE_CONNECTOR_DSI);

    drm_connector_helper_add(connector, &mcde_dsi_connector_helper_funcs);
    drm_connector_attach_encoder(connector, encoder);
    drm_connector_register(connector);

This works fine. I managed to create the encoder and connector for this
DSI. Then:

    ret = drm_of_find_panel_or_bridge(dev->of_node,
                      0, 0, &panel, &bridge);
    if (panel) {
        bridge = drm_panel_bridge_add(panel,
                          DRM_MODE_CONNECTOR_DSI);
        if (IS_ERR(bridge)) {
            dev_err(dev, "error adding panel bridge\n");
            return PTR_ERR(bridge);
        }
        dev_info(dev, "connected to panel\n");
        d->panel = panel;
    }
    d->connector.status = connector_status_connected;
    ret = drm_bridge_attach(encoder, bridge, NULL);

And this is where it blows up: it fails in
device_link_add(connector->dev->dev, panel->dev, 0);
device_is_dependent(consumer, supplier) because the
consumer (connector) is dependent on the supplier (bridge).

This happens because the connector struct device is the
same as the bridge struct device, I suppose.

Isn't that OK? Please help me to see what's wrong with
my architecture here, and what I need to do :/ I tried to
follow examples set by MSM and Exynos DSI.

Yours,
Linus Walleij

Andrzej Hajda Aug. 30, 2018, 2:41 p.m. UTC | #21

On 30.08.2018 14:32, Linus Walleij wrote:
> On Thu, Apr 26, 2018 at 10:07 AM Jyri Sarha <[e-mail]> wrote:
>
>> 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 <[e-mail]>
>> Reviewed-by: Eric Anholt <[e-mail]>
> Hi I have a problem with an in-development driver and this patch.
>
> Do not worry! It is no regression. I just need help to do things right
> now.
>
> My panel is a DSI device on a DSI master:
>
>                 mcde@a0350000 {
>                         status = "okay";
>
>                         dsi@a0351000 {
>                                 port {
>                                         dsi0_out: endpoint {
>                                                 remote-endpoint = <&panel_in>;
>                                         };
>                                 };
>
>                                 panel: display {
>                                         compatible = "samsung,s6d16d0";
>                                         reg = <0>;
>                                         /* VDD1 on the component */
>                                         power-supply = <&ab8500_ldo_aux1_reg>;
>                                         reset-gpios = <&gpio2 1
> GPIO_ACTIVE_LOW>;
>
>                                         port {
>                                                 panel_in: endpoint {
>
> remote-endpoint = <&dsi0_out>;
>                                                 };
>                                         };
>                                 };
>                         };
>                 };
>
> So in my DSI host driver .bind() I do:
>
>     drm_encoder_init(drm, encoder, &mcde_dsi_encoder_funcs,
>              DRM_MODE_ENCODER_DSI, NULL);
>     drm_encoder_helper_add(encoder, &mcde_dsi_encoder_helper_funcs);
>     ret = drm_connector_init(encoder->dev, connector,
>                  &mcde_dsi_connector_funcs,
>                  DRM_MODE_CONNECTOR_DSI);
>
>     drm_connector_helper_add(connector, &mcde_dsi_connector_helper_funcs);
>     drm_connector_attach_encoder(connector, encoder);
>     drm_connector_register(connector);
>
> This works fine. I managed to create the encoder and connector for this
> DSI. Then:
>
>     ret = drm_of_find_panel_or_bridge(dev->of_node,
>                       0, 0, &panel, &bridge);
>     if (panel) {
>         bridge = drm_panel_bridge_add(panel,
>                           DRM_MODE_CONNECTOR_DSI);
>         if (IS_ERR(bridge)) {
>             dev_err(dev, "error adding panel bridge\n");
>             return PTR_ERR(bridge);
>         }
>         dev_info(dev, "connected to panel\n");
>         d->panel = panel;
>     }
>     d->connector.status = connector_status_connected;
>     ret = drm_bridge_attach(encoder, bridge, NULL);
>
> And this is where it blows up: it fails in
> device_link_add(connector->dev->dev, panel->dev, 0);
> device_is_dependent(consumer, supplier) because the
> consumer (connector) is dependent on the supplier (bridge).
>
> This happens because the connector struct device is the
> same as the bridge struct device, I suppose.

I guess it is rather because the code tries to make circular dependency:
1. panel depends on dsi-host because it is MIPI-DSI child device.
2. dsi-host probably depends on drm parent device (connector->dev->dev)
- what drm driver do you use?
3. drm parent dev depends on panel: this patch adds this dependency.

If 2nd point is true it becomes circular dependency, but please verify it.

Regards
Andrzej

>
> Isn't that OK? Please help me to see what's wrong with
> my architecture here, and what I need to do :/ I tried to
> follow examples set by MSM and Exynos DSI.
>
> Yours,
> Linus Walleij
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Linus Walleij Aug. 31, 2018, 8:56 a.m. UTC | #22

On Thu, Aug 30, 2018 at 4:41 PM Andrzej Hajda <[e-mail]> wrote:
> [Me]
> > This happens because the connector struct device is the
> > same as the bridge struct device, I suppose.
>
> I guess it is rather because the code tries to make circular dependency:
> 1. panel depends on dsi-host because it is MIPI-DSI child device.
> 2. dsi-host probably depends on drm parent device (connector->dev->dev)
> - what drm driver do you use?

The driver is added in this patch, it's at this uncomfortable stage
where I have to make a big upfront design for a new DRM driver
and everything is shaky and unreviewed.

So to get it out for proper review it needs to be working and to
get it working I need review :D DRM development catch 22.

But here is the patch adding it all, in some in-flight state:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-stericsson.git/commit/?h=ux500-mcde

Thanks a lot for these notes, I hope I can figure it out!

> 3. drm parent dev depends on panel: this patch adds this dependency.
>
> If 2nd point is true it becomes circular dependency, but please verify it.

I tried to not make the DRM parent dev depend on the panel.

AFAICT (1) is true, (2) is true but not (3).

Yours,
Linus Walleij

Andrzej Hajda Aug. 31, 2018, 10:08 a.m. UTC | #23

On 31.08.2018 10:56, Linus Walleij wrote:
> On Thu, Aug 30, 2018 at 4:41 PM Andrzej Hajda <[e-mail]> wrote:
>> [Me]
>>> This happens because the connector struct device is the
>>> same as the bridge struct device, I suppose.
>> I guess it is rather because the code tries to make circular dependency:
>> 1. panel depends on dsi-host because it is MIPI-DSI child device.
>> 2. dsi-host probably depends on drm parent device (connector->dev->dev)
>> - what drm driver do you use?
> The driver is added in this patch, it's at this uncomfortable stage
> where I have to make a big upfront design for a new DRM driver
> and everything is shaky and unreviewed.
>
> So to get it out for proper review it needs to be working and to
> get it working I need review :D DRM development catch 22.
>
> But here is the patch adding it all, in some in-flight state:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-stericsson.git/commit/?h=ux500-mcde
>
> Thanks a lot for these notes, I hope I can figure it out!
>
>> 3. drm parent dev depends on panel: this patch adds this dependency.
>>
>> If 2nd point is true it becomes circular dependency, but please verify it.
> I tried to not make the DRM parent dev depend on the panel.
>
> AFAICT (1) is true, (2) is true but not (3).

See code of drm_panel_attach after subject patch [1]:
    panel->link = device_link_add(connector->dev->dev, panel->dev, 0);

It is quite clear that connector->dev->dev is 'drm_device parent device'
so you have drm-parent depends on panel->dev.
IMO this is incorrect dependency, and the cause of error, it is not
drm_device parent who depends on the panel, but drm_device itself.

[1]:
https://elixir.bootlin.com/linux/v4.19-rc1/source/drivers/gpu/drm/drm_panel.c#L108

Regards
Andrzej

>
> Yours,
> Linus Walleij
>
>

Daniel Vetter Oct. 22, 2019, 8:44 a.m. UTC | #24

On Fri, May 18, 2018 at 1:52 PM Andrzej Hajda <[e-mail]> wrote:
>
> On 26.04.2018 10:07, Jyri Sarha wrote:
> > 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 <[e-mail]>
> > Reviewed-by: Eric Anholt <[e-mail]>
>
> Hi Jyri,
>
> This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
> using exynos_drm_dsi and dsi panels.
> Exynos-DSI properly handles panels unbind - ie references to panel are
> properly removed on panels removal and respective drm_connector enters
> disconnected state, without destroying whole drm device.
> And again on panel driver re-bind drm_panel is properly attached to
> exynos-dsi and panel pipeline is working again.
> This patch will break this behavior, ie it will destroy whole drm device.
>
> Making device_links for panels optional seems to me the best solution,
> what do you think?

I think all the device_link bugs have been fixed by now (lots of
discussion in some other thread around using them for bridges). Time
to resurrect/retest this patch and land it?

I still like the idea very much of making panel/bridges Just Work.

Cheers, Daniel

>
> The funny thing is that due to bug in device link code, your patch has
> no effect on these platforms. So it does not hurt these platform yet,
> but the bug will be fixed sooner or later.
> The trick in case of exynos-dsi is to use mipi_dsi attach/detach
> callbacks to get notifications about drm_panel's
> appearance/disappearance, I guess ultimately such callbacks should be a
> part of drm_panel
> framework.
>
> Below description how all the code works:
> Initialization:
> 1. exynos_dsi (component of exynos_drm) at the end of bind callback
> registers mipi_dsi bus.
> 2. during mipi_dsi bus registration or later (if panel driver was not
> yet registered), panel driver is bound ie - its probe callback is called.
> 3. during panel's probe drm_panel_add is called, and later mipi_dsi_attach.
> 4. mipi_dsi_attach calls dsi .attach callback of exynos_dsi.
> 5. exynos_dsi.attach callback looks for drm_panel, calls
> drm_panel_attach and changes connector's status to connected.
> ....
> Panel's unbind:
> 1. device_release_driver calls panel's remove callback.
> 2. panel's remove callback calls mipi_dsi_detach.
> 3. mipi_dsi_detach calls exynos_dsi.detach callback.
> 4. exynos_dsi.detach calls drm_panel_detach and changes connector's
> state to disconnected.
>
> Panel's rebind is equivalent of steps 2-4 of Initialization.
>
> I have wrote the code few years ago, and it seemed to me the only safe
> way to handle dynamic panel bind/unbind.
> For example please note that step 5 of initialization and steps 2-4 of
> unbind are executed always under panel's device_lock so it makes it
> safe. Otherwise you risk that between panel lookup and panel attach
> panel's driver can be unbind and consumer is left with dangling panel's
> pointer, so even with your patch most of the drivers are still buggy.
>
> And few words about the bug(?) in device_link code: If you call
> device_link_add from probe of the supplier(as it is in this case) its
> status is set to DL_STATE_DORMANT, and after successful bind it is
> changed to DL_STATE_AVAILABLE, which is incorrect (it should be
> DL_STATE_ACTIVE), as a result on supplier's unbind consumer is not unbound.
>
> Regards
> Andrzej
>
> > ---
> >  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 71e4075..965530a 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>
> >
> > @@ -104,6 +105,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;
> >
> > @@ -125,6 +133,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;
> >
>
>
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 71e4075..965530a 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>
 
@@ -104,6 +105,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;
 
@@ -125,6 +133,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;