On 27/04/2017 15:37, Chris Wilson wrote:
On Thu, Apr 20, 2017 at 03:58:19PM +0100, Tvrtko Ursulin wrote:
static void record_context(struct drm_i915_error_context *e,
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 1642fff9cf13..370373c97b81 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -658,7 +658,7 @@ static void nested_enable_signaling(struct drm_i915_gem_request *rq)
static bool i915_guc_dequeue(struct intel_engine_cs *engine)
{
struct execlist_port *port = engine->execlist_port;
- struct drm_i915_gem_request *last = port[0].request;
+ struct drm_i915_gem_request *last = port[0].request_count;
It's confusing that in this new scheme sometimes we have direct
access to the request and sometimes we have to go through the
port_request macro.
So maybe we should always use the port_request macro. Hm, could we
invent a new type to help enforce that? Like:
struct drm_i915_gem_port_request_slot {
struct drm_i915_gem_request *req_count;
};
And then execlist port would contain these and helpers would need to
be functions?
I've also noticed some GVT/GuC patches which sounded like they are
adding the same single submission constraints so maybe now is the
time to unify the dequeue? (Haven't looked at those patches deeper
than the subject line so might be wrong.)
Not sure 100% of all the above, would need to sketch it. What are
your thoughts?
I forsee a use for the count in guc as well, so conversion is ok with
me.
Conversion to a wrapper structure as I proposed or keeping it as you
have it?
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index d25b88467e5e..39b733e5cfd3 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -377,8 +377,12 @@ struct intel_engine_cs {
/* Execlists */
struct tasklet_struct irq_tasklet;
struct execlist_port {
- struct drm_i915_gem_request *request;
- unsigned int count;
+ struct drm_i915_gem_request *request_count;
Would req(uest)_slot maybe be better?
It's definitely a count (of how many times this request has been
submitted), and I like long verbose names when I don't want them to be
used directly. So expect guc to be tidied.
It is a pointer and a count. My point was that request_count sounds too
much like a count of how many times has something been done or happened
to the request.
Request_slot was my attempt to make it obvious in the name itself there
is more to it. And the wrapper struct was another step further, plus the
idea was it would make sure you always need to access this field via the
accessor. Since I think going sometimes directly and sometimes via
wrapper is too fragile.
Anyway, my big issue is I am not sure if we are in agreement or not. Do
you agree going with the wrapper structure makes sense or not?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
