Re: [PATCH 04/12] drm: Add drm_vblank_on()

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2684 bytes --]

On Wed, May 14, 2014 at 08:51:06PM +0200, Daniel Vetter wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> drm_vblank_off() will turn off vblank interrupts, but as long as the
> refcount is elevated drm_vblank_get() will not re-enable them. This
> is a problem is someone is holding a vblank reference while a modeset is
> happening, and the driver requires vblank interrupt to work during that
> time.
> 
> Add drm_vblank_on() as a counterpart to drm_vblank_off() which will
> re-enabled vblank interrupts if the refcount is already elevated. This
> will allow drivers to choose the specific places in the modeset sequence
> at which vblank interrupts get disabled and enabled.
> 
> Testcase: igt/kms_flip/*-vs-suspend
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> [danvet: Add Testcase tag for the igt I've written.]
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c            | 72 ++++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_display.c |  8 ++++
>  include/drm/drmP.h                   |  1 +
>  3 files changed, 62 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 13d671ed3421..dd786d84daab 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -840,6 +840,41 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
>  }
>  
>  /**
> + * drm_vblank_enable - enable the vblank interrupt on a CRTC
> + * @dev: DRM device
> + * @crtc: CRTC in question
> + */

Perhaps the kernel-doc here should contain some of what's described in
the commit message? Also a "Return:" section would be useful here to
specify what's an error and what isn't.

> +static int drm_vblank_enable(struct drm_device *dev, int crtc)

On second thought, since this is a local function, my comments above
apply to drm_vblank_on() below rather than drm_vblank_enable().

> +{
> +	int ret = 0;
> +
> +	assert_spin_locked(&dev->vbl_lock);
> +
> +	spin_lock(&dev->vblank_time_lock);
> +
> +	if (!dev->vblank[crtc].enabled) {
> +		/* Enable vblank irqs under vblank_time_lock protection.
> +		 * All vblank count & timestamp updates are held off
> +		 * until we are done reinitializing master counter and
> +		 * timestamps. Filtercode in drm_handle_vblank() will
> +		 * prevent double-accounting of same vblank interval.
> +		 */

Coding style:

	/*
	 * Enable...
	 */

?

> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c

Perhaps split off the i915 changes into a separate patch?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel