Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
> On Tue, May 30, 2017 at 03:33:41PM +0300, Mika Kuoppala wrote:
>> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
>> > ---
>> > 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@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
