Re: [PATCH v6 28/30] drm/rockchip: Disable PSR from reboot notifier

[Enric Balletbo Serra]

Hi Andrzej, Tomasz

2018-04-16 15:12 GMT+02:00 Tomasz Figa :
> Hi Andrzej,
>
> On Mon, Apr 16, 2018 at 6:57 PM Andrzej Hajda  wrote:
>
>> On 05.04.2018 11:49, Enric Balletbo i Serra wrote:
>> > From: Tomasz Figa 
>> >
>> > It looks like the driver subsystem detaches devices from power domains
>> > at shutdown without consent of the drivers.
>
>> It looks bit strange. Could you elaborate more on it. Could you show the
>> code performing the detach?
>
> It not only looks strange, but it is strange. The code was present in 4.4:
>
> https://elixir.bootlin.com/linux/v4.4.128/source/drivers/base/platform.c#L553
>
> but was apparently removed in 4.5:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/base/platform.c?h=next-20180416=2d30bb0b3889adf09b342722b2ce596c0763bc93
>
> So we might not need this patch anymore.
>

Right, seems that we don't need this patch anymore, I'll do more few
tests and likely remove this patch from this series. Thanks for
catching this.

Best regards,
  Enric

> Best regards,
> Tomasz
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 28/30] drm/rockchip: Disable PSR from reboot notifier

[Tomasz Figa]

Hi Andrzej,

On Mon, Apr 16, 2018 at 6:57 PM Andrzej Hajda  wrote:

> On 05.04.2018 11:49, Enric Balletbo i Serra wrote:
> > From: Tomasz Figa 
> >
> > It looks like the driver subsystem detaches devices from power domains
> > at shutdown without consent of the drivers.

> It looks bit strange. Could you elaborate more on it. Could you show the
> code performing the detach?

It not only looks strange, but it is strange. The code was present in 4.4:

https://elixir.bootlin.com/linux/v4.4.128/source/drivers/base/platform.c#L553

but was apparently removed in 4.5:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/base/platform.c?h=next-20180416=2d30bb0b3889adf09b342722b2ce596c0763bc93

So we might not need this patch anymore.

Best regards,
Tomasz
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 28/30] drm/rockchip: Disable PSR from reboot notifier

[Andrzej Hajda]

On 05.04.2018 11:49, Enric Balletbo i Serra wrote:
> From: Tomasz Figa 
>
> It looks like the driver subsystem detaches devices from power domains
> at shutdown without consent of the drivers.

It looks bit strange. Could you elaborate more on it. Could you show the
code performing the detach?


Regards
Andrzej


