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 <[email protected]> >>> Cc: Rodrigo Vivi <[email protected]> >>> Cc: Maarten Lankhorst <[email protected]> >>> Signed-off-by: José Roberto de Souza <[email protected]> >>> --- >>> 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. :)
_______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
