[PATCH 12/22] drm/sti: Use drm_fb_cma_fbdev_init/fini()
Benjamin Gaignard
benjamin.gaignard at linaro.org
Thu Nov 9 09:20:21 UTC 2017
2017-11-09 9:20 GMT+01:00 Daniel Vetter <daniel at ffwll.ch>:
> On Wed, Nov 08, 2017 at 04:21:04PM +0100, Noralf Trønnes wrote:
>>
>> 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.
>
> That's kinda how you should be doing it - fbdev init needs to probe the
> outputs, if your poll work doesn't work yet then that's not going to pan
> out well. Now afaiui most of the current drivers need polling for fun
> stuff like TV-out or vga, so no one notices.
>
> And it might very well be that it's all 100% cargo-culted and people just
> put it there because everyone else does, not because they actually need
> output polling.
>>
>> 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()
>
> This looks reasonable.
>>
>>
>> atmel-hlcdc, imx, meson, pl111, sun4i, tilcdc, tve200:
>>
>> drm_mode_config_reset()
>> drm_fbdev_cma_init()
>> drm_kms_helper_poll_init()
>> drm_dev_register()
>
> Probably they don't really need polling.
>>
>>
>> sti:
>>
>> drm_kms_helper_poll_init()
>> drm_dev_register()
>> drm_mode_config_reset()
>> drm_fbdev_cma_init()
>
> drm_dev_register should be last. Otherwise even looks correct.
I have redo the test, current ordering is working even with the Noralf patches,
no need to change drm_kms_helper_poll_init() location.
Benjamin
>
>> vc4:
>>
>> drm_dev_register()
>> drm_mode_config_reset()
>> drm_fbdev_cma_init()
>> drm_kms_helper_poll_init()
>
> Very confusing. Hm I thought I've once checked the vc4 conversion to
> explicit register/unregister, I guess not carefully enough.
> -Daniel
>
>> > 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
>> > >
>> >
>> >
>>
>> _______________________________________________
>> 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
More information about the dri-devel
mailing list