>  This means that we might have
> our power domain turned off behind our back and the only way to avoid
> problems is to stop doing any hardware programming at some point before
> the power is cut. A reboot notifier, despite being a misnomer and
> handling shutdowns as well, is a good place to do it.
> Signed-off-by: Tomasz Figa 
> Signed-off-by: Thierry Escande 
> Signed-off-by: Enric Balletbo i Serra 
> Tested-by: Marek Szyprowski 
> ---
>
>  drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 24 
>  1 file changed, 24 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c 
> b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> index e7e16d92d5a1..1bf5cba9a64d 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -33,6 +34,7 @@ struct psr_drv {
>   struct delayed_work flush_work;
>   struct work_struct  disable_work;
>  
> + struct notifier_block   reboot_nb;
>   struct input_handlerinput_handler;
>  
>   int (*set)(struct drm_encoder *encoder, bool enable);
> @@ -309,6 +311,24 @@ static const struct input_device_id psr_ids[] = {
>   { },
>  };
>  
> +static int rockchip_drm_psr_reboot_notifier(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct psr_drv *psr = container_of(nb, struct psr_drv, reboot_nb);
> +
> + /*
> +  * It looks like the driver subsystem detaches devices from power
> +  * domains at shutdown without consent of the drivers. This means
> +  * that we might have our power domain turned off behind our back
> +  * and the only way to avoid problems is to stop doing any hardware
> +  * programming after this point, which is achieved by the unbalanced
> +  * call below.
> +  */
> + rockchip_drm_psr_inhibit_get(psr->encoder);
> +
> + return 0;
> +}
> +
>  /**
>   * rockchip_drm_psr_register - register encoder to psr driver
>   * @encoder: encoder that obtain the PSR function
> @@ -361,6 +381,9 @@ int rockchip_drm_psr_register(struct drm_encoder *encoder,
>   if (error)
>   goto err1;
>  
> + psr->reboot_nb.notifier_call = rockchip_drm_psr_reboot_notifier;
> + register_reboot_notifier(>reboot_nb);
> +
>   mutex_lock(_drv->psr_list_lock);
>   list_add_tail(>list, _drv->psr_list);
>   mutex_unlock(_drv->psr_list_lock);
> @@ -403,6 +426,7 @@ void rockchip_drm_psr_unregister(struct drm_encoder 
> *encoder)
>   WARN_ON(psr->inhibit_count != 1);
>  
>   list_del(>list);
> + unregister_reboot_notifier(>reboot_nb);
>   input_unregister_handler(>input_handler);
>   kfree(psr->input_handler.name);
>   kfree(psr);


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

[PATCH v6 28/30] drm/rockchip: Disable PSR from reboot notifier

[Enric Balletbo i Serra]

From: Tomasz Figa 

It looks like the driver subsystem detaches devices from power domains
at shutdown without consent of the drivers. This means that we might have
our power domain turned off behind our back and the only way to avoid
problems is to stop doing any hardware programming at some point before
the power is cut. A reboot notifier, despite being a misnomer and
handling shutdowns as well, is a good place to do it.

Signed-off-by: Tomasz Figa 
Signed-off-by: Thierry Escande 
Signed-off-by: Enric Balletbo i Serra 
Tested-by: Marek Szyprowski 
---

 drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
index e7e16d92d5a1..1bf5cba9a64d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
@@ -13,6 +13,7 @@
  */
 
 #include 
+#include 
 
 #include 
 #include 
@@ -33,6 +34,7 @@ struct psr_drv {
struct delayed_work flush_work;
struct work_struct  disable_work;
 
+   struct notifier_block   reboot_nb;
struct input_handlerinput_handler;
 
int (*set)(struct drm_encoder *encoder, bool enable);
@@ -309,6 +311,24 @@ static const struct input_device_id psr_ids[] = {
{ },
 };
 
+static int rockchip_drm_psr_reboot_notifier(struct notifier_block *nb,
+   unsigned long action, void *data)
+{
+   struct psr_drv *psr = container_of(nb, struct psr_drv, reboot_nb);
+
+   /*
+* It looks like the driver subsystem detaches devices from power
+* domains at shutdown without consent of the drivers. This means
+* that we might have our power domain turned off behind our back
+* and the only way to avoid problems is to stop doing any hardware
+* programming after this point, which is achieved by the unbalanced
+* call below.
+*/
+   rockchip_drm_psr_inhibit_get(psr->encoder);
+
+   return 0;
+}
+
 /**
  * rockchip_drm_psr_register - register encoder to psr driver
  * @encoder: encoder that obtain the PSR function
@@ -361,6 +381,9 @@ int rockchip_drm_psr_register(struct drm_encoder *encoder,
if (error)
goto err1;
 
+   psr->reboot_nb.notifier_call = rockchip_drm_psr_reboot_notifier;
+   register_reboot_notifier(>reboot_nb);
+
mutex_lock(_drv->psr_list_lock);
list_add_tail(>list, _drv->psr_list);
mutex_unlock(_drv->psr_list_lock);
@@ -403,6 +426,7 @@ void rockchip_drm_psr_unregister(struct drm_encoder 
*encoder)
WARN_ON(psr->inhibit_count != 1);
 
list_del(>list);
+   unregister_reboot_notifier(>reboot_nb);
input_unregister_handler(>input_handler);
kfree(psr->input_handler.name);
kfree(psr);
-- 
2.16.3

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