Re: [PATCH 1/3] drm/i915: Allow i915_gem_object_get_page() on userptr as well

From: Rodrigo Vivi <rodrigo.vivi@gmail.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: Re: [PATCH 1/3] drm/i915: Allow i915_gem_object_get_page() on userptr as well
Date: Wed, 03 Feb 2016 18:27:51 +0000	[thread overview]
Message-ID: <CABVU7+ugNUfA+HHvR3W_No7Yd_gTFmH2_b=5qnxcnx-kpCTKPg@mail.gmail.com> (raw)
In-Reply-To: <1454086145-16160-1-git-send-email-chris@chris-wilson.co.uk>

[-- Attachment #1.1: Type: text/plain, Size: 4699 bytes --]

Apparently this patch was blocking other teams that were pinging us at irc
and with 2 rv-b, 2 tested-by and ci success I merged this patch.
However other 2 patches in this series are still pending review so not
merged.

On Fri, Jan 29, 2016 at 8:49 AM Chris Wilson <chris@chris-wilson.co.uk>
wrote:

> commit 033908aed5a596f6202c848c6bbc8a40fb1a8490
> Author: Dave Gordon <david.s.gordon@intel.com>
> Date:   Thu Dec 10 18:51:23 2015 +0000
>
>     drm/i915: mark GEM object pages dirty when mapped & written by the CPU
>
> introduced a check into i915_gem_object_get_dirty_pages() that returned
> a NULL pointer when called with a bad object, one that was not backed by
> shmemfs. This WARN was too strict as we can work on all struct page
> backed objects, and resulted in a WARN + GPF for existing userspace. In
> order to differentiate the various types of objects, add a new flags field
> to the i915_gem_object_ops struct to describe their capabilities, with
> the first flag being whether the object has struct pages.
>
> v2: Drop silly const before an integer in the structure declaration.
>
> Testcase: igt/gem_userptr_blits/relocations
> Reported-and-tested-by: Kristian Høgsberg Kristensen <krh@bitplanet.net>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Cc: Kristian Høgsberg Kristensen <krh@bitplanet.net>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
> Reviewed-by: Kristian Høgsberg Kristensen <krh@bitplanet.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         | 4 ++++
>  drivers/gpu/drm/i915/i915_gem.c         | 3 ++-
>  drivers/gpu/drm/i915/i915_gem_userptr.c | 3 ++-
>  3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 905e90f25957..a2d2f08b7515 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2000,6 +2000,9 @@ enum hdmi_force_audio {
>  #define I915_GTT_OFFSET_NONE ((u32)-1)
>
>  struct drm_i915_gem_object_ops {
> +       unsigned int flags;
> +#define I915_GEM_OBJECT_HAS_STRUCT_PAGE 0x1
> +
>         /* Interface between the GEM object and its backing storage.
>          * get_pages() is called once prior to the use of the associated
> set
>          * of pages before to binding them into the GTT, and put_pages() is
> @@ -2015,6 +2018,7 @@ struct drm_i915_gem_object_ops {
>          */
>         int (*get_pages)(struct drm_i915_gem_object *);
>         void (*put_pages)(struct drm_i915_gem_object *);
> +
>         int (*dmabuf_export)(struct drm_i915_gem_object *);
>         void (*release)(struct drm_i915_gem_object *);
>  };
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index a928823507c5..e9b19bca1383 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4465,6 +4465,7 @@ void i915_gem_object_init(struct drm_i915_gem_object
> *obj,
>  }
>
>  static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
> +       .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE,
>         .get_pages = i915_gem_object_get_pages_gtt,
>         .put_pages = i915_gem_object_put_pages_gtt,
>  };
> @@ -5309,7 +5310,7 @@ i915_gem_object_get_dirty_page(struct
> drm_i915_gem_object *obj, int n)
>         struct page *page;
>
>         /* Only default objects have per-page dirty tracking */
> -       if (WARN_ON(obj->ops != &i915_gem_object_ops))
> +       if (WARN_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) ==
> 0))
>                 return NULL;
>
>         page = i915_gem_object_get_page(obj, n);
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c
> b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 74a4d1714879..7107f2fd38f5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -708,9 +708,10 @@ i915_gem_userptr_dmabuf_export(struct
> drm_i915_gem_object *obj)
>  }
>
>  static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
> -       .dmabuf_export = i915_gem_userptr_dmabuf_export,
> +       .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE,
>         .get_pages = i915_gem_userptr_get_pages,
>         .put_pages = i915_gem_userptr_put_pages,
> +       .dmabuf_export = i915_gem_userptr_dmabuf_export,
>         .release = i915_gem_userptr_release,
>  };
>
> --
> 2.7.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 5968 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx