* [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion. @ 2018-02-09 9:53 Maarten Lankhorst 2018-02-09 9:54 ` [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion Maarten Lankhorst ` (8 more replies) 0 siblings, 9 replies; 33+ messages in thread From: Maarten Lankhorst @ 2018-02-09 9:53 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion. 2018-02-09 9:53 [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion Maarten Lankhorst @ 2018-02-09 9:54 ` Maarten Lankhorst 2018-02-12 15:10 ` Chris Wilson 2018-02-09 9:54 ` [PATCH 2/5] drm/i915: Grab uncore.lock around enabling " Maarten Lankhorst ` (7 subsequent siblings) 8 siblings, 1 reply; 33+ messages in thread From: Maarten Lankhorst @ 2018-02-09 9:54 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion. 2018-02-09 9:54 ` [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion Maarten Lankhorst @ 2018-02-12 15:10 ` Chris Wilson 2018-02-12 15:16 ` Maarten Lankhorst 0 siblings, 1 reply; 33+ messages in thread From: Chris Wilson @ 2018-02-12 15:10 UTC (permalink / raw) To: Maarten Lankhorst, intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion. 2018-02-12 15:10 ` Chris Wilson @ 2018-02-12 15:16 ` Maarten Lankhorst 2018-02-12 15:22 ` Chris Wilson 0 siblings, 1 reply; 33+ messages in thread From: Maarten Lankhorst @ 2018-02-12 15:16 UTC (permalink / raw) To: Chris Wilson, intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion. 2018-02-12 15:16 ` Maarten Lankhorst @ 2018-02-12 15:22 ` Chris Wilson 2018-02-12 15:27 ` Maarten Lankhorst 0 siblings, 1 reply; 33+ messages in thread From: Chris Wilson @ 2018-02-12 15:22 UTC (permalink / raw) To: Maarten Lankhorst, intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion. 2018-02-12 15:22 ` Chris Wilson @ 2018-02-12 15:27 ` Maarten Lankhorst 2018-02-12 15:31 ` Chris Wilson 0 siblings, 1 reply; 33+ messages in thread From: Maarten Lankhorst @ 2018-02-12 15:27 UTC (permalink / raw) To: Chris Wilson, intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion. 2018-02-12 15:27 ` Maarten Lankhorst @ 2018-02-12 15:31 ` Chris Wilson 2018-02-12 15:41 ` Maarten Lankhorst 0 siblings, 1 reply; 33+ messages in thread From: Chris Wilson @ 2018-02-12 15:31 UTC (permalink / raw) To: Maarten Lankhorst, intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion. 2018-02-12 15:31 ` Chris Wilson @ 2018-02-12 15:41 ` Maarten Lankhorst 2018-02-12 16:55 ` Ville Syrjälä 0 siblings, 1 reply; 33+ messages in thread From: Maarten Lankhorst @ 2018-02-12 15:41 UTC (permalink / raw) To: Chris Wilson, intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion. 2018-02-12 15:41 ` Maarten Lankhorst @ 2018-02-12 16:55 ` Ville Syrjälä 2018-02-12 17:24 ` Chris Wilson 0 siblings, 1 reply; 33+ messages in thread From: Ville Syrjälä @ 2018-02-12 16:55 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion. 2018-02-12 16:55 ` Ville Syrjälä @ 2018-02-12 17:24 ` Chris Wilson 2018-02-12 18:06 ` Ville Syrjälä 0 siblings, 1 reply; 33+ messages in thread From: Chris Wilson @ 2018-02-12 17:24 UTC (permalink / raw) To: Ville Syrjälä, Maarten Lankhorst; +Cc: intel-gfx 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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion. 2018-02-12 17:24 ` Chris Wilson @ 2018-02-12 18:06 ` Ville Syrjälä 2018-02-12 20:55 ` Chris Wilson 0 siblings, 1 reply; 33+ messages in thread From: Ville Syrjälä @ 2018-02-12 18:06 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion. 2018-02-12 18:06 ` Ville Syrjälä @ 2018-02-12 20:55 ` Chris Wilson 2018-02-13 8:59 ` Maarten Lankhorst 0 siblings, 1 reply; 33+ messages in thread From: Chris Wilson @ 2018-02-12 20:55 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion. 2018-02-12 20:55 ` Chris Wilson @ 2018-02-13 8:59 ` Maarten Lankhorst 0 siblings, 0 replies; 33+ messages in thread From: Maarten Lankhorst @ 2018-02-13 8:59 UTC (permalink / raw) To: Chris Wilson, Ville Syrjälä; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 2/5] drm/i915: Grab uncore.lock around enabling vblank evasion 2018-02-09 9:53 [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion Maarten Lankhorst 2018-02-09 9:54 ` [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion Maarten Lankhorst @ 2018-02-09 9:54 ` Maarten Lankhorst 2018-02-12 15:16 ` Chris Wilson 2018-02-09 9:54 ` [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held Maarten Lankhorst ` (6 subsequent siblings) 8 siblings, 1 reply; 33+ messages in thread From: Maarten Lankhorst @ 2018-02-09 9:54 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 2/5] drm/i915: Grab uncore.lock around enabling vblank evasion 2018-02-09 9:54 ` [PATCH 2/5] drm/i915: Grab uncore.lock around enabling " Maarten Lankhorst @ 2018-02-12 15:16 ` Chris Wilson 0 siblings, 0 replies; 33+ messages in thread From: Chris Wilson @ 2018-02-12 15:16 UTC (permalink / raw) To: Maarten Lankhorst, intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held. 2018-02-09 9:53 [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion Maarten Lankhorst 2018-02-09 9:54 ` [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion Maarten Lankhorst 2018-02-09 9:54 ` [PATCH 2/5] drm/i915: Grab uncore.lock around enabling " Maarten Lankhorst @ 2018-02-09 9:54 ` Maarten Lankhorst 2018-02-09 23:08 ` James Ausmus 2018-02-12 15:19 ` Chris Wilson 2018-02-09 9:54 ` [PATCH 4/5] drm/i915: Move all locking for plane updates to caller Maarten Lankhorst ` (5 subsequent siblings) 8 siblings, 2 replies; 33+ messages in thread From: Maarten Lankhorst @ 2018-02-09 9:54 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held. 2018-02-09 9:54 ` [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held Maarten Lankhorst @ 2018-02-09 23:08 ` James Ausmus 2018-02-10 8:46 ` Maarten Lankhorst 2018-02-12 15:19 ` Chris Wilson 1 sibling, 1 reply; 33+ messages in thread From: James Ausmus @ 2018-02-09 23:08 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held. 2018-02-09 23:08 ` James Ausmus @ 2018-02-10 8:46 ` Maarten Lankhorst 2018-02-10 9:05 ` Chris Wilson 0 siblings, 1 reply; 33+ messages in thread From: Maarten Lankhorst @ 2018-02-10 8:46 UTC (permalink / raw) To: James Ausmus; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held. 2018-02-10 8:46 ` Maarten Lankhorst @ 2018-02-10 9:05 ` Chris Wilson 0 siblings, 0 replies; 33+ messages in thread From: Chris Wilson @ 2018-02-10 9:05 UTC (permalink / raw) To: Maarten Lankhorst, James Ausmus; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held. 2018-02-09 9:54 ` [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held Maarten Lankhorst 2018-02-09 23:08 ` James Ausmus @ 2018-02-12 15:19 ` Chris Wilson 2018-02-12 15:39 ` Maarten Lankhorst 1 sibling, 1 reply; 33+ messages in thread From: Chris Wilson @ 2018-02-12 15:19 UTC (permalink / raw) To: Maarten Lankhorst, intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held. 2018-02-12 15:19 ` Chris Wilson @ 2018-02-12 15:39 ` Maarten Lankhorst 2018-02-12 15:44 ` Chris Wilson 0 siblings, 1 reply; 33+ messages in thread From: Maarten Lankhorst @ 2018-02-12 15:39 UTC (permalink / raw) To: Chris Wilson, intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held. 2018-02-12 15:39 ` Maarten Lankhorst @ 2018-02-12 15:44 ` Chris Wilson 2018-02-12 16:41 ` Maarten Lankhorst 0 siblings, 1 reply; 33+ messages in thread From: Chris Wilson @ 2018-02-12 15:44 UTC (permalink / raw) To: Maarten Lankhorst, intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held. 2018-02-12 15:44 ` Chris Wilson @ 2018-02-12 16:41 ` Maarten Lankhorst 0 siblings, 0 replies; 33+ messages in thread From: Maarten Lankhorst @ 2018-02-12 16:41 UTC (permalink / raw) To: Chris Wilson, intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 4/5] drm/i915: Move all locking for plane updates to caller 2018-02-09 9:53 [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion Maarten Lankhorst ` (2 preceding siblings ...) 2018-02-09 9:54 ` [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held Maarten Lankhorst @ 2018-02-09 9:54 ` Maarten Lankhorst 2018-02-09 9:54 ` [PATCH 5/5] drm/i915: Use DOUBLE_BUFFER_CTL on top of vblank evasion for GEN9+ Maarten Lankhorst ` (4 subsequent siblings) 8 siblings, 0 replies; 33+ messages in thread From: Maarten Lankhorst @ 2018-02-09 9:54 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 5/5] drm/i915: Use DOUBLE_BUFFER_CTL on top of vblank evasion for GEN9+. 2018-02-09 9:53 [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion Maarten Lankhorst ` (3 preceding siblings ...) 2018-02-09 9:54 ` [PATCH 4/5] drm/i915: Move all locking for plane updates to caller Maarten Lankhorst @ 2018-02-09 9:54 ` Maarten Lankhorst 2018-02-09 10:04 ` [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion Chris Wilson ` (3 subsequent siblings) 8 siblings, 0 replies; 33+ messages in thread From: Maarten Lankhorst @ 2018-02-09 9:54 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion. 2018-02-09 9:53 [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion Maarten Lankhorst ` (4 preceding siblings ...) 2018-02-09 9:54 ` [PATCH 5/5] drm/i915: Use DOUBLE_BUFFER_CTL on top of vblank evasion for GEN9+ Maarten Lankhorst @ 2018-02-09 10:04 ` Chris Wilson 2018-02-09 17:21 ` Maarten Lankhorst 2018-02-09 14:24 ` ✗ Fi.CI.BAT: failure for " Patchwork ` (2 subsequent siblings) 8 siblings, 1 reply; 33+ messages in thread From: Chris Wilson @ 2018-02-09 10:04 UTC (permalink / raw) To: Maarten Lankhorst, intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion. 2018-02-09 10:04 ` [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion Chris Wilson @ 2018-02-09 17:21 ` Maarten Lankhorst 2018-02-12 17:01 ` Ville Syrjälä 0 siblings, 1 reply; 33+ messages in thread From: Maarten Lankhorst @ 2018-02-09 17:21 UTC (permalink / raw) To: Chris Wilson, intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion. 2018-02-09 17:21 ` Maarten Lankhorst @ 2018-02-12 17:01 ` Ville Syrjälä 2018-02-13 10:19 ` Maarten Lankhorst 0 siblings, 1 reply; 33+ messages in thread From: Ville Syrjälä @ 2018-02-12 17:01 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion. 2018-02-12 17:01 ` Ville Syrjälä @ 2018-02-13 10:19 ` Maarten Lankhorst 2018-02-13 10:40 ` Chris Wilson 0 siblings, 1 reply; 33+ messages in thread From: Maarten Lankhorst @ 2018-02-13 10:19 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion. 2018-02-13 10:19 ` Maarten Lankhorst @ 2018-02-13 10:40 ` Chris Wilson 0 siblings, 0 replies; 33+ messages in thread From: Chris Wilson @ 2018-02-13 10:40 UTC (permalink / raw) To: Maarten Lankhorst, Ville Syrjälä; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: Grab the vblank evasion lock around the entire evasion. 2018-02-09 9:53 [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion Maarten Lankhorst ` (5 preceding siblings ...) 2018-02-09 10:04 ` [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion Chris Wilson @ 2018-02-09 14:24 ` Patchwork 2018-02-12 15:02 ` ✓ Fi.CI.BAT: success " Patchwork 2018-02-12 16:56 ` ✗ Fi.CI.IGT: warning " Patchwork 8 siblings, 0 replies; 33+ messages in thread From: Patchwork @ 2018-02-09 14:24 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx == 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Grab the vblank evasion lock around the entire evasion. 2018-02-09 9:53 [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion Maarten Lankhorst ` (6 preceding siblings ...) 2018-02-09 14:24 ` ✗ Fi.CI.BAT: failure for " Patchwork @ 2018-02-12 15:02 ` Patchwork 2018-02-12 16:56 ` ✗ Fi.CI.IGT: warning " Patchwork 8 siblings, 0 replies; 33+ messages in thread From: Patchwork @ 2018-02-12 15:02 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx == 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* ✗ Fi.CI.IGT: warning for drm/i915: Grab the vblank evasion lock around the entire evasion. 2018-02-09 9:53 [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion Maarten Lankhorst ` (7 preceding siblings ...) 2018-02-12 15:02 ` ✓ Fi.CI.BAT: success " Patchwork @ 2018-02-12 16:56 ` Patchwork 8 siblings, 0 replies; 33+ messages in thread From: Patchwork @ 2018-02-12 16:56 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx == 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2018-02-13 10:40 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-09 9:53 [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion Maarten Lankhorst 2018-02-09 9:54 ` [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion Maarten Lankhorst 2018-02-12 15:10 ` Chris Wilson 2018-02-12 15:16 ` Maarten Lankhorst 2018-02-12 15:22 ` Chris Wilson 2018-02-12 15:27 ` Maarten Lankhorst 2018-02-12 15:31 ` Chris Wilson 2018-02-12 15:41 ` Maarten Lankhorst 2018-02-12 16:55 ` Ville Syrjälä 2018-02-12 17:24 ` Chris Wilson 2018-02-12 18:06 ` Ville Syrjälä 2018-02-12 20:55 ` Chris Wilson 2018-02-13 8:59 ` Maarten Lankhorst 2018-02-09 9:54 ` [PATCH 2/5] drm/i915: Grab uncore.lock around enabling " Maarten Lankhorst 2018-02-12 15:16 ` Chris Wilson 2018-02-09 9:54 ` [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held Maarten Lankhorst 2018-02-09 23:08 ` James Ausmus 2018-02-10 8:46 ` Maarten Lankhorst 2018-02-10 9:05 ` Chris Wilson 2018-02-12 15:19 ` Chris Wilson 2018-02-12 15:39 ` Maarten Lankhorst 2018-02-12 15:44 ` Chris Wilson 2018-02-12 16:41 ` Maarten Lankhorst 2018-02-09 9:54 ` [PATCH 4/5] drm/i915: Move all locking for plane updates to caller Maarten Lankhorst 2018-02-09 9:54 ` [PATCH 5/5] drm/i915: Use DOUBLE_BUFFER_CTL on top of vblank evasion for GEN9+ Maarten Lankhorst 2018-02-09 10:04 ` [PATCH 0/5] drm/i915: Grab the vblank evasion lock around the entire evasion Chris Wilson 2018-02-09 17:21 ` Maarten Lankhorst 2018-02-12 17:01 ` Ville Syrjälä 2018-02-13 10:19 ` Maarten Lankhorst 2018-02-13 10:40 ` Chris Wilson 2018-02-09 14:24 ` ✗ Fi.CI.BAT: failure for " Patchwork 2018-02-12 15:02 ` ✓ Fi.CI.BAT: success " Patchwork 2018-02-12 16:56 ` ✗ Fi.CI.IGT: warning " Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.