| Message ID | 1454086145-16160-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
On Fri, 29 Jan 2016, 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> Cc: drm-intel-fixes@lists.freedesktop.org to pick for 4.5 > --- > 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, > };
On Wed, 03 Feb 2016, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Fri, 29 Jan 2016, 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> > > Cc: drm-intel-fixes@lists.freedesktop.org > > to pick for 4.5 Picked up in drm-intel-fixes. BR, Jani. > >> --- >> 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, >> };
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, };
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(-)