From: Jason Ekstrand <jason@jlekstrand.net>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: "Dave Airlie" <airlied@redhat.com>,
"Zbigniew Kempczyński" <zbigniew.kempczynski@intel.com>,
"Intel GFX" <intel-gfx@lists.freedesktop.org>,
"Maling list - DRI developers" <dri-devel@lists.freedesktop.org>,
"Daniel Vetter" <daniel.vetter@intel.com>
Subject: Re: [PATCH] i915: Drop relocation support on all new hardware (v3)
Date: Thu, 11 Mar 2021 10:24:38 -0600 [thread overview]
Message-ID: <CAOFGe96bL1Wu2Qgie7QOhMDoY-3hFhDxhokCekyEHnAC8CgjnQ@mail.gmail.com> (raw)
In-Reply-To: <CAKMK7uHwYczVHD7SmHAJd9niP4+aanDvEhPviofjmQiEweK3CQ@mail.gmail.com>
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
next prev parent reply other threads:[~2021-03-11 16:24 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-07 15:36 [PATCH] RFC: i915: Drop relocation support on Gen12+ Jason Ekstrand
2020-05-07 15:44 ` Chris Wilson
2020-05-07 16:00 ` Jason Ekstrand
2020-05-07 18:27 ` Dave Airlie
2020-05-08 5:58 ` Joonas Lahtinen
2020-06-25 17:22 ` Dave Airlie
2021-03-10 21:26 ` [PATCH] i915: Drop relocation support on all new hardware Jason Ekstrand
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 10:20 ` Matthew Auld
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 15:57 ` Daniel Vetter
2021-03-11 16:24 ` Jason Ekstrand [this message]
2021-03-11 16:50 ` Zbigniew Kempczyński
2021-03-11 17:18 ` Jason Ekstrand
2021-03-11 18:19 ` Zbigniew Kempczyński
2021-03-11 18:57 ` Jason Ekstrand
2021-03-12 14:16 ` Daniel Vetter
2021-03-11 16:31 ` Chris Wilson
2021-03-11 16:40 ` Jason Ekstrand
2021-03-11 16:26 ` [PATCH] drm/i915/gem: Drop relocation support on all new hardware (v4) Jason Ekstrand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOFGe96bL1Wu2Qgie7QOhMDoY-3hFhDxhokCekyEHnAC8CgjnQ@mail.gmail.com \
--to=jason@jlekstrand.net \
--cc=airlied@redhat.com \
--cc=daniel.vetter@intel.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=zbigniew.kempczynski@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).