Re: [Intel-gfx] [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.
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 +, 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 >>> --- >>> 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
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.
Quoting Ville Syrjälä (2018-02-12 18:06:58) > On Mon, Feb 12, 2018 at 05:24:54PM +, 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 > > > > >> > > > > >> --- > > > > >> 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 sc
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.
On Mon, Feb 12, 2018 at 05:24:54PM +, 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
> > > >>
> > > >> ---
> > > >> 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.
*/
con
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.
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 > > >> --- > > >> drivers/gpu/drm/i915/intel_sprite.c | 11 --- > > >> 1 file changed, 4 insertions(+), 7 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > >> b/drivers/gpu/drm/i915/intel_sprite.c > > >> index 3be22c0fcfb5..971a1ea0db45 100644 > > >> --- a/drivers/gpu/drm/i915/intel_sprite.c > > >> +++ b/drivers/gpu/drm/i915/intel_sprite.c > > >> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct > > >> intel_crtc_state *new_crtc_state) > > >> > > >> local_irq_disable(); > > >> > > >> - if (min <= 0 || max <= 0) > > >> + if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) > > >> return; > > >> > > >> - if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) > > >> + if (min <= 0 || max <= 0) > > >> return; > > >> > > > The corresponding vblank_put is the one later in update_start(), > > > right? > > > I don't think you intended to keep this chunk. > > > -Chris > > I'm not sure what you mean? The vblank_put is now in pipe_update_end, > > except if the > > event takes over the reference. I think the code is correct. :) > > >>> Then it's unbalanced in the case of error still. > > >>> -Chris > > >> It already would have been for events, hence the WARN_ON there. > > >> I don't think we can do anything about it, this shouldn't ever > > >> happen in practice, could be a BUG_ON for all I care. :) > > > I would much prefer that over intentionally bad code. > > > > > > But do we really need to enable the vblank irq here? If the event > > > requires it, doesn't it already enable the vblank. Here, we only need it > > > when sleeping, can we not determine we have enough time before the > > > vblank without enabling the interrupt? > > I'm not sure why we get a reference to the vblank counter here. Perhaps > > Ville does? > > We need the vblank irq to be enabled before we check the scanline since > otherwise we may end up doing: > > 1. check scanline > 3. vblank irq fires > 2. enable vblank irq > 3. wait for the next vblank > > So we'd end up wasting an entire frame. Step: 2.5, check_scanline? Something like, diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 574bd02c5a2e..70c2ee1c7b8c 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -97,6 +97,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state) bool need_vlv_dsi_wa = (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) && intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI); DEFINE_WAIT(wait); + bool have_vblank_irq = false; vblank_start = adjusted_mode->crtc_vblank_start; if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) @@ -112,9 +113,6 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state) if (min <= 0 || max <= 0) return; - if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) - return; - crtc->debug.min_vbl = min; crtc->debug.max_vbl = max; trace_i915_pipe_update_start(crtc); @@ -127,6 +125,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state) */ prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE); + if (!have_vblank_irq) + have_vblank_irq = !drm_crtc_vblank_get(&crtc->base); + scanline = intel_get_crtc_scanline(crtc); if (scanline < min || scanline > max) break; @@ -145,8 +147,8 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state) } finish_wait(wq, &wait); - - drm_crtc_vblank_put(&crtc->base); + if (have_vblank_irq) + drm_crtc_vblank_put(&crtc->base); /* * On VLV/CHV DSI the scanline counter would appear to ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.
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 > >> --- > >> drivers/gpu/drm/i915/intel_sprite.c | 11 --- > >> 1 file changed, 4 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c > >> b/drivers/gpu/drm/i915/intel_sprite.c > >> index 3be22c0fcfb5..971a1ea0db45 100644 > >> --- a/drivers/gpu/drm/i915/intel_sprite.c > >> +++ b/drivers/gpu/drm/i915/intel_sprite.c > >> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct > >> intel_crtc_state *new_crtc_state) > >> > >> local_irq_disable(); > >> > >> - if (min <= 0 || max <= 0) > >> + if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) > >> return; > >> > >> - if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) > >> + if (min <= 0 || max <= 0) > >> return; > >> > > The corresponding vblank_put is the one later in update_start(), right? > > I don't think you intended to keep this chunk. > > -Chris > I'm not sure what you mean? The vblank_put is now in pipe_update_end, > except if the > event takes over the reference. I think the code is correct. :) > >>> Then it's unbalanced in the case of error still. > >>> -Chris > >> It already would have been for events, hence the WARN_ON there. > >> I don't think we can do anything about it, this shouldn't ever > >> happen in practice, could be a BUG_ON for all I care. :) > > I would much prefer that over intentionally bad code. > > > > But do we really need to enable the vblank irq here? If the event > > requires it, doesn't it already enable the vblank. Here, we only need it > > when sleeping, can we not determine we have enough time before the > > vblank without enabling the interrupt? > I'm not sure why we get a reference to the vblank counter here. Perhaps Ville > does? We need the vblank irq to be enabled before we check the scanline since otherwise we may end up doing: 1. check scanline 3. vblank irq fires 2. enable vblank irq 3. wait for the next vblank So we'd end up wasting an entire frame. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.
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 >> --- >> drivers/gpu/drm/i915/intel_sprite.c | 11 --- >> 1 file changed, 4 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c >> b/drivers/gpu/drm/i915/intel_sprite.c >> index 3be22c0fcfb5..971a1ea0db45 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct >> intel_crtc_state *new_crtc_state) >> >> local_irq_disable(); >> >> - if (min <= 0 || max <= 0) >> + if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) >> return; >> >> - if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) >> + if (min <= 0 || max <= 0) >> return; >> > The corresponding vblank_put is the one later in update_start(), right? > I don't think you intended to keep this chunk. > -Chris I'm not sure what you mean? The vblank_put is now in pipe_update_end, except if the event takes over the reference. I think the code is correct. :) >>> Then it's unbalanced in the case of error still. >>> -Chris >> It already would have been for events, hence the WARN_ON there. >> I don't think we can do anything about it, this shouldn't ever >> happen in practice, could be a BUG_ON for all I care. :) > I would much prefer that over intentionally bad code. > > But do we really need to enable the vblank irq here? If the event > requires it, doesn't it already enable the vblank. Here, we only need it > when sleeping, can we not determine we have enough time before the > vblank without enabling the interrupt? I'm not sure why we get a reference to the vblank counter here. Perhaps Ville does? ~Maarten ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.
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 > --- > drivers/gpu/drm/i915/intel_sprite.c | 11 --- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > b/drivers/gpu/drm/i915/intel_sprite.c > index 3be22c0fcfb5..971a1ea0db45 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct > intel_crtc_state *new_crtc_state) > > local_irq_disable(); > > - if (min <= 0 || max <= 0) > + if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) > return; > > - if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) > + if (min <= 0 || max <= 0) > return; > > >>> The corresponding vblank_put is the one later in update_start(), right? > >>> I don't think you intended to keep this chunk. > >>> -Chris > >> I'm not sure what you mean? The vblank_put is now in pipe_update_end, > >> except if the > >> event takes over the reference. I think the code is correct. :) > > Then it's unbalanced in the case of error still. > > -Chris > > It already would have been for events, hence the WARN_ON there. > I don't think we can do anything about it, this shouldn't ever > happen in practice, could be a BUG_ON for all I care. :) I would much prefer that over intentionally bad code. But do we really need to enable the vblank irq here? If the event requires it, doesn't it already enable the vblank. Here, we only need it when sleeping, can we not determine we have enough time before the vblank without enabling the interrupt? -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.
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 --- drivers/gpu/drm/i915/intel_sprite.c | 11 --- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 3be22c0fcfb5..971a1ea0db45 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state) local_irq_disable(); - if (min <= 0 || max <= 0) + if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) return; - if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) + if (min <= 0 || max <= 0) return; >>> The corresponding vblank_put is the one later in update_start(), right? >>> I don't think you intended to keep this chunk. >>> -Chris >> I'm not sure what you mean? The vblank_put is now in pipe_update_end, except >> if the >> event takes over the reference. I think the code is correct. :) > Then it's unbalanced in the case of error still. > -Chris It already would have been for events, hence the WARN_ON there. I don't think we can do anything about it, this shouldn't ever happen in practice, could be a BUG_ON for all I care. :) ~Maarten ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.
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 > >> --- > >> drivers/gpu/drm/i915/intel_sprite.c | 11 --- > >> 1 file changed, 4 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c > >> b/drivers/gpu/drm/i915/intel_sprite.c > >> index 3be22c0fcfb5..971a1ea0db45 100644 > >> --- a/drivers/gpu/drm/i915/intel_sprite.c > >> +++ b/drivers/gpu/drm/i915/intel_sprite.c > >> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct > >> intel_crtc_state *new_crtc_state) > >> > >> local_irq_disable(); > >> > >> - if (min <= 0 || max <= 0) > >> + if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) > >> return; > >> > >> - if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) > >> + if (min <= 0 || max <= 0) > >> return; > >> > > The corresponding vblank_put is the one later in update_start(), right? > > I don't think you intended to keep this chunk. > > -Chris > > I'm not sure what you mean? The vblank_put is now in pipe_update_end, except > if the > event takes over the reference. I think the code is correct. :) Then it's unbalanced in the case of error still. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.
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 >> --- >> drivers/gpu/drm/i915/intel_sprite.c | 11 --- >> 1 file changed, 4 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c >> b/drivers/gpu/drm/i915/intel_sprite.c >> index 3be22c0fcfb5..971a1ea0db45 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct >> intel_crtc_state *new_crtc_state) >> >> local_irq_disable(); >> >> - if (min <= 0 || max <= 0) >> + if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) >> return; >> >> - if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) >> + if (min <= 0 || max <= 0) >> return; >> > The corresponding vblank_put is the one later in update_start(), right? > I don't think you intended to keep this chunk. > -Chris I'm not sure what you mean? The vblank_put is now in pipe_update_end, except if the event takes over the reference. I think the code is correct. :) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Keep vblank irq enabled during vblank evasion.
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 > --- > 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
