Re: [PATCH] drm/i915/display: Simplify modular operations with vtotal

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

On Fri, Sep 12, 2025 at 02:29:17PM +0000, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com> 
> Sent: Friday, September 12, 2025 1:56 AM
> To: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Gupta, saurabhg <saurabhg.gupta@intel.com>; Zuo, Alex <alex.zuo@intel.com>; Cavitt, Jonathan <jonathan.cavitt@intel.com>; ville.syrjala@linux.intel.com; Manna, Animesh <animesh.manna@intel.com>
> Subject: Re: [PATCH] drm/i915/display: Simplify modular operations with vtotal
> > 
> > On Thu, 11 Sep 2025, Jonathan Cavitt <jonathan.cavitt@intel.com> wrote:
> > > There are a couple of modulus operations in the i915 display code with
> > > vtotal as the divisor that add vtotal to the dividend.  In modular
> > > arithmetic, adding the divisor to the dividend is equivalent to adding
> > > zero to the dividend, so this addition can be dropped.
> > 
> > The result might become negative with this?
> > 
> > BR,
> > Jani.
> > 
> > >
> > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Animesh Manna <animesh.manna@intel.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dsb.c    | 4 ++--
> > >  drivers/gpu/drm/i915/display/intel_vblank.c | 2 +-
> > >  2 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > index dee44d45b668..67315116839b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > @@ -173,7 +173,7 @@ static int dsb_scanline_to_hw(struct intel_atomic_state *state,
> > >  		intel_pre_commit_crtc_state(state, crtc);
> > >  	int vtotal = dsb_vtotal(state, crtc);
> > >  
> > > -	return (scanline + vtotal - intel_crtc_scanline_offset(crtc_state)) % vtotal;
> > > +	return (scanline - intel_crtc_scanline_offset(crtc_state)) % vtotal;
> 
> intel_crtc_scanline_offset returns -1, 1, or 2.  So the result here could only be negative if
> the value of scanline is less than 2.
> 
> > >  }
> > >  
> > >  /*
> > > @@ -482,7 +482,7 @@ static void assert_dsl_ok(struct intel_atomic_state *state,
> > >  	 * 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,
> > > +	drm_WARN(crtc->base.dev, (end - start) % vtotal == vtotal - 1,
> 
> This can only be negative if start is less than end, which doesn't seem possible.
> 
> > >  		 "[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);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> > > index c15234c1d96e..bcfca2fcef3c 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> > > @@ -288,7 +288,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
> > >  	 * See update_scanline_offset() for the details on the
> > >  	 * scanline_offset adjustment.
> > >  	 */
> > > -	return (position + vtotal + crtc->scanline_offset) % vtotal;
> > > +	return (position + crtc->scanline_offset) % vtotal;
> 
> crtc->scanline_offset = intel_crtc_scanline_offset(crtc_state).
> And position = intel_de_read_fw(display, PIPEDSL(display, pipe)) & PIPEDSL_LINE_MASK.
> Finally, #define   PIPEDSL_LINE_MASK	REG_GENMASK(19, 0)
> So, unless position = 0 on display versions 1 or 2 (where intel_crtc_scanline_offset returns -1), this cannot be negative.

Scanlines can be anything from 0 to vtotal-1.
So nak on this patch.

> 
> ...
> Wait, if crtc->scanline_offset = intel_crtc_scanline_offset(crtc_state), then why are we recalculating
> it in dsb_scanline_to_hw?  That should also probably be fixed, but not in this patch.

Not sure what you think needs fixing. dsb_scanline_to_hw() is the
inverse of most other uses of scanline_offset.

> -Jonathan Cavitt
> 
> > >  }
> > >  
> > >  /*
> > 
> > -- 
> > Jani Nikula, Intel
> > 

-- 
Ville Syrjälä
Intel