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]

 



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);
> 
> 

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

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

  Powered by Linux