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

2017-03-27 Thread Dong, Chuanxiao


> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Chris Wilson
> Sent: Monday, March 27, 2017 10:33 PM
> To: Dong, Chuanxiao
> Cc: Zheng, Xiao; intel-gfx@lists.freedesktop.org;
> joonas.lahti...@linux.intel.com; intel-gvt-...@lists.freedesktop.org; Tian,
> Kevin
> Subject: Re: [PATCH v5] drm/i915/scheduler: add gvt notification for guc
> submission
> 
> On Mon, Mar 27, 2017 at 02:22:10PM +, Dong, Chuanxiao wrote:
> > > -Original Message-
> > > From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
> > > Sent: Monday, March 27, 2017 9:50 PM
> > > To: Dong, Chuanxiao
> > > Cc: intel-gfx@lists.freedesktop.org;
> > > intel-gvt-...@lists.freedesktop.org;
> > > Zheng, Xiao; Tian, Kevin; joonas.lahti...@linux.intel.com
> > > Subject: Re: [PATCH v5] drm/i915/scheduler: add gvt notification for
> > > guc submission
> > >
> > > On Mon, Mar 27, 2017 at 09:32:20PM +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)
> > > > v4: fix indent issues (Joonas)
> > > > rename the context_status_change to
> > > > intel_gvt_notify_context_status (Chris)
> > > > v5: move intel_gvt_notify_context_status to intel_gvt.h (Joonas)
> > > >
> > > > Cc: xiao.zh...@intel.com
> > > > Cc: kevin.t...@intel.com
> > > > Cc: joonas.lahti...@linux.intel.com
> > > > Cc: ch...@chris-wilson.co.uk
> > > > Signed-off-by: Chuanxiao Dong 
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_guc_submission.c |  4 
> > > >  drivers/gpu/drm/i915/intel_gvt.h   | 13 +
> > > >  drivers/gpu/drm/i915/intel_lrc.c   | 21 +++--
> > > >  3 files changed, 20 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > index 991e76e..1223169 100644
> > > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > @@ -606,6 +606,8 @@ static void __i915_guc_submit(struct
> > > drm_i915_gem_request *rq)
> > > > unsigned long flags;
> > > > int b_ret;
> > > >
> > > > +   intel_gvt_notify_context_status(rq, INTEL_CONTEXT_SCHEDULE_IN);
> > >
> > > So this gets called for every request, rather than at the context
> > > switch boundaries, and we only once signal the SCHEDULE_OUT. Does
> that matter?
> > Hi Chris, each request submitted by Guc should have a SCHEDULE_OUT in
> i915_guc_irq_handler to match with this SCHEDULE_IN. Any possible reason
> for this OUT/IN not mached?
> >
> > >
> > > Hmm, shouldn't happen in execlists due to force-single-submission.
> > GuC should also use force-single-submission for GVT request as execlists
> does. I have another patch to add this. Do you think if I should combine that
> patch with this one?
> 
> Currently, each request is emitting SCHEDULE_IN, but then being
> amalgamated if in the same context as the earlier. Then we only see
> SCHEDULE_OUT from the final request in the context batch. It sounds like
> you need to apply the force-single-submission patch first, and then the
> context status notifier. Please send them in a single series, in the order 
> they
> need to be applied.

I see. Thanks Chris. I will drop this one and send out a new serial patches.

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 v5] drm/i915/scheduler: add gvt notification for guc submission

