* [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error @ 2018-05-09 20:14 ` Ezequiel Garcia 0 siblings, 0 replies; 22+ messages in thread From: Ezequiel Garcia @ 2018-05-09 20:14 UTC (permalink / raw) To: Sumit Semwal, Gustavo Padovan Cc: linux-media, dri-devel, kernel, Ezequiel Garcia Change how dma_fence_add_callback() behaves, when the fence has error-signaled by the time it is being add. After this commit, dma_fence_add_callback() returns the fence error, if it has error-signaled before dma_fence_add_callback() is called. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/dma-buf/dma-fence.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 4edb9fd3cf47..298b440c5b68 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -226,7 +226,8 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); * * Note that the callback can be called from an atomic context. If * fence is already signaled, this function will return -ENOENT (and - * *not* call the callback) + * *not* call the callback). If the fence is error-signaled, this + * function returns the fence error. * * Add a software callback to the fence. Same restrictions apply to * refcount as it does to dma_fence_wait, however the caller doesn't need to @@ -235,8 +236,8 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); * after it signals with dma_fence_signal. The callback itself can be called * from irq context. * - * Returns 0 in case of success, -ENOENT if the fence is already signaled - * and -EINVAL in case of error. + * Returns 0 in case of success, -ENOENT (or the error value) if the fence is + * already signaled and -EINVAL in case of error. */ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, dma_fence_func_t func) @@ -250,7 +251,8 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { INIT_LIST_HEAD(&cb->node); - return -ENOENT; + ret = (fence->error < 0) ? fence->error : -ENOENT; + return ret; } spin_lock_irqsave(fence->lock, flags); -- 2.16.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error @ 2018-05-09 20:14 ` Ezequiel Garcia 0 siblings, 0 replies; 22+ messages in thread From: Ezequiel Garcia @ 2018-05-09 20:14 UTC (permalink / raw) To: Sumit Semwal, Gustavo Padovan Cc: kernel, Ezequiel Garcia, dri-devel, linux-media Change how dma_fence_add_callback() behaves, when the fence has error-signaled by the time it is being add. After this commit, dma_fence_add_callback() returns the fence error, if it has error-signaled before dma_fence_add_callback() is called. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/dma-buf/dma-fence.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 4edb9fd3cf47..298b440c5b68 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -226,7 +226,8 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); * * Note that the callback can be called from an atomic context. If * fence is already signaled, this function will return -ENOENT (and - * *not* call the callback) + * *not* call the callback). If the fence is error-signaled, this + * function returns the fence error. * * Add a software callback to the fence. Same restrictions apply to * refcount as it does to dma_fence_wait, however the caller doesn't need to @@ -235,8 +236,8 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); * after it signals with dma_fence_signal. The callback itself can be called * from irq context. * - * Returns 0 in case of success, -ENOENT if the fence is already signaled - * and -EINVAL in case of error. + * Returns 0 in case of success, -ENOENT (or the error value) if the fence is + * already signaled and -EINVAL in case of error. */ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, dma_fence_func_t func) @@ -250,7 +251,8 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { INIT_LIST_HEAD(&cb->node); - return -ENOENT; + ret = (fence->error < 0) ? fence->error : -ENOENT; + return ret; } spin_lock_irqsave(fence->lock, flags); -- 2.16.3 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error 2018-05-09 20:14 ` Ezequiel Garcia @ 2018-05-09 22:42 ` Gustavo Padovan -1 siblings, 0 replies; 22+ messages in thread From: Gustavo Padovan @ 2018-05-09 22:42 UTC (permalink / raw) To: Ezequiel Garcia, Sumit Semwal; +Cc: linux-media, dri-devel, kernel Hi Ezequiel, On Wed, 2018-05-09 at 17:14 -0300, Ezequiel Garcia wrote: > Change how dma_fence_add_callback() behaves, when the fence > has error-signaled by the time it is being add. After this commit, > dma_fence_add_callback() returns the fence error, if it > has error-signaled before dma_fence_add_callback() is called. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/dma-buf/dma-fence.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > fence.c > index 4edb9fd3cf47..298b440c5b68 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -226,7 +226,8 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); > * > * Note that the callback can be called from an atomic context. If > * fence is already signaled, this function will return -ENOENT (and > - * *not* call the callback) > + * *not* call the callback). If the fence is error-signaled, this > + * function returns the fence error. > * > * Add a software callback to the fence. Same restrictions apply to > * refcount as it does to dma_fence_wait, however the caller doesn't > need to > @@ -235,8 +236,8 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); > * after it signals with dma_fence_signal. The callback itself can > be called > * from irq context. > * > - * Returns 0 in case of success, -ENOENT if the fence is already > signaled > - * and -EINVAL in case of error. > + * Returns 0 in case of success, -ENOENT (or the error value) if the > fence is > + * already signaled and -EINVAL in case of error. > */ > int dma_fence_add_callback(struct dma_fence *fence, struct > dma_fence_cb *cb, > dma_fence_func_t func) > @@ -250,7 +251,8 @@ int dma_fence_add_callback(struct dma_fence > *fence, struct dma_fence_cb *cb, > > if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { > INIT_LIST_HEAD(&cb->node); > - return -ENOENT; > + ret = (fence->error < 0) ? fence->error : -ENOENT; > + return ret; > } It looks good to me, but I'd first go check all place we call it in the kernel because I have some memory of callers relying on the -ENOENT return code for some decision. I might be wrong though. Regards, Gustavo ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error @ 2018-05-09 22:42 ` Gustavo Padovan 0 siblings, 0 replies; 22+ messages in thread From: Gustavo Padovan @ 2018-05-09 22:42 UTC (permalink / raw) To: Ezequiel Garcia, Sumit Semwal; +Cc: kernel, dri-devel, linux-media Hi Ezequiel, On Wed, 2018-05-09 at 17:14 -0300, Ezequiel Garcia wrote: > Change how dma_fence_add_callback() behaves, when the fence > has error-signaled by the time it is being add. After this commit, > dma_fence_add_callback() returns the fence error, if it > has error-signaled before dma_fence_add_callback() is called. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/dma-buf/dma-fence.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > fence.c > index 4edb9fd3cf47..298b440c5b68 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -226,7 +226,8 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); > * > * Note that the callback can be called from an atomic context. If > * fence is already signaled, this function will return -ENOENT (and > - * *not* call the callback) > + * *not* call the callback). If the fence is error-signaled, this > + * function returns the fence error. > * > * Add a software callback to the fence. Same restrictions apply to > * refcount as it does to dma_fence_wait, however the caller doesn't > need to > @@ -235,8 +236,8 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); > * after it signals with dma_fence_signal. The callback itself can > be called > * from irq context. > * > - * Returns 0 in case of success, -ENOENT if the fence is already > signaled > - * and -EINVAL in case of error. > + * Returns 0 in case of success, -ENOENT (or the error value) if the > fence is > + * already signaled and -EINVAL in case of error. > */ > int dma_fence_add_callback(struct dma_fence *fence, struct > dma_fence_cb *cb, > dma_fence_func_t func) > @@ -250,7 +251,8 @@ int dma_fence_add_callback(struct dma_fence > *fence, struct dma_fence_cb *cb, > > if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { > INIT_LIST_HEAD(&cb->node); > - return -ENOENT; > + ret = (fence->error < 0) ? fence->error : -ENOENT; > + return ret; > } It looks good to me, but I'd first go check all place we call it in the kernel because I have some memory of callers relying on the -ENOENT return code for some decision. I might be wrong though. Regards, Gustavo _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error 2018-05-09 22:42 ` Gustavo Padovan @ 2018-05-10 12:51 ` Ezequiel Garcia -1 siblings, 0 replies; 22+ messages in thread From: Ezequiel Garcia @ 2018-05-10 12:51 UTC (permalink / raw) To: Gustavo Padovan, Sumit Semwal; +Cc: linux-media, dri-devel, kernel On Wed, 2018-05-09 at 19:42 -0300, Gustavo Padovan wrote: > Hi Ezequiel, > > On Wed, 2018-05-09 at 17:14 -0300, Ezequiel Garcia wrote: > > Change how dma_fence_add_callback() behaves, when the fence > > has error-signaled by the time it is being add. After this commit, > > dma_fence_add_callback() returns the fence error, if it > > has error-signaled before dma_fence_add_callback() is called. > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > drivers/dma-buf/dma-fence.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > > fence.c > > index 4edb9fd3cf47..298b440c5b68 100644 > > --- a/drivers/dma-buf/dma-fence.c > > +++ b/drivers/dma-buf/dma-fence.c > > @@ -226,7 +226,8 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); > > * > > * Note that the callback can be called from an atomic context. If > > * fence is already signaled, this function will return -ENOENT (and > > - * *not* call the callback) > > + * *not* call the callback). If the fence is error-signaled, this > > + * function returns the fence error. > > * > > * Add a software callback to the fence. Same restrictions apply to > > * refcount as it does to dma_fence_wait, however the caller doesn't > > need to > > @@ -235,8 +236,8 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); > > * after it signals with dma_fence_signal. The callback itself can > > be called > > * from irq context. > > * > > - * Returns 0 in case of success, -ENOENT if the fence is already > > signaled > > - * and -EINVAL in case of error. > > + * Returns 0 in case of success, -ENOENT (or the error value) if the > > fence is > > + * already signaled and -EINVAL in case of error. > > */ > > int dma_fence_add_callback(struct dma_fence *fence, struct > > dma_fence_cb *cb, > > dma_fence_func_t func) > > @@ -250,7 +251,8 @@ int dma_fence_add_callback(struct dma_fence > > *fence, struct dma_fence_cb *cb, > > > > if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { > > INIT_LIST_HEAD(&cb->node); > > - return -ENOENT; > > + ret = (fence->error < 0) ? fence->error : -ENOENT; > > + return ret; > > } > > It looks good to me, but I'd first go check all place we call it in the > kernel because I have some memory of callers relying on the -ENOENT > return code for some decision. I might be wrong though. > > I checked all users before submitting this patch. git grep " = dma_fence_add_callback" drivers/gpu/drm/i915/i915_sw_fence.c: ret = dma_fence_add_callback(dma, &cb->base, func); drivers/gpu/drm/scheduler/gpu_scheduler.c: r = dma_fence_add_callback(fence, &s_fence->cb, drivers/gpu/drm/scheduler/gpu_scheduler.c: r = dma_fence_add_callback(fence, &s_fence->cb, And from what I could see, all of them handle the error properly. Thanks, Eze ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error @ 2018-05-10 12:51 ` Ezequiel Garcia 0 siblings, 0 replies; 22+ messages in thread From: Ezequiel Garcia @ 2018-05-10 12:51 UTC (permalink / raw) To: Gustavo Padovan, Sumit Semwal; +Cc: kernel, dri-devel, linux-media On Wed, 2018-05-09 at 19:42 -0300, Gustavo Padovan wrote: > Hi Ezequiel, > > On Wed, 2018-05-09 at 17:14 -0300, Ezequiel Garcia wrote: > > Change how dma_fence_add_callback() behaves, when the fence > > has error-signaled by the time it is being add. After this commit, > > dma_fence_add_callback() returns the fence error, if it > > has error-signaled before dma_fence_add_callback() is called. > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > drivers/dma-buf/dma-fence.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > > fence.c > > index 4edb9fd3cf47..298b440c5b68 100644 > > --- a/drivers/dma-buf/dma-fence.c > > +++ b/drivers/dma-buf/dma-fence.c > > @@ -226,7 +226,8 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); > > * > > * Note that the callback can be called from an atomic context. If > > * fence is already signaled, this function will return -ENOENT (and > > - * *not* call the callback) > > + * *not* call the callback). If the fence is error-signaled, this > > + * function returns the fence error. > > * > > * Add a software callback to the fence. Same restrictions apply to > > * refcount as it does to dma_fence_wait, however the caller doesn't > > need to > > @@ -235,8 +236,8 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); > > * after it signals with dma_fence_signal. The callback itself can > > be called > > * from irq context. > > * > > - * Returns 0 in case of success, -ENOENT if the fence is already > > signaled > > - * and -EINVAL in case of error. > > + * Returns 0 in case of success, -ENOENT (or the error value) if the > > fence is > > + * already signaled and -EINVAL in case of error. > > */ > > int dma_fence_add_callback(struct dma_fence *fence, struct > > dma_fence_cb *cb, > > dma_fence_func_t func) > > @@ -250,7 +251,8 @@ int dma_fence_add_callback(struct dma_fence > > *fence, struct dma_fence_cb *cb, > > > > if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { > > INIT_LIST_HEAD(&cb->node); > > - return -ENOENT; > > + ret = (fence->error < 0) ? fence->error : -ENOENT; > > + return ret; > > } > > It looks good to me, but I'd first go check all place we call it in the > kernel because I have some memory of callers relying on the -ENOENT > return code for some decision. I might be wrong though. > > I checked all users before submitting this patch. git grep " = dma_fence_add_callback" drivers/gpu/drm/i915/i915_sw_fence.c: ret = dma_fence_add_callback(dma, &cb->base, func); drivers/gpu/drm/scheduler/gpu_scheduler.c: r = dma_fence_add_callback(fence, &s_fence->cb, drivers/gpu/drm/scheduler/gpu_scheduler.c: r = dma_fence_add_callback(fence, &s_fence->cb, And from what I could see, all of them handle the error properly. Thanks, Eze _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error 2018-05-10 12:51 ` Ezequiel Garcia @ 2018-05-11 7:23 ` Chris Wilson -1 siblings, 0 replies; 22+ messages in thread From: Chris Wilson @ 2018-05-11 7:23 UTC (permalink / raw) To: Ezequiel Garcia, Gustavo Padovan, Sumit Semwal Cc: kernel, dri-devel, linux-media Quoting Ezequiel Garcia (2018-05-10 13:51:56) > On Wed, 2018-05-09 at 19:42 -0300, Gustavo Padovan wrote: > > Hi Ezequiel, > > > > On Wed, 2018-05-09 at 17:14 -0300, Ezequiel Garcia wrote: > > > Change how dma_fence_add_callback() behaves, when the fence > > > has error-signaled by the time it is being add. After this commit, > > > dma_fence_add_callback() returns the fence error, if it > > > has error-signaled before dma_fence_add_callback() is called. > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > --- > > > drivers/dma-buf/dma-fence.c | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > > > fence.c > > > index 4edb9fd3cf47..298b440c5b68 100644 > > > --- a/drivers/dma-buf/dma-fence.c > > > +++ b/drivers/dma-buf/dma-fence.c > > > @@ -226,7 +226,8 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); > > > * > > > * Note that the callback can be called from an atomic context. If > > > * fence is already signaled, this function will return -ENOENT (and > > > - * *not* call the callback) > > > + * *not* call the callback). If the fence is error-signaled, this > > > + * function returns the fence error. > > > * > > > * Add a software callback to the fence. Same restrictions apply to > > > * refcount as it does to dma_fence_wait, however the caller doesn't > > > need to > > > @@ -235,8 +236,8 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); > > > * after it signals with dma_fence_signal. The callback itself can > > > be called > > > * from irq context. > > > * > > > - * Returns 0 in case of success, -ENOENT if the fence is already > > > signaled > > > - * and -EINVAL in case of error. > > > + * Returns 0 in case of success, -ENOENT (or the error value) if the > > > fence is > > > + * already signaled and -EINVAL in case of error. > > > */ > > > int dma_fence_add_callback(struct dma_fence *fence, struct > > > dma_fence_cb *cb, > > > dma_fence_func_t func) > > > @@ -250,7 +251,8 @@ int dma_fence_add_callback(struct dma_fence > > > *fence, struct dma_fence_cb *cb, > > > > > > if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { > > > INIT_LIST_HEAD(&cb->node); > > > - return -ENOENT; > > > + ret = (fence->error < 0) ? fence->error : -ENOENT; > > > + return ret; > > > } > > > > It looks good to me, but I'd first go check all place we call it in the > > kernel because I have some memory of callers relying on the -ENOENT > > return code for some decision. I might be wrong though. > > > > > > I checked all users before submitting this patch. > > git grep " = dma_fence_add_callback" > drivers/gpu/drm/i915/i915_sw_fence.c: ret = dma_fence_add_callback(dma, &cb->base, func); > > And from what I could see, all of them handle the error > properly. Err, no. That error then is propagated back to userspace, and that is not part of our ABI... -Chris ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error @ 2018-05-11 7:23 ` Chris Wilson 0 siblings, 0 replies; 22+ messages in thread From: Chris Wilson @ 2018-05-11 7:23 UTC (permalink / raw) To: Ezequiel Garcia, Gustavo Padovan, Sumit Semwal Cc: kernel, dri-devel, linux-media Quoting Ezequiel Garcia (2018-05-10 13:51:56) > On Wed, 2018-05-09 at 19:42 -0300, Gustavo Padovan wrote: > > Hi Ezequiel, > > > > On Wed, 2018-05-09 at 17:14 -0300, Ezequiel Garcia wrote: > > > Change how dma_fence_add_callback() behaves, when the fence > > > has error-signaled by the time it is being add. After this commit, > > > dma_fence_add_callback() returns the fence error, if it > > > has error-signaled before dma_fence_add_callback() is called. > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > --- > > > drivers/dma-buf/dma-fence.c | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > > > fence.c > > > index 4edb9fd3cf47..298b440c5b68 100644 > > > --- a/drivers/dma-buf/dma-fence.c > > > +++ b/drivers/dma-buf/dma-fence.c > > > @@ -226,7 +226,8 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); > > > * > > > * Note that the callback can be called from an atomic context. If > > > * fence is already signaled, this function will return -ENOENT (and > > > - * *not* call the callback) > > > + * *not* call the callback). If the fence is error-signaled, this > > > + * function returns the fence error. > > > * > > > * Add a software callback to the fence. Same restrictions apply to > > > * refcount as it does to dma_fence_wait, however the caller doesn't > > > need to > > > @@ -235,8 +236,8 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); > > > * after it signals with dma_fence_signal. The callback itself can > > > be called > > > * from irq context. > > > * > > > - * Returns 0 in case of success, -ENOENT if the fence is already > > > signaled > > > - * and -EINVAL in case of error. > > > + * Returns 0 in case of success, -ENOENT (or the error value) if the > > > fence is > > > + * already signaled and -EINVAL in case of error. > > > */ > > > int dma_fence_add_callback(struct dma_fence *fence, struct > > > dma_fence_cb *cb, > > > dma_fence_func_t func) > > > @@ -250,7 +251,8 @@ int dma_fence_add_callback(struct dma_fence > > > *fence, struct dma_fence_cb *cb, > > > > > > if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { > > > INIT_LIST_HEAD(&cb->node); > > > - return -ENOENT; > > > + ret = (fence->error < 0) ? fence->error : -ENOENT; > > > + return ret; > > > } > > > > It looks good to me, but I'd first go check all place we call it in the > > kernel because I have some memory of callers relying on the -ENOENT > > return code for some decision. I might be wrong though. > > > > > > I checked all users before submitting this patch. > > git grep " = dma_fence_add_callback" > drivers/gpu/drm/i915/i915_sw_fence.c: ret = dma_fence_add_callback(dma, &cb->base, func); > > And from what I could see, all of them handle the error > properly. Err, no. That error then is propagated back to userspace, and that is not part of our ABI... -Chris _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error 2018-05-09 20:14 ` Ezequiel Garcia @ 2018-05-11 7:27 ` Chris Wilson -1 siblings, 0 replies; 22+ messages in thread From: Chris Wilson @ 2018-05-11 7:27 UTC (permalink / raw) To: Ezequiel Garcia, Sumit Semwal, Gustavo Padovan Cc: kernel, Ezequiel Garcia, dri-devel, linux-media Quoting Ezequiel Garcia (2018-05-09 21:14:49) > Change how dma_fence_add_callback() behaves, when the fence > has error-signaled by the time it is being add. After this commit, > dma_fence_add_callback() returns the fence error, if it > has error-signaled before dma_fence_add_callback() is called. Why? What problem are you trying to solve? fence->error does not imply that the fence has yet been signaled, and the caller wants a callback when it is signaled. -Chris ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error @ 2018-05-11 7:27 ` Chris Wilson 0 siblings, 0 replies; 22+ messages in thread From: Chris Wilson @ 2018-05-11 7:27 UTC (permalink / raw) To: Sumit Semwal, Gustavo Padovan Cc: kernel, Ezequiel Garcia, dri-devel, linux-media Quoting Ezequiel Garcia (2018-05-09 21:14:49) > Change how dma_fence_add_callback() behaves, when the fence > has error-signaled by the time it is being add. After this commit, > dma_fence_add_callback() returns the fence error, if it > has error-signaled before dma_fence_add_callback() is called. Why? What problem are you trying to solve? fence->error does not imply that the fence has yet been signaled, and the caller wants a callback when it is signaled. -Chris _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error 2018-05-11 7:27 ` Chris Wilson @ 2018-05-14 16:48 ` Daniel Vetter -1 siblings, 0 replies; 22+ messages in thread From: Daniel Vetter @ 2018-05-14 16:48 UTC (permalink / raw) To: Chris Wilson Cc: Ezequiel Garcia, Sumit Semwal, Gustavo Padovan, kernel, dri-devel, linux-media On Fri, May 11, 2018 at 08:27:41AM +0100, Chris Wilson wrote: > Quoting Ezequiel Garcia (2018-05-09 21:14:49) > > Change how dma_fence_add_callback() behaves, when the fence > > has error-signaled by the time it is being add. After this commit, > > dma_fence_add_callback() returns the fence error, if it > > has error-signaled before dma_fence_add_callback() is called. > > Why? What problem are you trying to solve? fence->error does not imply > that the fence has yet been signaled, and the caller wants a callback > when it is signaled. On top this is incosistent, e.g. we don't do the same for any of the other dma_fence interfaces. Plus there's the issue that you might alias errno values with fence errno values. I think keeping the error codes from the functions you're calling distinct from the error code of the fence itself makes a lot of sense. The first tells you whether your request worked out (or why not), the second tells you whether the asynchronous dma operation (gpu rendering, page flip, whatever) that the dma_fence represents worked out (or why not). That's 2 distinct things imo. Might be good to show us the driver code that needs this behaviour so we can discuss how to best handle your use-case. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error @ 2018-05-14 16:48 ` Daniel Vetter 0 siblings, 0 replies; 22+ messages in thread From: Daniel Vetter @ 2018-05-14 16:48 UTC (permalink / raw) To: Chris Wilson; +Cc: dri-devel, kernel, Ezequiel Garcia, linux-media On Fri, May 11, 2018 at 08:27:41AM +0100, Chris Wilson wrote: > Quoting Ezequiel Garcia (2018-05-09 21:14:49) > > Change how dma_fence_add_callback() behaves, when the fence > > has error-signaled by the time it is being add. After this commit, > > dma_fence_add_callback() returns the fence error, if it > > has error-signaled before dma_fence_add_callback() is called. > > Why? What problem are you trying to solve? fence->error does not imply > that the fence has yet been signaled, and the caller wants a callback > when it is signaled. On top this is incosistent, e.g. we don't do the same for any of the other dma_fence interfaces. Plus there's the issue that you might alias errno values with fence errno values. I think keeping the error codes from the functions you're calling distinct from the error code of the fence itself makes a lot of sense. The first tells you whether your request worked out (or why not), the second tells you whether the asynchronous dma operation (gpu rendering, page flip, whatever) that the dma_fence represents worked out (or why not). That's 2 distinct things imo. Might be good to show us the driver code that needs this behaviour so we can discuss how to best handle your use-case. Cheers, 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error 2018-05-14 16:48 ` Daniel Vetter @ 2018-05-14 21:28 ` Ezequiel Garcia -1 siblings, 0 replies; 22+ messages in thread From: Ezequiel Garcia @ 2018-05-14 21:28 UTC (permalink / raw) To: Daniel Vetter, Chris Wilson Cc: Sumit Semwal, Gustavo Padovan, kernel, dri-devel, linux-media On Mon, 2018-05-14 at 18:48 +0200, Daniel Vetter wrote: > On Fri, May 11, 2018 at 08:27:41AM +0100, Chris Wilson wrote: > > Quoting Ezequiel Garcia (2018-05-09 21:14:49) > > > Change how dma_fence_add_callback() behaves, when the fence > > > has error-signaled by the time it is being add. After this commit, > > > dma_fence_add_callback() returns the fence error, if it > > > has error-signaled before dma_fence_add_callback() is called. > > > > Why? What problem are you trying to solve? fence->error does not imply > > that the fence has yet been signaled, and the caller wants a callback > > when it is signaled. > > On top this is incosistent, e.g. we don't do the same for any of the other > dma_fence interfaces. Plus there's the issue that you might alias errno > values with fence errno values. > Right. > I think keeping the error codes from the functions you're calling distinct > from the error code of the fence itself makes a lot of sense. The first > tells you whether your request worked out (or why not), the second tells > you whether the asynchronous dma operation (gpu rendering, page flip, > whatever) that the dma_fence represents worked out (or why not). That's 2 > distinct things imo. > > Might be good to show us the driver code that needs this behaviour so we > can discuss how to best handle your use-case. > This change arose while discussing the in-fences support for video4linux. Here's the patch that calls dma_fence_add_callback https://lkml.org/lkml/2018/5/4/766. The code snippet currently looks something like this: if (vb->in_fence) { ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb, vb2_qbuf_fence_cb); /* is the fence signaled? */ if (ret == -ENOENT) { dma_fence_put(vb->in_fence); vb->in_fence = NULL; } else if (ret) { goto unlock; } } In this use case, if the callback is added successfully, the video4linux core defers the activation of the buffer until the fence signals. If the fence is signaled (currently disregarding of errors) then the buffer is assumed to be ready to be activated, and so it gets queued for hardware usage. Giving some more thought to this, I'm not so sure what is the right action if a fence signaled with error. In this case, it appears to me that we shouldn't be using this buffer if its in-fence is in error, but perhaps I'm missing something. Thanks! Eze ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error @ 2018-05-14 21:28 ` Ezequiel Garcia 0 siblings, 0 replies; 22+ messages in thread From: Ezequiel Garcia @ 2018-05-14 21:28 UTC (permalink / raw) To: Daniel Vetter, Chris Wilson; +Cc: kernel, dri-devel, linux-media On Mon, 2018-05-14 at 18:48 +0200, Daniel Vetter wrote: > On Fri, May 11, 2018 at 08:27:41AM +0100, Chris Wilson wrote: > > Quoting Ezequiel Garcia (2018-05-09 21:14:49) > > > Change how dma_fence_add_callback() behaves, when the fence > > > has error-signaled by the time it is being add. After this commit, > > > dma_fence_add_callback() returns the fence error, if it > > > has error-signaled before dma_fence_add_callback() is called. > > > > Why? What problem are you trying to solve? fence->error does not imply > > that the fence has yet been signaled, and the caller wants a callback > > when it is signaled. > > On top this is incosistent, e.g. we don't do the same for any of the other > dma_fence interfaces. Plus there's the issue that you might alias errno > values with fence errno values. > Right. > I think keeping the error codes from the functions you're calling distinct > from the error code of the fence itself makes a lot of sense. The first > tells you whether your request worked out (or why not), the second tells > you whether the asynchronous dma operation (gpu rendering, page flip, > whatever) that the dma_fence represents worked out (or why not). That's 2 > distinct things imo. > > Might be good to show us the driver code that needs this behaviour so we > can discuss how to best handle your use-case. > This change arose while discussing the in-fences support for video4linux. Here's the patch that calls dma_fence_add_callback https://lkml.org/lkml/2018/5/4/766. The code snippet currently looks something like this: if (vb->in_fence) { ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb, vb2_qbuf_fence_cb); /* is the fence signaled? */ if (ret == -ENOENT) { dma_fence_put(vb->in_fence); vb->in_fence = NULL; } else if (ret) { goto unlock; } } In this use case, if the callback is added successfully, the video4linux core defers the activation of the buffer until the fence signals. If the fence is signaled (currently disregarding of errors) then the buffer is assumed to be ready to be activated, and so it gets queued for hardware usage. Giving some more thought to this, I'm not so sure what is the right action if a fence signaled with error. In this case, it appears to me that we shouldn't be using this buffer if its in-fence is in error, but perhaps I'm missing something. Thanks! Eze _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error 2018-05-14 21:28 ` Ezequiel Garcia @ 2018-05-15 12:16 ` Chris Wilson -1 siblings, 0 replies; 22+ messages in thread From: Chris Wilson @ 2018-05-15 12:16 UTC (permalink / raw) To: Ezequiel Garcia, Daniel Vetter Cc: Sumit Semwal, Gustavo Padovan, kernel, dri-devel, linux-media Quoting Ezequiel Garcia (2018-05-14 22:28:31) > On Mon, 2018-05-14 at 18:48 +0200, Daniel Vetter wrote: > > On Fri, May 11, 2018 at 08:27:41AM +0100, Chris Wilson wrote: > > > Quoting Ezequiel Garcia (2018-05-09 21:14:49) > > > > Change how dma_fence_add_callback() behaves, when the fence > > > > has error-signaled by the time it is being add. After this commit, > > > > dma_fence_add_callback() returns the fence error, if it > > > > has error-signaled before dma_fence_add_callback() is called. > > > > > > Why? What problem are you trying to solve? fence->error does not imply > > > that the fence has yet been signaled, and the caller wants a callback > > > when it is signaled. > > > > On top this is incosistent, e.g. we don't do the same for any of the other > > dma_fence interfaces. Plus there's the issue that you might alias errno > > values with fence errno values. > > > > Right. > > > I think keeping the error codes from the functions you're calling distinct > > from the error code of the fence itself makes a lot of sense. The first > > tells you whether your request worked out (or why not), the second tells > > you whether the asynchronous dma operation (gpu rendering, page flip, > > whatever) that the dma_fence represents worked out (or why not). That's 2 > > distinct things imo. > > > > Might be good to show us the driver code that needs this behaviour so we > > can discuss how to best handle your use-case. > > > > This change arose while discussing the in-fences support for video4linux. > Here's the patch that calls dma_fence_add_callback https://lkml.org/lkml/2018/5/4/766. > > The code snippet currently looks something like this: > > if (vb->in_fence) { > ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb, > > vb2_qbuf_fence_cb); > /* is the fence signaled? */ > if (ret == -ENOENT) { > > dma_fence_put(vb->in_fence); > vb->in_fence = NULL; > } else if (ret) > { > goto unlock; > } > } > > In this use case, if the callback is added successfully, > the video4linux core defers the activation of the buffer > until the fence signals. > > If the fence is signaled (currently disregarding of errors) > then the buffer is assumed to be ready to be activated, > and so it gets queued for hardware usage. > > Giving some more thought to this, I'm not so sure what is > the right action if a fence signaled with error. In this case, > it appears to me that we shouldn't be using this buffer > if its in-fence is in error, but perhaps I'm missing > something. What I have in mind for async errors is to skip the operation and propagate the error onto the next fence. Mostly because those async errors may include fatal errors such as unable to pin the backing storage for the operation, but even "trivial" errors such as an early operation failing means that this request is then subject to garbage-in, garbage-out. However, for trivial errors I would just propagate the error status (so the caller knows something went wrong if they care, but in all likelihood no one will notice) and continue on with the glitchy operation. -Chris ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error @ 2018-05-15 12:16 ` Chris Wilson 0 siblings, 0 replies; 22+ messages in thread From: Chris Wilson @ 2018-05-15 12:16 UTC (permalink / raw) To: Ezequiel Garcia, Daniel Vetter; +Cc: kernel, dri-devel, linux-media Quoting Ezequiel Garcia (2018-05-14 22:28:31) > On Mon, 2018-05-14 at 18:48 +0200, Daniel Vetter wrote: > > On Fri, May 11, 2018 at 08:27:41AM +0100, Chris Wilson wrote: > > > Quoting Ezequiel Garcia (2018-05-09 21:14:49) > > > > Change how dma_fence_add_callback() behaves, when the fence > > > > has error-signaled by the time it is being add. After this commit, > > > > dma_fence_add_callback() returns the fence error, if it > > > > has error-signaled before dma_fence_add_callback() is called. > > > > > > Why? What problem are you trying to solve? fence->error does not imply > > > that the fence has yet been signaled, and the caller wants a callback > > > when it is signaled. > > > > On top this is incosistent, e.g. we don't do the same for any of the other > > dma_fence interfaces. Plus there's the issue that you might alias errno > > values with fence errno values. > > > > Right. > > > I think keeping the error codes from the functions you're calling distinct > > from the error code of the fence itself makes a lot of sense. The first > > tells you whether your request worked out (or why not), the second tells > > you whether the asynchronous dma operation (gpu rendering, page flip, > > whatever) that the dma_fence represents worked out (or why not). That's 2 > > distinct things imo. > > > > Might be good to show us the driver code that needs this behaviour so we > > can discuss how to best handle your use-case. > > > > This change arose while discussing the in-fences support for video4linux. > Here's the patch that calls dma_fence_add_callback https://lkml.org/lkml/2018/5/4/766. > > The code snippet currently looks something like this: > > if (vb->in_fence) { > ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb, > > vb2_qbuf_fence_cb); > /* is the fence signaled? */ > if (ret == -ENOENT) { > > dma_fence_put(vb->in_fence); > vb->in_fence = NULL; > } else if (ret) > { > goto unlock; > } > } > > In this use case, if the callback is added successfully, > the video4linux core defers the activation of the buffer > until the fence signals. > > If the fence is signaled (currently disregarding of errors) > then the buffer is assumed to be ready to be activated, > and so it gets queued for hardware usage. > > Giving some more thought to this, I'm not so sure what is > the right action if a fence signaled with error. In this case, > it appears to me that we shouldn't be using this buffer > if its in-fence is in error, but perhaps I'm missing > something. What I have in mind for async errors is to skip the operation and propagate the error onto the next fence. Mostly because those async errors may include fatal errors such as unable to pin the backing storage for the operation, but even "trivial" errors such as an early operation failing means that this request is then subject to garbage-in, garbage-out. However, for trivial errors I would just propagate the error status (so the caller knows something went wrong if they care, but in all likelihood no one will notice) and continue on with the glitchy operation. -Chris _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error 2018-05-15 12:16 ` Chris Wilson @ 2018-05-16 9:42 ` Daniel Vetter -1 siblings, 0 replies; 22+ messages in thread From: Daniel Vetter @ 2018-05-16 9:42 UTC (permalink / raw) To: Chris Wilson Cc: Ezequiel Garcia, Daniel Vetter, Sumit Semwal, Gustavo Padovan, kernel, dri-devel, linux-media On Tue, May 15, 2018 at 01:16:30PM +0100, Chris Wilson wrote: > Quoting Ezequiel Garcia (2018-05-14 22:28:31) > > On Mon, 2018-05-14 at 18:48 +0200, Daniel Vetter wrote: > > > On Fri, May 11, 2018 at 08:27:41AM +0100, Chris Wilson wrote: > > > > Quoting Ezequiel Garcia (2018-05-09 21:14:49) > > > > > Change how dma_fence_add_callback() behaves, when the fence > > > > > has error-signaled by the time it is being add. After this commit, > > > > > dma_fence_add_callback() returns the fence error, if it > > > > > has error-signaled before dma_fence_add_callback() is called. > > > > > > > > Why? What problem are you trying to solve? fence->error does not imply > > > > that the fence has yet been signaled, and the caller wants a callback > > > > when it is signaled. > > > > > > On top this is incosistent, e.g. we don't do the same for any of the other > > > dma_fence interfaces. Plus there's the issue that you might alias errno > > > values with fence errno values. > > > > > > > Right. > > > > > I think keeping the error codes from the functions you're calling distinct > > > from the error code of the fence itself makes a lot of sense. The first > > > tells you whether your request worked out (or why not), the second tells > > > you whether the asynchronous dma operation (gpu rendering, page flip, > > > whatever) that the dma_fence represents worked out (or why not). That's 2 > > > distinct things imo. > > > > > > Might be good to show us the driver code that needs this behaviour so we > > > can discuss how to best handle your use-case. > > > > > > > This change arose while discussing the in-fences support for video4linux. > > Here's the patch that calls dma_fence_add_callback https://lkml.org/lkml/2018/5/4/766. > > > > The code snippet currently looks something like this: > > > > if (vb->in_fence) { > > ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb, > > > > vb2_qbuf_fence_cb); > > /* is the fence signaled? */ > > if (ret == -ENOENT) { > > > > dma_fence_put(vb->in_fence); > > vb->in_fence = NULL; > > } else if (ret) > > { > > goto unlock; > > } > > } > > > > In this use case, if the callback is added successfully, > > the video4linux core defers the activation of the buffer > > until the fence signals. > > > > If the fence is signaled (currently disregarding of errors) > > then the buffer is assumed to be ready to be activated, > > and so it gets queued for hardware usage. > > > > Giving some more thought to this, I'm not so sure what is > > the right action if a fence signaled with error. In this case, > > it appears to me that we shouldn't be using this buffer > > if its in-fence is in error, but perhaps I'm missing > > something. > > What I have in mind for async errors is to skip the operation and > propagate the error onto the next fence. Mostly because those async > errors may include fatal errors such as unable to pin the backing > storage for the operation, but even "trivial" errors such as an early > operation failing means that this request is then subject to garbage-in, > garbage-out. However, for trivial errors I would just propagate the > error status (so the caller knows something went wrong if they care, but > in all likelihood no one will notice) and continue on with the glitchy > operation. In general, there's not really any hard rule about propagating fence errors across devices. It's mostly just used by drivers internally to keep track of failed stuff (gpu hangs or anything else async like Chris describes here). For v4l I'm not sure you want to care much about this, since right now the main use of fence errors is gpu hang recovery (whether it's the driver or hw that's hung doesn't matter here). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error @ 2018-05-16 9:42 ` Daniel Vetter 0 siblings, 0 replies; 22+ messages in thread From: Daniel Vetter @ 2018-05-16 9:42 UTC (permalink / raw) To: Chris Wilson; +Cc: dri-devel, kernel, Ezequiel Garcia, linux-media On Tue, May 15, 2018 at 01:16:30PM +0100, Chris Wilson wrote: > Quoting Ezequiel Garcia (2018-05-14 22:28:31) > > On Mon, 2018-05-14 at 18:48 +0200, Daniel Vetter wrote: > > > On Fri, May 11, 2018 at 08:27:41AM +0100, Chris Wilson wrote: > > > > Quoting Ezequiel Garcia (2018-05-09 21:14:49) > > > > > Change how dma_fence_add_callback() behaves, when the fence > > > > > has error-signaled by the time it is being add. After this commit, > > > > > dma_fence_add_callback() returns the fence error, if it > > > > > has error-signaled before dma_fence_add_callback() is called. > > > > > > > > Why? What problem are you trying to solve? fence->error does not imply > > > > that the fence has yet been signaled, and the caller wants a callback > > > > when it is signaled. > > > > > > On top this is incosistent, e.g. we don't do the same for any of the other > > > dma_fence interfaces. Plus there's the issue that you might alias errno > > > values with fence errno values. > > > > > > > Right. > > > > > I think keeping the error codes from the functions you're calling distinct > > > from the error code of the fence itself makes a lot of sense. The first > > > tells you whether your request worked out (or why not), the second tells > > > you whether the asynchronous dma operation (gpu rendering, page flip, > > > whatever) that the dma_fence represents worked out (or why not). That's 2 > > > distinct things imo. > > > > > > Might be good to show us the driver code that needs this behaviour so we > > > can discuss how to best handle your use-case. > > > > > > > This change arose while discussing the in-fences support for video4linux. > > Here's the patch that calls dma_fence_add_callback https://lkml.org/lkml/2018/5/4/766. > > > > The code snippet currently looks something like this: > > > > if (vb->in_fence) { > > ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb, > > > > vb2_qbuf_fence_cb); > > /* is the fence signaled? */ > > if (ret == -ENOENT) { > > > > dma_fence_put(vb->in_fence); > > vb->in_fence = NULL; > > } else if (ret) > > { > > goto unlock; > > } > > } > > > > In this use case, if the callback is added successfully, > > the video4linux core defers the activation of the buffer > > until the fence signals. > > > > If the fence is signaled (currently disregarding of errors) > > then the buffer is assumed to be ready to be activated, > > and so it gets queued for hardware usage. > > > > Giving some more thought to this, I'm not so sure what is > > the right action if a fence signaled with error. In this case, > > it appears to me that we shouldn't be using this buffer > > if its in-fence is in error, but perhaps I'm missing > > something. > > What I have in mind for async errors is to skip the operation and > propagate the error onto the next fence. Mostly because those async > errors may include fatal errors such as unable to pin the backing > storage for the operation, but even "trivial" errors such as an early > operation failing means that this request is then subject to garbage-in, > garbage-out. However, for trivial errors I would just propagate the > error status (so the caller knows something went wrong if they care, but > in all likelihood no one will notice) and continue on with the glitchy > operation. In general, there's not really any hard rule about propagating fence errors across devices. It's mostly just used by drivers internally to keep track of failed stuff (gpu hangs or anything else async like Chris describes here). For v4l I'm not sure you want to care much about this, since right now the main use of fence errors is gpu hang recovery (whether it's the driver or hw that's hung doesn't matter here). -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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error 2018-05-16 9:42 ` Daniel Vetter @ 2018-05-16 10:26 ` Lucas Stach -1 siblings, 0 replies; 22+ messages in thread From: Lucas Stach @ 2018-05-16 10:26 UTC (permalink / raw) To: Daniel Vetter, Chris Wilson Cc: dri-devel, kernel, Ezequiel Garcia, linux-media Am Mittwoch, den 16.05.2018, 11:42 +0200 schrieb Daniel Vetter: > On Tue, May 15, 2018 at 01:16:30PM +0100, Chris Wilson wrote: > > Quoting Ezequiel Garcia (2018-05-14 22:28:31) > > > On Mon, 2018-05-14 at 18:48 +0200, Daniel Vetter wrote: > > > > On Fri, May 11, 2018 at 08:27:41AM +0100, Chris Wilson wrote: > > > > > Quoting Ezequiel Garcia (2018-05-09 21:14:49) > > > > > > Change how dma_fence_add_callback() behaves, when the fence > > > > > > has error-signaled by the time it is being add. After this commit, > > > > > > dma_fence_add_callback() returns the fence error, if it > > > > > > has error-signaled before dma_fence_add_callback() is called. > > > > > > > > > > Why? What problem are you trying to solve? fence->error does not imply > > > > > that the fence has yet been signaled, and the caller wants a callback > > > > > when it is signaled. > > > > > > > > On top this is incosistent, e.g. we don't do the same for any of the other > > > > dma_fence interfaces. Plus there's the issue that you might alias errno > > > > values with fence errno values. > > > > > > > > > > Right. > > > > > > > I think keeping the error codes from the functions you're calling distinct > > > > from the error code of the fence itself makes a lot of sense. The first > > > > tells you whether your request worked out (or why not), the second tells > > > > you whether the asynchronous dma operation (gpu rendering, page flip, > > > > whatever) that the dma_fence represents worked out (or why not). That's 2 > > > > distinct things imo. > > > > > > > > Might be good to show us the driver code that needs this behaviour so we > > > > can discuss how to best handle your use-case. > > > > > > > > > > This change arose while discussing the in-fences support for video4linux. > > > Here's the patch that calls dma_fence_add_callback https://lkml.org/lkml/2018/5/4/766. > > > > > > The code snippet currently looks something like this: > > > > > > if (vb->in_fence) { > > > ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb, > > > > > > vb2_qbuf_fence_cb); > > > /* is the fence signaled? */ > > > if (ret == -ENOENT) { > > > > > > dma_fence_put(vb->in_fence); > > > vb->in_fence = NULL; > > > } else if (ret) > > > { > > > goto unlock; > > > } > > > } > > > > > > In this use case, if the callback is added successfully, > > > the video4linux core defers the activation of the buffer > > > until the fence signals. > > > > > > If the fence is signaled (currently disregarding of errors) > > > then the buffer is assumed to be ready to be activated, > > > and so it gets queued for hardware usage. > > > > > > Giving some more thought to this, I'm not so sure what is > > > the right action if a fence signaled with error. In this case, > > > it appears to me that we shouldn't be using this buffer > > > if its in-fence is in error, but perhaps I'm missing > > > something. > > > > What I have in mind for async errors is to skip the operation and > > propagate the error onto the next fence. Mostly because those async > > errors may include fatal errors such as unable to pin the backing > > storage for the operation, but even "trivial" errors such as an early > > operation failing means that this request is then subject to garbage-in, > > garbage-out. However, for trivial errors I would just propagate the > > error status (so the caller knows something went wrong if they care, but > > in all likelihood no one will notice) and continue on with the glitchy > > operation. > > In general, there's not really any hard rule about propagating fence > errors across devices. It's mostly just used by drivers internally to keep > track of failed stuff (gpu hangs or anything else async like Chris > describes here). > > For v4l I'm not sure you want to care much about this, since right now the > main use of fence errors is gpu hang recovery (whether it's the driver or > hw that's hung doesn't matter here). Yes, my understanding is that fence signal and errors are two distinct things that should not be conflated like it is done in this patch. In my understanding signaling a fence means the HW or SW component which added the fence is done with the buffer and will not touch it anymore. In case of an unrecoverable error the fence will be signaled with error status set, so we correctly reflect the buffer status as being free to be used by whoever is waiting for it, but may contain garbage. If a fence waiter cares about the buffer content and may wish to skip its operation if the fence is signaled with an error it should do it by explicitly checking the fence error status, instead of making this implicit behavior. Regards, Lucas ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error @ 2018-05-16 10:26 ` Lucas Stach 0 siblings, 0 replies; 22+ messages in thread From: Lucas Stach @ 2018-05-16 10:26 UTC (permalink / raw) To: Daniel Vetter, Chris Wilson Cc: kernel, Ezequiel Garcia, dri-devel, linux-media Am Mittwoch, den 16.05.2018, 11:42 +0200 schrieb Daniel Vetter: > On Tue, May 15, 2018 at 01:16:30PM +0100, Chris Wilson wrote: > > Quoting Ezequiel Garcia (2018-05-14 22:28:31) > > > On Mon, 2018-05-14 at 18:48 +0200, Daniel Vetter wrote: > > > > On Fri, May 11, 2018 at 08:27:41AM +0100, Chris Wilson wrote: > > > > > Quoting Ezequiel Garcia (2018-05-09 21:14:49) > > > > > > Change how dma_fence_add_callback() behaves, when the fence > > > > > > has error-signaled by the time it is being add. After this commit, > > > > > > dma_fence_add_callback() returns the fence error, if it > > > > > > has error-signaled before dma_fence_add_callback() is called. > > > > > > > > > > Why? What problem are you trying to solve? fence->error does not imply > > > > > that the fence has yet been signaled, and the caller wants a callback > > > > > when it is signaled. > > > > > > > > On top this is incosistent, e.g. we don't do the same for any of the other > > > > dma_fence interfaces. Plus there's the issue that you might alias errno > > > > values with fence errno values. > > > > > > > > > > Right. > > > > > > > I think keeping the error codes from the functions you're calling distinct > > > > from the error code of the fence itself makes a lot of sense. The first > > > > tells you whether your request worked out (or why not), the second tells > > > > you whether the asynchronous dma operation (gpu rendering, page flip, > > > > whatever) that the dma_fence represents worked out (or why not). That's 2 > > > > distinct things imo. > > > > > > > > Might be good to show us the driver code that needs this behaviour so we > > > > can discuss how to best handle your use-case. > > > > > > > > > > This change arose while discussing the in-fences support for video4linux. > > > Here's the patch that calls dma_fence_add_callback https://lkml.org/lkml/2018/5/4/766. > > > > > > The code snippet currently looks something like this: > > > > > > if (vb->in_fence) { > > > ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb, > > > > > > vb2_qbuf_fence_cb); > > > /* is the fence signaled? */ > > > if (ret == -ENOENT) { > > > > > > dma_fence_put(vb->in_fence); > > > vb->in_fence = NULL; > > > } else if (ret) > > > { > > > goto unlock; > > > } > > > } > > > > > > In this use case, if the callback is added successfully, > > > the video4linux core defers the activation of the buffer > > > until the fence signals. > > > > > > If the fence is signaled (currently disregarding of errors) > > > then the buffer is assumed to be ready to be activated, > > > and so it gets queued for hardware usage. > > > > > > Giving some more thought to this, I'm not so sure what is > > > the right action if a fence signaled with error. In this case, > > > it appears to me that we shouldn't be using this buffer > > > if its in-fence is in error, but perhaps I'm missing > > > something. > > > > What I have in mind for async errors is to skip the operation and > > propagate the error onto the next fence. Mostly because those async > > errors may include fatal errors such as unable to pin the backing > > storage for the operation, but even "trivial" errors such as an early > > operation failing means that this request is then subject to garbage-in, > > garbage-out. However, for trivial errors I would just propagate the > > error status (so the caller knows something went wrong if they care, but > > in all likelihood no one will notice) and continue on with the glitchy > > operation. > > In general, there's not really any hard rule about propagating fence > errors across devices. It's mostly just used by drivers internally to keep > track of failed stuff (gpu hangs or anything else async like Chris > describes here). > > For v4l I'm not sure you want to care much about this, since right now the > main use of fence errors is gpu hang recovery (whether it's the driver or > hw that's hung doesn't matter here). Yes, my understanding is that fence signal and errors are two distinct things that should not be conflated like it is done in this patch. In my understanding signaling a fence means the HW or SW component which added the fence is done with the buffer and will not touch it anymore. In case of an unrecoverable error the fence will be signaled with error status set, so we correctly reflect the buffer status as being free to be used by whoever is waiting for it, but may contain garbage. If a fence waiter cares about the buffer content and may wish to skip its operation if the fence is signaled with an error it should do it by explicitly checking the fence error status, instead of making this implicit behavior. Regards, Lucas _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error 2018-05-16 10:26 ` Lucas Stach @ 2018-05-16 18:27 ` Ezequiel Garcia -1 siblings, 0 replies; 22+ messages in thread From: Ezequiel Garcia @ 2018-05-16 18:27 UTC (permalink / raw) To: Lucas Stach, Daniel Vetter, Chris Wilson; +Cc: dri-devel, kernel, linux-media On Wed, 2018-05-16 at 12:26 +0200, Lucas Stach wrote: > Am Mittwoch, den 16.05.2018, 11:42 +0200 schrieb Daniel Vetter: > > On Tue, May 15, 2018 at 01:16:30PM +0100, Chris Wilson wrote: > > > Quoting Ezequiel Garcia (2018-05-14 22:28:31) > > > > On Mon, 2018-05-14 at 18:48 +0200, Daniel Vetter wrote: > > > > > On Fri, May 11, 2018 at 08:27:41AM +0100, Chris Wilson wrote: > > > > > > Quoting Ezequiel Garcia (2018-05-09 21:14:49) > > > > > > > Change how dma_fence_add_callback() behaves, when the fence > > > > > > > has error-signaled by the time it is being add. After this commit, > > > > > > > dma_fence_add_callback() returns the fence error, if it > > > > > > > has error-signaled before dma_fence_add_callback() is called. > > > > > > > > > > > > Why? What problem are you trying to solve? fence->error does not imply > > > > > > that the fence has yet been signaled, and the caller wants a callback > > > > > > when it is signaled. > > > > > > > > > > On top this is incosistent, e.g. we don't do the same for any of the other > > > > > dma_fence interfaces. Plus there's the issue that you might alias errno > > > > > values with fence errno values. > > > > > > > > > > > > > Right. > > > > > > > > > I think keeping the error codes from the functions you're calling distinct > > > > > from the error code of the fence itself makes a lot of sense. The first > > > > > tells you whether your request worked out (or why not), the second tells > > > > > you whether the asynchronous dma operation (gpu rendering, page flip, > > > > > whatever) that the dma_fence represents worked out (or why not). That's 2 > > > > > distinct things imo. > > > > > > > > > > Might be good to show us the driver code that needs this behaviour so we > > > > > can discuss how to best handle your use-case. > > > > > > > > > > > > > This change arose while discussing the in-fences support for video4linux. > > > > Here's the patch that calls dma_fence_add_callback https://lkml.org/lkml/2018/5/4/766. > > > > > > > > The code snippet currently looks something like this: > > > > > > > > if (vb->in_fence) { > > > > ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb, > > > > > > > > vb2_qbuf_fence_cb); > > > > /* is the fence signaled? */ > > > > if (ret == -ENOENT) { > > > > > > > > dma_fence_put(vb->in_fence); > > > > vb->in_fence = NULL; > > > > } else if (ret) > > > > { > > > > goto unlock; > > > > } > > > > } > > > > > > > > In this use case, if the callback is added successfully, > > > > the video4linux core defers the activation of the buffer > > > > until the fence signals. > > > > > > > > If the fence is signaled (currently disregarding of errors) > > > > then the buffer is assumed to be ready to be activated, > > > > and so it gets queued for hardware usage. > > > > > > > > Giving some more thought to this, I'm not so sure what is > > > > the right action if a fence signaled with error. In this case, > > > > it appears to me that we shouldn't be using this buffer > > > > if its in-fence is in error, but perhaps I'm missing > > > > something. > > > > > > What I have in mind for async errors is to skip the operation and > > > propagate the error onto the next fence. Mostly because those async > > > errors may include fatal errors such as unable to pin the backing > > > storage for the operation, but even "trivial" errors such as an early > > > operation failing means that this request is then subject to garbage-in, > > > garbage-out. However, for trivial errors I would just propagate the > > > error status (so the caller knows something went wrong if they care, but > > > in all likelihood no one will notice) and continue on with the glitchy > > > operation. > > > > In general, there's not really any hard rule about propagating fence > > errors across devices. It's mostly just used by drivers internally to keep > > track of failed stuff (gpu hangs or anything else async like Chris > > describes here). > > > > For v4l I'm not sure you want to care much about this, since right now the > > main use of fence errors is gpu hang recovery (whether it's the driver or > > hw that's hung doesn't matter here). > > Yes, my understanding is that fence signal and errors are two distinct > things that should not be conflated like it is done in this patch. > > In my understanding signaling a fence means the HW or SW component > which added the fence is done with the buffer and will not touch it > anymore. In case of an unrecoverable error the fence will be signaled > with error status set, so we correctly reflect the buffer status as > being free to be used by whoever is waiting for it, but may contain > garbage. > > If a fence waiter cares about the buffer content and may wish to skip > its operation if the fence is signaled with an error it should do it by > explicitly checking the fence error status, instead of making this > implicit behavior. > Yes, that sounds right. Thanks for the help, Eze ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error @ 2018-05-16 18:27 ` Ezequiel Garcia 0 siblings, 0 replies; 22+ messages in thread From: Ezequiel Garcia @ 2018-05-16 18:27 UTC (permalink / raw) To: Lucas Stach, Daniel Vetter, Chris Wilson; +Cc: kernel, dri-devel, linux-media On Wed, 2018-05-16 at 12:26 +0200, Lucas Stach wrote: > Am Mittwoch, den 16.05.2018, 11:42 +0200 schrieb Daniel Vetter: > > On Tue, May 15, 2018 at 01:16:30PM +0100, Chris Wilson wrote: > > > Quoting Ezequiel Garcia (2018-05-14 22:28:31) > > > > On Mon, 2018-05-14 at 18:48 +0200, Daniel Vetter wrote: > > > > > On Fri, May 11, 2018 at 08:27:41AM +0100, Chris Wilson wrote: > > > > > > Quoting Ezequiel Garcia (2018-05-09 21:14:49) > > > > > > > Change how dma_fence_add_callback() behaves, when the fence > > > > > > > has error-signaled by the time it is being add. After this commit, > > > > > > > dma_fence_add_callback() returns the fence error, if it > > > > > > > has error-signaled before dma_fence_add_callback() is called. > > > > > > > > > > > > Why? What problem are you trying to solve? fence->error does not imply > > > > > > that the fence has yet been signaled, and the caller wants a callback > > > > > > when it is signaled. > > > > > > > > > > On top this is incosistent, e.g. we don't do the same for any of the other > > > > > dma_fence interfaces. Plus there's the issue that you might alias errno > > > > > values with fence errno values. > > > > > > > > > > > > > Right. > > > > > > > > > I think keeping the error codes from the functions you're calling distinct > > > > > from the error code of the fence itself makes a lot of sense. The first > > > > > tells you whether your request worked out (or why not), the second tells > > > > > you whether the asynchronous dma operation (gpu rendering, page flip, > > > > > whatever) that the dma_fence represents worked out (or why not). That's 2 > > > > > distinct things imo. > > > > > > > > > > Might be good to show us the driver code that needs this behaviour so we > > > > > can discuss how to best handle your use-case. > > > > > > > > > > > > > This change arose while discussing the in-fences support for video4linux. > > > > Here's the patch that calls dma_fence_add_callback https://lkml.org/lkml/2018/5/4/766. > > > > > > > > The code snippet currently looks something like this: > > > > > > > > if (vb->in_fence) { > > > > ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb, > > > > > > > > vb2_qbuf_fence_cb); > > > > /* is the fence signaled? */ > > > > if (ret == -ENOENT) { > > > > > > > > dma_fence_put(vb->in_fence); > > > > vb->in_fence = NULL; > > > > } else if (ret) > > > > { > > > > goto unlock; > > > > } > > > > } > > > > > > > > In this use case, if the callback is added successfully, > > > > the video4linux core defers the activation of the buffer > > > > until the fence signals. > > > > > > > > If the fence is signaled (currently disregarding of errors) > > > > then the buffer is assumed to be ready to be activated, > > > > and so it gets queued for hardware usage. > > > > > > > > Giving some more thought to this, I'm not so sure what is > > > > the right action if a fence signaled with error. In this case, > > > > it appears to me that we shouldn't be using this buffer > > > > if its in-fence is in error, but perhaps I'm missing > > > > something. > > > > > > What I have in mind for async errors is to skip the operation and > > > propagate the error onto the next fence. Mostly because those async > > > errors may include fatal errors such as unable to pin the backing > > > storage for the operation, but even "trivial" errors such as an early > > > operation failing means that this request is then subject to garbage-in, > > > garbage-out. However, for trivial errors I would just propagate the > > > error status (so the caller knows something went wrong if they care, but > > > in all likelihood no one will notice) and continue on with the glitchy > > > operation. > > > > In general, there's not really any hard rule about propagating fence > > errors across devices. It's mostly just used by drivers internally to keep > > track of failed stuff (gpu hangs or anything else async like Chris > > describes here). > > > > For v4l I'm not sure you want to care much about this, since right now the > > main use of fence errors is gpu hang recovery (whether it's the driver or > > hw that's hung doesn't matter here). > > Yes, my understanding is that fence signal and errors are two distinct > things that should not be conflated like it is done in this patch. > > In my understanding signaling a fence means the HW or SW component > which added the fence is done with the buffer and will not touch it > anymore. In case of an unrecoverable error the fence will be signaled > with error status set, so we correctly reflect the buffer status as > being free to be used by whoever is waiting for it, but may contain > garbage. > > If a fence waiter cares about the buffer content and may wish to skip > its operation if the fence is signaled with an error it should do it by > explicitly checking the fence error status, instead of making this > implicit behavior. > Yes, that sounds right. Thanks for the help, Eze _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2018-05-16 18:28 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-09 20:14 [PATCH] dma-fence: Make dma_fence_add_callback() fail if signaled with error Ezequiel Garcia 2018-05-09 20:14 ` Ezequiel Garcia 2018-05-09 22:42 ` Gustavo Padovan 2018-05-09 22:42 ` Gustavo Padovan 2018-05-10 12:51 ` Ezequiel Garcia 2018-05-10 12:51 ` Ezequiel Garcia 2018-05-11 7:23 ` Chris Wilson 2018-05-11 7:23 ` Chris Wilson 2018-05-11 7:27 ` Chris Wilson 2018-05-11 7:27 ` Chris Wilson 2018-05-14 16:48 ` Daniel Vetter 2018-05-14 16:48 ` Daniel Vetter 2018-05-14 21:28 ` Ezequiel Garcia 2018-05-14 21:28 ` Ezequiel Garcia 2018-05-15 12:16 ` Chris Wilson 2018-05-15 12:16 ` Chris Wilson 2018-05-16 9:42 ` Daniel Vetter 2018-05-16 9:42 ` Daniel Vetter 2018-05-16 10:26 ` Lucas Stach 2018-05-16 10:26 ` Lucas Stach 2018-05-16 18:27 ` Ezequiel Garcia 2018-05-16 18:27 ` Ezequiel Garcia
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.