[Intel-gfx] [PATCH 06/31] drm/i915/execlists: Unify CSB access pointers

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-06-27 14:24:53)
> 
> On 27/06/2018 14:09, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-06-27 14:03:07)
> >>
> >> On 27/06/2018 11:35, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2018-06-27 10:52:45)
> >>>>
> >>>> On 25/06/2018 10:48, Chris Wilson wrote:
> >>>>> @@ -1109,16 +1089,11 @@ static void process_csb(struct intel_engine_cs *engine)
> >>>>>                 } else {
> >>>>>                         port_set(port, port_pack(rq, count));
> >>>>>                 }
> >>>>> -     }
> >>>>> +     } while (head != tail);
> >>>>>     
> >>>>> -     if (head != execlists->csb_head) {
> >>>>> -             execlists->csb_head = head;
> >>>>> -             writel(_MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, head << 8),
> >>>>> -                    i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)));
> >>>>> -     }
> >>>>> -
> >>>>> -     if (unlikely(fw))
> >>>>> -             intel_uncore_forcewake_put(i915, execlists->fw_domains);
> >>>>> +     writel(_MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, head << 8),
> >>>>> +            execlists->csb_read);
> >>>>
> >>>> Continuing from the last round - so what to do with this one? It does
> >>>> need forcewake. So I think it needs to go if we are claiming there is no
> >>>> mmio any longer.
> >>>
> >>>   From last round, we decided it didn't, or at least concluded the
> >>> (from the lack of) evidence that it does not, because we are not using
> >>> forcewake right now...
> >>
> >> But we are not sure if our writes stick 100% of the time due using the
> >> HWSP path. And we are wasting time on MMIO for nothing. Put an "if
> >> (execlists->csb_use_mmio)" on it?
> > 
> > We only ever read from the status buffer. We always commit to HW with a
> > mmio write to let the HW know how far we read up to (there's no slot in
> > the HWSP for our read pointer, just the HW write pointer).
> 
> This is the field not used by the HW. I am saying that if we want to 
> keep writing to it, lets write to it only in the mode in which we are 
> also reading from it. Since this is GVT (csb_use_mmio), the missing 
> forcewake problem automatically goes away then. (Comment at the call 
> site to say this would also be good.)

This is the field that lets HW know how far we have read up to so it
doesn't overwrite unread entries. (At least if it was obeying classing
ringbuffer rules.)
-Chris