Re: [Intel-gfx] [PATCH] drm/i915/gem: Drop relocation support on all new hardware (v4)

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 12/03/2021 11:33, Maarten Lankhorst wrote:
> Op 2021-03-12 om 11:56 schreef Matthew Auld:
>> On Fri, 12 Mar 2021 at 09:50, Tvrtko Ursulin
>> <tvrtko.ursulin@linux.intel.com> wrote:
>>>
>>> On 11/03/2021 18:17, 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.
>>>>
>>>> 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;
>>> I still recommend ENODEV as more inline with our established error
>>> codes. (Platform does not support vs dear userspace you messed up your
>>> flags, modes, whatever.)
>>>
>>>> +
>>>> +     /* All discrete memory platforms are Gen12 or above */
>>>> +     if (WARN_ON(HAS_LMEM(eb->i915)))
>>>> +             return -EINVAL;
>>> What was the conclusion on value of supporting fake lmem?
>> >From the previous thread, nothing is currently using it, we did have a
>> dedicated machine in CI but that has been gone for some months it
>> seems, so it might already be broken. Also its use was limited only to
>> the live selftests, which can't even hit this path. The plan was to
>> eventually remove it, since supporting both real and fake lmem in the
>> same tree is likely more effort than it's worth.
> 
> I think -EINVAL is fine, but not against -ENODEV either, up to author imo.

It's pretty well established in our code that ENODEV is used when API is 
used on a platform which does not support or implement it.

Arguably relocations are a grey area since they can be supported but we 
don't want to. So perhaps ENOTSUPP would also work. I just don't see yet 
another overload of EINVAL is the ideal choice.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx