It's the default.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch --- drivers/gpu/drm/drm_writeback.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index 43d9e3bb3a94..dccf4504f1bb 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -108,7 +108,6 @@ static const struct dma_fence_ops drm_writeback_fence_ops = { .get_driver_name = drm_writeback_fence_get_driver_name, .get_timeline_name = drm_writeback_fence_get_timeline_name, .enable_signaling = drm_writeback_fence_enable_signaling, - .wait = dma_fence_default_wait, };
static int create_writeback_properties(struct drm_device *dev)
No need to micro-optmize when we're waiting in a mocked object ...
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Sumit Semwal sumit.semwal@linaro.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org --- drivers/dma-buf/st-dma-fence.c | 41 ---------------------------------- 1 file changed, 41 deletions(-)
diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c index e593064341c8..8166d2984702 100644 --- a/drivers/dma-buf/st-dma-fence.c +++ b/drivers/dma-buf/st-dma-fence.c @@ -33,50 +33,9 @@ static void mock_fence_release(struct dma_fence *f) kmem_cache_free(slab_fences, to_mock_fence(f)); }
-struct wait_cb { - struct dma_fence_cb cb; - struct task_struct *task; -}; - -static void mock_wakeup(struct dma_fence *f, struct dma_fence_cb *cb) -{ - wake_up_process(container_of(cb, struct wait_cb, cb)->task); -} - -static long mock_wait(struct dma_fence *f, bool intr, long timeout) -{ - const int state = intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE; - struct wait_cb cb = { .task = current }; - - if (dma_fence_add_callback(f, &cb.cb, mock_wakeup)) - return timeout; - - while (timeout) { - set_current_state(state); - - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &f->flags)) - break; - - if (signal_pending_state(state, current)) - break; - - timeout = schedule_timeout(timeout); - } - __set_current_state(TASK_RUNNING); - - if (!dma_fence_remove_callback(f, &cb.cb)) - return timeout; - - if (signal_pending_state(state, current)) - return -ERESTARTSYS; - - return -ETIME; -} - static const struct dma_fence_ops mock_ops = { .get_driver_name = mock_name, .get_timeline_name = mock_name, - .wait = mock_wait, .release = mock_fence_release, };
On Mon, May 11, 2020 at 10:41:03AM +0100, Chris Wilson wrote:
Quoting Daniel Vetter (2020-05-11 10:11:41)
No need to micro-optmize when we're waiting in a mocked object ...
It's setting up the expected return values for the test.
Drat, I suspect something like that but didn't spot it. Kinda wondering whether we should maybe lift the -ETIME special case to the generic version. But that's not really a safe thing to do there, drivers might actually use it for funny stuff.
Anyway motivation is that I'm pondering some extensions of dma_fence_wait and removing as many of the ->wait hooks as possible would have helped. But there's some nastier stuff like the legacy nouvea and radeon ones. -Daniel
-----Original Message----- From: Intel-gfx intel-gfx-bounces@lists.freedesktop.org On Behalf Of Daniel Vetter Sent: Monday, May 11, 2020 5:12 AM To: LKML linux-kernel@vger.kernel.org Cc: Daniel Vetter daniel.vetter@ffwll.ch; Intel Graphics Development intel-gfx@lists.freedesktop.org; DRI Development <dri- devel@lists.freedesktop.org>; linaro-mm-sig@lists.linaro.org; Vetter, Daniel daniel.vetter@intel.com; Sumit Semwal sumit.semwal@linaro.org; linux- media@vger.kernel.org Subject: [Intel-gfx] [PATCH 2/3] dma-fence: use default wait function for mock fences
No need to micro-optmize when we're waiting in a mocked object ...
Makes sense to me.
Acked-by: Michael J. Ruhl michael.j.ruhl@intel.com
M
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Sumit Semwal sumit.semwal@linaro.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org
drivers/dma-buf/st-dma-fence.c | 41 ---------------------------------- 1 file changed, 41 deletions(-)
diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma- fence.c index e593064341c8..8166d2984702 100644 --- a/drivers/dma-buf/st-dma-fence.c +++ b/drivers/dma-buf/st-dma-fence.c @@ -33,50 +33,9 @@ static void mock_fence_release(struct dma_fence *f) kmem_cache_free(slab_fences, to_mock_fence(f)); }
-struct wait_cb {
- struct dma_fence_cb cb;
- struct task_struct *task;
-};
-static void mock_wakeup(struct dma_fence *f, struct dma_fence_cb *cb) -{
- wake_up_process(container_of(cb, struct wait_cb, cb)->task);
-}
-static long mock_wait(struct dma_fence *f, bool intr, long timeout) -{
- const int state = intr ? TASK_INTERRUPTIBLE :
TASK_UNINTERRUPTIBLE;
- struct wait_cb cb = { .task = current };
- if (dma_fence_add_callback(f, &cb.cb, mock_wakeup))
return timeout;- while (timeout) {
set_current_state(state);if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &f->flags))break;if (signal_pending_state(state, current))break;timeout = schedule_timeout(timeout);- }
- __set_current_state(TASK_RUNNING);
- if (!dma_fence_remove_callback(f, &cb.cb))
return timeout;- if (signal_pending_state(state, current))
return -ERESTARTSYS;- return -ETIME;
-}
static const struct dma_fence_ops mock_ops = { .get_driver_name = mock_name, .get_timeline_name = mock_name,
- .wait = mock_wait, .release = mock_fence_release,
};
-- 2.26.2
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Ruhl, Michael J Sent: Monday, May 11, 2020 2:13 PM To: Daniel Vetter daniel.vetter@ffwll.ch; LKML <linux- kernel@vger.kernel.org> Cc: Intel Graphics Development intel-gfx@lists.freedesktop.org; DRI Development dri-devel@lists.freedesktop.org; linaro-mm- sig@lists.linaro.org; Vetter, Daniel daniel.vetter@intel.com; linux- media@vger.kernel.org Subject: RE: [Intel-gfx] [PATCH 2/3] dma-fence: use default wait function for mock fences
-----Original Message----- From: Intel-gfx intel-gfx-bounces@lists.freedesktop.org On Behalf Of Daniel Vetter Sent: Monday, May 11, 2020 5:12 AM To: LKML linux-kernel@vger.kernel.org Cc: Daniel Vetter daniel.vetter@ffwll.ch; Intel Graphics Development intel-gfx@lists.freedesktop.org; DRI Development <dri- devel@lists.freedesktop.org>; linaro-mm-sig@lists.linaro.org; Vetter, Daniel daniel.vetter@intel.com; Sumit Semwal sumit.semwal@linaro.org;
linux-
media@vger.kernel.org Subject: [Intel-gfx] [PATCH 2/3] dma-fence: use default wait function for mock fences
No need to micro-optmize when we're waiting in a mocked object ...
Makes sense to me.
/sigh.
Reading Chris comment, I am no longer sure it make sense...
Un-ack?
m
Acked-by: Michael J. Ruhl michael.j.ruhl@intel.com
M
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Sumit Semwal sumit.semwal@linaro.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org
drivers/dma-buf/st-dma-fence.c | 41 ---------------------------------- 1 file changed, 41 deletions(-)
diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma- fence.c index e593064341c8..8166d2984702 100644 --- a/drivers/dma-buf/st-dma-fence.c +++ b/drivers/dma-buf/st-dma-fence.c @@ -33,50 +33,9 @@ static void mock_fence_release(struct dma_fence *f) kmem_cache_free(slab_fences, to_mock_fence(f)); }
-struct wait_cb {
- struct dma_fence_cb cb;
- struct task_struct *task;
-};
-static void mock_wakeup(struct dma_fence *f, struct dma_fence_cb *cb) -{
- wake_up_process(container_of(cb, struct wait_cb, cb)->task);
-}
-static long mock_wait(struct dma_fence *f, bool intr, long timeout) -{
- const int state = intr ? TASK_INTERRUPTIBLE :
TASK_UNINTERRUPTIBLE;
- struct wait_cb cb = { .task = current };
- if (dma_fence_add_callback(f, &cb.cb, mock_wakeup))
return timeout;- while (timeout) {
set_current_state(state);if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &f->flags))break;if (signal_pending_state(state, current))break;timeout = schedule_timeout(timeout);- }
- __set_current_state(TASK_RUNNING);
- if (!dma_fence_remove_callback(f, &cb.cb))
return timeout;- if (signal_pending_state(state, current))
return -ERESTARTSYS;- return -ETIME;
-}
static const struct dma_fence_ops mock_ops = { .get_driver_name = mock_name, .get_timeline_name = mock_name,
- .wait = mock_wait, .release = mock_fence_release,
};
-- 2.26.2
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
It's the default.
Also so much for "we're not going to tell the graphics people how to review their code", dma_fence is a pretty core piece of gpu driver infrastructure. And it's very much uapi relevant, including piles of corresponding userspace protocols and libraries for how to pass these around.
Would be great if habanalabs would not use this (from a quick look it's not needed at all), since open source the userspace and playing by the usual rules isn't on the table. If that's not possible (because it's actually using the uapi part of dma_fence to interact with gpu drivers) then we have exactly what everyone promised we'd want to avoid.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Olof Johansson olof@lixom.net Cc: Oded Gabbay oded.gabbay@gmail.com Cc: Sumit Semwal sumit.semwal@linaro.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org --- drivers/misc/habanalabs/command_submission.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/misc/habanalabs/command_submission.c b/drivers/misc/habanalabs/command_submission.c index 409276b6374d..cc3ce759b6c3 100644 --- a/drivers/misc/habanalabs/command_submission.c +++ b/drivers/misc/habanalabs/command_submission.c @@ -46,7 +46,6 @@ static const struct dma_fence_ops hl_fence_ops = { .get_driver_name = hl_fence_get_driver_name, .get_timeline_name = hl_fence_get_timeline_name, .enable_signaling = hl_fence_enable_signaling, - .wait = dma_fence_default_wait, .release = hl_fence_release };
On Mon, May 11, 2020 at 12:11 PM Daniel Vetter daniel.vetter@ffwll.ch wrote:
It's the default.
Thanks for catching that.
Also so much for "we're not going to tell the graphics people how to review their code", dma_fence is a pretty core piece of gpu driver infrastructure. And it's very much uapi relevant, including piles of corresponding userspace protocols and libraries for how to pass these around.
Would be great if habanalabs would not use this (from a quick look it's not needed at all), since open source the userspace and playing by the usual rules isn't on the table. If that's not possible (because it's actually using the uapi part of dma_fence to interact with gpu drivers) then we have exactly what everyone promised we'd want to avoid.
We don't use the uapi parts, we currently only using the fencing and signaling ability of this module inside our kernel code. But maybe I didn't understand what you request. You want us *not* to use this well-written piece of kernel code because it is only used by graphics drivers ? I'm sorry but I don't get this argument, if this is indeed what you meant.
Oded
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Olof Johansson olof@lixom.net Cc: Oded Gabbay oded.gabbay@gmail.com Cc: Sumit Semwal sumit.semwal@linaro.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org
drivers/misc/habanalabs/command_submission.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/misc/habanalabs/command_submission.c b/drivers/misc/habanalabs/command_submission.c index 409276b6374d..cc3ce759b6c3 100644 --- a/drivers/misc/habanalabs/command_submission.c +++ b/drivers/misc/habanalabs/command_submission.c @@ -46,7 +46,6 @@ static const struct dma_fence_ops hl_fence_ops = { .get_driver_name = hl_fence_get_driver_name, .get_timeline_name = hl_fence_get_timeline_name, .enable_signaling = hl_fence_enable_signaling,
.wait = dma_fence_default_wait, .release = hl_fence_release};
-- 2.26.2
And just FYI, the driver was written internally at 2016-17, where the dma-buf module didn't check the .wait ops before calling it and that's why the initialization of the default wait was there in the first place. I should have removed it when I upstreamed it but it missed my review. Thanks, Oded
On Mon, May 11, 2020 at 12:36 PM Oded Gabbay oded.gabbay@gmail.com wrote:
On Mon, May 11, 2020 at 12:11 PM Daniel Vetter daniel.vetter@ffwll.ch wrote:
It's the default.
Thanks for catching that.
Also so much for "we're not going to tell the graphics people how to review their code", dma_fence is a pretty core piece of gpu driver infrastructure. And it's very much uapi relevant, including piles of corresponding userspace protocols and libraries for how to pass these around.
Would be great if habanalabs would not use this (from a quick look it's not needed at all), since open source the userspace and playing by the usual rules isn't on the table. If that's not possible (because it's actually using the uapi part of dma_fence to interact with gpu drivers) then we have exactly what everyone promised we'd want to avoid.
We don't use the uapi parts, we currently only using the fencing and signaling ability of this module inside our kernel code. But maybe I didn't understand what you request. You want us *not* to use this well-written piece of kernel code because it is only used by graphics drivers ? I'm sorry but I don't get this argument, if this is indeed what you meant.
Oded
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Olof Johansson olof@lixom.net Cc: Oded Gabbay oded.gabbay@gmail.com Cc: Sumit Semwal sumit.semwal@linaro.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org
drivers/misc/habanalabs/command_submission.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/misc/habanalabs/command_submission.c b/drivers/misc/habanalabs/command_submission.c index 409276b6374d..cc3ce759b6c3 100644 --- a/drivers/misc/habanalabs/command_submission.c +++ b/drivers/misc/habanalabs/command_submission.c @@ -46,7 +46,6 @@ static const struct dma_fence_ops hl_fence_ops = { .get_driver_name = hl_fence_get_driver_name, .get_timeline_name = hl_fence_get_timeline_name, .enable_signaling = hl_fence_enable_signaling,
.wait = dma_fence_default_wait, .release = hl_fence_release};
-- 2.26.2
On Mon, 11 May 2020 at 19:37, Oded Gabbay oded.gabbay@gmail.com wrote:
On Mon, May 11, 2020 at 12:11 PM Daniel Vetter daniel.vetter@ffwll.ch wrote:
It's the default.
Thanks for catching that.
Also so much for "we're not going to tell the graphics people how to review their code", dma_fence is a pretty core piece of gpu driver infrastructure. And it's very much uapi relevant, including piles of corresponding userspace protocols and libraries for how to pass these around.
Would be great if habanalabs would not use this (from a quick look it's not needed at all), since open source the userspace and playing by the usual rules isn't on the table. If that's not possible (because it's actually using the uapi part of dma_fence to interact with gpu drivers) then we have exactly what everyone promised we'd want to avoid.
We don't use the uapi parts, we currently only using the fencing and signaling ability of this module inside our kernel code. But maybe I didn't understand what you request. You want us *not* to use this well-written piece of kernel code because it is only used by graphics drivers ? I'm sorry but I don't get this argument, if this is indeed what you meant.
We would rather drivers using a feature that has requirements on correct userspace implementations of the feature have a userspace that is open source and auditable.
Fencing is tricky, cross-device fencing is really tricky, and having the ability for a closed userspace component to mess up other people's drivers, think i915 shared with closed habana userspace and shared fences, decreases ability to debug things.
Ideally we wouldn't offer users known untested/broken scenarios, so yes we'd prefer that drivers that intend to expose a userspace fencing api around dma-fence would adhere to the rules of the gpu drivers.
I'm not say you have to drop using dma-fence, but if you move towards cross-device stuff I believe other drivers would be correct in refusing to interact with fences from here.
Dave.
On Tue, May 12, 2020 at 4:14 AM Dave Airlie airlied@gmail.com wrote:
On Mon, 11 May 2020 at 19:37, Oded Gabbay oded.gabbay@gmail.com wrote:
On Mon, May 11, 2020 at 12:11 PM Daniel Vetter daniel.vetter@ffwll.ch wrote:
It's the default.
Thanks for catching that.
Also so much for "we're not going to tell the graphics people how to review their code", dma_fence is a pretty core piece of gpu driver infrastructure. And it's very much uapi relevant, including piles of corresponding userspace protocols and libraries for how to pass these around.
Would be great if habanalabs would not use this (from a quick look it's not needed at all), since open source the userspace and playing by the usual rules isn't on the table. If that's not possible (because it's actually using the uapi part of dma_fence to interact with gpu drivers) then we have exactly what everyone promised we'd want to avoid.
We don't use the uapi parts, we currently only using the fencing and signaling ability of this module inside our kernel code. But maybe I didn't understand what you request. You want us *not* to use this well-written piece of kernel code because it is only used by graphics drivers ? I'm sorry but I don't get this argument, if this is indeed what you meant.
We would rather drivers using a feature that has requirements on correct userspace implementations of the feature have a userspace that is open source and auditable.
Fencing is tricky, cross-device fencing is really tricky, and having the ability for a closed userspace component to mess up other people's drivers, think i915 shared with closed habana userspace and shared fences, decreases ability to debug things.
Ideally we wouldn't offer users known untested/broken scenarios, so yes we'd prefer that drivers that intend to expose a userspace fencing api around dma-fence would adhere to the rules of the gpu drivers.
I'm not say you have to drop using dma-fence, but if you move towards cross-device stuff I believe other drivers would be correct in refusing to interact with fences from here.
The flip side is if you only used dma-fence.c "because it's there", and not because it comes with an uapi attached and a cross-driver kernel internal contract for how to interact with gpu drivers, then there's really not much point in using it. It's a custom-rolled wait_queue/event thing, that's all. Without the gpu uapi and gpu cross-driver contract it would be much cleaner to just use wait_queue directly, and that's a construct all kernel developers understand, not just gpu folks. From a quick look at least habanalabs doesn't use any of these uapi/cross-driver/gpu bits. -Daniel
On Tue, May 12, 2020 at 9:12 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:
On Tue, May 12, 2020 at 4:14 AM Dave Airlie airlied@gmail.com wrote:
On Mon, 11 May 2020 at 19:37, Oded Gabbay oded.gabbay@gmail.com wrote:
On Mon, May 11, 2020 at 12:11 PM Daniel Vetter daniel.vetter@ffwll.ch wrote:
It's the default.
Thanks for catching that.
Also so much for "we're not going to tell the graphics people how to review their code", dma_fence is a pretty core piece of gpu driver infrastructure. And it's very much uapi relevant, including piles of corresponding userspace protocols and libraries for how to pass these around.
Would be great if habanalabs would not use this (from a quick look it's not needed at all), since open source the userspace and playing by the usual rules isn't on the table. If that's not possible (because it's actually using the uapi part of dma_fence to interact with gpu drivers) then we have exactly what everyone promised we'd want to avoid.
We don't use the uapi parts, we currently only using the fencing and signaling ability of this module inside our kernel code. But maybe I didn't understand what you request. You want us *not* to use this well-written piece of kernel code because it is only used by graphics drivers ? I'm sorry but I don't get this argument, if this is indeed what you meant.
We would rather drivers using a feature that has requirements on correct userspace implementations of the feature have a userspace that is open source and auditable.
Fencing is tricky, cross-device fencing is really tricky, and having the ability for a closed userspace component to mess up other people's drivers, think i915 shared with closed habana userspace and shared fences, decreases ability to debug things.
Ideally we wouldn't offer users known untested/broken scenarios, so yes we'd prefer that drivers that intend to expose a userspace fencing api around dma-fence would adhere to the rules of the gpu drivers.
I'm not say you have to drop using dma-fence, but if you move towards cross-device stuff I believe other drivers would be correct in refusing to interact with fences from here.
The flip side is if you only used dma-fence.c "because it's there", and not because it comes with an uapi attached and a cross-driver kernel internal contract for how to interact with gpu drivers, then there's really not much point in using it. It's a custom-rolled wait_queue/event thing, that's all. Without the gpu uapi and gpu cross-driver contract it would be much cleaner to just use wait_queue directly, and that's a construct all kernel developers understand, not just gpu folks. From a quick look at least habanalabs doesn't use any of these uapi/cross-driver/gpu bits. -Daniel
Hi Daniel, I want to say explicitly that we don't use the dma-buf uapi parts, nor we intend to use them to communicate with any GPU device. We only use it as simple completion mechanism as it was convenient to use. I do understand I can exchange that mechanism with a simpler one, and I will add an internal task to do it (albeit not in a very high priority) and upstream it, its just that it is part of our data path so we need to thoroughly validate it first.
Thanks, Oded
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, May 14, 2020 at 02:38:38PM +0300, Oded Gabbay wrote:
On Tue, May 12, 2020 at 9:12 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:
On Tue, May 12, 2020 at 4:14 AM Dave Airlie airlied@gmail.com wrote:
On Mon, 11 May 2020 at 19:37, Oded Gabbay oded.gabbay@gmail.com wrote:
On Mon, May 11, 2020 at 12:11 PM Daniel Vetter daniel.vetter@ffwll.ch wrote:
It's the default.
Thanks for catching that.
Also so much for "we're not going to tell the graphics people how to review their code", dma_fence is a pretty core piece of gpu driver infrastructure. And it's very much uapi relevant, including piles of corresponding userspace protocols and libraries for how to pass these around.
Would be great if habanalabs would not use this (from a quick look it's not needed at all), since open source the userspace and playing by the usual rules isn't on the table. If that's not possible (because it's actually using the uapi part of dma_fence to interact with gpu drivers) then we have exactly what everyone promised we'd want to avoid.
We don't use the uapi parts, we currently only using the fencing and signaling ability of this module inside our kernel code. But maybe I didn't understand what you request. You want us *not* to use this well-written piece of kernel code because it is only used by graphics drivers ? I'm sorry but I don't get this argument, if this is indeed what you meant.
We would rather drivers using a feature that has requirements on correct userspace implementations of the feature have a userspace that is open source and auditable.
Fencing is tricky, cross-device fencing is really tricky, and having the ability for a closed userspace component to mess up other people's drivers, think i915 shared with closed habana userspace and shared fences, decreases ability to debug things.
Ideally we wouldn't offer users known untested/broken scenarios, so yes we'd prefer that drivers that intend to expose a userspace fencing api around dma-fence would adhere to the rules of the gpu drivers.
I'm not say you have to drop using dma-fence, but if you move towards cross-device stuff I believe other drivers would be correct in refusing to interact with fences from here.
The flip side is if you only used dma-fence.c "because it's there", and not because it comes with an uapi attached and a cross-driver kernel internal contract for how to interact with gpu drivers, then there's really not much point in using it. It's a custom-rolled wait_queue/event thing, that's all. Without the gpu uapi and gpu cross-driver contract it would be much cleaner to just use wait_queue directly, and that's a construct all kernel developers understand, not just gpu folks. From a quick look at least habanalabs doesn't use any of these uapi/cross-driver/gpu bits. -Daniel
Hi Daniel, I want to say explicitly that we don't use the dma-buf uapi parts, nor we intend to use them to communicate with any GPU device. We only use it as simple completion mechanism as it was convenient to use. I do understand I can exchange that mechanism with a simpler one, and I will add an internal task to do it (albeit not in a very high priority) and upstream it, its just that it is part of our data path so we need to thoroughly validate it first.
Sounds good.
Wrt merging this patch here, can you include that in one of your next pulls? Or should I toss it entirely, waiting for you to remove dma_fence outright?
Thanks, Daniel
On Wed, May 20, 2020 at 9:05 PM Daniel Vetter daniel@ffwll.ch wrote:
On Thu, May 14, 2020 at 02:38:38PM +0300, Oded Gabbay wrote:
On Tue, May 12, 2020 at 9:12 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:
On Tue, May 12, 2020 at 4:14 AM Dave Airlie airlied@gmail.com wrote:
On Mon, 11 May 2020 at 19:37, Oded Gabbay oded.gabbay@gmail.com wrote:
On Mon, May 11, 2020 at 12:11 PM Daniel Vetter daniel.vetter@ffwll.ch wrote:
It's the default.
Thanks for catching that.
Also so much for "we're not going to tell the graphics people how to review their code", dma_fence is a pretty core piece of gpu driver infrastructure. And it's very much uapi relevant, including piles of corresponding userspace protocols and libraries for how to pass these around.
Would be great if habanalabs would not use this (from a quick look it's not needed at all), since open source the userspace and playing by the usual rules isn't on the table. If that's not possible (because it's actually using the uapi part of dma_fence to interact with gpu drivers) then we have exactly what everyone promised we'd want to avoid.
We don't use the uapi parts, we currently only using the fencing and signaling ability of this module inside our kernel code. But maybe I didn't understand what you request. You want us *not* to use this well-written piece of kernel code because it is only used by graphics drivers ? I'm sorry but I don't get this argument, if this is indeed what you meant.
We would rather drivers using a feature that has requirements on correct userspace implementations of the feature have a userspace that is open source and auditable.
Fencing is tricky, cross-device fencing is really tricky, and having the ability for a closed userspace component to mess up other people's drivers, think i915 shared with closed habana userspace and shared fences, decreases ability to debug things.
Ideally we wouldn't offer users known untested/broken scenarios, so yes we'd prefer that drivers that intend to expose a userspace fencing api around dma-fence would adhere to the rules of the gpu drivers.
I'm not say you have to drop using dma-fence, but if you move towards cross-device stuff I believe other drivers would be correct in refusing to interact with fences from here.
The flip side is if you only used dma-fence.c "because it's there", and not because it comes with an uapi attached and a cross-driver kernel internal contract for how to interact with gpu drivers, then there's really not much point in using it. It's a custom-rolled wait_queue/event thing, that's all. Without the gpu uapi and gpu cross-driver contract it would be much cleaner to just use wait_queue directly, and that's a construct all kernel developers understand, not just gpu folks. From a quick look at least habanalabs doesn't use any of these uapi/cross-driver/gpu bits. -Daniel
Hi Daniel, I want to say explicitly that we don't use the dma-buf uapi parts, nor we intend to use them to communicate with any GPU device. We only use it as simple completion mechanism as it was convenient to use. I do understand I can exchange that mechanism with a simpler one, and I will add an internal task to do it (albeit not in a very high priority) and upstream it, its just that it is part of our data path so we need to thoroughly validate it first.
Sounds good.
Wrt merging this patch here, can you include that in one of your next pulls? Or should I toss it entirely, waiting for you to remove dma_fence outright?
I'll include it in the next pull. Thanks, Oded
Thanks, Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Daniel Vetter Sent: Monday, May 11, 2020 5:12 AM To: LKML linux-kernel@vger.kernel.org Cc: David Airlie airlied@linux.ie; Daniel Vetter daniel.vetter@ffwll.ch; Intel Graphics Development intel-gfx@lists.freedesktop.org; DRI Development dri-devel@lists.freedesktop.org; Thomas Zimmermann tzimmermann@suse.de; Vetter, Daniel daniel.vetter@intel.com Subject: [PATCH 1/3] drm/writeback: don't set fence->ops to default
It's the default.
I can get behind that. 😊
Reviewed-by: Michael J. Ruhl michael.j.ruhl@intel.com
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_writeback.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index 43d9e3bb3a94..dccf4504f1bb 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -108,7 +108,6 @@ static const struct dma_fence_ops drm_writeback_fence_ops = { .get_driver_name = drm_writeback_fence_get_driver_name, .get_timeline_name = drm_writeback_fence_get_timeline_name, .enable_signaling = drm_writeback_fence_enable_signaling,
- .wait = dma_fence_default_wait,
};
static int create_writeback_properties(struct drm_device *dev)
2.26.2
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, May 11, 2020 at 06:12:32PM +0000, Ruhl, Michael J wrote:
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Daniel Vetter Sent: Monday, May 11, 2020 5:12 AM To: LKML linux-kernel@vger.kernel.org Cc: David Airlie airlied@linux.ie; Daniel Vetter daniel.vetter@ffwll.ch; Intel Graphics Development intel-gfx@lists.freedesktop.org; DRI Development dri-devel@lists.freedesktop.org; Thomas Zimmermann tzimmermann@suse.de; Vetter, Daniel daniel.vetter@intel.com Subject: [PATCH 1/3] drm/writeback: don't set fence->ops to default
It's the default.
I can get behind that. 😊
Reviewed-by: Michael J. Ruhl michael.j.ruhl@intel.com
Applied to drm-misc-next, thanks for reviewing. -Daniel
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_writeback.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index 43d9e3bb3a94..dccf4504f1bb 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -108,7 +108,6 @@ static const struct dma_fence_ops drm_writeback_fence_ops = { .get_driver_name = drm_writeback_fence_get_driver_name, .get_timeline_name = drm_writeback_fence_get_timeline_name, .enable_signaling = drm_writeback_fence_enable_signaling,
- .wait = dma_fence_default_wait,
};
static int create_writeback_properties(struct drm_device *dev)
2.26.2
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Daniel Vetter -
Dave Airlie -
Oded Gabbay -
Ruhl, Michael J