Re: [PATCH 4/4] drm/xe/guc: Avoid named enum types in GuC KLV ABI

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

On 11.06.2024 22:16, John Harrison wrote:
> On 6/11/2024 11:07, Michal Wajdeczko wrote:
>> The xe_guc_klv_ids name just for workaround keys is too generic,
> So rename it to xe_guc_wa_klv_ids?

but the idea is to make GuC ABI headers shareable between i915 and xe,
see [1], thus having driver specific prefix in the enum is not desired

what about guc_wa_klv_keys (as we have KEY not ID in the KLV)

[1] https://patchwork.freedesktop.org/series/134722/

> 
>> as workaround keys are not the only keys used by the GuC firmware.
>> Other KLVs keys are defined as plain defines or anonymous enums.
> Just because other instances are done badly doesn't mean we should break
> the good one to match.

all these are ABI definitions so IMO there is nothing wrong with using
plain defines for values expected at the binary interface

also using defines allows to have similarly named related definitions
that we could all place together (like KEY and LEN for each KLV), which
would be hard to achieve if we will have separate enums for those

> Having a named type gives you an extra level of compiler type checks
> that you don't get with a u16.

fine, but at the end you need to convert to u16 anyway

and hopefully there will be no clash when those WA keys will be used
where we use another named enum for KLV keys

> 
> John.
> 
> 
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> Cc: Badal Nilawar <badal.nilawar@intel.com>
>> ---
>>   drivers/gpu/drm/xe/abi/guc_klvs_abi.h | 2 +-
>>   drivers/gpu/drm/xe/xe_guc_ads.c       | 4 ++--
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
>> b/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
>> index a0b80c9fc5ef..831a2b57f308 100644
>> --- a/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
>> +++ b/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
>> @@ -346,7 +346,7 @@ enum {
>>   /*
>>    * Workaround keys:
>>    */
>> -enum xe_guc_klv_ids {
>> +enum {
>>      
>> GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED                =
>> 0x9002,
>>       GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING               
>> = 0x9005,
>>      
>> GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE               
>> = 0x9007,
>> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c
>> b/drivers/gpu/drm/xe/xe_guc_ads.c
>> index 1c60b685dbc6..a3de00e92b4e 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
>> @@ -285,7 +285,7 @@ static size_t calculate_golden_lrc_size(struct
>> xe_guc_ads *ads)
>>   }
>>     static void guc_waklv_enable_one_word(struct xe_guc_ads *ads,
>> -                      enum xe_guc_klv_ids klv_id,
>> +                      u16 klv_id,
>>                         u32 value,
>>                         u32 *offset, u32 *remain)
>>   {
>> @@ -312,7 +312,7 @@ static void guc_waklv_enable_one_word(struct
>> xe_guc_ads *ads,
>>   }
>>     static void guc_waklv_enable_simple(struct xe_guc_ads *ads,
>> -                    enum xe_guc_klv_ids klv_id, u32 *offset, u32
>> *remain)
>> +                    u16 klv_id, u32 *offset, u32 *remain)
>>   {
>>       u32 klv_entry[] = {
>>           /* 16:16 key/length */
>