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_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
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
