[RFC PATCH] drm/xe/display: Program double buffered LUT registers
Borah, Chaitanya Kumar
chaitanya.kumar.borah at intel.com
Tue Jan 7 09:52:48 UTC 2025
> -----Original Message-----
> From: Shankar, Uma <uma.shankar at intel.com>
> Sent: Monday, January 6, 2025 2:05 AM
> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah at intel.com>; intel-
> xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org
> Cc: ville.syrjala at linux.intel.com
> Subject: RE: [RFC PATCH] drm/xe/display: Program double buffered LUT
> registers
>
>
>
> > -----Original Message-----
> > From: Borah, Chaitanya Kumar <chaitanya.kumar.borah at intel.com>
> > Sent: Wednesday, December 11, 2024 11:49 PM
> > To: intel-xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org
> > Cc: Shankar, Uma <uma.shankar at intel.com>;
> > ville.syrjala at linux.intel.com; Borah, Chaitanya Kumar
> > <chaitanya.kumar.borah at intel.com>
> > Subject: [RFC PATCH] drm/xe/display: Program double buffered LUT
> > registers
> >
> > From PTL, LUT registers are made double buffered. This helps us to
> > program them in the active region without any concern of tearing.
> > This particulary helps in case of displays with high refresh rates
> > where vblank periods are shorter.
> >
> > This patch tries to incorporates LUT programming to the noarm commit
> > path for PTL without making significant changes to the color callback
> framework itself.
> > DSB0 is still used to program to non LUT color registers (for ex.
> > CTM). However, it does not chain DSB1 to program the LUT registers.
> > Instead, it is programmed through intel_pre_update_crtc path.
> >
> > LUT programming is also disabled in the vblank worker.
>
> Approach Looks Good to me. But we can still use DSB to program the same in
> active or Is there any limitation ?
>
Thank you for the review, Uma. The patch [1] indicates that there are issues with loading LUTs using DSB outside the vblank region.
That is the reason this patch avoids using DSB while programming LUTs in the active region and uses the MMIO route. Perhaps Ville can shed more light on it.
[1] https://cgit.freedesktop.org/drm-tip/commit/?id=5ae0da3fc78d3fdef278a22e874d6d5c305d1e03
Regards
Chaitanya
> Regards,
> Uma Shankar
>
> > Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah at intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_color.c | 28 +++++++++++++++++++-
> > drivers/gpu/drm/i915/display/intel_crtc.c | 4 ++-
> > drivers/gpu/drm/i915/display/intel_display.c | 2 +-
> > 3 files changed, 31 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> > b/drivers/gpu/drm/i915/display/intel_color.c
> > index 7cd902bbd244..513b2718bf5a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > @@ -1911,6 +1911,16 @@ static void chv_load_luts(const struct
> > intel_crtc_state
> > *crtc_state)
> > crtc_state->cgm_mode);
> > }
> >
> > +static void ptl_color_commit_noarm(struct intel_dsb *dsb,
> > + const struct intel_crtc_state *crtc_state) {
> > + icl_load_csc_matrix(dsb, crtc_state);
> > + if (crtc_state->preload_luts || intel_crtc_needs_modeset(crtc_state)
> > +||
> > dsb)
> > + return;
> > +
> > + icl_load_luts(crtc_state);
> > +}
> > +
> > void intel_color_load_luts(const struct intel_crtc_state *crtc_state) {
> > struct intel_display *display = to_intel_display(crtc_state); @@
> > -1980,6
> > +1990,9 @@ void intel_color_prepare_commit(struct intel_atomic_state
> > +*state,
> > if (!crtc_state->pre_csc_lut && !crtc_state->post_csc_lut)
> > return;
> >
> > + if (DISPLAY_VER(display) >= 30)
> > + return;
> > +
> > crtc_state->dsb_color_vblank = intel_dsb_prepare(state, crtc,
> > INTEL_DSB_1, 1024);
> > if (!crtc_state->dsb_color_vblank)
> > return;
> > @@ -3842,6 +3855,17 @@ static const struct intel_color_funcs
> > i9xx_color_funcs = {
> > .get_config = i9xx_get_config,
> > };
> >
> > +static const struct intel_color_funcs ptl_color_funcs = {
> > + .color_check = icl_color_check,
> > + .color_commit_noarm = ptl_color_commit_noarm,
> > + .color_commit_arm = icl_color_commit_arm,
> > + .load_luts = icl_load_luts,
> > + .read_luts = icl_read_luts,
> > + .lut_equal = icl_lut_equal,
> > + .read_csc = icl_read_csc,
> > + .get_config = skl_get_config,
> > +};
> > +
> > static const struct intel_color_funcs tgl_color_funcs = {
> > .color_check = icl_color_check,
> > .color_commit_noarm = icl_color_commit_noarm, @@ -3989,7
> +4013,9 @@
> > void intel_color_init_hooks(struct intel_display *display)
> > else
> > display->funcs.color = &i9xx_color_funcs;
> > } else {
> > - if (DISPLAY_VER(display) >= 12)
> > + if (DISPLAY_VER(display) >= 30)
> > + display->funcs.color = &ptl_color_funcs;
> > + else if (DISPLAY_VER(display) >= 12)
> > display->funcs.color = &tgl_color_funcs;
> > else if (DISPLAY_VER(display) == 11)
> > display->funcs.color = &icl_color_funcs; diff --git
> > a/drivers/gpu/drm/i915/display/intel_crtc.c
> > b/drivers/gpu/drm/i915/display/intel_crtc.c
> > index a2c528d707f4..cb02af401085 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > @@ -429,10 +429,12 @@ static void intel_crtc_vblank_work(struct
> > kthread_work *base)
> > struct intel_crtc_state *crtc_state =
> > container_of(work, typeof(*crtc_state), vblank_work);
> > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > + struct intel_display *display = to_intel_display(crtc_state);
> >
> > trace_intel_crtc_vblank_work_start(crtc);
> >
> > - intel_color_load_luts(crtc_state);
> > + if (DISPLAY_VER(display) < 30)
> > + intel_color_load_luts(crtc_state);
> >
> > if (crtc_state->uapi.event) {
> > spin_lock_irq(&crtc->base.dev->event_lock);
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 35c8904ecf44..a0bcffe470e5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -7203,7 +7203,7 @@ static void intel_pre_update_crtc(struct
> > intel_atomic_state *state,
> >
> > if (!modeset &&
> > intel_crtc_needs_color_update(new_crtc_state) &&
> > - !new_crtc_state->use_dsb)
> > + (!new_crtc_state->use_dsb || !new_crtc_state->dsb_color_vblank))
> > intel_color_commit_noarm(NULL, new_crtc_state);
> >
> > if (!new_crtc_state->use_dsb)
> > --
> > 2.25.1
More information about the Intel-gfx
mailing list