[PATCH 12/22] drm/sti: Use drm_fb_cma_fbdev_init/fini()
Daniel Vetter
daniel at ffwll.ch
Thu Nov 9 08:20:16 UTC 2017
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.
> 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