Re: [PATCH 1/2] drm/i915/display: Add MMIO path for double-buffered LUT registers

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Mar 04, 2025 at 02:43:06PM +0000, Borah, Chaitanya Kumar wrote:
> Thank you Ville for the review.
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: Friday, February 28, 2025 8:45 PM
> > To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>
> > Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Shankar,
> > Uma <uma.shankar@intel.com>
> > Subject: Re: [PATCH 1/2] drm/i915/display: Add MMIO path for double-
> > buffered LUT registers
> > 
> > On Tue, Feb 25, 2025 at 11:39:04PM +0530, Chaitanya Kumar Borah wrote:
> > > >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 makes the following changes
> > >
> > > 	- Adds the macro HAS_DOUBLE_BUFFERED_LUT() to distinguish
> > > 	  platforms that have double buffered LUT registers.
> > >
> > > 	- Program LUT values in active region through
> > > 	  intel_pre_update_crtc()
> > >
> > > 	- Disable updating of LUT values during vblank.
> > >
> > > 	- Disable pre-loading of LUT values as they are no longer
> > > 	  single buffered.
> > >
> > > Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_color.c          | 4 ++++
> > >  drivers/gpu/drm/i915/display/intel_crtc.c           | 4 +++-
> > >  drivers/gpu/drm/i915/display/intel_display.c        | 6 +++++-
> > >  drivers/gpu/drm/i915/display/intel_display_device.h | 1 +
> > >  4 files changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> > > b/drivers/gpu/drm/i915/display/intel_color.c
> > > index cfe14162231d..c3ee34b96c15 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > > @@ -2022,6 +2022,10 @@ static bool intel_can_preload_luts(struct
> > > intel_atomic_state *state,  {
> > >  	const struct intel_crtc_state *old_crtc_state =
> > >  		intel_atomic_get_old_crtc_state(state, crtc);
> > > +	struct intel_display *display = to_intel_display(crtc);
> > > +
> > > +	if (HAS_DOUBLE_BUFFERED_LUT(display))
> > > +		return false;
> > >
> > >  	return !old_crtc_state->post_csc_lut &&
> > >  		!old_crtc_state->pre_csc_lut;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c
> > > b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > index 5b2603ef2ff7..927f9acf61c4 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > @@ -432,10 +432,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 (!HAS_DOUBLE_BUFFERED_LUT(display))
> > 
> > Wrong place. You don't even want to schedule the vblank worker for this.
> > 
> 
> Ack.
> 
> > > +		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 065fdf6dbb88..919e236a9650 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -6879,9 +6879,13 @@ 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) {
> > >  		intel_color_commit_noarm(NULL, new_crtc_state);
> > >
> > > +		if (HAS_DOUBLE_BUFFERED_LUT(display))
> > > +			intel_color_load_luts(new_crtc_state);
> > 
> > Explanation missing on the double buffering behaviour of the LUT.
> > This now assumes that it's not self-arming, and therefore some other register
> > must be the arming register. Which register is it?
> > 
> 
> You are correct (took some brain workout😐). The assumption here that the LUT registers are not self-arming is wrong.
> They are self-arming and will latch on to HW at double buffer update point. I will add this to the commit message in the next version.
> 
> Now to ensure atomicity, that leaves us with two possibilities.
> 
> 	1. Write the LUT registers during vblank evasion critical section. While I have to profile it, this might not be possible because
> 	    of the number of registers that needs to be written.

Looking at some numbers I grabbed at some point (on tgl/adl/dg2)
the current (randomly chosen) 20usec vblank evasion should be
ok for ~1500-2000 registers, even with low cdclk/memory clock.

>  	2. Use double buffer stalling. This approach might carry the risk of stalling updates of other registers.

... but I think this is probably what we want to do. I think to
start we could enable the DB stall only for the LUT registers.
I presume we still have those "allow DB stall" bits strewn
about in various control registers? And I presume PTL now
has usable DB stall control (ie. some kind of per-pipe bits
rather than a single global bit)?

Bspec also seems to say that the double buffer swap isn't
immediate for the LUT, so we might need yet another scanline
evasion to make this 100% safe. I think I saw something
about ~500 cdclk cycles, and with a low cdclk of ~170 MHz
that would come out to about 3 usecs, which to me sounds
long enough to potentially be a problem.

Also once we have the basics of DB stall enabled, we could
start extending it to cover other hw resources to make
the critical section even leaner.

-- 
Ville Syrjälä
Intel