Op 21-10-2021 om 16:43 schreef Matthew Auld:
> On Thu, 21 Oct 2021 at 11:36, Maarten Lankhorst
> <maarten.lankhorst@xxxxxxxxxxxxxxx> wrote:
>> The signaled bit is already used for quick testing if a fence is signaled.
> Why do we need this change? Can you add some more details to the commit please?
It's a terrible abuse of dma-fence api, and in the common case where the object is already locked by the caller, the trylock will fail.
If it were useful, the core dma-api would have exposed the same functionality.
On top of that, the fact that i915 has a dma_resv_utils.c file should be a warning that the functionality either belongs in core, or is not very useful at all. In this case the latter.
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/Makefile | 1 -
>> drivers/gpu/drm/i915/dma_resv_utils.c | 17 -----------------
>> drivers/gpu/drm/i915/dma_resv_utils.h | 13 -------------
>> drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 3 ---
>> drivers/gpu/drm/i915/gem/i915_gem_wait.c | 8 --------
>> 5 files changed, 42 deletions(-)
>> delete mode 100644 drivers/gpu/drm/i915/dma_resv_utils.c
>> delete mode 100644 drivers/gpu/drm/i915/dma_resv_utils.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index 467872cca027..b87e3ed10d86 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -60,7 +60,6 @@ i915-y += i915_drv.o \
>>
>> # core library code
>> i915-y += \
>> - dma_resv_utils.o \
>> i915_memcpy.o \
>> i915_mm.o \
>> i915_sw_fence.o \
>> diff --git a/drivers/gpu/drm/i915/dma_resv_utils.c b/drivers/gpu/drm/i915/dma_resv_utils.c
>> deleted file mode 100644
>> index 7df91b7e4ca8..000000000000
>> --- a/drivers/gpu/drm/i915/dma_resv_utils.c
>> +++ /dev/null
>> @@ -1,17 +0,0 @@
>> -// SPDX-License-Identifier: MIT
>> -/*
>> - * Copyright © 2020 Intel Corporation
>> - */
>> -
>> -#include <linux/dma-resv.h>
>> -
>> -#include "dma_resv_utils.h"
>> -
>> -void dma_resv_prune(struct dma_resv *resv)
>> -{
>> - if (dma_resv_trylock(resv)) {
>> - if (dma_resv_test_signaled(resv, true))
>> - dma_resv_add_excl_fence(resv, NULL);
>> - dma_resv_unlock(resv);
>> - }
>> -}
>> diff --git a/drivers/gpu/drm/i915/dma_resv_utils.h b/drivers/gpu/drm/i915/dma_resv_utils.h
>> deleted file mode 100644
>> index b9d8fb5f8367..000000000000
>> --- a/drivers/gpu/drm/i915/dma_resv_utils.h
>> +++ /dev/null
>> @@ -1,13 +0,0 @@
>> -/* SPDX-License-Identifier: MIT */
>> -/*
>> - * Copyright © 2020 Intel Corporation
>> - */
>> -
>> -#ifndef DMA_RESV_UTILS_H
>> -#define DMA_RESV_UTILS_H
>> -
>> -struct dma_resv;
>> -
>> -void dma_resv_prune(struct dma_resv *resv);
>> -
>> -#endif /* DMA_RESV_UTILS_H */
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
>> index 5ab136ffdeb2..af3eb7fd951d 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
>> @@ -15,7 +15,6 @@
>>
>> #include "gt/intel_gt_requests.h"
>>
>> -#include "dma_resv_utils.h"
>> #include "i915_trace.h"
>>
>> static bool swap_available(void)
>> @@ -229,8 +228,6 @@ i915_gem_shrink(struct i915_gem_ww_ctx *ww,
>> i915_gem_object_unlock(obj);
>> }
>>
>> - dma_resv_prune(obj->base.resv);
> Like here, why do we want to drop this? Later in the series it looks
> like it gets added back, just in a slightly different form.
>
>> -
>> scanned += obj->base.size >> PAGE_SHIFT;
>> skip:
>> i915_gem_object_put(obj);
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
>> index 840c13706999..1592d95c3ead 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
>> @@ -10,7 +10,6 @@
>>
>> #include "gt/intel_engine.h"
>>
>> -#include "dma_resv_utils.h"
>> #include "i915_gem_ioctls.h"
>> #include "i915_gem_object.h"
>>
>> @@ -52,13 +51,6 @@ i915_gem_object_wait_reservation(struct dma_resv *resv,
>> }
>> dma_resv_iter_end(&cursor);
>>
>> - /*
>> - * Opportunistically prune the fences iff we know they have *all* been
>> - * signaled.
>> - */
>> - if (timeout > 0)
>> - dma_resv_prune(resv);
>> -
>> return ret;
>> }
>>
>> --
>> 2.33.0
>>
