[PATCH 2/3] drm/i915: WARN if RPS adjustment is more than range

[Chris Wilson]

On Fri, Jan 13, 2017 at 08:21:45PM +0530, Kamble, Sagar A wrote:
> 
> 
> On 1/13/2017 4:26 PM, Chris Wilson wrote:
> >On Fri, Jan 13, 2017 at 04:12:18PM +0530, Kamble, Sagar A wrote:
> >>
> >>On 1/13/2017 3:55 PM, Chris Wilson wrote:
> >>>On Fri, Jan 13, 2017 at 03:46:11PM +0530, Sagar Arun Kamble wrote:
> >>>>Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >>>>Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> >>>>---
> >>>>  drivers/gpu/drm/i915/i915_irq.c | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >>>>index ce5663d..b85bef9 100644
> >>>>--- a/drivers/gpu/drm/i915/i915_irq.c
> >>>>+++ b/drivers/gpu/drm/i915/i915_irq.c
> >>>>@@ -1195,6 +1195,8 @@ static void gen6_pm_rps_work(struct work_struct *work)
> >>>>  		adj = 0;
> >>>>  	}
> >>>>+	WARN_ON(abs(adj) > (max-min));
> >>>Do we actually care? The adjustment is just an exponential function that
> >>>will be clamped (and then when it is finally clamped will become zero).
> >>>Before it is clamped, it is quite permissible that the last adj will be
> >>>larger than the entire range.
> >>>-Chris
> >>Oh. right. I coded this incorrectly. Will it be okay if we WARN on
> >>-ve adjustment for Up interrupts and +ve for Down interrupts.
> >The code has to assume that the pm_iir may contain multiple interrupts
> >(i.e. both up, down, timeout) because we coalesce the hw interrupt into
> >the worker. I fear you will just end up repeating the if-else chain, at
> >which point you could just review the code above.
> >-Chris
> Ok. I am thinking of following options. Could you suggest which one
> to go ahead.
> 1. Adjustment rollover to negative value due to overflow of adj on
> consecutive UP interrupts can be avoided if Up interrupts are masked
> properly. Masking with GUC is fixed in earlier patch.

How can we overflow? Hmm, we are generating interrupts that we don't
care for. That's the actual bug, right?

>     Should we just have that patch and ignore the WARN_ON or bounds
> check altogether.
> 2. Add WARN_ON in individual condition blocks for Up/Down interrupts
> on invalid adjustment.
> 3. Without WARN_ON on invalid adjustment just reset adjustment to zero

Something like:

if (WARN_ON_ONCE(slpc && (pm_iir & INVALID_SLPC_MASK)))
	pm_iir &= ~INVALID_SLPC_MASK

near the start?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre