[PATCH 12/22] drm/sti: Use drm_fb_cma_fbdev_init/fini()

[Noralf Trønnes]

Den 08.11.2017 13.21, skrev Benjamin Gaignard:
> 2017-11-04 14:04 GMT+01:00 Noralf Trønnes <noralf at tronnes.org>:
>> Use drm_fb_cma_fbdev_init() and drm_fb_cma_fbdev_fini() which relies on
>> the fact that drm_device holds a pointer to the drm_fb_helper structure.
>> This means that the driver doesn't have to keep track of that.
>> Also use the drm_fb_helper functions directly.
>>
>> Cc: Benjamin Gaignard <benjamin.gaignard at linaro.org>
>> Cc: Vincent Abriou <vincent.abriou at st.com>
>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>> ---
>>   drivers/gpu/drm/sti/sti_drv.c | 25 +++++--------------------
>>   drivers/gpu/drm/sti/sti_drv.h |  1 -
>>   2 files changed, 5 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c
>> index 9e9343101738..d61efef30a82 100644
>> --- a/drivers/gpu/drm/sti/sti_drv.c
>> +++ b/drivers/gpu/drm/sti/sti_drv.c
>> @@ -17,6 +17,7 @@
>>   #include <drm/drm_crtc_helper.h>
>>   #include <drm/drm_gem_cma_helper.h>
>>   #include <drm/drm_gem_framebuffer_helper.h>
>> +#include <drm/drm_fb_helper.h>
>>   #include <drm/drm_fb_cma_helper.h>
>>   #include <drm/drm_of.h>
>>
>> @@ -138,16 +139,9 @@ static int sti_atomic_check(struct drm_device *dev,
>>          return ret;
>>   }
>>
>> -static void sti_output_poll_changed(struct drm_device *ddev)
>> -{
>> -       struct sti_private *private = ddev->dev_private;
>> -
>> -       drm_fbdev_cma_hotplug_event(private->fbdev);
>> -}
>> -
>>   static const struct drm_mode_config_funcs sti_mode_config_funcs = {
>>          .fb_create = drm_gem_fb_create,
>> -       .output_poll_changed = sti_output_poll_changed,
>> +       .output_poll_changed = drm_fb_helper_output_poll_changed,
>>          .atomic_check = sti_atomic_check,
>>          .atomic_commit = drm_atomic_helper_commit,
>>   };
>> @@ -230,11 +224,7 @@ static void sti_cleanup(struct drm_device *ddev)
>>   {
>>          struct sti_private *private = ddev->dev_private;
>>
>> -       if (private->fbdev) {
>> -               drm_fbdev_cma_fini(private->fbdev);
>> -               private->fbdev = NULL;
>> -       }
>> -
>> +       drm_fb_cma_fbdev_fini(ddev);
>>          drm_kms_helper_poll_fini(ddev);
>>          component_unbind_all(ddev->dev, ddev);
>>          kfree(private);
>> @@ -245,7 +235,6 @@ static int sti_bind(struct device *dev)
>>   {
>>          struct drm_device *ddev;
>>          struct sti_private *private;
>> -       struct drm_fbdev_cma *fbdev;
>>          int ret;
>>
>>          ddev = drm_dev_alloc(&sti_driver, dev);
>> @@ -268,13 +257,9 @@ static int sti_bind(struct device *dev)
>>
>>          private = ddev->dev_private;
>>          if (ddev->mode_config.num_connector) {
>> -               fbdev = drm_fbdev_cma_init(ddev, 32,
>> -                                          ddev->mode_config.num_connector);
>> -               if (IS_ERR(fbdev)) {
>> +               ret = drm_fb_cma_fbdev_init(ddev, 32, 0);
>> +               if (ret)
>>                          DRM_DEBUG_DRIVER("Warning: fails to create fbdev\n");
>> -                       fbdev = NULL;
>> -               }
>> -               private->fbdev = fbdev;
>>          }
> Like for stm driver I think that drm_kms_helper_poll_init() call
> should be move after
> this block of code.

I fail to see how this patchset affects output polling.
What kind of problem are you seeing?

I think we need to understand what's happening here because more
drivers call drm_kms_helper_poll_init() before fbdev init.

These are the various call sequences in the cma helper drivers:

arc, hdlcd, kirin, mxsfb, rcar-du, stm, zte:

drm_mode_config_reset()
drm_kms_helper_poll_init()
drm_fbdev_cma_init()
drm_dev_register()


atmel-hlcdc, imx, meson, pl111, sun4i, tilcdc, tve200:

drm_mode_config_reset()
drm_fbdev_cma_init()
drm_kms_helper_poll_init()
drm_dev_register()


sti:

drm_kms_helper_poll_init()
drm_dev_register()
drm_mode_config_reset()
drm_fbdev_cma_init()


vc4:

drm_dev_register()
drm_mode_config_reset()
drm_fbdev_cma_init()
drm_kms_helper_poll_init()

> struct sti_private *private could also be remove of the function.

Will do.

> When testing your patches I have a kernel panic in drm_fbdev_cma_create()
> because fbdev_cma->fb_funcs is NULL so using/testing fbdev_cma->fb_funcs->dirty
> make kernel crash.

Sorry about this, I was certain that I had tested this on the vc4 driver,
but it's clear that I haven't :/

Thanks for catching this!

Noralf.

> Just adding a test on fbdev_cma->fb_funcs value fix the problem but I
> wonder why I'm
> the only to report this issue ?
>
> With all these fix I have a fonctionnal frame buffer.
>
> Benjamin
>>          return 0;
>> diff --git a/drivers/gpu/drm/sti/sti_drv.h b/drivers/gpu/drm/sti/sti_drv.h
>> index 6502ed2d3351..16c5c9110cb0 100644
>> --- a/drivers/gpu/drm/sti/sti_drv.h
>> +++ b/drivers/gpu/drm/sti/sti_drv.h
>> @@ -24,7 +24,6 @@ struct sti_private {
>>          struct sti_compositor *compo;
>>          struct drm_property *plane_zorder_property;
>>          struct drm_device *drm_dev;
>> -       struct drm_fbdev_cma *fbdev;
>>   };
>>
>>   extern struct platform_driver sti_tvout_driver;
>> --
>> 2.14.2
>>
>
>