On 04/29/2019 06:03 PM, Chris Wilson wrote:
> Quoting Xiaolin Zhang (2019-04-29 04:10:54)
>> +static void pv_submit(struct intel_engine_cs *engine)
>> +{
>> + struct intel_engine_execlists * const execlists = &engine->execlists;
>> + struct execlist_port *port = execlists->port;
>> + unsigned int n;
>> + struct gvt_shared_page *shared_page = engine->i915->vgpu.shared_page;
>> + u64 descs[2];
>> +
>> + for (n = 0; n < execlists_num_ports(execlists); n++) {
>> + struct i915_request *rq;
>> + unsigned int count = 0;
>> +
>> + descs[n] = 0;
>> + rq = port_unpack(&port[n], &count);
>> + if (rq && count == 0) {
>> + port_set(&port[n], port_pack(rq, ++count));
>> + descs[n] = execlists_update_context(rq);
>> + }
>> + }
>> +
>> + spin_lock(&engine->i915->vgpu.shared_page_lock[engine->id]);
> Only one engine at a time now accesses that portion of pv_elsp, so the
> spin lock is not required per-se, aiui.
>
> The problem is that there is no coordination between pv_submit and the
> other side of the pipe, as far as I can see. So it seems very possible
> for us to overwrite entries before they have been read. I expect to see
> some ack mechanism for VGT_G2V_PV_SUBMISSION.
>
>> + for (n = 0; n < execlists_num_ports(execlists); n++)
>> + shared_page->pv_elsp[engine->id].descs[n] = descs[n];
> I'd recommend not using engine->id, that's just our internal id and may
> vary between host/guest. Use engine->hw_id? Or negotiate using pv setup
> and store pv_id?
>
> But starting to look more solid over all. We just need to finish
> splitting out the various similar-but-quite-different-really submission
> backends. :)
> -Chris
>
Chris, Thanks your great comments and will be addressed next version
(will remove spin lock, use hw_id, add ack mechanism and refine patches).
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
