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

[Maarten Lankhorst]

Op 2021-03-12 om 11:56 schreef Matthew Auld:
> On Fri, 12 Mar 2021 at 09:50, Tvrtko Ursulin
> <tvrtko.ursulin at 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 at jlekstrand.net>
>>> Acked-by: Keith Packard <keithp at keithp.com>
>>> Cc: Dave Airlie <airlied at redhat.com>
>>> Cc: Daniel Vetter <daniel.vetter at 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.

Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>