Re: [PATCH v7] drm/i915: Add soft-pinning API for execbuffer

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

On 09/12/15 13:33, Michel Thierry wrote:
> On 12/9/2015 12:34 PM, Tvrtko Ursulin wrote:
>>
>> On 09/12/15 10:51, Chris Wilson wrote:
>>> On Wed, Dec 09, 2015 at 10:30:41AM +0000, Tvrtko Ursulin wrote:
>>>>
>>>> On 08/12/15 18:49, Michel Thierry wrote:
>>>>> On 12/8/2015 11:55 AM, Thomas Daniel wrote:
>>>>>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>>
>>>>>> Userspace can pass in an offset that it presumes the object is
>>>>>> located
>>>>>> at. The kernel will then do its utmost to fit the object into that
>>>>>> location. The assumption is that userspace is handling its own object
>>>>>> locations (for example along with full-ppgtt) and that the kernel
>>>>>> will
>>>>>> rarely have to make space for the user's requests.
>>>>>>
>>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>>
>>>>>> v2: Fixed incorrect eviction found by Michal Winiarski - fix
>>>>>> suggested
>>>>>> by Chris
>>>>>> Wilson.  Fixed incorrect error paths causing crash found by Michal
>>>>>> Winiarski.
>>>>>> (Not published externally)
>>>>>>
>>>>>> v3: Rebased because of trivial conflict in object_bind_to_vm.  Fixed
>>>>>> eviction
>>>>>> to allow eviction of soft-pinned objects when another soft-pinned
>>>>>> object used
>>>>>> by a subsequent execbuffer overlaps reported by Michal Winiarski.
>>>>>> (Not published externally)
>>>>>>
>>>>>> v4: Moved soft-pinned objects to the front of ordered_vmas so that
>>>>>> they are
>>>>>> pinned first after an address conflict happens to avoid repeated
>>>>>> conflicts in
>>>>>> rare cases (Suggested by Chris Wilson).  Expanded comment on
>>>>>> drm_i915_gem_exec_object2.offset to cover this new API.
>>>>>>
>>>>>> v5: Added I915_PARAM_HAS_EXEC_SOFTPIN parameter for detecting this
>>>>>> capability
>>>>>> (Kristian). Added check for multiple pinnings on eviction (Akash).
>>>>>> Made sure
>>>>>> buffers are not considered misplaced without the user specifying
>>>>>> EXEC_OBJECT_SUPPORTS_48B_ADDRESS.  User must assume responsibility
>>>>>> for
>>>>>> any
>>>>>> addressing workarounds.  Updated object2.offset field comment
>>>>>> again to
>>>>>> clarify
>>>>>> NO_RELOC case (Chris).  checkpatch cleanup.
>>>>>>
>>>>>> v6: Trivial rebase on latest drm-intel-nightly
>>>>>>
>>>>>> v7: Catch attempts to pin above the max virtual address size and
>>>>>> return
>>>>>> EINVAL (Tvrtko). Decouple EXEC_OBJECT_SUPPORTS_48B_ADDRESS and
>>>>>> EXEC_OBJECT_PINNED flags, user must pass both flags in any attempt
>>>>>> to pin
>>>>>> something at an offset above 4GB (Chris, Daniel Vetter).
>>>>>>
>>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>> Cc: Akash Goel <akash.goel@intel.com>
>>>>>> Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>>>>>> Cc: Michal Winiarski <michal.winiarski@intel.com>
>>>>>> Cc: Zou Nanhai <nanhai.zou@intel.com>
>>>>>> Cc: Kristian Høgsberg <hoegsberg@gmail.com>
>>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>>>>> Signed-off-by: Thomas Daniel <thomas.daniel@intel.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/i915/i915_dma.c            |  3 ++
>>>>>>   drivers/gpu/drm/i915/i915_drv.h            |  2 +
>>>>>>   drivers/gpu/drm/i915/i915_gem.c            | 64
>>>>>> ++++++++++++++++++++----------
>>>>>>   drivers/gpu/drm/i915/i915_gem_evict.c      | 39 ++++++++++++++++++
>>>>>>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 16 +++++++-
>>>>>>   include/uapi/drm/i915_drm.h                | 12 ++++--
>>>>>>   6 files changed, 111 insertions(+), 25 deletions(-)
>>>>>>
>>>>>
>>>>> Extra support from the other patch aside, v6 already had rb from Akash
>>>>> and this one,
>>>>> Reviewed-by: Michel Thierry <michel.thierry@intel.com>
>>>>
>>>> This patch was acked by the PDT so I merged it to
>>>> drm-intel-next-queued.
>>>
>>> Please revert immediately. We need to fix the ABI for canonical
>>> addressing before proceeding. Then please work on the better patch.
>>
>> Sounds like this is a valid comment, guys please check the thread with
>> subject "[PATCH v2] drm/i915: Avoid writing relocs with addresses in
>> non-canonical form".
>>
>> amd64 ABI mandates rules on virtual addresess -
>> https://en.wikipedia.org/wiki/X86-64#Canonical_form_addresses.
>>
>> Regards,
>>
>> Tvrtko
>
> And if the someone tries to use softpin with a virtual address in
> non-canonical form, reject with EINVAL?

I think so, since they are not valid userspace virtual addresses.

Regards,

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