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

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(+)

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);
}

> +
> +/**
> + * 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
>   *
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index a5627eb8621f..20874b3903a6 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -599,6 +599,25 @@ struct drm_crtc_funcs {
>  	 */
>  	void (*atomic_print_state)(struct drm_printer *p,
>  				   const struct drm_crtc_state *state);
> +
> +	/**
> +	 * @enable_vblank:
> +	 *
> +	 * Enable vblank interrupts for the CRTC.
> +	 *
> +	 * Returns:
> +	 *
> +	 * Zero on success, appropriate errno if the vblank interrupt cannot
> +	 * be enabled.
> +	 */
> +	int (*enable_vblank)(struct drm_crtc *crtc);
> +
> +	/**
> +	 * @disable_vblank:
> +	 *
> +	 * Disable vblank interrupts for the CRTC.
> +	 */
> +	void (*disable_vblank)(struct drm_crtc *crtc);

Please also update the kerneldoc for these two hooks in drm_drv.h, and
link between the two (using the &drm_driver.enable_vblank syntax). And
then mention that the drm_driver one is deprecated.

Also I think it'd would make sense to do the same for
get_vblank_counter(), since those are the 3 core->driver interface
functions. The other 2 are kinda just helpers for precise vblank
timestamp support.
-Daniel

>  };
>  
>  /**
> @@ -831,6 +850,8 @@ void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
>  
>  int drm_mode_set_config_internal(struct drm_mode_set *set);
>  struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx);
> +int drm_crtc_enable_vblank(struct drm_device *dev, unsigned int pipe);
> +void drm_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe);
>  
>  /* Helpers */
>  static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
> -- 
> 1.9.1
>

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
> >   *