| Message ID | 20190103142107.18304-3-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/6] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks | expand |
Op 03-01-2019 om 15:21 schreef José Roberto de Souza: > intel_psr_set_debugfs_mode() don't just handle the PSR mode but it is > also handling input validation, setting the new debug value and > changing PSR IRQ masks. > Lets move the roles listed above to the caller to make the function > name and what it does accurate. > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 22 ++++++++++++++++++++-- > drivers/gpu/drm/i915/intel_drv.h | 2 +- > drivers/gpu/drm/i915/intel_psr.c | 26 ++++++++++---------------- > 3 files changed, 31 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 1a31921598e7..77b097b50fd5 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2639,19 +2639,29 @@ i915_edp_psr_debug_set(void *data, u64 val) > { > struct drm_i915_private *dev_priv = data; > struct drm_modeset_acquire_ctx ctx; > - int ret; > + const u32 mode = val & I915_PSR_DEBUG_MODE_MASK; > + int ret = 0; > > if (!CAN_PSR(dev_priv)) > return -ENODEV; > > DRM_DEBUG_KMS("Setting PSR debug to %llx\n", val); > > + if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) || > + mode > I915_PSR_DEBUG_FORCE_PSR1) { > + DRM_DEBUG_KMS("Invalid debug mask %llx\n", val); > + return -EINVAL; > + } This would only work for (psr.debug & MASK) == (val & MASK). So you need to take the lock before you can be sure. While at it, you probably also need the intel_runtime_pm_get() reference.. so you really don't complicate locking much. I would honestly just grab the extra locks unnecessarily for simplicity. It's only used from debugfs after all. > + > + if (!mode) > + goto skip_mode; > + > intel_runtime_pm_get(dev_priv); > > drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE); > > retry: > - ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val); > + ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, mode); > if (ret == -EDEADLK) { > ret = drm_modeset_backoff(&ctx); > if (!ret) > @@ -2663,6 +2673,14 @@ i915_edp_psr_debug_set(void *data, u64 val) > > intel_runtime_pm_put(dev_priv); > > +skip_mode: > + if (!ret) { > + mutex_lock(&dev_priv->psr.lock); > + dev_priv->psr.debug = val; > + intel_psr_irq_control(dev_priv, dev_priv->psr.debug); > + mutex_unlock(&dev_priv->psr.lock); > + } > + > return ret; > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 1a11c2beb7f3..2367f07ba29e 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -2063,7 +2063,7 @@ 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); > + u32 mode); > 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 0ef6c5f8c298..bba4f7da68b3 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -69,13 +69,14 @@ static bool psr_global_enabled(u32 debug) > } > > static bool intel_psr2_enabled(struct drm_i915_private *dev_priv, > - const struct intel_crtc_state *crtc_state) > + const struct intel_crtc_state *crtc_state, > + u32 debug) > { > /* Cannot enable DSC and PSR2 simultaneously */ > WARN_ON(crtc_state->dsc_params.compression_enable && > crtc_state->has_psr2); > > - switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) { > + switch (debug & I915_PSR_DEBUG_MODE_MASK) { > case I915_PSR_DEBUG_DISABLE: > case I915_PSR_DEBUG_FORCE_PSR1: > return false; > @@ -758,7 +759,8 @@ void intel_psr_enable(struct intel_dp *intel_dp, > goto unlock; > } > > - dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state); > + dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state, > + dev_priv->psr.debug); > dev_priv->psr.busy_frontbuffer_bits = 0; > dev_priv->psr.prepared = true; > dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)->pipe; > @@ -944,7 +946,7 @@ static bool switching_psr(struct drm_i915_private *dev_priv, > > int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv, > struct drm_modeset_acquire_ctx *ctx, > - u64 val) > + u32 mode) > { > struct drm_device *dev = &dev_priv->drm; > struct drm_connector_state *conn_state; > @@ -954,13 +956,6 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv, > struct intel_dp *dp; > int ret; > bool enable; > - u32 mode = val & I915_PSR_DEBUG_MODE_MASK; > - > - if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) || > - mode > I915_PSR_DEBUG_FORCE_PSR1) { > - DRM_DEBUG_KMS("Invalid debug mask %llx\n", val); > - return -EINVAL; > - } > > ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx); > if (ret) > @@ -990,16 +985,15 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv, > if (ret) > return ret; > > - enable = psr_global_enabled(val); > + enable = psr_global_enabled(mode); > > if (!enable || switching_psr(dev_priv, crtc_state, mode)) > intel_psr_disable_locked(dev_priv->psr.dp); > > - dev_priv->psr.debug = val; > if (crtc) > - dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state); > - > - intel_psr_irq_control(dev_priv, dev_priv->psr.debug); > + dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, > + crtc_state, > + mode); > > if (dev_priv->psr.prepared && enable) > intel_psr_enable_locked(dev_priv, crtc_state);
On Fri, 2019-01-04 at 07:53 +0100, Maarten Lankhorst wrote: > Op 03-01-2019 om 15:21 schreef José Roberto de Souza: > > intel_psr_set_debugfs_mode() don't just handle the PSR mode but it > > is > > also handling input validation, setting the new debug value and > > changing PSR IRQ masks. > > Lets move the roles listed above to the caller to make the function > > name and what it does accurate. > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 22 ++++++++++++++++++++-- > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > drivers/gpu/drm/i915/intel_psr.c | 26 ++++++++++--------------- > > - > > 3 files changed, 31 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c > > index 1a31921598e7..77b097b50fd5 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -2639,19 +2639,29 @@ i915_edp_psr_debug_set(void *data, u64 val) > > { > > struct drm_i915_private *dev_priv = data; > > struct drm_modeset_acquire_ctx ctx; > > - int ret; > > + const u32 mode = val & I915_PSR_DEBUG_MODE_MASK; > > + int ret = 0; > > > > if (!CAN_PSR(dev_priv)) > > return -ENODEV; > > > > DRM_DEBUG_KMS("Setting PSR debug to %llx\n", val); > > > > + if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) || > > + mode > I915_PSR_DEBUG_FORCE_PSR1) { > > + DRM_DEBUG_KMS("Invalid debug mask %llx\n", val); > > + return -EINVAL; > > + } > > This would only work for (psr.debug & MASK) == (val & MASK). > > So you need to take the lock before you can be sure. > > While at it, you probably also need the intel_runtime_pm_get() > reference.. so you really don't complicate locking much. > > I would honestly just grab the extra locks unnecessarily for > simplicity. It's only used from debugfs after all. Thanks for the catch. Something like this? diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 938ad2107ead..3a6ccf815ee1 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2656,11 +2656,13 @@ i915_edp_psr_debug_set(void *data, u64 val) return -EINVAL; } - if (!mode) - goto skip_mode; - intel_runtime_pm_get(dev_priv); + mutex_lock(&dev_priv->psr.lock); + if (mode == (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK)) + goto skip_mode; + mutex_unlock(&dev_priv->psr.lock); + drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE); retry: @@ -2674,8 +2676,6 @@ i915_edp_psr_debug_set(void *data, u64 val) drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); - intel_runtime_pm_put(dev_priv); - skip_mode: if (!ret) { mutex_lock(&dev_priv->psr.lock); @@ -2684,6 +2684,8 @@ i915_edp_psr_debug_set(void *data, u64 val) mutex_unlock(&dev_priv->psr.lock); } + intel_runtime_pm_put(dev_priv); + return ret; } > > > + > > + if (!mode) > > + goto skip_mode; > > + > > intel_runtime_pm_get(dev_priv); > > > > drm_modeset_acquire_init(&ctx, > > DRM_MODESET_ACQUIRE_INTERRUPTIBLE); > > > > retry: > > - ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val); > > + ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, mode); > > if (ret == -EDEADLK) { > > ret = drm_modeset_backoff(&ctx); > > if (!ret) > > @@ -2663,6 +2673,14 @@ i915_edp_psr_debug_set(void *data, u64 val) > > > > intel_runtime_pm_put(dev_priv); > > > > +skip_mode: > > + if (!ret) { > > + mutex_lock(&dev_priv->psr.lock); > > + dev_priv->psr.debug = val; > > + intel_psr_irq_control(dev_priv, dev_priv->psr.debug); > > + mutex_unlock(&dev_priv->psr.lock); > > + } > > + > > return ret; > > } > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 1a11c2beb7f3..2367f07ba29e 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -2063,7 +2063,7 @@ 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); > > + u32 mode); > > 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 0ef6c5f8c298..bba4f7da68b3 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -69,13 +69,14 @@ static bool psr_global_enabled(u32 debug) > > } > > > > static bool intel_psr2_enabled(struct drm_i915_private *dev_priv, > > - const struct intel_crtc_state > > *crtc_state) > > + const struct intel_crtc_state > > *crtc_state, > > + u32 debug) > > { > > /* Cannot enable DSC and PSR2 simultaneously */ > > WARN_ON(crtc_state->dsc_params.compression_enable && > > crtc_state->has_psr2); > > > > - switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) { > > + switch (debug & I915_PSR_DEBUG_MODE_MASK) { > > case I915_PSR_DEBUG_DISABLE: > > case I915_PSR_DEBUG_FORCE_PSR1: > > return false; > > @@ -758,7 +759,8 @@ void intel_psr_enable(struct intel_dp > > *intel_dp, > > goto unlock; > > } > > > > - dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, > > crtc_state); > > + dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, > > crtc_state, > > + dev_priv- > > >psr.debug); > > dev_priv->psr.busy_frontbuffer_bits = 0; > > dev_priv->psr.prepared = true; > > dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)- > > >pipe; > > @@ -944,7 +946,7 @@ static bool switching_psr(struct > > drm_i915_private *dev_priv, > > > > int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv, > > struct drm_modeset_acquire_ctx *ctx, > > - u64 val) > > + u32 mode) > > { > > struct drm_device *dev = &dev_priv->drm; > > struct drm_connector_state *conn_state; > > @@ -954,13 +956,6 @@ int intel_psr_set_debugfs_mode(struct > > drm_i915_private *dev_priv, > > struct intel_dp *dp; > > int ret; > > bool enable; > > - u32 mode = val & I915_PSR_DEBUG_MODE_MASK; > > - > > - if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) || > > - mode > I915_PSR_DEBUG_FORCE_PSR1) { > > - DRM_DEBUG_KMS("Invalid debug mask %llx\n", val); > > - return -EINVAL; > > - } > > > > ret = drm_modeset_lock(&dev->mode_config.connection_mutex, > > ctx); > > if (ret) > > @@ -990,16 +985,15 @@ int intel_psr_set_debugfs_mode(struct > > drm_i915_private *dev_priv, > > if (ret) > > return ret; > > > > - enable = psr_global_enabled(val); > > + enable = psr_global_enabled(mode); > > > > if (!enable || switching_psr(dev_priv, crtc_state, mode)) > > intel_psr_disable_locked(dev_priv->psr.dp); > > > > - dev_priv->psr.debug = val; > > if (crtc) > > - dev_priv->psr.psr2_enabled = > > intel_psr2_enabled(dev_priv, crtc_state); > > - > > - intel_psr_irq_control(dev_priv, dev_priv->psr.debug); > > + dev_priv->psr.psr2_enabled = > > intel_psr2_enabled(dev_priv, > > + crtc_st > > ate, > > + mode); > > > > if (dev_priv->psr.prepared && enable) > > intel_psr_enable_locked(dev_priv, crtc_state); > >
Op 04-01-2019 om 14:28 schreef Souza, Jose: > On Fri, 2019-01-04 at 07:53 +0100, Maarten Lankhorst wrote: >> Op 03-01-2019 om 15:21 schreef José Roberto de Souza: >>> intel_psr_set_debugfs_mode() don't just handle the PSR mode but it >>> is >>> also handling input validation, setting the new debug value and >>> changing PSR IRQ masks. >>> Lets move the roles listed above to the caller to make the function >>> name and what it does accurate. >>> >>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_debugfs.c | 22 ++++++++++++++++++++-- >>> drivers/gpu/drm/i915/intel_drv.h | 2 +- >>> drivers/gpu/drm/i915/intel_psr.c | 26 ++++++++++--------------- >>> - >>> 3 files changed, 31 insertions(+), 19 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >>> b/drivers/gpu/drm/i915/i915_debugfs.c >>> index 1a31921598e7..77b097b50fd5 100644 >>> --- a/drivers/gpu/drm/i915/i915_debugfs.c >>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >>> @@ -2639,19 +2639,29 @@ i915_edp_psr_debug_set(void *data, u64 val) >>> { >>> struct drm_i915_private *dev_priv = data; >>> struct drm_modeset_acquire_ctx ctx; >>> - int ret; >>> + const u32 mode = val & I915_PSR_DEBUG_MODE_MASK; >>> + int ret = 0; >>> >>> if (!CAN_PSR(dev_priv)) >>> return -ENODEV; >>> >>> DRM_DEBUG_KMS("Setting PSR debug to %llx\n", val); >>> >>> + if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) || >>> + mode > I915_PSR_DEBUG_FORCE_PSR1) { >>> + DRM_DEBUG_KMS("Invalid debug mask %llx\n", val); >>> + return -EINVAL; >>> + } >> This would only work for (psr.debug & MASK) == (val & MASK). >> >> So you need to take the lock before you can be sure. >> >> While at it, you probably also need the intel_runtime_pm_get() >> reference.. so you really don't complicate locking much. >> >> I would honestly just grab the extra locks unnecessarily for >> simplicity. It's only used from debugfs after all. > Thanks for the catch. > > Something like this? > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 938ad2107ead..3a6ccf815ee1 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2656,11 +2656,13 @@ i915_edp_psr_debug_set(void *data, u64 val) > return -EINVAL; > } > > - if (!mode) > - goto skip_mode; > - > intel_runtime_pm_get(dev_priv); > > + mutex_lock(&dev_priv->psr.lock); > + if (mode == (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK)) > + goto skip_mode; > + mutex_unlock(&dev_priv->psr.lock); > + > drm_modeset_acquire_init(&ctx, > DRM_MODESET_ACQUIRE_INTERRUPTIBLE); > > retry: > @@ -2674,8 +2676,6 @@ i915_edp_psr_debug_set(void *data, u64 val) > drm_modeset_drop_locks(&ctx); > drm_modeset_acquire_fini(&ctx); > > - intel_runtime_pm_put(dev_priv); > - > skip_mode: > if (!ret) { > mutex_lock(&dev_priv->psr.lock); > @@ -2684,6 +2684,8 @@ i915_edp_psr_debug_set(void *data, u64 val) > mutex_unlock(&dev_priv->psr.lock); > } > > + intel_runtime_pm_put(dev_priv); > + > return ret; > } > > > > >>> + >>> + if (!mode) >>> + goto skip_mode; >>> + >>> intel_runtime_pm_get(dev_priv); >>> >>> drm_modeset_acquire_init(&ctx, >>> DRM_MODESET_ACQUIRE_INTERRUPTIBLE); >>> >>> retry: >>> - ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val); >>> + ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, mode); >>> if (ret == -EDEADLK) { >>> ret = drm_modeset_backoff(&ctx); >>> if (!ret) >>> @@ -2663,6 +2673,14 @@ i915_edp_psr_debug_set(void *data, u64 val) >>> >>> intel_runtime_pm_put(dev_priv); >>> >>> +skip_mode: >>> + if (!ret) { >>> + mutex_lock(&dev_priv->psr.lock); >>> + dev_priv->psr.debug = val; >>> + intel_psr_irq_control(dev_priv, dev_priv->psr.debug); >>> + mutex_unlock(&dev_priv->psr.lock); >>> + } >>> + >>> return ret; >>> } >>> >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h >>> b/drivers/gpu/drm/i915/intel_drv.h >>> index 1a11c2beb7f3..2367f07ba29e 100644 >>> --- a/drivers/gpu/drm/i915/intel_drv.h >>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>> @@ -2063,7 +2063,7 @@ 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); >>> + u32 mode); >>> 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 0ef6c5f8c298..bba4f7da68b3 100644 >>> --- a/drivers/gpu/drm/i915/intel_psr.c >>> +++ b/drivers/gpu/drm/i915/intel_psr.c >>> @@ -69,13 +69,14 @@ static bool psr_global_enabled(u32 debug) >>> } >>> >>> static bool intel_psr2_enabled(struct drm_i915_private *dev_priv, >>> - const struct intel_crtc_state >>> *crtc_state) >>> + const struct intel_crtc_state >>> *crtc_state, >>> + u32 debug) >>> { >>> /* Cannot enable DSC and PSR2 simultaneously */ >>> WARN_ON(crtc_state->dsc_params.compression_enable && >>> crtc_state->has_psr2); >>> >>> - switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) { >>> + switch (debug & I915_PSR_DEBUG_MODE_MASK) { >>> case I915_PSR_DEBUG_DISABLE: >>> case I915_PSR_DEBUG_FORCE_PSR1: >>> return false; >>> @@ -758,7 +759,8 @@ void intel_psr_enable(struct intel_dp >>> *intel_dp, >>> goto unlock; >>> } >>> >>> - dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, >>> crtc_state); >>> + dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, >>> crtc_state, >>> + dev_priv- >>>> psr.debug); >>> dev_priv->psr.busy_frontbuffer_bits = 0; >>> dev_priv->psr.prepared = true; >>> dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)- >>>> pipe; >>> @@ -944,7 +946,7 @@ static bool switching_psr(struct >>> drm_i915_private *dev_priv, >>> >>> int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv, >>> struct drm_modeset_acquire_ctx *ctx, >>> - u64 val) >>> + u32 mode) >>> { >>> struct drm_device *dev = &dev_priv->drm; >>> struct drm_connector_state *conn_state; >>> @@ -954,13 +956,6 @@ int intel_psr_set_debugfs_mode(struct >>> drm_i915_private *dev_priv, >>> struct intel_dp *dp; >>> int ret; >>> bool enable; >>> - u32 mode = val & I915_PSR_DEBUG_MODE_MASK; >>> - >>> - if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) || >>> - mode > I915_PSR_DEBUG_FORCE_PSR1) { >>> - DRM_DEBUG_KMS("Invalid debug mask %llx\n", val); >>> - return -EINVAL; >>> - } >>> >>> ret = drm_modeset_lock(&dev->mode_config.connection_mutex, >>> ctx); >>> if (ret) >>> @@ -990,16 +985,15 @@ int intel_psr_set_debugfs_mode(struct >>> drm_i915_private *dev_priv, >>> if (ret) >>> return ret; >>> >>> - enable = psr_global_enabled(val); >>> + enable = psr_global_enabled(mode); >>> >>> if (!enable || switching_psr(dev_priv, crtc_state, mode)) >>> intel_psr_disable_locked(dev_priv->psr.dp); >>> >>> - dev_priv->psr.debug = val; >>> if (crtc) >>> - dev_priv->psr.psr2_enabled = >>> intel_psr2_enabled(dev_priv, crtc_state); >>> - >>> - intel_psr_irq_control(dev_priv, dev_priv->psr.debug); >>> + dev_priv->psr.psr2_enabled = >>> intel_psr2_enabled(dev_priv, >>> + crtc_st >>> ate, >>> + mode); >>> >>> if (dev_priv->psr.prepared && enable) >>> intel_psr_enable_locked(dev_priv, crtc_state); >> Hm I would change the psr irq inside the lock, then jump to pm_put to finish and call that label out. Most race-free way to do so. :)
On Fri, 2019-01-04 at 15:35 +0100, Maarten Lankhorst wrote: > Op 04-01-2019 om 14:28 schreef Souza, Jose: > > On Fri, 2019-01-04 at 07:53 +0100, Maarten Lankhorst wrote: > > > Op 03-01-2019 om 15:21 schreef José Roberto de Souza: > > > > intel_psr_set_debugfs_mode() don't just handle the PSR mode but > > > > it > > > > is > > > > also handling input validation, setting the new debug value and > > > > changing PSR IRQ masks. > > > > Lets move the roles listed above to the caller to make the > > > > function > > > > name and what it does accurate. > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_debugfs.c | 22 ++++++++++++++++++++- > > > > - > > > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > > > drivers/gpu/drm/i915/intel_psr.c | 26 ++++++++++----------- > > > > ---- > > > > - > > > > 3 files changed, 31 insertions(+), 19 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > > > b/drivers/gpu/drm/i915/i915_debugfs.c > > > > index 1a31921598e7..77b097b50fd5 100644 > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > > @@ -2639,19 +2639,29 @@ i915_edp_psr_debug_set(void *data, u64 > > > > val) > > > > { > > > > struct drm_i915_private *dev_priv = data; > > > > struct drm_modeset_acquire_ctx ctx; > > > > - int ret; > > > > + const u32 mode = val & I915_PSR_DEBUG_MODE_MASK; > > > > + int ret = 0; > > > > > > > > if (!CAN_PSR(dev_priv)) > > > > return -ENODEV; > > > > > > > > DRM_DEBUG_KMS("Setting PSR debug to %llx\n", val); > > > > > > > > + if (val & ~(I915_PSR_DEBUG_IRQ | > > > > I915_PSR_DEBUG_MODE_MASK) || > > > > + mode > I915_PSR_DEBUG_FORCE_PSR1) { > > > > + DRM_DEBUG_KMS("Invalid debug mask %llx\n", > > > > val); > > > > + return -EINVAL; > > > > + } > > > This would only work for (psr.debug & MASK) == (val & MASK). > > > > > > So you need to take the lock before you can be sure. > > > > > > While at it, you probably also need the intel_runtime_pm_get() > > > reference.. so you really don't complicate locking much. > > > > > > I would honestly just grab the extra locks unnecessarily for > > > simplicity. It's only used from debugfs after all. > > Thanks for the catch. > > > > Something like this? > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c > > index 938ad2107ead..3a6ccf815ee1 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -2656,11 +2656,13 @@ i915_edp_psr_debug_set(void *data, u64 val) > > return -EINVAL; > > } > > > > - if (!mode) > > - goto skip_mode; > > - > > intel_runtime_pm_get(dev_priv); > > > > + mutex_lock(&dev_priv->psr.lock); > > + if (mode == (dev_priv->psr.debug & > > I915_PSR_DEBUG_MODE_MASK)) > > + goto skip_mode; > > + mutex_unlock(&dev_priv->psr.lock); > > + > > drm_modeset_acquire_init(&ctx, > > DRM_MODESET_ACQUIRE_INTERRUPTIBLE); > > > > retry: > > @@ -2674,8 +2676,6 @@ i915_edp_psr_debug_set(void *data, u64 val) > > drm_modeset_drop_locks(&ctx); > > drm_modeset_acquire_fini(&ctx); > > > > - intel_runtime_pm_put(dev_priv); > > - > > skip_mode: > > if (!ret) { > > mutex_lock(&dev_priv->psr.lock); > > @@ -2684,6 +2684,8 @@ i915_edp_psr_debug_set(void *data, u64 val) > > mutex_unlock(&dev_priv->psr.lock); > > } > > > > + intel_runtime_pm_put(dev_priv); > > + > > return ret; > > } > > > > > > > > > > > > + > > > > + if (!mode) > > > > + goto skip_mode; > > > > + > > > > intel_runtime_pm_get(dev_priv); > > > > > > > > drm_modeset_acquire_init(&ctx, > > > > DRM_MODESET_ACQUIRE_INTERRUPTIBLE); > > > > > > > > retry: > > > > - ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val); > > > > + ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, mode); > > > > if (ret == -EDEADLK) { > > > > ret = drm_modeset_backoff(&ctx); > > > > if (!ret) > > > > @@ -2663,6 +2673,14 @@ i915_edp_psr_debug_set(void *data, u64 > > > > val) > > > > > > > > intel_runtime_pm_put(dev_priv); > > > > > > > > +skip_mode: > > > > + if (!ret) { > > > > + mutex_lock(&dev_priv->psr.lock); > > > > + dev_priv->psr.debug = val; > > > > + intel_psr_irq_control(dev_priv, dev_priv- > > > > >psr.debug); > > > > + mutex_unlock(&dev_priv->psr.lock); > > > > + } > > > > + > > > > return ret; > > > > } > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > index 1a11c2beb7f3..2367f07ba29e 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -2063,7 +2063,7 @@ 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); > > > > + u32 mode); > > > > 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 0ef6c5f8c298..bba4f7da68b3 100644 > > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > > @@ -69,13 +69,14 @@ static bool psr_global_enabled(u32 debug) > > > > } > > > > > > > > static bool intel_psr2_enabled(struct drm_i915_private > > > > *dev_priv, > > > > - const struct intel_crtc_state > > > > *crtc_state) > > > > + const struct intel_crtc_state > > > > *crtc_state, > > > > + u32 debug) > > > > { > > > > /* Cannot enable DSC and PSR2 simultaneously */ > > > > WARN_ON(crtc_state->dsc_params.compression_enable && > > > > crtc_state->has_psr2); > > > > > > > > - switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) > > > > { > > > > + switch (debug & I915_PSR_DEBUG_MODE_MASK) { > > > > case I915_PSR_DEBUG_DISABLE: > > > > case I915_PSR_DEBUG_FORCE_PSR1: > > > > return false; > > > > @@ -758,7 +759,8 @@ void intel_psr_enable(struct intel_dp > > > > *intel_dp, > > > > goto unlock; > > > > } > > > > > > > > - dev_priv->psr.psr2_enabled = > > > > intel_psr2_enabled(dev_priv, > > > > crtc_state); > > > > + dev_priv->psr.psr2_enabled = > > > > intel_psr2_enabled(dev_priv, > > > > crtc_state, > > > > + dev_pri > > > > v- > > > > > psr.debug); > > > > dev_priv->psr.busy_frontbuffer_bits = 0; > > > > dev_priv->psr.prepared = true; > > > > dev_priv->psr.pipe = to_intel_crtc(crtc_state- > > > > >base.crtc)- > > > > > pipe; > > > > @@ -944,7 +946,7 @@ static bool switching_psr(struct > > > > drm_i915_private *dev_priv, > > > > > > > > int intel_psr_set_debugfs_mode(struct drm_i915_private > > > > *dev_priv, > > > > struct drm_modeset_acquire_ctx > > > > *ctx, > > > > - u64 val) > > > > + u32 mode) > > > > { > > > > struct drm_device *dev = &dev_priv->drm; > > > > struct drm_connector_state *conn_state; > > > > @@ -954,13 +956,6 @@ int intel_psr_set_debugfs_mode(struct > > > > drm_i915_private *dev_priv, > > > > struct intel_dp *dp; > > > > int ret; > > > > bool enable; > > > > - u32 mode = val & I915_PSR_DEBUG_MODE_MASK; > > > > - > > > > - if (val & ~(I915_PSR_DEBUG_IRQ | > > > > I915_PSR_DEBUG_MODE_MASK) || > > > > - mode > I915_PSR_DEBUG_FORCE_PSR1) { > > > > - DRM_DEBUG_KMS("Invalid debug mask %llx\n", > > > > val); > > > > - return -EINVAL; > > > > - } > > > > > > > > ret = drm_modeset_lock(&dev- > > > > >mode_config.connection_mutex, > > > > ctx); > > > > if (ret) > > > > @@ -990,16 +985,15 @@ int intel_psr_set_debugfs_mode(struct > > > > drm_i915_private *dev_priv, > > > > if (ret) > > > > return ret; > > > > > > > > - enable = psr_global_enabled(val); > > > > + enable = psr_global_enabled(mode); > > > > > > > > if (!enable || switching_psr(dev_priv, crtc_state, > > > > mode)) > > > > intel_psr_disable_locked(dev_priv->psr.dp); > > > > > > > > - dev_priv->psr.debug = val; > > > > if (crtc) > > > > - dev_priv->psr.psr2_enabled = > > > > intel_psr2_enabled(dev_priv, crtc_state); > > > > - > > > > - intel_psr_irq_control(dev_priv, dev_priv->psr.debug); > > > > + dev_priv->psr.psr2_enabled = > > > > intel_psr2_enabled(dev_priv, > > > > + > > > > crtc_st > > > > ate, > > > > + > > > > mode); > > > > > > > > if (dev_priv->psr.prepared && enable) > > > > intel_psr_enable_locked(dev_priv, crtc_state); > Hm I would change the psr irq inside the lock, then jump to pm_put to > finish and call that label out. Most race-free way to do so. :) > Well doing that will keep intel_psr_set_debugfs_mode() doing more stuff than it is supposed to but yeah it is taking the lock 3 times through i915_edp_psr_debug_set(), I'm not so concerned about race conditions because the use cases that we have would not cause it.
Op 04-01-2019 om 16:52 schreef Souza, Jose: > On Fri, 2019-01-04 at 15:35 +0100, Maarten Lankhorst wrote: >> Op 04-01-2019 om 14:28 schreef Souza, Jose: >>> On Fri, 2019-01-04 at 07:53 +0100, Maarten Lankhorst wrote: >>>> Op 03-01-2019 om 15:21 schreef José Roberto de Souza: >>>>> intel_psr_set_debugfs_mode() don't just handle the PSR mode but >>>>> it >>>>> is >>>>> also handling input validation, setting the new debug value and >>>>> changing PSR IRQ masks. >>>>> Lets move the roles listed above to the caller to make the >>>>> function >>>>> name and what it does accurate. >>>>> >>>>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/i915_debugfs.c | 22 ++++++++++++++++++++- >>>>> - >>>>> drivers/gpu/drm/i915/intel_drv.h | 2 +- >>>>> drivers/gpu/drm/i915/intel_psr.c | 26 ++++++++++----------- >>>>> ---- >>>>> - >>>>> 3 files changed, 31 insertions(+), 19 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >>>>> b/drivers/gpu/drm/i915/i915_debugfs.c >>>>> index 1a31921598e7..77b097b50fd5 100644 >>>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c >>>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >>>>> @@ -2639,19 +2639,29 @@ i915_edp_psr_debug_set(void *data, u64 >>>>> val) >>>>> { >>>>> struct drm_i915_private *dev_priv = data; >>>>> struct drm_modeset_acquire_ctx ctx; >>>>> - int ret; >>>>> + const u32 mode = val & I915_PSR_DEBUG_MODE_MASK; >>>>> + int ret = 0; >>>>> >>>>> if (!CAN_PSR(dev_priv)) >>>>> return -ENODEV; >>>>> >>>>> DRM_DEBUG_KMS("Setting PSR debug to %llx\n", val); >>>>> >>>>> + if (val & ~(I915_PSR_DEBUG_IRQ | >>>>> I915_PSR_DEBUG_MODE_MASK) || >>>>> + mode > I915_PSR_DEBUG_FORCE_PSR1) { >>>>> + DRM_DEBUG_KMS("Invalid debug mask %llx\n", >>>>> val); >>>>> + return -EINVAL; >>>>> + } >>>> This would only work for (psr.debug & MASK) == (val & MASK). >>>> >>>> So you need to take the lock before you can be sure. >>>> >>>> While at it, you probably also need the intel_runtime_pm_get() >>>> reference.. so you really don't complicate locking much. >>>> >>>> I would honestly just grab the extra locks unnecessarily for >>>> simplicity. It's only used from debugfs after all. >>> Thanks for the catch. >>> >>> Something like this? >>> >>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >>> b/drivers/gpu/drm/i915/i915_debugfs.c >>> index 938ad2107ead..3a6ccf815ee1 100644 >>> --- a/drivers/gpu/drm/i915/i915_debugfs.c >>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >>> @@ -2656,11 +2656,13 @@ i915_edp_psr_debug_set(void *data, u64 val) >>> return -EINVAL; >>> } >>> >>> - if (!mode) >>> - goto skip_mode; >>> - >>> intel_runtime_pm_get(dev_priv); >>> >>> + mutex_lock(&dev_priv->psr.lock); >>> + if (mode == (dev_priv->psr.debug & >>> I915_PSR_DEBUG_MODE_MASK)) >>> + goto skip_mode; >>> + mutex_unlock(&dev_priv->psr.lock); >>> + >>> drm_modeset_acquire_init(&ctx, >>> DRM_MODESET_ACQUIRE_INTERRUPTIBLE); >>> >>> retry: >>> @@ -2674,8 +2676,6 @@ i915_edp_psr_debug_set(void *data, u64 val) >>> drm_modeset_drop_locks(&ctx); >>> drm_modeset_acquire_fini(&ctx); >>> >>> - intel_runtime_pm_put(dev_priv); >>> - >>> skip_mode: >>> if (!ret) { >>> mutex_lock(&dev_priv->psr.lock); >>> @@ -2684,6 +2684,8 @@ i915_edp_psr_debug_set(void *data, u64 val) >>> mutex_unlock(&dev_priv->psr.lock); >>> } >>> >>> + intel_runtime_pm_put(dev_priv); >>> + >>> return ret; >>> } >>> >>> >>> >>> >>>>> + >>>>> + if (!mode) >>>>> + goto skip_mode; >>>>> + >>>>> intel_runtime_pm_get(dev_priv); >>>>> >>>>> drm_modeset_acquire_init(&ctx, >>>>> DRM_MODESET_ACQUIRE_INTERRUPTIBLE); >>>>> >>>>> retry: >>>>> - ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val); >>>>> + ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, mode); >>>>> if (ret == -EDEADLK) { >>>>> ret = drm_modeset_backoff(&ctx); >>>>> if (!ret) >>>>> @@ -2663,6 +2673,14 @@ i915_edp_psr_debug_set(void *data, u64 >>>>> val) >>>>> >>>>> intel_runtime_pm_put(dev_priv); >>>>> >>>>> +skip_mode: >>>>> + if (!ret) { >>>>> + mutex_lock(&dev_priv->psr.lock); >>>>> + dev_priv->psr.debug = val; >>>>> + intel_psr_irq_control(dev_priv, dev_priv- >>>>>> psr.debug); >>>>> + mutex_unlock(&dev_priv->psr.lock); >>>>> + } >>>>> + >>>>> return ret; >>>>> } >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h >>>>> b/drivers/gpu/drm/i915/intel_drv.h >>>>> index 1a11c2beb7f3..2367f07ba29e 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>>> @@ -2063,7 +2063,7 @@ 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); >>>>> + u32 mode); >>>>> 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 0ef6c5f8c298..bba4f7da68b3 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_psr.c >>>>> +++ b/drivers/gpu/drm/i915/intel_psr.c >>>>> @@ -69,13 +69,14 @@ static bool psr_global_enabled(u32 debug) >>>>> } >>>>> >>>>> static bool intel_psr2_enabled(struct drm_i915_private >>>>> *dev_priv, >>>>> - const struct intel_crtc_state >>>>> *crtc_state) >>>>> + const struct intel_crtc_state >>>>> *crtc_state, >>>>> + u32 debug) >>>>> { >>>>> /* Cannot enable DSC and PSR2 simultaneously */ >>>>> WARN_ON(crtc_state->dsc_params.compression_enable && >>>>> crtc_state->has_psr2); >>>>> >>>>> - switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) >>>>> { >>>>> + switch (debug & I915_PSR_DEBUG_MODE_MASK) { >>>>> case I915_PSR_DEBUG_DISABLE: >>>>> case I915_PSR_DEBUG_FORCE_PSR1: >>>>> return false; >>>>> @@ -758,7 +759,8 @@ void intel_psr_enable(struct intel_dp >>>>> *intel_dp, >>>>> goto unlock; >>>>> } >>>>> >>>>> - dev_priv->psr.psr2_enabled = >>>>> intel_psr2_enabled(dev_priv, >>>>> crtc_state); >>>>> + dev_priv->psr.psr2_enabled = >>>>> intel_psr2_enabled(dev_priv, >>>>> crtc_state, >>>>> + dev_pri >>>>> v- >>>>>> psr.debug); >>>>> dev_priv->psr.busy_frontbuffer_bits = 0; >>>>> dev_priv->psr.prepared = true; >>>>> dev_priv->psr.pipe = to_intel_crtc(crtc_state- >>>>>> base.crtc)- >>>>>> pipe; >>>>> @@ -944,7 +946,7 @@ static bool switching_psr(struct >>>>> drm_i915_private *dev_priv, >>>>> >>>>> int intel_psr_set_debugfs_mode(struct drm_i915_private >>>>> *dev_priv, >>>>> struct drm_modeset_acquire_ctx >>>>> *ctx, >>>>> - u64 val) >>>>> + u32 mode) >>>>> { >>>>> struct drm_device *dev = &dev_priv->drm; >>>>> struct drm_connector_state *conn_state; >>>>> @@ -954,13 +956,6 @@ int intel_psr_set_debugfs_mode(struct >>>>> drm_i915_private *dev_priv, >>>>> struct intel_dp *dp; >>>>> int ret; >>>>> bool enable; >>>>> - u32 mode = val & I915_PSR_DEBUG_MODE_MASK; >>>>> - >>>>> - if (val & ~(I915_PSR_DEBUG_IRQ | >>>>> I915_PSR_DEBUG_MODE_MASK) || >>>>> - mode > I915_PSR_DEBUG_FORCE_PSR1) { >>>>> - DRM_DEBUG_KMS("Invalid debug mask %llx\n", >>>>> val); >>>>> - return -EINVAL; >>>>> - } >>>>> >>>>> ret = drm_modeset_lock(&dev- >>>>>> mode_config.connection_mutex, >>>>> ctx); >>>>> if (ret) >>>>> @@ -990,16 +985,15 @@ int intel_psr_set_debugfs_mode(struct >>>>> drm_i915_private *dev_priv, >>>>> if (ret) >>>>> return ret; >>>>> >>>>> - enable = psr_global_enabled(val); >>>>> + enable = psr_global_enabled(mode); >>>>> >>>>> if (!enable || switching_psr(dev_priv, crtc_state, >>>>> mode)) >>>>> intel_psr_disable_locked(dev_priv->psr.dp); >>>>> >>>>> - dev_priv->psr.debug = val; >>>>> if (crtc) >>>>> - dev_priv->psr.psr2_enabled = >>>>> intel_psr2_enabled(dev_priv, crtc_state); >>>>> - >>>>> - intel_psr_irq_control(dev_priv, dev_priv->psr.debug); >>>>> + dev_priv->psr.psr2_enabled = >>>>> intel_psr2_enabled(dev_priv, >>>>> + >>>>> crtc_st >>>>> ate, >>>>> + >>>>> mode); >>>>> >>>>> if (dev_priv->psr.prepared && enable) >>>>> intel_psr_enable_locked(dev_priv, crtc_state); >> Hm I would change the psr irq inside the lock, then jump to pm_put to >> finish and call that label out. Most race-free way to do so. :) >> > Well doing that will keep intel_psr_set_debugfs_mode() doing more stuff > than it is supposed to but yeah it is taking the lock 3 times through > i915_edp_psr_debug_set(), I'm not so concerned about race conditions > because the use cases that we have would not cause it. > Meh we should make fastboot default, then we can test a real path.. Create a commit with crtc_state->mode_changed to true.. has_drrs and has_psr(2) can be updated dynamically based on actual values. the encoder->update_pipe() callback can en/disable psr/drrs as needed. ~Maarten
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 1a31921598e7..77b097b50fd5 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2639,19 +2639,29 @@ i915_edp_psr_debug_set(void *data, u64 val) { struct drm_i915_private *dev_priv = data; struct drm_modeset_acquire_ctx ctx; - int ret; + const u32 mode = val & I915_PSR_DEBUG_MODE_MASK; + int ret = 0; if (!CAN_PSR(dev_priv)) return -ENODEV; DRM_DEBUG_KMS("Setting PSR debug to %llx\n", val); + if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) || + mode > I915_PSR_DEBUG_FORCE_PSR1) { + DRM_DEBUG_KMS("Invalid debug mask %llx\n", val); + return -EINVAL; + } + + if (!mode) + goto skip_mode; + intel_runtime_pm_get(dev_priv); drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE); retry: - ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val); + ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, mode); if (ret == -EDEADLK) { ret = drm_modeset_backoff(&ctx); if (!ret) @@ -2663,6 +2673,14 @@ i915_edp_psr_debug_set(void *data, u64 val) intel_runtime_pm_put(dev_priv); +skip_mode: + if (!ret) { + mutex_lock(&dev_priv->psr.lock); + dev_priv->psr.debug = val; + intel_psr_irq_control(dev_priv, dev_priv->psr.debug); + mutex_unlock(&dev_priv->psr.lock); + } + return ret; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 1a11c2beb7f3..2367f07ba29e 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -2063,7 +2063,7 @@ 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); + u32 mode); 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 0ef6c5f8c298..bba4f7da68b3 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -69,13 +69,14 @@ static bool psr_global_enabled(u32 debug) } static bool intel_psr2_enabled(struct drm_i915_private *dev_priv, - const struct intel_crtc_state *crtc_state) + const struct intel_crtc_state *crtc_state, + u32 debug) { /* Cannot enable DSC and PSR2 simultaneously */ WARN_ON(crtc_state->dsc_params.compression_enable && crtc_state->has_psr2); - switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) { + switch (debug & I915_PSR_DEBUG_MODE_MASK) { case I915_PSR_DEBUG_DISABLE: case I915_PSR_DEBUG_FORCE_PSR1: return false; @@ -758,7 +759,8 @@ void intel_psr_enable(struct intel_dp *intel_dp, goto unlock; } - dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state); + dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state, + dev_priv->psr.debug); dev_priv->psr.busy_frontbuffer_bits = 0; dev_priv->psr.prepared = true; dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)->pipe; @@ -944,7 +946,7 @@ static bool switching_psr(struct drm_i915_private *dev_priv, int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv, struct drm_modeset_acquire_ctx *ctx, - u64 val) + u32 mode) { struct drm_device *dev = &dev_priv->drm; struct drm_connector_state *conn_state; @@ -954,13 +956,6 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv, struct intel_dp *dp; int ret; bool enable; - u32 mode = val & I915_PSR_DEBUG_MODE_MASK; - - if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) || - mode > I915_PSR_DEBUG_FORCE_PSR1) { - DRM_DEBUG_KMS("Invalid debug mask %llx\n", val); - return -EINVAL; - } ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx); if (ret) @@ -990,16 +985,15 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv, if (ret) return ret; - enable = psr_global_enabled(val); + enable = psr_global_enabled(mode); if (!enable || switching_psr(dev_priv, crtc_state, mode)) intel_psr_disable_locked(dev_priv->psr.dp); - dev_priv->psr.debug = val; if (crtc) - dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state); - - intel_psr_irq_control(dev_priv, dev_priv->psr.debug); + dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, + crtc_state, + mode); if (dev_priv->psr.prepared && enable) intel_psr_enable_locked(dev_priv, crtc_state);
intel_psr_set_debugfs_mode() don't just handle the PSR mode but it is also handling input validation, setting the new debug value and changing PSR IRQ masks. Lets move the roles listed above to the caller to make the function name and what it does accurate. Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 22 ++++++++++++++++++++-- drivers/gpu/drm/i915/intel_drv.h | 2 +- drivers/gpu/drm/i915/intel_psr.c | 26 ++++++++++---------------- 3 files changed, 31 insertions(+), 19 deletions(-)