diff mbox series

[03/28] drm/i915: Remove dma_resv_prune

Message ID 20211021103605.735002-3-maarten.lankhorst@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [01/28] drm/i915: Fix i915_request fence wait semantics | expand

Commit Message

Maarten Lankhorst Oct. 21, 2021, 10:35 a.m. UTC 
The signaled bit is already used for quick testing if a fence is signaled.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 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

Comments

Matthew Auld Oct. 21, 2021, 2:43 p.m. UTC | #1 
On Thu, 21 Oct 2021 at 11:36, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> 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?

>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  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
>
Maarten Lankhorst Oct. 22, 2021, 8:48 a.m. UTC | #2 
Op 21-10-2021 om 16:43 schreef Matthew Auld:
> On Thu, 21 Oct 2021 at 11:36, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> 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@linux.intel.com>
>> ---
>>  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
>>
diff mbox series

Patch

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);
-
 			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;
 }