For discrete, users of pin_map() needs to obey the same rules at the TTM backend, where we map system only objects as WB, and everything else as WC. The simplest for now is to just force the correct mapping type as per the new rules for discrete.
Suggested-by: Thomas Hellström thomas.hellstrom@linux.intel.com Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Ramalingam C ramalingam.c@intel.com --- drivers/gpu/drm/i915/gem/i915_gem_object.c | 34 ++++++++++++++++++++++ drivers/gpu/drm/i915/gem/i915_gem_object.h | 4 +++ drivers/gpu/drm/i915/gem/i915_gem_pages.c | 22 ++++++++++++-- 3 files changed, 58 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c index 547cc9dad90d..9da7b288b7ed 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -625,6 +625,40 @@ int i915_gem_object_migrate(struct drm_i915_gem_object *obj, return obj->ops->migrate(obj, mr); }
+/** + * i915_gem_object_placement_possible - Check whether the object can be + * placed at certain memory type + * @obj: Pointer to the object + * @type: The memory type to check + * + * Return: True if the object can be placed in @type. False otherwise. + */ +bool i915_gem_object_placement_possible(struct drm_i915_gem_object *obj, + enum intel_memory_type type) +{ + unsigned int i; + + if (!obj->mm.n_placements) { + switch (type) { + case INTEL_MEMORY_LOCAL: + return i915_gem_object_has_iomem(obj); + case INTEL_MEMORY_SYSTEM: + return i915_gem_object_has_pages(obj); + default: + /* Ignore stolen for now */ + GEM_BUG_ON(1); + return false; + } + } + + for (i = 0; i < obj->mm.n_placements; i++) { + if (obj->mm.placements[i]->type == type) + return true; + } + + return false; +} + void i915_gem_init__objects(struct drm_i915_private *i915) { INIT_WORK(&i915->mm.free_work, __i915_gem_free_work); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index d423d8cac4f2..8be4fadeee48 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -12,6 +12,7 @@ #include <drm/drm_device.h>
#include "display/intel_frontbuffer.h" +#include "intel_memory_region.h" #include "i915_gem_object_types.h" #include "i915_gem_gtt.h" #include "i915_gem_ww.h" @@ -607,6 +608,9 @@ bool i915_gem_object_can_migrate(struct drm_i915_gem_object *obj, int i915_gem_object_wait_migration(struct drm_i915_gem_object *obj, unsigned int flags);
+bool i915_gem_object_placement_possible(struct drm_i915_gem_object *obj, + enum intel_memory_type type); + #ifdef CONFIG_MMU_NOTIFIER static inline bool i915_gem_object_is_userptr(struct drm_i915_gem_object *obj) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c index f2f850e31b8e..810a157a18f8 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c @@ -321,8 +321,7 @@ static void *i915_gem_object_map_pfn(struct drm_i915_gem_object *obj, dma_addr_t addr; void *vaddr;
- if (type != I915_MAP_WC) - return ERR_PTR(-ENODEV); + GEM_BUG_ON(type != I915_MAP_WC);
if (n_pfn > ARRAY_SIZE(stack)) { /* Too big for stack -- allocate temporary array instead */ @@ -374,6 +373,25 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj, } GEM_BUG_ON(!i915_gem_object_has_pages(obj));
+ /* + * For discrete our CPU mappings needs to be consistent in order to + * function correctly on !x86. When mapping things through TTM, we use + * the same rules to determine the caching type. + * + * Internal users of lmem are already expected to get this right, so no + * fudging needed there. + */ + if (i915_gem_object_placement_possible(obj, INTEL_MEMORY_LOCAL)) { + if (type != I915_MAP_WC && !obj->mm.n_placements) { + ptr = ERR_PTR(-ENODEV); + goto err_unpin; + } + + type = I915_MAP_WC; + } else if (IS_DGFX(to_i915(obj->base.dev))) { + type = I915_MAP_WB; + } + ptr = page_unpack_bits(obj->mm.mapping, &has_type); if (ptr && has_type != type) { if (pinned) {
It's a noop on DG1, and in the future when need to support other devices which let us control the coherency, then it should be an immutable creation time property for the BO. This will likely be controlled through a new gem_create_ext extension.
Suggested-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Jordan Justen jordan.l.justen@intel.com Cc: Kenneth Graunke kenneth@whitecape.org Cc: Jason Ekstrand jason@jlekstrand.net Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Ramalingam C ramalingam.c@intel.com --- drivers/gpu/drm/i915/gem/i915_gem_domain.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c index 7d1400b13429..43004bef55cb 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c @@ -268,6 +268,9 @@ int i915_gem_get_caching_ioctl(struct drm_device *dev, void *data, struct drm_i915_gem_object *obj; int err = 0;
+ if (IS_DGFX(to_i915(dev))) + return -ENODEV; + rcu_read_lock(); obj = i915_gem_object_lookup_rcu(file, args->handle); if (!obj) { @@ -303,6 +306,9 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data, enum i915_cache_level level; int ret = 0;
+ if (IS_DGFX(i915)) + return -ENODEV; + switch (args->caching) { case I915_CACHING_NONE: level = I915_CACHE_NONE;
The CPU domain should be static for discrete, and on DG1 we don't need any flushing since everything is already coherent, so really all this does is an object wait, for which we have an ioctl. Longer term the desired caching should be an immutable creation time property for the BO, which can be set with something like gem_create_ext.
One other user is iris + userptr, which uses the set_domain to probe all the pages to check if the GUP succeeds, however keeping the set_domain around just for that seems rather scuffed. We could equally just submit a dummy batch, which should hopefully be good enough, otherwise adding a new creation time flag for userptr might be an option. Although longer term we will also have vm_bind, which should also be a nice fit for this, so adding a whole new flag is likely overkill.
Suggested-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Jordan Justen jordan.l.justen@intel.com Cc: Kenneth Graunke kenneth@whitecape.org Cc: Jason Ekstrand jason@jlekstrand.net Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Ramalingam C ramalingam.c@intel.com --- drivers/gpu/drm/i915/gem/i915_gem_domain.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c index 43004bef55cb..b684a62bf3b0 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c @@ -490,6 +490,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, u32 write_domain = args->write_domain; int err;
+ if (IS_DGFX(to_i915(dev))) + return -ENODEV; + /* Only handle setting domains to types used by the CPU. */ if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS) return -EINVAL;
On Thu, 1 Jul 2021 at 16:10, Matthew Auld matthew.auld@intel.com wrote:
The CPU domain should be static for discrete, and on DG1 we don't need any flushing since everything is already coherent, so really all this does is an object wait, for which we have an ioctl. Longer term the desired caching should be an immutable creation time property for the BO, which can be set with something like gem_create_ext.
One other user is iris + userptr, which uses the set_domain to probe all the pages to check if the GUP succeeds, however keeping the set_domain around just for that seems rather scuffed. We could equally just submit a dummy batch, which should hopefully be good enough, otherwise adding a new creation time flag for userptr might be an option. Although longer term we will also have vm_bind, which should also be a nice fit for this, so adding a whole new flag is likely overkill.
Kenneth, do you have a preference for the iris + userptr use case? Adding the flag shouldn't be much work, if you feel the dummy batch is too ugly. I don't mind either way.
Suggested-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Jordan Justen jordan.l.justen@intel.com Cc: Kenneth Graunke kenneth@whitecape.org Cc: Jason Ekstrand jason@jlekstrand.net Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Ramalingam C ramalingam.c@intel.com
drivers/gpu/drm/i915/gem/i915_gem_domain.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c index 43004bef55cb..b684a62bf3b0 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c @@ -490,6 +490,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, u32 write_domain = args->write_domain; int err;
if (IS_DGFX(to_i915(dev)))return -ENODEV;/* Only handle setting domains to types used by the CPU. */ if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS) return -EINVAL;-- 2.26.3
On 01/07/2021 16:10, Matthew Auld wrote:
The CPU domain should be static for discrete, and on DG1 we don't need any flushing since everything is already coherent, so really all this
Knowledge of the write combine buffer is assumed to be had by anyone involved?
does is an object wait, for which we have an ioctl. Longer term the desired caching should be an immutable creation time property for the BO, which can be set with something like gem_create_ext.
One other user is iris + userptr, which uses the set_domain to probe all the pages to check if the GUP succeeds, however keeping the set_domain around just for that seems rather scuffed. We could equally just submit a dummy batch, which should hopefully be good enough, otherwise adding a new creation time flag for userptr might be an option. Although longer term we will also have vm_bind, which should also be a nice fit for this, so adding a whole new flag is likely overkill.
Execbuf sounds horrible. But it all reminds me of past work by Chris which is surprisingly hard to find in the archives. Patches like:
commit 7706a433388016983052a27c0fd74a64b1897ae7 Author: Chris Wilson chris@chris-wilson.co.uk Date: Wed Nov 8 17:04:07 2017 +0000
drm/i915/userptr: Probe existence of backing struct pages upon creation
Jason Ekstrand requested a more efficient method than userptr+set-domain to determine if the userptr object was backed by a complete set of pages upon creation. To be more efficient than simply populating the userptr using get_user_pages() (as done by the call to set-domain or execbuf), we can walk the tree of vm_area_struct and check for gaps or vma not backed by struct page (VM_PFNMAP). The question is how to handle VM_MIXEDMAP which may be either struct page or pfn backed...
commit 7ca21d3390eec23db99b8131ed18bc036efaba18 Author: Chris Wilson chris@chris-wilson.co.uk Date: Wed Nov 8 17:48:22 2017 +0000
drm/i915/userptr: Add a flag to populate the userptr on creation
Acquiring the backing struct pages for the userptr range is not free; the first client for userptr would insist on frequently creating userptr objects ahead of time and not use them. For that first client, deferring the cost of populating the userptr (calling get_user_pages()) to the actual execbuf was a substantial improvement. However, not all clients are the same, and most would like to validate that the userptr is valid and backed by struct pages upon creation, so offer a I915_USERPTR_POPULATE flag to do just that.
Note that big difference between I915_USERPTR_POPULATE and the deferred scheme is that POPULATE is guaranteed to be synchronous, the result is known before the ioctl returns (and the handle exposed). However, due to system memory pressure, the object may be paged out before use, requiring them to be paged back in on execbuf (as may always happen).
At least with the first one I think I was skeptical, since probing at point A makes a weak test versus userptr getting used at point B. Populate is kind of same really when user controls the backing store. At least these two arguments I think stand if we are trying to sell these flags as validation. But if the idea is limited to pure preload, with no guarantees that it keeps working by time of real use, then I guess it may be passable.
Disclaimer that I haven't been following the story on why it is desirable to abandon set domain. Only judging from this series, mmap caching mode is implied from the object? Should set domain availability be driven by the object backing store instead of outright rejection?
Regards,
Tvrtko
Suggested-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Jordan Justen jordan.l.justen@intel.com Cc: Kenneth Graunke kenneth@whitecape.org Cc: Jason Ekstrand jason@jlekstrand.net Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Ramalingam C ramalingam.c@intel.com
drivers/gpu/drm/i915/gem/i915_gem_domain.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c index 43004bef55cb..b684a62bf3b0 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c @@ -490,6 +490,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, u32 write_domain = args->write_domain; int err;
- if (IS_DGFX(to_i915(dev)))
return -ENODEV;- /* Only handle setting domains to types used by the CPU. */ if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS) return -EINVAL;
On Fri, Jul 02, 2021 at 03:31:08PM +0100, Tvrtko Ursulin wrote:
On 01/07/2021 16:10, Matthew Auld wrote:
The CPU domain should be static for discrete, and on DG1 we don't need any flushing since everything is already coherent, so really all this
Knowledge of the write combine buffer is assumed to be had by anyone involved?
does is an object wait, for which we have an ioctl. Longer term the desired caching should be an immutable creation time property for the BO, which can be set with something like gem_create_ext.
One other user is iris + userptr, which uses the set_domain to probe all the pages to check if the GUP succeeds, however keeping the set_domain around just for that seems rather scuffed. We could equally just submit a dummy batch, which should hopefully be good enough, otherwise adding a new creation time flag for userptr might be an option. Although longer term we will also have vm_bind, which should also be a nice fit for this, so adding a whole new flag is likely overkill.
Execbuf sounds horrible. But it all reminds me of past work by Chris which is surprisingly hard to find in the archives. Patches like:
commit 7706a433388016983052a27c0fd74a64b1897ae7 Author: Chris Wilson chris@chris-wilson.co.uk Date: Wed Nov 8 17:04:07 2017 +0000
drm/i915/userptr: Probe existence of backing struct pages upon creation Jason Ekstrand requested a more efficient method than userptr+set-domain to determine if the userptr object was backed by a complete set of pages upon creation. To be more efficient than simply populating the userptr using get_user_pages() (as done by the call to set-domain or execbuf), we can walk the tree of vm_area_struct and check for gaps or vma not backed by struct page (VM_PFNMAP). The question is how to handle VM_MIXEDMAP which may be either struct page or pfn backed...commit 7ca21d3390eec23db99b8131ed18bc036efaba18 Author: Chris Wilson chris@chris-wilson.co.uk Date: Wed Nov 8 17:48:22 2017 +0000
drm/i915/userptr: Add a flag to populate the userptr on creation Acquiring the backing struct pages for the userptr range is not free; the first client for userptr would insist on frequently creating userptr objects ahead of time and not use them. For that first client, deferring the cost of populating the userptr (calling get_user_pages()) to the actual execbuf was a substantial improvement. However, not all clients are the same, and most would like to validate that the userptr is valid and backed by struct pages upon creation, so offer a I915_USERPTR_POPULATE flag to do just that. Note that big difference between I915_USERPTR_POPULATE and the deferred scheme is that POPULATE is guaranteed to be synchronous, the result is known before the ioctl returns (and the handle exposed). However, due to system memory pressure, the object may be paged out before use, requiring them to be paged back in on execbuf (as may always happen).At least with the first one I think I was skeptical, since probing at point A makes a weak test versus userptr getting used at point B. Populate is kind of same really when user controls the backing store. At least these two arguments I think stand if we are trying to sell these flags as validation. But if the idea is limited to pure preload, with no guarantees that it keeps working by time of real use, then I guess it may be passable.
Well we've thrown this out again because there was no userspace. But if this is requested by mesa, then the _PROBE flag should be entirely sufficient.
Since I don't want to hold up dg1 pciids on this it'd be nice if we could just go ahead with the dummy batch, if Ken/Jordan don't object - iris is the only umd that needs this.
Disclaimer that I haven't been following the story on why it is desirable to abandon set domain. Only judging from this series, mmap caching mode is implied from the object? Should set domain availability be driven by the object backing store instead of outright rejection?
In theory yes.
In practice umd have allowed and all the api are now allocating objects with static properties, and the only reason we ever call set_domain is due to slightly outdated buffer caching schemes dating back to og libdrm from 12+ years ago.
The other practical reason is that clflush is simply the slowest way to upload data of all the ones we have :-)
So even when this comes back I don't expect this ioctl will come back.
Regards,
Tvrtko
Suggested-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Jordan Justen jordan.l.justen@intel.com Cc: Kenneth Graunke kenneth@whitecape.org Cc: Jason Ekstrand jason@jlekstrand.net Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Ramalingam C ramalingam.c@intel.com
drivers/gpu/drm/i915/gem/i915_gem_domain.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c index 43004bef55cb..b684a62bf3b0 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c @@ -490,6 +490,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, u32 write_domain = args->write_domain; int err;
- if (IS_DGFX(to_i915(dev)))
return -ENODEV;- /* Only handle setting domains to types used by the CPU. */ if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS) return -EINVAL;
On 02/07/2021 20:22, Daniel Vetter wrote:
On Fri, Jul 02, 2021 at 03:31:08PM +0100, Tvrtko Ursulin wrote:
On 01/07/2021 16:10, Matthew Auld wrote:
The CPU domain should be static for discrete, and on DG1 we don't need any flushing since everything is already coherent, so really all this
Knowledge of the write combine buffer is assumed to be had by anyone involved?
What about this question? For discrete userspace will assume WC and will know how to flush WC buffer? Or it is assumed the flush will be hit implicitly? Will this be documented?
does is an object wait, for which we have an ioctl. Longer term the desired caching should be an immutable creation time property for the BO, which can be set with something like gem_create_ext.
One other user is iris + userptr, which uses the set_domain to probe all the pages to check if the GUP succeeds, however keeping the set_domain around just for that seems rather scuffed. We could equally just submit a dummy batch, which should hopefully be good enough, otherwise adding a new creation time flag for userptr might be an option. Although longer term we will also have vm_bind, which should also be a nice fit for this, so adding a whole new flag is likely overkill.
Execbuf sounds horrible. But it all reminds me of past work by Chris which is surprisingly hard to find in the archives. Patches like:
commit 7706a433388016983052a27c0fd74a64b1897ae7 Author: Chris Wilson chris@chris-wilson.co.uk Date: Wed Nov 8 17:04:07 2017 +0000
drm/i915/userptr: Probe existence of backing struct pages upon creation Jason Ekstrand requested a more efficient method than userptr+set-domain to determine if the userptr object was backed by a complete set of pages upon creation. To be more efficient than simply populating the userptr using get_user_pages() (as done by the call to set-domain or execbuf), we can walk the tree of vm_area_struct and check for gaps or vma not backed by struct page (VM_PFNMAP). The question is how to handle VM_MIXEDMAP which may be either struct page or pfn backed...commit 7ca21d3390eec23db99b8131ed18bc036efaba18 Author: Chris Wilson chris@chris-wilson.co.uk Date: Wed Nov 8 17:48:22 2017 +0000
drm/i915/userptr: Add a flag to populate the userptr on creation Acquiring the backing struct pages for the userptr range is not free; the first client for userptr would insist on frequently creating userptr objects ahead of time and not use them. For that first client, deferring the cost of populating the userptr (calling get_user_pages()) to the actual execbuf was a substantial improvement. However, not all clients are the same, and most would like to validate that the userptr is valid and backed by struct pages upon creation, so offer a I915_USERPTR_POPULATE flag to do just that. Note that big difference between I915_USERPTR_POPULATE and the deferred scheme is that POPULATE is guaranteed to be synchronous, the result is known before the ioctl returns (and the handle exposed). However, due to system memory pressure, the object may be paged out before use, requiring them to be paged back in on execbuf (as may always happen).At least with the first one I think I was skeptical, since probing at point A makes a weak test versus userptr getting used at point B. Populate is kind of same really when user controls the backing store. At least these two arguments I think stand if we are trying to sell these flags as validation. But if the idea is limited to pure preload, with no guarantees that it keeps working by time of real use, then I guess it may be passable.
Well we've thrown this out again because there was no userspace. But if this is requested by mesa, then the _PROBE flag should be entirely sufficient.
Why probe and not populate? For me probe is weak and implies to give a guarantee which cannot really be given. If the pointer is not trusted, there is no reason to think it cannot go bad between creating the buffer (probe) and actual use. Populate on the other hand could be described as simply instantiate the backing store with the same caveat mentioned. No guarantees about the future validity of the backing store in either case should be implied.
Since I don't want to hold up dg1 pciids on this it'd be nice if we could just go ahead with the dummy batch, if Ken/Jordan don't object - iris is the only umd that needs this.
I am not up to speed to understand how to PCI ids come into play here, but what is the suggestion with the dummy batch - to actually submit something which ends up executing, waking up the GPU etc? Or be crafty and make it fail after it acquires backing store? Not sure if we have such a spot that late so just asking to start with. If the plan is to wake up the GPU that's quite ugly in my opinion. Especially since patch which adds the flag already exists so shouldn't really be much a delay to sync userspace and i915 merge.
Disclaimer that I haven't been following the story on why it is desirable to abandon set domain. Only judging from this series, mmap caching mode is implied from the object? Should set domain availability be driven by the object backing store instead of outright rejection?
In theory yes.
In practice umd have allowed and all the api are now allocating objects with static properties, and the only reason we ever call set_domain is due to slightly outdated buffer caching schemes dating back to og libdrm from 12+ years ago.
I didn't get what the UMDs have allowed?
Regards,
Tvrtko
The other practical reason is that clflush is simply the slowest way to upload data of all the ones we have :-)
So even when this comes back I don't expect this ioctl will come back.
Regards,
Tvrtko
Suggested-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Jordan Justen jordan.l.justen@intel.com Cc: Kenneth Graunke kenneth@whitecape.org Cc: Jason Ekstrand jason@jlekstrand.net Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Ramalingam C ramalingam.c@intel.com
drivers/gpu/drm/i915/gem/i915_gem_domain.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c index 43004bef55cb..b684a62bf3b0 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c @@ -490,6 +490,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, u32 write_domain = args->write_domain; int err;
- if (IS_DGFX(to_i915(dev)))
return -ENODEV;- /* Only handle setting domains to types used by the CPU. */ if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS) return -EINVAL;
On Mon, Jul 05, 2021 at 09:34:22AM +0100, Tvrtko Ursulin wrote:
On 02/07/2021 20:22, Daniel Vetter wrote:
On Fri, Jul 02, 2021 at 03:31:08PM +0100, Tvrtko Ursulin wrote:
On 01/07/2021 16:10, Matthew Auld wrote:
The CPU domain should be static for discrete, and on DG1 we don't need any flushing since everything is already coherent, so really all this
Knowledge of the write combine buffer is assumed to be had by anyone involved?
What about this question? For discrete userspace will assume WC and will know how to flush WC buffer? Or it is assumed the flush will be hit implicitly? Will this be documented?
The kernel doesn't pick something at random, it's just fixed. So yeah userspace needs to flush the WC buffer or anything else.
does is an object wait, for which we have an ioctl. Longer term the desired caching should be an immutable creation time property for the BO, which can be set with something like gem_create_ext.
One other user is iris + userptr, which uses the set_domain to probe all the pages to check if the GUP succeeds, however keeping the set_domain around just for that seems rather scuffed. We could equally just submit a dummy batch, which should hopefully be good enough, otherwise adding a new creation time flag for userptr might be an option. Although longer term we will also have vm_bind, which should also be a nice fit for this, so adding a whole new flag is likely overkill.
Execbuf sounds horrible. But it all reminds me of past work by Chris which is surprisingly hard to find in the archives. Patches like:
commit 7706a433388016983052a27c0fd74a64b1897ae7 Author: Chris Wilson chris@chris-wilson.co.uk Date: Wed Nov 8 17:04:07 2017 +0000
drm/i915/userptr: Probe existence of backing struct pages upon creation Jason Ekstrand requested a more efficient method than userptr+set-domain to determine if the userptr object was backed by a complete set of pages upon creation. To be more efficient than simply populating the userptr using get_user_pages() (as done by the call to set-domain or execbuf), we can walk the tree of vm_area_struct and check for gaps or vma not backed by struct page (VM_PFNMAP). The question is how to handle VM_MIXEDMAP which may be either struct page or pfn backed...commit 7ca21d3390eec23db99b8131ed18bc036efaba18 Author: Chris Wilson chris@chris-wilson.co.uk Date: Wed Nov 8 17:48:22 2017 +0000
drm/i915/userptr: Add a flag to populate the userptr on creation Acquiring the backing struct pages for the userptr range is not free; the first client for userptr would insist on frequently creating userptr objects ahead of time and not use them. For that first client, deferring the cost of populating the userptr (calling get_user_pages()) to the actual execbuf was a substantial improvement. However, not all clients are the same, and most would like to validate that the userptr is valid and backed by struct pages upon creation, so offer a I915_USERPTR_POPULATE flag to do just that. Note that big difference between I915_USERPTR_POPULATE and the deferred scheme is that POPULATE is guaranteed to be synchronous, the result is known before the ioctl returns (and the handle exposed). However, due to system memory pressure, the object may be paged out before use, requiring them to be paged back in on execbuf (as may always happen).At least with the first one I think I was skeptical, since probing at point A makes a weak test versus userptr getting used at point B. Populate is kind of same really when user controls the backing store. At least these two arguments I think stand if we are trying to sell these flags as validation. But if the idea is limited to pure preload, with no guarantees that it keeps working by time of real use, then I guess it may be passable.
Well we've thrown this out again because there was no userspace. But if this is requested by mesa, then the _PROBE flag should be entirely sufficient.
Why probe and not populate? For me probe is weak and implies to give a guarantee which cannot really be given. If the pointer is not trusted, there is no reason to think it cannot go bad between creating the buffer (probe) and actual use. Populate on the other hand could be described as simply instantiate the backing store with the same caveat mentioned. No guarantees about the future validity of the backing store in either case should be implied.
The pointer can also go bad with populate. The only thing probe guarantees is that "right now I should be able to call get_user_pages and the only reasons it could fail is ENOMEM". Which is pretty much the same as we guarantee when we create a normal object.
Neither does guarantee that by the time you execbuf you won't hit an ENOMEM. Userptr on top also could make the pointer go invalid if userspace munmaps or does something else funny.
Since I don't want to hold up dg1 pciids on this it'd be nice if we could just go ahead with the dummy batch, if Ken/Jordan don't object - iris is the only umd that needs this.
I am not up to speed to understand how to PCI ids come into play here, but what is the suggestion with the dummy batch - to actually submit something which ends up executing, waking up the GPU etc? Or be crafty and make it fail after it acquires backing store? Not sure if we have such a spot that late so just asking to start with. If the plan is to wake up the GPU that's quite ugly in my opinion. Especially since patch which adds the flag already exists so shouldn't really be much a delay to sync userspace and i915 merge.
Just submit a real batch with just MI_BATCHBUFFER_END in it.
Disclaimer that I haven't been following the story on why it is desirable to abandon set domain. Only judging from this series, mmap caching mode is implied from the object? Should set domain availability be driven by the object backing store instead of outright rejection?
In theory yes.
In practice umd have allowed and all the api are now allocating objects with static properties, and the only reason we ever call set_domain is due to slightly outdated buffer caching schemes dating back to og libdrm from 12+ years ago.
I didn't get what the UMDs have allowed?
There's no umd need anymore to make everything mutable. API design has enormously changed over the past 10+ years, and even gl looks internally a lot like something modern thanks to gallium (or the fancy new glonvk thing). -Daniel
Regards,
Tvrtko
The other practical reason is that clflush is simply the slowest way to upload data of all the ones we have :-)
So even when this comes back I don't expect this ioctl will come back.
Regards,
Tvrtko
Suggested-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Jordan Justen jordan.l.justen@intel.com Cc: Kenneth Graunke kenneth@whitecape.org Cc: Jason Ekstrand jason@jlekstrand.net Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Ramalingam C ramalingam.c@intel.com
drivers/gpu/drm/i915/gem/i915_gem_domain.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c index 43004bef55cb..b684a62bf3b0 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c @@ -490,6 +490,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, u32 write_domain = args->write_domain; int err;
- if (IS_DGFX(to_i915(dev)))
return -ENODEV;- /* Only handle setting domains to types used by the CPU. */ if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS) return -EINVAL;
On 05/07/2021 15:25, Daniel Vetter wrote:
On Mon, Jul 05, 2021 at 09:34:22AM +0100, Tvrtko Ursulin wrote:
On 02/07/2021 20:22, Daniel Vetter wrote:
On Fri, Jul 02, 2021 at 03:31:08PM +0100, Tvrtko Ursulin wrote:
On 01/07/2021 16:10, Matthew Auld wrote:
The CPU domain should be static for discrete, and on DG1 we don't need any flushing since everything is already coherent, so really all this
Knowledge of the write combine buffer is assumed to be had by anyone involved?
What about this question? For discrete userspace will assume WC and will know how to flush WC buffer? Or it is assumed the flush will be hit implicitly? Will this be documented?
The kernel doesn't pick something at random, it's just fixed. So yeah userspace needs to flush the WC buffer or anything else.
Right, so does that needs to be documented somewhere or thinking is it is common knowledge? Probably does to be mentioned in conjunction with the mmap usage.
does is an object wait, for which we have an ioctl. Longer term the desired caching should be an immutable creation time property for the BO, which can be set with something like gem_create_ext.
One other user is iris + userptr, which uses the set_domain to probe all the pages to check if the GUP succeeds, however keeping the set_domain around just for that seems rather scuffed. We could equally just submit a dummy batch, which should hopefully be good enough, otherwise adding a new creation time flag for userptr might be an option. Although longer term we will also have vm_bind, which should also be a nice fit for this, so adding a whole new flag is likely overkill.
Execbuf sounds horrible. But it all reminds me of past work by Chris which is surprisingly hard to find in the archives. Patches like:
commit 7706a433388016983052a27c0fd74a64b1897ae7 Author: Chris Wilson chris@chris-wilson.co.uk Date: Wed Nov 8 17:04:07 2017 +0000
drm/i915/userptr: Probe existence of backing struct pages upon creation Jason Ekstrand requested a more efficient method than userptr+set-domain to determine if the userptr object was backed by a complete set of pages upon creation. To be more efficient than simply populating the userptr using get_user_pages() (as done by the call to set-domain or execbuf), we can walk the tree of vm_area_struct and check for gaps or vma not backed by struct page (VM_PFNMAP). The question is how to handle VM_MIXEDMAP which may be either struct page or pfn backed...commit 7ca21d3390eec23db99b8131ed18bc036efaba18 Author: Chris Wilson chris@chris-wilson.co.uk Date: Wed Nov 8 17:48:22 2017 +0000
drm/i915/userptr: Add a flag to populate the userptr on creation Acquiring the backing struct pages for the userptr range is not free; the first client for userptr would insist on frequently creating userptr objects ahead of time and not use them. For that first client, deferring the cost of populating the userptr (calling get_user_pages()) to the actual execbuf was a substantial improvement. However, not all clients are the same, and most would like to validate that the userptr is valid and backed by struct pages upon creation, so offer a I915_USERPTR_POPULATE flag to do just that. Note that big difference between I915_USERPTR_POPULATE and the deferred scheme is that POPULATE is guaranteed to be synchronous, the result is known before the ioctl returns (and the handle exposed). However, due to system memory pressure, the object may be paged out before use, requiring them to be paged back in on execbuf (as may always happen).At least with the first one I think I was skeptical, since probing at point A makes a weak test versus userptr getting used at point B. Populate is kind of same really when user controls the backing store. At least these two arguments I think stand if we are trying to sell these flags as validation. But if the idea is limited to pure preload, with no guarantees that it keeps working by time of real use, then I guess it may be passable.
Well we've thrown this out again because there was no userspace. But if this is requested by mesa, then the _PROBE flag should be entirely sufficient.
Why probe and not populate? For me probe is weak and implies to give a guarantee which cannot really be given. If the pointer is not trusted, there is no reason to think it cannot go bad between creating the buffer (probe) and actual use. Populate on the other hand could be described as simply instantiate the backing store with the same caveat mentioned. No guarantees about the future validity of the backing store in either case should be implied.
The pointer can also go bad with populate. The only thing probe guarantees is that "right now I should be able to call get_user_pages and the only reasons it could fail is ENOMEM". Which is pretty much the same as we guarantee when we create a normal object.
Neither does guarantee that by the time you execbuf you won't hit an ENOMEM. Userptr on top also could make the pointer go invalid if userspace munmaps or does something else funny.
That's pretty much what I wrote so mostly agreed. Modulo that I think probe guarantees even less than what you wrote above. Due mmu notifier invalidation definitely less than with the normal buffer objects (hypothetically, if there was a populate flag for them). So the only think which I think makes sense is a populate flag with a disclaimer explaining how the backing store may not be there any more as soon as the ioctl successfully returns.
Since I don't want to hold up dg1 pciids on this it'd be nice if we could just go ahead with the dummy batch, if Ken/Jordan don't object - iris is the only umd that needs this.
I am not up to speed to understand how to PCI ids come into play here, but what is the suggestion with the dummy batch - to actually submit something which ends up executing, waking up the GPU etc? Or be crafty and make it fail after it acquires backing store? Not sure if we have such a spot that late so just asking to start with. If the plan is to wake up the GPU that's quite ugly in my opinion. Especially since patch which adds the flag already exists so shouldn't really be much a delay to sync userspace and i915 merge.
Just submit a real batch with just MI_BATCHBUFFER_END in it.
Okay, so my opinion stands for this to being quite wasteful and flag should be preferable.
Regards,
Tvrtko
On Friday, July 2, 2021 12:22:58 PM PDT Daniel Vetter wrote:
On Fri, Jul 02, 2021 at 03:31:08PM +0100, Tvrtko Ursulin wrote:
On 01/07/2021 16:10, Matthew Auld wrote:
The CPU domain should be static for discrete, and on DG1 we don't need any flushing since everything is already coherent, so really all this
Knowledge of the write combine buffer is assumed to be had by anyone involved?
does is an object wait, for which we have an ioctl. Longer term the desired caching should be an immutable creation time property for the BO, which can be set with something like gem_create_ext.
One other user is iris + userptr, which uses the set_domain to probe all the pages to check if the GUP succeeds, however keeping the set_domain around just for that seems rather scuffed. We could equally just submit a dummy batch, which should hopefully be good enough, otherwise adding a new creation time flag for userptr might be an option. Although longer term we will also have vm_bind, which should also be a nice fit for this, so adding a whole new flag is likely overkill.
Execbuf sounds horrible. But it all reminds me of past work by Chris which is surprisingly hard to find in the archives. Patches like:
commit 7706a433388016983052a27c0fd74a64b1897ae7 Author: Chris Wilson chris@chris-wilson.co.uk Date: Wed Nov 8 17:04:07 2017 +0000
drm/i915/userptr: Probe existence of backing struct pages upon creation Jason Ekstrand requested a more efficient method than userptr+set-domain to determine if the userptr object was backed by a complete set of pages upon creation. To be more efficient than simply populating the userptr using get_user_pages() (as done by the call to set-domain or execbuf), we can walk the tree of vm_area_struct and check for gaps or vma not backed by struct page (VM_PFNMAP). The question is how to handle VM_MIXEDMAP which may be either struct page or pfn backed...commit 7ca21d3390eec23db99b8131ed18bc036efaba18 Author: Chris Wilson chris@chris-wilson.co.uk Date: Wed Nov 8 17:48:22 2017 +0000
drm/i915/userptr: Add a flag to populate the userptr on creation Acquiring the backing struct pages for the userptr range is not free; the first client for userptr would insist on frequently creating userptr objects ahead of time and not use them. For that first client, deferring the cost of populating the userptr (calling get_user_pages()) to the actual execbuf was a substantial improvement. However, not all clients are the same, and most would like to validate that the userptr is valid and backed by struct pages upon creation, so offer a I915_USERPTR_POPULATE flag to do just that. Note that big difference between I915_USERPTR_POPULATE and the deferred scheme is that POPULATE is guaranteed to be synchronous, the result is known before the ioctl returns (and the handle exposed). However, due to system memory pressure, the object may be paged out before use, requiring them to be paged back in on execbuf (as may always happen).At least with the first one I think I was skeptical, since probing at point A makes a weak test versus userptr getting used at point B. Populate is kind of same really when user controls the backing store. At least these two arguments I think stand if we are trying to sell these flags as validation. But if the idea is limited to pure preload, with no guarantees that it keeps working by time of real use, then I guess it may be passable.
Well we've thrown this out again because there was no userspace. But if this is requested by mesa, then the _PROBE flag should be entirely sufficient.
Since I don't want to hold up dg1 pciids on this it'd be nice if we could just go ahead with the dummy batch, if Ken/Jordan don't object - iris is the only umd that needs this.
I really would rather not have to submit a dummy batchbuffer.
The GL_AMD_pinned_memory extension requires throwing an error when performing the initial userptr import if "the store cannot be mapped to the GPU address space". So this is not a weird thing that iris is doing, it's part of the actual API we're implementing.
Today, I can use SET_DOMAIN which is almost no code. In the future, I'll have VM_BIND, which also makes sense. In the meantime, this is taking away my easy implementation for a bunch of code that I have to keep supporting forever.
From the point of view of having a clean API...
- Using SET_DOMAIN is clearly a hack. It works, but we're not intending to do anything with cache domains. Dropping this API is a good plan.
- Passing a flag to USERPTR that says "please actually make sure it works" seems entirely reasonable to have as part of the API, and matches our userspace API.
- Using VM_BIND would also make sense and seems reasonable.
- Having to construct an entire batch and submit it for actual execution on the GPU just to check for an error case seems like awful design IMO. Error checking buffers is not what execbuf is for. And it's not simple to use, either.
I checked with Jason and I believe he also prefers having a new flag on the userptr ioctl.
--Ken
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Kenneth Graunke -
Matthew Auld -
Matthew Auld -
Tvrtko Ursulin