| Message ID | 1494324690-29033-1-git-send-email-chuanxiao.dong@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On ti, 2017-05-09 at 18:11 +0800, Chuanxiao Dong wrote: > Currently GVT-g cannot work properly when host GuC submission > is enabled, so disable GVT in this case. > > v2: update the user message (Joonas) > > 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..d85742c 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 as Graphics virtualization is not yet supported with GuC submission [i915.enable_guc_submission module parameter]\n"); > + goto bail; > + } As discussed earlier, driver loading should fail with -EIO when incompatible options are specified. Also, the message is now even more cryptic, just: DRM_INFO("Graphics virtualization is not yet supported with GuC submission"); Should do. Please Cc: the people who've previously commented on the patch to make it easier to spot new versions. Regards, Joonas
> -----Original Message----- > From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com] > Sent: Wednesday, May 10, 2017 8:48 PM > To: Dong, Chuanxiao <chuanxiao.dong@intel.com>; intel-gvt- > dev@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/gvt: disable GVT-g if host GuC > submission is enabled > > On ti, 2017-05-09 at 18:11 +0800, Chuanxiao Dong wrote: > > Currently GVT-g cannot work properly when host GuC submission is > > enabled, so disable GVT in this case. > > > > v2: update the user message (Joonas) > > > > 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..d85742c 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 as > Graphics virtualization is not yet supported with GuC submission > [i915.enable_guc_submission module parameter]\n"); > > + goto bail; > > + } > > As discussed earlier, driver loading should fail with -EIO when incompatible > options are specified. Sorry for not getting why should fail with -EIO? By looking into the source, Intel_gvt_init is part of i915 driver loading, and fail with -EIO will make i915 driver failed to load. The loading of intel gvt is in intel_gvt_init, and "goto bail" can skip gvt loading which should be the purpose of disabling gvt when incompatible options are specified, so it doesn't rely on the return value of intel_gvt_init. > > Also, the message is now even more cryptic, just: > > DRM_INFO("Graphics virtualization is not yet supported with GuC > submission"); Ah...It is my misunderstanding of your last comment. Will fix this. > > Should do. > > Please Cc: the people who've previously commented on the patch to make it > easier to spot new versions. Sorry for missing this. Thanks Chuanxiao
On to, 2017-05-11 at 02:33 +0000, Dong, Chuanxiao wrote: > > > > > -----Original Message----- > > From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com] > > Sent: Wednesday, May 10, 2017 8:48 PM > > To: Dong, Chuanxiao <chuanxiao.dong@intel.com>; intel-gvt- > > dev@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/gvt: disable GVT-g if host GuC > > submission is enabled > > > > On ti, 2017-05-09 at 18:11 +0800, Chuanxiao Dong wrote: > > > > > > Currently GVT-g cannot work properly when host GuC submission is > > > enabled, so disable GVT in this case. > > > > > > v2: update the user message (Joonas) > > > > > > > > > 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..d85742c 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 as > > Graphics virtualization is not yet supported with GuC submission > > [i915.enable_guc_submission module parameter]\n"); > > > > > > + goto bail; > > > + } > > > > As discussed earlier, driver loading should fail with -EIO when incompatible > > options are specified. > > Sorry for not getting why should fail with -EIO? By looking into the > source, Intel_gvt_init is part of i915 driver loading, and fail with > -EIO will make i915 driver failed to load. Yes, the user has specified an unsafe kernel configuration option, enable_guc_submission and the driver can rightfully stop loading if enable_gvt option was passed in the same command line. Regards, Joonas
> -----Original Message----- > From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com] > Sent: Thursday, May 11, 2017 8:50 PM > To: Dong, Chuanxiao <chuanxiao.dong@intel.com>; intel-gvt- > dev@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/gvt: disable GVT-g if host GuC > submission is enabled > > On to, 2017-05-11 at 02:33 +0000, Dong, Chuanxiao wrote: > > > > > > > > -----Original Message----- > > > From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com] > > > Sent: Wednesday, May 10, 2017 8:48 PM > > > To: Dong, Chuanxiao <chuanxiao.dong@intel.com>; intel-gvt- > > > dev@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > > > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/gvt: disable GVT-g if > > > host GuC submission is enabled > > > > > > On ti, 2017-05-09 at 18:11 +0800, Chuanxiao Dong wrote: > > > > > > > > Currently GVT-g cannot work properly when host GuC submission is > > > > enabled, so disable GVT in this case. > > > > > > > > v2: update the user message (Joonas) > > > > > > > > > > > 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..d85742c 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 as > > > Graphics virtualization is not yet supported with GuC submission > > > [i915.enable_guc_submission module parameter]\n"); > > > > > > > > + goto bail; > > > > + } > > > > > > As discussed earlier, driver loading should fail with -EIO when > > > incompatible options are specified. > > > > Sorry for not getting why should fail with -EIO? By looking into the > > source, Intel_gvt_init is part of i915 driver loading, and fail with > > -EIO will make i915 driver failed to load. > > Yes, the user has specified an unsafe kernel configuration option, > enable_guc_submission and the driver can rightfully stop loading if > enable_gvt option was passed in the same command line. > Thanks Joonas. If to fail with -EIO, how about for the other two checks: " if (!is_supported_device(dev_priv))" and " if (!i915.enable_execlists)"? Currently these two cases are failed with 0 instead of -EIO. Looks like should also fail with -EIO? For the gvt init failure case, it is also failed with 0 right now, which means that even gvt init is failed, i915 loading will be continue instead of failed due to gvt. Right now we also want to fail with -EIO, right? Thanks Chuanxiao
> -----Original Message----- > From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On > Behalf Of Dong, Chuanxiao > Sent: Thursday, May 11, 2017 9:38 PM > To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; intel-gvt- > dev@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > Subject: RE: [Intel-gfx] [PATCH v2] drm/i915/gvt: disable GVT-g if host GuC > submission is enabled > > > > > -----Original Message----- > > From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com] > > Sent: Thursday, May 11, 2017 8:50 PM > > To: Dong, Chuanxiao <chuanxiao.dong@intel.com>; intel-gvt- > > dev@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/gvt: disable GVT-g if > > host GuC submission is enabled > > > > On to, 2017-05-11 at 02:33 +0000, Dong, Chuanxiao wrote: > > > > > > > > > > > -----Original Message----- > > > > From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com] > > > > Sent: Wednesday, May 10, 2017 8:48 PM > > > > To: Dong, Chuanxiao <chuanxiao.dong@intel.com>; intel-gvt- > > > > dev@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > > > > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/gvt: disable GVT-g if > > > > host GuC submission is enabled > > > > > > > > On ti, 2017-05-09 at 18:11 +0800, Chuanxiao Dong wrote: > > > > > > > > > > Currently GVT-g cannot work properly when host GuC submission is > > > > > enabled, so disable GVT in this case. > > > > > > > > > > v2: update the user message (Joonas) > > > > > > > > > > > > > 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..d85742c 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 as > > > > Graphics virtualization is not yet supported with GuC submission > > > > [i915.enable_guc_submission module parameter]\n"); > > > > > > > > > > + goto bail; > > > > > + } > > > > > > > > As discussed earlier, driver loading should fail with -EIO when > > > > incompatible options are specified. > > > > > > Sorry for not getting why should fail with -EIO? By looking into the > > > source, Intel_gvt_init is part of i915 driver loading, and fail with > > > -EIO will make i915 driver failed to load. > > > > Yes, the user has specified an unsafe kernel configuration option, > > enable_guc_submission and the driver can rightfully stop loading if > > enable_gvt option was passed in the same command line. > > > Thanks Joonas. If to fail with -EIO, how about for the other two checks: " if > (!is_supported_device(dev_priv))" and " if (!i915.enable_execlists)"? > Currently these two cases are failed with 0 instead of -EIO. Looks like should > also fail with -EIO? > > For the gvt init failure case, it is also failed with 0 right now, which means > that even gvt init is failed, i915 loading will be continue instead of failed due > to gvt. Right now we also want to fail with -EIO, right? > Hi Joonas, Do you have any suggestion on how to handle the above two cases? Or you only want to fail with -EIO for the guc case, and keep the original for the others? Thanks Chuanxiao
On ti, 2017-05-16 at 07:53 +0000, Dong, Chuanxiao wrote: > > > > Thanks Joonas. If to fail with -EIO, how about for the other two checks: " if > > (!is_supported_device(dev_priv))" and " if (!i915.enable_execlists)"? > > Currently these two cases are failed with 0 instead of -EIO. Looks like should > > also fail with -EIO? !is_supported_device check should probably be done in parameter sanitization phase, it should not result in EIO, because enable_gvt is not marked as an unsafe option. enable_execlists on the other hand is a an _unsafe option and should result in -EIO. Regards, Joonas
> -----Original Message----- > From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com] > Sent: Thursday, May 18, 2017 6:37 PM > To: Dong, Chuanxiao <chuanxiao.dong@intel.com>; intel-gvt- > dev@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/gvt: disable GVT-g if host GuC > submission is enabled > > On ti, 2017-05-16 at 07:53 +0000, Dong, Chuanxiao wrote: > > > > > > Thanks Joonas. If to fail with -EIO, how about for the other two > > > checks: " if (!is_supported_device(dev_priv))" and " if > (!i915.enable_execlists)"? > > > Currently these two cases are failed with 0 instead of -EIO. Looks > > > like should also fail with -EIO? > > !is_supported_device check should probably be done in parameter > sanitization phase, it should not result in EIO, because enable_gvt is not > marked as an unsafe option. > > enable_execlists on the other hand is a an _unsafe option and should result > in -EIO. > Thanks Joonas for the suggestion. Looks like we need two more patches here, one for add a sanitize API for GVT so it can be called by intel_sanitize_options in sanitization phase. Another patch is to return -EIO if !i915.enable_execlists. Will send out these two patches first. Thanks Chuanxiao
diff --git a/drivers/gpu/drm/i915/intel_gvt.c b/drivers/gpu/drm/i915/intel_gvt.c index e1ab643..d85742c 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 as Graphics virtualization is not yet supported with 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. v2: update the user message (Joonas) 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(+)