| Message ID | 20170309154434.29303-6-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 09, 2017 at 05:44:34PM +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Use I915_{READ,WRITE}_FW() for updating the DSPARB registers on > VLV/CHV. This is less expesive as we can grab the uncore.lock across > the entire sequence of reads and writes instead of each register > access grabbing it. > > This also allows us to eliminate the dsparb lock entirely as the > uncore.lock now effectively protects the contents of the DSPARB > registers. > > 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 | 36 +++++++++++++++++++++--------------- > 3 files changed, 21 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index b1e9027a4f80..debb74a2b2a9 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -827,7 +827,6 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv, > > 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); > 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 3002996ddbed..6af0b1d33cab 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2375,9 +2375,6 @@ 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 99e09f63d4b3..24cdc13a416a 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -1358,13 +1358,19 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, > > trace_vlv_fifo_size(crtc, sprite0_start, sprite1_start, fifo_size); > > - spin_lock(&dev_priv->wm.dsparb_lock); > + /* > + * uncore.lock serves a double purpose here. It allows us to > + * use the less expensive I915_{READ,WRITE}_FW() functions, and > + * it protects the DSPARB registers from getting clobbered by > + * parallel updates from multiple pipes. > + */ > + spin_lock(&dev_priv->uncore.lock); Might be nice to document that the irq is disabled by intel_pipe_update_start() (hence why spin_lock is safe). -Chris
On Thu, Mar 09, 2017 at 09:26:36PM +0000, Chris Wilson wrote: > On Thu, Mar 09, 2017 at 05:44:34PM +0200, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Use I915_{READ,WRITE}_FW() for updating the DSPARB registers on > > VLV/CHV. This is less expesive as we can grab the uncore.lock across > > the entire sequence of reads and writes instead of each register > > access grabbing it. > > > > This also allows us to eliminate the dsparb lock entirely as the > > uncore.lock now effectively protects the contents of the DSPARB > > registers. > > > > 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 | 36 +++++++++++++++++++++--------------- > > 3 files changed, 21 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index b1e9027a4f80..debb74a2b2a9 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -827,7 +827,6 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv, > > > > 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); > > 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 3002996ddbed..6af0b1d33cab 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -2375,9 +2375,6 @@ 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 99e09f63d4b3..24cdc13a416a 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -1358,13 +1358,19 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, > > > > trace_vlv_fifo_size(crtc, sprite0_start, sprite1_start, fifo_size); > > > > - spin_lock(&dev_priv->wm.dsparb_lock); > > + /* > > + * uncore.lock serves a double purpose here. It allows us to > > + * use the less expensive I915_{READ,WRITE}_FW() functions, and > > + * it protects the DSPARB registers from getting clobbered by > > + * parallel updates from multiple pipes. > > + */ > > + spin_lock(&dev_priv->uncore.lock); > > Might be nice to document that the irq is disabled by > intel_pipe_update_start() (hence why spin_lock is safe). Sure. I'll add a note.
On Fri, Mar 10, 2017 at 12:07:53PM +0200, Ville Syrjälä wrote: > On Thu, Mar 09, 2017 at 09:26:36PM +0000, Chris Wilson wrote: > > On Thu, Mar 09, 2017 at 05:44:34PM +0200, ville.syrjala@linux.intel.com wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Use I915_{READ,WRITE}_FW() for updating the DSPARB registers on > > > VLV/CHV. This is less expesive as we can grab the uncore.lock across > > > the entire sequence of reads and writes instead of each register > > > access grabbing it. > > > > > > This also allows us to eliminate the dsparb lock entirely as the > > > uncore.lock now effectively protects the contents of the DSPARB > > > registers. > > > > > > 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 | 36 +++++++++++++++++++++--------------- > > > 3 files changed, 21 insertions(+), 19 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > index b1e9027a4f80..debb74a2b2a9 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > @@ -827,7 +827,6 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv, > > > > > > 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); > > > 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 3002996ddbed..6af0b1d33cab 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -2375,9 +2375,6 @@ 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 99e09f63d4b3..24cdc13a416a 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -1358,13 +1358,19 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, > > > > > > trace_vlv_fifo_size(crtc, sprite0_start, sprite1_start, fifo_size); > > > > > > - spin_lock(&dev_priv->wm.dsparb_lock); > > > + /* > > > + * uncore.lock serves a double purpose here. It allows us to > > > + * use the less expensive I915_{READ,WRITE}_FW() functions, and > > > + * it protects the DSPARB registers from getting clobbered by > > > + * parallel updates from multiple pipes. > > > + */ > > > + spin_lock(&dev_priv->uncore.lock); > > > > Might be nice to document that the irq is disabled by > > intel_pipe_update_start() (hence why spin_lock is safe). > > Sure. I'll add a note. + * intel_pipe_update_start() has already disabled interrupts + * for us, so a plain spin_lock() is sufficient here. */ And entire series pushed to dinq. Thanks for the reviews.
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index b1e9027a4f80..debb74a2b2a9 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -827,7 +827,6 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv, 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); 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 3002996ddbed..6af0b1d33cab 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2375,9 +2375,6 @@ 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 99e09f63d4b3..24cdc13a416a 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -1358,13 +1358,19 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, trace_vlv_fifo_size(crtc, sprite0_start, sprite1_start, fifo_size); - spin_lock(&dev_priv->wm.dsparb_lock); + /* + * uncore.lock serves a double purpose here. It allows us to + * use the less expensive I915_{READ,WRITE}_FW() functions, and + * it protects the DSPARB registers from getting clobbered by + * parallel updates from multiple pipes. + */ + spin_lock(&dev_priv->uncore.lock); switch (crtc->pipe) { uint32_t dsparb, dsparb2, dsparb3; case PIPE_A: - dsparb = I915_READ(DSPARB); - dsparb2 = I915_READ(DSPARB2); + dsparb = I915_READ_FW(DSPARB); + dsparb2 = I915_READ_FW(DSPARB2); dsparb &= ~(VLV_FIFO(SPRITEA, 0xff) | VLV_FIFO(SPRITEB, 0xff)); @@ -1376,12 +1382,12 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, dsparb2 |= (VLV_FIFO(SPRITEA_HI, sprite0_start >> 8) | VLV_FIFO(SPRITEB_HI, sprite1_start >> 8)); - I915_WRITE(DSPARB, dsparb); - I915_WRITE(DSPARB2, dsparb2); + I915_WRITE_FW(DSPARB, dsparb); + I915_WRITE_FW(DSPARB2, dsparb2); break; case PIPE_B: - dsparb = I915_READ(DSPARB); - dsparb2 = I915_READ(DSPARB2); + dsparb = I915_READ_FW(DSPARB); + dsparb2 = I915_READ_FW(DSPARB2); dsparb &= ~(VLV_FIFO(SPRITEC, 0xff) | VLV_FIFO(SPRITED, 0xff)); @@ -1393,12 +1399,12 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, dsparb2 |= (VLV_FIFO(SPRITEC_HI, sprite0_start >> 8) | VLV_FIFO(SPRITED_HI, sprite1_start >> 8)); - I915_WRITE(DSPARB, dsparb); - I915_WRITE(DSPARB2, dsparb2); + I915_WRITE_FW(DSPARB, dsparb); + I915_WRITE_FW(DSPARB2, dsparb2); break; case PIPE_C: - dsparb3 = I915_READ(DSPARB3); - dsparb2 = I915_READ(DSPARB2); + dsparb3 = I915_READ_FW(DSPARB3); + dsparb2 = I915_READ_FW(DSPARB2); dsparb3 &= ~(VLV_FIFO(SPRITEE, 0xff) | VLV_FIFO(SPRITEF, 0xff)); @@ -1410,16 +1416,16 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, dsparb2 |= (VLV_FIFO(SPRITEE_HI, sprite0_start >> 8) | VLV_FIFO(SPRITEF_HI, sprite1_start >> 8)); - I915_WRITE(DSPARB3, dsparb3); - I915_WRITE(DSPARB2, dsparb2); + I915_WRITE_FW(DSPARB3, dsparb3); + I915_WRITE_FW(DSPARB2, dsparb2); break; default: break; } - POSTING_READ(DSPARB); + POSTING_READ_FW(DSPARB); - spin_unlock(&dev_priv->wm.dsparb_lock); + spin_unlock(&dev_priv->uncore.lock); } #undef VLV_FIFO