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.
> +
> /*
> * 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 @@
>
> _SEL_FETCH_PLANE_OFFSET_5_A, _SEL_FETCH_PLANE_OFFSET_5_B, \
>
> _SEL_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), \
> +
> _PLANE_PIXEL_NORMALIZE_1_A, _PLANE_PIXEL_NORMALIZE_1_B, \
> +
> _PLANE_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__ */
--
Jani Nikula, Intel