2017-03-27 Thread Chris Wilson
On Mon, Mar 27, 2017 at 02:22:10PM +, Dong, Chuanxiao wrote:
> > -Original Message-
> > From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
> > Sent: Monday, March 27, 2017 9:50 PM
> > To: Dong, Chuanxiao
> > Cc: intel-gfx@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org;
> > Zheng, Xiao; Tian, Kevin; joonas.lahti...@linux.intel.com
> > Subject: Re: [PATCH v5] drm/i915/scheduler: add gvt notification for guc
> > submission
> > 
> > On Mon, Mar 27, 2017 at 09:32:20PM +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)
> > > v4: fix indent issues (Joonas)
> > > rename the context_status_change to
> > > intel_gvt_notify_context_status (Chris)
> > > v5: move intel_gvt_notify_context_status to intel_gvt.h (Joonas)
> > >
> > > Cc: xiao.zh...@intel.com
> > > Cc: kevin.t...@intel.com
> > > Cc: joonas.lahti...@linux.intel.com
> > > Cc: ch...@chris-wilson.co.uk
> > > Signed-off-by: Chuanxiao Dong 
> > > ---
> > >  drivers/gpu/drm/i915/i915_guc_submission.c |  4 
> > >  drivers/gpu/drm/i915/intel_gvt.h   | 13 +
> > >  drivers/gpu/drm/i915/intel_lrc.c   | 21 +++--
> > >  3 files changed, 20 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
> > > b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > index 991e76e..1223169 100644
> > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > @@ -606,6 +606,8 @@ static void __i915_guc_submit(struct
> > drm_i915_gem_request *rq)
> > >   unsigned long flags;
> > >   int b_ret;
> > >
> > > + intel_gvt_notify_context_status(rq, INTEL_CONTEXT_SCHEDULE_IN);
> > 
> > So this gets called for every request, rather than at the context switch
> > boundaries, and we only once signal the SCHEDULE_OUT. Does that matter?
> Hi Chris, each request submitted by Guc should have a SCHEDULE_OUT in 
> i915_guc_irq_handler to match with this SCHEDULE_IN. Any possible reason for 
> this OUT/IN not mached?
> 
> > 
> > Hmm, shouldn't happen in execlists due to force-single-submission.
> GuC should also use force-single-submission for GVT request as execlists 
> does. I have another patch to add this. Do you think if I should combine that 
> patch with this one?

Currently, each request is emitting SCHEDULE_IN, but then being
amalgamated if in the same context as the earlier. Then we only see
SCHEDULE_OUT from the final request in the context batch. It sounds like
you need to apply the force-single-submission patch first, and then the
context status notifier. Please send them in a single series, in the
order they need to be applied.
-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 v5] drm/i915/scheduler: add gvt notification for guc submission

2017-03-27 Thread Dong, Chuanxiao


> -Original Message-
> From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
> Sent: Monday, March 27, 2017 9:50 PM
> To: Dong, Chuanxiao
> Cc: intel-gfx@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org;
> Zheng, Xiao; Tian, Kevin; joonas.lahti...@linux.intel.com
> Subject: Re: [PATCH v5] drm/i915/scheduler: add gvt notification for guc
> submission
> 
> On Mon, Mar 27, 2017 at 09:32:20PM +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)
> > v4: fix indent issues (Joonas)
> > rename the context_status_change to
> > intel_gvt_notify_context_status (Chris)
> > v5: move intel_gvt_notify_context_status to intel_gvt.h (Joonas)
> >
> > Cc: xiao.zh...@intel.com
> > Cc: kevin.t...@intel.com
> > Cc: joonas.lahti...@linux.intel.com
> > Cc: ch...@chris-wilson.co.uk
> > Signed-off-by: Chuanxiao Dong 
> > ---
> >  drivers/gpu/drm/i915/i915_guc_submission.c |  4 
> >  drivers/gpu/drm/i915/intel_gvt.h   | 13 +
> >  drivers/gpu/drm/i915/intel_lrc.c   | 21 +++--
> >  3 files changed, 20 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
> > b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index 991e76e..1223169 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -606,6 +606,8 @@ static void __i915_guc_submit(struct
> drm_i915_gem_request *rq)
> > unsigned long flags;
> > int b_ret;
> >
> > +   intel_gvt_notify_context_status(rq, INTEL_CONTEXT_SCHEDULE_IN);
> 
> So this gets called for every request, rather than at the context switch
> boundaries, and we only once signal the SCHEDULE_OUT. Does that matter?
Hi Chris, each request submitted by Guc should have a SCHEDULE_OUT in 
i915_guc_irq_handler to match with this SCHEDULE_IN. Any possible reason for 
this OUT/IN not mached?

> 
> Hmm, shouldn't happen in execlists due to force-single-submission.
GuC should also use force-single-submission for GVT request as execlists does. 
I have another patch to add this. Do you think if I should combine that patch 
with this one?

> 
> Does it matter to gvt that we repeat the SCHEDULE_IN after reset (happens
> for execlists as well)?
This should be fine.

Thanks
Chuanxiao

> -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 v5] drm/i915/scheduler: add gvt notification for guc submission

2017-03-27 Thread Chris Wilson
On Mon, Mar 27, 2017 at 09:32:20PM +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)
> v4: fix indent issues (Joonas)
> rename the context_status_change to intel_gvt_notify_context_status 
> (Chris)
> v5: move intel_gvt_notify_context_status to intel_gvt.h (Joonas)
> 
> Cc: xiao.zh...@intel.com
> Cc: kevin.t...@intel.com
> Cc: joonas.lahti...@linux.intel.com
> Cc: ch...@chris-wilson.co.uk
> Signed-off-by: Chuanxiao Dong 
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c |  4 
>  drivers/gpu/drm/i915/intel_gvt.h   | 13 +
>  drivers/gpu/drm/i915/intel_lrc.c   | 21 +++--
>  3 files changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
> b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 991e76e..1223169 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -606,6 +606,8 @@ static void __i915_guc_submit(struct drm_i915_gem_request 
> *rq)
>   unsigned long flags;
>   int b_ret;
>  
> + intel_gvt_notify_context_status(rq, INTEL_CONTEXT_SCHEDULE_IN);

So this gets called for every request, rather than at the context switch
boundaries, and we only once signal the SCHEDULE_OUT. Does that matter?

Hmm, shouldn't happen in execlists due to force-single-submission.

Does it matter to gvt that we repeat the SCHEDULE_IN after reset
(happens for execlists as well)?
-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


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

2017-03-27 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)
v4: fix indent issues (Joonas)
rename the context_status_change to intel_gvt_notify_context_status (Chris)
v5: move intel_gvt_notify_context_status to intel_gvt.h (Joonas)

Cc: xiao.zh...@intel.com
Cc: kevin.t...@intel.com
Cc: joonas.lahti...@linux.intel.com
Cc: ch...@chris-wilson.co.uk
Signed-off-by: Chuanxiao Dong 
---
 drivers/gpu/drm/i915/i915_guc_submission.c |  4 
 drivers/gpu/drm/i915/intel_gvt.h   | 13 +
 drivers/gpu/drm/i915/intel_lrc.c   | 21 +++--
 3 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 991e76e..1223169 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -606,6 +606,8 @@ static void __i915_guc_submit(struct drm_i915_gem_request 
*rq)
unsigned long flags;
int b_ret;
 
+   intel_gvt_notify_context_status(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);
@@ -720,6 +722,8 @@ 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);
+   intel_gvt_notify_context_status(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_gvt.h b/drivers/gpu/drm/i915/intel_gvt.h
index 25df2d6..9175018 100644
--- a/drivers/gpu/drm/i915/intel_gvt.h
+++ b/drivers/gpu/drm/i915/intel_gvt.h
@@ -32,6 +32,14 @@ void intel_gvt_cleanup(struct drm_i915_private *dev_priv);
 int intel_gvt_init_device(struct drm_i915_private *dev_priv);
 void intel_gvt_clean_device(struct drm_i915_private *dev_priv);
 int intel_gvt_init_host(void);
+
+static inline void
+intel_gvt_notify_context_status(struct drm_i915_gem_request *rq,
+   unsigned long status)
+{
+   atomic_notifier_call_chain(>engine->context_status_notifier,
+  status, rq);
+}
 #else
 static inline int intel_gvt_init(struct drm_i915_private *dev_priv)
 {
@@ -40,6 +48,11 @@ static inline int intel_gvt_init(struct drm_i915_private 
*dev_priv)
 static inline void intel_gvt_cleanup(struct drm_i915_private *dev_priv)
 {
 }
+static inline void
+intel_gvt_notify_context_status(struct drm_i915_gem_request *rq,
+   unsigned long status)
+{
+}
 #endif
 
 #endif /* _INTEL_GVT_H_ */
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index dd0e9d587..8708515 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,
+   intel_gvt_notify_context_status(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,
+