On Thu, Mar 11, 2021 at 5:44 AM Zbigniew Kempczyński <zbigniew.kempczynski@xxxxxxxxx> 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@xxxxxxxxxxxxxx> > > Cc: Dave Airlie <airlied@xxxxxxxxxx> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
