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 <tfiga@xxxxxxxxxxxx>
>
> 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 <tfiga@xxxxxxxxxxxx>
> Signed-off-by: Thierry Escande <thierry.escande@xxxxxxxxxxxxx>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
> Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> ---
>
>  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 <linux/input.h>
> +#include <linux/reboot.h>
>  
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc_helper.h>
> @@ -33,6 +34,7 @@ struct psr_drv {
>  	struct delayed_work	flush_work;
>  	struct work_struct	disable_work;
>  
> +	struct notifier_block	reboot_nb;
>  	struct input_handler    input_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(&psr->reboot_nb);
> +
>  	mutex_lock(&drm_drv->psr_list_lock);
>  	list_add_tail(&psr->list, &drm_drv->psr_list);
>  	mutex_unlock(&drm_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(&psr->list);
> +			unregister_reboot_notifier(&psr->reboot_nb);
>  			input_unregister_handler(&psr->input_handler);
>  			kfree(psr->input_handler.name);
>  			kfree(psr);