[PATCH 1/2] drm: hdlcd: Skip PM callbacks if unbound

[PATCH 1/2] drm: hdlcd: Skip PM callbacks if unbound

[Robin Murphy]

Until an encoder component is available to trigger the HDLCD's component
master bind callback, the drm_device we keep in drvdata isn't allocated.
Since it is quite possible to, say, hibernate the system without having
loaded the encoder driver, the PM callbacks need to check for a valid
drm_device before trying to take the lock and suspend/resume the crtcs,
otherwise this happens:

[   51.779092] Unable to handle kernel NULL pointer dereference at virtual address 00000200
[   51.787143] pgd = ffff800974462000
[   51.790514] [00000200] *pgd=0000000000000000
[   51.794768] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[   51.800291] Modules linked in:
[   51.803325] CPU: 4 PID: 2098 Comm: systemd-sleep Not tainted 4.6.0-rc6+ #4
[   51.810140] Hardware name: ARM Juno development board (r1) (DT)
[   51.816008] task: ffff800974430000 ti: ffff80097380c000 task.ti: ffff80097380c000
[   51.823433] PC is at mutex_lock+0x14/0x60
[   51.827411] LR is at drm_modeset_lock_all+0x3c/0xe0
[   51.832246] pc : [<ffff0000087d55c4>] lr : [<ffff0000084839fc>] pstate: 40000145
[   51.839577] sp : ffff80097380faf0
[   51.842859] x29: ffff80097380faf0 x28: 0000000000000000
[   51.848132] x27: 0000000000000002 x26: ffff000008e01000
[   51.853405] x25: ffff0000084cc000 x24: ffff000008dcb408
[   51.858678] x23: ffff000008e7c000 x22: ffff8009760e8870
[   51.863962] x21: 0000000000000200 x20: 0000000000000000
[   51.869244] x19: 0000000000000200 x18: ffff800079357dd0
[   51.874527] x17: 0000ffff8a00ab88 x16: 0000000000000018
[   51.879809] x15: 0000000000000018 x14: 0000000000000000
[   51.885091] x13: 0000000000000002 x12: 0000000000000000
[   51.890374] x11: ffff0000087ff904 x10: 0000000000000840
[   51.895656] x9 : 0000000000000000 x8 : ffff800973821680
[   51.900938] x7 : 0000000000000000 x6 : 000000000000003f
[   51.906220] x5 : 0000000000000040 x4 : 0000000000000000
[   51.911502] x3 : 0000000000000004 x2 : 0000000000000000
[   51.916784] x1 : 0000000000000001 x0 : 0000000000000200
...
[   52.339406] [<ffff0000087d55c4>] mutex_lock+0x14/0x60
[   52.344425] [<ffff0000084839fc>] drm_modeset_lock_all+0x3c/0xe0
[   52.350305] [<ffff00000848a034>] hdlcd_pm_suspend+0x2c/0x90
[   52.355842] [<ffff0000084c0d14>] platform_pm_suspend+0x24/0x58
[   52.361638] [<ffff0000084cc190>] dpm_run_callback.isra.4+0x20/0x68
[   52.367778] [<ffff0000084ccad4>] __device_suspend+0xe4/0x238
[   52.373400] [<ffff0000084cdcac>] dpm_suspend+0x10c/0x240
[   52.378679] [<ffff0000084ce0b8>] dpm_suspend_start+0x70/0x80
[   52.384302] [<ffff0000080f80e4>] suspend_devices_and_enter+0x9c/0x480
[   52.390699] [<ffff0000080f8698>] pm_suspend+0x1d0/0x240
[   52.395890] [<ffff0000080f7280>] state_store+0x80/0x100
[   52.401084] [<ffff0000083503f4>] kobj_attr_store+0x14/0x28
[   52.406535] [<ffff000008238580>] sysfs_kf_write+0x48/0x58
[   52.411899] [<ffff00000823798c>] kernfs_fop_write+0xbc/0x188
[   52.417521] [<ffff0000081c71f4>] __vfs_write+0x1c/0xe0
[   52.422626] [<ffff0000081c8144>] vfs_write+0x8c/0x1a8
[   52.427644] [<ffff0000081c953c>] SyS_write+0x44/0xa0
[   52.432578] [<ffff000008084ecc>] __sys_trace_return+0x0/0x4
[   52.438114] Code: 910003fd f9000bf3 aa0003f3 f9800011 (885ffc01)
[   52.444229] ---[ end trace 34b87a5fb9453f65 ]---

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

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index 734899c4e4bb..86a5eea4cf53 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -498,7 +498,7 @@ static int __maybe_unused hdlcd_pm_suspend(struct device *dev)
 	struct drm_device *drm = dev_get_drvdata(dev);
 	struct drm_crtc *crtc;
 
-	if (pm_runtime_suspended(dev))
+	if (pm_runtime_suspended(dev) || !drm)
 		return 0;
 
 	drm_modeset_lock_all(drm);
@@ -513,7 +513,7 @@ static int __maybe_unused hdlcd_pm_resume(struct device *dev)
 	struct drm_device *drm = dev_get_drvdata(dev);
 	struct drm_crtc *crtc;
 
-	if (!pm_runtime_suspended(dev))
+	if (!pm_runtime_suspended(dev) || !drm)
 		return 0;
 
 	drm_modeset_lock_all(drm);
-- 
2.8.1.dirty

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

[Robin Murphy]

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

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

[liviu.dudau at arm.com]

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!  /
  ---------------
    ?\_(?)_/?

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

[Daniel Vetter]

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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

[liviu.dudau at arm.com]

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 at 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!  /
  ---------------
    ?\_(?)_/?

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

[Robin Murphy]

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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[PATCH 1/2] drm: hdlcd: Skip PM callbacks if unbound

[Thierry Reding]

On Thu, May 05, 2016 at 05:13:37PM +0100, Robin Murphy wrote:
> Until an encoder component is available to trigger the HDLCD's component
> master bind callback, the drm_device we keep in drvdata isn't allocated.
> Since it is quite possible to, say, hibernate the system without having
> loaded the encoder driver, the PM callbacks need to check for a valid
> drm_device before trying to take the lock and suspend/resume the crtcs,
> otherwise this happens:
> 
> [   51.779092] Unable to handle kernel NULL pointer dereference at virtual address 00000200
> [   51.787143] pgd = ffff800974462000
> [   51.790514] [00000200] *pgd=0000000000000000
> [   51.794768] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> [   51.800291] Modules linked in:
> [   51.803325] CPU: 4 PID: 2098 Comm: systemd-sleep Not tainted 4.6.0-rc6+ #4
> [   51.810140] Hardware name: ARM Juno development board (r1) (DT)
> [   51.816008] task: ffff800974430000 ti: ffff80097380c000 task.ti: ffff80097380c000
> [   51.823433] PC is at mutex_lock+0x14/0x60
> [   51.827411] LR is at drm_modeset_lock_all+0x3c/0xe0
> [   51.832246] pc : [<ffff0000087d55c4>] lr : [<ffff0000084839fc>] pstate: 40000145
> [   51.839577] sp : ffff80097380faf0
> [   51.842859] x29: ffff80097380faf0 x28: 0000000000000000
> [   51.848132] x27: 0000000000000002 x26: ffff000008e01000
> [   51.853405] x25: ffff0000084cc000 x24: ffff000008dcb408
> [   51.858678] x23: ffff000008e7c000 x22: ffff8009760e8870
> [   51.863962] x21: 0000000000000200 x20: 0000000000000000
> [   51.869244] x19: 0000000000000200 x18: ffff800079357dd0
> [   51.874527] x17: 0000ffff8a00ab88 x16: 0000000000000018
> [   51.879809] x15: 0000000000000018 x14: 0000000000000000
> [   51.885091] x13: 0000000000000002 x12: 0000000000000000
> [   51.890374] x11: ffff0000087ff904 x10: 0000000000000840
> [   51.895656] x9 : 0000000000000000 x8 : ffff800973821680
> [   51.900938] x7 : 0000000000000000 x6 : 000000000000003f
> [   51.906220] x5 : 0000000000000040 x4 : 0000000000000000
> [   51.911502] x3 : 0000000000000004 x2 : 0000000000000000
> [   51.916784] x1 : 0000000000000001 x0 : 0000000000000200
> ...
> [   52.339406] [<ffff0000087d55c4>] mutex_lock+0x14/0x60
> [   52.344425] [<ffff0000084839fc>] drm_modeset_lock_all+0x3c/0xe0
> [   52.350305] [<ffff00000848a034>] hdlcd_pm_suspend+0x2c/0x90
> [   52.355842] [<ffff0000084c0d14>] platform_pm_suspend+0x24/0x58
> [   52.361638] [<ffff0000084cc190>] dpm_run_callback.isra.4+0x20/0x68
> [   52.367778] [<ffff0000084ccad4>] __device_suspend+0xe4/0x238
> [   52.373400] [<ffff0000084cdcac>] dpm_suspend+0x10c/0x240
> [   52.378679] [<ffff0000084ce0b8>] dpm_suspend_start+0x70/0x80
> [   52.384302] [<ffff0000080f80e4>] suspend_devices_and_enter+0x9c/0x480
> [   52.390699] [<ffff0000080f8698>] pm_suspend+0x1d0/0x240
> [   52.395890] [<ffff0000080f7280>] state_store+0x80/0x100
> [   52.401084] [<ffff0000083503f4>] kobj_attr_store+0x14/0x28
> [   52.406535] [<ffff000008238580>] sysfs_kf_write+0x48/0x58
> [   52.411899] [<ffff00000823798c>] kernfs_fop_write+0xbc/0x188
> [   52.417521] [<ffff0000081c71f4>] __vfs_write+0x1c/0xe0
> [   52.422626] [<ffff0000081c8144>] vfs_write+0x8c/0x1a8
> [   52.427644] [<ffff0000081c953c>] SyS_write+0x44/0xa0
> [   52.432578] [<ffff000008084ecc>] __sys_trace_return+0x0/0x4
> [   52.438114] Code: 910003fd f9000bf3 aa0003f3 f9800011 (885ffc01)
> [   52.444229] ---[ end trace 34b87a5fb9453f65 ]---
> 
> Acked-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/gpu/drm/arm/hdlcd_drv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> index 734899c4e4bb..86a5eea4cf53 100644
> --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> @@ -498,7 +498,7 @@ static int __maybe_unused hdlcd_pm_suspend(struct device *dev)
>  	struct drm_device *drm = dev_get_drvdata(dev);
>  	struct drm_crtc *crtc;
>  
> -	if (pm_runtime_suspended(dev))
> +	if (pm_runtime_suspended(dev) || !drm)
>  		return 0;
>  
>  	drm_modeset_lock_all(drm);
> @@ -513,7 +513,7 @@ static int __maybe_unused hdlcd_pm_resume(struct device *dev)
>  	struct drm_device *drm = dev_get_drvdata(dev);
>  	struct drm_crtc *crtc;
>  
> -	if (!pm_runtime_suspended(dev))
> +	if (!pm_runtime_suspended(dev) || !drm)
>  		return 0;

Perhaps rather than this workaround you should only enable runtime PM on
this after the master has been bound?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160506/f3fa93c9/attachment-0001.sig>

[PATCH 1/2] drm: hdlcd: Skip PM callbacks if unbound

[liviu.dudau at arm.com]

On Fri, May 06, 2016 at 04:54:17PM +0200, Thierry Reding wrote:
> On Thu, May 05, 2016 at 05:13:37PM +0100, Robin Murphy wrote:
> > Until an encoder component is available to trigger the HDLCD's component
> > master bind callback, the drm_device we keep in drvdata isn't allocated.
> > Since it is quite possible to, say, hibernate the system without having
> > loaded the encoder driver, the PM callbacks need to check for a valid
> > drm_device before trying to take the lock and suspend/resume the crtcs,
> > otherwise this happens:
> > 
> > [   51.779092] Unable to handle kernel NULL pointer dereference at virtual address 00000200
> > [   51.787143] pgd = ffff800974462000
> > [   51.790514] [00000200] *pgd=0000000000000000
> > [   51.794768] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> > [   51.800291] Modules linked in:
> > [   51.803325] CPU: 4 PID: 2098 Comm: systemd-sleep Not tainted 4.6.0-rc6+ #4
> > [   51.810140] Hardware name: ARM Juno development board (r1) (DT)
> > [   51.816008] task: ffff800974430000 ti: ffff80097380c000 task.ti: ffff80097380c000
> > [   51.823433] PC is at mutex_lock+0x14/0x60
> > [   51.827411] LR is at drm_modeset_lock_all+0x3c/0xe0
> > [   51.832246] pc : [<ffff0000087d55c4>] lr : [<ffff0000084839fc>] pstate: 40000145
> > [   51.839577] sp : ffff80097380faf0
> > [   51.842859] x29: ffff80097380faf0 x28: 0000000000000000
> > [   51.848132] x27: 0000000000000002 x26: ffff000008e01000
> > [   51.853405] x25: ffff0000084cc000 x24: ffff000008dcb408
> > [   51.858678] x23: ffff000008e7c000 x22: ffff8009760e8870
> > [   51.863962] x21: 0000000000000200 x20: 0000000000000000
> > [   51.869244] x19: 0000000000000200 x18: ffff800079357dd0
> > [   51.874527] x17: 0000ffff8a00ab88 x16: 0000000000000018
> > [   51.879809] x15: 0000000000000018 x14: 0000000000000000
> > [   51.885091] x13: 0000000000000002 x12: 0000000000000000
> > [   51.890374] x11: ffff0000087ff904 x10: 0000000000000840
> > [   51.895656] x9 : 0000000000000000 x8 : ffff800973821680
> > [   51.900938] x7 : 0000000000000000 x6 : 000000000000003f
> > [   51.906220] x5 : 0000000000000040 x4 : 0000000000000000
> > [   51.911502] x3 : 0000000000000004 x2 : 0000000000000000
> > [   51.916784] x1 : 0000000000000001 x0 : 0000000000000200
> > ...
> > [   52.339406] [<ffff0000087d55c4>] mutex_lock+0x14/0x60
> > [   52.344425] [<ffff0000084839fc>] drm_modeset_lock_all+0x3c/0xe0
> > [   52.350305] [<ffff00000848a034>] hdlcd_pm_suspend+0x2c/0x90
> > [   52.355842] [<ffff0000084c0d14>] platform_pm_suspend+0x24/0x58
> > [   52.361638] [<ffff0000084cc190>] dpm_run_callback.isra.4+0x20/0x68
> > [   52.367778] [<ffff0000084ccad4>] __device_suspend+0xe4/0x238
> > [   52.373400] [<ffff0000084cdcac>] dpm_suspend+0x10c/0x240
> > [   52.378679] [<ffff0000084ce0b8>] dpm_suspend_start+0x70/0x80
> > [   52.384302] [<ffff0000080f80e4>] suspend_devices_and_enter+0x9c/0x480
> > [   52.390699] [<ffff0000080f8698>] pm_suspend+0x1d0/0x240
> > [   52.395890] [<ffff0000080f7280>] state_store+0x80/0x100
> > [   52.401084] [<ffff0000083503f4>] kobj_attr_store+0x14/0x28
> > [   52.406535] [<ffff000008238580>] sysfs_kf_write+0x48/0x58
> > [   52.411899] [<ffff00000823798c>] kernfs_fop_write+0xbc/0x188
> > [   52.417521] [<ffff0000081c71f4>] __vfs_write+0x1c/0xe0
> > [   52.422626] [<ffff0000081c8144>] vfs_write+0x8c/0x1a8
> > [   52.427644] [<ffff0000081c953c>] SyS_write+0x44/0xa0
> > [   52.432578] [<ffff000008084ecc>] __sys_trace_return+0x0/0x4
> > [   52.438114] Code: 910003fd f9000bf3 aa0003f3 f9800011 (885ffc01)
> > [   52.444229] ---[ end trace 34b87a5fb9453f65 ]---
> > 
> > Acked-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > ---
> >  drivers/gpu/drm/arm/hdlcd_drv.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> > index 734899c4e4bb..86a5eea4cf53 100644
> > --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> > +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> > @@ -498,7 +498,7 @@ static int __maybe_unused hdlcd_pm_suspend(struct device *dev)
> >  	struct drm_device *drm = dev_get_drvdata(dev);
> >  	struct drm_crtc *crtc;
> >  
> > -	if (pm_runtime_suspended(dev))
> > +	if (pm_runtime_suspended(dev) || !drm)
> >  		return 0;
> >  
> >  	drm_modeset_lock_all(drm);
> > @@ -513,7 +513,7 @@ static int __maybe_unused hdlcd_pm_resume(struct device *dev)
> >  	struct drm_device *drm = dev_get_drvdata(dev);
> >  	struct drm_crtc *crtc;
> >  
> > -	if (!pm_runtime_suspended(dev))
> > +	if (!pm_runtime_suspended(dev) || !drm)
> >  		return 0;
> 
> Perhaps rather than this workaround you should only enable runtime PM on
> this after the master has been bound?

Yes, that is probably a better solution, even if the patch is going to be much bigger.

Best regards,
Liviu

> 
> Thierry


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