Re: [PATCH v5 1/5] drm/i915: Add enable/disable flip done and flip done handler

From: Daniel Vetter <daniel@ffwll.ch>
To: "Michel Dänzer" <michel@daenzer.net>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	Karthik B S <karthik.b.s@intel.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Vandita Kulkarni <vandita.kulkarni@intel.com>,
	Uma Shankar <uma.shankar@intel.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	"Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com>
Subject: Re: [PATCH v5 1/5] drm/i915: Add enable/disable flip done and flip done handler
Date: Mon, 27 Jul 2020 23:34:34 +0200	[thread overview]
Message-ID: <CAKMK7uFzvuT81J38Y_4NnZx3xQhJ4Ha4dVyDy+pwCRF8gbuRVw@mail.gmail.com> (raw)
In-Reply-To: <f00f637e-639e-5d12-83bd-274ad0a23abe@daenzer.net>

On Mon, Jul 27, 2020 at 2:27 PM Michel Dänzer <michel@daenzer.net> wrote:
>
> On 2020-07-25 1:26 a.m., Paulo Zanoni wrote:
> > Em seg, 2020-07-20 às 17:01 +0530, Karthik B S escreveu:
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> index 1fa67700d8f4..95953b393941 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -697,14 +697,24 @@ u32 i915_get_vblank_counter(struct drm_crtc *crtc)
> >>      return (((high1 << 8) | low) + (pixel >= vbl_start)) & 0xffffff;
> >>  }
> >>
> >> +static u32 g4x_get_flip_counter(struct drm_crtc *crtc)
> >> +{
> >> +    struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> >> +    enum pipe pipe = to_intel_crtc(crtc)->pipe;
> >> +
> >> +    return I915_READ(PIPE_FLIPCOUNT_G4X(pipe));
> >> +}
> >> +
> >>  u32 g4x_get_vblank_counter(struct drm_crtc *crtc)
> >>  {
> >>      struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> >>      enum pipe pipe = to_intel_crtc(crtc)->pipe;
> >>
> >> +    if (crtc->state->async_flip)
> >> +            return g4x_get_flip_counter(crtc);
> >> +
> >>      return I915_READ(PIPE_FRMCOUNT_G4X(pipe));
> >
> > I don't understand the intention behind this, can you please clarify?
> > This goes back to my reply of the cover letter. It seems that here
> > we're going to alternate between two different counters in our vblank
> > count. So if user space alternates between sometimes using async flips
> > and sometimes using normal flip it's going to get some very weird
> > deltas, isn't it? At least this is what I remember from when I played
> > with these registers: FLIPCOUNT drifts away from FRMCOUNT when we start
> > using async flips.
>
> This definitely looks wrong. The counter value returned by the
> get_vblank_counter hook is supposed to increment when a vertical blank
> period occurs; page flips are not supposed to affect this in any way.

Also you just flat out can't access crtc->state from interrupt
context. Anything you need in there needs to be protected by the right
irq-type spin_lock, updates correctly synchronized against both the
interrupt handler and atomic updates, and data copied over, not
pointers. Otherwise just crash&burn.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel