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

[John Harrison <john.c.harrison@intel.com>]

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?

> 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.
Having a named type gives you an extra level of compiler type checks 
that you don't get with a u16.

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 */