Re: [Intel-gfx] [RFC v2 02/12] drm/i915/svm: Runtime (RT) allocator support

From: Niranjan Vishwanathapura <niranjana.vishwanathapura@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Graunke, Kenneth W" <kenneth.w.graunke@intel.com>,
	Intel GFX <intel-gfx@lists.freedesktop.org>,
	sanjay.k.kumar@intel.com,
	Maling list - DRI developers <dri-devel@lists.freedesktop.org>,
	Jason Ekstrand <jason.ekstrand@intel.com>,
	dave.hansen@intel.com, jglisse@redhat.com, jgg@mellanox.com,
	Jason Ekstrand <jason@jlekstrand.net>,
	Daniel Vetter <daniel.vetter@intel.com>,
	dan.j.williams@intel.com, ira.weiny@intel.com
Subject: Re: [Intel-gfx] [RFC v2 02/12] drm/i915/svm: Runtime (RT) allocator support
Date: Sun, 15 Dec 2019 20:13:26 -0800	[thread overview]
Message-ID: <20191216041326.GT14488@nvishwa1-DESK.sc.intel.com> (raw)
In-Reply-To: <157631949753.7535.12359359451927943176@skylake-alporthouse-com>

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@intel.com> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel