drm/i915: Allow i915_gem_object_get_page() on userptr as well
diff mbox

Message ID 1452642006-23393-1-git-send-email-chris@chris-wilson.co.uk
State New
Headers show

Commit Message

Chris Wilson Jan. 12, 2016, 11:40 p.m. UTC
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.

Reported-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>
---
 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(-)

Comments

Chris Wilson Jan. 12, 2016, 11:49 p.m. UTC | #1
On Tue, Jan 12, 2016 at 11:40:06PM +0000, Chris Wilson wrote:
>  struct drm_i915_gem_object_ops {
> +	const unsigned int flags;

Bleh, const is redundant as the definitions should be const themselves.
-Chris
Dave Gordon Jan. 13, 2016, 7:08 p.m. UTC | #2
On 12/01/16 23:49, Chris Wilson wrote:
> On Tue, Jan 12, 2016 at 11:40:06PM +0000, Chris Wilson wrote:
>>   struct drm_i915_gem_object_ops {
>> +	const unsigned int flags;
>
> Bleh, const is redundant as the definitions should be const themselves.
> -Chris

Yeah, the const-ness attaches to the instances of and references to the 
structure rather than the abstract type. But it doesn't actually do any 
harm, so with or without the redundant 'const' it gets:

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
Kristian Hogsberg Jan. 21, 2016, 11:28 p.m. UTC | #3
On Wed, Jan 13, 2016 at 11:08 AM, Dave Gordon <david.s.gordon@intel.com> wrote:
> On 12/01/16 23:49, Chris Wilson wrote:
>>
>> On Tue, Jan 12, 2016 at 11:40:06PM +0000, Chris Wilson wrote:
>>>
>>>   struct drm_i915_gem_object_ops {
>>> +       const unsigned int flags;
>>
>>
>> Bleh, const is redundant as the definitions should be const themselves.
>> -Chris
>
>
> Yeah, the const-ness attaches to the instances of and references to the
> structure rather than the abstract type. But it doesn't actually do any
> harm, so with or without the redundant 'const' it gets:
>
> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
>

Fixes the crash I was seeing with relocs in a userptr bo.

Reviewed-by: Kristian Høgsberg <krh@bitplanet.net>
Daniel Vetter Jan. 25, 2016, 4:08 p.m. UTC | #4
On Sat, Jan 23, 2016 at 07:13:55AM -0000, Patchwork wrote:
> == Summary ==
> 
> Built on 8fe9e785ae04fa7c37f7935cff12d62e38054b60 drm-intel-nightly: 2016y-01m-21d-11h-02m-42s UTC integration manifest
> 
> Test kms_flip:
>         Subgroup basic-flip-vs-dpms:
>                 pass       -> DMESG-WARN (ilk-hp8440p)
>         Subgroup basic-flip-vs-modeset:
>                 pass       -> DMESG-WARN (ilk-hp8440p)

Our dear friend, the fifo underrun on ilk:

https://bugs.freedesktop.org/show_bug.cgi?id=93787

> Test kms_force_connector_basic:
>         Subgroup force-connector-state:
>                 pass       -> SKIP       (snb-dellxps)

Still undebugged whether this is a loose connector or something really
fishy going on in the kernel:

https://bugs.freedesktop.org/show_bug.cgi?id=93769

> 
> bdw-nuci7        total:140  pass:131  dwarn:0   dfail:0   fail:0   skip:9  
> bdw-ultra        total:143  pass:137  dwarn:0   dfail:0   fail:0   skip:6  
> bsw-nuc-2        total:143  pass:119  dwarn:0   dfail:0   fail:0   skip:24 
> byt-nuc          total:143  pass:128  dwarn:0   dfail:0   fail:0   skip:15 
> hsw-brixbox      total:143  pass:136  dwarn:0   dfail:0   fail:0   skip:7  
> hsw-gt2          total:143  pass:139  dwarn:0   dfail:0   fail:0   skip:4  
> ilk-hp8440p      total:143  pass:102  dwarn:3   dfail:0   fail:0   skip:38 
> ivb-t430s        total:143  pass:137  dwarn:0   dfail:0   fail:0   skip:6  
> skl-i5k-2        total:143  pass:134  dwarn:1   dfail:0   fail:0   skip:8  
> snb-dellxps      total:143  pass:128  dwarn:0   dfail:0   fail:0   skip:15 
> snb-x220t        total:143  pass:129  dwarn:0   dfail:0   fail:1   skip:13 
> 
> Results at /archive/results/CI_IGT_test/Patchwork_1253/
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 104bd1809936..e6ad06c5c2f5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1994,6 +1994,9 @@  enum hdmi_force_audio {
 #define I915_GTT_OFFSET_NONE ((u32)-1)
 
 struct drm_i915_gem_object_ops {
+	const 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
@@ -2009,6 +2012,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 ddc21d4b388d..bb44bad15403 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4425,6 +4425,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,
 };
@@ -5261,7 +5262,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 19fb0bddc1cd..59e45b3a6937 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -789,9 +789,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,
 };