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 entire relocation UAPI and related infrastructure, therefore, doesn't have any open-source userspace consumer starting with Gen12.
Rejecting relocations starting with Gen12 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.
Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Dave Airlie airlied@redhat.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 4f9c1f5a4dedb..e10c93aff945d 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -1533,7 +1533,8 @@ eb_relocate_vma_slow(struct i915_execbuffer *eb, struct i915_vma *vma) 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; @@ -1543,6 +1544,9 @@ static int check_relocations(const struct drm_i915_gem_exec_object2 *entry) if (size == 0) return 0;
+ if (size && eb->reloc_cache.gen >= 12) + return -EINVAL; + if (size > N_RELOC(ULONG_MAX)) return -EINVAL;
@@ -1576,7 +1580,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;
Quoting Jason Ekstrand (2020-05-07 16:36:00)
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 entire relocation UAPI and related infrastructure, therefore, doesn't have any open-source userspace consumer starting with Gen12.
Rejecting relocations starting with Gen12 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.
You are not supplying them, the kernel is not checking them [as they don't exist], so there is no material benefit. The only question is maintainability.
How confident are you that you will never use them and rewrite the media-driver? The code exists, will be tested, and can just as easily expire with the rest of execbuffer2. -Chris
On Thu, May 7, 2020 at 10:44 AM Chris Wilson chris@chris-wilson.co.uk wrote:
Quoting Jason Ekstrand (2020-05-07 16:36:00)
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 entire relocation UAPI and related infrastructure, therefore, doesn't have any open-source userspace consumer starting with Gen12.
Rejecting relocations starting with Gen12 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.
You are not supplying them, the kernel is not checking them [as they don't exist], so there is no material benefit. The only question is maintainability.
How confident are you that you will never use them
Confident enough to send this patch. Especially in a Vulkan world where it's very hard to tell which bits of memory are actually in-use on the GPU, stalling to relocate is performance death. With a 48-bit GTT, there's no need to have the kernel involved in address space assignment so relocations don't really serve much purpose. We did potentially have one use for them with VK_KHR_performance_query but we're going out of our way to avoid them there. If we desperately need relocations, we can do them from userspace.
and rewrite the media-driver?
I'm pretty sure they're working on getting rid of them. I'm checking on that right now.
The code exists, will be tested, and can just as easily expire with the rest of execbuffer2.
Sure. The question, again, is maintenance. If we're spending piles of time trying to figure out how to keep relocations going in a local memory world, that's likely a waste. Relocations can sit and rot on Gen11 and below until we finally make execbuffer3 a reality and then they can rot in the deprecated execbuffer2 ioct.
There is a bit of a question here about what to do with IGT. I suspect it uses relocations for a lot of stuff and the fallout there could be significant. (I explicitly made this patch so it won't actually build because I don't hate our CI people.)
--Jason
On Fri, 8 May 2020 at 01:44, Chris Wilson chris@chris-wilson.co.uk wrote:
Quoting Jason Ekstrand (2020-05-07 16:36:00)
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 entire relocation UAPI and related infrastructure, therefore, doesn't have any open-source userspace consumer starting with Gen12.
Rejecting relocations starting with Gen12 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.
You are not supplying them, the kernel is not checking them [as they don't exist], so there is no material benefit. The only question is maintainability.
How confident are you that you will never use them and rewrite the media-driver? The code exists, will be tested, and can just as easily expire with the rest of execbuffer2.
From an upstream POV I say hell yes, if the hw isn't generally available yet,
and the media-driver/intel-compute-runtime people are warned in advance,
I'm all in on ripping it out for new GENs.
Dave.
Quoting Dave Airlie (2020-05-07 21:27:27)
On Fri, 8 May 2020 at 01:44, Chris Wilson chris@chris-wilson.co.uk wrote:
Quoting Jason Ekstrand (2020-05-07 16:36:00)
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 entire relocation UAPI and related infrastructure, therefore, doesn't have any open-source userspace consumer starting with Gen12.
Rejecting relocations starting with Gen12 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.
You are not supplying them, the kernel is not checking them [as they don't exist], so there is no material benefit. The only question is maintainability.
How confident are you that you will never use them and rewrite the media-driver? The code exists, will be tested, and can just as easily expire with the rest of execbuffer2.
From an upstream POV I say hell yes, if the hw isn't generally available yet, and the media-driver/intel-compute-runtime people are warned in advance,
I'm all in on ripping it out for new GENs.
There have been discussions with our media driver team about eliminating any relocations, but today they are still being used. They have started partially using soft-pinning, which is a great first step to that direction.
The compute driver does not rely on relocations, they use soft-pinning everywhere and explicitly manage the address space.
Be assured that I'm also in favor of eliminating relocations (just like execbuffer2, userptr and couple other things), just that we still need to have a functional stack before they can be dropped for new hardware.
Like Chris mentioned, enough optimization have been put in the code so that there is zero impact from the relocations to the exclusively soft-pinning drivers. So the sole benefit would be being able to drop the relocations code in the future when the Gen11 hardware has gone exctinct and that is a worthy goal, too.
But for now the feature is still needed for Gen12, so forcefully disabling it is untimely.
Regards, Joonas
On Fri, 8 May 2020 at 15:58, Joonas Lahtinen joonas.lahtinen@linux.intel.com wrote:
Quoting Dave Airlie (2020-05-07 21:27:27)
On Fri, 8 May 2020 at 01:44, Chris Wilson chris@chris-wilson.co.uk wrote:
Quoting Jason Ekstrand (2020-05-07 16:36:00)
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 entire relocation UAPI and related infrastructure, therefore, doesn't have any open-source userspace consumer starting with Gen12.
Rejecting relocations starting with Gen12 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.
You are not supplying them, the kernel is not checking them [as they don't exist], so there is no material benefit. The only question is maintainability.
How confident are you that you will never use them and rewrite the media-driver? The code exists, will be tested, and can just as easily expire with the rest of execbuffer2.
From an upstream POV I say hell yes, if the hw isn't generally available yet, and the media-driver/intel-compute-runtime people are warned in advance,
I'm all in on ripping it out for new GENs.
There have been discussions with our media driver team about eliminating any relocations, but today they are still being used. They have started partially using soft-pinning, which is a great first step to that direction.
The compute driver does not rely on relocations, they use soft-pinning everywhere and explicitly manage the address space.
Be assured that I'm also in favor of eliminating relocations (just like execbuffer2, userptr and couple other things), just that we still need to have a functional stack before they can be dropped for new hardware.
Like Chris mentioned, enough optimization have been put in the code so that there is zero impact from the relocations to the exclusively soft-pinning drivers. So the sole benefit would be being able to drop the relocations code in the future when the Gen11 hardware has gone exctinct and that is a worthy goal, too.
But for now the feature is still needed for Gen12, so forcefully disabling it is untimely.
I'm going to ask that this be revisited for DG1.
DG1 is a discrete GPU,a brand new thing that in no way requires relocations. If relocations are required for legacy software, that software is being updated to add local memory support, relocations should be removed at the same time.
The main reason for this is I believe a lot of effort is being put into making relocations faster and better that is impacting all over the i915 driver. instead of just fixing userspace to not require them anymore moving forward.
I'd rather DG1 support gets upstream in a sane fashion without having to worry about how super-optimised the relocation paths are for some corner case userspace code that if it was part of the mesa project would have been updated by now.
Dave.
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.
Rejecting relocations starting with Gen12 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.
Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Dave Airlie airlied@redhat.com Cc: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 14 +++++++++++--- 1 file changed, 11 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..de093acb7721c 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,13 @@ 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 */ + if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915)) + return -EINVAL; + + /* All discrete memory platforms are Gen12 or above */ + BUG_ON(HAS_LMEM(eb->i915)); + if (size > N_RELOC(ULONG_MAX)) return -EINVAL;
@@ -1807,7 +1815,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 +1888,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; }
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.
Rejecting relocations starting with Gen12 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
Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Dave Airlie airlied@redhat.com Cc: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++++++++--- 1 file changed, 12 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..b02dbd16bfa03 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,14 @@ 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 */ + if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915)) + return -EINVAL; + + /* All discrete memory platforms are Gen12 or above */ + if (WARN_ON(HAS_LMEM(eb->i915))) + return -EINVAL; + if (size > N_RELOC(ULONG_MAX)) return -EINVAL;
@@ -1807,7 +1816,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 +1889,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; }
+Zbigniew for review
On Wed, Mar 10, 2021 at 3:50 PM Jason Ekstrand jason@jlekstrand.net wrote:
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.
Rejecting relocations starting with Gen12 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
Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Dave Airlie airlied@redhat.com Cc: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++++++++--- 1 file changed, 12 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..b02dbd16bfa03 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,14 @@ 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 */if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915))return -EINVAL;/* All discrete memory platforms are Gen12 or above */if (WARN_ON(HAS_LMEM(eb->i915)))return -EINVAL;if (size > N_RELOC(ULONG_MAX)) return -EINVAL;@@ -1807,7 +1816,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 +1889,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; }-- 2.29.2
On Wed, Mar 10, 2021 at 1:50 PM Jason Ekstrand jason@jlekstrand.net wrote:
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.
Rejecting relocations starting with Gen12 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
Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Dave Airlie airlied@redhat.com Cc: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++++++++--- 1 file changed, 12 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..b02dbd16bfa03 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,14 @@ 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 */if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915))return -EINVAL;/* All discrete memory platforms are Gen12 or above */if (WARN_ON(HAS_LMEM(eb->i915)))
HAS_LMEM() will return true for the fake lmem support, which may be < gen12. Dropping the fake lmem would be a possibility.
Lucas De Marchi
return -EINVAL;if (size > N_RELOC(ULONG_MAX)) return -EINVAL;@@ -1807,7 +1816,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 +1889,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; }-- 2.29.2
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 11/03/2021 08:14, Lucas De Marchi wrote:
On Wed, Mar 10, 2021 at 1:50 PM Jason Ekstrand jason@jlekstrand.net wrote:
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.
Rejecting relocations starting with Gen12 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
Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Dave Airlie airlied@redhat.com Cc: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++++++++--- 1 file changed, 12 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..b02dbd16bfa03 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)@@ -1774,6 +1775,14 @@ 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 */if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915))return -EINVAL;/* All discrete memory platforms are Gen12 or above */if (WARN_ON(HAS_LMEM(eb->i915)))HAS_LMEM() will return true for the fake lmem support, which may be < gen12. Dropping the fake lmem would be a possibility.
Nothing in CI is currently using the fake-lmem configuration(for some months now I think), also its use was limited only to the selftests, which can't hit this path, so shouldn't matter here. Plan was to just drop it.
Lucas De Marchi
return -EINVAL;if (size > N_RELOC(ULONG_MAX)) return -EINVAL;@@ -1807,7 +1816,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 +1889,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; }-- 2.29.2
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 10/03/2021 21:50, Jason Ekstrand wrote:
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.
How does "supposed to be" translates to actually being ready? Cc someone from media so they can ack?
Making softpin a requirement for all future hardware seems reasonable.
Rejecting relocations starting with Gen12 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
Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Dave Airlie airlied@redhat.com Cc: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++++++++--- 1 file changed, 12 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..b02dbd16bfa03 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)@@ -1774,6 +1775,14 @@ 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 */
- if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915))
return -EINVAL;- /* All discrete memory platforms are Gen12 or above */
- if (WARN_ON(HAS_LMEM(eb->i915)))
return -EINVAL;
Maybe ENODEV as our more typical "this platform does not support this" instead of "you are using it wrong".
Regards,
Tvrtko
- if (size > N_RELOC(ULONG_MAX)) return -EINVAL;
@@ -1807,7 +1816,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]);@@ -1880,7 +1889,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]);
On Wed, Mar 10, 2021 at 03:50:07PM -0600, Jason Ekstrand wrote:
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.
Rejecting relocations starting with Gen12 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
I was asked to review of this patch. It works along with expected IGT check https://patchwork.freedesktop.org/patch/423361/?series=82954&rev=25
Before I'll give you r-b - isn't i915_gem_execbuffer2_ioctl() better place to do for loop just after copy_from_user() and check relocation_count? We have an access to exec2_list there, we know the gen so we're able to say relocations are not supported immediate, without entering i915_gem_do_execbuffer().
-- Zbigniew
Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Dave Airlie airlied@redhat.com Cc: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++++++++--- 1 file changed, 12 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..b02dbd16bfa03 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,14 @@ 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 */
- if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915))
return -EINVAL;- /* All discrete memory platforms are Gen12 or above */
- if (WARN_ON(HAS_LMEM(eb->i915)))
return -EINVAL;- if (size > N_RELOC(ULONG_MAX)) return -EINVAL;
@@ -1807,7 +1816,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]);@@ -1880,7 +1889,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]);-- 2.29.2
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Mar 11, 2021 at 5:44 AM Zbigniew Kempczyński zbigniew.kempczynski@intel.com wrote:
On Wed, Mar 10, 2021 at 03:50:07PM -0600, Jason Ekstrand wrote:
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.
Rejecting relocations starting with Gen12 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
I was asked to review of this patch. It works along with expected IGT check https://patchwork.freedesktop.org/patch/423361/?series=82954&rev=25
Before I'll give you r-b - isn't i915_gem_execbuffer2_ioctl() better place to do for loop just after copy_from_user() and check relocation_count? We have an access to exec2_list there, we know the gen so we're able to say relocations are not supported immediate, without entering i915_gem_do_execbuffer().
I considered that but it adds an extra object list walk for a case which we expect to not happen. I'm not sure how expensive the list walk would be if all we do is check the number of relocations on each object. I guess, if it comes right after a copy_from_user, it's all hot in the cache so it shouldn't matter. Ok. I've convinced myself. I'll move it.
--Jason
-- Zbigniew
Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Dave Airlie airlied@redhat.com Cc: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++++++++--- 1 file changed, 12 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..b02dbd16bfa03 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,14 @@ 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 */if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915))return -EINVAL;/* All discrete memory platforms are Gen12 or above */if (WARN_ON(HAS_LMEM(eb->i915)))return -EINVAL;if (size > N_RELOC(ULONG_MAX)) return -EINVAL;@@ -1807,7 +1816,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 +1889,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; }-- 2.29.2
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Mar 11, 2021 at 4:50 PM Jason Ekstrand jason@jlekstrand.net wrote:
On Thu, Mar 11, 2021 at 5:44 AM Zbigniew Kempczyński zbigniew.kempczynski@intel.com wrote:
On Wed, Mar 10, 2021 at 03:50:07PM -0600, Jason Ekstrand wrote:
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.
Rejecting relocations starting with Gen12 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
I was asked to review of this patch. It works along with expected IGT check https://patchwork.freedesktop.org/patch/423361/?series=82954&rev=25
Before I'll give you r-b - isn't i915_gem_execbuffer2_ioctl() better place to do for loop just after copy_from_user() and check relocation_count? We have an access to exec2_list there, we know the gen so we're able to say relocations are not supported immediate, without entering i915_gem_do_execbuffer().
I considered that but it adds an extra object list walk for a case which we expect to not happen. I'm not sure how expensive the list walk would be if all we do is check the number of relocations on each object. I guess, if it comes right after a copy_from_user, it's all hot in the cache so it shouldn't matter. Ok. I've convinced myself. I'll move it.
I really wouldn't move it if it's another list walk. Execbuf has a lot of fast-paths going on, and we have extensive tests to make sure it unwinds correctly in all cases. It's not very intuitive, but execbuf code isn't scoring very high on that. -Daniel
--Jason
-- Zbigniew
Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Dave Airlie airlied@redhat.com Cc: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++++++++--- 1 file changed, 12 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..b02dbd16bfa03 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,14 @@ 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 */if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915))return -EINVAL;/* All discrete memory platforms are Gen12 or above */if (WARN_ON(HAS_LMEM(eb->i915)))return -EINVAL;if (size > N_RELOC(ULONG_MAX)) return -EINVAL;@@ -1807,7 +1816,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 +1889,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; }-- 2.29.2
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Mar 11, 2021 at 9:57 AM Daniel Vetter daniel@ffwll.ch wrote:
On Thu, Mar 11, 2021 at 4:50 PM Jason Ekstrand jason@jlekstrand.net wrote:
On Thu, Mar 11, 2021 at 5:44 AM Zbigniew Kempczyński zbigniew.kempczynski@intel.com wrote:
On Wed, Mar 10, 2021 at 03:50:07PM -0600, Jason Ekstrand wrote:
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.
Rejecting relocations starting with Gen12 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
I was asked to review of this patch. It works along with expected IGT check https://patchwork.freedesktop.org/patch/423361/?series=82954&rev=25
Before I'll give you r-b - isn't i915_gem_execbuffer2_ioctl() better place to do for loop just after copy_from_user() and check relocation_count? We have an access to exec2_list there, we know the gen so we're able to say relocations are not supported immediate, without entering i915_gem_do_execbuffer().
I considered that but it adds an extra object list walk for a case which we expect to not happen. I'm not sure how expensive the list walk would be if all we do is check the number of relocations on each object. I guess, if it comes right after a copy_from_user, it's all hot in the cache so it shouldn't matter. Ok. I've convinced myself. I'll move it.
I really wouldn't move it if it's another list walk. Execbuf has a lot of fast-paths going on, and we have extensive tests to make sure it unwinds correctly in all cases. It's not very intuitive, but execbuf code isn't scoring very high on that.
And here I'd just finished doing the typing to move it. Good thing I hadn't closed vim yet and it was still in my undo buffer. :-)
--Jason
-Daniel
--Jason
-- Zbigniew
Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Dave Airlie airlied@redhat.com Cc: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++++++++--- 1 file changed, 12 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..b02dbd16bfa03 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,14 @@ 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 */if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915))return -EINVAL;/* All discrete memory platforms are Gen12 or above */if (WARN_ON(HAS_LMEM(eb->i915)))return -EINVAL;if (size > N_RELOC(ULONG_MAX)) return -EINVAL;@@ -1807,7 +1816,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 +1889,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; }-- 2.29.2
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Thu, Mar 11, 2021 at 10:24:38AM -0600, Jason Ekstrand wrote:
On Thu, Mar 11, 2021 at 9:57 AM Daniel Vetter daniel@ffwll.ch wrote:
On Thu, Mar 11, 2021 at 4:50 PM Jason Ekstrand jason@jlekstrand.net wrote:
On Thu, Mar 11, 2021 at 5:44 AM Zbigniew Kempczyński zbigniew.kempczynski@intel.com wrote:
On Wed, Mar 10, 2021 at 03:50:07PM -0600, Jason Ekstrand wrote:
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.
Rejecting relocations starting with Gen12 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
I was asked to review of this patch. It works along with expected IGT check https://patchwork.freedesktop.org/patch/423361/?series=82954&rev=25
Before I'll give you r-b - isn't i915_gem_execbuffer2_ioctl() better place to do for loop just after copy_from_user() and check relocation_count? We have an access to exec2_list there, we know the gen so we're able to say relocations are not supported immediate, without entering i915_gem_do_execbuffer().
I considered that but it adds an extra object list walk for a case which we expect to not happen. I'm not sure how expensive the list walk would be if all we do is check the number of relocations on each object. I guess, if it comes right after a copy_from_user, it's all hot in the cache so it shouldn't matter. Ok. I've convinced myself. I'll move it.
I really wouldn't move it if it's another list walk. Execbuf has a lot of fast-paths going on, and we have extensive tests to make sure it unwinds correctly in all cases. It's not very intuitive, but execbuf code isn't scoring very high on that.
And here I'd just finished doing the typing to move it. Good thing I hadn't closed vim yet and it was still in my undo buffer. :-)
Before entering "slower" path from my perspective I would just check batch object at that place. We still would have single list walkthrough and quick check on the very beginning.
-- Zbigniew
--Jason
-Daniel
--Jason
-- Zbigniew
Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Dave Airlie airlied@redhat.com Cc: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++++++++--- 1 file changed, 12 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..b02dbd16bfa03 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,14 @@ 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 */if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915))return -EINVAL;/* All discrete memory platforms are Gen12 or above */if (WARN_ON(HAS_LMEM(eb->i915)))return -EINVAL;if (size > N_RELOC(ULONG_MAX)) return -EINVAL;@@ -1807,7 +1816,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 +1889,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; }-- 2.29.2
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Thu, Mar 11, 2021 at 10:51 AM Zbigniew Kempczyński zbigniew.kempczynski@intel.com wrote:
On Thu, Mar 11, 2021 at 10:24:38AM -0600, Jason Ekstrand wrote:
On Thu, Mar 11, 2021 at 9:57 AM Daniel Vetter daniel@ffwll.ch wrote:
On Thu, Mar 11, 2021 at 4:50 PM Jason Ekstrand jason@jlekstrand.net wrote:
On Thu, Mar 11, 2021 at 5:44 AM Zbigniew Kempczyński zbigniew.kempczynski@intel.com wrote:
On Wed, Mar 10, 2021 at 03:50:07PM -0600, Jason Ekstrand wrote:
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.
Rejecting relocations starting with Gen12 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
I was asked to review of this patch. It works along with expected IGT check https://patchwork.freedesktop.org/patch/423361/?series=82954&rev=25
Before I'll give you r-b - isn't i915_gem_execbuffer2_ioctl() better place to do for loop just after copy_from_user() and check relocation_count? We have an access to exec2_list there, we know the gen so we're able to say relocations are not supported immediate, without entering i915_gem_do_execbuffer().
I considered that but it adds an extra object list walk for a case which we expect to not happen. I'm not sure how expensive the list walk would be if all we do is check the number of relocations on each object. I guess, if it comes right after a copy_from_user, it's all hot in the cache so it shouldn't matter. Ok. I've convinced myself. I'll move it.
I really wouldn't move it if it's another list walk. Execbuf has a lot of fast-paths going on, and we have extensive tests to make sure it unwinds correctly in all cases. It's not very intuitive, but execbuf code isn't scoring very high on that.
And here I'd just finished doing the typing to move it. Good thing I hadn't closed vim yet and it was still in my undo buffer. :-)
Before entering "slower" path from my perspective I would just check batch object at that place. We still would have single list walkthrough and quick check on the very beginning.
Can you be more specific about what exactly you think we can check at the beginning? Either we add a flag that we can O(1) check at the beginning or we have to check everything in exec2_list for exec2_list[n].relocation_count == 0. That's a list walk. I'm not seeing what up-front check you're thinking we can do without the list walk.
--Jason
-- Zbigniew
--Jason
-Daniel
--Jason
-- Zbigniew
Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Dave Airlie airlied@redhat.com Cc: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++++++++--- 1 file changed, 12 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..b02dbd16bfa03 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,14 @@ 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 */if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915))return -EINVAL;/* All discrete memory platforms are Gen12 or above */if (WARN_ON(HAS_LMEM(eb->i915)))return -EINVAL;if (size > N_RELOC(ULONG_MAX)) return -EINVAL;@@ -1807,7 +1816,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 +1889,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; }-- 2.29.2
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Thu, Mar 11, 2021 at 11:18:11AM -0600, Jason Ekstrand wrote:
On Thu, Mar 11, 2021 at 10:51 AM Zbigniew Kempczyński zbigniew.kempczynski@intel.com wrote:
On Thu, Mar 11, 2021 at 10:24:38AM -0600, Jason Ekstrand wrote:
On Thu, Mar 11, 2021 at 9:57 AM Daniel Vetter daniel@ffwll.ch wrote:
On Thu, Mar 11, 2021 at 4:50 PM Jason Ekstrand jason@jlekstrand.net wrote:
On Thu, Mar 11, 2021 at 5:44 AM Zbigniew Kempczyński zbigniew.kempczynski@intel.com wrote:
On Wed, Mar 10, 2021 at 03:50:07PM -0600, Jason Ekstrand wrote: > 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. > > Rejecting relocations starting with Gen12 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
I was asked to review of this patch. It works along with expected IGT check https://patchwork.freedesktop.org/patch/423361/?series=82954&rev=25
Before I'll give you r-b - isn't i915_gem_execbuffer2_ioctl() better place to do for loop just after copy_from_user() and check relocation_count? We have an access to exec2_list there, we know the gen so we're able to say relocations are not supported immediate, without entering i915_gem_do_execbuffer().
I considered that but it adds an extra object list walk for a case which we expect to not happen. I'm not sure how expensive the list walk would be if all we do is check the number of relocations on each object. I guess, if it comes right after a copy_from_user, it's all hot in the cache so it shouldn't matter. Ok. I've convinced myself. I'll move it.
I really wouldn't move it if it's another list walk. Execbuf has a lot of fast-paths going on, and we have extensive tests to make sure it unwinds correctly in all cases. It's not very intuitive, but execbuf code isn't scoring very high on that.
And here I'd just finished doing the typing to move it. Good thing I hadn't closed vim yet and it was still in my undo buffer. :-)
Before entering "slower" path from my perspective I would just check batch object at that place. We still would have single list walkthrough and quick check on the very beginning.
Can you be more specific about what exactly you think we can check at the beginning? Either we add a flag that we can O(1) check at the beginning or we have to check everything in exec2_list for exec2_list[n].relocation_count == 0. That's a list walk. I'm not seeing what up-front check you're thinking we can do without the list walk.
I expect that last (default) or first (I915_EXEC_BATCH_FIRST) execobj (batch) will likely has relocations. So we can check that single object without entering i915_gem_do_execbuffer(). If that check is missed (relocation_count = 0) you'll catch relocations in other objects in check_relocations() as you already did. This is simple optimization but we can avoid two iterations over buffer list (first is in eb_lookup_vmas()).
-- Zbigniew
--Jason
-- Zbigniew
--Jason
-Daniel
--Jason
-- Zbigniew
> > Signed-off-by: Jason Ekstrand jason@jlekstrand.net > Cc: Dave Airlie airlied@redhat.com > Cc: Daniel Vetter daniel.vetter@intel.com > --- > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++++++++--- > 1 file changed, 12 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..b02dbd16bfa03 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,14 @@ 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 */ > + if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915)) > + return -EINVAL; > + > + /* All discrete memory platforms are Gen12 or above */ > + if (WARN_ON(HAS_LMEM(eb->i915))) > + return -EINVAL; > + > if (size > N_RELOC(ULONG_MAX)) > return -EINVAL; > > @@ -1807,7 +1816,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 +1889,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; > } > -- > 2.29.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Thu, Mar 11, 2021 at 12:20 PM Zbigniew Kempczyński zbigniew.kempczynski@intel.com wrote:
On Thu, Mar 11, 2021 at 11:18:11AM -0600, Jason Ekstrand wrote:
On Thu, Mar 11, 2021 at 10:51 AM Zbigniew Kempczyński zbigniew.kempczynski@intel.com wrote:
On Thu, Mar 11, 2021 at 10:24:38AM -0600, Jason Ekstrand wrote:
On Thu, Mar 11, 2021 at 9:57 AM Daniel Vetter daniel@ffwll.ch wrote:
On Thu, Mar 11, 2021 at 4:50 PM Jason Ekstrand jason@jlekstrand.net wrote:
On Thu, Mar 11, 2021 at 5:44 AM Zbigniew Kempczyński zbigniew.kempczynski@intel.com wrote: > > On Wed, Mar 10, 2021 at 03:50:07PM -0600, Jason Ekstrand wrote: > > 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. > > > > Rejecting relocations starting with Gen12 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 > > I was asked to review of this patch. It works along with expected > IGT check https://patchwork.freedesktop.org/patch/423361/?series=82954&rev=25 > > Before I'll give you r-b - isn't i915_gem_execbuffer2_ioctl() better place > to do for loop just after copy_from_user() and check relocation_count? > We have an access to exec2_list there, we know the gen so we're able to say > relocations are not supported immediate, without entering i915_gem_do_execbuffer().
I considered that but it adds an extra object list walk for a case which we expect to not happen. I'm not sure how expensive the list walk would be if all we do is check the number of relocations on each object. I guess, if it comes right after a copy_from_user, it's all hot in the cache so it shouldn't matter. Ok. I've convinced myself. I'll move it.
I really wouldn't move it if it's another list walk. Execbuf has a lot of fast-paths going on, and we have extensive tests to make sure it unwinds correctly in all cases. It's not very intuitive, but execbuf code isn't scoring very high on that.
And here I'd just finished doing the typing to move it. Good thing I hadn't closed vim yet and it was still in my undo buffer. :-)
Before entering "slower" path from my perspective I would just check batch object at that place. We still would have single list walkthrough and quick check on the very beginning.
Can you be more specific about what exactly you think we can check at the beginning? Either we add a flag that we can O(1) check at the beginning or we have to check everything in exec2_list for exec2_list[n].relocation_count == 0. That's a list walk. I'm not seeing what up-front check you're thinking we can do without the list walk.
I expect that last (default) or first (I915_EXEC_BATCH_FIRST) execobj (batch) will likely has relocations. So we can check that single object without entering i915_gem_do_execbuffer(). If that check is missed (relocation_count = 0) you'll catch relocations in other objects in check_relocations() as you already did. This is simple optimization but we can avoid two iterations over buffer list (first is in eb_lookup_vmas()).
Sure, we can do an early-exit check of the first and last objects. I'm just not seeing what that saves us given that we still have to do the full list-walk check anyway. Especially since this is an error path which shouldn't be hit by real userspace drivers.
--Jason
-- Zbigniew
--Jason
-- Zbigniew
--Jason
-Daniel
--Jason
> -- > Zbigniew > > > > > Signed-off-by: Jason Ekstrand jason@jlekstrand.net > > Cc: Dave Airlie airlied@redhat.com > > Cc: Daniel Vetter daniel.vetter@intel.com > > --- > > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++++++++--- > > 1 file changed, 12 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..b02dbd16bfa03 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,14 @@ 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 */ > > + if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915)) > > + return -EINVAL; > > + > > + /* All discrete memory platforms are Gen12 or above */ > > + if (WARN_ON(HAS_LMEM(eb->i915))) > > + return -EINVAL; > > + > > if (size > N_RELOC(ULONG_MAX)) > > return -EINVAL; > > > > @@ -1807,7 +1816,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 +1889,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; > > } > > -- > > 2.29.2 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Thu, Mar 11, 2021 at 12:57:25PM -0600, Jason Ekstrand wrote:
On Thu, Mar 11, 2021 at 12:20 PM Zbigniew Kempczyński zbigniew.kempczynski@intel.com wrote:
On Thu, Mar 11, 2021 at 11:18:11AM -0600, Jason Ekstrand wrote:
On Thu, Mar 11, 2021 at 10:51 AM Zbigniew Kempczyński zbigniew.kempczynski@intel.com wrote:
On Thu, Mar 11, 2021 at 10:24:38AM -0600, Jason Ekstrand wrote:
On Thu, Mar 11, 2021 at 9:57 AM Daniel Vetter daniel@ffwll.ch wrote:
On Thu, Mar 11, 2021 at 4:50 PM Jason Ekstrand jason@jlekstrand.net wrote: > > On Thu, Mar 11, 2021 at 5:44 AM Zbigniew Kempczyński > zbigniew.kempczynski@intel.com wrote: > > > > On Wed, Mar 10, 2021 at 03:50:07PM -0600, Jason Ekstrand wrote: > > > 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. > > > > > > Rejecting relocations starting with Gen12 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 > > > > I was asked to review of this patch. It works along with expected > > IGT check https://patchwork.freedesktop.org/patch/423361/?series=82954&rev=25 > > > > Before I'll give you r-b - isn't i915_gem_execbuffer2_ioctl() better place > > to do for loop just after copy_from_user() and check relocation_count? > > We have an access to exec2_list there, we know the gen so we're able to say > > relocations are not supported immediate, without entering i915_gem_do_execbuffer(). > > I considered that but it adds an extra object list walk for a case > which we expect to not happen. I'm not sure how expensive the list > walk would be if all we do is check the number of relocations on each > object. I guess, if it comes right after a copy_from_user, it's all > hot in the cache so it shouldn't matter. Ok. I've convinced myself. > I'll move it.
I really wouldn't move it if it's another list walk. Execbuf has a lot of fast-paths going on, and we have extensive tests to make sure it unwinds correctly in all cases. It's not very intuitive, but execbuf code isn't scoring very high on that.
And here I'd just finished doing the typing to move it. Good thing I hadn't closed vim yet and it was still in my undo buffer. :-)
Before entering "slower" path from my perspective I would just check batch object at that place. We still would have single list walkthrough and quick check on the very beginning.
Can you be more specific about what exactly you think we can check at the beginning? Either we add a flag that we can O(1) check at the beginning or we have to check everything in exec2_list for exec2_list[n].relocation_count == 0. That's a list walk. I'm not seeing what up-front check you're thinking we can do without the list walk.
I expect that last (default) or first (I915_EXEC_BATCH_FIRST) execobj (batch) will likely has relocations. So we can check that single object without entering i915_gem_do_execbuffer(). If that check is missed (relocation_count = 0) you'll catch relocations in other objects in check_relocations() as you already did. This is simple optimization but we can avoid two iterations over buffer list (first is in eb_lookup_vmas()).
Sure, we can do an early-exit check of the first and last objects. I'm just not seeing what that saves us given that we still have to do the full list-walk check anyway. Especially since this is an error path which shouldn't be hit by real userspace drivers.
Yeah optimizing error checking sounds like the wrong thing to optimize. Userspace is wrong, it might as well have to wait a bit until it gets that rejection :-) -Daniel
Quoting Zbigniew Kempczyński (2021-03-11 11:44:32)
On Wed, Mar 10, 2021 at 03:50:07PM -0600, Jason Ekstrand wrote:
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.
Rejecting relocations starting with Gen12 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
I was asked to review of this patch. It works along with expected IGT check https://patchwork.freedesktop.org/patch/423361/?series=82954&rev=25
Before I'll give you r-b - isn't i915_gem_execbuffer2_ioctl() better place to do for loop just after copy_from_user() and check relocation_count? We have an access to exec2_list there, we know the gen so we're able to say relocations are not supported immediate, without entering i915_gem_do_execbuffer().
There's a NORELOC flag you can enforce as mandatory. That's trivial for userspace to set, really makes sure they are aware of the change afoot, and i915_gem_ceck_execbuffer() will perform the validation upfront with the other flag checks. -Chris
On Thu, Mar 11, 2021 at 10:31 AM Chris Wilson chris@chris-wilson.co.uk wrote:
Quoting Zbigniew Kempczyński (2021-03-11 11:44:32)
On Wed, Mar 10, 2021 at 03:50:07PM -0600, Jason Ekstrand wrote:
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.
Rejecting relocations starting with Gen12 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
I was asked to review of this patch. It works along with expected IGT check https://patchwork.freedesktop.org/patch/423361/?series=82954&rev=25
Before I'll give you r-b - isn't i915_gem_execbuffer2_ioctl() better place to do for loop just after copy_from_user() and check relocation_count? We have an access to exec2_list there, we know the gen so we're able to say relocations are not supported immediate, without entering i915_gem_do_execbuffer().
There's a NORELOC flag you can enforce as mandatory. That's trivial for userspace to set, really makes sure they are aware of the change afoot, and i915_gem_ceck_execbuffer() will perform the validation upfront with the other flag checks.
NORELOC doesn't quite ensure that there are no relocations; it just makes things optional if the kernel hasn't moved anything. I guess we could require userspace to set it but it also doesn't do anything if there are no relocations to begin with. I think I'd personally err on the side of not requiring pointless flags.
--Jason
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
Signed-off-by: Jason Ekstrand jason@jlekstrand.net Acked-by: Keith Packard keithp@keithp.com Cc: Dave Airlie airlied@redhat.com Cc: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++++++++--- 1 file changed, 12 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..b02dbd16bfa03 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,14 @@ 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 */ + if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915)) + return -EINVAL; + + /* All discrete memory platforms are Gen12 or above */ + if (WARN_ON(HAS_LMEM(eb->i915))) + return -EINVAL; + if (size > N_RELOC(ULONG_MAX)) return -EINVAL;
@@ -1807,7 +1816,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 +1889,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; }
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Dave Airlie -
Jason Ekstrand -
Joonas Lahtinen -
Lucas De Marchi -
Matthew Auld -
Tvrtko Ursulin -
Zbigniew Kempczyński