Re: [PATCH 10/10] drm/i915/chv: Freq(opcode) request for CHV.

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

On Mon, May 05, 2014 at 06:17:39PM +0530, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
> 
> On CHV, All the freq request should be even. So, we need to make sure we
> request the opcode accordingly.
> 
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 1 +
>  drivers/gpu/drm/i915/i915_irq.c | 9 +++++++--
>  drivers/gpu/drm/i915/i915_reg.h | 3 +++
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e70a9f0..3966ff2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1318,6 +1318,7 @@ struct drm_i915_private {
>  	u32 gt_irq_mask;
>  	u32 pm_irq_mask;
>  	u32 pm_rps_events;
> +	u32 pm_rps_freq_req;
>  	u32 pipestat_irq_mask[I915_MAX_PIPES];
>  
>  	struct work_struct hotplug_work;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6af51ad..3e8bcca 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1162,7 +1162,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  		if (adj > 0)
>  			adj *= 2;
>  		else
> -			adj = 1;
> +			adj = dev_priv->pm_rps_freq_req;
>  		new_delay = dev_priv->rps.cur_freq + adj;
>  
>  		/*
> @@ -1181,7 +1181,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  		if (adj < 0)
>  			adj *= 2;
>  		else
> -			adj = -1;
> +			adj = -1 * dev_priv->pm_rps_freq_req;

I'm not sure pm_rps_freq_req is very descriptive. Maybe it's better to
just open code the thing and add a few comments. Eg.:
 /* CHV requires even values */
 adj = IS_CHERRYVIEW(dev) ? 2 : 1;
 ...
 /* CHV requires even values */
 adj = IS_CHERRYVIEW(dev) ? -2 : -1;

>  		new_delay = dev_priv->rps.cur_freq + adj;

In any case this doesn't seem to enough to guarantee an even value.
You'd also need to make sure efficient_freq, min_freq_softlimit and
max_freq_softlimit are even.

The alternative is to just make the final value even in 
valleyview_set_rps(), but then you might still trip these
 WARN_ON(val > dev_priv->rps.max_freq_softlimit);
 WARN_ON(val < dev_priv->rps.min_freq_softlimit);
in there. So I guess it's back to option 1 and making sure all the
limits are also even. Also leaving it up to valleyview_set_rps() could
introduce a slight delay before the frequency starts to change since it
could effectively ignore the first interrupt (depending on which way
it's going and which we we would round the final value).

>  	} else { /* unknown event */
>  		new_delay = dev_priv->rps.cur_freq;
> @@ -4088,6 +4088,11 @@ void intel_irq_init(struct drm_device *dev)
>  	/* Let's track the enabled rps events */
>  	dev_priv->pm_rps_events = GEN6_PM_RPS_EVENTS;
>  
> +	if (IS_CHERRYVIEW(dev))
> +		dev_priv->pm_rps_freq_req = CHV_GPU_FREQ_REQ;
> +	else
> +		dev_priv->pm_rps_freq_req = GEN6_GPU_FREQ_REQ;
> +
>  	setup_timer(&dev_priv->gpu_error.hangcheck_timer,
>  		    i915_hangcheck_elapsed,
>  		    (unsigned long) dev);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 3ff34c4..4998d6b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -529,6 +529,9 @@ enum punit_power_well {
>  #define CHV_FB_RPE_FREQ_SHIFT			8
>  #define CHV_FB_RPE_FREQ_MASK			0xff
>  
> +#define CHV_GPU_FREQ_REQ			2
> +#define GEN6_GPU_FREQ_REQ			1

I don't think these defines buy us anything. Instead I think just
adding a comment or two to where the even vs. odd magic happens
should be a better option.

> +
>  #define IOSF_NC_FB_GFX_FREQ_FUSE		0x1c
>  #define   FB_GFX_MAX_FREQ_FUSE_SHIFT		3
>  #define   FB_GFX_MAX_FREQ_FUSE_MASK		0x000007f8
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC