[PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion.

[PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion.

[Maarten Lankhorst]

Some cleanups to move the uncore.lock around vblank evasion, so run
to completion without racing on uncore.lock. Hopefully this will reduce
the chance of underruns, and perhaps allows us to decrease 
VBLANK_EVASION_TIME_US as well as a followup patch.

Tested on KBL and BSW.

Maarten Lankhorst (5):
  drm/i915: Keep vblank irq enabled during vblank evasion.
  drm/i915: Grab uncore.lock around enabling vblank evasion
  drm/i915: Call i915_pipe_update_start with uncore.lock held.
  drm/i915: Move all locking for plane updates to caller
  drm/i915: Use DOUBLE_BUFFER_CTL on top of vblank evasion for GEN9+.

 drivers/gpu/drm/i915/i915_irq.c      | 68 ++++++++++++++++++++++++------
 drivers/gpu/drm/i915/i915_reg.h      |  3 ++
 drivers/gpu/drm/i915/i915_trace.h    | 20 ++++-----
 drivers/gpu/drm/i915/intel_display.c | 74 +++++++++++++-------------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +
 drivers/gpu/drm/i915/intel_pm.c      | 16 +++----
 drivers/gpu/drm/i915/intel_sprite.c  | 81 ++++++++++--------------------------
 7 files changed, 128 insertions(+), 136 deletions(-)

-- 
2.16.1

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

[PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.

[Maarten Lankhorst]

This is a nice preparation for grabbing the uncore lock during evasion.
Grabbing the spinlock with the lock held messes up the locking,
so it's easier to handover the reference to the event.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 3be22c0fcfb5..971a1ea0db45 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 
 	local_irq_disable();
 
-	if (min <= 0 || max <= 0)
+	if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
 		return;
 
-	if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
+	if (min <= 0 || max <= 0)
 		return;
 
 	crtc->debug.min_vbl = min;
@@ -146,8 +146,6 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 
 	finish_wait(wq, &wait);
 
-	drm_crtc_vblank_put(&crtc->base);
-
 	/*
 	 * On VLV/CHV DSI the scanline counter would appear to
 	 * increment approx. 1/3 of a scanline before start of vblank.
@@ -197,14 +195,13 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
 	 * event outside of the critical section - the spinlock might spin for a
 	 * while ... */
 	if (new_crtc_state->base.event) {
-		WARN_ON(drm_crtc_vblank_get(&crtc->base) != 0);
-
 		spin_lock(&crtc->base.dev->event_lock);
 		drm_crtc_arm_vblank_event(&crtc->base, new_crtc_state->base.event);
 		spin_unlock(&crtc->base.dev->event_lock);
 
 		new_crtc_state->base.event = NULL;
-	}
+	} else
+		drm_crtc_vblank_put(&crtc->base);
 
 	local_irq_enable();
 
-- 
2.16.1

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

Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.

[Chris Wilson]

Quoting Maarten Lankhorst (2018-02-09 09:54:00)
> This is a nice preparation for grabbing the uncore lock during evasion.
> Grabbing the spinlock with the lock held messes up the locking,
> so it's easier to handover the reference to the event.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 3be22c0fcfb5..971a1ea0db45 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>  
>         local_irq_disable();
>  
> -       if (min <= 0 || max <= 0)
> +       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
>                 return;
>  
> -       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> +       if (min <= 0 || max <= 0)
>                 return;
>  

The corresponding vblank_put is the one later in update_start(), right?
I don't think you intended to keep this chunk.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.

[Maarten Lankhorst]

Op 12-02-18 om 16:10 schreef Chris Wilson:
> Quoting Maarten Lankhorst (2018-02-09 09:54:00)
>> This is a nice preparation for grabbing the uncore lock during evasion.
>> Grabbing the spinlock with the lock held messes up the locking,
>> so it's easier to handover the reference to the eve
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_sprite.c | 11 ++++-------
>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index 3be22c0fcfb5..971a1ea0db45 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>>  
>>         local_irq_disable();
>>  
>> -       if (min <= 0 || max <= 0)
>> +       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
>>                 return;
>>  
>> -       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
>> +       if (min <= 0 || max <= 0)
>>                 return;
>>  
> The corresponding vblank_put is the one later in update_start(), right?
> I don't think you intended to keep this chunk.
> -Chris

I'm not sure what you mean? The vblank_put is now in pipe_update_end, except if the
event takes over the reference. I think the code is correct. :)

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

Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.

[Chris Wilson]

Quoting Maarten Lankhorst (2018-02-12 15:16:39)
> Op 12-02-18 om 16:10 schreef Chris Wilson:
> > Quoting Maarten Lankhorst (2018-02-09 09:54:00)
> >> This is a nice preparation for grabbing the uncore lock during evasion.
> >> Grabbing the spinlock with the lock held messes up the locking,
> >> so it's easier to handover the reference to the eve
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_sprite.c | 11 ++++-------
> >>  1 file changed, 4 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >> index 3be22c0fcfb5..971a1ea0db45 100644
> >> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
> >>  
> >>         local_irq_disable();
> >>  
> >> -       if (min <= 0 || max <= 0)
> >> +       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> >>                 return;
> >>  
> >> -       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> >> +       if (min <= 0 || max <= 0)
> >>                 return;
> >>  
> > The corresponding vblank_put is the one later in update_start(), right?
> > I don't think you intended to keep this chunk.
> > -Chris
> 
> I'm not sure what you mean? The vblank_put is now in pipe_update_end, except if the
> event takes over the reference. I think the code is correct. :)

Then it's unbalanced in the case of error still.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.

[Maarten Lankhorst]

Op 12-02-18 om 16:22 schreef Chris Wilson:
> Quoting Maarten Lankhorst (2018-02-12 15:16:39)
>> Op 12-02-18 om 16:10 schreef Chris Wilson:
>>> Quoting Maarten Lankhorst (2018-02-09 09:54:00)
>>>> This is a nice preparation for grabbing the uncore lock during evasion.
>>>> Grabbing the spinlock with the lock held messes up the locking,
>>>> so it's easier to handover the reference to the eve
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_sprite.c | 11 ++++-------
>>>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>>>> index 3be22c0fcfb5..971a1ea0db45 100644
>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>>> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>>>>  
>>>>         local_irq_disable();
>>>>  
>>>> -       if (min <= 0 || max <= 0)
>>>> +       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
>>>>                 return;
>>>>  
>>>> -       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
>>>> +       if (min <= 0 || max <= 0)
>>>>                 return;
>>>>  
>>> The corresponding vblank_put is the one later in update_start(), right?
>>> I don't think you intended to keep this chunk.
>>> -Chris
>> I'm not sure what you mean? The vblank_put is now in pipe_update_end, except if the
>> event takes over the reference. I think the code is correct. :)
> Then it's unbalanced in the case of error still.
> -Chris

It already would have been for events, hence the WARN_ON there.
I don't think we can do anything about it, this shouldn't ever
happen in practice, could be a BUG_ON for all I care. :)

~Maarten

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

Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.

[Chris Wilson]

Quoting Maarten Lankhorst (2018-02-12 15:27:34)
> Op 12-02-18 om 16:22 schreef Chris Wilson:
> > Quoting Maarten Lankhorst (2018-02-12 15:16:39)
> >> Op 12-02-18 om 16:10 schreef Chris Wilson:
> >>> Quoting Maarten Lankhorst (2018-02-09 09:54:00)
> >>>> This is a nice preparation for grabbing the uncore lock during evasion.
> >>>> Grabbing the spinlock with the lock held messes up the locking,
> >>>> so it's easier to handover the reference to the eve
> >>>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/intel_sprite.c | 11 ++++-------
> >>>>  1 file changed, 4 insertions(+), 7 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >>>> index 3be22c0fcfb5..971a1ea0db45 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >>>> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
> >>>>  
> >>>>         local_irq_disable();
> >>>>  
> >>>> -       if (min <= 0 || max <= 0)
> >>>> +       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> >>>>                 return;
> >>>>  
> >>>> -       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> >>>> +       if (min <= 0 || max <= 0)
> >>>>                 return;
> >>>>  
> >>> The corresponding vblank_put is the one later in update_start(), right?
> >>> I don't think you intended to keep this chunk.
> >>> -Chris
> >> I'm not sure what you mean? The vblank_put is now in pipe_update_end, except if the
> >> event takes over the reference. I think the code is correct. :)
> > Then it's unbalanced in the case of error still.
> > -Chris
> 
> It already would have been for events, hence the WARN_ON there.
> I don't think we can do anything about it, this shouldn't ever
> happen in practice, could be a BUG_ON for all I care. :)

I would much prefer that over intentionally bad code.

But do we really need to enable the vblank irq here? If the event
requires it, doesn't it already enable the vblank. Here, we only need it
when sleeping, can we not determine we have enough time before the
vblank without enabling the interrupt?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.

[Maarten Lankhorst]

Op 12-02-18 om 16:31 schreef Chris Wilson:
> Quoting Maarten Lankhorst (2018-02-12 15:27:34)
>> Op 12-02-18 om 16:22 schreef Chris Wilson:
>>> Quoting Maarten Lankhorst (2018-02-12 15:16:39)
>>>> Op 12-02-18 om 16:10 schreef Chris Wilson:
>>>>> Quoting Maarten Lankhorst (2018-02-09 09:54:00)
>>>>>> This is a nice preparation for grabbing the uncore lock during evasion.
>>>>>> Grabbing the spinlock with the lock held messes up the locking,
>>>>>> so it's easier to handover the reference to the eve
>>>>>>
>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/i915/intel_sprite.c | 11 ++++-------
>>>>>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>>>>>> index 3be22c0fcfb5..971a1ea0db45 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>>>>> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>>>>>>  
>>>>>>         local_irq_disable();
>>>>>>  
>>>>>> -       if (min <= 0 || max <= 0)
>>>>>> +       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
>>>>>>                 return;
>>>>>>  
>>>>>> -       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
>>>>>> +       if (min <= 0 || max <= 0)
>>>>>>                 return;
>>>>>>  
>>>>> The corresponding vblank_put is the one later in update_start(), right?
>>>>> I don't think you intended to keep this chunk.
>>>>> -Chris
>>>> I'm not sure what you mean? The vblank_put is now in pipe_update_end, except if the
>>>> event takes over the reference. I think the code is correct. :)
>>> Then it's unbalanced in the case of error still.
>>> -Chris
>> It already would have been for events, hence the WARN_ON there.
>> I don't think we can do anything about it, this shouldn't ever
>> happen in practice, could be a BUG_ON for all I care. :)
> I would much prefer that over intentionally bad code.
>
> But do we really need to enable the vblank irq here? If the event
> requires it, doesn't it already enable the vblank. Here, we only need it
> when sleeping, can we not determine we have enough time before the
> vblank without enabling the interrupt?
I'm not sure why we get a reference to the vblank counter here. Perhaps Ville does?

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

Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.

[Ville Syrjälä]

On Mon, Feb 12, 2018 at 04:41:05PM +0100, Maarten Lankhorst wrote:
> Op 12-02-18 om 16:31 schreef Chris Wilson:
> > Quoting Maarten Lankhorst (2018-02-12 15:27:34)
> >> Op 12-02-18 om 16:22 schreef Chris Wilson:
> >>> Quoting Maarten Lankhorst (2018-02-12 15:16:39)
> >>>> Op 12-02-18 om 16:10 schreef Chris Wilson:
> >>>>> Quoting Maarten Lankhorst (2018-02-09 09:54:00)
> >>>>>> This is a nice preparation for grabbing the uncore lock during evasion.
> >>>>>> Grabbing the spinlock with the lock held messes up the locking,
> >>>>>> so it's easier to handover the reference to the eve
> >>>>>>
> >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>>> ---
> >>>>>>  drivers/gpu/drm/i915/intel_sprite.c | 11 ++++-------
> >>>>>>  1 file changed, 4 insertions(+), 7 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >>>>>> index 3be22c0fcfb5..971a1ea0db45 100644
> >>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >>>>>> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
> >>>>>>  
> >>>>>>         local_irq_disable();
> >>>>>>  
> >>>>>> -       if (min <= 0 || max <= 0)
> >>>>>> +       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> >>>>>>                 return;
> >>>>>>  
> >>>>>> -       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> >>>>>> +       if (min <= 0 || max <= 0)
> >>>>>>                 return;
> >>>>>>  
> >>>>> The corresponding vblank_put is the one later in update_start(), right?
> >>>>> I don't think you intended to keep this chunk.
> >>>>> -Chris
> >>>> I'm not sure what you mean? The vblank_put is now in pipe_update_end, except if the
> >>>> event takes over the reference. I think the code is correct. :)
> >>> Then it's unbalanced in the case of error still.
> >>> -Chris
> >> It already would have been for events, hence the WARN_ON there.
> >> I don't think we can do anything about it, this shouldn't ever
> >> happen in practice, could be a BUG_ON for all I care. :)
> > I would much prefer that over intentionally bad code.
> >
> > But do we really need to enable the vblank irq here? If the event
> > requires it, doesn't it already enable the vblank. Here, we only need it
> > when sleeping, can we not determine we have enough time before the
> > vblank without enabling the interrupt?
> I'm not sure why we get a reference to the vblank counter here. Perhaps Ville does?

We need the vblank irq to be enabled before we check the scanline since
otherwise we may end up doing:

1. check scanline
3. vblank irq fires
2. enable vblank irq
3. wait for the next vblank

So we'd end up wasting an entire frame.

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

Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.

[Chris Wilson]

Quoting Ville Syrjälä (2018-02-12 16:55:28)
> On Mon, Feb 12, 2018 at 04:41:05PM +0100, Maarten Lankhorst wrote:
> > Op 12-02-18 om 16:31 schreef Chris Wilson:
> > > Quoting Maarten Lankhorst (2018-02-12 15:27:34)
> > >> Op 12-02-18 om 16:22 schreef Chris Wilson:
> > >>> Quoting Maarten Lankhorst (2018-02-12 15:16:39)
> > >>>> Op 12-02-18 om 16:10 schreef Chris Wilson:
> > >>>>> Quoting Maarten Lankhorst (2018-02-09 09:54:00)
> > >>>>>> This is a nice preparation for grabbing the uncore lock during evasion.
> > >>>>>> Grabbing the spinlock with the lock held messes up the locking,
> > >>>>>> so it's easier to handover the reference to the eve
> > >>>>>>
> > >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >>>>>> ---
> > >>>>>>  drivers/gpu/drm/i915/intel_sprite.c | 11 ++++-------
> > >>>>>>  1 file changed, 4 insertions(+), 7 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > >>>>>> index 3be22c0fcfb5..971a1ea0db45 100644
> > >>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> > >>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > >>>>>> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
> > >>>>>>  
> > >>>>>>         local_irq_disable();
> > >>>>>>  
> > >>>>>> -       if (min <= 0 || max <= 0)
> > >>>>>> +       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> > >>>>>>                 return;
> > >>>>>>  
> > >>>>>> -       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> > >>>>>> +       if (min <= 0 || max <= 0)
> > >>>>>>                 return;
> > >>>>>>  
> > >>>>> The corresponding vblank_put is the one later in update_start(), right?
> > >>>>> I don't think you intended to keep this chunk.
> > >>>>> -Chris
> > >>>> I'm not sure what you mean? The vblank_put is now in pipe_update_end, except if the
> > >>>> event takes over the reference. I think the code is correct. :)
> > >>> Then it's unbalanced in the case of error still.
> > >>> -Chris
> > >> It already would have been for events, hence the WARN_ON there.
> > >> I don't think we can do anything about it, this shouldn't ever
> > >> happen in practice, could be a BUG_ON for all I care. :)
> > > I would much prefer that over intentionally bad code.
> > >
> > > But do we really need to enable the vblank irq here? If the event
> > > requires it, doesn't it already enable the vblank. Here, we only need it
> > > when sleeping, can we not determine we have enough time before the
> > > vblank without enabling the interrupt?
> > I'm not sure why we get a reference to the vblank counter here. Perhaps Ville does?
> 
> We need the vblank irq to be enabled before we check the scanline since
> otherwise we may end up doing:
> 
> 1. check scanline
> 3. vblank irq fires
> 2. enable vblank irq
> 3. wait for the next vblank
> 
> So we'd end up wasting an entire frame.

Step: 2.5, check_scanline?

Something like,

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 574bd02c5a2e..70c2ee1c7b8c 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -97,6 +97,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
        bool need_vlv_dsi_wa = (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
                intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI);
        DEFINE_WAIT(wait);
+       bool have_vblank_irq = false;
 
        vblank_start = adjusted_mode->crtc_vblank_start;
        if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
@@ -112,9 +113,6 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
        if (min <= 0 || max <= 0)
                return;
 
-       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
-               return;
-
        crtc->debug.min_vbl = min;
        crtc->debug.max_vbl = max;
        trace_i915_pipe_update_start(crtc);
@@ -127,6 +125,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
                 */
                prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
 
+               if (!have_vblank_irq)
+                       have_vblank_irq = !drm_crtc_vblank_get(&crtc->base);
+
                scanline = intel_get_crtc_scanline(crtc);
                if (scanline < min || scanline > max)
                        break;
@@ -145,8 +147,8 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
        }
 
        finish_wait(wq, &wait);
-
-       drm_crtc_vblank_put(&crtc->base);
+       if (have_vblank_irq)
+               drm_crtc_vblank_put(&crtc->base);
 
        /*
         * On VLV/CHV DSI the scanline counter would appear to

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

Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.

[Ville Syrjälä]

On Mon, Feb 12, 2018 at 05:24:54PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjälä (2018-02-12 16:55:28)
> > On Mon, Feb 12, 2018 at 04:41:05PM +0100, Maarten Lankhorst wrote:
> > > Op 12-02-18 om 16:31 schreef Chris Wilson:
> > > > Quoting Maarten Lankhorst (2018-02-12 15:27:34)
> > > >> Op 12-02-18 om 16:22 schreef Chris Wilson:
> > > >>> Quoting Maarten Lankhorst (2018-02-12 15:16:39)
> > > >>>> Op 12-02-18 om 16:10 schreef Chris Wilson:
> > > >>>>> Quoting Maarten Lankhorst (2018-02-09 09:54:00)
> > > >>>>>> This is a nice preparation for grabbing the uncore lock during evasion.
> > > >>>>>> Grabbing the spinlock with the lock held messes up the locking,
> > > >>>>>> so it's easier to handover the reference to the eve
> > > >>>>>>
> > > >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > >>>>>> ---
> > > >>>>>>  drivers/gpu/drm/i915/intel_sprite.c | 11 ++++-------
> > > >>>>>>  1 file changed, 4 insertions(+), 7 deletions(-)
> > > >>>>>>
> > > >>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > >>>>>> index 3be22c0fcfb5..971a1ea0db45 100644
> > > >>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > >>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > >>>>>> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
> > > >>>>>>  
> > > >>>>>>         local_irq_disable();
> > > >>>>>>  
> > > >>>>>> -       if (min <= 0 || max <= 0)
> > > >>>>>> +       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> > > >>>>>>                 return;
> > > >>>>>>  
> > > >>>>>> -       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> > > >>>>>> +       if (min <= 0 || max <= 0)
> > > >>>>>>                 return;
> > > >>>>>>  
> > > >>>>> The corresponding vblank_put is the one later in update_start(), right?
> > > >>>>> I don't think you intended to keep this chunk.
> > > >>>>> -Chris
> > > >>>> I'm not sure what you mean? The vblank_put is now in pipe_update_end, except if the
> > > >>>> event takes over the reference. I think the code is correct. :)
> > > >>> Then it's unbalanced in the case of error still.
> > > >>> -Chris
> > > >> It already would have been for events, hence the WARN_ON there.
> > > >> I don't think we can do anything about it, this shouldn't ever
> > > >> happen in practice, could be a BUG_ON for all I care. :)
> > > > I would much prefer that over intentionally bad code.
> > > >
> > > > But do we really need to enable the vblank irq here? If the event
> > > > requires it, doesn't it already enable the vblank. Here, we only need it
> > > > when sleeping, can we not determine we have enough time before the
> > > > vblank without enabling the interrupt?
> > > I'm not sure why we get a reference to the vblank counter here. Perhaps Ville does?
> > 
> > We need the vblank irq to be enabled before we check the scanline since
> > otherwise we may end up doing:
> > 
> > 1. check scanline
> > 3. vblank irq fires
> > 2. enable vblank irq
> > 3. wait for the next vblank
> > 
> > So we'd end up wasting an entire frame.
> 
> Step: 2.5, check_scanline?
> 
> Something like,
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 574bd02c5a2e..70c2ee1c7b8c 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -97,6 +97,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>         bool need_vlv_dsi_wa = (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>                 intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI);
>         DEFINE_WAIT(wait);
> +       bool have_vblank_irq = false;
>  
>         vblank_start = adjusted_mode->crtc_vblank_start;
>         if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
> @@ -112,9 +113,6 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>         if (min <= 0 || max <= 0)
>                 return;
>  
> -       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> -               return;
> -
>         crtc->debug.min_vbl = min;
>         crtc->debug.max_vbl = max;
>         trace_i915_pipe_update_start(crtc);
> @@ -127,6 +125,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>                  */
>                 prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
>  
> +               if (!have_vblank_irq)
> +                       have_vblank_irq = !drm_crtc_vblank_get(&crtc->base);
> +

This doesn't seem to change anything.

Did you mean something like this perhaps?

for (;;) {
	prepare_to_wait();

	if (scanline ok)
		break;

	if (!have_vbl_irq) {
		have_vbl_irq = !vbl_get();
		/*
		 * Check the scanline again to make sure
		 * we didn't just miss the vblank interrupt.
		 */
		continue;
	}

	local_irq_enable();
	schedule_timeout();
	local_irq_disable();
}

I guess something like that might work. Though if we're actually
worried about the vbl_get() failing, we'll need another flag besides
have_vbl_irq to avoid the inifinite loop.

>                 scanline = intel_get_crtc_scanline(crtc);
>                 if (scanline < min || scanline > max)
>                         break;
> @@ -145,8 +147,8 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>         }
>  
>         finish_wait(wq, &wait);
> -
> -       drm_crtc_vblank_put(&crtc->base);
> +       if (have_vblank_irq)
> +               drm_crtc_vblank_put(&crtc->base);
>  
>         /*
>          * On VLV/CHV DSI the scanline counter would appear to

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

Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.

[Chris Wilson]

Quoting Ville Syrjälä (2018-02-12 18:06:58)
> On Mon, Feb 12, 2018 at 05:24:54PM +0000, Chris Wilson wrote:
> > Quoting Ville Syrjälä (2018-02-12 16:55:28)
> > > On Mon, Feb 12, 2018 at 04:41:05PM +0100, Maarten Lankhorst wrote:
> > > > Op 12-02-18 om 16:31 schreef Chris Wilson:
> > > > > Quoting Maarten Lankhorst (2018-02-12 15:27:34)
> > > > >> Op 12-02-18 om 16:22 schreef Chris Wilson:
> > > > >>> Quoting Maarten Lankhorst (2018-02-12 15:16:39)
> > > > >>>> Op 12-02-18 om 16:10 schreef Chris Wilson:
> > > > >>>>> Quoting Maarten Lankhorst (2018-02-09 09:54:00)
> > > > >>>>>> This is a nice preparation for grabbing the uncore lock during evasion.
> > > > >>>>>> Grabbing the spinlock with the lock held messes up the locking,
> > > > >>>>>> so it's easier to handover the reference to the eve
> > > > >>>>>>
> > > > >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > >>>>>> ---
> > > > >>>>>>  drivers/gpu/drm/i915/intel_sprite.c | 11 ++++-------
> > > > >>>>>>  1 file changed, 4 insertions(+), 7 deletions(-)
> > > > >>>>>>
> > > > >>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > > >>>>>> index 3be22c0fcfb5..971a1ea0db45 100644
> > > > >>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > >>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > >>>>>> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
> > > > >>>>>>  
> > > > >>>>>>         local_irq_disable();
> > > > >>>>>>  
> > > > >>>>>> -       if (min <= 0 || max <= 0)
> > > > >>>>>> +       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> > > > >>>>>>                 return;
> > > > >>>>>>  
> > > > >>>>>> -       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> > > > >>>>>> +       if (min <= 0 || max <= 0)
> > > > >>>>>>                 return;
> > > > >>>>>>  
> > > > >>>>> The corresponding vblank_put is the one later in update_start(), right?
> > > > >>>>> I don't think you intended to keep this chunk.
> > > > >>>>> -Chris
> > > > >>>> I'm not sure what you mean? The vblank_put is now in pipe_update_end, except if the
> > > > >>>> event takes over the reference. I think the code is correct. :)
> > > > >>> Then it's unbalanced in the case of error still.
> > > > >>> -Chris
> > > > >> It already would have been for events, hence the WARN_ON there.
> > > > >> I don't think we can do anything about it, this shouldn't ever
> > > > >> happen in practice, could be a BUG_ON for all I care. :)
> > > > > I would much prefer that over intentionally bad code.
> > > > >
> > > > > But do we really need to enable the vblank irq here? If the event
> > > > > requires it, doesn't it already enable the vblank. Here, we only need it
> > > > > when sleeping, can we not determine we have enough time before the
> > > > > vblank without enabling the interrupt?
> > > > I'm not sure why we get a reference to the vblank counter here. Perhaps Ville does?
> > > 
> > > We need the vblank irq to be enabled before we check the scanline since
> > > otherwise we may end up doing:
> > > 
> > > 1. check scanline
> > > 3. vblank irq fires
> > > 2. enable vblank irq
> > > 3. wait for the next vblank
> > > 
> > > So we'd end up wasting an entire frame.
> > 
> > Step: 2.5, check_scanline?
> > 
> > Something like,
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 574bd02c5a2e..70c2ee1c7b8c 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -97,6 +97,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
> >         bool need_vlv_dsi_wa = (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
> >                 intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI);
> >         DEFINE_WAIT(wait);
> > +       bool have_vblank_irq = false;
> >  
> >         vblank_start = adjusted_mode->crtc_vblank_start;
> >         if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
> > @@ -112,9 +113,6 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
> >         if (min <= 0 || max <= 0)
> >                 return;
> >  
> > -       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> > -               return;
> > -
> >         crtc->debug.min_vbl = min;
> >         crtc->debug.max_vbl = max;
> >         trace_i915_pipe_update_start(crtc);
> > @@ -127,6 +125,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
> >                  */
> >                 prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
> >  
> > +               if (!have_vblank_irq)
> > +                       have_vblank_irq = !drm_crtc_vblank_get(&crtc->base);
> > +
> 
> This doesn't seem to change anything.

Nothing wrt to the existing code :)

The idea is that we have to enable the vblank-irq before doing the
scanline check in order to not miss the interrupt, which as I understand
it was the danger you highlighted with trying to avoid taking the
vblank-irq.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.

[Maarten Lankhorst]

Op 12-02-18 om 21:55 schreef Chris Wilson:
> Quoting Ville Syrjälä (2018-02-12 18:06:58)
>> On Mon, Feb 12, 2018 at 05:24:54PM +0000, Chris Wilson wrote:
>>> Quoting Ville Syrjälä (2018-02-12 16:55:28)
>>>> On Mon, Feb 12, 2018 at 04:41:05PM +0100, Maarten Lankhorst wrote:
>>>>> Op 12-02-18 om 16:31 schreef Chris Wilson:
>>>>>> Quoting Maarten Lankhorst (2018-02-12 15:27:34)
>>>>>>> Op 12-02-18 om 16:22 schreef Chris Wilson:
>>>>>>>> Quoting Maarten Lankhorst (2018-02-12 15:16:39)
>>>>>>>>> Op 12-02-18 om 16:10 schreef Chris Wilson:
>>>>>>>>>> Quoting Maarten Lankhorst (2018-02-09 09:54:00)
>>>>>>>>>>> This is a nice preparation for grabbing the uncore lock during evasion.
>>>>>>>>>>> Grabbing the spinlock with the lock held messes up the locking,
>>>>>>>>>>> so it's easier to handover the reference to the eve
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>>>>>> ---
>>>>>>>>>>>  drivers/gpu/drm/i915/intel_sprite.c | 11 ++++-------
>>>>>>>>>>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>>>>> index 3be22c0fcfb5..971a1ea0db45 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>>>>> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>>>>>>>>>>>  
>>>>>>>>>>>         local_irq_disable();
>>>>>>>>>>>  
>>>>>>>>>>> -       if (min <= 0 || max <= 0)
>>>>>>>>>>> +       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
>>>>>>>>>>>                 return;
>>>>>>>>>>>  
>>>>>>>>>>> -       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
>>>>>>>>>>> +       if (min <= 0 || max <= 0)
>>>>>>>>>>>                 return;
>>>>>>>>>>>  
>>>>>>>>>> The corresponding vblank_put is the one later in update_start(), right?
>>>>>>>>>> I don't think you intended to keep this chunk.
>>>>>>>>>> -Chris
>>>>>>>>> I'm not sure what you mean? The vblank_put is now in pipe_update_end, except if the
>>>>>>>>> event takes over the reference. I think the code is correct. :)
>>>>>>>> Then it's unbalanced in the case of error still.
>>>>>>>> -Chris
>>>>>>> It already would have been for events, hence the WARN_ON there.
>>>>>>> I don't think we can do anything about it, this shouldn't ever
>>>>>>> happen in practice, could be a BUG_ON for all I care. :)
>>>>>> I would much prefer that over intentionally bad code.
>>>>>>
>>>>>> But do we really need to enable the vblank irq here? If the event
>>>>>> requires it, doesn't it already enable the vblank. Here, we only need it
>>>>>> when sleeping, can we not determine we have enough time before the
>>>>>> vblank without enabling the interrupt?
>>>>> I'm not sure why we get a reference to the vblank counter here. Perhaps Ville does?
>>>> We need the vblank irq to be enabled before we check the scanline since
>>>> otherwise we may end up doing:
>>>>
>>>> 1. check scanline
>>>> 3. vblank irq fires
>>>> 2. enable vblank irq
>>>> 3. wait for the next vblank
>>>>
>>>> So we'd end up wasting an entire frame.
>>> Step: 2.5, check_scanline?
>>>
>>> Something like,
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>>> index 574bd02c5a2e..70c2ee1c7b8c 100644
>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>> @@ -97,6 +97,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>>>         bool need_vlv_dsi_wa = (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>>>                 intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI);
>>>         DEFINE_WAIT(wait);
>>> +       bool have_vblank_irq = false;
>>>  
>>>         vblank_start = adjusted_mode->crtc_vblank_start;
>>>         if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
>>> @@ -112,9 +113,6 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>>>         if (min <= 0 || max <= 0)
>>>                 return;
>>>  
>>> -       if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
>>> -               return;
>>> -
>>>         crtc->debug.min_vbl = min;
>>>         crtc->debug.max_vbl = max;
>>>         trace_i915_pipe_update_start(crtc);
>>> @@ -127,6 +125,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>>>                  */
>>>                 prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
>>>  
>>> +               if (!have_vblank_irq)
>>> +                       have_vblank_irq = !drm_crtc_vblank_get(&crtc->base);
>>> +
>> This doesn't seem to change anything.
> Nothing wrt to the existing code :)
>
> The idea is that we have to enable the vblank-irq before doing the
> scanline check in order to not miss the interrupt, which as I understand
> it was the danger you highlighted with trying to avoid taking the
> vblank-irq.
> -Chris

