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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 08-08-18 om 02:07 schreef Dhinakaran Pandiyan:
> On Tue, 2018-07-31 at 15:35 +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)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c |  21 ++++-
>>  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    | 136 +++++++++++++++++++++++---
>> --
>>  5 files changed, 144 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 59dc0610ea44..a7e927413e8e 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2705,7 +2705,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);
>>  
>> @@ -2747,14 +2747,29 @@ 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);
> Is there any reason not backoff where modeset_lock() is called, i.e.,
> inside set_debugfs_mode()?
Readability, same reason why runtime_pm_put is also kept in i915_edp_psr_debug_set.

The actual debugfs set function shouldn't have to worry about it, same as we do in atomic commits.
>> +		if (!ret)
>> +			goto retry;
>> +	}
>> +
>> +	drm_modeset_drop_locks(&ctx);
> Same with this?
>
>> +	drm_modeset_acquire_fini(&ctx);
>> +
>>  	intel_runtime_pm_put(dev_priv);
>>  
>>  	return 0;
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 0f49f9988dfa..ef04b6cd7863 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_IRQ	BIT(0)
> nit: looks a bit odd to use the BIT macro only for the first bit. Is
> this the new guideline for single bit definitions?
I tried to preserve the old behavior, so echo 1 > debug would still enable irq debugging.

I'm all for breaking this if it makes your life easier. :)
>
>> +#define I915_PSR_DEBUG_MODE_MASK (7 << 1)
>> +#define I915_PSR_DEBUG_DEFAULT	(0 << 1)
>> +#define I915_PSR_DEBUG_DISABLE	(1 << 1)
>> +#define I915_PSR_DEBUG_ENABLE	(2 << 1)
> Can we also please define the enable/disable bits from 0 and move the
> debug one towards MSB? The enable/disable bits will be the most
> commonly used ones and it'd be easy to remember if there was no shift?
> I'm fine if you don't want to change too.
Yeah that's fine.
>> +
>> +	u32 debug;
>>  	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 5dadefca2ad2..defd696e71fc 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -4051,7 +4051,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 99a5f5be5b82..f4cfb7ed2262 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1930,6 +1930,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..7848829094ca 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);
>>  }
>>  
>> @@ -212,6 +223,7 @@ void intel_psr_init_dpcd(struct intel_dp
>> *intel_dp)
>>  	dev_priv->psr.sink_support = true;
>>  	dev_priv->psr.sink_sync_latency =
>>  		intel_dp_get_sink_sync_latency(intel_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)) {
>> @@ -471,11 +483,6 @@ 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");
>> -		return;
>> -	}
>> -
>>  	/*
>>  	 * HSW spec explicitly says PSR is tied to port A.
>>  	 * BDW+ platforms with DDI implementation of PSR have
>> different
>> @@ -517,7 +524,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 +595,23 @@ 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
>> @@ -611,20 +634,20 @@ void intel_psr_enable(struct intel_dp
>> *intel_dp,
>>  
>>  	WARN_ON(dev_priv->drrs.dp);
> We could add a new line here?
Indeed, maybe change to if (WARN_ON(intel_dp != dev_priv->drrs.dp)) return; ?
>
>>  	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;
>> +	dev_priv->psr.dp = intel_dp;
> .dp is already set in init_dpcd()
Paranoia, in case we somehow ended up with 2 eDP's due to a bug. Maybe the change above?

>>  
>> -	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 +706,13 @@ static void intel_psr_disable_locked(struct
>> intel_dp *intel_dp)
>>  	if (!dev_priv->psr.enabled)
>>  		return;
>>  
>> +	DRM_DEBUG_KMS("Disabling PSR\n");
> Print PSR1/2 too? Like you've done in the enable counter part.
Thought about it, might as well do so.
>
>>  	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 +736,14 @@ void intel_psr_disable(struct intel_dp
>> *intel_dp,
>>  		return;
>>  
>>  	mutex_lock(&dev_priv->psr.lock);
>> +	if (intel_dp != dev_priv->psr.dp || !dev_priv->psr.prepared)
> Won't intel_dp == dev_priv->psr.dp if old_crtc_state->has_psr was true?
In case we have 2 eDP's somehow, again.
>
>>  {
>> +		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 +755,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;
>> -
>>  	/*
>>  	 * The sole user right now is intel_pipe_update_start(),
>>  	 * which won't race with psr_enable/disable, which is
>> @@ -737,6 +765,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;
>> @@ -756,13 +787,11 @@ int intel_psr_wait_for_idle(const struct
>> intel_crtc_state *new_crtc_state)
>>  
>>  static bool __psr_wait_for_idle_locked(struct drm_i915_private
>> *dev_priv)
>>  {
>> -	struct intel_dp *intel_dp;
>>  	i915_reg_t reg;
>>  	u32 mask;
>>  	int err;
>>  
>> -	intel_dp = dev_priv->psr.enabled;
>> -	if (!intel_dp)
>> +	if (!dev_priv->psr.enabled)
>>  		return false;
>>  
>>  	if (dev_priv->psr.psr2_enabled) {
>> @@ -784,6 +813,62 @@ static bool __psr_wait_for_idle_locked(struct
>> drm_i915_private *dev_priv)
>>  	return err == 0 && dev_priv->psr.enabled;
>>  }
>>  
>> +int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
>> +			       struct drm_modeset_acquire_ctx *ctx,
>> +			       u64 val)
>> +{
>> +	struct drm_device *dev = &dev_priv->drm;
>> +	struct drm_connector_state *conn_state;
>> +	struct drm_crtc *crtc;
>> +	struct intel_dp *dp;
>> +	int ret;
>> +	bool enable;
>> +
>> +	if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK)
>> ||
>> +	    (val & I915_PSR_DEBUG_MODE_MASK) >
>> I915_PSR_DEBUG_ENABLE) {
>> +		DRM_DEBUG_KMS("Invalid debug mask %llx\n", val);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
>> ctx);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* dev_priv->psr.dp should be set once and then never
>> touched again. */
>> +	dp = READ_ONCE(dev_priv->psr.dp);
>> +	conn_state = dp->attached_connector->base.state;
>> +	crtc = conn_state->crtc;
>> +	if (crtc) {
>> +		ret = drm_modeset_lock(&crtc->mutex, ctx);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = wait_for_completion_interruptible(&crtc-
>>> state->commit->hw_done);
>> +	} else
>> +		ret = wait_for_completion_interruptible(&conn_state-
>>> commit->hw_done);
> Shouldn't this wait on connection state be done even if a crtc was
> present?
If the connector is bound to a crtc, then having that idle will mean connector is idle too.

