Re: [PATCH 05/11] drm/i915/psr: Export intel_psr_activate/exit()

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Mar 30, 2018 at 03:23:30PM -0700, José Roberto de Souza wrote:
> Export this functions so other modules can activate and exit PSR.

"module" is not a right word here.

But I also don't like the approach. In the past we had the use spread
on our driver and it was confusing and hard to deal with more corner
cases. frontbuffer tracking was created to fix that.

I understand where you are going because we need to disable it on aux
transactions, etc. But I don't believe the right way is to exporting
active/exit.

But to create one unified infrastructure to block and release psr
with atomic counters.

> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  2 +
>  drivers/gpu/drm/i915/intel_psr.c | 94 +++++++++++++++++++++++++++++++++++++---
>  2 files changed, 89 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d1452fd2a58d..70026b772721 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1891,6 +1891,8 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv,
>  				   unsigned frontbuffer_bits);
>  void intel_psr_compute_config(struct intel_dp *intel_dp,
>  			      struct intel_crtc_state *crtc_state);
> +void intel_psr_exit(struct intel_dp *intel_dp, bool wait_idle);
> +void intel_psr_activate(struct intel_dp *intel_dp, bool schedule);
>  
>  /* intel_runtime_pm.c */
>  int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index c4720b0152c3..906a12ea934d 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -56,6 +56,8 @@
>  #include "intel_drv.h"
>  #include "i915_drv.h"
>  
> +#define ACTIVATE_WORK_MSEC_DELAY 100
> +
>  static inline enum intel_display_power_domain
>  psr_aux_domain(struct intel_dp *intel_dp)
>  {
> @@ -548,7 +550,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : "");
>  }
>  
> -static void intel_psr_activate(struct intel_dp *intel_dp)
> +static void __intel_psr_activate(struct intel_dp *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;
> @@ -645,7 +647,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  	dev_priv->psr.enabled = intel_dp;
>  
>  	if (INTEL_GEN(dev_priv) >= 9) {
> -		intel_psr_activate(intel_dp);
> +		__intel_psr_activate(intel_dp);
>  	} else {
>  		/*
>  		 * FIXME: Activation should happen immediately since this
> @@ -794,7 +796,7 @@ static void intel_psr_work(struct work_struct *work)
>  	if (dev_priv->psr.busy_frontbuffer_bits)
>  		goto unlock;
>  
> -	intel_psr_activate(intel_dp);
> +	__intel_psr_activate(intel_dp);
>  unlock:
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
> @@ -880,7 +882,7 @@ static void vlv_psr_exit(struct intel_dp *intel_dp, bool disabling)
>  			   DP_SET_POWER_D0);
>  }
>  
> -static void intel_psr_exit(struct drm_i915_private *dev_priv)
> +static void __intel_psr_exit(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_dp *intel_dp = dev_priv->psr.enabled;
>  
> @@ -977,7 +979,7 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv,
>  	dev_priv->psr.busy_frontbuffer_bits |= frontbuffer_bits;
>  
>  	if (frontbuffer_bits)
> -		intel_psr_exit(dev_priv);
> +		__intel_psr_exit(dev_priv);
>  
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
> @@ -1023,7 +1025,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
>  	if (frontbuffer_bits) {
>  		if (dev_priv->psr.psr2_enabled ||
>  		    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> -			intel_psr_exit(dev_priv);
> +			__intel_psr_exit(dev_priv);
>  		} else {
>  			/*
>  			 * Display WA #0884: all
> @@ -1041,7 +1043,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
>  	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
>  		if (!work_busy(&dev_priv->psr.work.work))
>  			schedule_delayed_work(&dev_priv->psr.work,
> -					      msecs_to_jiffies(100));
> +					      msecs_to_jiffies(ACTIVATE_WORK_MSEC_DELAY));
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> @@ -1108,3 +1110,81 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
>  		dev_priv->psr.exit = hsw_psr_exit;
>  	}
>  }
> +
> +/**
> + * intel_psr_exit - Exit PSR
> + * @intel_dp: DisplayPort that should have PSR exited if active
> + * @wait_idle: if true wait until PSR is inactive otherwise just request it
> + *
> + * This function exit PSR if enabled and active in this DisplayPort.
> + */
> +void intel_psr_exit(struct intel_dp *intel_dp, bool wait_idle)
> +{
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +	if (!CAN_PSR(dev_priv))
> +		return;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +	if (dev_priv->psr.enabled != intel_dp)
> +		goto out;
> +	if (!dev_priv->psr.active && !wait_idle)
> +		goto out;
> +
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +		struct drm_crtc *crtc = dig_port->base.base.crtc;
> +		enum pipe pipe = to_intel_crtc(crtc)->pipe;
> +
> +		__intel_psr_exit(dev_priv);
> +
> +		if (!wait_idle)
> +			goto out;
> +
> +		if (intel_wait_for_register(dev_priv, VLV_PSRSTAT(pipe),
> +					    VLV_EDP_PSR_IN_TRANS, 0, 2000))
> +			DRM_ERROR("Timed out waiting for PSR Idle State\n");
> +	} else {
> +		/* HSW+ PSR disable is actually, exit + wait transition */
> +		dev_priv->psr.exit(intel_dp, wait_idle);
> +		dev_priv->psr.active = false;
> +	}
> +
> +out:
> +	mutex_unlock(&dev_priv->psr.lock);
> +}
> +
> +/**
> + * intel_psr_activate - Activate PSR
> + * @intel_dp: DisplayPort that should have PSR activated if enabled
> + * @schedule: if true schedule the active to happens in a few milliseconds
> + * otherwise activate right now.
> + *
> + * This function activate PSR if enabled in this DisplayPort.
> + */
> +void intel_psr_activate(struct intel_dp *intel_dp, bool schedule)
> +{
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +	if (!CAN_PSR(dev_priv))
> +		return;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +	if (dev_priv->psr.enabled != intel_dp || dev_priv->psr.active)
> +		goto out;
> +
> +	if (dev_priv->psr.busy_frontbuffer_bits)
> +		goto out;
> +
> +	if (schedule) {
> +		if (!work_busy(&dev_priv->psr.work.work))
> +			schedule_delayed_work(&dev_priv->psr.work,
> +					      msecs_to_jiffies(ACTIVATE_WORK_MSEC_DELAY));
> +	} else
> +		__intel_psr_activate(intel_dp);
> +out:
> +	mutex_unlock(&dev_priv->psr.lock);
> +}
> -- 
> 2.16.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx