-----Original Message-----
From: Tvrtko Ursulin [mailto:tvrtko.ursu...@linux.intel.com]
Sent: Monday, May 16, 2016 7:00 AM
To: Wang, Zhi A <zhi.a.w...@intel.com>; intel-gfx@lists.freedesktop.org;
Gordon, David S <david.s.gor...@intel.com>; joonas.lahti...@linux.intel.com;
Tian, Kevin <kevin.t...@intel.com>; Lv, Zhiyuan <zhiyuan...@intel.com>
Subject: Re: [Intel-gfx] [PATCH 11/15] drm/i915: Introduce execlist context
status change notification
On 15/05/16 18:32, Zhi Wang wrote:
> This patch introduces an approach to track the execlist context status
> change.
>
> GVT-g uses GVT context as the "shadow context". The content inside GVT
> context will be copied back to guest after the context is idle. So
> GVT-g has to know the status of the execlist context.
>
> This function is configurable in the context creation service.
> Currently, Only GVT-g will create the "status-change-notification"
> enabled GEM context.
>
> Signed-off-by: Zhi Wang <zhi.a.w...@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 5 +++++
> drivers/gpu/drm/i915/intel_lrc.c | 23 +++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_lrc.h | 5 +++++
> 3 files changed, 33 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h index 7f050a3..1c9e865 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -884,6 +884,8 @@ struct intel_context {
> bool initialised;
> bool skip_init_context;
> u32 ring_buffer_size;
> + bool enable_status_change_notification;
This flag will be per context and not per context-engine, correct? In which
case it would be worth moving it to the context. Would make
execlists_context_status_change cheaper as well.
OK. Will do.
> + struct atomic_notifier_head status_notifier_head;
> } engine[I915_NUM_ENGINES];
> bool use_48bit_addressing_mode;
>
> @@ -2403,6 +2405,9 @@ struct drm_i915_gem_request {
>
> /** Execlists context hardware id. */
> unsigned ctx_hw_id;
> +
> + /** GVT workload **/
> + void *gvt_workload;
Why is this a void *, for what it will be used ? I can't see in this patch
series what will be stored here so asking. Is it only temporary until further
patches?
Also, is it really per-request? I am just wondering since other patches are
mostly concerned about contexts.
Will remove that according to today discussion.
> };
>
> struct drm_i915_gem_request * __must_check diff --git
> a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 0a96d4a..aeaea2e 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -415,6 +415,19 @@ static void execlists_submit_requests(struct
> drm_i915_gem_request *rq0,
> spin_unlock_irq(&dev_priv->uncore.lock);
> }
>
> +static inline void execlists_context_status_change(
> + struct drm_i915_gem_request *req,
> + unsigned long status)
> +{
> + if (!req->ctx->engine[req->engine->id].
> + enable_status_change_notification)
> + return;
> +
> + atomic_notifier_call_chain(
> + &req->ctx->engine[req->engine->id].status_notifier_head,
> + status, req->gvt_workload);
> +}
I would really like to see this compiled out when !CONFIG_DRM_I915_GVT.
Otherwise is just adding useless conditionals on submission fast paths.
Let's do that.
> +
> static void execlists_context_unqueue(struct intel_engine_cs *engine)
> {
> struct drm_i915_gem_request *req0 = NULL, *req1 = NULL; @@ -450,6
> +463,11 @@ static void execlists_context_unqueue(struct intel_engine_cs
> *engine)
> if (unlikely(!req0))
> return;
>
> + execlists_context_status_change(req0, CONTEXT_SCHEDULE_IN);
> +
> + if (req1)
> + execlists_context_status_change(req1, CONTEXT_SCHEDULE_IN);
> +
> if (req0->elsp_submitted & engine->idle_lite_restore_wa) {
> /*
> * WaIdleLiteRestore: make sure we never cause a lite restore
> @@
> -488,6 +506,8 @@ execlists_check_remove_request(struct intel_engine_cs
> *engine, u32 ctx_id)
> if (--head_req->elsp_submitted > 0)
> return 0;
>
> + execlists_context_status_change(head_req, CONTEXT_SCHEDULE_OUT);
> +
> list_del(&head_req->execlist_link);
> i915_gem_request_unreference(head_req);
>
> @@ -2541,6 +2561,9 @@ static int execlists_context_deferred_alloc(struct
> intel_context *ctx,
> ctx->engine[engine->id].state = ctx_obj;
> ctx->engine[engine->id].initialised = engine_initialised(ctx,
> engine);
>
> + if (ctx->engine[engine->id].enable_status_change_notification)
> + ATOMIC_INIT_NOTIFIER_HEAD(
> + &ctx->engine[engine->id].status_notifier_head);
> return 0;
>
> error_ringbuf:
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h
> b/drivers/gpu/drm/i915/intel_lrc.h
> index 1afba03..98c94ee 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -57,6 +57,11 @@
> #define GEN8_CSB_READ_PTR(csb_status) \
> (((csb_status) & GEN8_CSB_READ_PTR_MASK) >> 8)
>
> +enum {
> + CONTEXT_SCHEDULE_IN = 0,
> + CONTEXT_SCHEDULE_OUT,
> +};
> +
> /* Logical Rings */
> int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request
> *request);
> int intel_logical_ring_reserve_space(struct drm_i915_gem_request
> *request);
>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
