Re: [PATCH 09/14] drm/i915/dsb: Introduce intel_dsb_wait_scanline_{in, out}()

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

On Wed, Jul 03, 2024 at 11:37:33AM +0000, Manna, Animesh wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> > Syrjala
> > Sent: Tuesday, June 25, 2024 12:40 AM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [PATCH 09/14] drm/i915/dsb: Introduce
> > intel_dsb_wait_scanline_{in, out}()
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add functions to emit a DSB scanline window wait instructions.
> > We can either wait for the scanline to be IN the window or OUT of the
> > window.
> > 
> > The hardware doesn't handle wraparound so we must manually deal with it
> > by swapping the IN range to the inverse OUT range, or vice versa.
> > 
> > Also add a bit of paranoia to catch the edge case of waiting for the entire
> > frame. That doesn't make sense since an IN wait would be a nop, and an
> > OUT wait would imply waiting forever. Most of the time this also results in
> > both scanline ranges (original and inverted) to have lower=upper+1 which is
> > nonsense from the hw POV.
> > 
> > For now we are only handling the case where the scanline wait happens prior
> > to latching the double buffered registers during the commit (which might
> > change the timings due to LRR/VRR/etc.)
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dsb.c | 73 ++++++++++++++++++++++++
> > drivers/gpu/drm/i915/display/intel_dsb.h |  6 ++
> >  2 files changed, 79 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> > b/drivers/gpu/drm/i915/display/intel_dsb.c
> > index 81937908c798..092cf082ac39 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > @@ -362,6 +362,79 @@ void intel_dsb_nonpost_end(struct intel_dsb *dsb)
> >  	intel_dsb_noop(dsb, 4);
> >  }
> > 
> > +static void intel_dsb_emit_wait_dsl(struct intel_dsb *dsb,
> > +				    u32 opcode, int lower, int upper) {
> > +	u64 window = ((u64)upper << DSB_SCANLINE_UPPER_SHIFT) |
> > +		((u64)lower << DSB_SCANLINE_LOWER_SHIFT);
> > +
> > +	intel_dsb_emit(dsb, lower_32_bits(window),
> > +		       (opcode << DSB_OPCODE_SHIFT) |
> > +		       upper_32_bits(window));
> > +}
> > +
> > +static void intel_dsb_wait_dsl(struct intel_atomic_state *state,
> > +			       struct intel_dsb *dsb,
> > +			       int lower_in, int upper_in,
> 
> Lower/upper keyword maybe confusing for during intel_dsb_wait_scanline_out(), maybe good to have name like in_start and in_end, similarly out_start and out_end.

lower/upper are what bspec calls them. I decided to stick to that
terminology in lower level parts of the code where we actually
deal with hw units. I used start/end in the user facing api to
make it a bit clearer that having start > end is perfectly
valid.

> 
> > +			       int lower_out, int upper_out) {
> > +	struct intel_crtc *crtc = dsb->crtc;
> > +
> > +	lower_in = dsb_scanline_to_hw(state, crtc, lower_in);
> > +	upper_in = dsb_scanline_to_hw(state, crtc, upper_in);
> > +
> > +	lower_out = dsb_scanline_to_hw(state, crtc, lower_out);
> > +	upper_out = dsb_scanline_to_hw(state, crtc, upper_out);
> 
> If lower_in > upper_in, then calculation for lower_out and upper_out is not needed. Even before calling dsb_scanline_to_hw() we can compare and check if it is for scanline_in or scanline_out... rt?

No. The addition of scanline_offset by dsb_scanline_to_hw()
can cause the numbers to wrap around, which would make the
early comparison completely meaningless. So we can only do
that comparison once the scanlines are in hw units.

And we don't want to convert to hw units earlier because then
the +-1 for the inverted range could again cause the numbers
to go out of bounds (which would need %vtotal to correct a
second time) and thus wrap around.

> 
> Regards,
> Animesh
> > +
> > +	if (upper_in >= lower_in)
> > +		intel_dsb_emit_wait_dsl(dsb, DSB_OPCODE_WAIT_DSL_IN,
> > +					lower_in, upper_in);
> > +	else if (upper_out >= lower_out)
> > +		intel_dsb_emit_wait_dsl(dsb, DSB_OPCODE_WAIT_DSL_OUT,
> > +					lower_out, upper_out);
> > +	else
> > +		drm_WARN_ON(crtc->base.dev, 1); /* assert_dsl_ok() should
> > have caught
> > +it already */ }
> > +
> > +static void assert_dsl_ok(struct intel_atomic_state *state,
> > +			  struct intel_dsb *dsb,
> > +			  int start, int end)
> > +{
> > +	struct intel_crtc *crtc = dsb->crtc;
> > +	int vtotal = dsb_vtotal(state, crtc);
> > +
> > +	/*
> > +	 * Waiting for the entire frame doesn't make sense,
> > +	 * (IN==don't wait, OUT=wait forever).
> > +	 */
> > +	drm_WARN(crtc->base.dev, (end - start + vtotal) % vtotal == vtotal -
> > 1,
> > +		 "[CRTC:%d:%s] DSB %d bad scanline window wait: %d-%d
> > (vt=%d)\n",
> > +		 crtc->base.base.id, crtc->base.name, dsb->id,
> > +		 start, end, vtotal);
> > +}
> > +
> > +void intel_dsb_wait_scanline_in(struct intel_atomic_state *state,
> > +				struct intel_dsb *dsb,
> > +				int start, int end)
> > +{
> > +	assert_dsl_ok(state, dsb, start, end);
> > +
> > +	intel_dsb_wait_dsl(state, dsb,
> > +			   start, end,
> > +			   end + 1, start - 1);
> > +}
> > +
> > +void intel_dsb_wait_scanline_out(struct intel_atomic_state *state,
> > +				 struct intel_dsb *dsb,
> > +				 int start, int end)
> > +{
> > +	assert_dsl_ok(state, dsb, start, end);
> > +
> > +	intel_dsb_wait_dsl(state, dsb,
> > +			   end + 1, start - 1,
> > +			   start, end);
> > +}
> > +
> >  static void intel_dsb_align_tail(struct intel_dsb *dsb)  {
> >  	u32 aligned_tail, tail;
> > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h
> > b/drivers/gpu/drm/i915/display/intel_dsb.h
> > index 84fc2f8434d1..d0737cefb393 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> > @@ -39,6 +39,12 @@ void intel_dsb_reg_write_masked(struct intel_dsb
> > *dsb,  void intel_dsb_noop(struct intel_dsb *dsb, int count);  void
> > intel_dsb_nonpost_start(struct intel_dsb *dsb);  void
> > intel_dsb_nonpost_end(struct intel_dsb *dsb);
> > +void intel_dsb_wait_scanline_in(struct intel_atomic_state *state,
> > +				struct intel_dsb *dsb,
> > +				int lower, int upper);
> > +void intel_dsb_wait_scanline_out(struct intel_atomic_state *state,
> > +				 struct intel_dsb *dsb,
> > +				 int lower, int upper);
> > 
> >  void intel_dsb_commit(struct intel_dsb *dsb,
> >  		      bool wait_for_vblank);
> > --
> > 2.44.2
> 

-- 
Ville Syrjälä
Intel