[PATCH v5 0/5] Use drm_clflush* instead of clflush

[PATCH v5 0/5] Use drm_clflush* instead of clflush

[Michael Cheng]

This patch series re-work a few i915 functions to use drm_clflush_virt_range
instead of calling clflush or clflushopt directly. This will prevent errors
when building for non-x86 architectures.

v2: s/PAGE_SIZE/sizeof(value) for Re-work intel_write_status_page and added
more patches to convert additional clflush/clflushopt to use drm_clflush*.
(Michael Cheng)

v3: Drop invalidate_csb_entries and directly invoke drm_clflush_virt_ran

v4: Remove extra memory barriers 

v5: s/cache_clflush_range/drm_clflush_virt_range

Michael Cheng (5):
  drm/i915/gt: Re-work intel_write_status_page
  drm/i915/gt: Drop invalidate_csb_entries
  drm/i915/gt: Re-work reset_csb
  drm/i915/: Re-work clflush_write32
  drm/i915/gt: replace cache_clflush_range

 .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |  8 +++-----
 drivers/gpu/drm/i915/gt/gen8_ppgtt.c          | 12 ++++++------
 drivers/gpu/drm/i915/gt/intel_engine.h        | 13 ++++---------
 .../drm/i915/gt/intel_execlists_submission.c  | 19 ++++++-------------
 drivers/gpu/drm/i915/gt/intel_gtt.c           |  2 +-
 drivers/gpu/drm/i915/gt/intel_ppgtt.c         |  2 +-
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  2 +-
 7 files changed, 22 insertions(+), 36 deletions(-)

-- 
2.25.1

[PATCH v5 1/5] drm/i915/gt: Re-work intel_write_status_page

[Michael Cheng]

Re-work intel_write_status_page to use drm_clflush_virt_range. This
will prevent compiler errors when building for non-x86 architectures.

Signed-off-by: Michael Cheng <michael.cheng@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine.h | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index 0e353d8c2bc8..986777c2430d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -4,6 +4,7 @@
 
 #include <asm/cacheflush.h>
 #include <drm/drm_util.h>
+#include <drm/drm_cache.h>
 
 #include <linux/hashtable.h>
 #include <linux/irq_work.h>
@@ -143,15 +144,9 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
 	 * of extra paranoia to try and ensure that the HWS takes the value
 	 * we give and that it doesn't end up trapped inside the CPU!
 	 */
-	if (static_cpu_has(X86_FEATURE_CLFLUSH)) {
-		mb();
-		clflush(&engine->status_page.addr[reg]);
-		engine->status_page.addr[reg] = value;
-		clflush(&engine->status_page.addr[reg]);
-		mb();
-	} else {
-		WRITE_ONCE(engine->status_page.addr[reg], value);
-	}
+	drm_clflush_virt_range(&engine->status_page.addr[reg], sizeof(value));
+	WRITE_ONCE(engine->status_page.addr[reg], value);
+	drm_clflush_virt_range(&engine->status_page.addr[reg], sizeof(value));
 }
 
 /*
-- 
2.25.1

[PATCH v5 2/5] drm/i915/gt: Drop invalidate_csb_entries

[Michael Cheng]

Drop invalidate_csb_entries and directly call drm_clflush_virt_range.
This allows for one less function call, and prevent complier errors when
building for non-x86 architectures.

v2(Michael Cheng): Drop invalidate_csb_entries function and directly
		   invoke drm_clflush_virt_range. Thanks to Tvrtko for the
		   sugguestion.

Signed-off-by: Michael Cheng <michael.cheng@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index 9bb7c863172f..7500c06562da 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -1646,12 +1646,6 @@ cancel_port_requests(struct intel_engine_execlists * const execlists,
 	return inactive;
 }
 
-static void invalidate_csb_entries(const u64 *first, const u64 *last)
-{
-	clflush((void *)first);
-	clflush((void *)last);
-}
-
 /*
  * Starting with Gen12, the status has a new format:
  *
@@ -1999,7 +1993,7 @@ process_csb(struct intel_engine_cs *engine, struct i915_request **inactive)
 	 * the wash as hardware, working or not, will need to do the
 	 * invalidation before.
 	 */
-	invalidate_csb_entries(&buf[0], &buf[num_entries - 1]);
+	drm_clflush_virt_range(&buf[0], num_entries * sizeof(buf[0]));
 
 	/*
 	 * We assume that any event reflects a change in context flow
@@ -2783,8 +2777,8 @@ static void reset_csb_pointers(struct intel_engine_cs *engine)
 
 	/* Check that the GPU does indeed update the CSB entries! */
 	memset(execlists->csb_status, -1, (reset_value + 1) * sizeof(u64));
-	invalidate_csb_entries(&execlists->csb_status[0],
-			       &execlists->csb_status[reset_value]);
+	drm_clflush_virt_range(&execlists->csb_status[0],
+				sizeof(&execlists->csb_status[reset_value]));
 
 	/* Once more for luck and our trusty paranoia */
 	ENGINE_WRITE(engine, RING_CONTEXT_STATUS_PTR,
-- 
2.25.1

Re: [PATCH v5 2/5] drm/i915/gt: Drop invalidate_csb_entries

[Tvrtko Ursulin]

On 04/02/2022 16:37, Michael Cheng wrote:
> Drop invalidate_csb_entries and directly call drm_clflush_virt_range.
> This allows for one less function call, and prevent complier errors when
> building for non-x86 architectures.
> 
> v2(Michael Cheng): Drop invalidate_csb_entries function and directly
> 		   invoke drm_clflush_virt_range. Thanks to Tvrtko for the
> 		   sugguestion.
> 
> Signed-off-by: Michael Cheng <michael.cheng@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index 9bb7c863172f..7500c06562da 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -1646,12 +1646,6 @@ cancel_port_requests(struct intel_engine_execlists * const execlists,
>   	return inactive;
>   }
>   
> -static void invalidate_csb_entries(const u64 *first, const u64 *last)
> -{
> -	clflush((void *)first);
> -	clflush((void *)last);
> -}
> -
>   /*
>    * Starting with Gen12, the status has a new format:
>    *
> @@ -1999,7 +1993,7 @@ process_csb(struct intel_engine_cs *engine, struct i915_request **inactive)
>   	 * the wash as hardware, working or not, will need to do the
>   	 * invalidation before.
>   	 */
> -	invalidate_csb_entries(&buf[0], &buf[num_entries - 1]);
> +	drm_clflush_virt_range(&buf[0], num_entries * sizeof(buf[0]));
>   
>   	/*
>   	 * We assume that any event reflects a change in context flow
> @@ -2783,8 +2777,8 @@ static void reset_csb_pointers(struct intel_engine_cs *engine)
>   
>   	/* Check that the GPU does indeed update the CSB entries! */
>   	memset(execlists->csb_status, -1, (reset_value + 1) * sizeof(u64));
> -	invalidate_csb_entries(&execlists->csb_status[0],
> -			       &execlists->csb_status[reset_value]);
> +	drm_clflush_virt_range(&execlists->csb_status[0],
> +				sizeof(&execlists->csb_status[reset_value]));

Hm I thought we covered this already, should be:

drm_clflush_virt_range(&execlists->csb_status[0],
		       execlists->csb_size * sizeof(execlists->csb_status[0]));

Regards,

Tvrtko

>   
>   	/* Once more for luck and our trusty paranoia */
>   	ENGINE_WRITE(engine, RING_CONTEXT_STATUS_PTR,

Re: [PATCH v5 2/5] drm/i915/gt: Drop invalidate_csb_entries

[Michael Cheng]

Ah, sorry thanks for pointing that out. We did discuss previously. I 
will go ahead and change it.

On 2022-02-07 3:57 a.m., Tvrtko Ursulin wrote:
>
> On 04/02/2022 16:37, Michael Cheng wrote:
>> Drop invalidate_csb_entries and directly call drm_clflush_virt_range.
>> This allows for one less function call, and prevent complier errors when
>> building for non-x86 architectures.
>>
>> v2(Michael Cheng): Drop invalidate_csb_entries function and directly
>>            invoke drm_clflush_virt_range. Thanks to Tvrtko for the
>>            sugguestion.
>>
>> Signed-off-by: Michael Cheng <michael.cheng@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 12 +++---------
>>   1 file changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c 
>> b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> index 9bb7c863172f..7500c06562da 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> @@ -1646,12 +1646,6 @@ cancel_port_requests(struct 
>> intel_engine_execlists * const execlists,
>>       return inactive;
>>   }
>>   -static void invalidate_csb_entries(const u64 *first, const u64 *last)
>> -{
>> -    clflush((void *)first);
>> -    clflush((void *)last);
>> -}
>> -
>>   /*
>>    * Starting with Gen12, the status has a new format:
>>    *
>> @@ -1999,7 +1993,7 @@ process_csb(struct intel_engine_cs *engine, 
>> struct i915_request **inactive)
>>        * the wash as hardware, working or not, will need to do the
>>        * invalidation before.
>>        */
>> -    invalidate_csb_entries(&buf[0], &buf[num_entries - 1]);
>> +    drm_clflush_virt_range(&buf[0], num_entries * sizeof(buf[0]));
>>         /*
>>        * We assume that any event reflects a change in context flow
>> @@ -2783,8 +2777,8 @@ static void reset_csb_pointers(struct 
>> intel_engine_cs *engine)
>>         /* Check that the GPU does indeed update the CSB entries! */
>>       memset(execlists->csb_status, -1, (reset_value + 1) * 
>> sizeof(u64));
>> -    invalidate_csb_entries(&execlists->csb_status[0],
>> -                   &execlists->csb_status[reset_value]);
>> +    drm_clflush_virt_range(&execlists->csb_status[0],
>> + sizeof(&execlists->csb_status[reset_value]));
>
> Hm I thought we covered this already, should be:
>
> drm_clflush_virt_range(&execlists->csb_status[0],
>                execlists->csb_size * sizeof(execlists->csb_status[0]));
>
> Regards,
>
> Tvrtko
>
>>         /* Once more for luck and our trusty paranoia */
>>       ENGINE_WRITE(engine, RING_CONTEXT_STATUS_PTR,

[PATCH v5 3/5] drm/i915/gt: Re-work reset_csb

[Michael Cheng]

Use drm_clflush_virt_range instead of directly invoking clflush. This
will prevent compiler errors when building for non-x86 architectures.

v2(Michael Cheng): Remove extra clflush

v3(Michael Cheng): Remove memory barrier since drm_clflush_virt_range
		   takes care of it.

Signed-off-by: Michael Cheng <michael.cheng@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index 7500c06562da..22505aa428d9 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -2944,9 +2944,8 @@ reset_csb(struct intel_engine_cs *engine, struct i915_request **inactive)
 {
 	struct intel_engine_execlists * const execlists = &engine->execlists;
 
-	mb(); /* paranoia: read the CSB pointers from after the reset */
-	clflush(execlists->csb_write);
-	mb();
+	drm_clflush_virt_range(execlists->csb_write,
+			sizeof(execlists->csb_write));
 
 	inactive = process_csb(engine, inactive); /* drain preemption events */
 
-- 
2.25.1

[PATCH v5 4/5] drm/i915/: Re-work clflush_write32

[Michael Cheng]

Use drm_clflush_virt_range instead of clflushopt and remove the memory
barrier, since drm_clflush_virt_range takes care of that.

Signed-off-by: Michael Cheng <michael.cheng@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 498b458fd784..0854276ff7ba 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1332,10 +1332,8 @@ static void *reloc_vaddr(struct i915_vma *vma,
 static void clflush_write32(u32 *addr, u32 value, unsigned int flushes)
 {
 	if (unlikely(flushes & (CLFLUSH_BEFORE | CLFLUSH_AFTER))) {
-		if (flushes & CLFLUSH_BEFORE) {
-			clflushopt(addr);
-			mb();
-		}
+		if (flushes & CLFLUSH_BEFORE)
+			drm_clflush_virt_range(addr, sizeof(addr));
 
 		*addr = value;
 
@@ -1347,7 +1345,7 @@ static void clflush_write32(u32 *addr, u32 value, unsigned int flushes)
 		 * to ensure ordering of clflush wrt to the system.
 		 */
 		if (flushes & CLFLUSH_AFTER)
-			clflushopt(addr);
+			drm_clflush_virt_range(addr, sizeof(addr));
 	} else
 		*addr = value;
 }
-- 
2.25.1

[PATCH v5 5/5] drm/i915/gt: replace cache_clflush_range

[Michael Cheng]

Replace all occurance of cache_clflush_range with
drm_clflush_virt_range. This will prevent compile errors on non-x86
platforms.

Signed-off-by: Michael Cheng <michael.cheng@intel.com>
---
 drivers/gpu/drm/i915/gt/gen8_ppgtt.c                 | 12 ++++++------
 drivers/gpu/drm/i915/gt/intel_execlists_submission.c |  2 +-
 drivers/gpu/drm/i915/gt/intel_gtt.c                  |  2 +-
 drivers/gpu/drm/i915/gt/intel_ppgtt.c                |  2 +-
 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c    |  2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
index c43e724afa9f..d0999e92621b 100644
--- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
+++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
@@ -444,11 +444,11 @@ gen8_ppgtt_insert_pte(struct i915_ppgtt *ppgtt,
 				pd = pdp->entry[gen8_pd_index(idx, 2)];
 			}
 
-			clflush_cache_range(vaddr, PAGE_SIZE);
+			drm_clflush_virt_range(vaddr, PAGE_SIZE);
 			vaddr = px_vaddr(i915_pt_entry(pd, gen8_pd_index(idx, 1)));
 		}
 	} while (1);
-	clflush_cache_range(vaddr, PAGE_SIZE);
+	drm_clflush_virt_range(vaddr, PAGE_SIZE);
 
 	return idx;
 }
@@ -532,7 +532,7 @@ static void gen8_ppgtt_insert_huge(struct i915_address_space *vm,
 			}
 		} while (rem >= page_size && index < I915_PDES);
 
-		clflush_cache_range(vaddr, PAGE_SIZE);
+		drm_clflush_virt_range(vaddr, PAGE_SIZE);
 
 		/*
 		 * Is it safe to mark the 2M block as 64K? -- Either we have
@@ -548,7 +548,7 @@ static void gen8_ppgtt_insert_huge(struct i915_address_space *vm,
 					      I915_GTT_PAGE_SIZE_2M)))) {
 			vaddr = px_vaddr(pd);
 			vaddr[maybe_64K] |= GEN8_PDE_IPS_64K;
-			clflush_cache_range(vaddr, PAGE_SIZE);
+			drm_clflush_virt_range(vaddr, PAGE_SIZE);
 			page_size = I915_GTT_PAGE_SIZE_64K;
 
 			/*
@@ -569,7 +569,7 @@ static void gen8_ppgtt_insert_huge(struct i915_address_space *vm,
 				for (i = 1; i < index; i += 16)
 					memset64(vaddr + i, encode, 15);
 
-				clflush_cache_range(vaddr, PAGE_SIZE);
+				drm_clflush_virt_range(vaddr, PAGE_SIZE);
 			}
 		}
 
@@ -617,7 +617,7 @@ static void gen8_ppgtt_insert_entry(struct i915_address_space *vm,
 
 	vaddr = px_vaddr(i915_pt_entry(pd, gen8_pd_index(idx, 1)));
 	vaddr[gen8_pd_index(idx, 0)] = gen8_pte_encode(addr, level, flags);
-	clflush_cache_range(&vaddr[gen8_pd_index(idx, 0)], sizeof(*vaddr));
+	drm_clflush_virt_range(&vaddr[gen8_pd_index(idx, 0)], sizeof(*vaddr));
 }
 
 static int gen8_init_scratch(struct i915_address_space *vm)
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index 22505aa428d9..5e74c99d5929 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -2822,7 +2822,7 @@ static void execlists_sanitize(struct intel_engine_cs *engine)
 	sanitize_hwsp(engine);
 
 	/* And scrub the dirty cachelines for the HWSP */
-	clflush_cache_range(engine->status_page.addr, PAGE_SIZE);
+	drm_clflush_virt_range(engine->status_page.addr, PAGE_SIZE);
 
 	intel_engine_reset_pinned_contexts(engine);
 }
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c
index 0d6bbc8c57f2..9b594be9102f 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -255,7 +255,7 @@ fill_page_dma(struct drm_i915_gem_object *p, const u64 val, unsigned int count)
 	void *vaddr = __px_vaddr(p);
 
 	memset64(vaddr, val, count);
-	clflush_cache_range(vaddr, PAGE_SIZE);
+	drm_clflush_virt_range(vaddr, PAGE_SIZE);
 }
 
 static void poison_scratch_page(struct drm_i915_gem_object *scratch)
diff --git a/drivers/gpu/drm/i915/gt/intel_ppgtt.c b/drivers/gpu/drm/i915/gt/intel_ppgtt.c
index 48e6e2f87700..bd474a5123cb 100644
--- a/drivers/gpu/drm/i915/gt/intel_ppgtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ppgtt.c
@@ -90,7 +90,7 @@ write_dma_entry(struct drm_i915_gem_object * const pdma,
 	u64 * const vaddr = __px_vaddr(pdma);
 
 	vaddr[idx] = encoded_entry;
-	clflush_cache_range(&vaddr[idx], sizeof(u64));
+	drm_clflush_virt_range(&vaddr[idx], sizeof(u64));
 }
 
 void
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index b3a429a92c0d..89020706adc4 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -3573,7 +3573,7 @@ static void guc_sanitize(struct intel_engine_cs *engine)
 	sanitize_hwsp(engine);
 
 	/* And scrub the dirty cachelines for the HWSP */
-	clflush_cache_range(engine->status_page.addr, PAGE_SIZE);
+	drm_clflush_virt_range(engine->status_page.addr, PAGE_SIZE);
 
 	intel_engine_reset_pinned_contexts(engine);
 }
