On Wed, 2025-10-15 at 18:11 +0300, Jani Nikula wrote:
> On Wed, 15 Oct 2025, Gustavo Sousa <[email protected]> wrote:
> > From: Vinod Govindapillai <[email protected]>
> >
> > To enable FBC for FP16 formats, we need to enable the pixel
> > normalizer
> > block. Introduce the register definitions and the initial steps for
> > configuring the pixel normalizer block. In this patch the pixel
> > normalizer block is kept as disabled. The follow-up patches will
> > handle
> > configuring the pixel normalizer block for hdr planes for FP16
> > formats.
> >
> > Bspec: 69863
> > Cc: Shekhar Chauhan <[email protected]>
> > Signed-off-by: Vinod Govindapillai <[email protected]>
> > Signed-off-by: Gustavo Sousa <[email protected]>
> > ---
> > drivers/gpu/drm/i915/display/intel_display_types.h | 3 +++
> > drivers/gpu/drm/i915/display/skl_universal_plane.c | 15
> > +++++++++++++++
> > drivers/gpu/drm/i915/display/skl_universal_plane_regs.h | 11
> > +++++++++++
> > 3 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 87b7cec35320..13652e2996a4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -679,6 +679,9 @@ struct intel_plane_state {
> > /* surface address register */
> > u32 surf;
> >
> > + /* plane pixel normalizer config for Xe3p_LPD+ FBC FP16 */
> > + u32 pixel_normalizer;
>
> I'm pretty strongly of the opinion that software state should not be
> just a 1:1 map of the hardware state. Software state is logical,
> hardware state is physical.
>
> The software state should have logical things like "normalize form
> factor" and "normalize enable", and the hardware then has those in
> some
> register(s) somewhere.
>
> Please let's not start storing software state as register contents.
> The
> registers and their contents *will* change across platforms.
Thanks for the comments. Got your point.
Well.. in this case we are forced to enable the pixel notmalizer with
factor as 1 because of the FBC hw block requirement. So far we don't
have any separate logic to decide on the normalization factor. We hard
code the normalization factor as 1 for display version >= 35 and FP16
format the plane is FBC capable.
So now I am wondering, if we need to maintain a state variable for that
- just directly program register with normalization factor as 1.0 +
enable for FP16 formats?
if (disp >= 35) {
if (fp16 format && FBC capable plane)
intel_de_write_dsb(factor_1_0 | enable)
else
intel_de_write_ds(0)
}
Do you have any suggestion on this?
BR
Vinod
>
> > +
> > /*
> > * scaler_id
> > * = -1 : not using a scaler
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 9f1111324dab..16a9c141281b 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -893,6 +893,12 @@ static void skl_write_plane_wm(struct
> > intel_dsb *dsb,
> >
> > xe3_plane_min_ddb_reg_val(min_ddb, interim_ddb));
> > }
> >
> > +static void
> > +xe3p_lpd_plane_check_pixel_normalizer(struct intel_plane_state
> > *plane_state)
>
> The function name has nothing to do with what the function does. What
> does "check" mean?
>
> > +{
> > + plane_state->pixel_normalizer = 0;
> > +}
> > +
> > static void
> > skl_plane_disable_arm(struct intel_dsb *dsb,
> > struct intel_plane *plane,
> > @@ -1671,6 +1677,11 @@ icl_plane_update_arm(struct intel_dsb *dsb,
> >
> > icl_plane_update_sel_fetch_arm(dsb, plane, crtc_state,
> > plane_state);
> >
> > + /* Only the HDR planes can have pixel normalizer */
> > + if (DISPLAY_VER(display) >= 35 &&
> > icl_is_hdr_plane(display, plane_id))
> > + intel_de_write_dsb(display, dsb,
> > + PLANE_PIXEL_NORMALIZE(plane-
> > >pipe, plane->id),
> > + plane_state->pixel_normalizer);
>
> This is the place where you'd look at the software state, and
> construct
> what will be written to hardware based on that.
>
> > /*
> > * The control register self-arms if the plane was
> > previously
> > * disabled. Try to make the plane enable atomic by
> > writing
> > @@ -2385,6 +2396,10 @@ static int skl_plane_check(struct
> > intel_crtc_state *crtc_state,
> > plane_state->damage = DRM_RECT_INIT(0, 0, 0, 0);
> > }
> >
> > + /* Pixel normalizer for Xe3p_LPD+ */
> > + if (DISPLAY_VER(display) >= 35 &&
> > icl_is_hdr_plane(display, plane->id))
> > + xe3p_lpd_plane_check_pixel_normalizer(plane_state)
> > ;
> > +
> > plane_state->ctl = skl_plane_ctl(plane_state);
> >
> > if (DISPLAY_VER(display) >= 10)
> > diff --git
> > a/drivers/gpu/drm/i915/display/skl_universal_plane_regs.h
> > b/drivers/gpu/drm/i915/display/skl_universal_plane_regs.h
> > index 84cf565bd653..11c713f9b237 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane_regs.h
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane_regs.h
> > @@ -456,4 +456,15 @@
> > _S
> > EL_FETCH_PLANE_OFFSET_5_A, _SEL_FETCH_PLANE_OFFSET_5_B, \
> > _S
> > EL_FETCH_PLANE_OFFSET_6_A, _SEL_FETCH_PLANE_OFFSET_6_B)
> >
> > +#define _PLANE_PIXEL_NORMALIZE_1_A 0x701a8
> > +#define _PLANE_PIXEL_NORMALIZE_2_A 0x702a8
> > +#define _PLANE_PIXEL_NORMALIZE_1_B 0x711a8
> > +#define _PLANE_PIXEL_NORMALIZE_2_B 0x712a8
> > +#define PLANE_PIXEL_NORMALIZE(pipe,
> > plane) _MMIO_SKL_PLANE((pipe), (plane), \
> > + _P
> > LANE_PIXEL_NORMALIZE_1_A, _PLANE_PIXEL_NORMALIZE_1_B, \
> > + _P
> > LANE_PIXEL_NORMALIZE_2_A, _PLANE_PIXEL_NORMALIZE_2_B)
> > +#define
> > PLANE_PIXEL_NORMALIZE_ENABLE REG_BIT(31)
> > +#define
> > PLANE_PIXEL_NORMALIZE_NORM_FACTOR_MASK REG_GENMASK(15, 0)
> > +#define
> > PLANE_PIXEL_NORMALIZE_NORM_FACTOR(val)
> > REG_FIELD_PREP(PLANE_PIXEL_NORMALIZE_NORM_FACTOR_MASK,(val))
> > +
> > #endif /* __SKL_UNIVERSAL_PLANE_REGS_H__ */
>
