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