Re: [Intel-gfx] [PATCH v2 4/4] drm/i915/: Re-work clflush_write32

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

On 28/01/2022 22:10, Michael Cheng wrote:
> 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;
>   }

Slightly annoying thing here (maybe in some other patches from the series as well) is that the change adds a function call to x86 only code path, because relocations are not supported on discrete as per:

static in
eb_validate_vma(...)
         /* Relocations are disallowed for all platforms after TGL-LP.  This
          * also covers all platforms with local memory.
          */

         if (entry->relocation_count &&
             GRAPHICS_VER(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915))
                 return -EINVAL;

How acceptable would be, for the whole series, to introduce a static inline i915 cluflush wrapper and so be able to avoid functions calls on x86? Is this something that has been discussed and discounted already?

Regards,

Tvrtko

P.S. Hmm I am now reminded of my really old per platform build patches. With them you would be able to compile out large portions of the driver when building for ARM. Probably like a 3rd if my memory serves me right.