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

[John Harrison]

On 6/11/2024 14:14, Michal Wajdeczko wrote:
> 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)
Sure. Although if this is in global space rather than confined to a 
specific driver then shouldn't it be intel_guc_wa_klv_keys?

>
> [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
But there is advantage to keeping things correctly typed at the software 
level before you get to the binary interface. E.g. you have a helper 
function specifically for working with w/a KLVs that will produce a 
compile time warning if passed a scheduler policy key by accident.

>
> 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
You can put #defines in the middle of an enum. Or you can quite easily 
and cleanly prefix the relevant defines with a comment to say which key 
they belong to.

Personally, I find the way that H2G/G2H action definitions are scattered 
through multiple files and hugely spaced out within those files makes it 
extremely difficult to work with them. The original single, concise enum 
of all actions was so much easier to use.

>
>> 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
But the later you do that conversion, the safer your code is.

>
> and hopefully there will be no clash when those WA keys will be used
> where we use another named enum for KLV keys
I'm not following. You are saying that there could be a clash because 
0x9002 is used for a w/a key and also for a policy key, for example? Or 
that there could be two intel_guc_wa_klv_key enums that are used for 
different platforms?

John.

>
>> John.
>>
>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> Cc: John Harrison <John.C.Harrison at Intel.com>
>>> Cc: Badal Nilawar <badal.nilawar at 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 */