-- 
2.25.1

Re: [PATCH v5 0/5] Use drm_clflush* instead of clflush

[Tvrtko Ursulin]

On 04/02/2022 16:37, Michael Cheng wrote:
> This patch series re-work a few i915 functions to use drm_clflush_virt_range
> instead of calling clflush or clflushopt directly. This will prevent errors
> when building for non-x86 architectures.
> 
> v2: s/PAGE_SIZE/sizeof(value) for Re-work intel_write_status_page and added
> more patches to convert additional clflush/clflushopt to use drm_clflush*.
> (Michael Cheng)
> 
> v3: Drop invalidate_csb_entries and directly invoke drm_clflush_virt_ran
> 
> v4: Remove extra memory barriers
> 
> v5: s/cache_clflush_range/drm_clflush_virt_range

Is anyone interested in this story noticing my open? I will repeat:

How about we add i915_clflush_virt_range as static inline and by doing 
so avoid adding function calls to code paths which are impossible on Arm 
builds? Case in point relocations, probably execlists backend as well.

Downside would be effectively duplicating drm_clfush_virt_range code. 
But for me, (Also considering no other driver calls it so why it is 
there? Should it be deleted?), that would be okay.

Regards,

Tvrtko

> Michael Cheng (5):
>    drm/i915/gt: Re-work intel_write_status_page
>    drm/i915/gt: Drop invalidate_csb_entries
>    drm/i915/gt: Re-work reset_csb
>    drm/i915/: Re-work clflush_write32
>    drm/i915/gt: replace cache_clflush_range
> 
>   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |  8 +++-----
>   drivers/gpu/drm/i915/gt/gen8_ppgtt.c          | 12 ++++++------
>   drivers/gpu/drm/i915/gt/intel_engine.h        | 13 ++++---------
>   .../drm/i915/gt/intel_execlists_submission.c  | 19 ++++++-------------
>   drivers/gpu/drm/i915/gt/intel_gtt.c           |  2 +-
>   drivers/gpu/drm/i915/gt/intel_ppgtt.c         |  2 +-
>   .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  2 +-
>   7 files changed, 22 insertions(+), 36 deletions(-)
> 

