On Tuesday 10 Jan 2017 11:39:03 Daniel Vetter wrote:
> On Mon, Jan 09, 2017 at 07:56:24PM +0800, Shawn Guo wrote:
> > From: Shawn Guo <shawn.guo@xxxxxxxxxx>
> >
> > The vblank is mostly CRTC specific and implemented as part of CRTC
> > driver. So having vblank hooks in struct drm_crtc_funcs should
> > generally help to reduce code from client drivers in implementing
> > drm_driver's vblank callbacks.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@xxxxxxxxxx>
> > ---
> >
> > drivers/gpu/drm/drm_crtc.c | 36 ++++++++++++++++++++++++++++++++++++
> > include/drm/drm_crtc.h | 21 +++++++++++++++++++++
> > 2 files changed, 57 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 85a7452d0fb4..59ff00f48101 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -70,6 +70,42 @@ struct drm_crtc *drm_crtc_from_index(struct drm_device
> > *dev, int idx)>
> > EXPORT_SYMBOL(drm_crtc_from_index);
> >
> > /**
> >
> > + * drm_crtc_enable_vblank - vblank enable callback helper
> > + * @dev: DRM device
> > + * @pipe: CRTC index
> > + *
> > + * It's a helper function as the generic vblank enable callback
> > implementation, + * which calls into &drm_crtc_funcs.enable_vblank
> > function.
> > + */
> > +int drm_crtc_enable_vblank(struct drm_device *dev, unsigned int pipe)
> > +{
> > + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> > +
> > + if (crtc && crtc->funcs && crtc->funcs->enable_vblank)
> > + return crtc->funcs->enable_vblank(crtc);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_crtc_enable_vblank);
>
> With the helper approach here there's still a pile of boilerplate in
> drivers (well, 2 lines to fill out the legacy helpers). What if instead we
> wrap all callers of enable/disable_vblank in drm_irq.c into something like
>
> __enable_vblank(dev, pipe)
> {
> if (DRIVER_MODESET) /* otherwise we'll oops on legacy drivers */
> {
> /* above code to call the new hook, if it's there. */
>
> if (crtc->funcs->enable_vblank)
> return crtc->funcs->enable_vblank(crtc);
> }
>
> /* fallback for everyone else */
>
> dev->driver->enable_vblank(dev, pipe);
> }
FWIW I like that approach much better. I'd even go as far as saying that
DRIVER_MODESET drivers should be mass-converted.
> > +
> > +/**
> > + * drm_crtc_disable_vblank - vblank disable callback helper
> > + * @dev: DRM device
> > + * @pipe: CRTC index
> > + *
> > + * It's a helper function as the generic vblank disable callback
> > implementation,
> > + * which calls into &drm_crtc_funcs.disable_vblank function.
> > + */
> > +void drm_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe)
> > +{
> > + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> > +
> > + if (crtc && crtc->funcs && crtc->funcs->disable_vblank)
> > + return crtc->funcs->disable_vblank(crtc);
> > +}
> > +EXPORT_SYMBOL(drm_crtc_disable_vblank);
> > +
> > +/**
> > * drm_crtc_force_disable - Forcibly turn off a CRTC
> > * @crtc: CRTC to turn off
> > *
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
