Re: [PATCH 1/2] drm/i915/display_wa: Add helpers to check wa

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 7/2/2025 7:42 PM, Jani Nikula wrote:
> On Wed, 02 Jul 2025, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> Quoting Ankit Nautiyal (2025-07-02 05:46:18-03:00)
>>> Introduce a generic helper to check display workarounds using an enum.
>>>
>>> Convert Wa_16023588340 to use the new interface, simplifying WA checks
>>> and making future additions easier.
>>>
>>> v2: Use drm_WARN instead of MISSING_CASE and simplify intel_display_wa
>>> macro. (Jani)
>>>
>>> Suggested-by: Jani Nikula <jani.nikula@intel.com>
>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/display/intel_display_wa.c | 15 +++++++++++++++
>>> drivers/gpu/drm/i915/display/intel_display_wa.h |  9 +++++++++
>>> drivers/gpu/drm/i915/display/intel_fbc.c        |  2 +-
>>> 3 files changed, 25 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.c b/drivers/gpu/drm/i915/display/intel_display_wa.c
>>> index f57280e9d041..f5e8d58d9a68 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_wa.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_wa.c
>>> @@ -3,6 +3,8 @@
>>>   * Copyright © 2023 Intel Corporation
>>>   */
>>>
>>> +#include "drm/drm_print.h"
>>> +
>>> #include "i915_reg.h"
>>> #include "intel_de.h"
>>> #include "intel_display_core.h"
>>> @@ -39,3 +41,16 @@ void intel_display_wa_apply(struct intel_display *display)
>>>          else if (DISPLAY_VER(display) == 11)
>>>                  gen11_display_wa_apply(display);
>>> }
>>> +
>>> +bool __intel_display_wa(struct intel_display *display, enum intel_display_wa wa)
>>> +{
>>> +        switch (wa) {
>>> +        case INTEL_DISPLAY_WA_16023588340:
>>> +                return intel_display_needs_wa_16023588340(display);
>>> +        default:
>>> +                drm_WARN(display->drm, 1, "Missing Wa number: %d\n", wa);
>> Hm... I wonder how useful the message would be if we just show the enum
>> value. For example, if the next workaround that we added was
>> INTEL_DISPLAY_WA_99999999999 and we had it missing here, I think we
>> would get the following warning message:
>>
>>      "Missing Wa number: 1"
>>
>> Perhaps the enum identifier could be found in the callstack that is
>> presented with the warning, but I'm wondering if we could do better
>> here.
>>
>> Not sure there is a good solution without requiring extra memory to map
>> each enum member to its corresponding the workaround number.
> The solution would be to make the function:
>
> bool __intel_display_wa(struct intel_display *display, enum intel_display_wa wa, const char *name);
>
> and the macro:
>
> #define intel_display_wa(__display, __wa) \
> 	__intel_display_wa((__display), INTEL_DISPLAY_WA_##__wa, __stringify(__wa))
>
> and then you could debug log the name.
>
> Worth it? Not sure.

Hmm... there might be some value to have some mechanism to print the WA 
name.

While it might not add much value in this particular case, but it could 
be helpful in other failure scenario,

where identifying the exact WA name might give clue while debugging.

Regards,

Ankit

>
>
> BR,
> Jani.
>
>
>> --
>> Gustavo Sousa
>>
>>> +                break;
>>> +        }
>>> +
>>> +        return false;
>>> +}
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h b/drivers/gpu/drm/i915/display/intel_display_wa.h
>>> index babd9d16603d..146ee70d66f7 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_wa.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h
>>> @@ -21,4 +21,13 @@ static inline bool intel_display_needs_wa_16023588340(struct intel_display *disp
>>> bool intel_display_needs_wa_16023588340(struct intel_display *display);
>>> #endif
>>>
>>> +enum intel_display_wa {
>>> +        INTEL_DISPLAY_WA_16023588340,
>>> +};
>>> +
>>> +bool __intel_display_wa(struct intel_display *display, enum intel_display_wa wa);
>>> +
>>> +#define intel_display_wa(__display, __wa) \
>>> +        __intel_display_wa((__display), INTEL_DISPLAY_WA_##__wa)
>>> +
>>> #endif
>>> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
>>> index 6e26cb4c5724..e2e03af520b2 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
>>> @@ -1464,7 +1464,7 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state,
>>>                  return 0;
>>>          }
>>>
>>> -        if (intel_display_needs_wa_16023588340(display)) {
>>> +        if (intel_display_wa(display, 16023588340)) {
>>>                  plane_state->no_fbc_reason = "Wa_16023588340";
>>>                  return 0;
>>>          }
>>> -- 
>>> 2.45.2
>>>