Re: [PATCH v7 09/11] drm/i915: Introduce execlist context status change notification

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ti, 2016-06-07 at 23:01 +0100, Chris Wilson wrote:
> On Tue, Jun 07, 2016 at 11:18:45AM -0400, 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. And GVT-g
> > has to know the status of the execlist context.
> > 
> > This function is configurable when creating a new GEM context. Currently,
> > Only GVT-g will create the "status-change-notification" enabled GEM
> > context.
> > 
> > v7:
> > 
> > - Remove per-engine ctx status notifiers. Use one status notifier for all
> > engines. (Joonas)
> > - Add prefix "INTEL_" for related definitions. (Joonas)
> > - Refine the comments in execlists_context_status_change(). (Joonas)
> > 
> > v6:
> > 
> > - When !CONFIG_DRM_I915_GVT, make GVT code as dead code then compiler
> > could automatically eliminate them for us. (Chris)
> > - Always initialize the notifier header, so it could be switched on/off
> > at runtime. (Chris)
> > 
> > v5:
> > 
> > - Only compile this feature when CONFIG_DRM_I915_GVT is enabled.(Tvrtko)
> > 
> > Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> > ---
> > +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;
> > +
> > +	if (!rq->ctx->enable_lrc_status_change_notification)
> > +		return;
> Was there any other justification for the boolean? (i.e. does it get
> used elsewhere) I thought we mentioned this as probably premature
> optimisation and should favour speeding up a no-op call_chain() if
> required. So can we have callbacks in the notifier but need to disable
> notification? If so, that is never explained.
> 

By my original comments, after removing this boolean, you can add my;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> > 
> > +
> > +	atomic_notifier_call_chain(&rq->ctx->status_notifier, status, rq);
> > +}
-- 
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