Re: [PATCH 24/32] drm/i915/xe3p_lpd: Introduce pixel normalizer config support

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

On Wed, 2025-10-15 at 18:11 +0300, Jani Nikula wrote:
> On Wed, 15 Oct 2025, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> > From: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > 
> > 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 <shekhar.chauhan@intel.com>
> > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > ---
> >  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__ */
>