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

Ben Widawsky
2014-04-25T22:32:27Z

On Mon, Apr 21, 2014 at 01:34:14PM +0530, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
>
> On CHV, All the freq request should be even. S0, we need to make sure we
> request the opcode accordingly.
>
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 9 +++++++++
> drivers/gpu/drm/i915/i915_irq.c | 4 ++--
> 2 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 ead2714..5435d87 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2641,6 +2641,15 @@ timespec_to_jiffies_timeout(const struct timespec *value)
> return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
> }
>
> +/* rps/turbo related */
> +static inline int i915_rps_freq_change(struct drm_device *dev)
> +{
> + if (IS_CHERRYVIEW(dev))
> + return 2;
> +
> + return 1;
> +}
> +
> /*
> * If you need to wait X milliseconds between events A and B, but event B
> * doesn't happen exactly after event A, you record the timestamp (jiffies) of
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index cf29668..11538fe 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1190,7 +1190,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
> if (adj > 0)
> adj *= 2;
> else
> - adj = 1;
> + adj = i915_rps_freq_change(dev_priv->dev);
> new_delay = dev_priv->rps.cur_freq + adj;
>
> /*
> @@ -1209,7 +1209,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
> if (adj < 0)
> adj *= 2;
> else
> - adj = -1;
> + adj = -1 * i915_rps_freq_change(dev_priv->dev);
> new_delay = dev_priv->rps.cur_freq + adj;
> } else { /* unknown event */
> new_delay = dev_priv->rps.cur_freq;

splitting hairs a bit, but adding a new function that isn't named well
doesn't really improve readability. Since we only ever do it for one
case, I think this logic is better stuffed in Cherryview specific
_set_rps() function.

I don't see anything incorrect though. I'd prefer my recommendation, but
it's
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

--
Ben Widawsky, Intel Open Source Technology Center