[Intel-xe] [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro

[Nilawar, Badal]

On 23-09-2023 00:33, Rodrigo Vivi wrote:
> On Fri, Sep 22, 2023 at 05:16:23PM +0200, Andi Shyti wrote:
>> Hi Rodrigo,
>>
>> On Thu, Sep 21, 2023 at 12:03:26PM -0400, Rodrigo Vivi wrote:
>>> On Thu, Sep 21, 2023 at 03:55:14PM +0530, Badal Nilawar wrote:
>>>> Add XE_MISSING_CASE macro to handle missing switch case
>>>>
>>>> v2: Add comment about macro usage (Himal)
>>>>
>>>> Cc: Andi Shyti <andi.shyti at linux.intel.com>
>>>> Signed-off-by: Badal Nilawar <badal.nilawar at intel.com>
>>>> Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
>>>> ---
>>>>   drivers/gpu/drm/xe/xe_macros.h | 4 ++++
>>>>   1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h
>>>> index daf56c846d03..6c74c69920ed 100644
>>>> --- a/drivers/gpu/drm/xe/xe_macros.h
>>>> +++ b/drivers/gpu/drm/xe/xe_macros.h
>>>> @@ -15,4 +15,8 @@
>>>>   			    "Ioctl argument check failed at %s:%d: %s", \
>>>>   			    __FILE__, __LINE__, #cond), 1))
>>>>   
>>>> +/* Parameter to macro should be a variable name */
>>>> +#define XE_MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
>>>> +				__stringify(x), (long)(x))
>>>> +
>>>
>>> No, please! Let's not add unnecessary macros.
>>
>> it's not a bad idea, actually... in i915 we have the MISSING_CASE
>> for switch cases with a defined number of cases and print
>> warnings in case none if them is the one inside the switch.
>>
>> It's so widely used and actually nice to have that I thought many
>> times to move it to some core kernel libraries.
> 
> to be really honest, I also liked the MISSING_CASE in many occasions.
> It is useful for platform enabling so once we add a new hardware we
> ensure to get some splats on the first power-on and can easily
> identify the missing cases.
> 
> But even in i915 I have already seen some miss-usages of that.
> And i915 is full of i915isms that we believe it is good but we
> never tried to move to core kernel or when we tried we got some push
> backs showing that that is not very common and desired way.
> 
> I know, just looking around Xe is also starting to get Xeisms,
> but I'd like to avoid that as much as we can. in this case here
> it is only 2 lines that could be a standard drm_warn or similar call.
> 
> I don't believe that it is worth adding a macro for this. So, maybe
> if we really want this the right approach is to move the i915's one
> to core kernel and then make Xe to start using it.
Agreed, for this use case I will also prefer to go with drm_warn.

Regards,
Badal
> 
>>
>> Andi