I've taken a look at the code, and most of the time we set crtc_state->event.
Either through calls like pageflip, or if not set we always allocate one in
drm_atomic_helper_setup_commit() except for legacy cursor updates.

Because of this I think the original patch is fine, and I kind of like
having everything prepared in pipe_update_start, while pipe_update_end
only has to release it.

~Maarten

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

[PATCH 2/5] drm/i915: Grab uncore.lock around enabling vblank evasion

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c     |  2 +-
 drivers/gpu/drm/i915/intel_drv.h    |  1 +
 drivers/gpu/drm/i915/intel_sprite.c | 10 ++++++----
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b886bd459acc..eda9543a0199 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -845,7 +845,7 @@ static u32 __intel_get_crtc_scanline_from_timestamp(struct intel_crtc *crtc)
 }
 
 /* I915_READ_FW, only for fast reads of display block, no need for forcewake etc. */
-static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
+int __intel_get_crtc_scanline(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 468ec1e90e16..fbdbbe741b2f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1340,6 +1340,7 @@ static inline bool intel_irqs_enabled(struct drm_i915_private *dev_priv)
 }
 
 int intel_get_crtc_scanline(struct intel_crtc *crtc);
+int __intel_get_crtc_scanline(struct intel_crtc *crtc);
 void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
 				     u8 pipe_mask);
 void gen8_irq_power_well_pre_disable(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 971a1ea0db45..3a34be4fd956 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -119,6 +119,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 	crtc->debug.max_vbl = max;
 	trace_i915_pipe_update_start(crtc);
 
+	spin_lock(&dev_priv->uncore.lock);
 	for (;;) {
 		/*
 		 * prepare_to_wait() has a memory barrier, which guarantees
@@ -127,7 +128,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 		 */
 		prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
 
-		scanline = intel_get_crtc_scanline(crtc);
+		scanline = __intel_get_crtc_scanline(crtc);
 		if (scanline < min || scanline > max)
 			break;
 
@@ -137,11 +138,11 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 			break;
 		}
 
-		local_irq_enable();
+		spin_unlock_irq(&dev_priv->uncore.lock);
 
 		timeout = schedule_timeout(timeout);
 
-		local_irq_disable();
+		spin_lock_irq(&dev_priv->uncore.lock);
 	}
 
 	finish_wait(wq, &wait);
@@ -162,8 +163,9 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 	 * FIXME figure out if BXT+ DSI suffers from this as well
 	 */
 	while (need_vlv_dsi_wa && scanline == vblank_start)
-		scanline = intel_get_crtc_scanline(crtc);
+		scanline = __intel_get_crtc_scanline(crtc);
 
+	spin_unlock(&dev_priv->uncore.lock);
 	crtc->debug.scanline_start = scanline;
 	crtc->debug.start_vbl_time = ktime_get();
 	crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc);
-- 
2.16.1

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

Re: [PATCH 2/5] drm/i915: Grab uncore.lock around enabling vblank evasion

[Chris Wilson]

Quoting Maarten Lankhorst (2018-02-09 09:54:01)
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c     |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h    |  1 +
>  drivers/gpu/drm/i915/intel_sprite.c | 10 ++++++----
>  3 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b886bd459acc..eda9543a0199 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -845,7 +845,7 @@ static u32 __intel_get_crtc_scanline_from_timestamp(struct intel_crtc *crtc)
>  }
>  
>  /* I915_READ_FW, only for fast reads of display block, no need for forcewake etc. */
> -static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
> +int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>  {
>         struct drm_device *dev = crtc->base.dev;
>         struct drm_i915_private *dev_priv = to_i915(dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 468ec1e90e16..fbdbbe741b2f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1340,6 +1340,7 @@ static inline bool intel_irqs_enabled(struct drm_i915_private *dev_priv)
>  }
>  
>  int intel_get_crtc_scanline(struct intel_crtc *crtc);
> +int __intel_get_crtc_scanline(struct intel_crtc *crtc);
>  void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
>                                      u8 pipe_mask);
>  void gen8_irq_power_well_pre_disable(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 971a1ea0db45..3a34be4fd956 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -119,6 +119,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>         crtc->debug.max_vbl = max;
>         trace_i915_pipe_update_start(crtc);
>  
> +       spin_lock(&dev_priv->uncore.lock);
>         for (;;) {
>                 /*
>                  * prepare_to_wait() has a memory barrier, which guarantees
> @@ -127,7 +128,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>                  */
>                 prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
>  
> -               scanline = intel_get_crtc_scanline(crtc);
> +               scanline = __intel_get_crtc_scanline(crtc);
>                 if (scanline < min || scanline > max)
>                         break;
>  
> @@ -137,11 +138,11 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>                         break;
>                 }
>  
> -               local_irq_enable();
> +               spin_unlock_irq(&dev_priv->uncore.lock);
>  
>                 timeout = schedule_timeout(timeout);
>  
> -               local_irq_disable();
> +               spin_lock_irq(&dev_priv->uncore.lock);
>         }
>  
>         finish_wait(wq, &wait);

There's no danger that drm_crtc_vblank_put() does something crazy here.
(Feels like a layering violation to call into DRM with the low level
uncore.lock held at least.) It looks like the driver can be tricked into
called ->disable_vblank()?

Overall though, I think it is just this need_vlv_dsi_wa chunk that has
any benefit here (although trading lock_irq for lock_irqsave is enough
to justify a change if frequently hit).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

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 <maarten.lankhorst@linux.intel.com>
---
 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)) & 0xffffff;
 }
 
+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);
+
+	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(
 			   __entry->pipe = crtc->pipe;
-			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
-										       crtc->pipe);
-			   __entry->scanline = intel_get_crtc_scanline(crtc);
+			   __entry->frame = __intel_crtc_get_vblank_counter(crtc);
+			   __entry->scanline = __intel_get_crtc_scanline(crtc);
 			   __entry->min = crtc->debug.min_vbl;
 			   __entry->max = crtc->debug.max_vbl;
 			   ),
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fbdbbe741b2f..048fcd3c960e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1489,6 +1489,7 @@ intel_wait_for_vblank_if_active(struct drm_i915_private *dev_priv, int pipe)
 }
 
 u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc);
+u32 __intel_crtc_get_vblank_counter(struct intel_crtc *crtc);
 
 int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
 void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 3a34be4fd956..95f0999ea18a 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -117,9 +117,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 
 	crtc->debug.min_vbl = min;
 	crtc->debug.max_vbl = max;
-	trace_i915_pipe_update_start(crtc);
 
 	spin_lock(&dev_priv->uncore.lock);
