2019-06-17 07:20:11

by Yannick FERTRE

Subject: [PATCH 1/3] drm/stm: drv: fix suspend/resume

Without this fix, the system can not go in "suspend" mode
due to an error in drv_suspend function.

Fixes: 35ab6cf ("drm/stm: support runtime power management")

Signed-off-by: Yannick Fertré <[email protected]>
---
drivers/gpu/drm/stm/drv.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
index 5659572..9dee4e4 100644
--- a/drivers/gpu/drm/stm/drv.c
+++ b/drivers/gpu/drm/stm/drv.c
@@ -136,8 +136,7 @@ static __maybe_unused int drv_suspend(struct device *dev)
struct ltdc_device *ldev = ddev->dev_private;
struct drm_atomic_state *state;

- if (WARN_ON(!ldev->suspend_state))
- return -ENOENT;
+ WARN_ON(ldev->suspend_state);

state = drm_atomic_helper_suspend(ddev);
if (IS_ERR(state))
@@ -155,15 +154,17 @@ static __maybe_unused int drv_resume(struct device *dev)
struct ltdc_device *ldev = ddev->dev_private;
int ret;

+ if (WARN_ON(!ldev->suspend_state))
+ return -ENOENT;
+
pm_runtime_force_resume(dev);
ret = drm_atomic_helper_resume(ddev, ldev->suspend_state);
- if (ret) {
+ if (ret)
pm_runtime_force_suspend(dev);
- ldev->suspend_state = NULL;
- return ret;
- }

- return 0;
+ ldev->suspend_state = NULL;
+
+ return ret;
}

static __maybe_unused int drv_runtime_suspend(struct device *dev)
--
2.7.4

2019-06-18 09:58:12

by Philippe Cornu

Subject: Re: [PATCH 1/3] drm/stm: drv: fix suspend/resume

Hi Yannick,

Thank you for your patch.

Acked-by: Philippe Cornu <[email protected]>

Philippe :-)

On 6/17/19 9:18 AM, Yannick Fertré wrote:
> Without this fix, the system can not go in "suspend" mode
> due to an error in drv_suspend function.
>
> Fixes: 35ab6cf ("drm/stm: support runtime power management")
>
> Signed-off-by: Yannick Fertré <[email protected]>
> ---
> drivers/gpu/drm/stm/drv.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> index 5659572..9dee4e4 100644
> --- a/drivers/gpu/drm/stm/drv.c
> +++ b/drivers/gpu/drm/stm/drv.c
> @@ -136,8 +136,7 @@ static __maybe_unused int drv_suspend(struct device *dev)
> struct ltdc_device *ldev = ddev->dev_private;
> struct drm_atomic_state *state;
>
> - if (WARN_ON(!ldev->suspend_state))
> - return -ENOENT;
> + WARN_ON(ldev->suspend_state);
>
> state = drm_atomic_helper_suspend(ddev);
> if (IS_ERR(state))
> @@ -155,15 +154,17 @@ static __maybe_unused int drv_resume(struct device *dev)
> struct ltdc_device *ldev = ddev->dev_private;
> int ret;
>
> + if (WARN_ON(!ldev->suspend_state))
> + return -ENOENT;
> +
> pm_runtime_force_resume(dev);
> ret = drm_atomic_helper_resume(ddev, ldev->suspend_state);
> - if (ret) {
> + if (ret)
> pm_runtime_force_suspend(dev);
> - ldev->suspend_state = NULL;
> - return ret;
> - }
>
> - return 0;
> + ldev->suspend_state = NULL;
> +
> + return ret;
> }
>
> static __maybe_unused int drv_runtime_suspend(struct device *dev)
>

2019-06-20 15:06:10

by Benjamin Gaignard

Subject: Re: [PATCH 1/3] drm/stm: drv: fix suspend/resume

Le mar. 18 juin 2019 à 11:57, Philippe CORNU <[email protected]> a écrit :
>
> Hi Yannick,
>
> Thank you for your patch.
>
> Acked-by: Philippe Cornu <[email protected]>

I have corrected Fixes sha1 (should be 12 digits)
Applied on drm-misc-next.

Benjamin

