Re: [PATCH] drm/i915: Reset the breadcrumbs IRQ more carefully

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

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

> On Thu, Oct 06, 2016 at 04:32:37PM +0300, Mika Kuoppala wrote:
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > Along with the interrupt, we want to restore the fake-irq and
>> > wait-timeout detection. If we use the breadcrumbs interface to setup the
>> > interrupt as it wants, the auxiliary timers will also be restored.
>> >
>> > Fixes: 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete requests")
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_breadcrumbs.c | 17 +++++++++++++++++
>> >  drivers/gpu/drm/i915/intel_engine_cs.c   | 15 ---------------
>> >  drivers/gpu/drm/i915/intel_lrc.c         |  2 +-
>> >  drivers/gpu/drm/i915/intel_ringbuffer.c  |  2 +-
>> >  drivers/gpu/drm/i915/intel_ringbuffer.h  |  2 +-
>> >  5 files changed, 20 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> > index 9dba4971fb1e..d27da6d69735 100644
>> > --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> > +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> > @@ -584,6 +584,23 @@ int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
>> >  	return 0;
>> >  }
>> >  
>> > +void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
>> > +{
>> > +	struct intel_breadcrumbs *b = &engine->breadcrumbs;
>> > +
>> 
>> Should we kill the timer before proceeding in here?
>
> Which timer? In breadcrumbs.c, we are concerned with the fake_irq and
> the wait-timeout. The wait-timeout is reset below, we should add the
> code to cancel the fake_irq along with clearing the bit.

I was considering that irqs are enabled and we have a
active breadcrumbs timer, triggering at the same time as
reset happens. So we would enable the fake irq as a post reset
race between reset/breadcrumbs hangcheck.

As in why not cancel and postpone the timer and only after
clear the missed_irq?

>  
>> Not relevant to this patch but I also noticed that the period
>> is identical to hangcheck period. Multiple of hangcheck period
>> would be better, as our kicking might help and we don't
>> want to fallback to fake irqs just so easily.
>
> ?
>
> The main GPU hangcheck is kicked off by the wait timeout. Keeping the
> two pieces independent (fake-irq, hangcheck) is quite nice, and the
> jiffie wake up serves as a backup, and either it is required or it will
> be disabled by the reset.

But we queue hangcheck also from retire work. So it could be that
we fallback to fake irqs, even if next hangcheck might have
managed to kick the wait and make forward progress?

And perhaps we should rename the breadcrumb hangcheck
as wait_watchdog to avoid confusion between different independant
'hangchecks'

-Mika

> -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