Re: [PATCH] drm/i915: Only warn for might_sleep() before a slow wait_for_register

["Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>]

On Wed, 2018-03-28 at 20:13 +0100, Chris Wilson wrote:
> Quoting Pandiyan, Dhinakaran (2018-03-28 20:01:40)
> > 
> > On Wed, 2018-03-28 at 18:53 +0100, Chris Wilson wrote:
> > > As intel_wait_for_register_fw() may use, and if successful only use, a
> > > busy-wait loop, the might_sleep() warning is a little over-zealous.
> > > Restrict it to a might_sleep_if() a slow timeout is specified (and so
> > > the caller authorises use of a usleep).
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/intel_uncore.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > index f37ecfc69e49..44c4654443ba 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -1996,7 +1996,7 @@ int __intel_wait_for_register(struct drm_i915_private *dev_priv,
> > >       u32 reg_value;
> > >       int ret;
> > >  
> > > -     might_sleep();
> > > +     might_sleep_if(slow_timeout_ms);
> > 
> > __wait_for() already has a might_sleep(), why is this needed?
> 
> To document that this routine is a sleeper, irrespective of the
> implementation. Sometimes it is implicit on the implementation and so
> should only be at the low level, sometimes we want to call out the
> requirements explicitly and clearly. We have "wait" in the name so
> shouting out that this may indeed sleep rather than busyspin seems to
> be in order.
> -Chris

Fair enough.

There seems to be a side effect from the second hunk that your commit
message does not explicitly state.

> -     if (ret)
> +     if (ret && slow_timeout_ms)

This results in a different return value if condition == true and
slow_timeout_ms == 0 after fast_timeout_us was exceeded.

Previously, __intel_wait_for_register would have passed along the 0 from
__wait_for(), now it returns -ETIMEDOUT. Which means this change should
have been a separate patch. 

As the patch itself is sensible, 
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx