Re: [PATCH v2 3/6] drm/i915/psr: Make intel_psr_set_debugfs_mode() only handle PSR mode

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux