Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1395210 > unrolled thread

[PATCH 2/2] drm: hdlcd: Suspend/resume only active crtcs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] drm: hdlcd: Suspend/resume only active crtcs Robin Murphy <robin.murphy@arm.com> - 2016-05-05 18:20 +0200
    Re: [PATCH 2/2] drm: hdlcd: Suspend/resume only active crtcs liviu.dudau@arm.com - 2016-05-05 19:00 +0200
    Re: [PATCH 2/2] drm: hdlcd: Suspend/resume only active crtcs Daniel Vetter <daniel@ffwll.ch> - 2016-05-05 19:10 +0200
      Re: [PATCH 2/2] drm: hdlcd: Suspend/resume only active crtcs liviu.dudau@arm.com - 2016-05-05 19:20 +0200
      Re: [PATCH 2/2] drm: hdlcd: Suspend/resume only active crtcs Robin Murphy <robin.murphy@arm.com> - 2016-05-05 20:10 +0200

#1395210 — [PATCH 2/2] drm: hdlcd: Suspend/resume only active crtcs

The current PM ops simply unconditionally enable/disable the HDLCD,
which proves problematic when there is no display plugged in - since
without a crtc the hardware itself is still in an uninitialised state,
coming out of suspend results in it being enabled without a valid
framebuffer address, which typically results in it trying to scan out
from bus address 0 and flooding the system with error interrupts.

Fix this by checking the crtc state on resume, and only enabling the
hardware if it's actually supposed to be. For the sake of consistency,
do the same on the suspend path as well, although there it's merely a
case of skipping unnecessary work.

CC: Liviu Dudau <liviu.dudau@arm.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/gpu/drm/arm/hdlcd_crtc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index fef1b04c2aab..bf6ff5e48adc 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -296,12 +296,14 @@ static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
 
 void hdlcd_crtc_suspend(struct drm_crtc *crtc)
 {
-	hdlcd_crtc_disable(crtc);
+	if (crtc->state->active)
+		hdlcd_crtc_disable(crtc);
 }
 
 void hdlcd_crtc_resume(struct drm_crtc *crtc)
 {
-	hdlcd_crtc_enable(crtc);
+	if (crtc->state->active)
+		hdlcd_crtc_enable(crtc);
 }
 
 int hdlcd_setup_crtc(struct drm_device *drm)
-- 
2.8.1.dirty

[toc] | [next] | [standalone]

#1395230

On Thu, May 05, 2016 at 05:13:38PM +0100, Robin Murphy wrote:
> The current PM ops simply unconditionally enable/disable the HDLCD,
> which proves problematic when there is no display plugged in - since
> without a crtc the hardware itself is still in an uninitialised state,
> coming out of suspend results in it being enabled without a valid
> framebuffer address, which typically results in it trying to scan out
> from bus address 0 and flooding the system with error interrupts.
> 
> Fix this by checking the crtc state on resume, and only enabling the
> hardware if it's actually supposed to be. For the sake of consistency,
> do the same on the suspend path as well, although there it's merely a
> case of skipping unnecessary work.
> 
> CC: Liviu Dudau <liviu.dudau@arm.com>

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

Thanks for the patch, Robin!

Liviu

> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/gpu/drm/arm/hdlcd_crtc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> index fef1b04c2aab..bf6ff5e48adc 100644
> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> @@ -296,12 +296,14 @@ static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
>  
>  void hdlcd_crtc_suspend(struct drm_crtc *crtc)
>  {
> -	hdlcd_crtc_disable(crtc);
> +	if (crtc->state->active)
> +		hdlcd_crtc_disable(crtc);
>  }
>  
>  void hdlcd_crtc_resume(struct drm_crtc *crtc)
>  {
> -	hdlcd_crtc_enable(crtc);
> +	if (crtc->state->active)
> +		hdlcd_crtc_enable(crtc);
>  }
>  
>  int hdlcd_setup_crtc(struct drm_device *drm)
> -- 
> 2.8.1.dirty
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

[toc] | [prev] | [next] | [standalone]

#1395234

On Thu, May 05, 2016 at 05:13:38PM +0100, Robin Murphy wrote:
> The current PM ops simply unconditionally enable/disable the HDLCD,
> which proves problematic when there is no display plugged in - since
> without a crtc the hardware itself is still in an uninitialised state,
> coming out of suspend results in it being enabled without a valid
> framebuffer address, which typically results in it trying to scan out
> from bus address 0 and flooding the system with error interrupts.
> 
> Fix this by checking the crtc state on resume, and only enabling the
> hardware if it's actually supposed to be. For the sake of consistency,
> do the same on the suspend path as well, although there it's merely a
> case of skipping unnecessary work.
> 
> CC: Liviu Dudau <liviu.dudau@arm.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/gpu/drm/arm/hdlcd_crtc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> index fef1b04c2aab..bf6ff5e48adc 100644
> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> @@ -296,12 +296,14 @@ static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
>  
>  void hdlcd_crtc_suspend(struct drm_crtc *crtc)
>  {
> -	hdlcd_crtc_disable(crtc);
> +	if (crtc->state->active)
> +		hdlcd_crtc_disable(crtc);
>  }
>  
>  void hdlcd_crtc_resume(struct drm_crtc *crtc)
>  {
> -	hdlcd_crtc_enable(crtc);
> +	if (crtc->state->active)
> +		hdlcd_crtc_enable(crtc);
>  }

If you use the atomic helpers to suspend/resume your entire display
pipeline these callbacks shouldn't even be needed at all. Tried just
removing them?
-Daniel

>  
>  int hdlcd_setup_crtc(struct drm_device *drm)
> -- 
> 2.8.1.dirty
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1395239

On Thu, May 05, 2016 at 07:06:02PM +0200, Daniel Vetter wrote:
> On Thu, May 05, 2016 at 05:13:38PM +0100, Robin Murphy wrote:
> > The current PM ops simply unconditionally enable/disable the HDLCD,
> > which proves problematic when there is no display plugged in - since
> > without a crtc the hardware itself is still in an uninitialised state,
> > coming out of suspend results in it being enabled without a valid
> > framebuffer address, which typically results in it trying to scan out
> > from bus address 0 and flooding the system with error interrupts.
> > 
> > Fix this by checking the crtc state on resume, and only enabling the
> > hardware if it's actually supposed to be. For the sake of consistency,
> > do the same on the suspend path as well, although there it's merely a
> > case of skipping unnecessary work.
> > 
> > CC: Liviu Dudau <liviu.dudau@arm.com>
> > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > ---
> >  drivers/gpu/drm/arm/hdlcd_crtc.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> > index fef1b04c2aab..bf6ff5e48adc 100644
> > --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> > +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> > @@ -296,12 +296,14 @@ static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
> >  
> >  void hdlcd_crtc_suspend(struct drm_crtc *crtc)
> >  {
> > -	hdlcd_crtc_disable(crtc);
> > +	if (crtc->state->active)
> > +		hdlcd_crtc_disable(crtc);
> >  }
> >  
> >  void hdlcd_crtc_resume(struct drm_crtc *crtc)
> >  {
> > -	hdlcd_crtc_enable(crtc);
> > +	if (crtc->state->active)
> > +		hdlcd_crtc_enable(crtc);
> >  }
> 
> If you use the atomic helpers to suspend/resume your entire display
> pipeline these callbacks shouldn't even be needed at all. Tried just
> removing them?

Yes, I need to cleanup the PM code in HDLCD.

Thanks,
Liviu


> -Daniel
> 
> >  
> >  int hdlcd_setup_crtc(struct drm_device *drm)
> > -- 
> > 2.8.1.dirty
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

[toc] | [prev] | [next] | [standalone]

#1395263

Hi Daniel,

On 05/05/16 18:06, Daniel Vetter wrote:
> On Thu, May 05, 2016 at 05:13:38PM +0100, Robin Murphy wrote:
>> The current PM ops simply unconditionally enable/disable the HDLCD,
>> which proves problematic when there is no display plugged in - since
>> without a crtc the hardware itself is still in an uninitialised state,
>> coming out of suspend results in it being enabled without a valid
>> framebuffer address, which typically results in it trying to scan out
>> from bus address 0 and flooding the system with error interrupts.
>>
>> Fix this by checking the crtc state on resume, and only enabling the
>> hardware if it's actually supposed to be. For the sake of consistency,
>> do the same on the suspend path as well, although there it's merely a
>> case of skipping unnecessary work.
>>
>> CC: Liviu Dudau <liviu.dudau@arm.com>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/gpu/drm/arm/hdlcd_crtc.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
>> index fef1b04c2aab..bf6ff5e48adc 100644
>> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
>> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
>> @@ -296,12 +296,14 @@ static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
>>
>>   void hdlcd_crtc_suspend(struct drm_crtc *crtc)
>>   {
>> -	hdlcd_crtc_disable(crtc);
>> +	if (crtc->state->active)
>> +		hdlcd_crtc_disable(crtc);
>>   }
>>
>>   void hdlcd_crtc_resume(struct drm_crtc *crtc)
>>   {
>> -	hdlcd_crtc_enable(crtc);
>> +	if (crtc->state->active)
>> +		hdlcd_crtc_enable(crtc);
>>   }
>
> If you use the atomic helpers to suspend/resume your entire display
> pipeline these callbacks shouldn't even be needed at all. Tried just
> removing them?

I'll have to leave that in Liviu's hands as I know literally nothing 
about the relationship between platform PM ops and DRM helpers ;)
My only motivation is for the arm64 hibernate support currently sat in 
-next for 4.7 to stop being broken on my Juno board by this.

Robin.

> -Daniel
>
>>
>>   int hdlcd_setup_crtc(struct drm_device *drm)
>> --
>> 2.8.1.dirty
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web