Re: [PATCH 1/4] drm: add vblank hooks to struct drm_crtc_funcs

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@linaro.org>
> > 
> > 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@linaro.org>
> > ---
> > 
> >  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel