| Message ID | 20200720113117.16131-1-karthik.b.s@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Asynchronous flip implementation for i915 | expand |
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. 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(-) >
> -----Original Message----- > From: Zanoni, Paulo R <paulo.r.zanoni@intel.com> > Sent: Saturday, July 25, 2020 4:56 AM > To: B S, Karthik <karthik.b.s@intel.com>; intel-gfx@lists.freedesktop.org > Cc: ville.syrjala@linux.intel.com; Vetter, Daniel <daniel.vetter@intel.com>; > Kulkarni, Vandita <vandita.kulkarni@intel.com>; > nicholas.kazlauskas@amd.com; harry.wentland@amd.com; Shankar, Uma > <uma.shankar@intel.com>; dri-devel@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(-) > >
On 2020-07-29 9:23 a.m., Kulkarni, Vandita wrote: > > 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). There's no such thing as a "flip sequence" in the KMS API. There's only the per-CRTC vblank counter. Each flip completion event needs to contain the value of that counter when the hardware completed the flip, regardless of whether it was an async flip or not. As for the timestamp in the event, I'm not sure what the expectations are for async flips, but I suspect it may not really matter. E.g. the timestamp calculated to correspond to the end of the previous vertical blank period might be fine.
> -----Original Message----- > From: Michel Dänzer <michel@daenzer.net> > Sent: Wednesday, July 29, 2020 1:04 PM > To: Kulkarni, Vandita <vandita.kulkarni@intel.com>; Zanoni, Paulo R > <paulo.r.zanoni@intel.com>; Vetter, Daniel <daniel.vetter@intel.com>; B S, > Karthik <karthik.b.s@intel.com>; intel-gfx@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org; Shankar, Uma > <uma.shankar@intel.com>; nicholas.kazlauskas@amd.com > Subject: Re: [PATCH v5 0/5] Asynchronous flip implementation for i915 > > On 2020-07-29 9:23 a.m., Kulkarni, Vandita wrote: > > > > 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). > > There's no such thing as a "flip sequence" in the KMS API. There's only the > per-CRTC vblank counter. Each flip completion event needs to contain the > value of that counter when the hardware completed the flip, regardless of > whether it was an async flip or not. > > As for the timestamp in the event, I'm not sure what the expectations are for > async flips, but I suspect it may not really matter. E.g. the timestamp > calculated to correspond to the end of the previous vertical blank period > might be fine. Thanks Michel, Paulo, Daniel, Nicholas, Ville for your inputs. After all the discussions, looks like the async flip time stamp is not of much use to the userspace and the async flip sequence; hence we will stick to the approach of sending vblank time stamp itself and have a test case in the igt to cover the async flips cases in a slightly different way. And update the documentation. Thanks, Vandita > > > -- > Earthling Michel Dänzer | https://redhat.com > Libre software enthusiast | Mesa and X developer
On 8/4/2020 11:19 AM, Kulkarni, Vandita wrote: >> -----Original Message----- >> From: Michel Dänzer <michel@daenzer.net> >> Sent: Wednesday, July 29, 2020 1:04 PM >> To: Kulkarni, Vandita <vandita.kulkarni@intel.com>; Zanoni, Paulo R >> <paulo.r.zanoni@intel.com>; Vetter, Daniel <daniel.vetter@intel.com>; B S, >> Karthik <karthik.b.s@intel.com>; intel-gfx@lists.freedesktop.org >> Cc: dri-devel@lists.freedesktop.org; Shankar, Uma >> <uma.shankar@intel.com>; nicholas.kazlauskas@amd.com >> Subject: Re: [PATCH v5 0/5] Asynchronous flip implementation for i915 >> >> On 2020-07-29 9:23 a.m., Kulkarni, Vandita wrote: >>> >>> 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). >> >> There's no such thing as a "flip sequence" in the KMS API. There's only the >> per-CRTC vblank counter. Each flip completion event needs to contain the >> value of that counter when the hardware completed the flip, regardless of >> whether it was an async flip or not. >> >> As for the timestamp in the event, I'm not sure what the expectations are for >> async flips, but I suspect it may not really matter. E.g. the timestamp >> calculated to correspond to the end of the previous vertical blank period >> might be fine. > > Thanks Michel, Paulo, Daniel, Nicholas, Ville for your inputs. > After all the discussions, looks like the async flip time stamp is not of much > use to the userspace and the async flip sequence; hence we will stick to the approach of sending vblank time stamp > itself and have a test case in the igt to cover the async flips cases in a slightly different way. > And update the documentation. > Thanks a lot for all the inputs. I will make changes in IGT to calculate the time stamps from userspace itself, as we have now concluded that the kernel will be returning vbl timestamps even in the case of async flips. Also, as suggested by Daniel, I will be adding one more subtest to verify that the async flip time stamp lies in between the timestamps of the previous and the next vblank. Thanks, Karthik.B.S > Thanks, > Vandita >> >> >> -- >> Earthling Michel Dänzer | https://redhat.com >> Libre software enthusiast | Mesa and X developer
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. 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(-)