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@xxxxxxxxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> ---
> 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()?
> + 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?
> +#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.
> +
> + 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?
> 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()
>
> - 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.
> 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?
> {
> + 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 (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
> 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) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
