| Message ID | 20250424130254.42046-6-phasta@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/nouveau: Simplify nouveau_fence.c | expand |
On Mon, 2025-04-28 at 16:45 +0200, Christian König wrote: > On 4/24/25 15:02, Philipp Stanner wrote: > > In nouveau_fence_done(), a fence is checked for being signaled by > > manually evaluating the base fence's bits. This can be done in a > > canonical manner through dma_fence_is_signaled(). > > > > Replace the bit-check with dma_fence_is_signaled(). > > > > Signed-off-by: Philipp Stanner <phasta@kernel.org> > > > I think the bit check was used here as fast path optimization because > we later call dma_fence_is_signaled() anyway. That fast path optimization effectively saves one JMP instruction to the function. I'm increasingly of the opinion that we shall work towards all DRM users only ever using infrastructure through officially documented API functions, without touching internal data structures. > Feel free to add my acked-by, but honestly what nouveau does here > looks rather suspicious to me. :) P. > > Regards, > Christian. > > > --- > > drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c > > b/drivers/gpu/drm/nouveau/nouveau_fence.c > > index fb9811938c82..d5654e26d5bc 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > > @@ -253,7 +253,7 @@ nouveau_fence_done(struct nouveau_fence *fence) > > struct nouveau_channel *chan; > > unsigned long flags; > > > > - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence- > > >base.flags)) > > + if (dma_fence_is_signaled(&fence->base)) > > return true; > > > > spin_lock_irqsave(&fctx->lock, flags); >
On 5/8/25 11:13, Philipp Stanner wrote: > On Mon, 2025-04-28 at 16:45 +0200, Christian König wrote: >> On 4/24/25 15:02, Philipp Stanner wrote: >>> In nouveau_fence_done(), a fence is checked for being signaled by >>> manually evaluating the base fence's bits. This can be done in a >>> canonical manner through dma_fence_is_signaled(). >>> >>> Replace the bit-check with dma_fence_is_signaled(). >>> >>> Signed-off-by: Philipp Stanner <phasta@kernel.org> >> >> >> I think the bit check was used here as fast path optimization because >> we later call dma_fence_is_signaled() anyway. > > That fast path optimization effectively saves one JMP instruction to > the function. What I meant was that we might completely drop that optimization. It looks like overkill and potentially hides bugs. Regards, Christian. > > I'm increasingly of the opinion that we shall work towards all DRM > users only ever using infrastructure through officially documented API > functions, without touching internal data structures. > >> Feel free to add my acked-by, but honestly what nouveau does here >> looks rather suspicious to me. > > :) > > > P. > >> >> Regards, >> Christian. >> >>> --- >>> drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c >>> b/drivers/gpu/drm/nouveau/nouveau_fence.c >>> index fb9811938c82..d5654e26d5bc 100644 >>> --- a/drivers/gpu/drm/nouveau/nouveau_fence.c >>> +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c >>> @@ -253,7 +253,7 @@ nouveau_fence_done(struct nouveau_fence *fence) >>> struct nouveau_channel *chan; >>> unsigned long flags; >>> >>> - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence- >>>> base.flags)) >>> + if (dma_fence_is_signaled(&fence->base)) >>> return true; >>> >>> spin_lock_irqsave(&fctx->lock, flags); >> >
diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c index fb9811938c82..d5654e26d5bc 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fence.c +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c @@ -253,7 +253,7 @@ nouveau_fence_done(struct nouveau_fence *fence) struct nouveau_channel *chan; unsigned long flags; - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags)) + if (dma_fence_is_signaled(&fence->base)) return true; spin_lock_irqsave(&fctx->lock, flags);
In nouveau_fence_done(), a fence is checked for being signaled by manually evaluating the base fence's bits. This can be done in a canonical manner through dma_fence_is_signaled(). Replace the bit-check with dma_fence_is_signaled(). Signed-off-by: Philipp Stanner <phasta@kernel.org> --- drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)