| Message ID | 9f92ca239245bce8db9c90a4d643d8101ffc9269.1615872114.git.ashutosh.dixit@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Drop legacy IOCTLs on new HW | expand |
On Mon, Mar 15, 2021 at 10:29:20PM -0700, Ashutosh Dixit wrote: > From: Jason Ekstrand <jason@jlekstrand.net> > > The Vulkan driver in Mesa for Intel hardware never uses relocations if > it's running on a version of i915 that supports at least softpin which > all versions of i915 supporting Gen12 do. On the OpenGL side, Gen12+ is > only supported by iris which never uses relocations. The older i965 > driver in Mesa does use relocations but it only supports Intel hardware > through Gen11 and has been deprecated for all hardware Gen9+. The > compute driver also never uses relocations. This only leaves the media > driver which is supposed to be switching to softpin going forward. > Making softpin a requirement for all future hardware seems reasonable. > > There is one piece of hardware enabled by default in i915: RKL which was > enabled by e22fa6f0a976 which has not yet landed in drm-next so this > almost but not really a userspace API change for RKL. If it becomes a > problem, we can always add !IS_ROCKETLAKE(eb->i915) to the condition. > > Rejecting relocations starting with newer Gen12 platforms has the > benefit that we don't have to bother supporting it on platforms with > local memory. Given how much CPU touching of memory is required for > relocations, not having to do so on platforms where not all memory is > directly CPU-accessible carries significant advantages. > > v2 (Jason Ekstrand): > - Allow TGL-LP platforms as they've already shipped > > v3 (Jason Ekstrand): > - WARN_ON platforms with LMEM support in case the check is wrong > > v4 (Jason Ekstrand): > - Call out Rocket Lake in the commit message > > v5 (Jason Ekstrand): > - Drop the HAS_LMEM check as it's already covered by the version check > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index 99772f37bff60..f66cff2943baa 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -1764,7 +1764,8 @@ eb_relocate_vma_slow(struct i915_execbuffer *eb, struct eb_vma *ev) > return err; > } > > -static int check_relocations(const struct drm_i915_gem_exec_object2 *entry) > +static int check_relocations(const struct i915_execbuffer *eb, > + const struct drm_i915_gem_exec_object2 *entry) > { > const char __user *addr, *end; > unsigned long size; > @@ -1774,6 +1775,12 @@ static int check_relocations(const struct drm_i915_gem_exec_object2 *entry) > if (size == 0) > return 0; > > + /* Relocations are disallowed for all platforms after TGL-LP. This > + * also covers all platforms with local memory. > + */ > + if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915)) > + return -EINVAL; > + > if (size > N_RELOC(ULONG_MAX)) > return -EINVAL; > > @@ -1807,7 +1814,7 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb) > if (nreloc == 0) > continue; > > - err = check_relocations(&eb->exec[i]); > + err = check_relocations(eb, &eb->exec[i]); > if (err) > goto err; > > @@ -1880,7 +1887,7 @@ static int eb_prefault_relocations(const struct i915_execbuffer *eb) > for (i = 0; i < count; i++) { > int err; > > - err = check_relocations(&eb->exec[i]); > + err = check_relocations(eb, &eb->exec[i]); > if (err) > return err; > } Disclaimer: I don't know deeply how any of this works. check_relocations() is only called if eb_relocate_parse goes on its slowpath, fast path doesn't check for gen. Should it, or am I misunderstanding something?
On Wed, Mar 17, 2021 at 2:55 AM Petri Latvala <petri.latvala@intel.com> wrote: > > On Mon, Mar 15, 2021 at 10:29:20PM -0700, Ashutosh Dixit wrote: > > From: Jason Ekstrand <jason@jlekstrand.net> > > > > The Vulkan driver in Mesa for Intel hardware never uses relocations if > > it's running on a version of i915 that supports at least softpin which > > all versions of i915 supporting Gen12 do. On the OpenGL side, Gen12+ is > > only supported by iris which never uses relocations. The older i965 > > driver in Mesa does use relocations but it only supports Intel hardware > > through Gen11 and has been deprecated for all hardware Gen9+. The > > compute driver also never uses relocations. This only leaves the media > > driver which is supposed to be switching to softpin going forward. > > Making softpin a requirement for all future hardware seems reasonable. > > > > There is one piece of hardware enabled by default in i915: RKL which was > > enabled by e22fa6f0a976 which has not yet landed in drm-next so this > > almost but not really a userspace API change for RKL. If it becomes a > > problem, we can always add !IS_ROCKETLAKE(eb->i915) to the condition. > > > > Rejecting relocations starting with newer Gen12 platforms has the > > benefit that we don't have to bother supporting it on platforms with > > local memory. Given how much CPU touching of memory is required for > > relocations, not having to do so on platforms where not all memory is > > directly CPU-accessible carries significant advantages. > > > > v2 (Jason Ekstrand): > > - Allow TGL-LP platforms as they've already shipped > > > > v3 (Jason Ekstrand): > > - WARN_ON platforms with LMEM support in case the check is wrong > > > > v4 (Jason Ekstrand): > > - Call out Rocket Lake in the commit message > > > > v5 (Jason Ekstrand): > > - Drop the HAS_LMEM check as it's already covered by the version check > > > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > > Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > index 99772f37bff60..f66cff2943baa 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > @@ -1764,7 +1764,8 @@ eb_relocate_vma_slow(struct i915_execbuffer *eb, struct eb_vma *ev) > > return err; > > } > > > > -static int check_relocations(const struct drm_i915_gem_exec_object2 *entry) > > +static int check_relocations(const struct i915_execbuffer *eb, > > + const struct drm_i915_gem_exec_object2 *entry) > > { > > const char __user *addr, *end; > > unsigned long size; > > @@ -1774,6 +1775,12 @@ static int check_relocations(const struct drm_i915_gem_exec_object2 *entry) > > if (size == 0) > > return 0; > > > > + /* Relocations are disallowed for all platforms after TGL-LP. This > > + * also covers all platforms with local memory. > > + */ > > + if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915)) > > + return -EINVAL; > > + > > if (size > N_RELOC(ULONG_MAX)) > > return -EINVAL; > > > > @@ -1807,7 +1814,7 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb) > > if (nreloc == 0) > > continue; > > > > - err = check_relocations(&eb->exec[i]); > > + err = check_relocations(eb, &eb->exec[i]); > > if (err) > > goto err; > > > > @@ -1880,7 +1887,7 @@ static int eb_prefault_relocations(const struct i915_execbuffer *eb) > > for (i = 0; i < count; i++) { > > int err; > > > > - err = check_relocations(&eb->exec[i]); > > + err = check_relocations(eb, &eb->exec[i]); > > if (err) > > return err; > > } > > > Disclaimer: I don't know deeply how any of this works. > > check_relocations() is only called if eb_relocate_parse goes on its > slowpath, fast path doesn't check for gen. Should it, or am I > misunderstanding something? Good eye! Yeah, this doesn't work for the reason you just said. I was trying to avoid an up-front list walk of the object list but if we want to be very sure it's correct, we probably have to. --Jason
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 99772f37bff60..f66cff2943baa 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -1764,7 +1764,8 @@ eb_relocate_vma_slow(struct i915_execbuffer *eb, struct eb_vma *ev) return err; } -static int check_relocations(const struct drm_i915_gem_exec_object2 *entry) +static int check_relocations(const struct i915_execbuffer *eb, + const struct drm_i915_gem_exec_object2 *entry) { const char __user *addr, *end; unsigned long size; @@ -1774,6 +1775,12 @@ static int check_relocations(const struct drm_i915_gem_exec_object2 *entry) if (size == 0) return 0; + /* Relocations are disallowed for all platforms after TGL-LP. This + * also covers all platforms with local memory. + */ + if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915)) + return -EINVAL; + if (size > N_RELOC(ULONG_MAX)) return -EINVAL; @@ -1807,7 +1814,7 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb) if (nreloc == 0) continue; - err = check_relocations(&eb->exec[i]); + err = check_relocations(eb, &eb->exec[i]); if (err) goto err; @@ -1880,7 +1887,7 @@ static int eb_prefault_relocations(const struct i915_execbuffer *eb) for (i = 0; i < count; i++) { int err; - err = check_relocations(&eb->exec[i]); + err = check_relocations(eb, &eb->exec[i]); if (err) return err; }