On Tue, 2025-12-16 at 15:44 -0800, Matt Roper wrote:
> On Tue, Dec 16, 2025 at 04:05:08PM +0200, Vinod Govindapillai wrote:
> > For DG2, wa_22014263786 is applicable only if the number of active
> > planes is greater than 1 in pipe A and pipe B. Cursor planes and
> > any planes on pipe C or pipe D are not considered for this.
> >
> > Bspec: 54077, 72197
> > Signed-off-by: Vinod Govindapillai <[email protected]>
> > ---
> > .../gpu/drm/i915/display/intel_display_wa.c | 29
> > ++++++++++++++++++-
> > drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++++
> > 2 files changed, 34 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 a00af39f7538..ffc2356283aa 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_wa.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_wa.c
> > @@ -7,6 +7,7 @@
> >
> > #include "i915_reg.h"
> > #include "intel_de.h"
> > +#include "intel_display_types.h"
> > #include "intel_display_core.h"
> > #include "intel_display_regs.h"
> > #include "intel_display_wa.h"
> > @@ -53,6 +54,32 @@ static bool
> > intel_display_needs_wa_16025573575(struct intel_display *display)
> > DISPLAY_VERx100(display) == 3500;
> > }
> >
> > +static bool intel_display_needs_wa_22014263786(struct
> > intel_display *display)
> > +{
> > + if (!IS_DISPLAY_VERx100(display, 1100, 1400))
> > + return false;
> > +
> > + if (display->platform.dg2) {
> > + u8 pipe_mask = PIPE_A | PIPE_B;
> > + int num_active_planes = 0;
> > + struct intel_crtc *crtc;
> > +
> > + for_each_intel_crtc_in_pipe_mask(display->drm,
> > crtc, pipe_mask) {
> > + const struct intel_crtc_state *crtc_state
> > =
> > + to_intel_crtc_state(crtc-
> > >base.state);
> > + u8 active_planes =
> > + crtc_state->active_planes &
> > ~BIT(PLANE_CURSOR);
> > +
> > + num_active_planes +=
> > hweight8(active_planes);
> > + }
> > +
> > + if (num_active_planes <= 1)
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > /*
> > * Wa_14011503117:
> > * Fixes: Before enabling the scaler DE fatal error is masked
> > @@ -69,7 +96,7 @@ bool __intel_display_wa(struct intel_display
> > *display, enum intel_display_wa wa,
> > case INTEL_DISPLAY_WA_14011503117:
> > return DISPLAY_VER(display) == 13;
> > case INTEL_DISPLAY_WA_22014263786:
> > - return IS_DISPLAY_VERx100(display, 1100, 1400);
> > + return
> > intel_display_needs_wa_22014263786(display);
> > case INTEL_DISPLAY_WA_15018326506:
> > return display->platform.battlemage;
> > case INTEL_DISPLAY_WA_14025769978:
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index fef2f35ff1e9..5b0a83cb5386 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -944,10 +944,16 @@ static void
> > intel_fbc_program_workarounds(struct intel_fbc *fbc)
> > * Wa_22014263786
> > * Fixes: Screen flicker with FBC and Package C state
> > enabled
> > * Workaround: Forced SLB invalidation before start of new
> > frame.
> > + * For DG2, wa is applied only if the number
> > of planes
> > + * in PIPE A and PIPE B is > 1. If the wa
> > condition is
> > + * not met, this chicken bit must be unset for
> > DG2.
>
> I might be misremembering the FBC flows in the driver now, but I
> don't
> think we come back to this point and re-evaluate the workaround once
> FBC
> has been enabled as other planes get enabled/disabled by later atomic
> transactions do we? E.g., if we initially just have a single plane
> enabled on pipes A/B, we'll apply the workaround as expected. But
> then
> if we later turn on an additional plane are we going to notice that
> the
> conditions have changed and that we now need to disable the
> workaround?
> Or alternatively if we have two planes turned on initially (so
> workaround not applied), but then we disable the one that FBC isn't
> bound to are we going to notice that we need to come back and apply
> the
> workaround?
>
>
> Matt
Hi Matt,
Ah.. sorry. I sent a wrong version for review :( This one is an earlier
version and later I clarified from HW team about resetting that bit in
case of plane count reduces. Earlier the assumption was that this bit
need to be set before enabling the fbc.
Well, in the one I was supposed to sent, this WA was moved as part of
intel_plane_update_noarm() like below so that it gets called for each
plane updates.
--- a/drivers/gpu/drm/i915/display/intel_plane.c
+++ b/drivers/gpu/drm/i915/display/intel_plane.c
@@ -856,9 +856,13 @@ void intel_plane_update_noarm(struct intel_dsb
*dsb,
trace_intel_plane_update_noarm(plane_state, crtc);
- if (plane->fbc)
+ if (plane->fbc){
intel_fbc_dirty_rect_update_noarm(dsb, plane);
+ /* TODO: check if group these into
intel_fbc_update_noarm()? */
+ intel_fbc_handle_workaround(dsb, plane);
+ }
+
But I see that this will still miss the other plane updates as this
will be called only for the plane update for the plane with the fbc.
I will need to recheck this still!
May be split the wa into parts where intel_wa_22014263786() checks on
the non DG2 case and call that as part of the program_workaround() as
it is now. And later on if (dg2 && number of planes in pipe A and B)
set/reset this bit from intel_atomic_commit_tail()?
Thanks
Vinod
>
> > */
> > if (intel_display_wa(display, 22014263786))
> > intel_de_rmw(display, ILK_DPFC_CHICKEN(fbc->id),
> > 0,
> > DPFC_CHICKEN_FORCE_SLB_INVALIDATION);
> > + else if (display->platform.dg2)
> > + intel_de_rmw(display, ILK_DPFC_CHICKEN(fbc->id),
> > + DPFC_CHICKEN_FORCE_SLB_INVALIDATION,
> > 0);
> >
> > /* wa_18038517565 Disable DPFC clock gating before FBC
> > enable */
> > if (display->platform.dg2 || DISPLAY_VER(display) >= 14)
> > --
> > 2.43.0
> >
>
