Re: [PATCH 03/11] drm/i915/execlists: Pull submit after dequeue under timeline lock

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 03/11] drm/i915/execlists: Pull submit after dequeue under timeline lock
Date: Mon, 04 Jun 2018 12:15:53 +0100	[thread overview]
Message-ID: <152811095377.7032.2461986225098713371@mail.alporthouse.com> (raw)
In-Reply-To: <bdfd5fa3-db4b-d17e-9f0f-526592ceeec1@linux.intel.com>

Quoting Tvrtko Ursulin (2018-06-04 11:58:00)
> 
> On 04/06/2018 11:12, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-06-04 10:25:46)
> >>
> >> On 01/06/2018 15:07, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2018-06-01 15:02:38)
> >>>>
> >>>> On 31/05/2018 19:51, Chris Wilson wrote:
> >>>>> In the next patch, we will begin processing the CSB from inside the
> >>>>> interrupt handler. This means that updating the execlists->port[] will
> >>>>
> >>>> The message that we will be processing CSB from irq handler, here and in
> >>>> following patch 5/11, doesn't seem to be true. So why is this needed?
> >>>> Why not just drop some patches from the series?
> >>>
> >>> It will still be called from irq context; as submit_request is called
> >>> from irq context. Hence we still require irqsafe variants.
> >>
> >> submit_request is already called from irqoff contexts so I don't get it.
> > 
> > Right, but the patch series pulls the CSB processing into
> > submit_request as well.
> >   
> >>> So yes, while the overall direction of the patchset changed slightly to
> >>> be less enthusiastic about calling from irq context, such calls were not
> >>> eliminated.
> >>
> >> I have a problem with commit messages where one says we'll be processing
> >> CSB directly from hard irq, while another says we'll always use the tasklet.
> > 
> > It's done inside the tasklet callback; the tasklet callback may be
> > invoked directly, and that may also happen inside an irq.
> 
> Via notify_ring yes. I was looking for something on the context switch 
> path all this time.

Also don't forget third-party callers may come in from under their irq.

> So CSB processing via notify_ring is quite optimistic and my question is 
> whether it brings anything? Or whole change is purely due dequeue?

That's the essence of the major improvement for ring switching. Pretty
sure I cooked up gem_exec_latency/rthog to show that one as well, if not
that's certainly one I'd like to show.

> Another mystery later in the patch series is what happens with 
> writel(execlists->csb_read), which is mmio, but I see you have removed 
> forcewake handling and one commit says there is no more mmio.

We removed forcewake for that last year. Where I say mmio, think
forcewaked mmio reads.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx