Re: [Intel-gfx] [PATCH v3] drm/i915/scheduler: add gvt notification for guc submission

2017-03-23 Thread Dong, Chuanxiao 


> -Original Message-
> From: Tvrtko Ursulin [mailto:tvrtko.ursu...@linux.intel.com]
> Sent: Thursday, March 23, 2017 5:52 PM
> To: Dong, Chuanxiao; intel-gfx@lists.freedesktop.org
> Cc: intel-gvt-...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v3] drm/i915/scheduler: add gvt notification
> for guc submission
> 
> 
> On 22/03/2017 06:34, Chuanxiao Dong wrote:
> > GVT request needs a manual mmio load/restore. Before GuC submit a
> > request, send notification to gvt for mmio loading. And after the GuC
> > finished this GVT request, notify gvt again for mmio restore. This
> > follows the usage when using execlists submission.
> >
> > v2: use context_status_change instead of
> execlists_context_status_change
> > for better understanding (ZhengXiao)
> > v3: remove the comment as it is obvious and not friendly to
> > the caller (Kevin)
> >
> > Cc: xiao.zh...@intel.com
> > Cc: kevin.t...@intel.com
> > Signed-off-by: Chuanxiao Dong <chuanxiao.d...@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_guc_submission.c |  3 +++
> >  drivers/gpu/drm/i915/intel_lrc.c   | 21 +++--
> >  drivers/gpu/drm/i915/intel_lrc.h   | 13 +
> >  3 files changed, 19 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
> > b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index 055467a..0195547 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -520,6 +520,8 @@ static void __i915_guc_submit(struct
> drm_i915_gem_request *rq)
> > unsigned long flags;
> > int b_ret;
> >
> > +   context_status_change(rq, INTEL_CONTEXT_SCHEDULE_IN);
> > +
> > /* WA to flush out the pending GMADR writes to ring buffer. */
> > if (i915_vma_is_map_and_fenceable(rq->ring->vma))
> > POSTING_READ_FW(GUC_STATUS);
> > @@ -634,6 +636,7 @@ static void i915_guc_irq_handler(unsigned long data)
> > rq = port[0].request;
> > while (rq && i915_gem_request_completed(rq)) {
> > trace_i915_gem_request_out(rq);
> > +   context_status_change(rq,
> INTEL_CONTEXT_SCHEDULE_OUT);
> 
> Does GVT care that the context will still be active on the GPU for a small
> window after this notification? (User interrupt happens before context
> complete, which GuC hides from the driver.)

Actually GVT cares. GVT driver will check the context status register to make 
sure the status is ACTIVE_IDLE in this notification before manually doing the 
context switch out for the GuC submission case.

Thanks
Chuanxiao

> 
> Regards,
> 
> Tvrtko
> 
> > i915_gem_request_put(rq);
> > port[0].request = port[1].request;
> > port[1].request = NULL;
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> > b/drivers/gpu/drm/i915/intel_lrc.c
> > index eec1e71..24c69b5 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -295,21 +295,6 @@ uint64_t intel_lr_context_descriptor(struct
> i915_gem_context *ctx,
> > return ctx->engine[engine->id].lrc_desc;  }
> >
> > -static inline void
> > -execlists_context_status_change(struct drm_i915_gem_request *rq,
> > -   unsigned long status)
> > -{
> > -   /*
> > -* Only used when GVT-g is enabled now. When GVT-g is disabled,
> > -* The compiler should eliminate this function as dead-code.
> > -*/
> > -   if (!IS_ENABLED(CONFIG_DRM_I915_GVT))
> > -   return;
> > -
> > -   atomic_notifier_call_chain(>engine->context_status_notifier,
> > -  status, rq);
> > -}
> > -
> >  static void
> >  execlists_update_context_pdps(struct i915_hw_ppgtt *ppgtt, u32
> > *reg_state)  { @@ -350,7 +335,7 @@ static void
> > execlists_submit_ports(struct intel_engine_cs *engine)
> >
> > GEM_BUG_ON(port[0].count > 1);
> > if (!port[0].count)
> > -   execlists_context_status_change(port[0].request,
> > +   context_status_change(port[0].request,
> >
>   INTEL_CONTEXT_SCHEDULE_IN);
> > desc[0] = execlists_update_context(port[0].request);
> > GEM_DEBUG_EXEC(port[0].context_id = upper_32_bits(desc[0]));
> @@
> > -358,7 +343,7 @@ static void execlists_submit_ports(struct
> > intel_engine_cs *engine)
> >
> > if (port[1].reques

Re: [Intel-gfx] [PATCH v3] drm/i915/scheduler: add gvt notification for guc submission

2017-03-23 Thread Dong, Chuanxiao 


> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Chris Wilson
> Sent: Thursday, March 23, 2017 5:43 PM
> To: Joonas Lahtinen
> Cc: intel-gfx@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org;
> Dong, Chuanxiao
> Subject: Re: [Intel-gfx] [PATCH v3] drm/i915/scheduler: add gvt notification
> for guc submission
> 
> On Thu, Mar 23, 2017 at 11:37:32AM +0200, Joonas Lahtinen wrote:
> > On ke, 2017-03-22 at 14:34 +0800, Chuanxiao Dong wrote:
> > > @@ -87,5 +87,18 @@ uint64_t intel_lr_context_descriptor(struct
> > > i915_gem_context *ctx,
> > >  /* Execlists */
> > >  int intel_sanitize_enable_execlists(struct drm_i915_private *dev_priv,
> > >   int enable_execlists);
> > > +static inline void
> > > +context_status_change(struct drm_i915_gem_request *rq, unsigned
> > > +long status)
> >
> > This needs intel_lr_ prefix now that it's in .h file.
> 
> But not in this header. The event is not strictly tied to execlists, it 
> consumes
> request for starters - but on the other hand it is gvt specific.
> 
> I'm tempted to say intel_gvt_notify_context_status().

It makes sense. Will change the name in the next version.

Thanks
Chuanxiao

> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915/scheduler: add gvt notification for guc submission

2017-03-23 Thread Dong, Chuanxiao 


> -Original Message-
> From: Joonas Lahtinen [mailto:joonas.lahti...@linux.intel.com]
> Sent: Thursday, March 23, 2017 5:38 PM
> To: Dong, Chuanxiao; intel-gfx@lists.freedesktop.org
> Cc: intel-gvt-...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v3] drm/i915/scheduler: add gvt notification
> for guc submission
> 
> On ke, 2017-03-22 at 14:34 +0800, Chuanxiao Dong wrote:
> > GVT request needs a manual mmio load/restore. Before GuC submit a
> > request, send notification to gvt for mmio loading. And after the GuC
> > finished this GVT request, notify gvt again for mmio restore. This
> > follows the usage when using execlists submission.
> >
> > v2: use context_status_change instead of
> > execlists_context_status_change
> > for better understanding (ZhengXiao)
> > v3: remove the comment as it is obvious and not friendly to
> > the caller (Kevin)
> >
> > Cc: xiao.zh...@intel.com
> > Cc: kevin.t...@intel.com
> > Signed-off-by: Chuanxiao Dong <chuanxiao.d...@intel.com>
> 
> 
> 
> > @@ -350,7 +335,7 @@ static void execlists_submit_ports(struct
> > intel_engine_cs *engine)
> >
> >     GEM_BUG_ON(port[0].count > 1);
> >     if (!port[0].count)
> > -   execlists_context_status_change(port[0].request,
> > +   context_status_change(port[0].request,
> >
>   INTEL_CONTEXT_SCHEDULE_IN);
> 
> Fix indent.
> 
> >     desc[0] = execlists_update_context(port[0].request);
> >     GEM_DEBUG_EXEC(port[0].context_id = upper_32_bits(desc[0]));
> @@
> > -358,7 +343,7 @@ static void execlists_submit_ports(struct
> > intel_engine_cs *engine)
> >
> >     if (port[1].request) {
> >     GEM_BUG_ON(port[1].count);
> > -   execlists_context_status_change(port[1].request,
> > +   context_status_change(port[1].request,
> >
>   INTEL_CONTEXT_SCHEDULE_IN);
> 
> Ditto.
> 
> > @@ -581,7 +566,7 @@ static void intel_lrc_irq_handler(unsigned long data)
> >     if (--port[0].count == 0) {
> >     GEM_BUG_ON(status &
> GEN8_CTX_STATUS_PREEMPTED);
> >
>   GEM_BUG_ON(!i915_gem_request_completed(port[0].request));
> > -
>   execlists_context_status_change(port[0].request,
> > +   context_status_change(port[0].request,
> >
>   INTEL_CONTEXT_SCHEDULE_OUT);
> 
> Ditto.
> 
> > @@ -87,5 +87,18 @@ uint64_t intel_lr_context_descriptor(struct
> > i915_gem_context *ctx,
> >  /* Execlists */
> >  int intel_sanitize_enable_execlists(struct drm_i915_private *dev_priv,
> >     int enable_execlists);
> > +static inline void
> > +context_status_change(struct drm_i915_gem_request *rq, unsigned long
> > +status)
> 
> This needs intel_lr_ prefix now that it's in .h file.
> 
> With those changes (make sure context_status_change doesn't become
> over character 80 line), this is;

Sure. Will fix in the next version.

Thanks
Chuanxiao

> 
> Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> 
> Regards, Joonas
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915/scheduler: add gvt notification for guc submission

2017-03-23 Thread Tvrtko Ursulin 


On 22/03/2017 06:34, Chuanxiao Dong wrote:

GVT request needs a manual mmio load/restore. Before GuC submit
a request, send notification to gvt for mmio loading. And after
the GuC finished this GVT request, notify gvt again for mmio
restore. This follows the usage when using execlists submission.

v2: use context_status_change instead of execlists_context_status_change
for better understanding (ZhengXiao)
v3: remove the comment as it is obvious and not friendly to
the caller (Kevin)

Cc: xiao.zh...@intel.com
Cc: kevin.t...@intel.com
Signed-off-by: Chuanxiao Dong 
---
 drivers/gpu/drm/i915/i915_guc_submission.c |  3 +++
 drivers/gpu/drm/i915/intel_lrc.c   | 21 +++--
 drivers/gpu/drm/i915/intel_lrc.h   | 13 +
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 055467a..0195547 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -520,6 +520,8 @@ static void __i915_guc_submit(struct drm_i915_gem_request 
*rq)
unsigned long flags;
int b_ret;

+   context_status_change(rq, INTEL_CONTEXT_SCHEDULE_IN);
+
/* WA to flush out the pending GMADR writes to ring buffer. */
if (i915_vma_is_map_and_fenceable(rq->ring->vma))
POSTING_READ_FW(GUC_STATUS);
@@ -634,6 +636,7 @@ static void i915_guc_irq_handler(unsigned long data)
rq = port[0].request;
while (rq && i915_gem_request_completed(rq)) {
trace_i915_gem_request_out(rq);
+   context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);


Does GVT care that the context will still be active on the GPU for a 
small window after this notification? (User interrupt happens before 
context complete, which GuC hides from the driver.)


Regards,

Tvrtko


i915_gem_request_put(rq);
port[0].request = port[1].request;
port[1].request = NULL;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index eec1e71..24c69b5 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -295,21 +295,6 @@ uint64_t intel_lr_context_descriptor(struct 
i915_gem_context *ctx,
return ctx->engine[engine->id].lrc_desc;
 }

-static inline void
-execlists_context_status_change(struct drm_i915_gem_request *rq,
-   unsigned long status)
-{
-   /*
-* Only used when GVT-g is enabled now. When GVT-g is disabled,
-* The compiler should eliminate this function as dead-code.
-*/
-   if (!IS_ENABLED(CONFIG_DRM_I915_GVT))
-   return;
-
-   atomic_notifier_call_chain(>engine->context_status_notifier,
-  status, rq);
-}
-
 static void
 execlists_update_context_pdps(struct i915_hw_ppgtt *ppgtt, u32 *reg_state)
 {
@@ -350,7 +335,7 @@ static void execlists_submit_ports(struct intel_engine_cs 
*engine)

GEM_BUG_ON(port[0].count > 1);
if (!port[0].count)
-   execlists_context_status_change(port[0].request,
+   context_status_change(port[0].request,
INTEL_CONTEXT_SCHEDULE_IN);
desc[0] = execlists_update_context(port[0].request);
GEM_DEBUG_EXEC(port[0].context_id = upper_32_bits(desc[0]));
@@ -358,7 +343,7 @@ static void execlists_submit_ports(struct intel_engine_cs 
*engine)

if (port[1].request) {
GEM_BUG_ON(port[1].count);
-   execlists_context_status_change(port[1].request,
+   context_status_change(port[1].request,
INTEL_CONTEXT_SCHEDULE_IN);
desc[1] = execlists_update_context(port[1].request);
GEM_DEBUG_EXEC(port[1].context_id = upper_32_bits(desc[1]));
@@ -581,7 +566,7 @@ static void intel_lrc_irq_handler(unsigned long data)
if (--port[0].count == 0) {
GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED);

GEM_BUG_ON(!i915_gem_request_completed(port[0].request));
-   execlists_context_status_change(port[0].request,
+   context_status_change(port[0].request,

INTEL_CONTEXT_SCHEDULE_OUT);

trace_i915_gem_request_out(port[0].request);
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index e8015e7..51e1be9 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -87,5 +87,18 @@ uint64_t intel_lr_context_descriptor(struct i915_gem_context 
*ctx,
 /* Execlists */
 int

Re: [Intel-gfx] [PATCH v3] drm/i915/scheduler: add gvt notification for guc submission

2017-03-23 Thread Chris Wilson 
On Thu, Mar 23, 2017 at 11:37:32AM +0200, Joonas Lahtinen wrote:
> On ke, 2017-03-22 at 14:34 +0800, Chuanxiao Dong wrote:
> > @@ -87,5 +87,18 @@ uint64_t intel_lr_context_descriptor(struct 
> > i915_gem_context *ctx,
> >  /* Execlists */
> >  int intel_sanitize_enable_execlists(struct drm_i915_private *dev_priv,
> >     int enable_execlists);
> > +static inline void
> > +context_status_change(struct drm_i915_gem_request *rq, unsigned long 
> > status)
> 
> This needs intel_lr_ prefix now that it's in .h file.

But not in this header. The event is not strictly tied to execlists, it
consumes request for starters - but on the other hand it is gvt specific.

I'm tempted to say intel_gvt_notify_context_status().
-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] [PATCH v3] drm/i915/scheduler: add gvt notification for guc submission

2017-03-23 Thread Joonas Lahtinen 
On ke, 2017-03-22 at 14:34 +0800, Chuanxiao Dong wrote:
> GVT request needs a manual mmio load/restore. Before GuC submit
> a request, send notification to gvt for mmio loading. And after
> the GuC finished this GVT request, notify gvt again for mmio
> restore. This follows the usage when using execlists submission.
> 
> v2: use context_status_change instead of execlists_context_status_change
> for better understanding (ZhengXiao)
> v3: remove the comment as it is obvious and not friendly to
> the caller (Kevin)
> 
> Cc: xiao.zh...@intel.com
> Cc: kevin.t...@intel.com
> Signed-off-by: Chuanxiao Dong 



> @@ -350,7 +335,7 @@ static void execlists_submit_ports(struct intel_engine_cs 
> *engine)
>  
>   GEM_BUG_ON(port[0].count > 1);
>   if (!port[0].count)
> - execlists_context_status_change(port[0].request,
> + context_status_change(port[0].request,
>   INTEL_CONTEXT_SCHEDULE_IN);

Fix indent.

>   desc[0] = execlists_update_context(port[0].request);
>   GEM_DEBUG_EXEC(port[0].context_id = upper_32_bits(desc[0]));
> @@ -358,7 +343,7 @@ static void execlists_submit_ports(struct intel_engine_cs 
> *engine)
>  
>   if (port[1].request) {
>   GEM_BUG_ON(port[1].count);
> - execlists_context_status_change(port[1].request,
> + context_status_change(port[1].request,
>   INTEL_CONTEXT_SCHEDULE_IN);

Ditto.

> @@ -581,7 +566,7 @@ static void intel_lrc_irq_handler(unsigned long data)
>   if (--port[0].count == 0) {
>   GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED);
>   
> GEM_BUG_ON(!i915_gem_request_completed(port[0].request));
> - execlists_context_status_change(port[0].request,
> + context_status_change(port[0].request,
>   
> INTEL_CONTEXT_SCHEDULE_OUT);

Ditto.

> @@ -87,5 +87,18 @@ uint64_t intel_lr_context_descriptor(struct 
> i915_gem_context *ctx,
>  /* Execlists */
>  int intel_sanitize_enable_execlists(struct drm_i915_private *dev_priv,
>   int enable_execlists);
> +static inline void
> +context_status_change(struct drm_i915_gem_request *rq, unsigned long status)

This needs intel_lr_ prefix now that it's in .h file.

With those changes (make sure context_status_change doesn't become over
character 80 line), this is;

Reviewed-by: Joonas Lahtinen 

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915/scheduler: add gvt notification for guc submission

2017-03-23 Thread Tian, Kevin 
> From: Dong, Chuanxiao
> Sent: Thursday, March 23, 2017 1:29 PM
> 
> Ping for review. GVT relies on the notification before a request submitted
> and after a request completed, just like when using execlist mode submission.
> 
> > -Original Message-
> > From: Dong, Chuanxiao
> > Sent: Wednesday, March 22, 2017 2:35 PM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: intel-gvt-...@lists.freedesktop.org; Dong, Chuanxiao
> > ; Zheng, Xiao ; Tian,
> > Kevin 
> > Subject: [PATCH v3] drm/i915/scheduler: add gvt notification for guc
> > submission
> >
> > GVT request needs a manual mmio load/restore. Before GuC submit a
> > request, send notification to gvt for mmio loading. And after the GuC
> > finished this GVT request, notify gvt again for mmio restore. This
> > follows the usage when using execlists submission.
> >
> > v2: use context_status_change instead of execlists_context_status_change
> > for better understanding (ZhengXiao)
> > v3: remove the comment as it is obvious and not friendly to
> > the caller (Kevin)
> >
> > Cc: xiao.zh...@intel.com
> > Cc: kevin.t...@intel.com
> > Signed-off-by: Chuanxiao Dong 

Reviewed-by: Kevin Tian 

> > ---
> >  drivers/gpu/drm/i915/i915_guc_submission.c |  3 +++
> >  drivers/gpu/drm/i915/intel_lrc.c   | 21 +++--
> >  drivers/gpu/drm/i915/intel_lrc.h   | 13 +
> >  3 files changed, 19 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
> > b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index 055467a..0195547 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -520,6 +520,8 @@ static void __i915_guc_submit(struct
> > drm_i915_gem_request *rq)
> > unsigned long flags;
> > int b_ret;
> >
> > +   context_status_change(rq, INTEL_CONTEXT_SCHEDULE_IN);
> > +
> > /* WA to flush out the pending GMADR writes to ring buffer. */
> > if (i915_vma_is_map_and_fenceable(rq->ring->vma))
> > POSTING_READ_FW(GUC_STATUS);
> > @@ -634,6 +636,7 @@ static void i915_guc_irq_handler(unsigned long
> data)
> > rq = port[0].request;
> > while (rq && i915_gem_request_completed(rq)) {
> > trace_i915_gem_request_out(rq);
> > +   context_status_change(rq,
> > INTEL_CONTEXT_SCHEDULE_OUT);
> > i915_gem_request_put(rq);
> > port[0].request = port[1].request;
> > port[1].request = NULL;
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> > b/drivers/gpu/drm/i915/intel_lrc.c
> > index eec1e71..24c69b5 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -295,21 +295,6 @@ uint64_t intel_lr_context_descriptor(struct
> > i915_gem_context *ctx,
> > return ctx->engine[engine->id].lrc_desc;  }
> >
> > -static inline void
> > -execlists_context_status_change(struct drm_i915_gem_request *rq,
> > -   unsigned long status)
> > -{
> > -   /*
> > -* Only used when GVT-g is enabled now. When GVT-g is disabled,
> > -* The compiler should eliminate this function as dead-code.
> > -*/
> > -   if (!IS_ENABLED(CONFIG_DRM_I915_GVT))
> > -   return;
> > -
> > -   atomic_notifier_call_chain(>engine->context_status_notifier,
> > -  status, rq);
> > -}
> > -
> >  static void
> >  execlists_update_context_pdps(struct i915_hw_ppgtt *ppgtt, u32
> > *reg_state)  { @@ -350,7 +335,7 @@ static void
> > execlists_submit_ports(struct intel_engine_cs *engine)
> >
> > GEM_BUG_ON(port[0].count > 1);
> > if (!port[0].count)
> > -   execlists_context_status_change(port[0].request,
> > +   context_status_change(port[0].request,
> >
> > INTEL_CONTEXT_SCHEDULE_IN);
> > desc[0] = execlists_update_context(port[0].request);
> > GEM_DEBUG_EXEC(port[0].context_id = upper_32_bits(desc[0]));
> @@
> > -358,7 +343,7 @@ static void execlists_submit_ports(struct
> > intel_engine_cs *engine)
> >
> > if (port[1].request) {
> > GEM_BUG_ON(port[1].count);
> > -   execlists_context_status_change(port[1].request,
> > +   context_status_change(port[1].request,
> >
> > INTEL_CONTEXT_SCHEDULE_IN);
> > desc[1] = execlists_update_context(port[1].request);
> > GEM_DEBUG_EXEC(port[1].context_id =
> upper_32_bits(desc[1])); @@
> > -581,7 +566,7 @@ static void intel_lrc_irq_handler(unsigned long data)
> > if (--port[0].count == 0) {
> > GEM_BUG_ON(status &
> > GEN8_CTX_STATUS_PREEMPTED);
> >
> > GEM_BUG_ON(!i915_gem_request_completed(port[0].request));
> > -
> > execlists_context_status_change(port[0].request,
> > +

Re: [Intel-gfx] [PATCH v3] drm/i915/scheduler: add gvt notification for guc submission

2017-03-22 Thread Dong, Chuanxiao 
Ping for review. GVT relies on the notification before a request submitted and 
after a request completed, just like when using execlist mode submission.

> -Original Message-
> From: Dong, Chuanxiao
> Sent: Wednesday, March 22, 2017 2:35 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: intel-gvt-...@lists.freedesktop.org; Dong, Chuanxiao
> ; Zheng, Xiao ; Tian,
> Kevin 
> Subject: [PATCH v3] drm/i915/scheduler: add gvt notification for guc
> submission
> 
> GVT request needs a manual mmio load/restore. Before GuC submit a
> request, send notification to gvt for mmio loading. And after the GuC
> finished this GVT request, notify gvt again for mmio restore. This follows the
> usage when using execlists submission.
> 
> v2: use context_status_change instead of execlists_context_status_change
> for better understanding (ZhengXiao)
> v3: remove the comment as it is obvious and not friendly to
> the caller (Kevin)
> 
> Cc: xiao.zh...@intel.com
> Cc: kevin.t...@intel.com
> Signed-off-by: Chuanxiao Dong 
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c |  3 +++
>  drivers/gpu/drm/i915/intel_lrc.c   | 21 +++--
>  drivers/gpu/drm/i915/intel_lrc.h   | 13 +
>  3 files changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
> b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 055467a..0195547 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -520,6 +520,8 @@ static void __i915_guc_submit(struct
> drm_i915_gem_request *rq)
>   unsigned long flags;
>   int b_ret;
> 
> + context_status_change(rq, INTEL_CONTEXT_SCHEDULE_IN);
> +
>   /* WA to flush out the pending GMADR writes to ring buffer. */
>   if (i915_vma_is_map_and_fenceable(rq->ring->vma))
>   POSTING_READ_FW(GUC_STATUS);
> @@ -634,6 +636,7 @@ static void i915_guc_irq_handler(unsigned long data)
>   rq = port[0].request;
>   while (rq && i915_gem_request_completed(rq)) {
>   trace_i915_gem_request_out(rq);
> + context_status_change(rq,
> INTEL_CONTEXT_SCHEDULE_OUT);
>   i915_gem_request_put(rq);
>   port[0].request = port[1].request;
>   port[1].request = NULL;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> b/drivers/gpu/drm/i915/intel_lrc.c
> index eec1e71..24c69b5 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -295,21 +295,6 @@ uint64_t intel_lr_context_descriptor(struct
> i915_gem_context *ctx,
>   return ctx->engine[engine->id].lrc_desc;  }
> 
> -static inline void
> -execlists_context_status_change(struct drm_i915_gem_request *rq,
> - unsigned long status)
> -{
> - /*
> -  * Only used when GVT-g is enabled now. When GVT-g is disabled,
> -  * The compiler should eliminate this function as dead-code.
> -  */
> - if (!IS_ENABLED(CONFIG_DRM_I915_GVT))
> - return;
> -
> - atomic_notifier_call_chain(>engine->context_status_notifier,
> -status, rq);
> -}
> -
>  static void
>  execlists_update_context_pdps(struct i915_hw_ppgtt *ppgtt, u32
> *reg_state)  { @@ -350,7 +335,7 @@ static void
> execlists_submit_ports(struct intel_engine_cs *engine)
> 
>   GEM_BUG_ON(port[0].count > 1);
>   if (!port[0].count)
> - execlists_context_status_change(port[0].request,
> + context_status_change(port[0].request,
> 
>   INTEL_CONTEXT_SCHEDULE_IN);
>   desc[0] = execlists_update_context(port[0].request);
>   GEM_DEBUG_EXEC(port[0].context_id = upper_32_bits(desc[0]));
> @@ -358,7 +343,7 @@ static void execlists_submit_ports(struct
> intel_engine_cs *engine)
> 
>   if (port[1].request) {
>   GEM_BUG_ON(port[1].count);
> - execlists_context_status_change(port[1].request,
> + context_status_change(port[1].request,
> 
>   INTEL_CONTEXT_SCHEDULE_IN);
>   desc[1] = execlists_update_context(port[1].request);
>   GEM_DEBUG_EXEC(port[1].context_id =
> upper_32_bits(desc[1])); @@ -581,7 +566,7 @@ static void
> intel_lrc_irq_handler(unsigned long data)
>   if (--port[0].count == 0) {
>   GEM_BUG_ON(status &
> GEN8_CTX_STATUS_PREEMPTED);
> 
>   GEM_BUG_ON(!i915_gem_request_completed(port[0].request));
> -
>   execlists_context_status_change(port[0].request,
> + context_status_change(port[0].request,
> 
>   INTEL_CONTEXT_SCHEDULE_OUT);
> 
> 
>   trace_i915_gem_request_out(port[0].request);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h
> b/drivers/gpu/drm/i915/intel_lrc.h
> index e8015e7..51e1be9 100644
> ---

[Intel-gfx] [PATCH v3] drm/i915/scheduler: add gvt notification for guc submission

2017-03-22 Thread Chuanxiao Dong 
GVT request needs a manual mmio load/restore. Before GuC submit
a request, send notification to gvt for mmio loading. And after
the GuC finished this GVT request, notify gvt again for mmio
restore. This follows the usage when using execlists submission.

v2: use context_status_change instead of execlists_context_status_change
for better understanding (ZhengXiao)
v3: remove the comment as it is obvious and not friendly to
the caller (Kevin)

Cc: xiao.zh...@intel.com
Cc: kevin.t...@intel.com
Signed-off-by: Chuanxiao Dong 
---
 drivers/gpu/drm/i915/i915_guc_submission.c |  3 +++
 drivers/gpu/drm/i915/intel_lrc.c   | 21 +++--
 drivers/gpu/drm/i915/intel_lrc.h   | 13 +
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 055467a..0195547 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -520,6 +520,8 @@ static void __i915_guc_submit(struct drm_i915_gem_request 
*rq)
unsigned long flags;
int b_ret;
 
+   context_status_change(rq, INTEL_CONTEXT_SCHEDULE_IN);
+
/* WA to flush out the pending GMADR writes to ring buffer. */
if (i915_vma_is_map_and_fenceable(rq->ring->vma))
POSTING_READ_FW(GUC_STATUS);
@@ -634,6 +636,7 @@ static void i915_guc_irq_handler(unsigned long data)
rq = port[0].request;
while (rq && i915_gem_request_completed(rq)) {
trace_i915_gem_request_out(rq);
+   context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
i915_gem_request_put(rq);
port[0].request = port[1].request;
port[1].request = NULL;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index eec1e71..24c69b5 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -295,21 +295,6 @@ uint64_t intel_lr_context_descriptor(struct 
i915_gem_context *ctx,
return ctx->engine[engine->id].lrc_desc;
 }
 
-static inline void
-execlists_context_status_change(struct drm_i915_gem_request *rq,
-   unsigned long status)
-{
-   /*
-* Only used when GVT-g is enabled now. When GVT-g is disabled,
-* The compiler should eliminate this function as dead-code.
-*/
-   if (!IS_ENABLED(CONFIG_DRM_I915_GVT))
-   return;
-
-   atomic_notifier_call_chain(>engine->context_status_notifier,
-  status, rq);
-}
-
 static void
 execlists_update_context_pdps(struct i915_hw_ppgtt *ppgtt, u32 *reg_state)
 {
@@ -350,7 +335,7 @@ static void execlists_submit_ports(struct intel_engine_cs 
*engine)
 
GEM_BUG_ON(port[0].count > 1);
if (!port[0].count)
-   execlists_context_status_change(port[0].request,
+   context_status_change(port[0].request,
INTEL_CONTEXT_SCHEDULE_IN);
desc[0] = execlists_update_context(port[0].request);
GEM_DEBUG_EXEC(port[0].context_id = upper_32_bits(desc[0]));
@@ -358,7 +343,7 @@ static void execlists_submit_ports(struct intel_engine_cs 
*engine)
 
if (port[1].request) {
GEM_BUG_ON(port[1].count);
-   execlists_context_status_change(port[1].request,
+   context_status_change(port[1].request,
INTEL_CONTEXT_SCHEDULE_IN);
desc[1] = execlists_update_context(port[1].request);
GEM_DEBUG_EXEC(port[1].context_id = upper_32_bits(desc[1]));
@@ -581,7 +566,7 @@ static void intel_lrc_irq_handler(unsigned long data)
if (--port[0].count == 0) {
GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED);

GEM_BUG_ON(!i915_gem_request_completed(port[0].request));
-   execlists_context_status_change(port[0].request,
+   context_status_change(port[0].request,

INTEL_CONTEXT_SCHEDULE_OUT);
 
trace_i915_gem_request_out(port[0].request);
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index e8015e7..51e1be9 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -87,5 +87,18 @@ uint64_t intel_lr_context_descriptor(struct i915_gem_context 
*ctx,
 /* Execlists */
 int intel_sanitize_enable_execlists(struct drm_i915_private *dev_priv,
int enable_execlists);
+static inline void
+context_status_change(struct drm_i915_gem_request *rq, unsigned long status)
+{
+   /*
+* Only used when GVT-g is enabled now. When