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@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.
> > 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.
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
