PRIME Synchronization
Subsystems (2)
v7 cover PRIME Synchronization
Hello all,
For a while now, I've been working to fix tearing with PRIME. This is the same as the sixth version of the DRM component for PRIME synchronization, but with my mail headers fixed so that the patches can be checked in.
In sixth version I modified the wait in prepare_plane_fb to properly handle interrupts by returning -ERESTARTSYS, and warn on other error codes before continuing.
Due to the inability to properly handle interrupts in mmio_flip_work_func, I changed it back to an uninterruptible wait, matching the semantics used by __i915_wait_request(). Like prepare_plane_fb, I also changed it to warn on error codes.
Repeat of overview below:
v1 was a more complicated patch set that added an additional fenced interface to page flipping. To avoid adding additional interfaces on top of a legacy path, v2 scrapped those patches and changed i915 page flipping paths to wait on fences attached to DMA-BUF-backed fbs. Subsequent versions involve incremental changes outlined in the patch descriptions.
I have two patches, one that implements fencing for i915's legacy mmio_flip path, and one for atomic modesetting for futureproofing. Currently the mmio_flip path is the one ultimately used by the X patches, due to the lack of asynchronous atomic modesetting support in i915.
With my synchronization patches to X, it is possible to export two shared buffers per crtc instead of just one. The sink driver uses the legacy drmModePageFlip() to flip between the buffers, as the rest of the driver has yet to be ported to atomics. In the pageflip/vblank event handler, the sink driver requests a present from the source using the new X ABI function pScreen->PresentTrackedFlippingPixmap(). If the call returns successfully, it uses drmModePageFlip() to flip to the updated buffer, otherwise it waits until the next vblank and tries again.
When the source driver presents on a given buffer, it first attaches a fence. The source driver is responsible for either using software signaling or hardware semaphore-backed fences to ensure the fence is signaled when the present is finished. If the sink's DRM driver implements fencing in the flipping path, it will guarantee that that flip won't occur until the present has finished.
This means that DRM drivers that don't implement fencing in their flipping paths won't be able to guarantee 100% tear-free PRIME with my X patches. However, the good news is that even without fencing, tearing is rare. Generally presenting finishes before the next vblank, so there is no need to wait on the fence. The X patches are a drastic improvement with or without fencing, but the fencing is nonetheless important to guarantee tear-free under all conditions.
To give some greater context, I've uploaded my branches for DRM and the X server to Github. I'll move forward with upstreaming the X changes if and when these DRM patches go in.
DRM Tree: https://github.com/GoinsWithTheWind/drm-prime-sync X Tree: https://github.com/GoinsWithTheWind/xserver-prime-sync
(branch agoins-prime-v6)
Thanks, Alex @ NVIDIA Linux Driver Team
Alex Goins (2):
i915: wait for fence in mmio_flip_work_func
i915: wait for fence in prepare_plane_fb
drivers/gpu/drm/i915/intel_display.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v7 1/2 i915: wait for fence in mmio_flip_work_func
If a buffer is backed by dmabuf, wait on its reservation object's exclusive fence before flipping.
v2: First commit
v3: Remove object_name_lock acquire
v4: Move wait ahead of mark_page_flip_active
Use crtc->primary->fb to get GEM object instead of pending_flip_obj use_mmio_flip() return true when exclusive fence is attached Wait only on exclusive fences, interruptible with no timeout
v5: Move wait from do_mmio_flip to mmio_flip_work_func
Style tweaks to more closely match rest of file
v6: Change back to unintteruptible wait to match __i915_wait_request due to
inability to properly handle interrupted wait.
Warn on error code from waiting.
--- drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
drivers/gpu/drm/i915/intel_display.c +13 -0
@@ -44,6 +44,8 @@#include <drm/drm_plane_helper.h> #include <drm/drm_rect.h> #include <linux/dma_remapping.h> +#include <linux/reservation.h> +#include <linux/dma-buf.h> /* Primary plane formats for gen <= 3 */ static const uint32_t i8xx_primary_formats[] = {
@@ -11088,6 +11090,8 @@ static bool use_mmio_flip(struct intel_engine_cs *ring,return true; else if (i915.enable_execlists) return true; + else if (obj->base.dma_buf && obj->base.dma_buf->resv->fence_excl) + return true; else return ring != i915_gem_request_get_ring(obj->last_write_req); }
@@ -11189,6 +11193,9 @@ static void intel_mmio_flip_work_func(struct work_struct *work) {struct intel_mmio_flip *mmio_flip = container_of(work, struct intel_mmio_flip, work); +struct intel_framebuffer *intel_fb = +to_intel_framebuffer (mmio_flip->crtc->base.primary->fb); +struct drm_i915_gem_object *obj = intel_fb->obj;if (mmio_flip->req) WARN_ON(__i915_wait_request(mmio_flip->req,
@@ -11196,6 +11203,12 @@ static void intel_mmio_flip_work_func(struct work_struct *work) false,NULL , &mmio_flip->i915->rps.mmioflips)); +/* For framebuffer backed by dmabuf, wait for fence */ +if (obj->base.dma_buf) +WARN_ON (reservation_object_wait_timeout_rcu (obj->base.dma_buf->resv, + false, false, +MAX_SCHEDULE_TIMEOUT ) <0 ); +intel_do_mmio_flip (mmio_flip->crtc);i915_gem_request_unreference__unlocked (mmio_flip->req);
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH i915 v7 1/2] i915: wait for fence in mmio_flip_work_func
Hi Alex,
Drive-by review because I was just reviewing something similar for a different device...
Quoted 34 lines · Alex Goins
If a buffer is backed by dmabuf, wait on its reservation object's exclusive fence before flipping.
v2: First commit
v3: Removeobject_name_lockacquire
v4: Move wait ahead ofmark_page_flip_active
Usecrtc->primary->fbto get GEM object instead ofpending_flip_objuse_mmio_flip()return true when exclusive fence is attached Wait only on exclusive fences, interruptible with no timeout
v5: Move wait fromdo_mmio_fliptommio_flip_work_func
Style tweaks to more closely match rest of file
v6: Change back to unintteruptible wait to match__i915_wait_requestdue to
inability to properly handle interrupted wait.
Warn on error code from waiting.Signed-off-by Alex Goins <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)drivers/gpu/drm/i915/intel_display.c +2
-0@@ -44,6 +44,8 @@#include <drm/drm_plane_helper.h> #include <drm/drm_rect.h> #include <linux/dma_remapping.h> +#include <linux/reservation.h> +#include <linux/dma-buf.h> /* Primary plane formats for gen <= 3 */ static const uint32_t i8xx_primary_formats[] = {
drivers/gpu/drm/i915/intel_display.c static bool use_mmio_flip(struct intel_engine_cs *ring, · line 11094
Quoted 5 lines
drivers/gpu/drm/i915/intel_display.c +2
-0@@ -11088,6 +11090,8 @@ static bool use_mmio_flip(struct intel_engine_cs *ring,return true; else if (i915.enable_execlists) return true; + else if (obj->base.dma_buf && obj->base.dma_buf->resv->fence_excl) + return true;
I'm not sure if this is really doing exactly what you want. When a reservation object's exclusive fence has signaled, I think the old pointer in fence_excl is not actually cleared; it keeps pointing to the old exclusive fence until that replaced by another.
So, just nake null-check here is like saying "did this reservation object ever have an exclusive fence at some point in the past", which is not necessarily the same as "is there an exclusive fence associated with the buffer that I am about to flip"?
The reservation object is a complicated rcu & ww_mutex protected beast, so I would be shy to access any of its fields directly.
Quoted 14 lines
drivers/gpu/drm/i915/intel_display.c +3
-0else return ring !=i915_gem_request_get_ring (obj->last_write_req); }@@ -11189,6 +11193,9 @@ static void intel_mmio_flip_work_func(struct work_struct *work) {struct intel_mmio_flip *mmio_flip = container_of(work, struct intel_mmio_flip, work); +struct intel_framebuffer *intel_fb = +to_intel_framebuffer (mmio_flip->crtc->base.primary->fb); +struct drm_i915_gem_object *obj = intel_fb->obj;if (mmio_flip->req) WARN_ON(__i915_wait_request(mmio_flip->req,
drivers/gpu/drm/i915/intel_display.c static void intel_mmio_flip_work_func(struct work_struct *work) · line 11211
Quoted 9 lines
drivers/gpu/drm/i915/intel_display.c +6
-0@@ -11196,6 +11203,12 @@ static void intel_mmio_flip_work_func(struct work_struct *work) false,NULL , &mmio_flip->i915->rps.mmioflips)); +/* For framebuffer backed by dmabuf, wait for fence */ +if (obj->base.dma_buf) +WARN_ON (reservation_object_wait_timeout_rcu (obj->base.dma_buf->resv, + false, false, +MAX_SCHEDULE_TIMEOUT ) <0 ); +
Hmm, don't you want an interrupt-able wait here? And if so, you probably don't want to WARN_ON() -ERESTARTSYS.
-Dan
Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +0
-1intel_do_mmio_flip (mmio_flip->crtc);i915_gem_request_unreference__unlocked (mmio_flip->req); --1.9.1
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH i915 v7 1/2] i915: wait for fence in mmio_flip_work_func
Thanks, Daniel. There sure are a lot of Daniels.
Quoted 8 lines
drivers/gpu/drm/i915/intel_display.c static bool use_mmio_flip(struct intel_engine_cs *ring, · line 11094
Quoted 2 lines
drivers/gpu/drm/i915/intel_display.c +2
-0+else if (obj->base.dma_buf && obj->base.dma_buf->resv->fence_excl) +return true;I'm not sure if this is really doing exactly what you want. When a reservation object's exclusive fence has signaled, I think the old pointer in
fence_exclis not actually cleared; it keeps pointing to the old exclusive fence until that replaced by another.
Quoted 7 lines
So, just nake null-check here is like saying "did this reservation object ever have an exclusive fence at some point in the past", which is not necessarily the same as "is there an exclusive fence associated with the buffer that I am about to flip"?
The reservation object is a complicated rcu &
ww_mutexprotected beast, so I would be shy to access any of its fields directly.
Hm, well, the goal is to ensure that mmio_flip is used if we might need to wait on a fence, so if there are false positives it shouldn't be lethal.
Nonetheless, you're probably right. Maybe it would be better to use !reservation_object_test_signaled_rcu(test_all=FALSE), which should be TRUE iff there is an unsignaled exclusive fence attached, meaning we might have to wait.
Quoted 6 lines
Quoted 6 lines
if (mmio_flip->req) WARN_ON(__i915_wait_request(mmio_flip->req,drivers/gpu/drm/i915/intel_display.c +0
-0@@ -11196,6 +11203,12 @@ static void intel_mmio_flip_work_func(struct work_struct *work) false,NULL , &mmio_flip->i915->rps.mmioflips));
Quoted 9 lines
drivers/gpu/drm/i915/intel_display.c static void intel_mmio_flip_work_func(struct work_struct *work) · line 11211
Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c +6
-0+/* For framebuffer backed by dmabuf, wait for fence */ +if (obj->base.dma_buf) +WARN_ON (reservation_object_wait_timeout_rcu (obj->base.dma_buf->resv, + false, false, +MAX_SCHEDULE_TIMEOUT ) <0 ); +Hmm, don't you want an interrupt-able wait here? And if so, you probably don't want to
WARN_ON()-ERESTARTSYS.
I had this as an interruptable wait previously, but I'm not sure how we would actually handle an interrupt in that case. Notice also how __i915_wait_request(interruptable=FALSE) just above is also an uninterruptable wait.
Thanks,
Alex
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH i915 v7 1/2] i915: wait for fence in mmio_flip_work_func
Quoted 18 lines · Alex Goins
Thanks, Daniel. There sure are a lot of Daniels.
Quoted 8 lines
drivers/gpu/drm/i915/intel_display.c static bool use_mmio_flip(struct intel_engine_cs *ring, · line 11094
Quoted 2 lines
drivers/gpu/drm/i915/intel_display.c +2
-0+else if (obj->base.dma_buf && obj->base.dma_buf->resv->fence_excl) +return true;I'm not sure if this is really doing exactly what you want. When a reservation object's exclusive fence has signaled, I think the old pointer in
fence_exclis not actually cleared; it keeps pointing to the old exclusive fence until that replaced by another.Quoted 7 lines
So, just nake null-check here is like saying "did this reservation object ever have an exclusive fence at some point in the past", which is not necessarily the same as "is there an exclusive fence associated with the buffer that I am about to flip"?
The reservation object is a complicated rcu &
ww_mutexprotected beast, so I would be shy to access any of its fields directly.
Quoted 6 lines
Hm, well, the goal is to ensure that
mmio_flipis used if we might need to wait on a fence, so if there are false positives it shouldn't be lethal.Nonetheless, you're probably right. Maybe it would be better to use !reservation_object_test_signaled_rcu(test_all=FALSE), which should be TRUE iff there is an unsignaled exclusive fence attached, meaning we might have to wait.
Yeah I think that's a good idea for better encapsulation of reservation/fence internals.
-Daniel
Quoted 28 lines
Quoted 7 lines
Quoted 7 lines
if (mmio_flip->req) WARN_ON(__i915_wait_request(mmio_flip->req,drivers/gpu/drm/i915/intel_display.c +0
-0@@ -11196,6 +11203,12 @@ static void intel_mmio_flip_work_func(struct work_struct *work) false,NULL , &mmio_flip->i915->rps.mmioflips));Quoted 9 lines
drivers/gpu/drm/i915/intel_display.c static void intel_mmio_flip_work_func(struct work_struct *work) · line 11211
Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c +6
-0+/* For framebuffer backed by dmabuf, wait for fence */ +if (obj->base.dma_buf) +WARN_ON (reservation_object_wait_timeout_rcu (obj->base.dma_buf->resv, + false, false, +MAX_SCHEDULE_TIMEOUT ) <0 ); +Hmm, don't you want an interrupt-able wait here? And if so, you probably don't want to
WARN_ON()-ERESTARTSYS.I had this as an interruptable wait previously, but I'm not sure how we would actually handle an interrupt in that case. Notice also how
__i915_wait_request(interruptable=FALSE)just above is also an uninterruptable wait.Thanks,
Alex
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v7 2/2 i915: wait for fence in prepare_plane_fb
In reply to Alex GoinsIn intel_prepare_plane_fb, if fb is backed by dma-buf, wait for exclusive fence
v2: First commit
v3: Remove object_name_lock acquire
Move wait from intel_atomic_commit() to intel_prepare_plane_fb()
v4: Wait only on exclusive fences, interruptible with no timeout
v5: Style tweaks to more closely match rest of file
v6: Properly handle interrupted waits
--- drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
drivers/gpu/drm/i915/intel_display.c +11 -0
@@ -13347,6 +13347,17 @@ intel_prepare_plane_fb(struct drm_plane *plane,if (!obj)return 0 ; +/* For framebuffer backed by dmabuf, wait for fence */ +if (obj->base.dma_buf) { + ret =reservation_object_wait_timeout_rcu (obj->base.dma_buf->resv, + false, true, +MAX_SCHEDULE_TIMEOUT ); +if (ret == -ERESTARTSYS ) +return ret; + +WARN_ON (ret <0 ); + } +mutex_lock (&dev->struct_mutex); if (plane->type == DRM_PLANE_TYPE_CURSOR &&
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel