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

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

On Mon, Sep 15, 2025 at 03:21:03PM +0000, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com> 
> Sent: Monday, September 15, 2025 8:17 AM
> To: Cavitt, Jonathan <jonathan.cavitt@intel.com>
> Cc: Nikula, Jani <jani.nikula@intel.com>; intel-gfx@lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta@intel.com>; Zuo, Alex <alex.zuo@intel.com>; Manna, Animesh <animesh.manna@intel.com>
> Subject: Re: [PATCH] drm/i915/display: Simplify modular operations with vtotal
> > 
> > On Mon, Sep 15, 2025 at 02:49:22PM +0000, Cavitt, Jonathan wrote:
> > > -----Original Message-----
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> 
> > > Sent: Friday, September 12, 2025 8:30 AM
> > > To: Cavitt, Jonathan <jonathan.cavitt@intel.com>
> > > Cc: Nikula, Jani <jani.nikula@intel.com>; intel-gfx@lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta@intel.com>; Zuo, Alex <alex.zuo@intel.com>; Manna, Animesh <animesh.manna@intel.com>
> > > Subject: Re: [PATCH] drm/i915/display: Simplify modular operations with vtotal
> > > > 
> > > > 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.
> > > 
> > > Well, yes, we subtract it instead of adding it.
> > > 
> > > But like, in dsb_scanline_to_hw:
> > > 
> > > """
> > > return (scanline + vtotal - intel_crtc_scanline_offset(crtc_state)) % vtotal;
> > > """
> > > 
> > > Can this not be simplified to:
> > > 
> > > """
> > > return (scanline + vtotal - crtc->scanline_offset) % vtotal;
> > > """
> > > 
> > > ?
> > 
> > No. crtc->scanline_offset may not be correct at that point in time.
> 
> Is it guaranteed to be accurate in __intel_get_crtc_scanline()?

Yes, for the purpose that it's used there.

> 
> Because that's the only place crtc->scanline_offset is read.
> -Jonathan Cavitt
> 
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> > 

-- 
Ville Syrjälä
Intel