On Mon, 2026-01-26 at 15:43 +0200, Jani Nikula wrote:
> On Mon, 26 Jan 2026, Vinod Govindapillai
> <[email protected]> wrote:
> > Pixel normalizer is enabled with normalization factor as 1.0 for
> > FP16 formats in order to support FBC for those formats in xe3p_lpd.
> > Previously pixel normalizer gets disabled during the plane disable
> > routine. But there could be plane format settings without
> > explicitly
> > calling the plane disable in-between and we could endup keeping the
> > pixel normalizer enabled for formats which we don't require that.
> > This is causing crc mismatches in yuv formats and FIFO underruns in
> > planar formats like NV12.
> >
> > Fix this by updating the pixel normalizer configuration based on
> > the
> > pixel formats explicitly during the plane settings arm calls itself
> > - enable it for FP16 and disable it for other formats in HDR
> > capable
> > planes. To avoid redundancies in these updates, normalization
> > factor
> > between old and new plane states are compared before the update.
> > The
> > function to check validity of the fp16 formats for fbc is now
> > updated
> > to return the normalization factor as 1.0 in case of fp16 formats
> > and
> > 0 in other cases.
> >
> > v2: avoid redundant pixel normalization setting updates
> >
> > Fixes: 5298eea7ed20 ("drm/i915/xe3p_lpd: use pixel normalizer for
> > fp16 formats for FBC")
> > Signed-off-by: Vinod Govindapillai <[email protected]>
> > ---
> > .../drm/i915/display/intel_display_device.h | 1 +
> > .../drm/i915/display/intel_display_types.h | 8 ++
> > drivers/gpu/drm/i915/display/intel_fbc.c | 17 +++-
> > drivers/gpu/drm/i915/display/intel_fbc.h | 4 +-
> > .../drm/i915/display/skl_universal_plane.c | 82
> > +++++++++++++++----
> > 5 files changed, 90 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h
> > b/drivers/gpu/drm/i915/display/intel_display_device.h
> > index 6c74d6b0cc48..126aa1eeeb6d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> > @@ -175,6 +175,7 @@ struct intel_display_platforms {
> > #define HAS_DSC_MST(__display) (DISPLAY_VER(__display) >=
> > 12 && HAS_DSC(__display))
> > #define
> > HAS_FBC(__display) (DISPLAY_RUNTIME_INFO(__display)->fbc_mask != 0)
> > #define HAS_FBC_DIRTY_RECT(__display) (DISPLAY_VER(__display) >=
> > 30)
> > +#define
> > HAS_FBC_FP16_FORMATS(__display) (DISPLAY_VER(__display) >= 35)
> > #define HAS_FBC_SYS_CACHE(__display) (DISPLAY_VER(__display) >=
> > 35 && !(__display)->platform.dgfx)
> > #define
> > HAS_FPGA_DBG_UNCLAIMED(__display) (DISPLAY_INFO(__display)->has_fpga_dbg)
> > #define HAS_FW_BLC(__display) (DISPLAY_VER(__display) >=
> > 3)
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 07b626d22748..f42f2609d837 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -685,6 +685,14 @@ struct intel_plane_state {
> > unsigned long flags;
> > #define PLANE_HAS_FENCE BIT(0)
> >
> > + /* xe3p_lpd+ */
> > + struct {
> > + /* 0x3c00 (1.0) for fp16 formats and else disable
> > (0) */
> > + unsigned int factor;
>
> What is the format of this number? What makes 0x3c00 mean 1.0?
>
> > + /* update is needed if factor differs between old
> > and new plane states */
> > + bool needs_update;
> > + } pixel_normalizer;
> > +
> > struct intel_fb_view view;
> >
> > /* for legacy cursor fb unpin */
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index fef2f35ff1e9..dbedc8ea8cdf 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -60,6 +60,7 @@
> > #include "intel_frontbuffer.h"
> > #include "intel_parent.h"
> > #include "intel_step.h"
> > +#include "skl_universal_plane_regs.h"
> >
> > #define for_each_fbc_id(__display, __fbc_id) \
> > for ((__fbc_id) = INTEL_FBC_A; (__fbc_id) < I915_MAX_FBCS;
> > (__fbc_id)++) \
> > @@ -1202,13 +1203,21 @@ static bool
> > xe3p_lpd_fbc_pixel_format_is_valid(const struct intel_plane_state
> > *p
> > }
> > }
> >
> > -bool
> > -intel_fbc_is_enable_pixel_normalizer(const struct
> > intel_plane_state *plane_state)
> > +unsigned int
> > +intel_fbc_normalization_factor(const struct intel_plane_state
> > *plane_state)
> > {
> > struct intel_display *display =
> > to_intel_display(plane_state);
> >
> > - return DISPLAY_VER(display) >= 35 &&
> > - xe3p_lpd_fbc_fp16_format_is_valid(plane_state);
> > + /*
> > + * In order to have FBC for fp16 formats pixel normalizer
> > block must be
> > + * active. For FP16 formats, use normalization factor as
> > 1.0 and enable
> > + * the block.
> > + */
> > + if (HAS_FBC_FP16_FORMATS(display) &&
> > + xe3p_lpd_fbc_fp16_format_is_valid(plane_state))
> > + return PLANE_PIXEL_NORMALIZE_NORM_FACTOR_1_0;
>
> Ditto here. I don't particularly like encoding register values to
> function return values like this. Basically, including
> skl_universal_plane_regs.h here feels wrong.
It is in the half precision floating point format as per the bspec.
This is a standard representation the register expect the format to be
for the normalization factor but it is not exactly as per the register
encoding for the corresponding bitfield.
I can return 1.0 from this function and a use a separate function to
encode this as per the bspec in skl_universal_plane. But as this is the
only case where, we have to use this, was wondering if it is too much
abstraction! Are you expecting something like this?
I did consider to use only that bool variable to check if the
normalization factor need an update and avoiding the factor being
passed from the fbc code. But that could lead to some redundant updates
to the normalization register especially during the disable_plane arm
call.
BR
Vinod
>
>
> BR,
> Jani.
>
>
> > +
> > + return 0;
> > }
> >
> > static bool pixel_format_is_valid(const struct intel_plane_state
> > *plane_state)
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h
> > b/drivers/gpu/drm/i915/display/intel_fbc.h
> > index f0255ddae2b6..b5888e98a659 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.h
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.h
> > @@ -56,7 +56,7 @@ void intel_fbc_prepare_dirty_rect(struct
> > intel_atomic_state *state,
> > struct intel_crtc *crtc);
> > void intel_fbc_dirty_rect_update_noarm(struct intel_dsb *dsb,
> > struct intel_plane *plane);
> > -bool
> > -intel_fbc_is_enable_pixel_normalizer(const struct
> > intel_plane_state *plane_state);
> > +unsigned int
> > +intel_fbc_normalization_factor(const struct intel_plane_state
> > *plane_state);
> >
> > #endif /* __INTEL_FBC_H__ */
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index b3d41705448a..05c227913b8d 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -891,20 +891,49 @@ static void
> > icl_plane_disable_sel_fetch_arm(struct intel_dsb *dsb,
> > intel_de_write_dsb(display, dsb, SEL_FETCH_PLANE_CTL(pipe,
> > plane->id), 0);
> > }
> >
> > -static void x3p_lpd_plane_update_pixel_normalizer(struct intel_dsb
> > *dsb,
> > - struct
> > intel_plane *plane,
> > - bool enable)
> > +static void xe3p_lpd_plane_disable_pixel_normalizer(struct
> > intel_dsb *dsb,
> > + struct
> > intel_plane *plane)
> > {
> > struct intel_display *display = to_intel_display(plane);
> > enum intel_fbc_id fbc_id = skl_fbc_id_for_pipe(plane-
> > >pipe);
> > - u32 val;
> > + const struct intel_plane_state *plane_state =
> > + to_intel_plane_state(plane->base.state);
> > +
> > + if (!HAS_FBC_FP16_FORMATS(display))
> > + return;
> > +
> > + if (!skl_plane_has_fbc(display, fbc_id, plane->id))
> > + return;
> > +
> > + if (!plane_state->pixel_normalizer.factor)
> > + return;
> > +
> > + intel_de_write_dsb(display, dsb,
> > + PLANE_PIXEL_NORMALIZE(plane->pipe,
> > plane->id), 0);
> > +}
> > +
> > +static void xe3p_lpd_plane_update_pixel_normalizer(struct
> > intel_dsb *dsb,
> > + struct
> > intel_plane *plane)
> > +{
> > + struct intel_display *display = to_intel_display(plane);
> > + enum intel_fbc_id fbc_id = skl_fbc_id_for_pipe(plane-
> > >pipe);
> > + const struct intel_plane_state *plane_state =
> > + to_intel_plane_state(plane->base.state);
> > + u32 val = 0;
> > +
> > + if (!HAS_FBC_FP16_FORMATS(display))
> > + return;
> >
> > - /* Only HDR planes have pixel normalizer and don't matter
> > if no FBC */
> > + /* Only HDR planes have pixel normalizer and don't matter
> > if FBC is fused off */
> > if (!skl_plane_has_fbc(display, fbc_id, plane->id))
> > return;
> >
> > - val = enable ?
> > PLANE_PIXEL_NORMALIZE_NORM_FACTOR(PLANE_PIXEL_NORMALIZE_NORM_FACTOR
> > _1_0) |
> > - PLANE_PIXEL_NORMALIZE_ENABLE : 0;
> > + if (!plane_state->pixel_normalizer.needs_update)
> > + return;
> > +
> > + if (plane_state->pixel_normalizer.factor)
> > + val =
> > PLANE_PIXEL_NORMALIZE_NORM_FACTOR(plane_state-
> > >pixel_normalizer.factor) |
> > + PLANE_PIXEL_NORMALIZE_ENABLE;
> >
> > intel_de_write_dsb(display, dsb,
> > PLANE_PIXEL_NORMALIZE(plane->pipe,
> > plane->id), val);
> > @@ -926,8 +955,7 @@ icl_plane_disable_arm(struct intel_dsb *dsb,
> >
> > icl_plane_disable_sel_fetch_arm(dsb, plane, crtc_state);
> >
> > - if (DISPLAY_VER(display) >= 35)
> > - x3p_lpd_plane_update_pixel_normalizer(dsb, plane,
> > false);
> > + xe3p_lpd_plane_disable_pixel_normalizer(dsb, plane);
> >
> > intel_de_write_dsb(display, dsb, PLANE_CTL(pipe,
> > plane_id), 0);
> > intel_de_write_dsb(display, dsb, PLANE_SURF(pipe,
> > plane_id), 0);
> > @@ -1674,13 +1702,7 @@ icl_plane_update_arm(struct intel_dsb *dsb,
> >
> > intel_color_plane_commit_arm(dsb, plane_state);
> >
> > - /*
> > - * In order to have FBC for fp16 formats pixel normalizer
> > block must be
> > - * active. Check if pixel normalizer block need to be
> > enabled for FBC.
> > - * If needed, use normalization factor as 1.0 and enable
> > the block.
> > - */
> > - if (intel_fbc_is_enable_pixel_normalizer(plane_state))
> > - x3p_lpd_plane_update_pixel_normalizer(dsb, plane,
> > true);
> > + xe3p_lpd_plane_update_pixel_normalizer(dsb, plane);
> >
> > /*
> > * The control register self-arms if the plane was
> > previously
> > @@ -2350,6 +2372,32 @@ static void clip_damage(struct
> > intel_plane_state *plane_state)
> > drm_rect_intersect(damage, &src);
> > }
> >
> > +static void check_pixel_normalizer(struct intel_plane_state
> > *plane_state)
> > +{
> > + struct intel_display *display =
> > to_intel_display(plane_state);
> > + struct intel_plane *plane = to_intel_plane(plane_state-
> > >uapi.plane);
> > + struct intel_atomic_state *state =
> > + to_intel_atomic_state(plane_state->uapi.state);
> > + const struct intel_plane_state *old_plane_state =
> > + intel_atomic_get_old_plane_state(state, plane);
> > +
> > + if (!HAS_FBC_FP16_FORMATS(display))
> > + return;
> > +
> > + plane_state->pixel_normalizer.factor =
> > + intel_fbc_normalization_factor(plane_state);
> > +
> > + /*
> > + * In case of no old state to compare, better to force
> > update the pixel
> > + * normalizer settings.
> > + */
> > + plane_state->pixel_normalizer.needs_update = true;
> > + if (old_plane_state && old_plane_state->hw.fb)
> > + plane_state->pixel_normalizer.needs_update =
> > + plane_state->pixel_normalizer.factor !=
> > + intel_fbc_normalization_factor(old_plane_s
> > tate);
> > +}
> > +
> > static int skl_plane_check(struct intel_crtc_state *crtc_state,
> > struct intel_plane_state *plane_state)
> > {
> > @@ -2400,6 +2448,8 @@ static int skl_plane_check(struct
> > intel_crtc_state *crtc_state,
> >
> > check_protection(plane_state);
> >
> > + check_pixel_normalizer(plane_state);
> > +
> > /* HW only has 8 bits pixel precision, disable plane if
> > invisible */
> > if (!(plane_state->hw.alpha >> 8)) {
> > plane_state->uapi.visible = false;
>
