| Message ID | 20170103110533.31880-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/01/2017 11:05, Chris Wilson wrote: > As the fence->status is an optional field that may be set before > dma_fence_signal() is called to convey that the fence completed with an > error, we have to ensure that it is always set to zero on initialisation > so that the typical use (i.e. unset) always flags a successful completion. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/dma-buf/dma-fence.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index 3444f293ad4a..9130f790ebf3 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -534,6 +534,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, > fence->context = context; > fence->seqno = seqno; > fence->flags = 0UL; > + fence->status = 0; > > trace_dma_fence_init(fence); > } > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
On Tue, Jan 03, 2017 at 02:04:44PM +0000, Tvrtko Ursulin wrote: > > On 03/01/2017 11:05, Chris Wilson wrote: > > As the fence->status is an optional field that may be set before > > dma_fence_signal() is called to convey that the fence completed with an > > error, we have to ensure that it is always set to zero on initialisation > > so that the typical use (i.e. unset) always flags a successful completion. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/dma-buf/dma-fence.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > index 3444f293ad4a..9130f790ebf3 100644 > > --- a/drivers/dma-buf/dma-fence.c > > +++ b/drivers/dma-buf/dma-fence.c > > @@ -534,6 +534,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, > > fence->context = context; > > fence->seqno = seqno; > > fence->flags = 0UL; > > + fence->status = 0; > > > > trace_dma_fence_init(fence); > > } > > > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Looking at existing users there's only the sync_file stuff. And that gets it wrong, because afaics no one ever sets fence->status to anything useful. But sync_file blindly assumes it's correct. Gustavo, Maarten? -Daniel
On Wed, Jan 04, 2017 at 10:15:01AM +0100, Daniel Vetter wrote: > On Tue, Jan 03, 2017 at 02:04:44PM +0000, Tvrtko Ursulin wrote: > > > > On 03/01/2017 11:05, Chris Wilson wrote: > > > As the fence->status is an optional field that may be set before > > > dma_fence_signal() is called to convey that the fence completed with an > > > error, we have to ensure that it is always set to zero on initialisation > > > so that the typical use (i.e. unset) always flags a successful completion. > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > --- > > > drivers/dma-buf/dma-fence.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > > index 3444f293ad4a..9130f790ebf3 100644 > > > --- a/drivers/dma-buf/dma-fence.c > > > +++ b/drivers/dma-buf/dma-fence.c > > > @@ -534,6 +534,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, > > > fence->context = context; > > > fence->seqno = seqno; > > > fence->flags = 0UL; > > > + fence->status = 0; > > > > > > trace_dma_fence_init(fence); > > > } > > > > > > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Looking at existing users there's only the sync_file stuff. And that gets > it wrong, because afaics no one ever sets fence->status to anything > useful. But sync_file blindly assumes it's correct. In terms of doc, sync_file is using it correctly, and dma-fence isn't living up to its doc. The documented behaviour (sync_file) seems useful. -Chris
On Wed, Jan 04, 2017 at 09:24:27AM +0000, Chris Wilson wrote: > On Wed, Jan 04, 2017 at 10:15:01AM +0100, Daniel Vetter wrote: > > On Tue, Jan 03, 2017 at 02:04:44PM +0000, Tvrtko Ursulin wrote: > > > > > > On 03/01/2017 11:05, Chris Wilson wrote: > > > > As the fence->status is an optional field that may be set before > > > > dma_fence_signal() is called to convey that the fence completed with an > > > > error, we have to ensure that it is always set to zero on initialisation > > > > so that the typical use (i.e. unset) always flags a successful completion. > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > --- > > > > drivers/dma-buf/dma-fence.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > > > index 3444f293ad4a..9130f790ebf3 100644 > > > > --- a/drivers/dma-buf/dma-fence.c > > > > +++ b/drivers/dma-buf/dma-fence.c > > > > @@ -534,6 +534,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, > > > > fence->context = context; > > > > fence->seqno = seqno; > > > > fence->flags = 0UL; > > > > + fence->status = 0; > > > > > > > > trace_dma_fence_init(fence); > > > > } > > > > > > > > > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > Looking at existing users there's only the sync_file stuff. And that gets > > it wrong, because afaics no one ever sets fence->status to anything > > useful. But sync_file blindly assumes it's correct. > > In terms of doc, sync_file is using it correctly, and dma-fence isn't > living up to its doc. The documented behaviour (sync_file) seems useful. Yeah, it looks like an oversight in merging sync_file and dma_fence together, but instead of piecemeal fixing (like this patch does), fixing it all&properly might be better. -Daniel
On Wed, Jan 04, 2017 at 10:37:32AM +0100, Daniel Vetter wrote: > On Wed, Jan 04, 2017 at 09:24:27AM +0000, Chris Wilson wrote: > > On Wed, Jan 04, 2017 at 10:15:01AM +0100, Daniel Vetter wrote: > > > On Tue, Jan 03, 2017 at 02:04:44PM +0000, Tvrtko Ursulin wrote: > > > > > > > > On 03/01/2017 11:05, Chris Wilson wrote: > > > > > As the fence->status is an optional field that may be set before > > > > > dma_fence_signal() is called to convey that the fence completed with an > > > > > error, we have to ensure that it is always set to zero on initialisation > > > > > so that the typical use (i.e. unset) always flags a successful completion. > > > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > --- > > > > > drivers/dma-buf/dma-fence.c | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > > > > index 3444f293ad4a..9130f790ebf3 100644 > > > > > --- a/drivers/dma-buf/dma-fence.c > > > > > +++ b/drivers/dma-buf/dma-fence.c > > > > > @@ -534,6 +534,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, > > > > > fence->context = context; > > > > > fence->seqno = seqno; > > > > > fence->flags = 0UL; > > > > > + fence->status = 0; > > > > > > > > > > trace_dma_fence_init(fence); > > > > > } > > > > > > > > > > > > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > > > Looking at existing users there's only the sync_file stuff. And that gets > > > it wrong, because afaics no one ever sets fence->status to anything > > > useful. But sync_file blindly assumes it's correct. > > > > In terms of doc, sync_file is using it correctly, and dma-fence isn't > > living up to its doc. The documented behaviour (sync_file) seems useful. > > Yeah, it looks like an oversight in merging sync_file and dma_fence > together, but instead of piecemeal fixing (like this patch does), fixing > it all&properly might be better. What's missing? * @status: Optional, only valid if < 0, must be set before calling * dma_fence_signal, indicates that the fence has completed with an * error. dma-fence must then guarantee that is zeroed during init, then the drivers can supply the optional information prior to calling dma_fence_signal() A dma_fence_set_status(fence, status) { BUG_ON(test_bit(SIGNALED, &fence->flags); BUG_ON(!IS_ERR_VALUE(status)); BUG_ON(fence->status); fence->status = status; } ? -Chris
On Wed, Jan 04, 2017 at 09:43:51AM +0000, Chris Wilson wrote: > On Wed, Jan 04, 2017 at 10:37:32AM +0100, Daniel Vetter wrote: > > On Wed, Jan 04, 2017 at 09:24:27AM +0000, Chris Wilson wrote: > > > On Wed, Jan 04, 2017 at 10:15:01AM +0100, Daniel Vetter wrote: > > > > On Tue, Jan 03, 2017 at 02:04:44PM +0000, Tvrtko Ursulin wrote: > > > > > > > > > > On 03/01/2017 11:05, Chris Wilson wrote: > > > > > > As the fence->status is an optional field that may be set before > > > > > > dma_fence_signal() is called to convey that the fence completed with an > > > > > > error, we have to ensure that it is always set to zero on initialisation > > > > > > so that the typical use (i.e. unset) always flags a successful completion. > > > > > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > --- > > > > > > drivers/dma-buf/dma-fence.c | 1 + > > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > > > > > index 3444f293ad4a..9130f790ebf3 100644 > > > > > > --- a/drivers/dma-buf/dma-fence.c > > > > > > +++ b/drivers/dma-buf/dma-fence.c > > > > > > @@ -534,6 +534,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, > > > > > > fence->context = context; > > > > > > fence->seqno = seqno; > > > > > > fence->flags = 0UL; > > > > > > + fence->status = 0; > > > > > > > > > > > > trace_dma_fence_init(fence); > > > > > > } > > > > > > > > > > > > > > > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > > > > > Looking at existing users there's only the sync_file stuff. And that gets > > > > it wrong, because afaics no one ever sets fence->status to anything > > > > useful. But sync_file blindly assumes it's correct. > > > > > > In terms of doc, sync_file is using it correctly, and dma-fence isn't > > > living up to its doc. The documented behaviour (sync_file) seems useful. > > > > Yeah, it looks like an oversight in merging sync_file and dma_fence > > together, but instead of piecemeal fixing (like this patch does), fixing > > it all&properly might be better. > > What's missing? > > * @status: Optional, only valid if < 0, must be set before calling > * dma_fence_signal, indicates that the fence has completed with an > * error. > > dma-fence must then guarantee that is zeroed during init, then the > drivers can supply the optional information prior to calling > dma_fence_signal() > > A dma_fence_set_status(fence, status) { > BUG_ON(test_bit(SIGNALED, &fence->flags); > BUG_ON(!IS_ERR_VALUE(status)); > BUG_ON(fence->status); > fence->status = status; > } ? The trouble I'm seeing is that sync_file.c and sync_debug.c _only_ look at fence->status, and assume that statue > 0 means the fence is singalled. That code doesn't check fence->flags at all ... So maybe we need to rename status to error_status to make it clear it's for failure modes only, and fix the sync_file code to look at flags, too. Please maybe even some userspace tests to sweeten the deal? Perhaps in our hangstats tests. -Daniel
On Wed, Jan 04, 2017 at 11:18:58AM +0100, Daniel Vetter wrote: > On Wed, Jan 04, 2017 at 09:43:51AM +0000, Chris Wilson wrote: > > On Wed, Jan 04, 2017 at 10:37:32AM +0100, Daniel Vetter wrote: > > > On Wed, Jan 04, 2017 at 09:24:27AM +0000, Chris Wilson wrote: > > > > On Wed, Jan 04, 2017 at 10:15:01AM +0100, Daniel Vetter wrote: > > > > > On Tue, Jan 03, 2017 at 02:04:44PM +0000, Tvrtko Ursulin wrote: > > > > > > > > > > > > On 03/01/2017 11:05, Chris Wilson wrote: > > > > > > > As the fence->status is an optional field that may be set before > > > > > > > dma_fence_signal() is called to convey that the fence completed with an > > > > > > > error, we have to ensure that it is always set to zero on initialisation > > > > > > > so that the typical use (i.e. unset) always flags a successful completion. > > > > > > > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > > --- > > > > > > > drivers/dma-buf/dma-fence.c | 1 + > > > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > > > > > > index 3444f293ad4a..9130f790ebf3 100644 > > > > > > > --- a/drivers/dma-buf/dma-fence.c > > > > > > > +++ b/drivers/dma-buf/dma-fence.c > > > > > > > @@ -534,6 +534,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, > > > > > > > fence->context = context; > > > > > > > fence->seqno = seqno; > > > > > > > fence->flags = 0UL; > > > > > > > + fence->status = 0; > > > > > > > > > > > > > > trace_dma_fence_init(fence); > > > > > > > } > > > > > > > > > > > > > > > > > > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > > > > > > > Looking at existing users there's only the sync_file stuff. And that gets > > > > > it wrong, because afaics no one ever sets fence->status to anything > > > > > useful. But sync_file blindly assumes it's correct. > > > > > > > > In terms of doc, sync_file is using it correctly, and dma-fence isn't > > > > living up to its doc. The documented behaviour (sync_file) seems useful. > > > > > > Yeah, it looks like an oversight in merging sync_file and dma_fence > > > together, but instead of piecemeal fixing (like this patch does), fixing > > > it all&properly might be better. > > > > What's missing? > > > > * @status: Optional, only valid if < 0, must be set before calling > > * dma_fence_signal, indicates that the fence has completed with an > > * error. > > > > dma-fence must then guarantee that is zeroed during init, then the > > drivers can supply the optional information prior to calling > > dma_fence_signal() > > > > A dma_fence_set_status(fence, status) { > > BUG_ON(test_bit(SIGNALED, &fence->flags); > > BUG_ON(!IS_ERR_VALUE(status)); > > BUG_ON(fence->status); > > fence->status = status; > > } ? > > The trouble I'm seeing is that sync_file.c and sync_debug.c _only_ look at > fence->status, and assume that statue > 0 means the fence is singalled. > That code doesn't check fence->flags at all ... sync_file: sync_fill_fence_info() { if (dma_fence_is_signaled(fence)) info->status = fence->status >= 0 ? 1 : fence->status; else info->status = 0; } The no_fences: path is confusing since it sets info.status, but that's a different struct entirely. sync_debug: sync_print_fence() { status = 1; if (dma_fence_is_is_signaled_locked(fence)) status = fence->status; That's backwards... > So maybe we need to rename status to error_status to make it clear it's > for failure modes only, and fix the sync_file code to look at flags, too. It is already ^^. > Please maybe even some userspace tests to sweeten the deal? Perhaps in our > hangstats tests. I am already querying the error code in igt. -Chris
On Wed, Jan 04, 2017 at 10:26:49AM +0000, Chris Wilson wrote: > On Wed, Jan 04, 2017 at 11:18:58AM +0100, Daniel Vetter wrote: > > On Wed, Jan 04, 2017 at 09:43:51AM +0000, Chris Wilson wrote: > > > On Wed, Jan 04, 2017 at 10:37:32AM +0100, Daniel Vetter wrote: > > > > On Wed, Jan 04, 2017 at 09:24:27AM +0000, Chris Wilson wrote: > > > > > On Wed, Jan 04, 2017 at 10:15:01AM +0100, Daniel Vetter wrote: > > > > > > On Tue, Jan 03, 2017 at 02:04:44PM +0000, Tvrtko Ursulin wrote: > > > > > > > > > > > > > > On 03/01/2017 11:05, Chris Wilson wrote: > > > > > > > > As the fence->status is an optional field that may be set before > > > > > > > > dma_fence_signal() is called to convey that the fence completed with an > > > > > > > > error, we have to ensure that it is always set to zero on initialisation > > > > > > > > so that the typical use (i.e. unset) always flags a successful completion. > > > > > > > > > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > > > --- > > > > > > > > drivers/dma-buf/dma-fence.c | 1 + > > > > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > > > > > > > index 3444f293ad4a..9130f790ebf3 100644 > > > > > > > > --- a/drivers/dma-buf/dma-fence.c > > > > > > > > +++ b/drivers/dma-buf/dma-fence.c > > > > > > > > @@ -534,6 +534,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, > > > > > > > > fence->context = context; > > > > > > > > fence->seqno = seqno; > > > > > > > > fence->flags = 0UL; > > > > > > > > + fence->status = 0; > > > > > > > > > > > > > > > > trace_dma_fence_init(fence); > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > > > > > > > > > Looking at existing users there's only the sync_file stuff. And that gets > > > > > > it wrong, because afaics no one ever sets fence->status to anything > > > > > > useful. But sync_file blindly assumes it's correct. > > > > > > > > > > In terms of doc, sync_file is using it correctly, and dma-fence isn't > > > > > living up to its doc. The documented behaviour (sync_file) seems useful. > > > > > > > > Yeah, it looks like an oversight in merging sync_file and dma_fence > > > > together, but instead of piecemeal fixing (like this patch does), fixing > > > > it all&properly might be better. > > > > > > What's missing? > > > > > > * @status: Optional, only valid if < 0, must be set before calling > > > * dma_fence_signal, indicates that the fence has completed with an > > > * error. > > > > > > dma-fence must then guarantee that is zeroed during init, then the > > > drivers can supply the optional information prior to calling > > > dma_fence_signal() > > > > > > A dma_fence_set_status(fence, status) { > > > BUG_ON(test_bit(SIGNALED, &fence->flags); > > > BUG_ON(!IS_ERR_VALUE(status)); > > > BUG_ON(fence->status); > > > fence->status = status; > > > } ? > > > > The trouble I'm seeing is that sync_file.c and sync_debug.c _only_ look at > > fence->status, and assume that statue > 0 means the fence is singalled. > > That code doesn't check fence->flags at all ... > > sync_file: > sync_fill_fence_info() { > if (dma_fence_is_signaled(fence)) > info->status = fence->status >= 0 ? 1 : fence->status; > else > info->status = 0; > } > > The no_fences: path is confusing since it sets info.status, but that's a > different struct entirely. Ah right, coffee didn't properly kick in this morning ;-) > sync_debug: > sync_print_fence() { > status = 1; > if (dma_fence_is_is_signaled_locked(fence)) > status = fence->status; > > That's backwards... Yeah, that's the one I've meant. But it's for debug only > > So maybe we need to rename status to error_status to make it clear it's > > for failure modes only, and fix the sync_file code to look at flags, too. > > It is already ^^. > > > Please maybe even some userspace tests to sweeten the deal? Perhaps in our > > hangstats tests. > > I am already querying the error code in igt. Excellent, and since sync_file works that explains why igt works ;-) Can you pls spin some patch to fix up sync_debug, so I can pull it all in? And I still think s/status/error_status/ would help clarity a lot. And then maybe even the dma_fence_set_error_status helper from above. -Daniel
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 3444f293ad4a..9130f790ebf3 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -534,6 +534,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, fence->context = context; fence->seqno = seqno; fence->flags = 0UL; + fence->status = 0; trace_dma_fence_init(fence); }
As the fence->status is an optional field that may be set before dma_fence_signal() is called to convey that the fence completed with an error, we have to ensure that it is always set to zero on initialisation so that the typical use (i.e. unset) always flags a successful completion. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/dma-buf/dma-fence.c | 1 + 1 file changed, 1 insertion(+)