Re: [PATCH 3/3] drm/i915: Check the ring is empty when declaring the engines are idle

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Tue, May 30, 2017 at 03:33:41PM +0300, Mika Kuoppala wrote:
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > As another precaution when testing whether the CS engine is actually
>> > idle, also inspect the ring's HEAD/TAIL registers, which should be equal
>> > when there are no commands left to execute by the GPU.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_engine_cs.c | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
>> > index 699f2d3861c7..bc38bd128b76 100644
>> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> > @@ -1212,6 +1212,11 @@ static bool ring_is_idle(struct intel_engine_cs *engine)
>> >  
>> >  	intel_runtime_pm_get(dev_priv);
>> >  
>> > +	/* First check that no commands are left in the ring */
>> > +	if ((I915_READ_HEAD(engine) & HEAD_ADDR) !=
>> > +	    (I915_READ_TAIL(engine) & TAIL_ADDR))
>> > +		idle = false;
>> > +
>> 
>> You are already certain that is not idle so why not goto out?
>
> In this case I could argue that extra path for the jump is not worth it.
> It saves a mmio read, yes, but will any one notice?
and one write :P

> It boils down to is it easier to read as:
>

Sold. It is easier to read as is. 3/3 is

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> static bool ring_is_idle(struct intel_engine_cs *engine)
> {
>         struct drm_i915_private *dev_priv = engine->i915;
>         bool idle = false;
>
>         intel_runtime_pm_get(dev_priv);
>
>         /* First check that no commands are left in the ring */
>         if ((I915_READ_HEAD(engine) & HEAD_ADDR) !=
>             (I915_READ_TAIL(engine) & TAIL_ADDR))
>                 goto busy;
>
>         /* No bit for gen2, so assume the CS parser is idle */
>         if (INTEL_GEN(dev_priv) > 2 && !(I915_READ_MODE(engine) & MODE_IDLE))
>                 goto busy;
>
>         idle = true;
> busy:
>         intel_runtime_pm_put(dev_priv);
>
>         return idle;
> }
>
> First instinct would be no because of the goto.
> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx