Re: [PATCH 4/9] drm/i915: Force PSR exit by inactivating it.

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jun 12, 2014 at 10:16:41AM -0700, Rodrigo Vivi wrote:
> The perfect solution for psr_exit is the hardware tracking the changes and
> doing the psr exit by itself. This scenario works for HSW and BDW with some
> environments like Gnome and Wayland.
> 
> However there are many other scenarios that this isn't true. Mainly one right
> now is KDE users on HSW and BDW with PSR on. User would miss many screen
> updates. For instances any key typed could be seen only when mouse cursor is
> moved. So this patch introduces the ability of trigger PSR exit on kernel side
> on some common cases that.
> 
> Most of the cases are coverred by psr_exit at set_domain. The remaining cases
> are coverred by triggering it at set_domain, busy_ioctl, sw_finish and
> mark_busy.
> 
> The downside here might be reducing the residency time on the cases this
> already work very wall like Gnome environment. But so far let's get focused
> on fixinge issues sio PSR couild be used for everybody and we could even
> get it enabled by default. Later we can add some alternatives to choose the
> level of PSR efficiency over boot flag of even over crtc property.
> 
> Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  3 ++
>  drivers/gpu/drm/i915/i915_gem.c      |  6 +++
>  drivers/gpu/drm/i915/intel_display.c | 20 +++++++-
>  drivers/gpu/drm/i915/intel_dp.c      | 91 ++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++
>  5 files changed, 111 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f36d9eb..64d520f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -637,6 +637,9 @@ struct i915_psr {
>  	bool sink_support;
>  	bool source_ok;
>  	bool setup_done;
> +	bool enabled;
> +	bool active;
> +	struct delayed_work work;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1dc4b74..3637add 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1395,6 +1395,8 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
>  		goto unlock;
>  	}
>  
> +	intel_edp_psr_exit(dev, true);
> +
>  	/* Try to flush the object off the GPU without holding the lock.
>  	 * We will repeat the flush holding the lock in the normal manner
>  	 * to catch cases where we are gazumped.
> @@ -1440,6 +1442,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
>  	if (ret)
>  		return ret;
>  
> +	intel_edp_psr_exit(dev, true);
> +
>  	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
>  	if (&obj->base == NULL) {
>  		ret = -ENOENT;
> @@ -4234,6 +4238,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>  	if (ret)
>  		return ret;
>  
> +	intel_edp_psr_exit(dev, true);
> +
>  	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
>  	if (&obj->base == NULL) {
>  		ret = -ENOENT;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5cbb28..70dc433 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5017,6 +5017,8 @@ static void intel_connector_check_state(struct intel_connector *connector)
>   * consider. */
>  void intel_connector_dpms(struct drm_connector *connector, int mode)
>  {
> +	struct drm_device *dev = connector->dev;
> +	struct intel_encoder *intel_encoder;
>  	/* All the simple cases only support two dpms states. */
>  	if (mode != DRM_MODE_DPMS_ON)
>  		mode = DRM_MODE_DPMS_OFF;
> @@ -5027,9 +5029,15 @@ void intel_connector_dpms(struct drm_connector *connector, int mode)
>  	connector->dpms = mode;
>  
>  	/* Only need to change hw state when actually enabled */
> -	if (connector->encoder)
> -		intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
> +	if (connector->encoder) {
> +
> +		intel_encoder = to_intel_encoder(connector->encoder);
> +		intel_encoder_dpms(intel_encoder, mode);
>  
> +		if (intel_encoder->type == INTEL_OUTPUT_EDP &&
> +		    mode == DRM_MODE_DPMS_OFF)
> +			intel_edp_psr_exit(dev, false);

Nope. intel_connector_dpms is just the generic implementation we have for
forwarding the drm core ->dpms interface to our own stuff. Sprinkling
random things in here is not ok.

This either needs to be in the crtc_disable or in an encoder->disable
function.

Sorry I didn't spot this earlier.

First two patches merged.
-Daniel

> +	}
>  	intel_modeset_check_state(connector->dev);
>  }
>  
> @@ -8826,12 +8834,15 @@ out:
>  	intel_runtime_pm_put(dev_priv);
>  }
>  
> +
>  void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
>  			struct intel_engine_cs *ring)
>  {
>  	struct drm_device *dev = obj->base.dev;
>  	struct drm_crtc *crtc;
>  
> +	intel_edp_psr_exit(dev, true);
> +
>  	if (!i915.powersave)
>  		return;
>  
> @@ -9300,6 +9311,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	if (work == NULL)
>  		return -ENOMEM;
>  
> +	/* Exit PSR early in page flip */
> +	intel_edp_psr_exit(dev, true);
> +
>  	work->event = event;
>  	work->crtc = crtc;
>  	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
> @@ -11491,6 +11505,8 @@ static void intel_setup_outputs(struct drm_device *dev)
>  	if (SUPPORTS_TV(dev))
>  		intel_tv_init(dev);
>  
> +	intel_edp_psr_init(dev);
> +
>  	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
>  		encoder->base.possible_crtcs = encoder->crtc_mask;
>  		encoder->base.possible_clones =
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 68289ce..f7b2414 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1797,10 +1797,11 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
>  
>  static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
>  {
> -	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (!intel_edp_psr_match_conditions(intel_dp) ||
> -	    intel_edp_is_psr_enabled(dev))
> +	if (intel_edp_is_psr_enabled(dev))
>  		return;
>  
>  	/* Enable PSR on the panel */
> @@ -1808,6 +1809,9 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
>  
>  	/* Enable PSR on the host */
>  	intel_edp_psr_enable_source(intel_dp);
> +
> +	dev_priv->psr.enabled = true;
> +	dev_priv->psr.active = true;
>  }
>  
>  void intel_edp_psr_enable(struct intel_dp *intel_dp)
> @@ -1827,8 +1831,7 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
>  	/* Setup PSR once */
>  	intel_edp_psr_setup(intel_dp);
>  
> -	if (intel_edp_psr_match_conditions(intel_dp) &&
> -	    !intel_edp_is_psr_enabled(dev))
> +	if (intel_edp_psr_match_conditions(intel_dp))
>  		intel_edp_psr_do_enable(intel_dp);
>  }
>  
> @@ -1837,7 +1840,7 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (!intel_edp_is_psr_enabled(dev))
> +	if (!dev_priv->psr.enabled)
>  		return;
>  
>  	I915_WRITE(EDP_PSR_CTL(dev),
> @@ -1847,13 +1850,13 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
>  	if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) &
>  		       EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
>  		DRM_ERROR("Timed out waiting for PSR Idle State\n");
> +
> +	dev_priv->psr.enabled = false;
>  }
>  
>  void intel_edp_psr_update(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_encoder *encoder;
> -	struct intel_dp *intel_dp = NULL;
>  
>  	if (!HAS_PSR(dev))
>  		return;
> @@ -1861,6 +1864,17 @@ void intel_edp_psr_update(struct drm_device *dev)
>  	if (!dev_priv->psr.setup_done)
>  		return;
>  
> +	intel_edp_psr_exit(dev, true);
> +}
> +
> +void intel_edp_psr_work(struct work_struct *work)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(work, typeof(*dev_priv), psr.work.work);
> +	struct drm_device *dev = dev_priv->dev;
> +	struct intel_encoder *encoder;
> +	struct intel_dp *intel_dp = NULL;
> +
>  	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
>  		if (encoder->type == INTEL_OUTPUT_EDP) {
>  			intel_dp = enc_to_intel_dp(&encoder->base);
> @@ -1868,9 +1882,66 @@ void intel_edp_psr_update(struct drm_device *dev)
>  			if (!intel_edp_psr_match_conditions(intel_dp))
>  				intel_edp_psr_disable(intel_dp);
>  			else
> -				if (!intel_edp_is_psr_enabled(dev))
> -					intel_edp_psr_do_enable(intel_dp);
> +				intel_edp_psr_do_enable(intel_dp);
> +		}
> +}
> +
> +void intel_edp_psr_inactivate(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_connector *connector;
> +	struct intel_encoder *encoder;
> +	struct intel_crtc *intel_crtc;
> +	struct intel_dp *intel_dp = NULL;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list,
> +			    base.head) {
> +
> +		if (connector->base.dpms != DRM_MODE_DPMS_ON)
> +			continue;
> +
> +		encoder = to_intel_encoder(connector->base.encoder);
> +		if (encoder->type == INTEL_OUTPUT_EDP) {
> +
> +			intel_dp = enc_to_intel_dp(&encoder->base);
> +			intel_crtc = to_intel_crtc(encoder->base.crtc);
> +
> +			dev_priv->psr.active = false;
> +
> +			I915_WRITE(EDP_PSR_CTL(dev), I915_READ(EDP_PSR_CTL(dev))
> +				   & ~EDP_PSR_ENABLE);
>  		}
> +	}
> +}
> +
> +void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (!HAS_PSR(dev))
> +		return;
> +
> +	if (!dev_priv->psr.setup_done)
> +		return;
> +
> +	cancel_delayed_work_sync(&dev_priv->psr.work);
> +
> +	if (dev_priv->psr.active)
> +		intel_edp_psr_inactivate(dev);
> +
> +	if (schedule_back)
> +		schedule_delayed_work(&dev_priv->psr.work,
> +				      msecs_to_jiffies(100));
> +}
> +
> +void intel_edp_psr_init(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (!HAS_PSR(dev))
> +		return;
> +
> +	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_edp_psr_work);
>  }
>  
>  static void intel_disable_dp(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1904ef8..bd90661 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -831,6 +831,9 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp);
>  void intel_edp_psr_disable(struct intel_dp *intel_dp);
>  void intel_edp_psr_update(struct drm_device *dev);
>  void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
> +void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back);
> +void intel_edp_psr_init(struct drm_device *dev);
> +
>  
>  /* intel_dsi.c */
>  void intel_dsi_init(struct drm_device *dev);
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@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