| Message ID | 20170306051541.GA4861@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Any comments to this patch? Thanks Chuanxiao > -----Original Message----- > From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On > Behalf Of Chuanxiao Dong > Sent: Monday, March 6, 2017 1:16 PM > To: intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org > Subject: [PATCH] drm/i915/gvt: add enable_execlists check before enable gvt > > The GVT-g needs execlists to be enabled otherwise gvt should be disabled. > Add a check for enable_execlists before enabling gvt. > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com> > --- > drivers/gpu/drm/i915/intel_gvt.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_gvt.c > b/drivers/gpu/drm/i915/intel_gvt.c > index d23c0fc..3799cb3 100644 > --- a/drivers/gpu/drm/i915/intel_gvt.c > +++ b/drivers/gpu/drm/i915/intel_gvt.c > @@ -77,6 +77,11 @@ int intel_gvt_init(struct drm_i915_private *dev_priv) > goto bail; > } > > + if (!i915.enable_execlists) { > + DRM_DEBUG_DRIVER("Execlists unsupported, GVT-g is > disabled\n"); > + goto bail; > + } > + > /* > * We're not in host or fail to find a MPT module, disable GVT-g > */ > -- > 2.7.4 > > _______________________________________________ > intel-gvt-dev mailing list > intel-gvt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
On Thu, Mar 09, 2017 at 12:32:18PM +0000, Dong, Chuanxiao wrote: > Hi, > > Any comments to this patch? > > Thanks > Chuanxiao > > > -----Original Message----- > > From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On > > Behalf Of Chuanxiao Dong > > Sent: Monday, March 6, 2017 1:16 PM > > To: intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org > > Subject: [PATCH] drm/i915/gvt: add enable_execlists check before enable gvt > > > > The GVT-g needs execlists to be enabled otherwise gvt should be disabled. > > Add a check for enable_execlists before enabling gvt. > > > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com> > > --- > > drivers/gpu/drm/i915/intel_gvt.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_gvt.c > > b/drivers/gpu/drm/i915/intel_gvt.c > > index d23c0fc..3799cb3 100644 > > --- a/drivers/gpu/drm/i915/intel_gvt.c > > +++ b/drivers/gpu/drm/i915/intel_gvt.c > > @@ -77,6 +77,11 @@ int intel_gvt_init(struct drm_i915_private *dev_priv) > > goto bail; > > } > > > > + if (!i915.enable_execlists) { > > + DRM_DEBUG_DRIVER("Execlists unsupported, GVT-g is > > disabled\n"); Should be DRM_INFO() in response to the user action, and should be treated as a user visible string. DRM_INFO("GPU guest virtualisation [GVT-g] disabled due to disabled execlist submission [i915.enable_execlists module parameter]\n"); -Chris
> -----Original Message----- > From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On > Behalf Of Chris Wilson > Sent: Thursday, March 9, 2017 8:45 PM > To: Dong, Chuanxiao <chuanxiao.dong@intel.com> > Cc: intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915/gvt: add enable_execlists check > before enable gvt > > On Thu, Mar 09, 2017 at 12:32:18PM +0000, Dong, Chuanxiao wrote: > > Hi, > > > > Any comments to this patch? > > > > Thanks > > Chuanxiao > > > > > -----Original Message----- > > > From: intel-gvt-dev > > > [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On Behalf Of > > > Chuanxiao Dong > > > Sent: Monday, March 6, 2017 1:16 PM > > > To: intel-gfx@lists.freedesktop.org; > > > intel-gvt-dev@lists.freedesktop.org > > > Subject: [PATCH] drm/i915/gvt: add enable_execlists check before > > > enable gvt > > > > > > The GVT-g needs execlists to be enabled otherwise gvt should be disabled. > > > Add a check for enable_execlists before enabling gvt. > > > > > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_gvt.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_gvt.c > > > b/drivers/gpu/drm/i915/intel_gvt.c > > > index d23c0fc..3799cb3 100644 > > > --- a/drivers/gpu/drm/i915/intel_gvt.c > > > +++ b/drivers/gpu/drm/i915/intel_gvt.c > > > @@ -77,6 +77,11 @@ int intel_gvt_init(struct drm_i915_private > *dev_priv) > > > goto bail; > > > } > > > > > > + if (!i915.enable_execlists) { > > > + DRM_DEBUG_DRIVER("Execlists unsupported, GVT-g is > > > disabled\n"); > > Should be DRM_INFO() in response to the user action, and should be treated > as a user visible string. > > DRM_INFO("GPU guest virtualisation [GVT-g] disabled due to disabled > execlist submission [i915.enable_execlists module parameter]\n"); -Chris Thanks Chris for the comments. Use "DRM_DEBUG_DRIVER" is just following the same print level with the other fail case in intel_gvt_init(). Anyway, I will take your suggestion to use DRM_INFO instead. The message printed is longer than 80 characters, and checkpatch tool also complain with " quoted string split across lines" by splitting to multiple lines. So should I use the message print like below? DRM_INFO("GPU guest virtualisation [GVT-g] disabled due to "); DRM_INFO("disabled execlist submission "); DRM_INFO("[i915.enable_execlists module parameter]\n"); Or any better idea? Thanks Chuanxiao > > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > intel-gvt-dev mailing list > intel-gvt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
On Thu, Mar 09, 2017 at 01:03:17PM +0000, Dong, Chuanxiao wrote: > > > > -----Original Message----- > > From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On > > Behalf Of Chris Wilson > > Sent: Thursday, March 9, 2017 8:45 PM > > To: Dong, Chuanxiao <chuanxiao.dong@intel.com> > > Cc: intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/gvt: add enable_execlists check > > before enable gvt > > > > On Thu, Mar 09, 2017 at 12:32:18PM +0000, Dong, Chuanxiao wrote: > > > Hi, > > > > > > Any comments to this patch? > > > > > > Thanks > > > Chuanxiao > > > > > > > -----Original Message----- > > > > From: intel-gvt-dev > > > > [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On Behalf Of > > > > Chuanxiao Dong > > > > Sent: Monday, March 6, 2017 1:16 PM > > > > To: intel-gfx@lists.freedesktop.org; > > > > intel-gvt-dev@lists.freedesktop.org > > > > Subject: [PATCH] drm/i915/gvt: add enable_execlists check before > > > > enable gvt > > > > > > > > The GVT-g needs execlists to be enabled otherwise gvt should be disabled. > > > > Add a check for enable_execlists before enabling gvt. > > > > > > > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_gvt.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_gvt.c > > > > b/drivers/gpu/drm/i915/intel_gvt.c > > > > index d23c0fc..3799cb3 100644 > > > > --- a/drivers/gpu/drm/i915/intel_gvt.c > > > > +++ b/drivers/gpu/drm/i915/intel_gvt.c > > > > @@ -77,6 +77,11 @@ int intel_gvt_init(struct drm_i915_private > > *dev_priv) > > > > goto bail; > > > > } > > > > > > > > + if (!i915.enable_execlists) { > > > > + DRM_DEBUG_DRIVER("Execlists unsupported, GVT-g is > > > > disabled\n"); > > > > Should be DRM_INFO() in response to the user action, and should be treated > > as a user visible string. > > > > DRM_INFO("GPU guest virtualisation [GVT-g] disabled due to disabled > > execlist submission [i915.enable_execlists module parameter]\n"); -Chris > > Thanks Chris for the comments. Use "DRM_DEBUG_DRIVER" is just following the same print level with the other fail case in intel_gvt_init(). Anyway, I will take your suggestion to use DRM_INFO instead. Just a challenge to identify something that may be in response to user action, typically module parameters or quirks, that may either have intended or unintended side-effects. > The message printed is longer than 80 characters, and checkpatch tool also complain with " quoted string split across lines" by splitting to multiple lines. So should I use the message print like below? > > DRM_INFO("GPU guest virtualisation [GVT-g] disabled due to "); > DRM_INFO("disabled execlist submission "); > DRM_INFO("[i915.enable_execlists module parameter]\n"); > > Or any better idea? Just ignore checkpatch. Either split the line or not, that's up to you :) -Chris
On Thu, 09 Mar 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Thu, Mar 09, 2017 at 01:03:17PM +0000, Dong, Chuanxiao wrote: >> The message printed is longer than 80 characters, and checkpatch tool also complain with " quoted string split across lines" by splitting to multiple lines. So should I use the message print like below? >> >> DRM_INFO("GPU guest virtualisation [GVT-g] disabled due to "); >> DRM_INFO("disabled execlist submission "); >> DRM_INFO("[i915.enable_execlists module parameter]\n"); >> >> Or any better idea? > > Just ignore checkpatch. Either split the line or not, that's up to you :) Btw ignoring checkpatch is not general advise. Please do always look at checkpatch results, it does give helpful feedback on silly mistakes. But just don't regard it as the final authority on kernel style, use your own discretion. In this case, checkpatch is just silly. BR, Jani.
> -----Original Message----- > From: Jani Nikula [mailto:jani.nikula@linux.intel.com] > Sent: Thursday, March 9, 2017 9:48 PM > To: Chris Wilson; Dong, Chuanxiao > Cc: intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915/gvt: add enable_execlists check > before enable gvt > > On Thu, 09 Mar 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Thu, Mar 09, 2017 at 01:03:17PM +0000, Dong, Chuanxiao wrote: > >> The message printed is longer than 80 characters, and checkpatch tool > also complain with " quoted string split across lines" by splitting to multiple > lines. So should I use the message print like below? > >> > >> DRM_INFO("GPU guest virtualisation [GVT-g] disabled due to "); > >> DRM_INFO("disabled execlist submission "); > >> DRM_INFO("[i915.enable_execlists module parameter]\n"); > >> > >> Or any better idea? > > > > Just ignore checkpatch. Either split the line or not, that's up to you > > :) > > Btw ignoring checkpatch is not general advise. Please do always look at > checkpatch results, it does give helpful feedback on silly mistakes. But just > don't regard it as the final authority on kernel style, use your own discretion. > In this case, checkpatch is just silly. I see. Thanks Chris and Jani for the guide. :) Will send out the v2 with the suggested message. Thanks Chuanxiao > > > BR, > Jani. > > > -- > Jani Nikula, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/i915/intel_gvt.c b/drivers/gpu/drm/i915/intel_gvt.c index d23c0fc..3799cb3 100644 --- a/drivers/gpu/drm/i915/intel_gvt.c +++ b/drivers/gpu/drm/i915/intel_gvt.c @@ -77,6 +77,11 @@ int intel_gvt_init(struct drm_i915_private *dev_priv) goto bail; } + if (!i915.enable_execlists) { + DRM_DEBUG_DRIVER("Execlists unsupported, GVT-g is disabled\n"); + goto bail; + } + /* * We're not in host or fail to find a MPT module, disable GVT-g */
The GVT-g needs execlists to be enabled otherwise gvt should be disabled. Add a check for enable_execlists before enabling gvt. Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com> --- drivers/gpu/drm/i915/intel_gvt.c | 5 +++++ 1 file changed, 5 insertions(+)