+	trace_i915_pipe_update_start(crtc);
+
 	for (;;) {
 		/*
 		 * prepare_to_wait() has a memory barrier, which guarantees
-- 
2.16.1

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

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

[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 <maarten.lankhorst@linux.intel.com>
> ---
>  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)) & 0xffffff;
>  }
>  
> +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(
>  			   __entry->pipe = crtc->pipe;
> -			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
> -										       crtc->pipe);
> -			   __entry->scanline = intel_get_crtc_scanline(crtc);
> +			   __entry->frame = __intel_crtc_get_vblank_counter(crtc);
> +			   __entry->scanline = __intel_get_crtc_scanline(crtc);
>  			   __entry->min = crtc->debug.min_vbl;
>  			   __entry->max = crtc->debug.max_vbl;
>  			   ),
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index fbdbbe741b2f..048fcd3c960e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1489,6 +1489,7 @@ intel_wait_for_vblank_if_active(struct drm_i915_private *dev_priv, int pipe)
>  }
>  
>  u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc);
> +u32 __intel_crtc_get_vblank_counter(struct intel_crtc *crtc);
>  
>  int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
>  void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 3a34be4fd956..95f0999ea18a 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -117,9 +117,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>  
>  	crtc->debug.min_vbl = min;
>  	crtc->debug.max_vbl = max;
> -	trace_i915_pipe_update_start(crtc);
>  
>  	spin_lock(&dev_priv->uncore.lock);
> +	trace_i915_pipe_update_start(crtc);
> +
>  	for (;;) {
>  		/*
>  		 * prepare_to_wait() has a memory barrier, which guarantees
> -- 
> 2.16.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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 <maarten.lankhorst@linux.intel.com>
>> ---
>>  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)) & 0xffffff;
>>  }
>>  
>> +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/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(
>>  			   __entry->pipe = crtc->pipe;
>> -			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
>> -										       crtc->pipe);
>> -			   __entry->scanline = intel_get_crtc_scanline(crtc);
>> +			   __entry->frame = __intel_crtc_get_vblank_counter(crtc);
>> +			   __entry->scanline = __intel_get_crtc_scanline(crtc);
>>  			   __entry->min = crtc->debug.min_vbl;
>>  			   __entry->max = crtc->debug.max_vbl;
>>  			   ),
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index fbdbbe741b2f..048fcd3c960e 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1489,6 +1489,7 @@ intel_wait_for_vblank_if_active(struct drm_i915_private *dev_priv, int pipe)
>>  }
>>  
>>  u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc);
>> +u32 __intel_crtc_get_vblank_counter(struct intel_crtc *crtc);
>>  
>>  int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
>>  void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index 3a34be4fd956..95f0999ea18a 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -117,9 +117,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>>  
>>  	crtc->debug.min_vbl = min;
>>  	crtc->debug.max_vbl = max;
>> -	trace_i915_pipe_update_start(crtc);
>>  
>>  	spin_lock(&dev_priv->uncore.lock);
>> +	trace_i915_pipe_update_start(crtc);
>> +
>>  	for (;;) {
>>  		/*
>>  		 * prepare_to_wait() has a memory barrier, which guarantees
>> -- 
>> 2.16.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


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

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

[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: [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held.

[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 <maarten.lankhorst@linux.intel.com>
> ---
>  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)) & 0xffffff;
>  }
>  
> +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: [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held.

[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 <maarten.lankhorst@linux.intel.com>
>> ---
>>  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)) & 0xffffff;
>>  }
>>  
>> +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: [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held.

[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 <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  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)) & 0xffffff;
> >>  }
> >>  
> >> +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: [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held.

[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 <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  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)) & 0xffffff;
>>>>  }
>>>>  
>>>> +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 mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915: Move all locking for plane updates to caller

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_trace.h    | 15 +++-----
 drivers/gpu/drm/i915/intel_display.c | 74 ++++++++++++++----------------------
 drivers/gpu/drm/i915/intel_pm.c      | 16 ++++----
 drivers/gpu/drm/i915/intel_sprite.c  | 61 +++++------------------------
 4 files changed, 52 insertions(+), 114 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index d4a5776282ff..84bad38b20ae 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -194,9 +194,8 @@ TRACE_EVENT(vlv_fifo_size,
 
 	    TP_fast_assign(
 			   __entry->pipe = crtc->pipe;
-			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
-										       crtc->pipe);
-			   __entry->scanline = intel_get_crtc_scanline(crtc);
+			   __entry->frame = __intel_crtc_get_vblank_counter(crtc);
+			   __entry->scanline = __intel_get_crtc_scanline(crtc);
 			   __entry->sprite0_start = sprite0_start;
 			   __entry->sprite1_start = sprite1_start;
 			   __entry->fifo_size = fifo_size;
@@ -226,9 +225,8 @@ TRACE_EVENT(intel_update_plane,
 	    TP_fast_assign(
 			   __entry->pipe = crtc->pipe;
 			   __entry->name = plane->name;
-			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
-										       crtc->pipe);
-			   __entry->scanline = intel_get_crtc_scanline(crtc);
+			   __entry->frame = __intel_crtc_get_vblank_counter(crtc);
+			   __entry->scanline = __intel_get_crtc_scanline(crtc);
 			   memcpy(__entry->src, &plane->state->src, sizeof(__entry->src));
 			   memcpy(__entry->dst, &plane->state->dst, sizeof(__entry->dst));
 			   ),
@@ -254,9 +252,8 @@ TRACE_EVENT(intel_disable_plane,
 	    TP_fast_assign(
 			   __entry->pipe = crtc->pipe;
 			   __entry->name = plane->name;
-			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
-										       crtc->pipe);
-			   __entry->scanline = intel_get_crtc_scanline(crtc);
+			   __entry->frame = __intel_crtc_get_vblank_counter(crtc);
+			   __entry->scanline = __intel_get_crtc_scanline(crtc);
 			   ),
 
 	    TP_printk("pipe %c, plane %s, frame=%u, scanline=%u",
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 60ba5bb3f34c..02f91a15d2aa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2734,14 +2734,17 @@ static void intel_plane_disable_noatomic(struct intel_crtc *crtc,
 		to_intel_crtc_state(crtc->base.state);
 	struct intel_plane_state *plane_state =
 		to_intel_plane_state(plane->base.state);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 
 	intel_set_plane_visible(crtc_state, plane_state, false);
 
 	if (plane->id == PLANE_PRIMARY)
 		intel_pre_disable_primary_noatomic(&crtc->base);
 
+	spin_lock_irq(&dev_priv->uncore.lock);
 	trace_intel_disable_plane(&plane->base, crtc);
 	plane->disable_plane(plane, crtc);
+	spin_unlock_irq(&dev_priv->uncore.lock);
 }
 
 static void
@@ -3255,7 +3258,6 @@ static void i9xx_update_plane(struct intel_plane *plane,
 	i915_reg_t reg = DSPCNTR(i9xx_plane);
 	int x = plane_state->main.x;
 	int y = plane_state->main.y;
-	unsigned long irqflags;
 	u32 dspaddr_offset;
 
 	linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
@@ -3265,8 +3267,6 @@ static void i9xx_update_plane(struct intel_plane *plane,
 	else
 		dspaddr_offset = linear_offset;
 
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
-
 	if (INTEL_GEN(dev_priv) < 4) {
 		/* pipesrc and dspsize control the size that is scaled from,
 		 * which should always be the user's requested size.
@@ -3303,8 +3303,6 @@ static void i9xx_update_plane(struct intel_plane *plane,
 			      dspaddr_offset);
 	}
 	POSTING_READ_FW(reg);
-
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
 static void i9xx_disable_plane(struct intel_plane *plane,
@@ -3312,9 +3310,6 @@ static void i9xx_disable_plane(struct intel_plane *plane,
 {
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
-	unsigned long irqflags;
-
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
 	I915_WRITE_FW(DSPCNTR(i9xx_plane), 0);
 	if (INTEL_GEN(dev_priv) >= 4)
@@ -3322,8 +3317,6 @@ static void i9xx_disable_plane(struct intel_plane *plane,
 	else
 		I915_WRITE_FW(DSPADDR(i9xx_plane), 0);
 	POSTING_READ_FW(DSPCNTR(i9xx_plane));
-
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
 static bool i9xx_plane_get_hw_state(struct intel_plane *plane)
@@ -3364,9 +3357,9 @@ static void skl_detach_scaler(struct intel_crtc *intel_crtc, int id)
 	struct drm_device *dev = intel_crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
-	I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe, id), 0);
-	I915_WRITE(SKL_PS_WIN_POS(intel_crtc->pipe, id), 0);
-	I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe, id), 0);
+	I915_WRITE_FW(SKL_PS_CTRL(intel_crtc->pipe, id), 0);
+	I915_WRITE_FW(SKL_PS_WIN_POS(intel_crtc->pipe, id), 0);
+	I915_WRITE_FW(SKL_PS_WIN_SZ(intel_crtc->pipe, id), 0);
 }
 
 /*
@@ -3752,9 +3745,9 @@ static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_sta
 	 * sized surface.
 	 */
 
-	I915_WRITE(PIPESRC(crtc->pipe),
-		   ((new_crtc_state->pipe_src_w - 1) << 16) |
-		   (new_crtc_state->pipe_src_h - 1));
+	I915_WRITE_FW(PIPESRC(crtc->pipe),
+		      ((new_crtc_state->pipe_src_w - 1) << 16) |
+		      (new_crtc_state->pipe_src_h - 1));
 
 	/* on skylake this is done by detaching scalers */
 	if (INTEL_GEN(dev_priv) >= 9) {
@@ -4841,10 +4834,10 @@ static void skylake_pfit_enable(struct intel_crtc *crtc)
 			return;
 
 		id = scaler_state->scaler_id;
-		I915_WRITE(SKL_PS_CTRL(pipe, id), PS_SCALER_EN |
+		I915_WRITE_FW(SKL_PS_CTRL(pipe, id), PS_SCALER_EN |
 			PS_FILTER_MEDIUM | scaler_state->scalers[id].mode);
-		I915_WRITE(SKL_PS_WIN_POS(pipe, id), crtc->config->pch_pfit.pos);
-		I915_WRITE(SKL_PS_WIN_SZ(pipe, id), crtc->config->pch_pfit.size);
+		I915_WRITE_FW(SKL_PS_WIN_POS(pipe, id), crtc->config->pch_pfit.pos);
+		I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, id), crtc->config->pch_pfit.size);
 	}
 }
 
@@ -4860,12 +4853,12 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
 		 * e.g. x201.
 		 */
 		if (IS_IVYBRIDGE(dev_priv) || IS_HASWELL(dev_priv))
-			I915_WRITE(PF_CTL(pipe), PF_ENABLE | PF_FILTER_MED_3x3 |
+			I915_WRITE_FW(PF_CTL(pipe), PF_ENABLE | PF_FILTER_MED_3x3 |
 						 PF_PIPE_SEL_IVB(pipe));
 		else
-			I915_WRITE(PF_CTL(pipe), PF_ENABLE | PF_FILTER_MED_3x3);
-		I915_WRITE(PF_WIN_POS(pipe), crtc->config->pch_pfit.pos);
-		I915_WRITE(PF_WIN_SZ(pipe), crtc->config->pch_pfit.size);
+			I915_WRITE_FW(PF_CTL(pipe), PF_ENABLE | PF_FILTER_MED_3x3);
+		I915_WRITE_FW(PF_WIN_POS(pipe), crtc->config->pch_pfit.pos);
+		I915_WRITE_FW(PF_WIN_SZ(pipe), crtc->config->pch_pfit.size);
 	}
 }
 
@@ -5172,14 +5165,17 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
 static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
 {
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_plane *p;
 	int pipe = intel_crtc->pipe;
 
 	intel_crtc_dpms_overlay_disable(intel_crtc);
 
+	spin_lock_irq(&dev_priv->uncore.lock);
 	drm_for_each_plane_mask(p, dev, plane_mask)
 		to_intel_plane(p)->disable_plane(to_intel_plane(p), intel_crtc);
+	spin_unlock_irq(&dev_priv->uncore.lock);
 
 	/*
 	 * FIXME: Once we grow proper nuclear flip support out of this we need
@@ -5477,10 +5473,12 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
 	if (psl_clkgate_wa)
 		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
 
+	spin_lock_irq(&dev_priv->uncore.lock);
 	if (INTEL_GEN(dev_priv) >= 9)
 		skylake_pfit_enable(intel_crtc);
 	else
 		ironlake_pfit_enable(intel_crtc);
+	spin_unlock_irq(&dev_priv->uncore.lock);
 
 	/*
 	 * On ILK+ LUT must be loaded before the pipe is running but with
@@ -5533,9 +5531,9 @@ static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force)
 	/* To avoid upsetting the power well on haswell only disable the pfit if
 	 * it's in use. The hw state code will make sure we get this right. */
 	if (force || crtc->config->pch_pfit.enabled) {
-		I915_WRITE(PF_CTL(pipe), 0);
-		I915_WRITE(PF_WIN_POS(pipe), 0);
-		I915_WRITE(PF_WIN_SZ(pipe), 0);
+		I915_WRITE_FW(PF_CTL(pipe), 0);
+		I915_WRITE_FW(PF_WIN_POS(pipe), 0);
+		I915_WRITE_FW(PF_WIN_SZ(pipe), 0);
 	}
 }
 
