Re: [PATCH 11/15] drm/i915: Protect DSPARB registers with a spinlock

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Dec 01, 2016 at 03:45:35PM +0100, Maarten Lankhorst wrote:
> Op 01-12-16 om 14:13 schreef Ville Syrjälä:
> > On Thu, Dec 01, 2016 at 12:56:16PM +0100, Maarten Lankhorst wrote:
> >> Op 28-11-16 om 18:37 schreef ville.syrjala@linux.intel.com:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> Each DSPARB register can house bits for two separate pipes, hence
> >>> we must protect the registers during reprogramming so that parallel
> >>> FIFO reconfigurations happening simultaneosly on multiple pipes won't
> >>> corrupt each others values.
> >>>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/i915_drv.c | 1 +
> >>>  drivers/gpu/drm/i915/i915_drv.h | 3 +++
> >>>  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
> >>>  3 files changed, 10 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >>> index 0fba4bb5655e..c98032e9112b 100644
> >>> --- a/drivers/gpu/drm/i915/i915_drv.c
> >>> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >>> @@ -811,6 +811,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
> >>>  	spin_lock_init(&dev_priv->uncore.lock);
> >>>  	spin_lock_init(&dev_priv->mm.object_stat_lock);
> >>>  	spin_lock_init(&dev_priv->mmio_flip_lock);
> >>> +	spin_lock_init(&dev_priv->wm.dsparb_lock);
> >> Can we make sure all wm updates are done with dev_priv->wm.wm_mutex held instead?
> > We can't grab a mutex from the vblank evade critical section. We'd have
> > to hold the mutex across the whole thing then. Not sure if that would be
> > a good or a bad thing.
> Ah right, I missed that part since it didn't happen in the patches yet, might be worth pointing out in the commit message.

Right. A bit of tunnel vision on my part here. I'll plop in a note about it.

> 
> Will vlv_wm_values also be updated with that lock in the future? Looks like it could be a fun race otherwise.

Yes, wm_mutex will be protecting all the global wm state.

> >> gen9 wm's and ilk wm's use that mutex, for similar reasons.
> >>>  	mutex_init(&dev_priv->sb_lock);
> >>>  	mutex_init(&dev_priv->modeset_restore_lock);
> >>>  	mutex_init(&dev_priv->av_mutex);
> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>> index 9d808b706824..75439e9a67f7 100644
> >>> --- a/drivers/gpu/drm/i915/i915_drv.h
> >>> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >>> @@ -2169,6 +2169,9 @@ struct drm_i915_private {
> >>>  	} sagv_status;
> >>>  
> >>>  	struct {
> >>> +		/* protects DSPARB registers on pre-g4x/vlv/chv */
> >>> +		spinlock_t dsparb_lock;
> >>> +
> >>>  		/*
> >>>  		 * Raw watermark latency values:
> >>>  		 * in 0.1us units for WM0,
> >>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>> index 24d85a4e4ed3..9f25f2195a6a 100644
> >>> --- a/drivers/gpu/drm/i915/intel_pm.c
> >>> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >>> @@ -1215,6 +1215,8 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
> >>>  		      pipe_name(crtc->pipe), sprite0_start,
> >>>  		      sprite1_start, fifo_size);
> >>>  
> >>> +	spin_lock(&dev_priv->wm.dsparb_lock);
> >>> +
> >>>  	switch (crtc->pipe) {
> >>>  		uint32_t dsparb, dsparb2, dsparb3;
> >>>  	case PIPE_A:
> >>> @@ -1271,6 +1273,10 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
> >>>  	default:
> >>>  		break;
> >>>  	}
> >>> +
> >>> +	POSTING_READ(DSPARB);
> >>> +
> >>> +	spin_unlock(&dev_priv->wm.dsparb_lock);
> >>>  }
> >>>  
> >>>  #undef VLV_FIFO
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx