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);
}
> +
> +/**
> + * 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
