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

[Dong, Chuanxiao]

> -----Original Message-----
> From: Joonas Lahtinen [mailto:joonas.lahtinen at linux.intel.com]
> Sent: Monday, March 27, 2017 6:14 PM
> To: Dong, Chuanxiao <chuanxiao.dong at intel.com>; intel-
> gfx at lists.freedesktop.org
> Cc: intel-gvt-dev at lists.freedesktop.org; Zheng, Xiao
> <xiao.zheng at intel.com>; Tian, Kevin <kevin.tian at intel.com>; chris at chris-
> wilson.co.uk
> Subject: Re: [PATCH v4] drm/i915/scheduler: add gvt notification for guc
> submission
> 
> On pe, 2017-03-24 at 09:49 +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)
> >
> > Cc: xiao.zheng at intel.com
> > Cc: kevin.tian at intel.com
> > Cc: joonas.lahtinen at linux.intel.com
> > Cc: chris at chris-wilson.co.uk
> > Signed-off-by: Chuanxiao Dong <chuanxiao.dong at intel.com>
> 
> <SNIP>
> 
> > @@ -634,6 +636,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);
> 
> Code indent is still broken.
Hi Joonas, I am sorry for not getting this code indent issue. The above code is just split by typing an enter due to longer than 80 characters. Are you expecting to see the code like below? The 2nd line will be longer than 80 characters in below case. If this is fine then I will change this in the next version.
			intel_gvt_notify_context_status(rq,
							INTEL_CONTEXT_SCHEDULE_OUT);

> 
> > @@ -87,5 +87,19 @@ 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
> > +intel_gvt_notify_context_status(struct drm_i915_gem_request *rq,
> > +				unsigned long status)
> 
> With that prefix, this needs to go to intel_gvt.h, where you can take
> advantage of the existing #ifdef block (which should really be #if
> IS_ENABLED() too).

Sure. Will move to intel_gvt.h in the next version.

Thanks
Chuanxiao

> 
> Regards, Joonas
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation