Re: [PATCH v2 1/1] drm/xe/xe2: Send flat_ccs value in struct drm_xe_query_config

["Jahagirdar, Akshata" <akshata.jahagirdar@intel.com>]

On 3/26/2024 1:25 AM, Matthew Auld wrote:
> On 26/03/2024 10:10, Akshata Jahagirdar wrote:
>> Currently, we don't have any check in IGT testcases if platform supports
>> compression or not. Send this flat_ccs value as part of the struct
>> drm_xe_query_config so that we can read this value in the igt testcases
>> and proceed accordingly.
>
> AFAIK it's not valid to add uAPI just for an IGT. uAPI normally needs 
> a real open source user (like Mesa etc) with UMD acks. However if this 
> is going to be used by Mesa etc, how do they tell the difference 
> between "this device doesn't have flat-CCS" and "this KMD version 
> doesn't support FLAG_HAS_FLAT_CCS"?
>
Yeah, maybe we can add some checks using helper functions of debugfs to 
see if the file actually contains "flat-ccs" line.
> If userspace doesn't need this, then other option could be debugfs. Or 
> if this just a single test perhaps we can just dynamically check if 
> the CCS state is zero for compressed buffer at the start of the test?
>
I got your point, I will try to explore the debugfs and other 
suggestions that you provided.

I thought this was a better/cleaner solution than debugfs, but didn't 
realize the actual uAPI use-case here.

Thank you for your review.
-Akshata

>> Fixes: VLK-56136
>>
>> Signed-off-by: Akshata Jahagirdar <akshata.jahagirdar@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_query.c | 3 +++
>>   include/uapi/drm/xe_drm.h     | 1 +
>>   2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_query.c 
>> b/drivers/gpu/drm/xe/xe_query.c
>> index e80321b34918..8a3a3117491c 100644
>> --- a/drivers/gpu/drm/xe/xe_query.c
>> +++ b/drivers/gpu/drm/xe/xe_query.c
>> @@ -332,6 +332,9 @@ static int query_config(struct xe_device *xe, 
>> struct drm_xe_device_query *query)
>>       config->num_params = num_params;
>>       config->info[DRM_XE_QUERY_CONFIG_REV_AND_DEVICE_ID] =
>>           xe->info.devid | (xe->info.revid << 16);
>> +    if (xe->info.has_flat_ccs)
>> +        config->info[DRM_XE_QUERY_CONFIG_FLAGS] =
>> +            DRM_XE_QUERY_CONFIG_FLAG_HAS_FLAT_CCS;
>>       if (xe_device_get_root_tile(xe)->mem.vram.usable_size)
>>           config->info[DRM_XE_QUERY_CONFIG_FLAGS] =
>
> This tramples the previous value AFAICT, like when we have VRAM + CCS 
> on DG2. I think we need |= here.
>
>> DRM_XE_QUERY_CONFIG_FLAG_HAS_VRAM;
>> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>> index f44a19ae9333..08783234bae1 100644
>> --- a/include/uapi/drm/xe_drm.h
>> +++ b/include/uapi/drm/xe_drm.h
>> @@ -405,6 +405,7 @@ struct drm_xe_query_config {
>>   #define DRM_XE_QUERY_CONFIG_REV_AND_DEVICE_ID    0
>>   #define DRM_XE_QUERY_CONFIG_FLAGS            1
>>       #define DRM_XE_QUERY_CONFIG_FLAG_HAS_VRAM    (1 << 0)
>> +    #define DRM_XE_QUERY_CONFIG_FLAG_HAS_FLAT_CCS    (1 << 1)
>>   #define DRM_XE_QUERY_CONFIG_MIN_ALIGNMENT        2
>>   #define DRM_XE_QUERY_CONFIG_VA_BITS            3
>>   #define DRM_XE_QUERY_CONFIG_MAX_EXEC_QUEUE_PRIORITY    4