Re: [Intel-gfx] [PATCH v5 0/5] Use drm_clflush* instead of clflush

[Jani Nikula]

On Mon, 07 Feb 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 04/02/2022 16:37, Michael Cheng wrote:
>> This patch series re-work a few i915 functions to use drm_clflush_virt_range
>> instead of calling clflush or clflushopt directly. This will prevent errors
>> when building for non-x86 architectures.
>> 
>> v2: s/PAGE_SIZE/sizeof(value) for Re-work intel_write_status_page and added
>> more patches to convert additional clflush/clflushopt to use drm_clflush*.
>> (Michael Cheng)
>> 
>> v3: Drop invalidate_csb_entries and directly invoke drm_clflush_virt_ran
>> 
>> v4: Remove extra memory barriers
>> 
>> v5: s/cache_clflush_range/drm_clflush_virt_range
>
> Is anyone interested in this story noticing my open? I will repeat:
>
> How about we add i915_clflush_virt_range as static inline and by doing 
> so avoid adding function calls to code paths which are impossible on Arm 
> builds? Case in point relocations, probably execlists backend as well.
>
> Downside would be effectively duplicating drm_clfush_virt_range code. 
> But for me, (Also considering no other driver calls it so why it is 
> there? Should it be deleted?), that would be okay.

Keep it simple first, optimize later if necessary?

BR,
Jani.


>
> Regards,
>
> Tvrtko
>
>> Michael Cheng (5):
>>    drm/i915/gt: Re-work intel_write_status_page
>>    drm/i915/gt: Drop invalidate_csb_entries
>>    drm/i915/gt: Re-work reset_csb
>>    drm/i915/: Re-work clflush_write32
>>    drm/i915/gt: replace cache_clflush_range
>> 
>>   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |  8 +++-----
>>   drivers/gpu/drm/i915/gt/gen8_ppgtt.c          | 12 ++++++------
>>   drivers/gpu/drm/i915/gt/intel_engine.h        | 13 ++++---------
>>   .../drm/i915/gt/intel_execlists_submission.c  | 19 ++++++-------------
>>   drivers/gpu/drm/i915/gt/intel_gtt.c           |  2 +-
>>   drivers/gpu/drm/i915/gt/intel_ppgtt.c         |  2 +-
>>   .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  2 +-
>>   7 files changed, 22 insertions(+), 36 deletions(-)
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH v5 0/5] Use drm_clflush* instead of clflush

[Tvrtko Ursulin]

On 07/02/2022 12:44, Jani Nikula wrote:
> On Mon, 07 Feb 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>> On 04/02/2022 16:37, Michael Cheng wrote:
>>> This patch series re-work a few i915 functions to use drm_clflush_virt_range
>>> instead of calling clflush or clflushopt directly. This will prevent errors
>>> when building for non-x86 architectures.
>>>
>>> v2: s/PAGE_SIZE/sizeof(value) for Re-work intel_write_status_page and added
>>> more patches to convert additional clflush/clflushopt to use drm_clflush*.
>>> (Michael Cheng)
>>>
>>> v3: Drop invalidate_csb_entries and directly invoke drm_clflush_virt_ran
>>>
>>> v4: Remove extra memory barriers
>>>
>>> v5: s/cache_clflush_range/drm_clflush_virt_range
>>
>> Is anyone interested in this story noticing my open? I will repeat:
>>
>> How about we add i915_clflush_virt_range as static inline and by doing
>> so avoid adding function calls to code paths which are impossible on Arm
>> builds? Case in point relocations, probably execlists backend as well.
>>
>> Downside would be effectively duplicating drm_clfush_virt_range code.
>> But for me, (Also considering no other driver calls it so why it is
>> there? Should it be deleted?), that would be okay.
> 
> Keep it simple first, optimize later if necessary?

I don't think it would ever happen unless done from the start. :/

Regards,

Tvrtko