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 03:07:04PM +0300, Ville Syrjälä wrote:
> 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.

I suppose one could argue intel_dsb_wait_dsl() should still
use the start/end terminology as the input are not yet
in hw units. Shrug.

-- 
Ville Syrjälä
Intel