Re: [PATCH 10/9] drm: Add dev->vblank_disable_immediate flag

[Matt Roper <matthew.d.roper@intel.com>]

On Mon, May 26, 2014 at 05:26:47PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add a flag to drm_device which will cause the vblank code to bypass the
> disable timer and always disable the vblank interrupt immediately when
> the last reference is dropped.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_irq.c |  6 +++---
>  include/drm/drmP.h        | 10 ++++++++++
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 20a4855..b008803 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -994,11 +994,11 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
>  
>  	/* Last user schedules interrupt disable */
>  	if (atomic_dec_and_test(&vblank->refcount)) {
> -		if (drm_vblank_offdelay > 0)
> +		if (dev->vblank_disable_immediate || drm_vblank_offdelay == 0)
> +			vblank_disable_fn((unsigned long)vblank);
> +		else if (drm_vblank_offdelay > 0)
>  			mod_timer(&vblank->disable_timer,
>  				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
> -		else if (drm_vblank_offdelay == 0)
> -			vblank_disable_fn((unsigned long)vblank);
>  	}
>  }
>  EXPORT_SYMBOL(drm_vblank_put);

Would it be better if we just squashed this device flag logic back into
patch 7, but kept the drm_vblank_offdelay == 0 meaning of "never
disable?"  I can see there being people who might already use this when
debugging new and potentially flaky hardware platforms who would be
surprised by the change in behavior.  So basically something like:

        if (drm_vblank_offdelay == 0)
                return
        else if (dev->vblank_disable_immediate)
                vblank_disable_fn((unsigned long)vblank);
        else
                mod_timer(...);


I'd also suggest adding a comment in drm_stub.c to indicate that
drm_vblank_offdelay gets ignored for drivers that set
vblank_disable_immediate.

Other than that, patches 1-8, 10, and 11 are
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

I'll finish up reviewing #9 and 12-14 tomorrow when I have some more
time.



Matt


> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 979a498..0944544 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1117,6 +1117,16 @@ struct drm_device {
>  	 */
>  	bool vblank_disable_allowed;
>  
> +	/*
> +	 * If true, vblank interrupt will be disabled immediately when the
> +	 * refcount drops to zero, as opposed to via the vblank disable
> +	 * timer.
> +	 * This can be set to true it the hardware has a working vblank
> +	 * counter and the driver uses drm_vblank_on() and drm_vblank_off()
> +	 * appropriately.
> +	 */
> +	bool vblank_disable_immediate;
> +
>  	/* array of size num_crtcs */
>  	struct drm_vblank_crtc *vblank;
>  
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795