@@ -5623,10 +5621,12 @@ static void haswell_crtc_disable(struct intel_crtc_state *old_crtc_state,
 	if (!transcoder_is_dsi(cpu_transcoder))
 		intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
 
+	spin_lock_irq(&dev_priv->uncore.lock);
 	if (INTEL_GEN(dev_priv) >= 9)
 		skylake_scaler_disable(intel_crtc);
 	else
 		ironlake_pfit_disable(intel_crtc, false);
+	spin_unlock_irq(&dev_priv->uncore.lock);
 
 	if (!transcoder_is_dsi(cpu_transcoder))
 		intel_ddi_disable_pipe_clock(intel_crtc->config);
@@ -9462,7 +9462,6 @@ static void i845_update_cursor(struct intel_plane *plane,
 {
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	u32 cntl = 0, base = 0, pos = 0, size = 0;
-	unsigned long irqflags;
 
 	if (plane_state && plane_state->base.visible) {
 		unsigned int width = plane_state->base.crtc_w;
@@ -9475,8 +9474,6 @@ static void i845_update_cursor(struct intel_plane *plane,
 		pos = intel_cursor_position(plane_state);
 	}
 
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
-
 	/* On these chipsets we can only modify the base/size/stride
 	 * whilst the cursor is disabled.
 	 */
@@ -9497,8 +9494,6 @@ static void i845_update_cursor(struct intel_plane *plane,
 	}
 
 	POSTING_READ_FW(CURCNTR(PIPE_A));
-
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
 static void i845_disable_cursor(struct intel_plane *plane,
@@ -9657,7 +9652,6 @@ static void i9xx_update_cursor(struct intel_plane *plane,
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	enum pipe pipe = plane->pipe;
 	u32 cntl = 0, base = 0, pos = 0, fbc_ctl = 0;
-	unsigned long irqflags;
 
 	if (plane_state && plane_state->base.visible) {
 		cntl = plane_state->ctl;
@@ -9669,8 +9663,6 @@ static void i9xx_update_cursor(struct intel_plane *plane,
 		pos = intel_cursor_position(plane_state);
 	}
 
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
-
 	/*
 	 * On some platforms writing CURCNTR first will also
 	 * cause CURPOS to be armed by the CURBASE write.
@@ -9707,8 +9699,6 @@ static void i9xx_update_cursor(struct intel_plane *plane,
 	}
 
 	POSTING_READ_FW(CURBASE(pipe));
-
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
 static void i9xx_disable_cursor(struct intel_plane *plane,
@@ -12070,16 +12060,6 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
 	return drm_atomic_helper_prepare_planes(dev, state);
 }
 
-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 dev->driver->get_vblank_counter(dev, crtc->pipe);
-}
-
 static void intel_update_crtc(struct drm_crtc *crtc,
 			      struct drm_atomic_state *state,
 			      struct drm_crtc_state *old_crtc_state,
@@ -13130,6 +13110,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 	/* Swap plane state */
 	plane->state = new_plane_state;
 
+	spin_lock_irq(&dev_priv->uncore.lock);
 	if (plane->state->visible) {
 		trace_intel_update_plane(plane, to_intel_crtc(crtc));
 		intel_plane->update_plane(intel_plane,
@@ -13139,6 +13120,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 		trace_intel_disable_plane(plane, to_intel_crtc(crtc));
 		intel_plane->disable_plane(intel_plane, to_intel_crtc(crtc));
 	}
+	spin_unlock_irq(&dev_priv->uncore.lock);
 
 	old_vma = fetch_and_zero(&to_intel_plane_state(old_plane_state)->vma);
 	if (old_vma)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b2f5e3b9ada8..1899dad5b9d9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1964,7 +1964,6 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
 	 * intel_pipe_update_start() has already disabled interrupts
 	 * for us, so a plain spin_lock() is sufficient here.
 	 */
-	spin_lock(&dev_priv->uncore.lock);
 
 	switch (crtc->pipe) {
 		uint32_t dsparb, dsparb2, dsparb3;
@@ -2024,8 +2023,6 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
 	}
 
 	POSTING_READ_FW(DSPARB);
-
-	spin_unlock(&dev_priv->uncore.lock);
 }
 
 #undef VLV_FIFO
@@ -4795,9 +4792,9 @@ static void skl_ddb_entry_write(struct drm_i915_private *dev_priv,
 				const struct skl_ddb_entry *entry)
 {
 	if (entry->end)
-		I915_WRITE(reg, (entry->end - 1) << 16 | entry->start);
+		I915_WRITE_FW(reg, (entry->end - 1) << 16 | entry->start);
 	else
-		I915_WRITE(reg, 0);
+		I915_WRITE_FW(reg, 0);
 }
 
 static void skl_write_wm_level(struct drm_i915_private *dev_priv,
@@ -4812,7 +4809,7 @@ static void skl_write_wm_level(struct drm_i915_private *dev_priv,
 		val |= level->plane_res_l << PLANE_WM_LINES_SHIFT;
 	}
 
-	I915_WRITE(reg, val);
+	I915_WRITE_FW(reg, val);
 }
 
 static void skl_write_plane_wm(struct intel_crtc *intel_crtc,
@@ -5181,7 +5178,7 @@ static void skl_atomic_update_crtc_wm(struct intel_atomic_state *state,
 	if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc->base)))
 		return;
 
-	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
+	I915_WRITE_FW(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
 
 	for_each_plane_id_on_crtc(crtc, plane_id) {
 		if (plane_id != PLANE_CURSOR)
@@ -5208,8 +5205,11 @@ static void skl_initial_wm(struct intel_atomic_state *state,
 
 	mutex_lock(&dev_priv->wm.wm_mutex);
 
-	if (cstate->base.active_changed)
+	if (cstate->base.active_changed) {
+		spin_lock_irq(&dev_priv->uncore.lock);
 		skl_atomic_update_crtc_wm(state, cstate);
+		spin_unlock_irq(&dev_priv->uncore.lock);
+	}
 
 	skl_copy_wm_for_pipe(hw_vals, results, pipe);
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 95f0999ea18a..094b331b522d 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -98,6 +98,9 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 		intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI);
 	DEFINE_WAIT(wait);
 
+	/* Must be called before we acquire the spinlock. */
+	WARN_ON(drm_crtc_vblank_get(&crtc->base));
+
 	vblank_start = adjusted_mode->crtc_vblank_start;
 	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
 		vblank_start = DIV_ROUND_UP(vblank_start, 2);
@@ -107,10 +110,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 						      VBLANK_EVASION_TIME_US);
 	max = vblank_start - 1;
 
-	local_irq_disable();
-
-	if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
-		return;
+	spin_lock_irq(&dev_priv->uncore.lock);
 
 	if (min <= 0 || max <= 0)
 		return;
@@ -118,7 +118,6 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 	crtc->debug.min_vbl = min;
 	crtc->debug.max_vbl = max;
 
-	spin_lock(&dev_priv->uncore.lock);
 	trace_i915_pipe_update_start(crtc);
 
 	for (;;) {
@@ -166,10 +165,9 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 	while (need_vlv_dsi_wa && scanline == vblank_start)
 		scanline = __intel_get_crtc_scanline(crtc);
 
-	spin_unlock(&dev_priv->uncore.lock);
 	crtc->debug.scanline_start = scanline;
 	crtc->debug.start_vbl_time = ktime_get();
-	crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc);
+	crtc->debug.start_vbl_count = __intel_crtc_get_vblank_counter(crtc);
 
 	trace_i915_pipe_update_vblank_evaded(crtc);
 }
@@ -186,8 +184,8 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
 	enum pipe pipe = crtc->pipe;
-	int scanline_end = intel_get_crtc_scanline(crtc);
-	u32 end_vbl_count = intel_crtc_get_vblank_counter(crtc);
+	int scanline_end = __intel_get_crtc_scanline(crtc);
+	u32 end_vbl_count = __intel_crtc_get_vblank_counter(crtc);
 	ktime_t end_vbl_time = ktime_get();
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 
@@ -197,11 +195,11 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
 	 * Would be slightly nice to just grab the vblank count and arm the
 	 * event outside of the critical section - the spinlock might spin for a
 	 * while ... */
+	spin_unlock(&dev_priv->uncore.lock);
 	if (new_crtc_state->base.event) {
 		spin_lock(&crtc->base.dev->event_lock);
 		drm_crtc_arm_vblank_event(&crtc->base, new_crtc_state->base.event);
 		spin_unlock(&crtc->base.dev->event_lock);
-
 		new_crtc_state->base.event = NULL;
 	} else
 		drm_crtc_vblank_put(&crtc->base);
@@ -211,7 +209,8 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
 	if (intel_vgpu_active(dev_priv))
 		return;
 
-	if (crtc->debug.start_vbl_count &&
+	if (INTEL_GEN(dev_priv) < 9 &&
+	    crtc->debug.start_vbl_count &&
 	    crtc->debug.start_vbl_count != end_vbl_count) {
 		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us, min %d, max %d, scanline start %d, end %d\n",
 			  pipe_name(pipe), crtc->debug.start_vbl_count,
@@ -253,7 +252,6 @@ skl_update_plane(struct intel_plane *plane,
 	uint32_t y = plane_state->main.y;
 	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
 	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
-	unsigned long irqflags;
 
 	/* Sizes are 0 based */
 	src_w--;
@@ -261,8 +259,6 @@ skl_update_plane(struct intel_plane *plane,
 	crtc_w--;
 	crtc_h--;
 
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
-
 	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
 		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
 			      plane_state->color_ctl);
@@ -303,8 +299,6 @@ skl_update_plane(struct intel_plane *plane,
 	I915_WRITE_FW(PLANE_SURF(pipe, plane_id),
 		      intel_plane_ggtt_offset(plane_state) + surf_addr);
 	POSTING_READ_FW(PLANE_SURF(pipe, plane_id));
-
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
 void
@@ -313,16 +307,11 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	enum plane_id plane_id = plane->id;
 	enum pipe pipe = plane->pipe;
-	unsigned long irqflags;
-
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
 	I915_WRITE_FW(PLANE_CTL(pipe, plane_id), 0);
 
 	I915_WRITE_FW(PLANE_SURF(pipe, plane_id), 0);
 	POSTING_READ_FW(PLANE_SURF(pipe, plane_id));
-
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
 bool
@@ -467,7 +456,6 @@ vlv_update_plane(struct intel_plane *plane,
 	uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
 	uint32_t x = plane_state->main.x;
 	uint32_t y = plane_state->main.y;
-	unsigned long irqflags;
 
 	/* Sizes are 0 based */
 	crtc_w--;
@@ -475,8 +463,6 @@ vlv_update_plane(struct intel_plane *plane,
 
 	linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
 
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
-
 	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
 		chv_update_csc(plane, fb->format->format);
 
@@ -500,8 +486,6 @@ vlv_update_plane(struct intel_plane *plane,
 	I915_WRITE_FW(SPSURF(pipe, plane_id),
 		      intel_plane_ggtt_offset(plane_state) + sprsurf_offset);
 	POSTING_READ_FW(SPSURF(pipe, plane_id));
-
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
 static void
@@ -510,16 +494,11 @@ vlv_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	enum pipe pipe = plane->pipe;
 	enum plane_id plane_id = plane->id;
-	unsigned long irqflags;
-
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
 	I915_WRITE_FW(SPCNTR(pipe, plane_id), 0);
 
 	I915_WRITE_FW(SPSURF(pipe, plane_id), 0);
 	POSTING_READ_FW(SPSURF(pipe, plane_id));
-
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
 static bool
@@ -618,7 +597,6 @@ ivb_update_plane(struct intel_plane *plane,
 	uint32_t y = plane_state->main.y;
 	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
 	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
-	unsigned long irqflags;
 
 	/* Sizes are 0 based */
 	src_w--;
@@ -631,8 +609,6 @@ ivb_update_plane(struct intel_plane *plane,
 
 	linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
 
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
-
 	if (key->flags) {
 		I915_WRITE_FW(SPRKEYVAL(pipe), key->min_value);
 		I915_WRITE_FW(SPRKEYMAX(pipe), key->max_value);
@@ -658,8 +634,6 @@ ivb_update_plane(struct intel_plane *plane,
 	I915_WRITE_FW(SPRSURF(pipe),
 		      intel_plane_ggtt_offset(plane_state) + sprsurf_offset);
 	POSTING_READ_FW(SPRSURF(pipe));
-
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
 static void
@@ -667,9 +641,6 @@ ivb_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	enum pipe pipe = plane->pipe;
-	unsigned long irqflags;
-
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
 	I915_WRITE_FW(SPRCTL(pipe), 0);
 	/* Can't leave the scaler enabled... */
@@ -678,8 +649,6 @@ ivb_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
 
 	I915_WRITE_FW(SPRSURF(pipe), 0);
 	POSTING_READ_FW(SPRSURF(pipe));
-
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
 static bool
@@ -774,7 +743,6 @@ g4x_update_plane(struct intel_plane *plane,
 	uint32_t y = plane_state->main.y;
 	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
 	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
-	unsigned long irqflags;
 
 	/* Sizes are 0 based */
 	src_w--;
@@ -787,8 +755,6 @@ g4x_update_plane(struct intel_plane *plane,
 
 	linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
 
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
-
 	if (key->flags) {
 		I915_WRITE_FW(DVSKEYVAL(pipe), key->min_value);
 		I915_WRITE_FW(DVSKEYMAX(pipe), key->max_value);
@@ -809,8 +775,6 @@ g4x_update_plane(struct intel_plane *plane,
 	I915_WRITE_FW(DVSSURF(pipe),
 		      intel_plane_ggtt_offset(plane_state) + dvssurf_offset);
 	POSTING_READ_FW(DVSSURF(pipe));
-
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
 static void
@@ -818,9 +782,6 @@ g4x_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	enum pipe pipe = plane->pipe;
-	unsigned long irqflags;
-
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
 	I915_WRITE_FW(DVSCNTR(pipe), 0);
 	/* Disable the scaler */
@@ -828,8 +789,6 @@ g4x_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
 
 	I915_WRITE_FW(DVSSURF(pipe), 0);
 	POSTING_READ_FW(DVSSURF(pipe));
-
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
 static bool
-- 
2.16.1

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

[PATCH 5/5] drm/i915: Use DOUBLE_BUFFER_CTL on top of vblank evasion for GEN9+.

[Maarten Lankhorst]

This way, if somehow we wait too long there won't be much damage done..

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h     | 3 +++
 drivers/gpu/drm/i915/intel_sprite.c | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e9c79b560823..6fb9239b786a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6588,6 +6588,9 @@ enum {
 #define  DIGITAL_PORTA_HOTPLUG_SHORT_DETECT	(1 << 0)
 #define  DIGITAL_PORTA_HOTPLUG_LONG_DETECT	(2 << 0)
 
+#define DOUBLE_BUFFER_CTL	_MMIO(0x44500)
+#define  DOUBLE_BUFFER_CTL_GLOBAL_DISABLE	(1 << 0)
+
 /* refresh rate hardware control */
 #define RR_HW_CTL       _MMIO(0x45300)
 #define  RR_HW_LOW_POWER_FRAMES_MASK    0xff
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 094b331b522d..f0c378ecb8ae 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -169,6 +169,9 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 	crtc->debug.start_vbl_time = ktime_get();
 	crtc->debug.start_vbl_count = __intel_crtc_get_vblank_counter(crtc);
 
+	if (INTEL_GEN(dev_priv) >= 9)
+		I915_WRITE_FW(DOUBLE_BUFFER_CTL, DOUBLE_BUFFER_CTL_GLOBAL_DISABLE);
+
 	trace_i915_pipe_update_vblank_evaded(crtc);
 }
 
@@ -191,6 +194,9 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
 
 	trace_i915_pipe_update_end(crtc, end_vbl_count, scanline_end);
 
+	if (INTEL_GEN(dev_priv) >= 9)
+		I915_WRITE_FW(DOUBLE_BUFFER_CTL, 0);
+
 	/* We're still in the vblank-evade critical section, this can't race.
 	 * Would be slightly nice to just grab the vblank count and arm the
 	 * event outside of the critical section - the spinlock might spin for a
-- 
2.16.1

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

Re: [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion.

[Chris Wilson]

Quoting Maarten Lankhorst (2018-02-09 09:53:59)
> Some cleanups to move the uncore.lock around vblank evasion, so run
> to completion without racing on uncore.lock. Hopefully this will reduce
> the chance of underruns, and perhaps allows us to decrease 
> VBLANK_EVASION_TIME_US as well as a followup patch.
> 
> Tested on KBL and BSW.

* shivers

uncore.lock is a brutally contested lock. Ville's patches did work on
splitting the uncore.lock into forcewake and display variants, which
cuts down on the nasty side effects.

Latency profiling, another item for the CI/QA wishlist.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion.

[Maarten Lankhorst]

Op 09-02-18 om 11:04 schreef Chris Wilson:
> Quoting Maarten Lankhorst (2018-02-09 09:53:59)
>> Some cleanups to move the uncore.lock around vblank evasion, so run
>> to completion without racing on uncore.lock. Hopefully this will reduce
>> the chance of underruns, and perhaps allows us to decrease 
>> VBLANK_EVASION_TIME_US as well as a followup patch.
>>
>> Tested on KBL and BSW.
> * shivers
>
> uncore.lock is a brutally contested lock. Ville's patches did work on
> splitting the uncore.lock into forcewake and display variants, which
> cuts down on the nasty side effects.
>
> Latency profiling, another item for the CI/QA wishlist.
> -Chris

Yeah, unfortunately this is not different from status quo. We already
require everything inside vblank evasion to run as fast as possible,
and it's down to a list of register writes and a few reads. Those
already need the uncore.lock, so all we do now is being more explicit
about when we take it and eliminate contention when we write out the
register values.

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

Re: [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion.

[Ville Syrjälä]

On Fri, Feb 09, 2018 at 06:21:08PM +0100, Maarten Lankhorst wrote:
> Op 09-02-18 om 11:04 schreef Chris Wilson:
> > Quoting Maarten Lankhorst (2018-02-09 09:53:59)
> >> Some cleanups to move the uncore.lock around vblank evasion, so run
> >> to completion without racing on uncore.lock. Hopefully this will reduce
> >> the chance of underruns, and perhaps allows us to decrease 
> >> VBLANK_EVASION_TIME_US as well as a followup patch.
> >>
> >> Tested on KBL and BSW.
> > * shivers
> >
> > uncore.lock is a brutally contested lock. Ville's patches did work on
> > splitting the uncore.lock into forcewake and display variants, which
> > cuts down on the nasty side effects.
> >
> > Latency profiling, another item for the CI/QA wishlist.
> > -Chris
> 
> Yeah, unfortunately this is not different from status quo. We already
> require everything inside vblank evasion to run as fast as possible,
> and it's down to a list of register writes and a few reads. Those
> already need the uncore.lock, so all we do now is being more explicit
> about when we take it and eliminate contention when we write out the
> register values.

Would be nice to have some results for this though. IIRC when I was
benchmarking my update optimizations and the de_lock stuff I was
simply logging how long the updates take, and staring at histograms
of that after running a bunch of igts and whatnot. I'm not sure I
have the results anymore, but IIRC I did see some improvement.

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

Re: [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion.

[Maarten Lankhorst]

Op 12-02-18 om 18:01 schreef Ville Syrjälä:
> On Fri, Feb 09, 2018 at 06:21:08PM +0100, Maarten Lankhorst wrote:
>> Op 09-02-18 om 11:04 schreef Chris Wilson:
>>> Quoting Maarten Lankhorst (2018-02-09 09:53:59)
>>>> Some cleanups to move the uncore.lock around vblank evasion, so run
>>>> to completion without racing on uncore.lock. Hopefully this will reduce
>>>> the chance of underruns, and perhaps allows us to decrease 
>>>> VBLANK_EVASION_TIME_US as well as a followup patch.
>>>>
>>>> Tested on KBL and BSW.
>>> * shivers
>>>
>>> uncore.lock is a brutally contested lock. Ville's patches did work on
>>> splitting the uncore.lock into forcewake and display variants, which
>>> cuts down on the nasty side effects.
>>>
>>> Latency profiling, another item for the CI/QA wishlist.
>>> -Chris
>> Yeah, unfortunately this is not different from status quo. We already
>> require everything inside vblank evasion to run as fast as possible,
>> and it's down to a list of register writes and a few reads. Those
>> already need the uncore.lock, so all we do now is being more explicit
>> about when we take it and eliminate contention when we write out the
>> register values.
> Would be nice to have some results for this though. IIRC when I was
> benchmarking my update optimizations and the de_lock stuff I was
> simply logging how long the updates take, and staring at histograms
> of that after running a bunch of igts and whatnot. I'm not sure I
> have the results anymore, but IIRC I did see some improvement.
>
When testing with KBL and BSW, this patch series most updates complete in <40 us even with
all debug options set, with the highest amount of time being a single update of 93 us for BSW.

Because we take all locking including the vblank reference in advance, latency from acquiring
locks no longer affects the time critical part of vblank evasion.

Tested with kms_rotation_crc,

~Maarten

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

Re: [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion.

[Chris Wilson]

Quoting Maarten Lankhorst (2018-02-13 10:19:42)
> Op 12-02-18 om 18:01 schreef Ville Syrjälä:
> > On Fri, Feb 09, 2018 at 06:21:08PM +0100, Maarten Lankhorst wrote:
> >> Op 09-02-18 om 11:04 schreef Chris Wilson:
> >>> Quoting Maarten Lankhorst (2018-02-09 09:53:59)
> >>>> Some cleanups to move the uncore.lock around vblank evasion, so run
> >>>> to completion without racing on uncore.lock. Hopefully this will reduce
> >>>> the chance of underruns, and perhaps allows us to decrease 
> >>>> VBLANK_EVASION_TIME_US as well as a followup patch.
> >>>>
> >>>> Tested on KBL and BSW.
> >>> * shivers
> >>>
> >>> uncore.lock is a brutally contested lock. Ville's patches did work on
> >>> splitting the uncore.lock into forcewake and display variants, which
> >>> cuts down on the nasty side effects.
> >>>
> >>> Latency profiling, another item for the CI/QA wishlist.
> >>> -Chris
> >> Yeah, unfortunately this is not different from status quo. We already
> >> require everything inside vblank evasion to run as fast as possible,
> >> and it's down to a list of register writes and a few reads. Those
> >> already need the uncore.lock, so all we do now is being more explicit
> >> about when we take it and eliminate contention when we write out the
> >> register values.
> > Would be nice to have some results for this though. IIRC when I was
> > benchmarking my update optimizations and the de_lock stuff I was
> > simply logging how long the updates take, and staring at histograms
> > of that after running a bunch of igts and whatnot. I'm not sure I
> > have the results anymore, but IIRC I did see some improvement.
> >
> When testing with KBL and BSW, this patch series most updates complete in <40 us even with
> all debug options set, with the highest amount of time being a single update of 93 us for BSW.

To put that into perspective, that's a 4% delay in submission (ok, once
every 16ms so ~.25% amoritized). They start to notice and complain at
about a .5% drop in throughput, but fortunately they also don't tend to
run with outputs enabled.

That being said, if we can say this is capped to less than 50us, sure.
Although I reserve the right to complain later when we get response
targets less than 50us.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Grab the vblank evasion lock around the entire evasion.

[Patchwork]

== Series Details ==

Series: drm/i915: Grab the vblank evasion lock around the entire evasion.
URL   : https://patchwork.freedesktop.org/series/37986/
State : failure

== Summary ==

Series 37986v1 drm/i915: Grab the vblank evasion lock around the entire evasion.
https://patchwork.freedesktop.org/api/1.0/series/37986/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-FAIL (fi-kbl-7567u)
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713
                pass       -> SKIP       (fi-kbl-7567u)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> SKIP       (fi-kbl-7567u)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> SKIP       (fi-kbl-7567u)
        Subgroup basic-rte:
                pass       -> SKIP       (fi-kbl-7567u)
Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> SKIP       (fi-kbl-7567u)
Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (fi-kbl-7567u)
        Subgroup basic-no-display:
                pass       -> DMESG-WARN (fi-kbl-7567u)
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (fi-kbl-7567u)

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:417s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:428s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:489s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:483s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:485s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:468s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:456s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:570s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:582s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:411s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:509s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:388s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:412s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:461s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:416s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:455s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:496s
fi-kbl-7567u     total:288  pass:259  dwarn:3   dfail:1   fail:0   skip:25  time:409s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:501s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:425s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:506s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:522s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:484s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:470s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:415s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:429s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:531s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:400s
Blacklisted hosts:
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:470s

6c10ba221576c523e2574d83e75a87cdc7b0bc1e drm-tip: 2018y-02m-08d-19h-13m-44s UTC integration manifest
576df4828086 drm/i915: Use DOUBLE_BUFFER_CTL on top of vblank evasion for GEN9+.
ed09ed8129b3 drm/i915: Move all locking for plane updates to caller
eb336be9bdce drm/i915: Call i915_pipe_update_start with uncore.lock held.
b9e946cfb283 drm/i915: Grab uncore.lock around enabling vblank evasion
976a99c13014 drm/i915: Keep vblank irq enabled during vblank evasion.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7964/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Grab the vblank evasion lock around the entire evasion.

[Patchwork]

== Series Details ==

Series: drm/i915: Grab the vblank evasion lock around the entire evasion.
URL   : https://patchwork.freedesktop.org/series/37986/
State : success

== Summary ==

Series 37986v1 drm/i915: Grab the vblank evasion lock around the entire evasion.
https://patchwork.freedesktop.org/api/1.0/series/37986/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-skl-6260u)
Test prime_vgem:
        Subgroup basic-fence-flip:
                fail       -> PASS       (fi-ilk-650)

fi-bdw-5557u     total:288  pass:265  dwarn:0   dfail:0   fail:2   skip:21  time:446s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:422s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:504s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:482s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:490s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:473s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:465s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:568s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:574s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:417s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:512s
fi-hsw-4770      total:288  pass:259  dwarn:0   dfail:0   fail:2   skip:27  time:414s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:413s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:453s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:419s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:460s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:496s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:501s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:430s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:510s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:527s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:487s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:486s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:415s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:430s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:520s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:399s
Blacklisted hosts:
fi-glk-dsi       total:117  pass:105  dwarn:0   dfail:0   fail:0   skip:12 

817d13e99013266e1dc5e7fc056faa960e9a53dc drm-tip: 2018y-02m-12d-13h-34m-48s UTC integration manifest
c178e786a11c drm/i915: Use DOUBLE_BUFFER_CTL on top of vblank evasion for GEN9+.
a5c32a2c2484 drm/i915: Move all locking for plane updates to caller
dc25ed08f485 drm/i915: Call i915_pipe_update_start with uncore.lock held.
01ef705a11de drm/i915: Grab uncore.lock around enabling vblank evasion
7b997261ce22 drm/i915: Keep vblank irq enabled during vblank evasion.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7979/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: warning for drm/i915: Grab the vblank evasion lock around the entire evasion.

[Patchwork]

== Series Details ==

Series: drm/i915: Grab the vblank evasion lock around the entire evasion.
URL   : https://patchwork.freedesktop.org/series/37986/
State : warning

== Summary ==

Test gem_eio:
        Subgroup in-flight:
                pass       -> DMESG-WARN (shard-snb) fdo#104058
Test pm_rpm:
        Subgroup gem-evict-pwrite:
                fail       -> PASS       (shard-hsw)
        Subgroup pm-caching:
                fail       -> PASS       (shard-hsw)
        Subgroup universal-planes-dpms:
                fail       -> PASS       (shard-hsw)
Test kms_frontbuffer_tracking:
        Subgroup fbc-2p-primscrn-pri-indfb-draw-pwrite:
                pass       -> SKIP       (shard-hsw)
Test drv_suspend:
        Subgroup fence-restore-untiled:
                pass       -> SKIP       (shard-snb)

fdo#104058 https://bugs.freedesktop.org/show_bug.cgi?id=104058

shard-apl        total:3391 pass:1763 dwarn:1   dfail:0   fail:20  skip:1606 time:12575s
shard-hsw        total:3444 pass:1719 dwarn:1   dfail:0   fail:52  skip:1671 time:11907s
shard-snb        total:3444 pass:1349 dwarn:2   dfail:0   fail:10  skip:2083 time:6712s
Blacklisted hosts:
shard-kbl        total:3444 pass:1914 dwarn:1   dfail:0   fail:21  skip:1508 time:9740s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7979/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx