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@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> ---
> 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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
