| Message ID | 20180712185315.3288-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Chris Wilson <chris@chris-wilson.co.uk> > Sent: Thursday, July 12, 2018 11:53 AM > To: intel-gfx@lists.freedesktop.org > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Zhenyu Wang > <zhenyuw@linux.intel.com>; Bloomfield, Jon <jon.bloomfield@intel.com>; > Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld > <matthew.william.auld@gmail.com> > Subject: [PATCH 3/6] drm/i915/gtt: Disable read-only support under GVT > > GVT is not propagating the PTE bits, and is always setting the > read-write bit, thus breaking read-only support. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Zhenyu Wang <zhenyuw@linux.intel.com> > Cc: Jon Bloomfield <jon.bloomfield@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Matthew Auld <matthew.william.auld@gmail.com> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 6c0b438afe46..8e70a45b8a90 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -1662,8 +1662,12 @@ static struct i915_hw_ppgtt > *gen8_ppgtt_create(struct drm_i915_private *i915) > 1ULL << 48 : > 1ULL << 32; > > - /* From bdw, there is support for read-only pages in the PPGTT */ > - ppgtt->vm.has_read_only = true; > + /* > + * From bdw, there is support for read-only pages in the PPGTT. > + * > + * XXX GVT is not setting honouring the PTE bits. > + */ > + ppgtt->vm.has_read_only = !intel_vgpu_active(i915); > > i915_address_space_init(&ppgtt->vm, i915); > > -- > 2.18.0 Is there a blocker that prevents gvt respecting this bit? I can't think of an obvious reason why it would be a bad thing to support.
On 2018.07.12 20:36:03 +0000, Bloomfield, Jon wrote: > > -----Original Message----- > > From: Chris Wilson <chris@chris-wilson.co.uk> > > Sent: Thursday, July 12, 2018 11:53 AM > > To: intel-gfx@lists.freedesktop.org > > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Zhenyu Wang > > <zhenyuw@linux.intel.com>; Bloomfield, Jon <jon.bloomfield@intel.com>; > > Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld > > <matthew.william.auld@gmail.com> > > Subject: [PATCH 3/6] drm/i915/gtt: Disable read-only support under GVT > > > > GVT is not propagating the PTE bits, and is always setting the > > read-write bit, thus breaking read-only support. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com> > > Cc: Jon Bloomfield <jon.bloomfield@intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Matthew Auld <matthew.william.auld@gmail.com> > > --- > > drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > > b/drivers/gpu/drm/i915/i915_gem_gtt.c > > index 6c0b438afe46..8e70a45b8a90 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > @@ -1662,8 +1662,12 @@ static struct i915_hw_ppgtt > > *gen8_ppgtt_create(struct drm_i915_private *i915) > > 1ULL << 48 : > > 1ULL << 32; > > > > - /* From bdw, there is support for read-only pages in the PPGTT */ > > - ppgtt->vm.has_read_only = true; > > + /* > > + * From bdw, there is support for read-only pages in the PPGTT. > > + * > > + * XXX GVT is not setting honouring the PTE bits. > > + */ > > + ppgtt->vm.has_read_only = !intel_vgpu_active(i915); > > > > i915_address_space_init(&ppgtt->vm, i915); > > > > -- > > 2.18.0 > > Is there a blocker that prevents gvt respecting this bit? I can't think of an obvious > reason why it would be a bad thing to support. I don't think any blocker for gvt support, we can respect that bit when shadowing. But we need capability check on host gvt when that support is ready.
Quoting Zhenyu Wang (2018-07-13 03:03:10) > On 2018.07.12 20:36:03 +0000, Bloomfield, Jon wrote: > > > -----Original Message----- > > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > Sent: Thursday, July 12, 2018 11:53 AM > > > To: intel-gfx@lists.freedesktop.org > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Zhenyu Wang > > > <zhenyuw@linux.intel.com>; Bloomfield, Jon <jon.bloomfield@intel.com>; > > > Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld > > > <matthew.william.auld@gmail.com> > > > Subject: [PATCH 3/6] drm/i915/gtt: Disable read-only support under GVT > > > > > > GVT is not propagating the PTE bits, and is always setting the > > > read-write bit, thus breaking read-only support. > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com> > > > Cc: Jon Bloomfield <jon.bloomfield@intel.com> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > Cc: Matthew Auld <matthew.william.auld@gmail.com> > > > --- > > > drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > index 6c0b438afe46..8e70a45b8a90 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > @@ -1662,8 +1662,12 @@ static struct i915_hw_ppgtt > > > *gen8_ppgtt_create(struct drm_i915_private *i915) > > > 1ULL << 48 : > > > 1ULL << 32; > > > > > > - /* From bdw, there is support for read-only pages in the PPGTT */ > > > - ppgtt->vm.has_read_only = true; > > > + /* > > > + * From bdw, there is support for read-only pages in the PPGTT. > > > + * > > > + * XXX GVT is not setting honouring the PTE bits. > > > + */ > > > + ppgtt->vm.has_read_only = !intel_vgpu_active(i915); > > > > > > i915_address_space_init(&ppgtt->vm, i915); > > > > > > -- > > > 2.18.0 > > > > Is there a blocker that prevents gvt respecting this bit? I can't think of an obvious > > reason why it would be a bad thing to support. > > I don't think any blocker for gvt support, we can respect that bit when shadowing. > But we need capability check on host gvt when that support is ready. Another cap bit required, so ack on both sides? -Chris
> -----Original Message----- > From: Chris Wilson <chris@chris-wilson.co.uk> > Sent: Friday, July 13, 2018 1:06 AM > To: Bloomfield, Jon <jon.bloomfield@intel.com>; Zhenyu Wang > <zhenyuw@linux.intel.com> > Cc: intel-gfx@lists.freedesktop.org; Zhenyu Wang > <zhenyuw@linux.intel.com>; Joonas Lahtinen > <joonas.lahtinen@linux.intel.com>; Matthew Auld > <matthew.william.auld@gmail.com> > Subject: Re: [PATCH 3/6] drm/i915/gtt: Disable read-only support under GVT > > Quoting Zhenyu Wang (2018-07-13 03:03:10) > > On 2018.07.12 20:36:03 +0000, Bloomfield, Jon wrote: > > > > -----Original Message----- > > > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > > Sent: Thursday, July 12, 2018 11:53 AM > > > > To: intel-gfx@lists.freedesktop.org > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Zhenyu Wang > > > > <zhenyuw@linux.intel.com>; Bloomfield, Jon > <jon.bloomfield@intel.com>; > > > > Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld > > > > <matthew.william.auld@gmail.com> > > > > Subject: [PATCH 3/6] drm/i915/gtt: Disable read-only support under > GVT > > > > > > > > GVT is not propagating the PTE bits, and is always setting the > > > > read-write bit, thus breaking read-only support. > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com> > > > > Cc: Jon Bloomfield <jon.bloomfield@intel.com> > > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > > Cc: Matthew Auld <matthew.william.auld@gmail.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++-- > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > index 6c0b438afe46..8e70a45b8a90 100644 > > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > @@ -1662,8 +1662,12 @@ static struct i915_hw_ppgtt > > > > *gen8_ppgtt_create(struct drm_i915_private *i915) > > > > 1ULL << 48 : > > > > 1ULL << 32; > > > > > > > > - /* From bdw, there is support for read-only pages in the PPGTT */ > > > > - ppgtt->vm.has_read_only = true; > > > > + /* > > > > + * From bdw, there is support for read-only pages in the PPGTT. > > > > + * > > > > + * XXX GVT is not setting honouring the PTE bits. > > > > + */ > > > > + ppgtt->vm.has_read_only = !intel_vgpu_active(i915); > > > > > > > > i915_address_space_init(&ppgtt->vm, i915); > > > > > > > > -- > > > > 2.18.0 > > > > > > Is there a blocker that prevents gvt respecting this bit? I can't think of an > obvious > > > reason why it would be a bad thing to support. > > > > I don't think any blocker for gvt support, we can respect that bit when > shadowing. > > But we need capability check on host gvt when that support is ready. > > Another cap bit required, so ack on both sides? > -Chris I see. Not as permanent disable, just more plumbing needed. I'm happy then :-) Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
Quoting Bloomfield, Jon (2018-07-13 14:51:04) > > -----Original Message----- > > From: Chris Wilson <chris@chris-wilson.co.uk> > > Sent: Friday, July 13, 2018 1:06 AM > > To: Bloomfield, Jon <jon.bloomfield@intel.com>; Zhenyu Wang > > <zhenyuw@linux.intel.com> > > Cc: intel-gfx@lists.freedesktop.org; Zhenyu Wang > > <zhenyuw@linux.intel.com>; Joonas Lahtinen > > <joonas.lahtinen@linux.intel.com>; Matthew Auld > > <matthew.william.auld@gmail.com> > > Subject: Re: [PATCH 3/6] drm/i915/gtt: Disable read-only support under GVT > > > > Quoting Zhenyu Wang (2018-07-13 03:03:10) > > > On 2018.07.12 20:36:03 +0000, Bloomfield, Jon wrote: > > > > > -----Original Message----- > > > > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > > > Sent: Thursday, July 12, 2018 11:53 AM > > > > > To: intel-gfx@lists.freedesktop.org > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Zhenyu Wang > > > > > <zhenyuw@linux.intel.com>; Bloomfield, Jon > > <jon.bloomfield@intel.com>; > > > > > Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld > > > > > <matthew.william.auld@gmail.com> > > > > > Subject: [PATCH 3/6] drm/i915/gtt: Disable read-only support under > > GVT > > > > > > > > > > GVT is not propagating the PTE bits, and is always setting the > > > > > read-write bit, thus breaking read-only support. > > > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com> > > > > > Cc: Jon Bloomfield <jon.bloomfield@intel.com> > > > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > > > Cc: Matthew Auld <matthew.william.auld@gmail.com> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++-- > > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > > b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > > index 6c0b438afe46..8e70a45b8a90 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > > @@ -1662,8 +1662,12 @@ static struct i915_hw_ppgtt > > > > > *gen8_ppgtt_create(struct drm_i915_private *i915) > > > > > 1ULL << 48 : > > > > > 1ULL << 32; > > > > > > > > > > - /* From bdw, there is support for read-only pages in the PPGTT */ > > > > > - ppgtt->vm.has_read_only = true; > > > > > + /* > > > > > + * From bdw, there is support for read-only pages in the PPGTT. > > > > > + * > > > > > + * XXX GVT is not setting honouring the PTE bits. > > > > > + */ > > > > > + ppgtt->vm.has_read_only = !intel_vgpu_active(i915); > > > > > > > > > > i915_address_space_init(&ppgtt->vm, i915); > > > > > > > > > > -- > > > > > 2.18.0 > > > > > > > > Is there a blocker that prevents gvt respecting this bit? I can't think of an > > obvious > > > > reason why it would be a bad thing to support. > > > > > > I don't think any blocker for gvt support, we can respect that bit when > > shadowing. > > > But we need capability check on host gvt when that support is ready. > > > > Another cap bit required, so ack on both sides? > > -Chris > I see. Not as permanent disable, just more plumbing needed. > I'm happy then :-) > Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com> With both a check in igt and selftests happy, let's go! Thanks for your patience, -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 6c0b438afe46..8e70a45b8a90 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1662,8 +1662,12 @@ static struct i915_hw_ppgtt *gen8_ppgtt_create(struct drm_i915_private *i915) 1ULL << 48 : 1ULL << 32; - /* From bdw, there is support for read-only pages in the PPGTT */ - ppgtt->vm.has_read_only = true; + /* + * From bdw, there is support for read-only pages in the PPGTT. + * + * XXX GVT is not setting honouring the PTE bits. + */ + ppgtt->vm.has_read_only = !intel_vgpu_active(i915); i915_address_space_init(&ppgtt->vm, i915);
GVT is not propagating the PTE bits, and is always setting the read-write bit, thus breaking read-only support. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Zhenyu Wang <zhenyuw@linux.intel.com> Cc: Jon Bloomfield <jon.bloomfield@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Matthew Auld <matthew.william.auld@gmail.com> --- drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)