Quoting Govindapillai, Vinod (2026-01-07 09:10:12-03:00)
>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!
We might need some global atomic state handling for this...
>
>>
>> > + }
>> > +
>> > + 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!
Well, the logic guarded by the if condition is for a workaround, so I
think it would be better to check if the workaround is valid.
Yes, today intel_display_wa(display, 22014263786) will be true if
display->platform.dg2 is also true, but IMO only using
display->platform.dg2 by itself has the following issues:
* It is not explicit in the code that the condition is for a workaround
(although a comment can help).
* In the future, if, for some yet unknown reason,
intel_display_wa(display, 22014263786) would return false for DG2, we
would still end up applying logic specific to the workaround, which
would be undesired. (A hypothetical example would be if we add some
debugfs support for enabling/disabling display workarounds and the
user decides to disable it.)
--
Gustavo Sousa
>
>>
>> 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
>> >
>
