On Mon, 2026-01-05 at 14:44 -0300, Gustavo Sousa wrote:
> Quoting Vinod Govindapillai (2026-01-05 07:48:58-03:00)
> > 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. As for
> > DG2 this wa condition is based on the number of active planes, the
> > check is moved to the fbc post plane update calls. The force slb
> > invalidation chicken bit is set/unset based on the condition. For
> > the other platforms where this wa is valid, the wa applied before
> > enabling the FBC Unconditionally as before.
> >
> > v2: wrong version send as the initial patchset
> > for DG2, active planes check should be done all pipes not just
> > the FBC pipe (Matt)
> >
> > Bspec: 54077, 72197
> > Signed-off-by: Vinod Govindapillai <[email protected]>
> > ---
> > drivers/gpu/drm/i915/display/intel_fbc.c | 62
> > ++++++++++++++++++++++--
> > 1 file changed, 59 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index 155b308ed66f..b01f9622510e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -915,6 +915,15 @@ static void intel_fbc_program_cfb(struct
> > intel_fbc *fbc)
> > fbc->funcs->program_cfb(fbc);
> > }
> >
> > +static void fbc_slb_invalidation_wa(struct intel_fbc *fbc,
> > + bool force_invalidation)
> > +{
> > + u32 set = force_invalidation ?
> > DPFC_CHICKEN_FORCE_SLB_INVALIDATION : 0;
> > + u32 clear = force_invalidation ? 0 :
> > DPFC_CHICKEN_FORCE_SLB_INVALIDATION;
> > +
> > + intel_de_rmw(fbc->display, ILK_DPFC_CHICKEN(fbc->id),
> > clear, set);
> > +}
> > +
> > static void intel_fbc_program_workarounds(struct intel_fbc *fbc)
> > {
> > struct intel_display *display = fbc->display;
> > @@ -946,10 +955,13 @@ 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. This wa criteria
> > is assessed
> > + * seprately on every post plane update call
> > to check if
> > + * the number of active planes condition is
> > met.
> > */
> > - if (intel_display_wa(display, 22014263786))
> > - intel_de_rmw(display, ILK_DPFC_CHICKEN(fbc->id),
> > - 0,
> > DPFC_CHICKEN_FORCE_SLB_INVALIDATION);
> > + if (intel_display_wa(display, 22014263786) && !display-
> > >platform.dg2)
> > + fbc_slb_invalidation_wa(fbc, true);
> >
> > /* wa_18038517565 Disable DPFC clock gating before FBC
> > enable */
> > if (display->platform.dg2 || DISPLAY_VER(display) >= 14)
> > @@ -1887,13 +1899,57 @@ static void __intel_fbc_post_update(struct
> > intel_fbc *fbc)
> > intel_fbc_activate(fbc);
> > }
> >
> > +static void
> > +dg2_fbc_update_slb_invalidation(const struct intel_atomic_state
> > *state)
> > +{
> > + struct intel_display *display = to_intel_display(state);
> > + int i, num_active_planes = 0;
> > + struct intel_crtc_state *crtc_state;
> > + struct intel_crtc *crtc;
> > + enum intel_fbc_id fbc_id;
> > +
> > + for_each_new_intel_crtc_in_state(state, crtc, crtc_state,
> > i) {
> > + u8 active_planes;
> > +
> > + if (crtc->pipe != PIPE_A && crtc->pipe != PIPE_B)
> > + continue;
> > +
> > + active_planes = crtc_state->active_planes &
> > ~BIT(PLANE_CURSOR);
> > + num_active_planes += hweight8(active_planes);
>
> I don't think this really counts the total number of active non-
> cursor
> planes in pipes A and B. What if this display commit is for only one
> of
> those pipes and the other one is already enabled with a non-zero
> non-cursor plane?
hmm.. yeah! thanks for pointing that out.
May be for_each_intel_crtc_in_pipe_mask() could be used. But then it
doesnt make sense to have this in the fbc post plane update as it gets
called each crtc in the state. May be a new call is needed from
atomic_commit_tail() just for this! Need to check!
>
> > + }
> > +
> > + for_each_fbc_id(display, fbc_id) {
> > + struct intel_fbc *fbc = display-
> > >fbc.instances[fbc_id];
> > +
> > + mutex_lock(&fbc->lock);
> > +
> > + if (fbc->state.plane)
> > + fbc_slb_invalidation_wa(fbc,
> > num_active_planes > 1);
> > +
> > + mutex_unlock(&fbc->lock);
> > + }
> > +}
> > +
> > void intel_fbc_post_update(struct intel_atomic_state *state,
> > struct intel_crtc *crtc)
> > {
> > + struct intel_display *display = to_intel_display(state);
> > const struct intel_plane_state __maybe_unused *plane_state;
> > struct intel_plane *plane;
> > int i;
> >
> > + /*
> > + * 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. This wa criteria
> > is assessed
> > + * seprately on every post plane update call
> > to check if
> > + * the number of active planes condition is
> > met.
> > + */
> > + if (display->platform.dg2)
>
> I think this should be
>
> if (intel_display_wa(display, 22014263786) && display-
> >platform.dg2)
>
> , for consistency.
Not sure if that add any value!
>
> Also, we probably should drop one of the comments to avoid two
> duplicated descriptions of the workaround; maybe drop this one.
Just wanted to have that wa_id mentioned here! May be I can get rid if
the description of the wa, but keep just the wa:id!
Thanks
Vinod
>
> --
> Gustavo Sousa
>
> > + dg2_fbc_update_slb_invalidation(state);
> > +
> > for_each_new_intel_plane_in_state(state, plane,
> > plane_state, i) {
> > struct intel_fbc *fbc = plane->fbc;
> >
> > --
> > 2.43.0
> >
