| Message ID | 20180731133559.6892-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: Allow control of PSR at runtime through debugfs, v4. | expand |
Op 31-07-18 om 15:35 schreef Maarten Lankhorst: > 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); > + if (!ret) > + goto retry; > + } > + > + drm_modeset_drop_locks(&ctx); > + drm_modeset_acquire_fini(&ctx); > + > intel_runtime_pm_put(dev_priv); > > return 0; ^Oops, needs to be changed to return ret; ~Maarten
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()? > + 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) {
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.
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); + if (!ret) + goto retry; + } + + drm_modeset_drop_locks(&ctx); + 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) +#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) + + 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); 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; - 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"); 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) { + 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); + + 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 +); 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) {
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(-)