Op 28-01-2021 om 17:47 schreef Jason Ekstrand:
> On Thu, Jan 28, 2021 at 10:26 AM Maarten Lankhorst
> <maarten.lankhorst@xxxxxxxxxxxxxxx> wrote:
>> There are a couple of ioctl's related to tiling and cache placement,
>> that make no sense for userptr, reject those:
>> - i915_gem_set_tiling_ioctl()
>> Tiling should always be linear for userptr. Changing placement will
>> fail with -ENXIO.
>> - i915_gem_set_caching_ioctl()
>> Userptr memory should always be cached. Changing caching mode will
>> fail with -ENXIO.
>> - i915_gem_set_domain_ioctl()
>> Changed to be equivalent to gem_wait, which is correct for the
>> cached linear userptr pointers. This is required because we
>> cannot grab a reference to the pages in the rework, but waiting
>> for idle will do the same.
>>
>> This plus the previous changes have been tested against beignet
>> by using its own unit tests, and intel-video-compute by using
>> piglit's opencl tests.
> Did you test against mesa at all?
I tested it and also looked at the code for manual inspection.
Unfortunately rechecking one more time, it seems I missed bo_alloc_internal in mesa. Fortunately it seems not to be capable of allocating userptr.
As far as I can tell, that means the changes to mesa are safe.
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.
~Maarten
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> Reviewed-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
>> Cc: Jason Ekstrand <jason@xxxxxxxxxxxxxx>
>>
>> -- Still needs an ack from relevant userspace that it won't break, but should be good.
>> ---
>> drivers/gpu/drm/i915/display/intel_display.c | 2 +-
>> drivers/gpu/drm/i915/gem/i915_gem_domain.c | 12 ++++++++++--
>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 6 ++++++
>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 ++-
>> 4 files changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index d013b0fab128..3e24db8b9ad6 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -14172,7 +14172,7 @@ static int intel_user_framebuffer_create_handle(struct drm_framebuffer *fb,
>> struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>> struct drm_i915_private *i915 = to_i915(obj->base.dev);
>>
>> - if (obj->userptr.mm) {
>> + if (i915_gem_object_is_userptr(obj)) {
>> drm_dbg(&i915->drm,
>> "attempting to use a userptr for a framebuffer, denied\n");
>> return -EINVAL;
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
>> index 36f54cedaaeb..3078e9a09f70 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
>> @@ -335,7 +335,13 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
>> * not allowed to be changed by userspace.
>> */
>> if (i915_gem_object_is_proxy(obj)) {
>> - ret = -ENXIO;
>> + /*
>> + * Silently allow cached for userptr; the vulkan driver
>> + * sets all objects to cached
>> + */
>> + if (!i915_gem_object_is_userptr(obj) ||
>> + args->caching != I915_CACHING_CACHED)
> Thanks for looking out for this case. I just double-checked and, yes,
> we set caching on userptr but we always set it to CACHED so this
> should take care of us, assuming it does what it looks like it does.
>
> Acked-by: Jason Ekstrand <jason@xxxxxxxxxxxxxx>
>
>> + ret = -ENXIO;
>> goto out;
>> }
>>
>> @@ -533,7 +539,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
>> * considered to be outside of any cache domain.
>> */
>> if (i915_gem_object_is_proxy(obj)) {
>> - err = -ENXIO;
>> + /* silently allow userptr to complete */
>> + if (!i915_gem_object_is_userptr(obj))
>> + err = -ENXIO;
>> goto out;
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> index e9a8ee96d64c..3f300a1d27ba 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> @@ -574,6 +574,12 @@ void __i915_gem_object_flush_frontbuffer(struct drm_i915_gem_object *obj,
>> void __i915_gem_object_invalidate_frontbuffer(struct drm_i915_gem_object *obj,
>> enum fb_op_origin origin);
>>
>> +static inline bool
>> +i915_gem_object_is_userptr(struct drm_i915_gem_object *obj)
>> +{
>> + return obj->userptr.mm;
>> +}
>> +
>> static inline void
>> i915_gem_object_flush_frontbuffer(struct drm_i915_gem_object *obj,
>> enum fb_op_origin origin)
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>> index 0c30ca52dee3..c89cf911fb29 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>> @@ -721,7 +721,8 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
>> .name = "i915_gem_object_userptr",
>> .flags = I915_GEM_OBJECT_IS_SHRINKABLE |
>> I915_GEM_OBJECT_NO_MMAP |
>> - I915_GEM_OBJECT_ASYNC_CANCEL,
>> + I915_GEM_OBJECT_ASYNC_CANCEL |
>> + I915_GEM_OBJECT_IS_PROXY,
>> .get_pages = i915_gem_userptr_get_pages,
>> .put_pages = i915_gem_userptr_put_pages,
>> .dmabuf_export = i915_gem_userptr_dmabuf_export,
>> --
>> 2.30.0
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
