Re: [Intel-gfx] [PATCH 2/6] drm/i915: Rework SAGV block time probing

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

On Wed, Mar 09, 2022 at 10:41:28AM +0000, Govindapillai, Vinod wrote:
> On Tue, 2022-03-08 at 19:32 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > I'd like to see the SAGV block time we got from the mailbox
> > in the logs regardless of whether other factors prevent the
> > use of SAGV.
> > 
> > So let's adjust the code to always query the SAGV block time,
> > log it, and then reset it if SAGV is not actually supported.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Hi Ville,
> 
> I wonder if it is better to comprehend if we clean assigning  -1 to a u32 variable first before
> this? I see that you are doing that anyway in the next patch in this series. Was there any
> particular reason?

No real reason. Probably best to reorder as you suggest so we can
backport the s/-1/0/ change, assuming it actually fixes something.

> 
> BR
> Vinod
> 
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 38 +++++++++++++++++++--------------
> >  1 file changed, 22 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 8ee31c9590a7..21c37115c36e 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3670,8 +3670,8 @@ intel_has_sagv(struct drm_i915_private *dev_priv)
> >  		dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED;
> >  }
> >  
> > -static void
> > -skl_setup_sagv_block_time(struct drm_i915_private *dev_priv)
> > +static u32
> > +intel_sagv_block_time(struct drm_i915_private *dev_priv)
> >  {
> >  	if (DISPLAY_VER(dev_priv) >= 12) {
> >  		u32 val = 0;
> > @@ -3680,24 +3680,31 @@ skl_setup_sagv_block_time(struct drm_i915_private *dev_priv)
> >  		ret = snb_pcode_read(dev_priv,
> >  				     GEN12_PCODE_READ_SAGV_BLOCK_TIME_US,
> >  				     &val, NULL);
> > -		if (!ret) {
> > -			dev_priv->sagv_block_time_us = val;
> > -			return;
> > +		if (ret) {
> > +			drm_dbg_kms(&dev_priv->drm, "Couldn't read SAGV block time!\n");
> > +			return -1;
> >  		}
> >  
> > -		drm_dbg(&dev_priv->drm, "Couldn't read SAGV block time!\n");
> > +		return val;
> >  	} else if (DISPLAY_VER(dev_priv) == 11) {
> > -		dev_priv->sagv_block_time_us = 10;
> > -		return;
> > -	} else if (DISPLAY_VER(dev_priv) == 9) {
> > -		dev_priv->sagv_block_time_us = 30;
> > -		return;
> > +		return 10;
> > +	} else if (DISPLAY_VER(dev_priv) == 9 && !IS_LP(dev_priv)) {
> > +		return 30;
> >  	} else {
> > -		MISSING_CASE(DISPLAY_VER(dev_priv));
> > +		/* Default to an unusable block time */
> > +		return -1;
> >  	}
> > +}
> >  
> > -	/* Default to an unusable block time */
> > -	dev_priv->sagv_block_time_us = -1;
> > +static void intel_sagv_init(struct drm_i915_private *i915)
> > +{
> > +	i915->sagv_block_time_us = intel_sagv_block_time(i915);
> > +
> > +	drm_dbg_kms(&i915->drm, "SAGV supported: %s, original SAGV block time: %u us\n",
> > +		    str_yes_no(intel_has_sagv(i915)), i915->sagv_block_time_us);
> > +
> > +	if (!intel_has_sagv(i915))
> > +		i915->sagv_block_time_us = -1;
> >  }
> >  
> >  /*
> > @@ -8173,8 +8180,7 @@ void intel_init_pm(struct drm_i915_private *dev_priv)
> >  	else if (GRAPHICS_VER(dev_priv) == 5)
> >  		ilk_get_mem_freq(dev_priv);
> >  
> > -	if (intel_has_sagv(dev_priv))
> > -		skl_setup_sagv_block_time(dev_priv);
> > +	intel_sagv_init(dev_priv);
> >  
> >  	/* For FIFO watermark updates */
> >  	if (DISPLAY_VER(dev_priv) >= 9) {

-- 
Ville Syrjälä
Intel