[Intel-gfx] [PATCH v5 0/5] Asynchronous flip implementation for i915

[Kulkarni, Vandita]

> -----Original Message-----
> From: Zanoni, Paulo R <paulo.r.zanoni at intel.com>
> Sent: Saturday, July 25, 2020 4:56 AM
> To: B S, Karthik <karthik.b.s at intel.com>; intel-gfx at lists.freedesktop.org
> Cc: ville.syrjala at linux.intel.com; Vetter, Daniel <daniel.vetter at intel.com>;
> Kulkarni, Vandita <vandita.kulkarni at intel.com>;
> nicholas.kazlauskas at amd.com; harry.wentland at amd.com; Shankar, Uma
> <uma.shankar at intel.com>; dri-devel at lists.freedesktop.org
> Subject: Re: [PATCH v5 0/5] Asynchronous flip implementation for i915
> 
> Em seg, 2020-07-20 às 17:01 +0530, Karthik B S escreveu:
> > Without async flip support in the kernel, fullscreen apps where game
> > resolution is equal to the screen resolution, must perform an extra
> > blit per frame prior to flipping.
> >
> > Asynchronous page flips will also boost the FPS of Mesa benchmarks.
> 
> We had a discussion in patch 1 of v3 regarding the semantics of asynchronous
> flips from the point of view of the user space: how we handle our vblank
> counters, how/when we increment the sequence events and how we
> handle timestamps, how/when we deliver vblank events. Since apparently
> AMD has already enabled this feature, our job would be to implement their
> current behavior so KMS clients can continue to work regardless of the
> driver.
Thanks for your comments Paulo.

On V3 patch1, yes there were comments with this regard.
But seems like we did not coclude on few of the things. There were comments from Ville on how we could implement
the timestamping for async flips and that is part of this version.
Also we heard from Nicholas in their driver the time stamp is not mapping to the scan out as it happens immediately.

On async flips, there needs to be some clarity/guideline on the behaviour and event expectation from the
driver by user space.
Here are few assumptions that we have,
1. Our understanding is that the user space doesn’t expect the timestamp for async flips (but still expects vblank timestamp) , or
doesn’t do anything with that, same is the assumption wrt the flip sequence, please correct us if we are wrong.
2. In the sequence the user space still expects the counter that marks vblanks.
3. The user space can use different event types like DRM_EVENT_VBLANK or DRM_EVENT_FLIP_COMPLETE
for getting the corresponding event. And their designs are still aligned to this even in case of async.

If there are any more expectations from the user space wrt to the event that is being sent from the
driver in case of async flip, please let us know.

If the user space doesn’t care much about the flip sequence then, we can just not do anything like returning
the flip counter like this version is doing and just stick to returning of the frame counter value(which marks vblanks).

Based on these, we can tune the current implementation
which right now sends the flip time stamp in case of async flips.

Thanks,
Vandita
> 
> From reading this series it's not super clear to me what exactly is the
> behavior that we're trying to follow. Can you please document somewhere
> what are these rules and expectations? This way, people writing user space
> code (or people improving the other drivers) will have an easier time. In
> addition to text documentation, I believe all our assumptions and rules
> should be coded in IGT: we want to be confident a driver implements async
> page flips correctly when we can verify it passes the IGT.
> 
> Also, in the other patches I raise some additional questions regarding mixing
> async with non-async vblanks: IMHO this should also be documented as text
> and as IGT.
> 
> >
> > v2: -Few patches have been squashed and patches have been shuffled as
> >      per the reviews on the previous version.
> >
> > v3: -Few patches have been squashed and patches have been shuffled as
> >      per the reviews on the previous version.
> >
> > v4: -Made changes to fix the sequence and time stamp issue as per the
> >      comments received on the previous version.
> >     -Timestamps are calculated using the flip done time stamp and current
> >      timestamp. Here
> I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP flag is used
> >      for timestamp calculations.
> >     -Event is sent from the interrupt handler immediately using this
> >      updated timestamps and sequence.
> >     -Added more state checks as async flip should only allow change in plane
> >      surface address and nothing else should be allowed to change.
> >     -Added a separate plane hook for async flip.
> >     -Need to find a way to reject fbc enabling if it comes as part of this
> >      flip as bspec states that changes to FBC are not allowed.
> >
> > v5: -Fixed the Checkpatch and sparse warnings.
> >
> > Karthik B S (5):
> >   drm/i915: Add enable/disable flip done and flip done handler
> >   drm/i915: Add support for async flips in I915
> >   drm/i915: Add checks specific to async flips
> >   drm/i915: Do not call drm_crtc_arm_vblank_event in async flips
> >   drm/i915: Enable async flips in i915
> >
> >  drivers/gpu/drm/i915/display/intel_display.c | 123
> > +++++++++++++++++++  drivers/gpu/drm/i915/display/intel_sprite.c  |  33
> ++++-
> >  drivers/gpu/drm/i915/i915_irq.c              |  83 +++++++++++--
> >  drivers/gpu/drm/i915/i915_irq.h              |   2 +
> >  drivers/gpu/drm/i915/i915_reg.h              |   5 +-
> >  5 files changed, 237 insertions(+), 9 deletions(-)
> >