Re: [PATCH v2] drm/i915/fbc: update the impacted platforms in wa_22014263786

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 01, 2025 at 08:19:23PM +0300, Ville Syrjälä wrote:
> On Wed, Oct 01, 2025 at 01:20:20PM +0000, Govindapillai, Vinod wrote:
> > + I had Lucas' email id wrong. Fixed.
> > 
> > On Wed, 2025-10-01 at 16:10 +0300, Jani Nikula wrote:
> > > On Wed, 01 Oct 2025, "Govindapillai, Vinod" <vinod.govindapillai@intel.com> wrote:
> > > > On Wed, 2025-10-01 at 13:03 +0300, Jani Nikula wrote:
> > > > > On Wed, 01 Oct 2025, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> > > > > > wa_22014263786 is not applicable to the BMG and hence exclude it
> > > > > > from the wa.
> > > > > > 
> > > > > > v2: Limit this wa to display verion 11 to 14, drop DG2 from the
> > > > > >     exclusion list, use intel_display_wa (Lucas)
> > > > > > 
> > > > > > Bspec: 74212, 66624
> > > > > > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/display/intel_display_wa.c | 12 ++++++++++++
> > > > > >  drivers/gpu/drm/i915/display/intel_display_wa.h |  1 +
> > > > > >  drivers/gpu/drm/i915/display/intel_fbc.c        |  3 +--
> > > > > >  3 files changed, 14 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_display_wa.c
> > > > > > index 31cd2c9cd488..7ca238725e30 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_wa.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_wa.c
> > > > > > @@ -52,6 +52,16 @@ static bool intel_display_needs_wa_16025573575(struct intel_display
> > > > > > *display)
> > > > > >  	return DISPLAY_VERx100(display) == 3000 || DISPLAY_VERx100(display) == 3002;
> > > > > >  }
> > > > > >  
> > > > > > +/*
> > > > > > + * Wa_22014263786:
> > > > > > + * Fixes: Screen flicker with FBC and Package C state enabled
> > > > > > + * Workaround: Forced SLB invalidation before start of new frame.
> > > > > > + */
> > > > > > +static bool intel_display_needs_wa_22014263786(struct intel_display *display)
> > > > > > +{
> > > > > > +	return DISPLAY_VERx100(display) >= 1100 && DISPLAY_VERx100(display) < 1401;
> > > > > 
> > > > > IS_DISPLAY_VERx100(display, 1100, 1400)
> > > > > 
> > > > > Also I'm not sure if we really need separate functions for one-liners
> > > > > like this. The documentation could be in the switch case too? *shrug*.
> > > > 
> > > > Thanks. I will update that. I will get rid of the comments. I dont think it adds any extra
> > > > information other than what it can be found from wa details.
> > > 
> > > But I did not say we should get rid of the comments! We don't want to
> > > make people look up the wa details, because it's not publicly available
> > > information.
> > > 
> > > I'm just wondering if we need the separate *function*.
> > 
> > I got that part. I thought I will remove the comments along with that change! Originally this wa
> > also did not have much information as comments other than the impacted platforms. 
> > 
> > Okay. Will wait for Ville's feedback before floating another version
> 
> I think production w/a should generally be included in the public PRMs.
> But I don't enjoy looking them up in either internal or public source.
> 
> What I would like to see described is the symptoms that are getting
> fixed, and whther those symptoms occur under specific circumstancs
> (esp. if we decided to apply the w/a as a bigger hammer without
> actually checking for those specific circumstances). That sort of
> information can be very useful when you're seeing some problem and
> pondering whether you might be missing some existing w/a.
> 
> Very low level details aren't probably particularly useful even to
> us. To really understand those you probably have to trawl the hsd(s)
> anyway.
> 
> So after some pondering I'm thinking the explanations should perhaps
> still stay where the w/a is implemented, and this w/a list would
> _only_ contain the platform checks. And I would also want to be able
> to immediately jump from the actual code to the relevant w/a platform
> check (as in with cscope/etc). Though that's still a somewhat annoying
> extra step or two, esp. since I can't directly jump to the defition of
> the enum value but rather need to jump to one of its uses in
> __intel_displa_wa() :/ I suppose if all of these were functions without
> the enum step in the middle that problem would be solved. But dunno if
> we want to have a function per w/a number?

The other annoyance is that soemtimes the same w/a lives under
a different number for each platform. What should we do with those?

Oh, and we do also have the unsolved problem of what to do with
w/as that predate the hsd based numbering (either using the skl
style w/a numbers or simply no numbers).

And we also have some undocumented w/as as well (eg. issues we
found long after the hw people had moved on).

-- 
Ville Syrjälä
Intel