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@xxxxxxxxxxxxxxx> > > > > >>>>>> --- > > > > >>>>>> 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
