On Sat, Dec 14, 2019 at 10:31:37AM +0000, Chris Wilson wrote:
Quoting Jason Ekstrand (2019-12-14 00:36:19)
On Fri, Dec 13, 2019 at 5:24 PM Niranjan Vishwanathapura <
niranjana.vishwanathapura@xxxxxxxxx> wrote:
On Fri, Dec 13, 2019 at 04:58:42PM -0600, Jason Ekstrand wrote:
>
> +/**
> + * struct drm_i915_gem_vm_bind
> + *
> + * Bind an object in a vm's page table.
>
> First off, this is something I've wanted for a while for Vulkan, it's
just
> never made its way high enough up the priority list. However, it's
going
> to have to come one way or another soon. I'm glad to see kernel API
for
> this being proposed.
> I do, however, have a few high-level comments/questions about the API:
> 1. In order to be useful for sparse memory support, the API has to go
the
> other way around so that it binds a VA range to a range within the BO.
It
> also needs to be able to handle overlapping where two different VA
ranges
> may map to the same underlying bytes in the BO. This likely means that
> unbind needs to also take a VA range and only unbind that range.
> 2. If this is going to be useful for managing GL's address space where
we
> have lots of BOs, we probably want it to take a list of ranges so we
> aren't making one ioctl for each thing we want to bind.
Hi Jason,
Yah, some of these requirements came up.
Yes, I have raised them every single time an API like this has come across my
e-mail inbox for years and they continue to get ignored. Why are we landing an
API that we know isn't the API we want especially when it's pretty obvious
roughly what the API we want is? It may be less time in the short term, but
long-term it means two ioctls and two implementations in i915, IGT tests for
both code paths, and code in all UMDs to call one or the other depending on
what kernel you're running on, and we have to maintain all that code going
forward forever. Sure, that's a price we pay today for a variety of things but
that's because they all seemed like the right thing at the time. Landing the
wrong API when we know it's the wrong API seems foolish.
Exactly. This is not even close to the uAPI we need. Reposting an RFC
without taking in the concerns last time (or the time before that, or
the time before that...) suggests that you aren't really requesting for
comments at all.
Thanks Jason for detailed exlanation.
Chris, all comments and guidance are much appreciated :)
I haven't looked in detail, but my concern is that implementing
partial object binding (offset, lenght) from vma down to [un]binding
in ppgtt might be a lot of work to include in this SVM patch series.
I believe we need the partial object binding in non-SVM scenario
as well?
Ok, let me change the interface as below.
struct drm_i915_gem_vm_bind_va
{
/** VA start to bind **/
__u64 start;
/** Offset in Object to bind for I915_GEM_VM_BIND_SVM_OBJ type **/
__u64 offset;
/** VA length to [un]bind **/
__u64 length;
/** Type of memory to [un]bind **/
__u32 type;
#define I915_GEM_VM_BIND_SVM_OBJ 0
#define I915_GEM_VM_BIND_SVM_BUFFER 1
/** Object handle to [un]bind for I915_GEM_VM_BIND_SVM_OBJ type **/
__u32 handle;
/** Flags **/
__u32 flags;
#define I915_GEM_VM_BIND_UNBIND (1 << 0)
#define I915_GEM_VM_BIND_READONLY (1 << 1)
}
struct drm_i915_gem_vm_bind {
/** vm to [un]bind **/
__u32 vm_id;
/** number of VAs to bind **/
__u32 num_vas;
/** Array of VAs to bind **/
struct drm_i915_gem_vm_bind_va *bind_vas;
/** User extensions **/
__u64 extensions;
};
When synchronization control is added as extension, it applies to all VAs in the array.
Does this looks good?
Niranjana
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
