Re: [Intel-gfx] [RFC 4/4] drm/i915: Stop tracking execlists retired requests
On Mon, Apr 11, 2016 at 10:10:56AM +0100, Tvrtko Ursulin wrote:
>
> On 08/04/16 15:57, Chris Wilson wrote:
> >On Fri, Apr 08, 2016 at 02:54:58PM +0100, Tvrtko Ursulin wrote:
> >>@@ -615,11 +613,6 @@ static void execlists_context_queue(struct
> >>drm_i915_gem_request *request)
> >>struct drm_i915_gem_request *cursor;
> >>int num_elements = 0;
> >>
> >>- if (request->ctx != request->i915->kernel_context)
> >>- intel_lr_context_pin(request->ctx, engine);
> >>-
> >>- i915_gem_request_reference(request);
> >>-
> >>spin_lock_bh(>execlist_lock);
> >>
> >>list_for_each_entry(cursor, >execlist_queue, execlist_link)
> >>@@ -636,11 +629,12 @@ static void execlists_context_queue(struct
> >>drm_i915_gem_request *request)
> >>if (request->ctx == tail_req->ctx) {
> >>WARN(tail_req->elsp_submitted != 0,
> >>"More than 2 already-submitted reqs queued\n");
> >>- list_move_tail(_req->execlist_link,
> >>- >execlist_retired_req_list);
> >>+ list_del(_req->execlist_link);
> >>+ i915_gem_request_unreference(tail_req);
> >>}
> >>}
> >>
> >>+ i915_gem_request_reference(request);
> >>list_add_tail(>execlist_link, >execlist_queue);
> >
> >If you want to get truly radical, we do not need the ref on the request
> >until it is submitted to hardware. (As the request cannot be retired
> >until it has done so, it can leave the execlist_queue until we commit it
> >to hw, or perform the cancel).
>
> Don't know. It is simple and nice that reference is tied to presence
> on execlist_queue.
>
> More importantly, the patch as presented has a flaw that it
> dereferences req->ctx from execlists_check_remove_request where the
> context pin may have disappeared already due context complete
> interrupts getting behind when coallescing.
>
> I will need to cache ctx_id in the request I think. It is fine do to
> that since ctx id must be unique and stable for a request.
For comparison, tracking by requests rather than contexts:
https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet=e23791ef88636aa6e3f850b61ab2c4e027af0e52
-Chris
>
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 4/4] drm/i915: Stop tracking execlists retired requests
On Mon, Apr 11, 2016 at 10:10:56AM +0100, Tvrtko Ursulin wrote:
>
> On 08/04/16 15:57, Chris Wilson wrote:
> >On Fri, Apr 08, 2016 at 02:54:58PM +0100, Tvrtko Ursulin wrote:
> >>@@ -615,11 +613,6 @@ static void execlists_context_queue(struct
> >>drm_i915_gem_request *request)
> >>struct drm_i915_gem_request *cursor;
> >>int num_elements = 0;
> >>
> >>- if (request->ctx != request->i915->kernel_context)
> >>- intel_lr_context_pin(request->ctx, engine);
> >>-
> >>- i915_gem_request_reference(request);
> >>-
> >>spin_lock_bh(>execlist_lock);
> >>
> >>list_for_each_entry(cursor, >execlist_queue, execlist_link)
> >>@@ -636,11 +629,12 @@ static void execlists_context_queue(struct
> >>drm_i915_gem_request *request)
> >>if (request->ctx == tail_req->ctx) {
> >>WARN(tail_req->elsp_submitted != 0,
> >>"More than 2 already-submitted reqs queued\n");
> >>- list_move_tail(_req->execlist_link,
> >>- >execlist_retired_req_list);
> >>+ list_del(_req->execlist_link);
> >>+ i915_gem_request_unreference(tail_req);
> >>}
> >>}
> >>
> >>+ i915_gem_request_reference(request);
> >>list_add_tail(>execlist_link, >execlist_queue);
> >
> >If you want to get truly radical, we do not need the ref on the request
> >until it is submitted to hardware. (As the request cannot be retired
> >until it has done so, it can leave the execlist_queue until we commit it
> >to hw, or perform the cancel).
>
> Don't know. It is simple and nice that reference is tied to presence
> on execlist_queue.
>
> More importantly, the patch as presented has a flaw that it
> dereferences req->ctx from execlists_check_remove_request where the
> context pin may have disappeared already due context complete
> interrupts getting behind when coallescing.
>
> I will need to cache ctx_id in the request I think. It is fine do to
> that since ctx id must be unique and stable for a request.
>
> Maybe even I should pull in your patch which makes ctx ids stable
> and persistent aligned with context existence and not pin. Think
> I've seen something like that somewhere.
Yes, both OA, PASID and vGPU depend upon having stable ctx-id for the
lifetime of the context. Dave expressed a concern that the GuC maybe
interpretting it as the LRCA, but as far as I can see, lrc->ring_lcra
and lrc->context_id are distinct and we never use the global context
identifier itself (the closest is the low 32bits of lrc_desc which do
not include our unique id).
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 4/4] drm/i915: Stop tracking execlists retired requests
On 08/04/16 15:57, Chris Wilson wrote:
On Fri, Apr 08, 2016 at 02:54:58PM +0100, Tvrtko Ursulin wrote:
@@ -615,11 +613,6 @@ static void execlists_context_queue(struct
drm_i915_gem_request *request)
struct drm_i915_gem_request *cursor;
int num_elements = 0;
- if (request->ctx != request->i915->kernel_context)
- intel_lr_context_pin(request->ctx, engine);
-
- i915_gem_request_reference(request);
-
spin_lock_bh(>execlist_lock);
list_for_each_entry(cursor, >execlist_queue, execlist_link)
@@ -636,11 +629,12 @@ static void execlists_context_queue(struct
drm_i915_gem_request *request)
if (request->ctx == tail_req->ctx) {
WARN(tail_req->elsp_submitted != 0,
"More than 2 already-submitted reqs queued\n");
- list_move_tail(_req->execlist_link,
- >execlist_retired_req_list);
+ list_del(_req->execlist_link);
+ i915_gem_request_unreference(tail_req);
}
}
+ i915_gem_request_reference(request);
list_add_tail(>execlist_link, >execlist_queue);
If you want to get truly radical, we do not need the ref on the request
until it is submitted to hardware. (As the request cannot be retired
until it has done so, it can leave the execlist_queue until we commit it
to hw, or perform the cancel).
Don't know. It is simple and nice that reference is tied to presence on
execlist_queue.
More importantly, the patch as presented has a flaw that it dereferences
req->ctx from execlists_check_remove_request where the context pin may
have disappeared already due context complete interrupts getting behind
when coallescing.
I will need to cache ctx_id in the request I think. It is fine do to
that since ctx id must be unique and stable for a request.
Maybe even I should pull in your patch which makes ctx ids stable and
persistent aligned with context existence and not pin. Think I've seen
something like that somewhere.
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 4/4] drm/i915: Stop tracking execlists retired requests
On Fri, Apr 08, 2016 at 02:54:58PM +0100, Tvrtko Ursulin wrote:
> @@ -615,11 +613,6 @@ static void execlists_context_queue(struct
> drm_i915_gem_request *request)
> struct drm_i915_gem_request *cursor;
> int num_elements = 0;
>
> - if (request->ctx != request->i915->kernel_context)
> - intel_lr_context_pin(request->ctx, engine);
> -
> - i915_gem_request_reference(request);
> -
> spin_lock_bh(>execlist_lock);
>
> list_for_each_entry(cursor, >execlist_queue, execlist_link)
> @@ -636,11 +629,12 @@ static void execlists_context_queue(struct
> drm_i915_gem_request *request)
> if (request->ctx == tail_req->ctx) {
> WARN(tail_req->elsp_submitted != 0,
> "More than 2 already-submitted reqs queued\n");
> - list_move_tail(_req->execlist_link,
> ->execlist_retired_req_list);
> + list_del(_req->execlist_link);
> + i915_gem_request_unreference(tail_req);
> }
> }
>
> + i915_gem_request_reference(request);
> list_add_tail(>execlist_link, >execlist_queue);
If you want to get truly radical, we do not need the ref on the request
until it is submitted to hardware. (As the request cannot be retired
until it has done so, it can leave the execlist_queue until we commit it
to hw, or perform the cancel).
Lgtm,
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
