Re: [PATCH v2 1/8] drm/i915: Treat SAGV block time 0 as SAGV disabled

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Mar 09, 2022 at 09:29:58PM +0200, Lisovskiy, Stanislav wrote:
> On Wed, Mar 09, 2022 at 06:49:41PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > 
> > For modern platforms the spec explicitly states that a
> > SAGV block time of zero means that SAGV is not supported.
> > Let's extend that to all platforms. Supposedly there should
> > be no systems where this isn't true, and it'll allow us to:
> > - use the same code regardless of older vs. newer platform
> > - wm latencies already treat 0 as disabled, so this fits well
> >   with other related code
> > - make it a bit more clear when SAGV is used vs. not
> > - avoid overflows from adding U32_MAX with a u16 wm0 latency value
> >   which could cause us to miscalculate the SAGV watermarks on tgl+
> > 
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 8ee31c9590a7..40a3094e55ca 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3696,8 +3696,7 @@ skl_setup_sagv_block_time(struct drm_i915_private *dev_priv)
> >  		MISSING_CASE(DISPLAY_VER(dev_priv));
> >  	}
> >  
> > -	/* Default to an unusable block time */
> > -	dev_priv->sagv_block_time_us = -1;
> > +	dev_priv->sagv_block_time_us = 0;
> >  }
> >  
> >  /*
> > @@ -5644,7 +5643,7 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
> >  	result->min_ddb_alloc = max(min_ddb_alloc, blocks) + 1;
> >  	result->enable = true;
> >  
> > -	if (DISPLAY_VER(dev_priv) < 12)
> > +	if (DISPLAY_VER(dev_priv) < 12 && dev_priv->sagv_block_time_us)
> >  		result->can_sagv = latency >= dev_priv->sagv_block_time_us;
> >  }
> >  
> > @@ -5677,7 +5676,10 @@ static void tgl_compute_sagv_wm(const struct intel_crtc_state *crtc_state,
> >  	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> >  	struct skl_wm_level *sagv_wm = &plane_wm->sagv.wm0;
> >  	struct skl_wm_level *levels = plane_wm->wm;
> > -	unsigned int latency = dev_priv->wm.skl_latency[0] + dev_priv->sagv_block_time_us;
> > +	unsigned int latency = 0;
> > +
> > +	if (dev_priv->sagv_block_time_us)
> > +		latency = dev_priv->sagv_block_time_us + dev_priv->wm.skl_latency[0];
> 
> Should we may be add this to intel_has_sagv? I thought this was supposed to tell,
> if SAGV is supported or not. Should we just call it hear as well, may be..
> Now we kinda making it less obvious. 

We already use intel_has_sagv() to see if we should zero out the
block time. I don't think I want to make it a full circle.

> 
> Stan
> 
> >  
> >  	skl_compute_plane_wm(crtc_state, plane, 0, latency,
> >  			     wm_params, &levels[0],
> > -- 
> > 2.34.1
> > 

-- 
Ville Syrjälä
Intel



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux