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

[Ville Syrjälä]

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.syrj...@linux.intel.com:
> >>> From: Ville Syrjälä 
> >>>
> >>> 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ä 
> >>> ---
> >>>  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(_priv->uncore.lock);
> >>>   spin_lock_init(_priv->mm.object_stat_lock);
> >>>   spin_lock_init(_priv->mmio_flip_lock);
> >>> + spin_lock_init(_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(_priv->sb_lock);
> >>>   mutex_init(_priv->modeset_restore_lock);
> >>>   mutex_init(_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(_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(_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

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

[Maarten Lankhorst]

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.syrj...@linux.intel.com:
>>> From: Ville Syrjälä 
>>>
>>> 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ä 
>>> ---
>>>  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(_priv->uncore.lock);
>>> spin_lock_init(_priv->mm.object_stat_lock);
>>> spin_lock_init(_priv->mmio_flip_lock);
>>> +   spin_lock_init(_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.

Will vlv_wm_values also be updated with that lock in the future? Looks like it 
could be a fun race otherwise.
>> gen9 wm's and ilk wm's use that mutex, for similar reasons.
>>> mutex_init(_priv->sb_lock);
>>> mutex_init(_priv->modeset_restore_lock);
>>> mutex_init(_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(_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(_priv->wm.dsparb_lock);
>>>  }
>>>  
>>>  #undef VLV_FIFO


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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.syrj...@linux.intel.com:
> > From: Ville Syrjälä 
> >
> > 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ä 
> > ---
> >  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(_priv->uncore.lock);
> > spin_lock_init(_priv->mm.object_stat_lock);
> > spin_lock_init(_priv->mmio_flip_lock);
> > +   spin_lock_init(_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.

> 
> gen9 wm's and ilk wm's use that mutex, for similar reasons.
> > mutex_init(_priv->sb_lock);
> > mutex_init(_priv->modeset_restore_lock);
> > mutex_init(_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(_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(_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

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

[Maarten Lankhorst]

Op 28-11-16 om 18:37 schreef ville.syrj...@linux.intel.com:
> From: Ville Syrjälä 
>
> 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ä 
> ---
>  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(_priv->uncore.lock);
>   spin_lock_init(_priv->mm.object_stat_lock);
>   spin_lock_init(_priv->mmio_flip_lock);
> + spin_lock_init(_priv->wm.dsparb_lock);
Can we make sure all wm updates are done with dev_priv->wm.wm_mutex held 
instead?

gen9 wm's and ilk wm's use that mutex, for similar reasons.
>   mutex_init(_priv->sb_lock);
>   mutex_init(_priv->modeset_restore_lock);
>   mutex_init(_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(_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(_priv->wm.dsparb_lock);
>  }
>  
>  #undef VLV_FIFO


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 11/15] drm/i915: Protect DSPARB registers with a spinlock

[ville . syrjala]

From: Ville Syrjälä 

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ä 
---
 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(_priv->uncore.lock);
spin_lock_init(_priv->mm.object_stat_lock);
spin_lock_init(_priv->mmio_flip_lock);
+   spin_lock_init(_priv->wm.dsparb_lock);
mutex_init(_priv->sb_lock);
mutex_init(_priv->modeset_restore_lock);
mutex_init(_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(_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(_priv->wm.dsparb_lock);
 }
 
 #undef VLV_FIFO
-- 
2.7.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx