| Message ID | 20230727145504.1919316-6-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Another take on PAT/object cache mode refactoring | expand |
On Thu, Jul 27, 2023 at 03:55:01PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Now that i915 understands the caching modes behind PAT indices, we can > refine the check in vm_fault_gtt() to not reject the uncached PAT if it > was set by userspace on a snoopable platform. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Fei Yang <fei.yang@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > index cd7f8ded0d6f..9aa6ecf68432 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > @@ -382,17 +382,9 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf) > goto err_reset; > } > > - /* > - * For objects created by userspace through GEM_CREATE with pat_index > - * set by set_pat extension, coherency is managed by userspace, make > - * sure we don't fail handling the vm fault by calling > - * i915_gem_object_has_cache_level() which always return true for such > - * objects. Otherwise this helper function would fall back to checking > - * whether the object is un-cached. > - */ > - if (!((obj->pat_set_by_user || > - i915_gem_object_has_cache_mode(obj, I915_CACHE_MODE_UC)) || > - HAS_LLC(i915))) { > + /* Access to snoopable pages through the GTT is incoherent. */ This comment was removed in the previous patch, but now it came back here. Should we have just left it be in the previous patch? I'm not really clear on what it means either. Are we using "GTT" as shorthand to refer to the aperture here? Matt > + if (!i915_gem_object_has_cache_mode(obj, I915_CACHE_MODE_UC) && > + !HAS_LLC(i915)) { > ret = -EFAULT; > goto err_unpin; > } > -- > 2.39.2 >
On 28/07/2023 01:04, Matt Roper wrote: > On Thu, Jul 27, 2023 at 03:55:01PM +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Now that i915 understands the caching modes behind PAT indices, we can >> refine the check in vm_fault_gtt() to not reject the uncached PAT if it >> was set by userspace on a snoopable platform. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Fei Yang <fei.yang@intel.com> >> Cc: Matt Roper <matthew.d.roper@intel.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_mman.c | 14 +++----------- >> 1 file changed, 3 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c >> index cd7f8ded0d6f..9aa6ecf68432 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c >> @@ -382,17 +382,9 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf) >> goto err_reset; >> } >> >> - /* >> - * For objects created by userspace through GEM_CREATE with pat_index >> - * set by set_pat extension, coherency is managed by userspace, make >> - * sure we don't fail handling the vm fault by calling >> - * i915_gem_object_has_cache_level() which always return true for such >> - * objects. Otherwise this helper function would fall back to checking >> - * whether the object is un-cached. >> - */ >> - if (!((obj->pat_set_by_user || >> - i915_gem_object_has_cache_mode(obj, I915_CACHE_MODE_UC)) || >> - HAS_LLC(i915))) { >> + /* Access to snoopable pages through the GTT is incoherent. */ > > This comment was removed in the previous patch, but now it came back > here. Should we have just left it be in the previous patch? Oops yes, fumble when splitting the single patch into this series. > I'm not really clear on what it means either. Are we using "GTT" as > shorthand to refer to the aperture here? It is about CPU mmap access so I think so. Original code was: /* Access to snoopable pages through the GTT is incoherent. */ if (obj->cache_level != I915_CACHE_NONE && !HAS_LLC(i915)) { ret = -EFAULT; goto err_unpin; } Which was disallowing anything not uncached on snoopable platforms. So I made it equivalent to that: /* Access to snoopable pages through the GTT is incoherent. */ if (!i915_gem_object_has_cache_mode(obj, I915_CACHE_MODE_UC) && !HAS_LLC(i915)) { ret = -EFAULT; goto err_unpin; } Should be like-for-like assuming PAT-to-cache-mode tables are all good. On Meteorlake it is no change in behaviour either way due !HAS_LLC. Regards, Tvrtko > > Matt > >> + if (!i915_gem_object_has_cache_mode(obj, I915_CACHE_MODE_UC) && >> + !HAS_LLC(i915)) { >> ret = -EFAULT; >> goto err_unpin; >> } >> -- >> 2.39.2 >> >
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index cd7f8ded0d6f..9aa6ecf68432 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -382,17 +382,9 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf) goto err_reset; } - /* - * For objects created by userspace through GEM_CREATE with pat_index - * set by set_pat extension, coherency is managed by userspace, make - * sure we don't fail handling the vm fault by calling - * i915_gem_object_has_cache_level() which always return true for such - * objects. Otherwise this helper function would fall back to checking - * whether the object is un-cached. - */ - if (!((obj->pat_set_by_user || - i915_gem_object_has_cache_mode(obj, I915_CACHE_MODE_UC)) || - HAS_LLC(i915))) { + /* Access to snoopable pages through the GTT is incoherent. */ + if (!i915_gem_object_has_cache_mode(obj, I915_CACHE_MODE_UC) && + !HAS_LLC(i915)) { ret = -EFAULT; goto err_unpin; }