Re: [PATCH] drm/xe: Use ttm_uncached for BO with NEEDS_UC flag

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

Hi Thomas,

On 11.06.2024 14:47, Thomas Hellström wrote:
> Hi, Michal,
> 
> On Thu, 2024-06-06 at 21:56 +0200, Michal Wajdeczko wrote:
>> We should honor requested uncached mode also at the TTM layer.
>> Otherwise, we risk losing updates to the memory based interrupts
>> source or status vectors, as those require uncached memory.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_bo.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>> index 2bae01ce4e5b..2573cc118f29 100644
>> --- a/drivers/gpu/drm/xe/xe_bo.c
>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>> @@ -378,6 +378,9 @@ static struct ttm_tt *xe_ttm_tt_create(struct
>> ttm_buffer_object *ttm_bo,
>>  	    (xe->info.graphics_verx100 >= 1270 && bo->flags &
>> XE_BO_FLAG_PAGETABLE))
>>  		caching = ttm_write_combined;
>>  
>> +	if (bo->flags & XE_BO_FLAG_NEEDS_UC)
>> +		caching = ttm_uncached;
>> +
>>  	err = ttm_tt_init(&tt->ttm, &bo->ttm, page_flags, caching,
>> extra_pages);
>>  	if (err) {
>>  		kfree(tt);
> 
> To me the preferred method is to teach bo->cpu_caching about the
> uncached mode and then include it in the switch statement above.
> 

but bo->cpu_caching is currently documented as:

/**
 * @cpu_caching: CPU caching mode. Currently only used for userspace
 * objects.
 */

and value 0 is implicitly reserved as kind of default, so 'teaching'
would likely mean either extending uapi with something like:

  #define DRM_XE_GEM_CPU_CACHING_WB                      1
  #define DRM_XE_GEM_CPU_CACHING_WC                      2
+ #define DRM_XE_GEM_CPU_CACHING_UC                      3

which will introduce lot of undesired right now code changes, or we will
introduce internal only flag:

+ #define XE_CPU_CACHING_UC                      ((u16)~0)

but that doesn't look like a clean solution.


OTOH, just above this new diff chunk, there is already a code that
updates caching mode outside the "switch statement above":

	if ((!bo->cpu_caching && bo->flags & XE_BO_FLAG_SCANOUT) ||
	    (xe->info.graphics_verx100 >= 1270 &&
	     bo->flags & XE_BO_FLAG_PAGETABLE))
		caching = ttm_write_combined;

so maybe as a short term solution we can keep this patch as it's doing
similar last resort stuff and return to 'preferred way' later:

	if (!bo->cpu_caching && bo->flags & XE_BO_FLAG_NEEDS_UC)
		caching = ttm_uncached;

Michal