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

[Manna, Animesh]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: Wednesday, July 3, 2024 5:40 PM
> To: Manna, Animesh <animesh.manna at intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [PATCH 09/14] drm/i915/dsb: Introduce
> intel_dsb_wait_scanline_{in, out}()
> 
> 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 at lists.freedesktop.org> On
> > > > Behalf Of Ville Syrjala
> > > > Sent: Tuesday, June 25, 2024 12:40 AM
> > > > To: intel-gfx at lists.freedesktop.org
> > > > Subject: [PATCH 09/14] drm/i915/dsb: Introduce
> > > > intel_dsb_wait_scanline_{in, out}()
> > > >
> > > > From: Ville Syrjälä <ville.syrjala at 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 at 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.

Variable name change is just a nitpick, with or without fix,

Reviewed-by: Animesh Manna <animesh.manna at intel.com>


> 
> --
> Ville Syrjälä
> Intel