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

[Daniel Vetter]

On Mon, Jan 09, 2017 at 07:56:24PM +0800, Shawn Guo wrote:
> From: Shawn Guo <shawn.guo at 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 at 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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch