diff mbox series

RFC: i915: Drop relocation support on Gen12+

Message ID 20200507153600.314454-1-jason@jlekstrand.net (mailing list archive)
State New, archived
Headers show
Series RFC: i915: Drop relocation support on Gen12+ | expand

Commit Message

Jason Ekstrand May 7, 2020, 3:36 p.m. UTC
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(-)

Comments

Chris Wilson May 7, 2020, 3:44 p.m. UTC | #1
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
Jason Ekstrand May 7, 2020, 4 p.m. UTC | #2
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
Dave Airlie May 7, 2020, 6:27 p.m. UTC | #3
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.
Joonas Lahtinen May 8, 2020, 5:58 a.m. UTC | #4
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
Dave Airlie June 25, 2020, 5:22 p.m. UTC | #5
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.
diff mbox series

Patch

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;