diff mbox

[v3,5/5] drm/fsl-dcu: only init fbdev if required

Message ID de9f2276a089aae2be5d71291cfbac27@agner.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Stefan Agner Oct. 18, 2016, 5:42 p.m. UTC 
On 2016-10-18 00:44, Daniel Vetter wrote:
> On Mon, Oct 17, 2016 at 02:33:21PM -0700, Stefan Agner wrote:
>> There is no need to request a CMA backed framebuffer if fbdev
>> emulation is not enabled.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>> index e04efbe..3a5880c 100644
>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>> @@ -87,7 +87,8 @@ static int fsl_dcu_load(struct drm_device *dev, unsigned long flags)
>>  		goto done;
>>  	dev->irq_enabled = true;
>>
>> -	fsl_dcu_fbdev_init(dev);
>> +	if (IS_ENABLED(CONFIG_DRM_FBDEV_EMULATION))
>> +		fsl_dcu_fbdev_init(dev);
> 
> Totally not required, since this will all no-op out. Also, please nuke
> that fsl_dcu_fbdv_init wrapper seems like pointless indirection.

Regarding fsl_dcu_fbdev_init wrapper: Fully agreed. That thing was there
since inception of that driver, and I always was on the fence of doing
it. Will do it for the next merge window!

I somehow remembered there was something more to it than just "no need",
but I somehow failed to document it in the patch description... So I
went back and tested some things without the patch, here is when it
blows:

Unable to handle kernel NULL pointer dereference at virtual address
000002f0
pgd = cc24c000
[000002f0] *pgd=8c0df831, *pte=00000000, *ppte=00000000
Internal error: Oops: 17 [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 536 Comm: sh Not tainted 4.9.0-rc1-00001-g4b1532a-dirty #568
Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree)
task: cc213000 task.stack: cc23e000
PC is at drm_fbdev_cma_fini+0x14/0x5c
LR is at fsl_dcu_unload+0x28/0x4c
pc : [<c04cfb20>]    lr : [<c04fad74>]    psr: a0000013
sp : cc23fd80  ip : cc23fd98  fp : cc23fd94
r10: cc1d1e4c  r9 : cc23e000  r8 : 00000000
r7 : c0e34888  r6 : 0000000d  r5 : 00000000  r4 : ce6ff100
r3 : c0e13b18  r2 : c0e13b18  r1 : 00000110  r0 : ce6ff100
Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 8c24c04a  DAC: 00000051
Process sh (pid: 536, stack limit = 0xcc23e210)
Stack: (0xcc23fd80 to 0xcc240000)
...
Backtrace:
[<c04cfb0c>] (drm_fbdev_cma_fini) from [<c04fad74>]
(fsl_dcu_unload+0x28/0x4c)
 r5:ce61e810[  372.213609]  r4:ce61f000

[<c04fad4c>] (fsl_dcu_unload) from [<c04d9550>]
(drm_dev_unregister+0x38/0xbc)
 r5:ce61f000[  372.228535]  r4:ce61f000
...

> 
> And if there really is an issue with the cma helpers allocating an fb when
> they should, then the correct fix is to fix that in the helpers, not in
> the drivers.

Hm, to safe memory, it would probably be best to do something like:

                dev_err(dev->dev, "Failed to allocate drm fbdev.\n");

But we don't have drm_fbdev_emulation emulation there.

Maybe just add some NULL check in drm_fbdev_cma_fini?

--
Stefan


> 
> Nack.
> -Daniel
>>
>>  	return 0;
>>  done:
>> --
>> 2.10.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Comments

Daniel Vetter Oct. 19, 2016, 7:42 a.m. UTC | #1 
On Tue, Oct 18, 2016 at 10:42:46AM -0700, Stefan Agner wrote:
> On 2016-10-18 00:44, Daniel Vetter wrote:
> > On Mon, Oct 17, 2016 at 02:33:21PM -0700, Stefan Agner wrote:
> >> There is no need to request a CMA backed framebuffer if fbdev
> >> emulation is not enabled.
> >>
> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> ---
> >>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> >> index e04efbe..3a5880c 100644
> >> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> >> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> >> @@ -87,7 +87,8 @@ static int fsl_dcu_load(struct drm_device *dev, unsigned long flags)
> >>  		goto done;
> >>  	dev->irq_enabled = true;
> >>
> >> -	fsl_dcu_fbdev_init(dev);
> >> +	if (IS_ENABLED(CONFIG_DRM_FBDEV_EMULATION))
> >> +		fsl_dcu_fbdev_init(dev);
> > 
> > Totally not required, since this will all no-op out. Also, please nuke
> > that fsl_dcu_fbdv_init wrapper seems like pointless indirection.
> 
> Regarding fsl_dcu_fbdev_init wrapper: Fully agreed. That thing was there
> since inception of that driver, and I always was on the fence of doing
> it. Will do it for the next merge window!
> 
> I somehow remembered there was something more to it than just "no need",
> but I somehow failed to document it in the patch description... So I
> went back and tested some things without the patch, here is when it
> blows:
> 
> Unable to handle kernel NULL pointer dereference at virtual address
> 000002f0
> pgd = cc24c000
> [000002f0] *pgd=8c0df831, *pte=00000000, *ppte=00000000
> Internal error: Oops: 17 [#1] SMP ARM
> Modules linked in:
> CPU: 0 PID: 536 Comm: sh Not tainted 4.9.0-rc1-00001-g4b1532a-dirty #568
> Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree)
> task: cc213000 task.stack: cc23e000
> PC is at drm_fbdev_cma_fini+0x14/0x5c
> LR is at fsl_dcu_unload+0x28/0x4c
> pc : [<c04cfb20>]    lr : [<c04fad74>]    psr: a0000013
> sp : cc23fd80  ip : cc23fd98  fp : cc23fd94
> r10: cc1d1e4c  r9 : cc23e000  r8 : 00000000
> r7 : c0e34888  r6 : 0000000d  r5 : 00000000  r4 : ce6ff100
> r3 : c0e13b18  r2 : c0e13b18  r1 : 00000110  r0 : ce6ff100
> Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> Control: 10c5387d  Table: 8c24c04a  DAC: 00000051
> Process sh (pid: 536, stack limit = 0xcc23e210)
> Stack: (0xcc23fd80 to 0xcc240000)
> ...
> Backtrace:
> [<c04cfb0c>] (drm_fbdev_cma_fini) from [<c04fad74>]
> (fsl_dcu_unload+0x28/0x4c)
>  r5:ce61e810[  372.213609]  r4:ce61f000
> 
> [<c04fad4c>] (fsl_dcu_unload) from [<c04d9550>]
> (drm_dev_unregister+0x38/0xbc)
>  r5:ce61f000[  372.228535]  r4:ce61f000
> ...
> 
> > 
> > And if there really is an issue with the cma helpers allocating an fb when
> > they should, then the correct fix is to fix that in the helpers, not in
> > the drivers.
> 
> Hm, to safe memory, it would probably be best to do something like:
> 
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -492,6 +492,9 @@ struct drm_fbdev_cma
> *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
>         struct drm_fb_helper *helper;
>         int ret;
>  
> +       if (!drm_fbdev_emulation)
> +               return NULL;
> +
>         fbdev_cma = kzalloc(sizeof(*fbdev_cma), GFP_KERNEL);
>         if (!fbdev_cma) {
>                 dev_err(dev->dev, "Failed to allocate drm fbdev.\n");
> 
> But we don't have drm_fbdev_emulation emulation there.

Not just that, but you'd leak memory since cma_init always does the
kmalloc of the fbdev_cma struct.

> Maybe just add some NULL check in drm_fbdev_cma_fini?

Probably. I can't read arm-oopses, so no idea what exactly blew up. On a
hunch it's probably drm_fbdev_cma_defio_fini that got things wrong and
needs to check for !fbi || !fbi->defio.
-Daniel
diff mbox

Patch

--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -492,6 +492,9 @@  struct drm_fbdev_cma
*drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
        struct drm_fb_helper *helper;
        int ret;
 
+       if (!drm_fbdev_emulation)
+               return NULL;
+
        fbdev_cma = kzalloc(sizeof(*fbdev_cma), GFP_KERNEL);
        if (!fbdev_cma) {