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 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