>
> Philippe :-)
>
> On 6/17/19 9:18 AM, Yannick Fertré wrote:
> > Without this fix, the system can not go in "suspend" mode
> > due to an error in drv_suspend function.
> >
> > Fixes: 35ab6cf ("drm/stm: support runtime power management")
> >
> > Signed-off-by: Yannick Fertré <[email protected]>
> > ---
> > drivers/gpu/drm/stm/drv.c | 15 ++++++++-------
> > 1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> > index 5659572..9dee4e4 100644
> > --- a/drivers/gpu/drm/stm/drv.c
> > +++ b/drivers/gpu/drm/stm/drv.c
> > @@ -136,8 +136,7 @@ static __maybe_unused int drv_suspend(struct device *dev)
> > struct ltdc_device *ldev = ddev->dev_private;
> > struct drm_atomic_state *state;
> >
> > - if (WARN_ON(!ldev->suspend_state))
> > - return -ENOENT;
> > + WARN_ON(ldev->suspend_state);
> >
> > state = drm_atomic_helper_suspend(ddev);
> > if (IS_ERR(state))
> > @@ -155,15 +154,17 @@ static __maybe_unused int drv_resume(struct device *dev)
> > struct ltdc_device *ldev = ddev->dev_private;
> > int ret;
> >
> > + if (WARN_ON(!ldev->suspend_state))
> > + return -ENOENT;
> > +
> > pm_runtime_force_resume(dev);
> > ret = drm_atomic_helper_resume(ddev, ldev->suspend_state);
> > - if (ret) {
> > + if (ret)
> > pm_runtime_force_suspend(dev);
> > - ldev->suspend_state = NULL;
> > - return ret;
> > - }
> >
> > - return 0;
> > + ldev->suspend_state = NULL;
> > +
> > + return ret;
> > }
> >
> > static __maybe_unused int drv_runtime_suspend(struct device *dev)
> >
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

2019-06-20 17:15:12

by Emil Velikov

Subject: Re: [PATCH 1/3] drm/stm: drv: fix suspend/resume

Hi Yannick,

On Mon, 17 Jun 2019 at 08:18, Yannick Fertré <[email protected]> wrote:

> @@ -155,15 +154,17 @@ static __maybe_unused int drv_resume(struct device *dev)
> struct ltdc_device *ldev = ddev->dev_private;
> int ret;
>
> + if (WARN_ON(!ldev->suspend_state))
> + return -ENOENT;
> +
> pm_runtime_force_resume(dev);
> ret = drm_atomic_helper_resume(ddev, ldev->suspend_state);
> - if (ret) {
> + if (ret)
Hmm the msm driver uses !ret here. Suspecting that you want the same,
although I haven't checked in detail.

HTH
-Emil

2019-06-21 14:02:23

by Yannick FERTRE

Subject: Re: [PATCH 1/3] drm/stm: drv: fix suspend/resume

Hi Emil,

The msm driver tests the return value & set state to NULL if no error is
detected.

the ltdc driver tests the return value & force to suspend if an error is
detected.

It's not exactly the same.

Best regards


--
Yannick Fertré | TINA: 166 7152 | Tel: +33 244027152 | Mobile: +33 620600270
Microcontrollers and Digital ICs Group | Microcontrolleurs Division

On 6/20/19 7:12 PM, Emil Velikov wrote:
> Hi Yannick,
>
> On Mon, 17 Jun 2019 at 08:18, Yannick Fertré <[email protected]> wrote:
>
>> @@ -155,15 +154,17 @@ static __maybe_unused int drv_resume(struct device *dev)
>> struct ltdc_device *ldev = ddev->dev_private;
>> int ret;
>>
>> + if (WARN_ON(!ldev->suspend_state))
>> + return -ENOENT;
>> +
>> pm_runtime_force_resume(dev);
>> ret = drm_atomic_helper_resume(ddev, ldev->suspend_state);
>> - if (ret) {
>> + if (ret)
> Hmm the msm driver uses !ret here. Suspecting that you want the same,
> although I haven't checked in detail.
>
> HTH
> -Emil
--
Yannick Fertré | TINA: 166 7152 | Tel: +33 244027152 | Mobile: +33 620600270
Microcontrollers and Digital ICs Group | Microcontrolleurs Division

2019-06-21 14:56:27

by Emil Velikov

Subject: Re: [PATCH 1/3] drm/stm: drv: fix suspend/resume

On Fri, 21 Jun 2019 at 15:01, Yannick FERTRE <[email protected]> wrote:
>
> Hi Emil,
>
> The msm driver tests the return value & set state to NULL if no error is
> detected.
>
> the ltdc driver tests the return value & force to suspend if an error is
> detected.
>
> It's not exactly the same.
>
D'oh I've misread that patch as pm_runtime_force_suspend() being
called when the atomic helper succeeds.

Thanks for the correction :-)
Emil