Re: [PATCH 6/7] drm/i915: Convert execlists_ctx_descriptor() for requests

[Mika Kuoppala <miku@iki.fi>]

On Wed Jul 08 2015 at 18:04:44 +0100, Dave Gordon wrote:
> On 08/07/15 17:40, Yu Dai wrote:
> >On 07/03/2015 07:09 AM, Mika Kuoppala wrote:
> >>Pass around requests to carry context deeper in callchain.
> >>
> >>Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/intel_lrc.c | 14 ++++++--------
> >>  1 file changed, 6 insertions(+), 8 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> >>b/drivers/gpu/drm/i915/intel_lrc.c
> >>index c3c029a..67ff460 100644
> >>--- a/drivers/gpu/drm/i915/intel_lrc.c
> >>+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >>@@ -261,10 +261,11 @@ u32 intel_execlists_ctx_id(struct
> >>drm_i915_gem_object *ctx_obj)
> >>      return lrca >> 12;
> >>  }
> >>-static uint64_t execlists_ctx_descriptor(struct intel_engine_cs *ring,
> >>-                     struct drm_i915_gem_object *ctx_obj)
> >>+static uint64_t execlists_ctx_descriptor(struct drm_i915_gem_request
> >>*rq)
> >>  {
> >
> >GuC patch series tries to utilize this function. However, changing from
> >ring/context to request makes it unusable in the case where request is
> >not needed (not available). This context descriptor has nothing to do
> >with drm_i915_gem_request IMO. And it is waste of time to call it for
> >each batch submission. This value is fixed when context is pinned. Maybe
> >add a member ctx_desc into intel_context->engine; initialize the value
> >within intel_lr_context_pin; then use it whenever needed.
> >
> >Thanks,
> >Alex
> 
> Yes, I've effectively reversed this in the next GuC submission
> series, since we don't have a request during setup, and this
> function doesn't actually need one either; it's only being used as a
> handle for extracting the context and ring.
> 
> So in my version it will take an intel_context and a ring so it's up
> to the caller to extract those from the drm_i915_gem_request
> (rq->ctx) and (rq->ring) and pass them separately. Then the GuC can
> use it during setup as well as at runtime :)
> 

I admit I went one step too far on this one in the series. The bspec says
the context ids created should be unique across all engines and all execlists.
I was concerned that our lrca only setup and resubmission for
pre-emption would break this rule. (and be the reason for skl hickups)

The bspec has a note about creating context id with ring/counter(seqno)/lrca
combination. Thus the request -> descriptor creation.

I didn't post that patch as it didn't cure the skl hang. But it made
debugging the context status queue easier as seqnos were part of
context_id.

-Mika

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx