Re: [PATCH 1/2] drm/i915: Allow control of PSR at runtime through debugfs, v5

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2018-08-08 at 16:19 +0200, Maarten Lankhorst wrote:
> Currently tests modify i915.enable_psr and then do a modeset cycle
> to change PSR. We can write a value to i915_edp_psr_debug to force
> a certain PSR mode without a modeset.
> 
> To retain compatibility with older userspace, we also still allow
> the override through the module parameter, and add some tracking
> to check whether a debugfs mode is specified.
> 
> Changes since v1:
> - Rename dev_priv->psr.enabled to .dp, and .hw_configured to
> .enabled.
> - Fix i915_psr_debugfs_mode to match the writes to debugfs.
> - Rename __i915_edp_psr_write to intel_psr_set_debugfs_mode, simplify
>   it and move it to intel_psr.c. This keeps all internals in
> intel_psr.c
> - Perform an interruptible wait for hw completion outside of the psr
>   lock, instead of being forced to trywait and return -EBUSY.
> Changes since v2:
> - Rebase on top of intel_psr changes.
> Changes since v3:
> - Assign psr.dp during init. (dhnkrn)
> - Add prepared bool, which should be used instead of relying on
> psr.dp. (dhnkrn)
> - Fix -EDEADLK handling in debugfs. (dhnkrn)
> - Clean up waiting for idle in intel_psr_set_debugfs_mode.
> - Print PSR mode when trying to enable PSR. (dhnkrn)
> - Move changing psr debug setting to i915_edp_psr_debug_set. (dhnkrn)
> Changes since v4:
> - Return error in _set() function.
> - Change flag values to make them easier to remember. (dhnkrn)
> - Only assign psr.dp once. (dhnkrn)
> - Only set crtc_state->has_psr on the crtc with psr.dp.
> - Fix typo. (dhnkrn)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  23 ++++-
>  drivers/gpu/drm/i915/i915_drv.h     |  12 ++-
>  drivers/gpu/drm/i915/i915_irq.c     |   2 +-
>  drivers/gpu/drm/i915/intel_drv.h    |   5 +-
>  drivers/gpu/drm/i915/intel_psr.c    | 138 +++++++++++++++++++++++---
> --
>  5 files changed, 149 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index f9ce35da4123..3e81301a94ba 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2708,7 +2708,7 @@ static int i915_edp_psr_status(struct seq_file
> *m, void *data)
>  	intel_runtime_pm_get(dev_priv);
>  
>  	mutex_lock(&dev_priv->psr.lock);
> -	seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv-
> >psr.enabled));
> +	seq_printf(m, "Enabled: %s\n", yesno(dev_priv-
> >psr.enabled));
>  	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
>  		   dev_priv->psr.busy_frontbuffer_bits);
>  
> @@ -2750,17 +2750,32 @@ static int
>  i915_edp_psr_debug_set(void *data, u64 val)
>  {
>  	struct drm_i915_private *dev_priv = data;
> +	struct drm_modeset_acquire_ctx ctx;
> +	int ret;
>  
>  	if (!CAN_PSR(dev_priv))
>  		return -ENODEV;
>  
> -	DRM_DEBUG_KMS("PSR debug %s\n", enableddisabled(val));
> +	DRM_DEBUG_KMS("Setting PSR debug to %llx\n", val);
>  
>  	intel_runtime_pm_get(dev_priv);
> -	intel_psr_irq_control(dev_priv, !!val);
> +
> +	drm_modeset_acquire_init(&ctx,
> DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> +
> +retry:
> +	ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val);
> +	if (ret == -EDEADLK) {
> +		ret = drm_modeset_backoff(&ctx);
> +		if (!ret)
> +			goto retry;
> +	}
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +
>  	intel_runtime_pm_put(dev_priv);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 657f46e0cae9..a3ea48ce1811 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -611,8 +611,17 @@ struct i915_drrs {
>  
>  struct i915_psr {
>  	struct mutex lock;
> +
> +#define I915_PSR_DEBUG_MODE_MASK	0x0f
> +#define I915_PSR_DEBUG_DEFAULT		0x00
> +#define I915_PSR_DEBUG_DISABLE		0x01
> +#define I915_PSR_DEBUG_ENABLE		0x02
> +#define I915_PSR_DEBUG_IRQ		0x10
> +
> +	u32 debug;
	u16?

>  	bool sink_support;
> -	struct intel_dp *enabled;
> +	bool prepared, enabled;
> +	struct intel_dp *dp;
>  	bool active;
>  	struct work_struct work;
>  	unsigned busy_frontbuffer_bits;
> @@ -622,7 +631,6 @@ struct i915_psr {
>  	bool alpm;
>  	bool psr2_enabled;
>  	u8 sink_sync_latency;
> -	bool debug;
>  	ktime_t last_entry_attempt;
>  	ktime_t last_exit;
>  };
> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> b/drivers/gpu/drm/i915/i915_irq.c
> index 8084e35b25c5..b2c9838442bc 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4048,7 +4048,7 @@ static int ironlake_irq_postinstall(struct
> drm_device *dev)
>  
>  	if (IS_HASWELL(dev_priv)) {
>  		gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
> -		intel_psr_irq_control(dev_priv, dev_priv-
> >psr.debug);
> +		intel_psr_irq_control(dev_priv, dev_priv->psr.debug
> & I915_PSR_DEBUG_IRQ);
>  		display_mask |= DE_EDP_PSR_INT_HSW;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 1ad7c1124bef..098ea2859a9b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1924,7 +1924,7 @@ int intel_hdcp_enable(struct intel_connector
> *connector);
>  int intel_hdcp_disable(struct intel_connector *connector);
>  int intel_hdcp_check_link(struct intel_connector *connector);
>  bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port
> port);
> -
> +//
^ seems like a ghost comment sneaked in.
>  
>  /* intel_psr.c */
>  #define CAN_PSR(dev_priv) (HAS_PSR(dev_priv) && dev_priv-
> >psr.sink_support)
>  void intel_psr_init_dpcd(struct intel_dp *intel_dp);
> @@ -1932,6 +1932,9 @@ void intel_psr_enable(struct intel_dp
> *intel_dp,
>  		      const struct intel_crtc_state *crtc_state);
>  void intel_psr_disable(struct intel_dp *intel_dp,
>  		      const struct intel_crtc_state
> *old_crtc_state);
> +int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
> +			       struct drm_modeset_acquire_ctx *ctx,
> +			       u64 value);
>  void intel_psr_invalidate(struct drm_i915_private *dev_priv,
>  			  unsigned frontbuffer_bits,
>  			  enum fb_op_origin origin);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 4bd5768731ee..12fbc59af5a4 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -56,6 +56,18 @@
>  #include "intel_drv.h"
>  #include "i915_drv.h"
>  
> +static bool psr_global_enabled(u32 debug)
> +{
> +	switch (debug & I915_PSR_DEBUG_MODE_MASK) {
> +	case I915_PSR_DEBUG_DEFAULT:
> +		return i915_modparams.enable_psr;
> +	case I915_PSR_DEBUG_DISABLE:
> +		return false;
> +	default:
> +		return true;
> +	}
> +}
> +
>  void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool
> debug)
>  {
>  	u32 debug_mask, mask;
> @@ -80,7 +92,6 @@ void intel_psr_irq_control(struct drm_i915_private
> *dev_priv, bool debug)
>  	if (debug)
>  		mask |= debug_mask;
>  
> -	WRITE_ONCE(dev_priv->psr.debug, debug);
>  	I915_WRITE(EDP_PSR_IMR, ~mask);
>  }
>  
> @@ -213,6 +224,9 @@ void intel_psr_init_dpcd(struct intel_dp
> *intel_dp)
>  	dev_priv->psr.sink_sync_latency =
>  		intel_dp_get_sink_sync_latency(intel_dp);
>  
> +	WARN_ON(dev_priv->psr.dp);
> +	dev_priv->psr.dp = intel_dp;
> +
>  	if (INTEL_GEN(dev_priv) >= 9 &&
>  	    (intel_dp->psr_dpcd[0] ==
> DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
>  		bool y_req = intel_dp->psr_dpcd[1] &
> @@ -471,8 +485,8 @@ void intel_psr_compute_config(struct intel_dp
> *intel_dp,
>  	if (!CAN_PSR(dev_priv))
>  		return;
>  
> -	if (!i915_modparams.enable_psr) {
> -		DRM_DEBUG_KMS("PSR disable by flag\n");
> +	if (intel_dp != dev_priv->psr.dp) {
> +		DRM_DEBUG_KMS("Not enabling PSR: wrong
> connector\n");
This debug message will get printed for non eDP ports, return silently?

>  		return;
>  	}
>  
> @@ -517,7 +531,6 @@ void intel_psr_compute_config(struct intel_dp
> *intel_dp,
>  
>  	crtc_state->has_psr = true;
>  	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp,
> crtc_state);
> -	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2"
> : "");
>  }
>  
>  static void intel_psr_activate(struct intel_dp *intel_dp)
> @@ -589,6 +602,24 @@ static void intel_psr_enable_source(struct
> intel_dp *intel_dp,
>  	}
>  }
>  
> +static void intel_psr_enable_locked(struct drm_i915_private
> *dev_priv,
> +				    const struct intel_crtc_state
> *crtc_state)
> +{
> +	struct intel_dp *intel_dp = dev_priv->psr.dp;
> +
> +	if (dev_priv->psr.enabled)
> +		return;
> +
> +	DRM_DEBUG_KMS("Enabling PSR%s\n",
> +		      dev_priv->psr.psr2_enabled ? "2" : "1");
> +	intel_psr_setup_vsc(intel_dp, crtc_state);
> +	intel_psr_enable_sink(intel_dp);
> +	intel_psr_enable_source(intel_dp, crtc_state);
> +	dev_priv->psr.enabled = true;
> +
> +	intel_psr_activate(intel_dp);
> +}
> +
>  /**
>   * intel_psr_enable - Enable PSR
>   * @intel_dp: Intel DP
> @@ -609,22 +640,20 @@ void intel_psr_enable(struct intel_dp
> *intel_dp,
>  	if (WARN_ON(!CAN_PSR(dev_priv)))
>  		return;
>  
> -	WARN_ON(dev_priv->drrs.dp);
This warning was to catch if DRRS was enabled along with PSR. the
hardware supports only one at a time.

>  	mutex_lock(&dev_priv->psr.lock);
> -	if (dev_priv->psr.enabled) {
> +	if (dev_priv->psr.prepared) {
>  		DRM_DEBUG_KMS("PSR already in use\n");
>  		goto unlock;
>  	}
>  
>  	dev_priv->psr.psr2_enabled = crtc_state->has_psr2;
>  	dev_priv->psr.busy_frontbuffer_bits = 0;
> +	dev_priv->psr.prepared = true;
>  
> -	intel_psr_setup_vsc(intel_dp, crtc_state);
> -	intel_psr_enable_sink(intel_dp);
> -	intel_psr_enable_source(intel_dp, crtc_state);
> -	dev_priv->psr.enabled = intel_dp;
> -
> -	intel_psr_activate(intel_dp);
> +	if (psr_global_enabled(dev_priv->psr.debug))
> +		intel_psr_enable_locked(dev_priv, crtc_state);
> +	else
> +		DRM_DEBUG_KMS("PSR disabled by flag\n");
>  
>  unlock:
>  	mutex_unlock(&dev_priv->psr.lock);
> @@ -683,12 +712,14 @@ static void intel_psr_disable_locked(struct
> intel_dp *intel_dp)
>  	if (!dev_priv->psr.enabled)
>  		return;
>  
> +	DRM_DEBUG_KMS("Disabling PSR%s\n",
> +		      dev_priv->psr.psr2_enabled ? "2" : "1");
>  	intel_psr_disable_source(intel_dp);
>  
>  	/* Disable PSR on Sink */
>  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
>  
> -	dev_priv->psr.enabled = NULL;
> +	dev_priv->psr.enabled = false;
>  }
>  
>  /**
> @@ -712,7 +743,14 @@ void intel_psr_disable(struct intel_dp
> *intel_dp,
>  		return;
>  
>  	mutex_lock(&dev_priv->psr.lock);
> +	if (!dev_priv->psr.prepared) {
> +		mutex_unlock(&dev_priv->psr.lock);
> +		return;
> +	}
> +
>  	intel_psr_disable_locked(intel_dp);
> +
> +	dev_priv->psr.prepared = false;
>  	mutex_unlock(&dev_priv->psr.lock);
>  	cancel_work_sync(&dev_priv->psr.work);
>  }
> @@ -724,9 +762,6 @@ int intel_psr_wait_for_idle(const struct
> intel_crtc_state *new_crtc_state)
>  	i915_reg_t reg;
>  	u32 mask;
>  
> -	if (!new_crtc_state->has_psr)
> -		return 0;
> -
We'll end up waiting for PSR to idle when updating CRTCs that are not
driving a PSR panel. I should have checked this in the previous
version, my bad.

The patch looks good to me other than for the issues pointed out.

-DK

>  	/*
>  	 * The sole user right now is intel_pipe_update_start(),
>  	 * which won't race with psr_enable/disable, which is
> @@ -737,6 +772,9 @@ int intel_psr_wait_for_idle(const struct
> intel_crtc_state *new_crtc_state)
>  	 * not needed and will induce latencies in the atomic
>  	 * update path.
>  	 */
> +	if (!dev_priv->psr.enabled)
> +		return 0;
> +
>  	if (dev_priv->psr.psr2_enabled) {
>  		reg = EDP_PSR2_STATUS;
>  		mask = EDP_PSR2_STATUS_STATE_MASK;



_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux