<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
<html>
<body>
<div dir="auto">
<div dir="auto"><span style="font-size: 12pt;">On January 29, 2021 05:42:47 Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:</span></div><div id="aqm-original" style="color: black;">
<div><br></div>
<blockquote type="cite" class="gmail_quote" style="margin: 0 0 0 0.75ex; border-left: 1px solid #808080; padding-left: 0.75ex;">
<div dir="auto">Op 28-01-2021 om 17:47 schreef Jason Ekstrand:</div>
<blockquote type="cite" class="gmail_quote" style="margin: 0 0 0 0.75ex; border-left: 1px solid #0099CC; padding-left: 0.75ex;">
<div dir="auto">On Thu, Jan 28, 2021 at 10:26 AM Maarten Lankhorst</div>
<div dir="auto"><maarten.lankhorst@linux.intel.com> wrote:</div>
<blockquote type="cite" class="gmail_quote" style="margin: 0 0 0 0.75ex; border-left: 1px solid #9933CC; padding-left: 0.75ex;">
<div dir="auto">There are a couple of ioctl's related to tiling and cache placement,</div>
<div dir="auto">that make no sense for userptr, reject those:</div>
<div dir="auto">- i915_gem_set_tiling_ioctl()</div>
<div dir="auto">Tiling should always be linear for userptr. Changing placement will</div>
<div dir="auto">fail with -ENXIO.</div>
<div dir="auto">- i915_gem_set_caching_ioctl()</div>
<div dir="auto">Userptr memory should always be cached. Changing caching mode will</div>
<div dir="auto">fail with -ENXIO.</div>
<div dir="auto">- i915_gem_set_domain_ioctl()</div>
<div dir="auto">Changed to be equivalent to gem_wait, which is correct for the</div>
<div dir="auto">cached linear userptr pointers. This is required because we</div>
<div dir="auto">cannot grab a reference to the pages in the rework, but waiting</div>
<div dir="auto">for idle will do the same.</div>
<div dir="auto"><br></div>
<div dir="auto">This plus the previous changes have been tested against beignet</div>
<div dir="auto">by using its own unit tests, and intel-video-compute by using</div>
<div dir="auto">piglit's opencl tests.</div>
</blockquote>
<div dir="auto">Did you test against mesa at all?</div>
</blockquote>
<div dir="auto"><br></div>
<div dir="auto">I tested it and also looked at the code for manual inspection.</div>
<div dir="auto"><br></div>
<div dir="auto">Unfortunately rechecking one more time, it seems I missed bo_alloc_internal in mesa. Fortunately it seems not to be capable of allocating userptr.</div>
<div dir="auto"><br></div>
<div dir="auto">As far as I can tell, that means the changes to mesa are safe.</div>
<div dir="auto"><br></div>
<div dir="auto">I tried to run parts of the vulkan cts as well, but it crashed after a while against my distro's vulkan package for non userptr related reasons.</div></blockquote></div><div dir="auto"><br></div><div dir="auto">Sounds good. I was mostly concerned by the fact that you called out some of your testing in the commit message and didn't say you tested Mesa. Made me wonder if you expected us to do that or if you just didn't list it.</div><div dir="auto"><br></div><div dir="auto">--Jason</div><div dir="auto"><br></div><div id="aqm-original" style="color: black;" dir="auto"><blockquote type="cite" class="gmail_quote" style="margin: 0 0 0 0.75ex; border-left: 1px solid #808080; padding-left: 0.75ex;"><div dir="auto"></div>
<div dir="auto">~Maarten</div>
<div dir="auto"><br></div>
<blockquote type="cite" class="gmail_quote" style="margin: 0 0 0 0.75ex; border-left: 1px solid #0099CC; padding-left: 0.75ex;">
<blockquote type="cite" class="gmail_quote" style="margin: 0 0 0 0.75ex; border-left: 1px solid #9933CC; padding-left: 0.75ex;">
<div dir="auto">Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com></div>
<div dir="auto">Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com></div>
<div dir="auto">Cc: Jason Ekstrand <jason@jlekstrand.net></div>
<div dir="auto"><br></div>
<div dir="auto">-- Still needs an ack from relevant userspace that it won't break, but should be good.</div>
<div dir="auto">---</div>
<div dir="auto">drivers/gpu/drm/i915/display/intel_display.c | 2 +-</div>
<div dir="auto">drivers/gpu/drm/i915/gem/i915_gem_domain.c | 12 ++++++++++--</div>
<div dir="auto">drivers/gpu/drm/i915/gem/i915_gem_object.h | 6 ++++++</div>
<div dir="auto">drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 ++-</div>
<div dir="auto">4 files changed, 19 insertions(+), 4 deletions(-)</div>
<div dir="auto"><br></div>
<div dir="auto">diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c</div>
<div dir="auto">index d013b0fab128..3e24db8b9ad6 100644</div>
<div dir="auto">--- a/drivers/gpu/drm/i915/display/intel_display.c</div>
<div dir="auto">+++ b/drivers/gpu/drm/i915/display/intel_display.c</div>
<div dir="auto">@@ -14172,7 +14172,7 @@ static int intel_user_framebuffer_create_handle(struct drm_framebuffer *fb,</div>
<div dir="auto"> struct drm_i915_gem_object *obj = intel_fb_obj(fb);</div>
<div dir="auto"> struct drm_i915_private *i915 = to_i915(obj->base.dev);</div>
<div dir="auto"><br></div>
<div dir="auto">- if (obj->userptr.mm) {</div>
<div dir="auto">+ if (i915_gem_object_is_userptr(obj)) {</div>
<div dir="auto"> drm_dbg(&i915->drm,</div>
<div dir="auto"> "attempting to use a userptr for a framebuffer, denied\n");</div>
<div dir="auto"> return -EINVAL;</div>
<div dir="auto">diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c</div>
<div dir="auto">index 36f54cedaaeb..3078e9a09f70 100644</div>
<div dir="auto">--- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c</div>
<div dir="auto">+++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c</div>
<div dir="auto">@@ -335,7 +335,13 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,</div>
<div dir="auto"> * not allowed to be changed by userspace.</div>
<div dir="auto"> */</div>
<div dir="auto"> if (i915_gem_object_is_proxy(obj)) {</div>
<div dir="auto">- ret = -ENXIO;</div>
<div dir="auto">+ /*</div>
<div dir="auto">+ * Silently allow cached for userptr; the vulkan driver</div>
<div dir="auto">+ * sets all objects to cached</div>
<div dir="auto">+ */</div>
<div dir="auto">+ if (!i915_gem_object_is_userptr(obj) ||</div>
<div dir="auto">+ args->caching != I915_CACHING_CACHED)</div>
</blockquote>
<div dir="auto">Thanks for looking out for this case. I just double-checked and, yes,</div>
<div dir="auto">we set caching on userptr but we always set it to CACHED so this</div>
<div dir="auto">should take care of us, assuming it does what it looks like it does.</div>
<div dir="auto"><br></div>
<div dir="auto">Acked-by: Jason Ekstrand <jason@jlekstrand.net></div>
<div dir="auto"><br></div>
<blockquote type="cite" class="gmail_quote" style="margin: 0 0 0 0.75ex; border-left: 1px solid #9933CC; padding-left: 0.75ex;">
<div dir="auto">+ ret = -ENXIO;</div>
<div dir="auto"> goto out;</div>
<div dir="auto"> }</div>
<div dir="auto"><br></div>
<div dir="auto">@@ -533,7 +539,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,</div>
<div dir="auto"> * considered to be outside of any cache domain.</div>
<div dir="auto"> */</div>
<div dir="auto"> if (i915_gem_object_is_proxy(obj)) {</div>
<div dir="auto">- err = -ENXIO;</div>
<div dir="auto">+ /* silently allow userptr to complete */</div>
<div dir="auto">+ if (!i915_gem_object_is_userptr(obj))</div>
<div dir="auto">+ err = -ENXIO;</div>
<div dir="auto"> goto out;</div>
<div dir="auto"> }</div>
<div dir="auto"><br></div>
<div dir="auto">diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h</div>
<div dir="auto">index e9a8ee96d64c..3f300a1d27ba 100644</div>
<div dir="auto">--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h</div>
<div dir="auto">+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h</div>
<div dir="auto">@@ -574,6 +574,12 @@ void __i915_gem_object_flush_frontbuffer(struct drm_i915_gem_object *obj,</div>
<div dir="auto">void __i915_gem_object_invalidate_frontbuffer(struct drm_i915_gem_object *obj,</div>
<div dir="auto"> enum fb_op_origin origin);</div>
<div dir="auto"><br></div>
<div dir="auto">+static inline bool</div>
<div dir="auto">+i915_gem_object_is_userptr(struct drm_i915_gem_object *obj)</div>
<div dir="auto">+{</div>
<div dir="auto">+ return obj->userptr.mm;</div>
<div dir="auto">+}</div>
<div dir="auto">+</div>
<div dir="auto">static inline void</div>
<div dir="auto">i915_gem_object_flush_frontbuffer(struct drm_i915_gem_object *obj,</div>
<div dir="auto"> enum fb_op_origin origin)</div>
<div dir="auto">diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c</div>
<div dir="auto">index 0c30ca52dee3..c89cf911fb29 100644</div>
<div dir="auto">--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c</div>
<div dir="auto">+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c</div>
<div dir="auto">@@ -721,7 +721,8 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {</div>
<div dir="auto"> .name = "i915_gem_object_userptr",</div>
<div dir="auto"> .flags = I915_GEM_OBJECT_IS_SHRINKABLE |</div>
<div dir="auto"> I915_GEM_OBJECT_NO_MMAP |</div>
<div dir="auto">- I915_GEM_OBJECT_ASYNC_CANCEL,</div>
<div dir="auto">+ I915_GEM_OBJECT_ASYNC_CANCEL |</div>
<div dir="auto">+ I915_GEM_OBJECT_IS_PROXY,</div>
<div dir="auto"> .get_pages = i915_gem_userptr_get_pages,</div>
<div dir="auto"> .put_pages = i915_gem_userptr_put_pages,</div>
<div dir="auto"> .dmabuf_export = i915_gem_userptr_dmabuf_export,</div>
<div dir="auto">--</div>
<div dir="auto">2.30.0</div>
<div dir="auto"><br></div>
</blockquote>
</blockquote>
</blockquote>
</div><div dir="auto"><br></div>
</div>
<div style="color: black;">
<p style="margin: 0 0 1em 0; color: black; font-family: sans-serif;">Sent with <a href="https://play.google.com/store/apps/details?id=org.kman.AquaMail">Aqua Mail for Android</a><br>
<a href="https://www.mobisystems.com/aqua-mail">https://www.mobisystems.com/aqua-mail</a></p>
</div>
</body>
</html>