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

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Nautiyal, Ankit K (2025-07-03 06:30:19-03:00)
>
>On 7/3/2025 3:19 AM, Ville Syrjälä wrote:
>> On Thu, Jul 03, 2025 at 12:29:37AM +0300, Ville Syrjälä wrote:
>>> On Wed, Jul 02, 2025 at 03:25:21PM -0500, Lucas De Marchi wrote:
>>>> On Wed, Jul 02, 2025 at 10:40:34PM +0300, Ville Syrjälä wrote:
>>>>> On Wed, Jul 02, 2025 at 02:16:18PM +0530, Ankit Nautiyal wrote:
>>>>>> 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);
>>>>>> +                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,
>>>>> How is anyone supposed to keep track of these random numbers
>>>>> and what they mean?
>>>> they mean there's a h/w workaround that requires that and this is the id
>>>> if you need to find more details about it or what platforms/IPs use
>>>> that.
>>> I don't want to go look up all the details in the common case.
>>> I just want to read the code and see that it generally makes
>>> sense without having to trawl through the spec/hsd for an
>>> hour every time.
>>>
>>>>> The only time I want to see these numbers is if I really have to
>>>>> open the spec/hsd for it to double check some details. Othwerwise
>>>>> it just seems like pointless noise that makes it harder to follow
>>>>> the code/figure out what the heck is going on.
>>>> what is the alternative? The current status quo checking by platform
>>>> and/or IP version, dissociated from the WA numbers?
>>> I find it easiest if everything is in one place. I think every
>>> w/a generally should have these:
>>> - which hardware is affected
>>> - what other runtime conditions are required to hit the issue
>>> - what is being done to avoid the issue
>>> - a short human readable explanation of the issue
>>> - the w/a number for looking up futher details
>>>
>>> Splitting it all up into random bits and pieces just means more
>>> jumping around all the time, which I find annoying at best.
>> I suppose one could argue for a more formal thing for these three:
>> - which hardware is affected
>> - a short human readable explanation of the issue
>> - the w/a number for looking up futher details
>
>Whether adding comments with platform and relevant information about Wa 
>would be sufficient?
>
>Something like:
>
>/*
>  * Wa_16025573575: PTL/WCL

I would not add the ": PTL/WCL" here. The information is already in the
function body and, based on what we have seen on i915, it is easy for
those getting out of sync with the conditions in the code if people are
not careful.

Also, PTL/WCL would not be very accurate: the workaround applies to the
display IP (which could get re-used on another platform) and not the
platform itself.

--
Gustavo Sousa

>  * Fix issue with bitbashing on PTL.
>  * Set masks bits in GPIO CTL and preserve it during bitbashing sequence.
>  */
>static bool intel_display_needs_wa_16025573575(struct intel_display 
>*display)
>{
>         return DISPLAY_VER(display) == 30;
>}
>
>Or we want to have some wa_struct with fields for platforms and stuff?
>
>
>Regards,
>
>Ankit
>
>>
>> Might be still a real pain to deal with that due to having to jump
>> around, but at least it could be used to force people to document
>> each w/a a bit better.
>>
>> Basically anything that avoids having to wait for the spec/hsd to
>> load is a good thing in my book.
>>
>> There's also the question of what to do with duplicates, as in often
>> it seems the same issue is present on multiple platforms under different
>> w/a numbers.
>>