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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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,