Re: [PATCH] drm/i915/execlists: Preempt-to-busy

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

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

> Quoting Mika Kuoppala (2019-06-20 13:41:26)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> > @@ -38,6 +39,10 @@ struct intel_context {
>> >       struct i915_gem_context *gem_context;
>> >       struct intel_engine_cs *engine;
>> >       struct intel_engine_cs *inflight;
>> > +#define intel_context_inflight(ce) ptr_mask_bits((ce)->inflight, 2)
>> > +#define intel_context_inflight_count(ce)  ptr_unmask_bits((ce)->inflight, 2)
>> > +#define intel_context_inflight_inc(ce) ptr_count_inc(&(ce)->inflight)
>> > +#define intel_context_inflight_dec(ce) ptr_count_dec(&(ce)->inflight)
>> 
>> Just curious here that what you consider the advantages of carrying
>> this info with the pointer?
>
> Packing. I just need a bit to track status, and one for overflow.
>
>> > +static inline u32 intel_hws_preempt_address(struct intel_engine_cs *engine)
>> > +{
>> > +     return (i915_ggtt_offset(engine->status_page.vma) +
>> > +             I915_GEM_HWS_PREEMPT_ADDR);
>> > +}
>> > +
>> > +#define ring_pause(E) ((E)->status_page.addr[I915_GEM_HWS_PREEMPT])
>> 
>> Scary. Please lets make a function of ring_pause and use
>> intel_write_status_page in it.
>
> I'd rather not unless you do __intel_write_state_page.
>
>> So I guess you have and you want squeeze the latency fruit.
>> 
>> When we have everything in place, CI is green and
>> everyone is happy, then we tear it down?
>
> Been there, done that.

My fears come from csb. Granted, it is a diffent
thing with a different direction of writes.

>
>> > @@ -442,13 +443,11 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
>> >               struct intel_engine_cs *owner;
>> >  
>> >               if (i915_request_completed(rq))
>> > -                     break;
>> > +                     continue; /* XXX */
>> 
>> Yeah, but what is the plan with the XXX.
>
> Mulling over tracking context not requests. We still end up with having
> to scan history within a context, so not yet seeing anything to
> encourage me to make the change. I worry about long request queues
> causing preemption latency, as this list is currently only trimmed in
> retirement.
>
> One idea in the background is for a scheduler (contemplating something
> like the isosynchronous MuQSS) and that might call for a change to
> using contexts as the primary, with requests within the contexts.
>
>> > @@ -1223,68 +1217,37 @@ static void process_csb(struct intel_engine_cs *engine)
>> >                * status notifier.
>> >                */
>> >  
>> > -             GEM_TRACE("%s csb[%d]: status=0x%08x:0x%08x, active=0x%x\n",
>> > +             GEM_TRACE("%s csb[%d]: status=0x%08x:0x%08x\n",
>> >                         engine->name, head,
>> > -                       buf[2 * head + 0], buf[2 * head + 1],
>> > -                       execlists->active);
>> > +                       buf[2 * head + 0], buf[2 * head + 1]);
>> >  
>> >               status = buf[2 * head];
>> > -             if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
>> > -                           GEN8_CTX_STATUS_PREEMPTED))
>> > -                     execlists_set_active(execlists,
>> > -                                          EXECLISTS_ACTIVE_HWACK);
>> > -             if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
>> > -                     execlists_clear_active(execlists,
>> > -                                            EXECLISTS_ACTIVE_HWACK);
>> > -
>> > -             if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
>> > -                     continue;
>> > +             if (status & GEN8_CTX_STATUS_IDLE_ACTIVE) {
>> > +promote:
>> > +                     GEM_BUG_ON(!assert_pending_valid(execlists, "promote"));
>> > +                     execlists->active =
>> > +                             memcpy(execlists->inflight,
>> > +                                    execlists->pending,
>> > +                                    execlists_num_ports(execlists) *
>> > +                                    sizeof(*execlists->pending));
>> > +                     execlists->pending[0] = NULL;
>> 
>> I can't decide if comment or a helper inline function would
>> serve better as documentation of between inflight and pending
>> movement.
>
> The magic is just this function, I think process_csb() reads quite
> nicely with the 3 branches and switching between different states. It's
> about 8 lines without the comments and asserts.
>

Agreed. It is more compact and more readable with this patch.

>> I guess it is better to be left as a future work after
>> the dust settles.
>> 
>> Just general yearning for a similar kind of level of documentation
>> steps as in dequeue.
>> 
>> >  
>> > -             /* We should never get a COMPLETED | IDLE_ACTIVE! */
>> > -             GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
>> 
>> Is our assert coverage going to suffer?
>
> You've looked at the added asserts and tracing; I claim we get stronger.
>
>> > @@ -2514,15 +2452,29 @@ static u32 *gen8_emit_wa_tail(struct i915_request *request, u32 *cs)
>> >       return cs;
>> >  }
>> >  
>> > +static u32 *emit_preempt_busywait(struct i915_request *request, u32 *cs)
>> > +{
>> > +     *cs++ = MI_SEMAPHORE_WAIT |
>> > +             MI_SEMAPHORE_GLOBAL_GTT |
>> > +             MI_SEMAPHORE_POLL |
>> > +             MI_SEMAPHORE_SAD_EQ_SDD;
>> > +     *cs++ = 0;
>> > +     *cs++ = intel_hws_preempt_address(request->engine);
>> > +     *cs++ = 0;
>> > +
>> > +     return cs;
>> > +}
>> > +
>> >  static u32 *gen8_emit_fini_breadcrumb(struct i915_request *request, u32 *cs)
>> >  {
>> >       cs = gen8_emit_ggtt_write(cs,
>> >                                 request->fence.seqno,
>> >                                 request->timeline->hwsp_offset,
>> >                                 0);
>> > -
>> >       *cs++ = MI_USER_INTERRUPT;
>> > +
>> >       *cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
>> 
>> This was discussed in irc, could warrant a comment here of
>> why this is needed. Precious info.
>
> Why the ARB, for reasons of yore. The comment for why we need it is
> actually in bb_start.
>
> commit 279f5a00c9a9b39f4f6e9813e6d4da8c181d34c8
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Thu Oct 5 20:10:05 2017 +0100
>
>     drm/i915/execlists: Add a comment for the extra MI_ARB_ENABLE

Ok, looks like it.

I do like the new way of handling ports.

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

>
>
>> > +     cs = emit_preempt_busywait(request, cs);
>
> Why we use the semaphore? That should be explained in dequeue upon
> setting up the preemption.
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx