| Message ID | 1493715511-12916-1-git-send-email-chuanxiao.dong@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017.05.02 16:58:31 +0800, Chuanxiao Dong wrote: > Currently GVT-g cannot work properly when host GuC submission > is enabled, so disable GVT in this case. > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com> > 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 e1ab643..0e1ff48 100644 > --- a/drivers/gpu/drm/i915/intel_gvt.c > +++ b/drivers/gpu/drm/i915/intel_gvt.c > @@ -84,6 +84,11 @@ int intel_gvt_init(struct drm_i915_private *dev_priv) > goto bail; > } > > + if (i915.enable_guc_submission) { > + DRM_INFO("GPU guest virtualisation [GVT-g] disabled due to enabled GuC submission [i915.enable_guc_submission module parameter]\n"); > + goto bail; > + } > + > /* > * We're not in host or fail to find a MPT module, disable GVT-g > */ > -- Applied, thanks!
+ Daniel On ke, 2017-05-03 at 16:36 +0800, Zhenyu Wang wrote: > On 2017.05.02 16:58:31 +0800, Chuanxiao Dong wrote: > > > > Currently GVT-g cannot work properly when host GuC submission > > is enabled, so disable GVT in this case. > > > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com> > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com> <SNIP> > > @@ -84,6 +84,11 @@ int intel_gvt_init(struct drm_i915_private *dev_priv) > > goto bail; > > } > > > > + if (i915.enable_guc_submission) { > > + DRM_INFO("GPU guest virtualisation [GVT-g] disabled due to enabled GuC submission [i915.enable_guc_submission module parameter]\n"); Guest module parameter is not the correct way of detetecting if host has GuC submission enabled. And even if it was, the message is overly verbose (and it'll be incorrect once i915.enable_guc_submission defaults to something else than zero). > > + goto bail; > > + } > > + > > /* > > * We're not in host or fail to find a MPT module, disable GVT-g > > */ > > -- > > Applied, thanks! The original patch should've included at least some Cc's, or wait being merged through drm-tip as it's not int drm/i915/gvt directory at all (unlike the message states). The patch should be reverted for being incorrect. Regards, Joonas
> -----Original Message----- > From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com] > Sent: Friday, May 5, 2017 4:55 PM > To: Zhenyu Wang <zhenyuw@linux.intel.com>; Dong, Chuanxiao > <chuanxiao.dong@intel.com>; Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915/gvt: disable GVT-g if host GuC > submission is enabled > > + Daniel > > On ke, 2017-05-03 at 16:36 +0800, Zhenyu Wang wrote: > > On 2017.05.02 16:58:31 +0800, Chuanxiao Dong wrote: > > > > > > Currently GVT-g cannot work properly when host GuC submission is > > > enabled, so disable GVT in this case. > > > > > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com> > > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com> > > <SNIP> > > > > @@ -84,6 +84,11 @@ int intel_gvt_init(struct drm_i915_private > *dev_priv) > > > goto bail; > > > } > > > > > > + if (i915.enable_guc_submission) { > > > + DRM_INFO("GPU guest virtualisation [GVT-g] disabled due to > > > +enabled GuC submission [i915.enable_guc_submission module > > > +parameter]\n"); > > Guest module parameter is not the correct way of detetecting if host has > GuC submission enabled. And even if it was, the message is overly verbose > (and it'll be incorrect once i915.enable_guc_submission defaults to > something else than zero). Hi Joonas, How about just to say " DRM_INFO("GPU guest virtualisation [GVT-g] disabled due to enabled GuC submission\n");" Thanks Chuanxiao > > > > + goto bail; > > > + } > > > + > > > /* > > > * We're not in host or fail to find a MPT module, disable GVT-g > > > */ > > > -- > > > > Applied, thanks! > > The original patch should've included at least some Cc's, or wait being > merged through drm-tip as it's not int drm/i915/gvt directory at all (unlike > the message states). > > The patch should be reverted for being incorrect. > > Regards, Joonas > -- > Joonas Lahtinen > Open Source Technology Center > Intel Corporation
On Fri, May 05, 2017 at 11:55:14AM +0300, Joonas Lahtinen wrote: > + Daniel > > On ke, 2017-05-03 at 16:36 +0800, Zhenyu Wang wrote: > > On 2017.05.02 16:58:31 +0800, Chuanxiao Dong wrote: > > > > > > Currently GVT-g cannot work properly when host GuC submission > > > is enabled, so disable GVT in this case. > > > > > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com> > > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com> > > <SNIP> > > > > @@ -84,6 +84,11 @@ int intel_gvt_init(struct drm_i915_private *dev_priv) > > > goto bail; > > > } > > > > > > + if (i915.enable_guc_submission) { > > > + DRM_INFO("GPU guest virtualisation [GVT-g] disabled due to enabled GuC submission [i915.enable_guc_submission module parameter]\n"); > > Guest module parameter is not the correct way of detetecting if host > has GuC submission enabled. And even if it was, the message is overly > verbose (and it'll be incorrect once i915.enable_guc_submission > defaults to something else than zero). It needs to be verbose because it is a message to the user, it should tell them what broke, what that will likely mean to them and if possible how to rectify. Even then we know they won't read the entirety of the message but at least it gives us a starting point for the inevitable bugs. We should always aim for clarity and avoid too much jargon in DRM_INFO+. In this case, you could argue that i915.enable_guc_submission is an unsafe parameter set to off by default and so the combination of gvt + guc is pure user error and they can keep both pieces. Ideally we wouldn't use i915.enable_guc_submission at all, but gvt should be disabled upon enabling guc -- since the combination is currently inoperable. But again, this is just a user error and we can just -EIO the driver load... -Chris
On 2017.05.05 11:55:14 +0300, Joonas Lahtinen wrote: > + Daniel > > On ke, 2017-05-03 at 16:36 +0800, Zhenyu Wang wrote: > > On 2017.05.02 16:58:31 +0800, Chuanxiao Dong wrote: > > > > > > Currently GVT-g cannot work properly when host GuC submission > > > is enabled, so disable GVT in this case. > > > > > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com> > > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com> > > <SNIP> > > > > @@ -84,6 +84,11 @@ int intel_gvt_init(struct drm_i915_private *dev_priv) > > > goto bail; > > > } > > > > > > + if (i915.enable_guc_submission) { > > > + DRM_INFO("GPU guest virtualisation [GVT-g] disabled due to enabled GuC submission [i915.enable_guc_submission module parameter]\n"); > > Guest module parameter is not the correct way of detetecting if host > has GuC submission enabled. And even if it was, the message is overly > verbose (and it'll be incorrect once i915.enable_guc_submission > defaults to something else than zero). > > > > + goto bail; > > > + } > > > + > > > /* > > > * We're not in host or fail to find a MPT module, disable GVT-g > > > */ > > > -- > > > > Applied, thanks! > > The original patch should've included at least some Cc's, or wait being > merged through drm-tip as it's not int drm/i915/gvt directory at all > (unlike the message states). > > The patch should be reverted for being incorrect. > ok, as this is not sent for any gvt pull, will drop this in gvt tree.
> -----Original Message----- > From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > Sent: Friday, May 5, 2017 5:11 PM > To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Zhenyu Wang <zhenyuw@linux.intel.com>; Dong, Chuanxiao > <chuanxiao.dong@intel.com>; Daniel Vetter <daniel.vetter@ffwll.ch>; intel- > gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915/gvt: disable GVT-g if host GuC > submission is enabled > > On Fri, May 05, 2017 at 11:55:14AM +0300, Joonas Lahtinen wrote: > > + Daniel > > > > On ke, 2017-05-03 at 16:36 +0800, Zhenyu Wang wrote: > > > On 2017.05.02 16:58:31 +0800, Chuanxiao Dong wrote: > > > > > > > > Currently GVT-g cannot work properly when host GuC submission is > > > > enabled, so disable GVT in this case. > > > > > > > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com> > > > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com> > > > > <SNIP> > > > > > > @@ -84,6 +84,11 @@ int intel_gvt_init(struct drm_i915_private > *dev_priv) > > > > goto bail; > > > > } > > > > > > > > + if (i915.enable_guc_submission) { > > > > + DRM_INFO("GPU guest virtualisation [GVT-g] disabled due to > > > > +enabled GuC submission [i915.enable_guc_submission module > > > > +parameter]\n"); > > > > Guest module parameter is not the correct way of detetecting if host > > has GuC submission enabled. And even if it was, the message is overly > > verbose (and it'll be incorrect once i915.enable_guc_submission > > defaults to something else than zero). > > It needs to be verbose because it is a message to the user, it should tell them > what broke, what that will likely mean to them and if possible how to rectify. > Even then we know they won't read the entirety of the message but at least > it gives us a starting point for the inevitable bugs. We should always aim for > clarity and avoid too much jargon in DRM_INFO+. > > In this case, you could argue that i915.enable_guc_submission is an unsafe > parameter set to off by default and so the combination of gvt + guc is pure > user error and they can keep both pieces. > > Ideally we wouldn't use i915.enable_guc_submission at all, but gvt should be > disabled upon enabling guc -- since the combination is currently inoperable. > But again, this is just a user error and we can just -EIO the driver load... > -Chris Hi Chris/Joonas, Do you agree to use i915.enable_guc_submission for telling gvt to disable itself? Thanks Chuanxiao > > -- > Chris Wilson, Intel Open Source Technology Centre
On ma, 2017-05-08 at 10:30 +0000, Dong, Chuanxiao wrote: <SNIP> > > > > > + if (i915.enable_guc_submission) { > > > > > + DRM_INFO("GPU guest virtualisation [GVT-g] disabled due to > > > > > +enabled GuC submission [i915.enable_guc_submission module > > > > > +parameter]\n"); <SNIP> Something along the lines: "Graphics virtualization is not yet supported with GuC" would be most informative to user. > > > > > It needs to be verbose because it is a message to the user, it should tell them > > what broke, what that will likely mean to them and if possible how to rectify. > > Even then we know they won't read the entirety of the message but at least > > it gives us a starting point for the inevitable bugs. We should always aim for > > clarity and avoid too much jargon in DRM_INFO+. > > > > In this case, you could argue that i915.enable_guc_submission is an unsafe > > parameter set to off by default and so the combination of gvt + guc is pure > > user error and they can keep both pieces. > > > > Ideally we wouldn't use i915.enable_guc_submission at all, but gvt should be > > disabled upon enabling guc -- since the combination is currently inoperable. > > But again, this is just a user error and we can just -EIO the driver load... I agree that currently when GuC submission is not enabled by default, enabling it could result in -EIO on driver load if GVT is enabled simultaneously. When GuC gets enabled by default, it would be preferable for GVT team to have support ready, or enable_gvt probably needs to become an _unsafe param. Regards, Joonas
diff --git a/drivers/gpu/drm/i915/intel_gvt.c b/drivers/gpu/drm/i915/intel_gvt.c index e1ab643..0e1ff48 100644 --- a/drivers/gpu/drm/i915/intel_gvt.c +++ b/drivers/gpu/drm/i915/intel_gvt.c @@ -84,6 +84,11 @@ int intel_gvt_init(struct drm_i915_private *dev_priv) goto bail; } + if (i915.enable_guc_submission) { + DRM_INFO("GPU guest virtualisation [GVT-g] disabled due to enabled GuC submission [i915.enable_guc_submission module parameter]\n"); + goto bail; + } + /* * We're not in host or fail to find a MPT module, disable GVT-g */
Currently GVT-g cannot work properly when host GuC submission is enabled, so disable GVT in this case. Cc: Zhenyu Wang <zhenyuw@linux.intel.com> Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com> --- drivers/gpu/drm/i915/intel_gvt.c | 5 +++++ 1 file changed, 5 insertions(+)