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

[Chris Wilson]

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