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

Chris Wilson
2018-02-12T15:31:56Z

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