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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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