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

Thu Jul 3 13:55:07 UTC 2025

On Thu, Jul 03, 2025 at 09:08:54AM -0300, Gustavo Sousa wrote:
>Quoting Ville Syrjälä (2025-07-02 18:49:30-03:00)
>>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 at intel.com>
>>> > >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at 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
>>
>>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.
>
>With regard to this last paragraph, in my experience, I have seen two
>types of situation:
>
>1. Usually we have a single w/a number that is shared accross different
>   platforms/IPs, which is what we call the lineage number in our
>   database. What happens sometimes is that people, by mistake, use the
>   platform specific ticket number instead of the w/a number.
>
>2. Another thing that happens sometimes is that we might have different
>   hw bugs that have the same workaround implementation. That is the
>   legitimate case of having our code mapping two or more w/a numbers to
>   the same implementation.

well... but this is the same mitigation for different bugs. They are not
duplicate bugs. It could be that the platforms affected are even
different. We should mark both as implemented to be able to cross check
what we have implemented in the drivers vs the list of workarounds.

Lucas De Marchi

>
>--
>Gustavo Sousa