* Re: [PATCH] i915: Drop relocation support on all new hardware (v3)
2021-03-10 21:50 ` [PATCH] i915: Drop relocation support on all new hardware (v3) Jason Ekstrand
@ 2021-03-10 22:56 ` Jason Ekstrand
2021-03-11 8:14 ` [Intel-gfx] " Lucas De Marchi
` (3 subsequent siblings)
4 siblings, 0 replies; 24+ messages in thread
From: Jason Ekstrand @ 2021-03-10 22:56 UTC (permalink / raw)
To: Intel GFX, Maling list - DRI developers
Cc: Dave Airlie, zbigniew.kempczynski, Daniel Vetter
+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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [PATCH] i915: Drop relocation support on all new hardware (v3)
2021-03-10 21:50 ` [PATCH] i915: Drop relocation support on all new hardware (v3) Jason Ekstrand
2021-03-10 22:56 ` Jason Ekstrand
@ 2021-03-11 8:14 ` Lucas De Marchi
2021-03-11 10:20 ` Matthew Auld
2021-03-11 9:54 ` Tvrtko Ursulin
` (2 subsequent siblings)
4 siblings, 1 reply; 24+ messages in thread
From: Lucas De Marchi @ 2021-03-11 8:14 UTC (permalink / raw)
To: Jason Ekstrand
Cc: Dave Airlie, Intel Graphics, Matthew Auld, DRI, Daniel Vetter
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [PATCH] i915: Drop relocation support on all new hardware (v3)
2021-03-11 8:14 ` [Intel-gfx] " Lucas De Marchi
@ 2021-03-11 10:20 ` Matthew Auld
0 siblings, 0 replies; 24+ messages in thread
From: Matthew Auld @ 2021-03-11 10:20 UTC (permalink / raw)
To: Lucas De Marchi, Jason Ekstrand
Cc: Dave Airlie, Intel Graphics, DRI, Daniel Vetter
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)
>> {
>> 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.
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [PATCH] i915: Drop relocation support on all new hardware (v3)
2021-03-10 21:50 ` [PATCH] i915: Drop relocation support on all new hardware (v3) Jason Ekstrand
2021-03-10 22:56 ` Jason Ekstrand
2021-03-11 8:14 ` [Intel-gfx] " Lucas De Marchi
@ 2021-03-11 9:54 ` Tvrtko Ursulin
2021-03-11 11:44 ` Zbigniew Kempczyński
2021-03-11 16:26 ` [PATCH] drm/i915/gem: Drop relocation support on all new hardware (v4) Jason Ekstrand
4 siblings, 0 replies; 24+ messages in thread
From: Tvrtko Ursulin @ 2021-03-11 9:54 UTC (permalink / raw)
To: Jason Ekstrand, intel-gfx, dri-devel; +Cc: Dave Airlie, Daniel Vetter
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)
> {
> 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;
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]);
> 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 mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] i915: Drop relocation support on all new hardware (v3)
2021-03-10 21:50 ` [PATCH] i915: Drop relocation support on all new hardware (v3) Jason Ekstrand
` (2 preceding siblings ...)
2021-03-11 9:54 ` Tvrtko Ursulin
@ 2021-03-11 11:44 ` Zbigniew Kempczyński
2021-03-11 15:50 ` Jason Ekstrand
2021-03-11 16:31 ` Chris Wilson
2021-03-11 16:26 ` [PATCH] drm/i915/gem: Drop relocation support on all new hardware (v4) Jason Ekstrand
4 siblings, 2 replies; 24+ messages in thread
From: Zbigniew Kempczyński @ 2021-03-11 11:44 UTC (permalink / raw)
To: Jason Ekstrand; +Cc: Dave Airlie, intel-gfx, dri-devel, Daniel Vetter
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]);
> 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] i915: Drop relocation support on all new hardware (v3)
2021-03-11 11:44 ` Zbigniew Kempczyński
@ 2021-03-11 15:50 ` Jason Ekstrand
2021-03-11 15:57 ` Daniel Vetter
2021-03-11 16:31 ` Chris Wilson
1 sibling, 1 reply; 24+ messages in thread
From: Jason Ekstrand @ 2021-03-11 15:50 UTC (permalink / raw)
To: Zbigniew Kempczyński
Cc: Dave Airlie, Intel GFX, Maling list - DRI developers, Daniel Vetter
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] i915: Drop relocation support on all new hardware (v3)
2021-03-11 15:50 ` Jason Ekstrand
@ 2021-03-11 15:57 ` Daniel Vetter
2021-03-11 16:24 ` Jason Ekstrand
0 siblings, 1 reply; 24+ messages in thread
From: Daniel Vetter @ 2021-03-11 15:57 UTC (permalink / raw)
To: Jason Ekstrand
Cc: Dave Airlie, Zbigniew Kempczyński, Intel GFX,
Maling list - DRI developers, Daniel Vetter
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] i915: Drop relocation support on all new hardware (v3)
2021-03-11 15:57 ` Daniel Vetter
@ 2021-03-11 16:24 ` Jason Ekstrand
2021-03-11 16:50 ` Zbigniew Kempczyński
0 siblings, 1 reply; 24+ messages in thread
From: Jason Ekstrand @ 2021-03-11 16:24 UTC (permalink / raw)
To: Daniel Vetter
Cc: Dave Airlie, Zbigniew Kempczyński, Intel GFX,
Maling list - DRI developers, Daniel Vetter
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] i915: Drop relocation support on all new hardware (v3)
2021-03-11 16:24 ` Jason Ekstrand
@ 2021-03-11 16:50 ` Zbigniew Kempczyński
2021-03-11 17:18 ` Jason Ekstrand
0 siblings, 1 reply; 24+ messages in thread
From: Zbigniew Kempczyński @ 2021-03-11 16:50 UTC (permalink / raw)
To: Jason Ekstrand
Cc: Dave Airlie, Intel GFX, Maling list - DRI developers, Daniel Vetter
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] i915: Drop relocation support on all new hardware (v3)
2021-03-11 16:50 ` Zbigniew Kempczyński
@ 2021-03-11 17:18 ` Jason Ekstrand
2021-03-11 18:19 ` Zbigniew Kempczyński
0 siblings, 1 reply; 24+ messages in thread
From: Jason Ekstrand @ 2021-03-11 17:18 UTC (permalink / raw)
To: Zbigniew Kempczyński
Cc: Dave Airlie, Intel GFX, Maling list - DRI developers, Daniel Vetter
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] i915: Drop relocation support on all new hardware (v3)
2021-03-11 17:18 ` Jason Ekstrand
@ 2021-03-11 18:19 ` Zbigniew Kempczyński
2021-03-11 18:57 ` Jason Ekstrand
0 siblings, 1 reply; 24+ messages in thread
From: Zbigniew Kempczyński @ 2021-03-11 18:19 UTC (permalink / raw)
To: Jason Ekstrand
Cc: Dave Airlie, Intel GFX, Maling list - DRI developers, Daniel Vetter
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] i915: Drop relocation support on all new hardware (v3)
2021-03-11 18:19 ` Zbigniew Kempczyński
@ 2021-03-11 18:57 ` Jason Ekstrand
2021-03-12 14:16 ` Daniel Vetter
0 siblings, 1 reply; 24+ messages in thread
From: Jason Ekstrand @ 2021-03-11 18:57 UTC (permalink / raw)
To: Zbigniew Kempczyński
Cc: Dave Airlie, Intel GFX, Maling list - DRI developers, Daniel Vetter
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] i915: Drop relocation support on all new hardware (v3)
2021-03-11 18:57 ` Jason Ekstrand
@ 2021-03-12 14:16 ` Daniel Vetter
0 siblings, 0 replies; 24+ messages in thread
From: Daniel Vetter @ 2021-03-12 14:16 UTC (permalink / raw)
To: Jason Ekstrand
Cc: Intel GFX, Maling list - DRI developers,
Zbigniew Kempczyński, Daniel Vetter, Dave Airlie
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] i915: Drop relocation support on all new hardware (v3)
2021-03-11 11:44 ` Zbigniew Kempczyński
2021-03-11 15:50 ` Jason Ekstrand
@ 2021-03-11 16:31 ` Chris Wilson
2021-03-11 16:40 ` Jason Ekstrand
1 sibling, 1 reply; 24+ messages in thread
From: Chris Wilson @ 2021-03-11 16:31 UTC (permalink / raw)
To: Jason Ekstrand, Zbigniew Kempczyński
Cc: Dave Airlie, intel-gfx, dri-devel, Daniel Vetter
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] i915: Drop relocation support on all new hardware (v3)
2021-03-11 16:31 ` Chris Wilson
@ 2021-03-11 16:40 ` Jason Ekstrand
0 siblings, 0 replies; 24+ messages in thread
From: Jason Ekstrand @ 2021-03-11 16:40 UTC (permalink / raw)
To: Chris Wilson
Cc: Dave Airlie, Zbigniew Kempczyński, Intel GFX,
Maling list - DRI developers, Daniel Vetter
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] drm/i915/gem: Drop relocation support on all new hardware (v4)
2021-03-10 21:50 ` [PATCH] i915: Drop relocation support on all new hardware (v3) Jason Ekstrand
` (3 preceding siblings ...)
2021-03-11 11:44 ` Zbigniew Kempczyński
@ 2021-03-11 16:26 ` Jason Ekstrand
4 siblings, 0 replies; 24+ messages in thread
From: Jason Ekstrand @ 2021-03-11 16:26 UTC (permalink / raw)
To: dri-devel, mintel-gfx
Cc: Dave Airlie, Keith Packard, Jason Ekstrand, Daniel Vetter
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;
}
--
2.29.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread