| Message ID | 20180405095000.9756-29-enric.balletbo@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05.04.2018 11:49, Enric Balletbo i Serra wrote: > From: Tomasz Figa <tfiga@chromium.org> > > 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@chromium.org> > Signed-off-by: Thierry Escande <thierry.escande@collabora.com> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > > 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);
Hi Andrzej, On Mon, Apr 16, 2018 at 6:57 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > On 05.04.2018 11:49, Enric Balletbo i Serra wrote: > > From: Tomasz Figa <tfiga@chromium.org> > > > > 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&id=2d30bb0b3889adf09b342722b2ce596c0763bc93 So we might not need this patch anymore. Best regards, Tomasz
Hi Andrzej, Tomasz 2018-04-16 15:12 GMT+02:00 Tomasz Figa <tfiga@chromium.org>: > Hi Andrzej, > > On Mon, Apr 16, 2018 at 6:57 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > >> On 05.04.2018 11:49, Enric Balletbo i Serra wrote: >> > From: Tomasz Figa <tfiga@chromium.org> >> > >> > 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&id=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
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);