See drm_atomic_helper_setup_commit()

Other way around is not always true.

>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = mutex_lock_interruptible(&dev_priv->psr.lock);
>> +	if (ret)
>> +		return ret;
>> +
>> +	enable = psr_global_enabled(val);
>> +
>> +	if (!enable)
>> +		intel_psr_disable_locked(dev_priv->psr.dp);
>> +
>> +	dev_priv->psr.debug = val;
>> +	intel_psr_irq_control(dev_priv, dev_priv->psr.debug &
>> I915_PSR_DEBUG_IRQ);
>> +
>> +	if (dev_priv->psr.prepared && enable)
>> +		intel_psr_enable_locked(dev_priv,
>> to_intel_crtc_state(crtc->state));
>> +
>> +	mutex_unlock(&dev_priv->psr.lock);
>> +	return ret;
>> +}
>> +
>>  static void intel_psr_work(struct work_struct *work)
>>  {
>>  	struct drm_i915_private *dev_priv =
>> @@ -811,7 +896,8 @@ static void intel_psr_work(struct work_struct
>> *work)
>>  	if (dev_priv->psr.busy_frontbuffer_bits || dev_priv-
>>> psr.active)
>>  		goto unlock;
>>  
>> -	intel_psr_activate(dev_priv->psr.enabled);
>> +	intel_psr_activate(dev_priv->psr.dp
>> +);
> ^typo
Indeed! Will fix.
>>  unlock:
>>  	mutex_unlock(&dev_priv->psr.lock);
>>  }
>> @@ -866,7 +952,7 @@ void intel_psr_invalidate(struct drm_i915_private
>> *dev_priv,
>>  		return;
>>  	}
>>  
>> -	crtc = dp_to_dig_port(dev_priv->psr.enabled)-
>>> base.base.crtc;
>> +	crtc = dp_to_dig_port(dev_priv->psr.dp)->base.base.crtc;
>>  	pipe = to_intel_crtc(crtc)->pipe;
>>  
>>  	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
>> @@ -909,7 +995,7 @@ void intel_psr_flush(struct drm_i915_private
>> *dev_priv,
>>  		return;
>>  	}
>>  
>> -	crtc = dp_to_dig_port(dev_priv->psr.enabled)-
>>> base.base.crtc;
>> +	crtc = dp_to_dig_port(dev_priv->psr.dp)->base.base.crtc;
>>  	pipe = to_intel_crtc(crtc)->pipe;
>>  
>>  	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
>> @@ -991,7 +1077,7 @@ void intel_psr_short_pulse(struct intel_dp
>> *intel_dp)
>>  
>>  	mutex_lock(&psr->lock);
>>  
>> -	if (psr->enabled != intel_dp)
>> +	if (!psr->enabled || psr->dp != intel_dp)
>>  		goto exit;
>>  
>>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val)
>> != 1) {

Thanks for review, will send a new version today.

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