[Intel-gfx] [RFC 4/6] drm/i915: DRRS calls based on frontbuffer

[Daniel Vetter]

On Fri, Sep 12, 2014 at 11:13:07PM +0530, Vandana Kannan wrote:
> Calls to switch between high and low refresh rates based on calls made to
> fb_invalidate and fb_flush.
> 
> Signed-off-by: Vandana Kannan <vandana.kannan at intel.com>

A few comments below.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |  5 ++++
>  drivers/gpu/drm/i915/intel_dp.c      | 51 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +++
>  3 files changed, 59 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6d6214a..3bf1732 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9146,6 +9146,11 @@ static void intel_mark_fb_busy(struct drm_device *dev,
>  		intel_increase_pllclock(dev, pipe);
>  		if (ring && intel_fbc_enabled(dev))
>  			ring->fbc_dirty = true;
> +
> +		if (ring)
> +			intel_edp_drrs_invalidate(dev, frontbuffer_bits);
> +		else
> +			intel_edp_drrs_flush(dev, frontbuffer_bits);

I think the integration here isn't quite right. I've written a small patch
to add a bit more documentation about how the frontbuffer tracking should
work. See

http://people.freedesktop.org/~danvet/drm/drmI915.html#idp72543008

So I think we need two cases here:
- Mark DRRS as busy with intel_edp_drrs_busy. You can place it into
  intel_mark_fb_busy since that will be called for all 3 events
  (invalidate, flush, flip) as needed. But the place right now is the
  wrong spot since it's in the pipe loop so means you're function gets
  called 3 times. And with the below item I think it's better to move this
  call out of fb_mark_busy anyway.
- Rearm the idle timer. This should _only_ be done for flush&flip, with a
  delay (100ms like in your patch sounds good) and _after_ you've cranked
  up the refresh rate to high. So checking for "ring" here isn't the right
  thing, we instead need to add an additional call at the end of
  intel_frontbuffer_flush. Or just add an "arm-idle-work" boolean to the
  _busy function and call it from intel_fb_obj_invalidate with false and
  intel_frontbuffer_flush with true.

>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index d50249e..bd0415b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4949,6 +4949,57 @@ unlock:
>  	mutex_unlock(&dev_priv->drrs.mutex);
>  }
>  
> +void intel_edp_drrs_invalidate(struct drm_device *dev,
> +			      unsigned frontbuffer_bits)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	enum pipe pipe;
> +
> +	mutex_lock(&dev_priv->drrs.mutex);
> +	if (!dev_priv->drrs.dp) {
> +		mutex_unlock(&dev_priv->drrs.mutex);
> +		return;
> +	}
> +
> +	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;

Dereferencing this pointer chain is a bit risk since this function gets
called without modeset locks held. It should work due to ordering, but I'd
prefer if we compute the DRRS pipe in drrs_enable and store it in
dev_prive->drrs.pipe.

> +	pipe = to_intel_crtc(crtc)->pipe;
> +
> +	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
> +		intel_dp_set_drrs_state(dev_priv->dev,
> +			dev_priv->drrs.dp->attached_connector->panel.
> +			fixed_mode->vrefresh);
> +
> +	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
> +
> +	dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits;
> +	mutex_unlock(&dev_priv->drrs.mutex);
> +}
> +
> +void intel_edp_drrs_flush(struct drm_device *dev,
> +			 unsigned frontbuffer_bits)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	enum pipe pipe;
> +
> +	mutex_lock(&dev_priv->drrs.mutex);
> +	if (!dev_priv->drrs.dp) {
> +		mutex_unlock(&dev_priv->drrs.mutex);
> +		return;
> +	}
> +
> +	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
> +	pipe = to_intel_crtc(crtc)->pipe;
> +	dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
> +
> +	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR &&
> +		!dev_priv->psr.busy_frontbuffer_bits)
> +			schedule_delayed_work(&dev_priv->drrs.work,
> +				      msecs_to_jiffies(100));
> +	mutex_unlock(&dev_priv->drrs.mutex);
> +}
> +
>  static struct drm_display_mode *
>  intel_dp_drrs_init(struct intel_connector *intel_connector,
>  		   struct drm_display_mode *fixed_mode)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a728cc8..380bfe4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -929,6 +929,9 @@ void intel_edp_psr_init(struct drm_device *dev);
>  
>  void intel_edp_drrs_enable(struct intel_dp *intel_dp);
>  void intel_edp_drrs_disable(struct intel_dp *intel_dp);
> +void intel_edp_drrs_invalidate(struct drm_device *dev,
> +				unsigned frontbuffer_bits);
> +void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
>  
>  int intel_dp_handle_hpd_irq(struct intel_digital_port *digport, bool long_hpd);
>  void intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connector);
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch