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 <[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_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 [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
