Re: [Intel-gfx] [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held.

2018-02-12 Thread Maarten Lankhorst 
Op 12-02-18 om 16:44 schreef Chris Wilson:
> Quoting Maarten Lankhorst (2018-02-12 15:39:16)
>> Op 12-02-18 om 16:19 schreef Chris Wilson:
>>> Quoting Maarten Lankhorst (2018-02-09 09:54:02)
 This requires being able to read the vblank counter with the
 uncore.lock already held. This is also a preparation for
 being able to run the entire vblank update sequence with
 the uncore lock held.

 Signed-off-by: Maarten Lankhorst 
 ---
  drivers/gpu/drm/i915/i915_irq.c | 66 
 ++---
  drivers/gpu/drm/i915/i915_trace.h   |  5 ++-
  drivers/gpu/drm/i915/intel_drv.h|  1 +
  drivers/gpu/drm/i915/intel_sprite.c |  3 +-
  4 files changed, 60 insertions(+), 15 deletions(-)

 diff --git a/drivers/gpu/drm/i915/i915_irq.c 
 b/drivers/gpu/drm/i915/i915_irq.c
 index eda9543a0199..6c491e63e07c 100644
 --- a/drivers/gpu/drm/i915/i915_irq.c
 +++ b/drivers/gpu/drm/i915/i915_irq.c
 @@ -736,13 +736,12 @@ static void i915_enable_asle_pipestat(struct 
 drm_i915_private *dev_priv)
  /* Called from drm generic code, passed a 'crtc', which
   * we use as a pipe index
   */
 -static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int 
 pipe)
 +static u32 __i915_get_vblank_counter(struct intel_crtc *crtc)
  {
 -   struct drm_i915_private *dev_priv = to_i915(dev);
 +   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 i915_reg_t high_frame, low_frame;
 u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
 -   const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode;
 -   unsigned long irqflags;
 +   const struct drm_display_mode *mode = 
 &crtc->base.dev->vblank[crtc->pipe].hwmode;
>>> lockdep_assert_held(&dev_priv->uncore.lock);
>>>
  
 htotal = mode->crtc_htotal;
 hsync_start = mode->crtc_hsync_start;
 @@ -756,10 +755,8 @@ static u32 i915_get_vblank_counter(struct drm_device 
 *dev, unsigned int pipe)
 /* Start of vblank event occurs at start of hsync */
 vbl_start -= htotal - hsync_start;
  
 -   high_frame = PIPEFRAME(pipe);
 -   low_frame = PIPEFRAMEPIXEL(pipe);
 -
 -   spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 +   high_frame = PIPEFRAME(crtc->pipe);
 +   low_frame = PIPEFRAMEPIXEL(crtc->pipe);
  
 /*
  * High & low register fields aren't synchronized, so make sure
 @@ -772,8 +769,6 @@ static u32 i915_get_vblank_counter(struct drm_device 
 *dev, unsigned int pipe)
 high2 = I915_READ_FW(high_frame) & PIPE_FRAME_HIGH_MASK;
 } while (high1 != high2);
  
 -   spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 -
 high1 >>= PIPE_FRAME_HIGH_SHIFT;
 pixel = low & PIPE_PIXEL_MASK;
 low >>= PIPE_FRAME_LOW_SHIFT;
 @@ -786,11 +781,60 @@ static u32 i915_get_vblank_counter(struct drm_device 
 *dev, unsigned int pipe)
 return (((high1 << 8) | low) + (pixel >= vbl_start)) & 0xff;
  }
  
 +static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int 
 pipe)
 +{
 +   struct drm_i915_private *dev_priv = to_i915(dev);
 +   unsigned long irqflags;
 +   u32 ret;
 +
 +   spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 +   ret = i915_get_vblank_counter(dev, pipe);
 +   spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 +
 +   return ret;
 +}
 +
 +static u32 __g4x_get_vblank_counter(struct intel_crtc *crtc)
 +{
 +   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>> lockdep_assert_held(&dev_priv->uncore.lock); ?
>>>
>>> Ok, why do we need uncore.lock held here at all? Serialisation of mmio
>>> access to the same cacheline is the age old reason, can we guarantee
>>> that doesn't happen anyway? (Probably not, but really most callers do
>>> not need the mmio w/a.)
>> Is the serialization only needed for writing?
> No, sadly not. Concurrent access of any type to the same cacheline is
> the trigger. (Supposed to be ivb-only.)
It's gonna be a pain to find all users, so I think keeping the uncore lock is 
good enough for now, or we need to split off the display engine lock..

>  
>> The only thing that can race with nonblocking atomic commits are legacy
>> cursor updates, but those can only happen if the cursor plane are not part
>> of the previous atomic commit. Those are also protected by plane->mutex,
>> so in theory same cache lines on the same pipes probably can't race..
> At worst, we could just use a vblank->spinlock?
Perhaps, but the amount of registers isn't exactly small, so I feel better if 
we use the same lock consistently..
___
Intel-gfx mail

Re: [Intel-gfx] [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held.

2018-02-12 Thread Chris Wilson 
Quoting Maarten Lankhorst (2018-02-12 15:39:16)
> Op 12-02-18 om 16:19 schreef Chris Wilson:
> > Quoting Maarten Lankhorst (2018-02-09 09:54:02)
> >> This requires being able to read the vblank counter with the
> >> uncore.lock already held. This is also a preparation for
> >> being able to run the entire vblank update sequence with
> >> the uncore lock held.
> >>
> >> Signed-off-by: Maarten Lankhorst 
> >> ---
> >>  drivers/gpu/drm/i915/i915_irq.c | 66 
> >> ++---
> >>  drivers/gpu/drm/i915/i915_trace.h   |  5 ++-
> >>  drivers/gpu/drm/i915/intel_drv.h|  1 +
> >>  drivers/gpu/drm/i915/intel_sprite.c |  3 +-
> >>  4 files changed, 60 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
> >> b/drivers/gpu/drm/i915/i915_irq.c
> >> index eda9543a0199..6c491e63e07c 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -736,13 +736,12 @@ static void i915_enable_asle_pipestat(struct 
> >> drm_i915_private *dev_priv)
> >>  /* Called from drm generic code, passed a 'crtc', which
> >>   * we use as a pipe index
> >>   */
> >> -static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int 
> >> pipe)
> >> +static u32 __i915_get_vblank_counter(struct intel_crtc *crtc)
> >>  {
> >> -   struct drm_i915_private *dev_priv = to_i915(dev);
> >> +   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> i915_reg_t high_frame, low_frame;
> >> u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
> >> -   const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode;
> >> -   unsigned long irqflags;
> >> +   const struct drm_display_mode *mode = 
> >> &crtc->base.dev->vblank[crtc->pipe].hwmode;
> > lockdep_assert_held(&dev_priv->uncore.lock);
> >
> >>  
> >> htotal = mode->crtc_htotal;
> >> hsync_start = mode->crtc_hsync_start;
> >> @@ -756,10 +755,8 @@ static u32 i915_get_vblank_counter(struct drm_device 
> >> *dev, unsigned int pipe)
> >> /* Start of vblank event occurs at start of hsync */
> >> vbl_start -= htotal - hsync_start;
> >>  
> >> -   high_frame = PIPEFRAME(pipe);
> >> -   low_frame = PIPEFRAMEPIXEL(pipe);
> >> -
> >> -   spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >> +   high_frame = PIPEFRAME(crtc->pipe);
> >> +   low_frame = PIPEFRAMEPIXEL(crtc->pipe);
> >>  
> >> /*
> >>  * High & low register fields aren't synchronized, so make sure
> >> @@ -772,8 +769,6 @@ static u32 i915_get_vblank_counter(struct drm_device 
> >> *dev, unsigned int pipe)
> >> high2 = I915_READ_FW(high_frame) & PIPE_FRAME_HIGH_MASK;
> >> } while (high1 != high2);
> >>  
> >> -   spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> >> -
> >> high1 >>= PIPE_FRAME_HIGH_SHIFT;
> >> pixel = low & PIPE_PIXEL_MASK;
> >> low >>= PIPE_FRAME_LOW_SHIFT;
> >> @@ -786,11 +781,60 @@ static u32 i915_get_vblank_counter(struct drm_device 
> >> *dev, unsigned int pipe)
> >> return (((high1 << 8) | low) + (pixel >= vbl_start)) & 0xff;
> >>  }
> >>  
> >> +static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int 
> >> pipe)
> >> +{
> >> +   struct drm_i915_private *dev_priv = to_i915(dev);
> >> +   unsigned long irqflags;
> >> +   u32 ret;
> >> +
> >> +   spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >> +   ret = i915_get_vblank_counter(dev, pipe);
> >> +   spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> >> +
> >> +   return ret;
> >> +}
> >> +
> >> +static u32 __g4x_get_vblank_counter(struct intel_crtc *crtc)
> >> +{
> >> +   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > lockdep_assert_held(&dev_priv->uncore.lock); ?
> >
> > Ok, why do we need uncore.lock held here at all? Serialisation of mmio
> > access to the same cacheline is the age old reason, can we guarantee
> > that doesn't happen anyway? (Probably not, but really most callers do
> > not need the mmio w/a.)
> 
> Is the serialization only needed for writing?

No, sadly not. Concurrent access of any type to the same cacheline is
the trigger. (Supposed to be ivb-only.)
 
> The only thing that can race with nonblocking atomic commits are legacy
> cursor updates, but those can only happen if the cursor plane are not part
> of the previous atomic commit. Those are also protected by plane->mutex,
> so in theory same cache lines on the same pipes probably can't race..

At worst, we could just use a vblank->spinlock?
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held.

2018-02-12 Thread Maarten Lankhorst 
Op 12-02-18 om 16:19 schreef Chris Wilson:
> Quoting Maarten Lankhorst (2018-02-09 09:54:02)
>> This requires being able to read the vblank counter with the
>> uncore.lock already held. This is also a preparation for
>> being able to run the entire vblank update sequence with
>> the uncore lock held.
>>
>> Signed-off-by: Maarten Lankhorst 
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 66 
>> ++---
>>  drivers/gpu/drm/i915/i915_trace.h   |  5 ++-
>>  drivers/gpu/drm/i915/intel_drv.h|  1 +
>>  drivers/gpu/drm/i915/intel_sprite.c |  3 +-
>>  4 files changed, 60 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index eda9543a0199..6c491e63e07c 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -736,13 +736,12 @@ static void i915_enable_asle_pipestat(struct 
>> drm_i915_private *dev_priv)
>>  /* Called from drm generic code, passed a 'crtc', which
>>   * we use as a pipe index
>>   */
>> -static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int 
>> pipe)
>> +static u32 __i915_get_vblank_counter(struct intel_crtc *crtc)
>>  {
>> -   struct drm_i915_private *dev_priv = to_i915(dev);
>> +   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> i915_reg_t high_frame, low_frame;
>> u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
>> -   const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode;
>> -   unsigned long irqflags;
>> +   const struct drm_display_mode *mode = 
>> &crtc->base.dev->vblank[crtc->pipe].hwmode;
> lockdep_assert_held(&dev_priv->uncore.lock);
>
>>  
>> htotal = mode->crtc_htotal;
>> hsync_start = mode->crtc_hsync_start;
>> @@ -756,10 +755,8 @@ static u32 i915_get_vblank_counter(struct drm_device 
>> *dev, unsigned int pipe)
>> /* Start of vblank event occurs at start of hsync */
>> vbl_start -= htotal - hsync_start;
>>  
>> -   high_frame = PIPEFRAME(pipe);
>> -   low_frame = PIPEFRAMEPIXEL(pipe);
>> -
>> -   spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>> +   high_frame = PIPEFRAME(crtc->pipe);
>> +   low_frame = PIPEFRAMEPIXEL(crtc->pipe);
>>  
>> /*
>>  * High & low register fields aren't synchronized, so make sure
>> @@ -772,8 +769,6 @@ static u32 i915_get_vblank_counter(struct drm_device 
>> *dev, unsigned int pipe)
>> high2 = I915_READ_FW(high_frame) & PIPE_FRAME_HIGH_MASK;
>> } while (high1 != high2);
>>  
>> -   spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>> -
>> high1 >>= PIPE_FRAME_HIGH_SHIFT;
>> pixel = low & PIPE_PIXEL_MASK;
>> low >>= PIPE_FRAME_LOW_SHIFT;
>> @@ -786,11 +781,60 @@ static u32 i915_get_vblank_counter(struct drm_device 
>> *dev, unsigned int pipe)
>> return (((high1 << 8) | low) + (pixel >= vbl_start)) & 0xff;
>>  }
>>  
>> +static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int 
>> pipe)
>> +{
>> +   struct drm_i915_private *dev_priv = to_i915(dev);
>> +   unsigned long irqflags;
>> +   u32 ret;
>> +
>> +   spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>> +   ret = i915_get_vblank_counter(dev, pipe);
>> +   spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>> +
>> +   return ret;
>> +}
>> +
>> +static u32 __g4x_get_vblank_counter(struct intel_crtc *crtc)
>> +{
>> +   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> lockdep_assert_held(&dev_priv->uncore.lock); ?
>
> Ok, why do we need uncore.lock held here at all? Serialisation of mmio
> access to the same cacheline is the age old reason, can we guarantee
> that doesn't happen anyway? (Probably not, but really most callers do
> not need the mmio w/a.)

Is the serialization only needed for writing?

The only thing that can race with nonblocking atomic commits are legacy
cursor updates, but those can only happen if the cursor plane are not part
of the previous atomic commit. Those are also protected by plane->mutex,
so in theory same cache lines on the same pipes probably can't race..

~Maarten

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

Re: [Intel-gfx] [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held.

2018-02-12 Thread Chris Wilson 
Quoting Maarten Lankhorst (2018-02-09 09:54:02)
> This requires being able to read the vblank counter with the
> uncore.lock already held. This is also a preparation for
> being able to run the entire vblank update sequence with
> the uncore lock held.
> 
> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 66 
> ++---
>  drivers/gpu/drm/i915/i915_trace.h   |  5 ++-
>  drivers/gpu/drm/i915/intel_drv.h|  1 +
>  drivers/gpu/drm/i915/intel_sprite.c |  3 +-
>  4 files changed, 60 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index eda9543a0199..6c491e63e07c 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -736,13 +736,12 @@ static void i915_enable_asle_pipestat(struct 
> drm_i915_private *dev_priv)
>  /* Called from drm generic code, passed a 'crtc', which
>   * we use as a pipe index
>   */
> -static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> +static u32 __i915_get_vblank_counter(struct intel_crtc *crtc)
>  {
> -   struct drm_i915_private *dev_priv = to_i915(dev);
> +   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> i915_reg_t high_frame, low_frame;
> u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
> -   const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode;
> -   unsigned long irqflags;
> +   const struct drm_display_mode *mode = 
> &crtc->base.dev->vblank[crtc->pipe].hwmode;

lockdep_assert_held(&dev_priv->uncore.lock);

>  
> htotal = mode->crtc_htotal;
> hsync_start = mode->crtc_hsync_start;
> @@ -756,10 +755,8 @@ static u32 i915_get_vblank_counter(struct drm_device 
> *dev, unsigned int pipe)
> /* Start of vblank event occurs at start of hsync */
> vbl_start -= htotal - hsync_start;
>  
> -   high_frame = PIPEFRAME(pipe);
> -   low_frame = PIPEFRAMEPIXEL(pipe);
> -
> -   spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> +   high_frame = PIPEFRAME(crtc->pipe);
> +   low_frame = PIPEFRAMEPIXEL(crtc->pipe);
>  
> /*
>  * High & low register fields aren't synchronized, so make sure
> @@ -772,8 +769,6 @@ static u32 i915_get_vblank_counter(struct drm_device 
> *dev, unsigned int pipe)
> high2 = I915_READ_FW(high_frame) & PIPE_FRAME_HIGH_MASK;
> } while (high1 != high2);
>  
> -   spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> -
> high1 >>= PIPE_FRAME_HIGH_SHIFT;
> pixel = low & PIPE_PIXEL_MASK;
> low >>= PIPE_FRAME_LOW_SHIFT;
> @@ -786,11 +781,60 @@ static u32 i915_get_vblank_counter(struct drm_device 
> *dev, unsigned int pipe)
> return (((high1 << 8) | low) + (pixel >= vbl_start)) & 0xff;
>  }
>  
> +static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> +{
> +   struct drm_i915_private *dev_priv = to_i915(dev);
> +   unsigned long irqflags;
> +   u32 ret;
> +
> +   spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> +   ret = i915_get_vblank_counter(dev, pipe);
> +   spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +
> +   return ret;
> +}
> +
> +static u32 __g4x_get_vblank_counter(struct intel_crtc *crtc)
> +{
> +   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);

lockdep_assert_held(&dev_priv->uncore.lock); ?

Ok, why do we need uncore.lock held here at all? Serialisation of mmio
access to the same cacheline is the age old reason, can we guarantee
that doesn't happen anyway? (Probably not, but really most callers do
not need the mmio w/a.)
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held.

2018-02-10 Thread Chris Wilson 
Quoting Maarten Lankhorst (2018-02-10 08:46:14)
> Op 10-02-18 om 00:08 schreef James Ausmus:
> > On Fri, Feb 09, 2018 at 10:54:02AM +0100, Maarten Lankhorst wrote:
> >> +static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int 
> >> pipe)
> >> +{
> >> +struct drm_i915_private *dev_priv = to_i915(dev);
> >> +unsigned long irqflags;
> >> +u32 ret;
> >> +
> >> +spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >> +ret = i915_get_vblank_counter(dev, pipe);
> > Shouldn't this be __i915_get_vblank_counter ?
> Good catch, it shouldn't have passed BAT. I guess we don't have a g4 or gen3 
> system there.

You killed them!
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held.

2018-02-10 Thread Maarten Lankhorst 
Op 10-02-18 om 00:08 schreef James Ausmus:
> On Fri, Feb 09, 2018 at 10:54:02AM +0100, Maarten Lankhorst wrote:
>> This requires being able to read the vblank counter with the
>> uncore.lock already held. This is also a preparation for
>> being able to run the entire vblank update sequence with
>> the uncore lock held.
>>
>> Signed-off-by: Maarten Lankhorst 
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 66 
>> ++---
>>  drivers/gpu/drm/i915/i915_trace.h   |  5 ++-
>>  drivers/gpu/drm/i915/intel_drv.h|  1 +
>>  drivers/gpu/drm/i915/intel_sprite.c |  3 +-
>>  4 files changed, 60 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index eda9543a0199..6c491e63e07c 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -736,13 +736,12 @@ static void i915_enable_asle_pipestat(struct 
>> drm_i915_private *dev_priv)
>>  /* Called from drm generic code, passed a 'crtc', which
>>   * we use as a pipe index
>>   */
>> -static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int 
>> pipe)
>> +static u32 __i915_get_vblank_counter(struct intel_crtc *crtc)
>>  {
>> -struct drm_i915_private *dev_priv = to_i915(dev);
>> +struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>  i915_reg_t high_frame, low_frame;
>>  u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
>> -const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode;
>> -unsigned long irqflags;
>> +const struct drm_display_mode *mode = 
>> &crtc->base.dev->vblank[crtc->pipe].hwmode;
>>  
>>  htotal = mode->crtc_htotal;
>>  hsync_start = mode->crtc_hsync_start;
>> @@ -756,10 +755,8 @@ static u32 i915_get_vblank_counter(struct drm_device 
>> *dev, unsigned int pipe)
>>  /* Start of vblank event occurs at start of hsync */
>>  vbl_start -= htotal - hsync_start;
>>  
>> -high_frame = PIPEFRAME(pipe);
>> -low_frame = PIPEFRAMEPIXEL(pipe);
>> -
>> -spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>> +high_frame = PIPEFRAME(crtc->pipe);
>> +low_frame = PIPEFRAMEPIXEL(crtc->pipe);
>>  
>>  /*
>>   * High & low register fields aren't synchronized, so make sure
>> @@ -772,8 +769,6 @@ static u32 i915_get_vblank_counter(struct drm_device 
>> *dev, unsigned int pipe)
>>  high2 = I915_READ_FW(high_frame) & PIPE_FRAME_HIGH_MASK;
>>  } while (high1 != high2);
>>  
>> -spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>> -
>>  high1 >>= PIPE_FRAME_HIGH_SHIFT;
>>  pixel = low & PIPE_PIXEL_MASK;
>>  low >>= PIPE_FRAME_LOW_SHIFT;
>> @@ -786,11 +781,60 @@ static u32 i915_get_vblank_counter(struct drm_device 
>> *dev, unsigned int pipe)
>>  return (((high1 << 8) | low) + (pixel >= vbl_start)) & 0xff;
>>  }
>>  
>> +static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int 
>> pipe)
>> +{
>> +struct drm_i915_private *dev_priv = to_i915(dev);
>> +unsigned long irqflags;
>> +u32 ret;
>> +
>> +spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>> +ret = i915_get_vblank_counter(dev, pipe);
> Shouldn't this be __i915_get_vblank_counter ?
Good catch, it shouldn't have passed BAT. I guess we don't have a g4 or gen3 
system there.

~Maarten
>
>> +spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>> +
>> +return ret;
>> +}
>> +
>> +static u32 __g4x_get_vblank_counter(struct intel_crtc *crtc)
>> +{
>> +struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +
>> +return I915_READ_FW(PIPE_FRMCOUNT_G4X(crtc->pipe));
>> +}
>> +
>>  static u32 g4x_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>>  {
>>  struct drm_i915_private *dev_priv = to_i915(dev);
>> +struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
>> +unsigned long irqflags;
>> +u32 ret;
>> +
>> +spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>> +ret = __g4x_get_vblank_counter(crtc);
>> +spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>> +
>> +return ret;
>> +}
>> +
>> +u32 __intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
>> +{
>> +struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +
>> +if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5)
>> +return __g4x_get_vblank_counter(crtc);
>> +else if (IS_GEN2(dev_priv))
>> +return 0;
>> +else
>> +return __i915_get_vblank_counter(crtc);
>> +}
>> +
>> +u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
>> +{
>> +struct drm_device *dev = crtc->base.dev;
>> +
>> +if (!dev->max_vblank_count)
>> +return drm_crtc_accurate_vblank_count(&crtc->base);
>>  
>> -return I915_READ(PIPE_FRMCOUNT_G4X(pipe));
>> +return dev->driver->get_vblank_counter(dev, crtc->pipe);
>>  }
>>  
>>  /*
>> diff --git a/drivers/gpu/drm/i915/i915_trace.h 
>> b/drivers/gpu/drm/i915

Re: [Intel-gfx] [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held.

2018-02-09 Thread James Ausmus 
On Fri, Feb 09, 2018 at 10:54:02AM +0100, Maarten Lankhorst wrote:
> This requires being able to read the vblank counter with the
> uncore.lock already held. This is also a preparation for
> being able to run the entire vblank update sequence with
> the uncore lock held.
> 
> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 66 
> ++---
>  drivers/gpu/drm/i915/i915_trace.h   |  5 ++-
>  drivers/gpu/drm/i915/intel_drv.h|  1 +
>  drivers/gpu/drm/i915/intel_sprite.c |  3 +-
>  4 files changed, 60 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index eda9543a0199..6c491e63e07c 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -736,13 +736,12 @@ static void i915_enable_asle_pipestat(struct 
> drm_i915_private *dev_priv)
>  /* Called from drm generic code, passed a 'crtc', which
>   * we use as a pipe index
>   */
> -static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> +static u32 __i915_get_vblank_counter(struct intel_crtc *crtc)
>  {
> - struct drm_i915_private *dev_priv = to_i915(dev);
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>   i915_reg_t high_frame, low_frame;
>   u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
> - const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode;
> - unsigned long irqflags;
> + const struct drm_display_mode *mode = 
> &crtc->base.dev->vblank[crtc->pipe].hwmode;
>  
>   htotal = mode->crtc_htotal;
>   hsync_start = mode->crtc_hsync_start;
> @@ -756,10 +755,8 @@ static u32 i915_get_vblank_counter(struct drm_device 
> *dev, unsigned int pipe)
>   /* Start of vblank event occurs at start of hsync */
>   vbl_start -= htotal - hsync_start;
>  
> - high_frame = PIPEFRAME(pipe);
> - low_frame = PIPEFRAMEPIXEL(pipe);
> -
> - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> + high_frame = PIPEFRAME(crtc->pipe);
> + low_frame = PIPEFRAMEPIXEL(crtc->pipe);
>  
>   /*
>* High & low register fields aren't synchronized, so make sure
> @@ -772,8 +769,6 @@ static u32 i915_get_vblank_counter(struct drm_device 
> *dev, unsigned int pipe)
>   high2 = I915_READ_FW(high_frame) & PIPE_FRAME_HIGH_MASK;
>   } while (high1 != high2);
>  
> - spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> -
>   high1 >>= PIPE_FRAME_HIGH_SHIFT;
>   pixel = low & PIPE_PIXEL_MASK;
>   low >>= PIPE_FRAME_LOW_SHIFT;
> @@ -786,11 +781,60 @@ static u32 i915_get_vblank_counter(struct drm_device 
> *dev, unsigned int pipe)
>   return (((high1 << 8) | low) + (pixel >= vbl_start)) & 0xff;
>  }
>  
> +static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> +{
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + unsigned long irqflags;
> + u32 ret;
> +
> + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> + ret = i915_get_vblank_counter(dev, pipe);

Shouldn't this be __i915_get_vblank_counter ?

> + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +
> + return ret;
> +}
> +
> +static u32 __g4x_get_vblank_counter(struct intel_crtc *crtc)
> +{
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +
> + return I915_READ_FW(PIPE_FRMCOUNT_G4X(crtc->pipe));
> +}
> +
>  static u32 g4x_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>  {
>   struct drm_i915_private *dev_priv = to_i915(dev);
> + struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> + unsigned long irqflags;
> + u32 ret;
> +
> + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> + ret = __g4x_get_vblank_counter(crtc);
> + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +
> + return ret;
> +}
> +
> +u32 __intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
> +{
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +
> + if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5)
> + return __g4x_get_vblank_counter(crtc);
> + else if (IS_GEN2(dev_priv))
> + return 0;
> + else
> + return __i915_get_vblank_counter(crtc);
> +}
> +
> +u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
> +{
> + struct drm_device *dev = crtc->base.dev;
> +
> + if (!dev->max_vblank_count)
> + return drm_crtc_accurate_vblank_count(&crtc->base);
>  
> - return I915_READ(PIPE_FRMCOUNT_G4X(pipe));
> + return dev->driver->get_vblank_counter(dev, crtc->pipe);
>  }
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/i915_trace.h 
> b/drivers/gpu/drm/i915/i915_trace.h
> index e1169c02eb2b..d4a5776282ff 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -280,9 +280,8 @@ TRACE_EVENT(i915_pipe_update_start,
>  
>   TP_fast_assign(
>