| Message ID | 1469005202-9659-4-git-send-email-chris@chris-wilson.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series |
"Series without cover letter"
( rev:
2
1
)
in
Intel GFX |
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2131e7f1e57a..47f244f9c64e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2286,7 +2286,25 @@ struct drm_i915_gem_object { } userptr; }; }; -#define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base) + +static inline struct drm_i915_gem_object * +to_intel_bo(struct drm_gem_object *gem) +{ + /* Assert that to_intel_bo(NULL) == NULL */ + BUILD_BUG_ON(offsetof(struct drm_i915_gem_object, base)); + + return container_of(gem, struct drm_i915_gem_object, base); +} + +static inline struct drm_i915_gem_object * +i915_gem_object_lookup(struct drm_file *file, u32 handle) +{ + return to_intel_bo(drm_gem_object_lookup(file, handle)); +} + +__deprecated +extern struct drm_gem_object * +drm_gem_object_lookup(struct drm_file *file, u32 handle); static inline bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index d825db7b763d..970496419098 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -864,8 +864,8 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data, if (ret) return ret; - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); - if (&obj->base == NULL) { + obj = i915_gem_object_lookup(file, args->handle); + if (!obj) { ret = -ENOENT; goto unlock; } @@ -1280,8 +1280,8 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, if (ret) goto put_rpm; - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); - if (&obj->base == NULL) { + obj = i915_gem_object_lookup(file, args->handle); + if (!obj) { ret = -ENOENT; goto unlock; } @@ -1497,8 +1497,8 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, if (ret) return ret; - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); - if (&obj->base == NULL) { + obj = i915_gem_object_lookup(file, args->handle); + if (!obj) { ret = -ENOENT; goto unlock; } @@ -1546,8 +1546,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, if (ret) return ret; - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); - if (&obj->base == NULL) { + obj = i915_gem_object_lookup(file, args->handle); + if (!obj) { ret = -ENOENT; goto unlock; } @@ -1587,7 +1587,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, struct drm_file *file) { struct drm_i915_gem_mmap *args = data; - struct drm_gem_object *obj; + struct drm_i915_gem_object *obj; unsigned long addr; if (args->flags & ~(I915_MMAP_WC)) @@ -1596,19 +1596,19 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, if (args->flags & I915_MMAP_WC && !boot_cpu_has(X86_FEATURE_PAT)) return -ENODEV; - obj = drm_gem_object_lookup(file, args->handle); - if (obj == NULL) + obj = i915_gem_object_lookup(file, args->handle); + if (!obj) return -ENOENT; /* prime objects have no backing filp to GEM mmap * pages from. */ - if (!obj->filp) { - drm_gem_object_unreference_unlocked(obj); + if (!obj->base.filp) { + drm_gem_object_unreference_unlocked(&obj->base); return -EINVAL; } - addr = vm_mmap(obj->filp, 0, args->size, + addr = vm_mmap(obj->base.filp, 0, args->size, PROT_READ | PROT_WRITE, MAP_SHARED, args->offset); if (args->flags & I915_MMAP_WC) { @@ -1616,7 +1616,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, struct vm_area_struct *vma; if (down_write_killable(&mm->mmap_sem)) { - drm_gem_object_unreference_unlocked(obj); + drm_gem_object_unreference_unlocked(&obj->base); return -EINTR; } vma = find_vma(mm, addr); @@ -1628,9 +1628,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, up_write(&mm->mmap_sem); /* This may race, but that's ok, it only gets set */ - WRITE_ONCE(to_intel_bo(obj)->has_wc_mmap, true); + WRITE_ONCE(obj->has_wc_mmap, true); } - drm_gem_object_unreference_unlocked(obj); + drm_gem_object_unreference_unlocked(&obj->base); if (IS_ERR((void *)addr)) return addr; @@ -1968,8 +1968,8 @@ i915_gem_mmap_gtt(struct drm_file *file, if (ret) return ret; - obj = to_intel_bo(drm_gem_object_lookup(file, handle)); - if (&obj->base == NULL) { + obj = i915_gem_object_lookup(file, handle); + if (!obj) { ret = -ENOENT; goto unlock; } @@ -2792,8 +2792,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) if (ret) return ret; - obj = to_intel_bo(drm_gem_object_lookup(file, args->bo_handle)); - if (&obj->base == NULL) { + obj = i915_gem_object_lookup(file, args->bo_handle); + if (!obj) { mutex_unlock(&dev->struct_mutex); return -ENOENT; } @@ -3596,8 +3596,8 @@ int i915_gem_get_caching_ioctl(struct drm_device *dev, void *data, struct drm_i915_gem_caching *args = data; struct drm_i915_gem_object *obj; - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); - if (&obj->base == NULL) + obj = i915_gem_object_lookup(file, args->handle); + if (!obj) return -ENOENT; switch (obj->cache_level) { @@ -3657,8 +3657,8 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data, if (ret) goto rpm_put; - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); - if (&obj->base == NULL) { + obj = i915_gem_object_lookup(file, args->handle); + if (!obj) { ret = -ENOENT; goto unlock; } @@ -4026,8 +4026,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, if (ret) return ret; - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); - if (&obj->base == NULL) { + obj = i915_gem_object_lookup(file, args->handle); + if (!obj) { ret = -ENOENT; goto unlock; } @@ -4091,8 +4091,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, if (ret) return ret; - obj = to_intel_bo(drm_gem_object_lookup(file_priv, args->handle)); - if (&obj->base == NULL) { + obj = i915_gem_object_lookup(file_priv, args->handle); + if (!obj) { ret = -ENOENT; goto unlock; } diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c index 8030199731db..46e80f30afcd 100644 --- a/drivers/gpu/drm/i915/i915_gem_tiling.c +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c @@ -166,8 +166,8 @@ i915_gem_set_tiling(struct drm_device *dev, void *data, struct drm_i915_gem_object *obj; int ret = 0; - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); - if (&obj->base == NULL) + obj = i915_gem_object_lookup(file, args->handle); + if (!obj) return -ENOENT; if (!i915_tiling_ok(dev, @@ -297,8 +297,8 @@ i915_gem_get_tiling(struct drm_device *dev, void *data, struct drm_i915_private *dev_priv = to_i915(dev); struct drm_i915_gem_object *obj; - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); - if (&obj->base == NULL) + obj = i915_gem_object_lookup(file, args->handle); + if (!obj) return -ENOENT; mutex_lock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2f7af855a585..77d320584478 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15089,8 +15089,8 @@ intel_user_framebuffer_create(struct drm_device *dev, struct drm_i915_gem_object *obj; struct drm_mode_fb_cmd2 mode_cmd = *user_mode_cmd; - obj = to_intel_bo(drm_gem_object_lookup(filp, mode_cmd.handles[0])); - if (&obj->base == NULL) + obj = i915_gem_object_lookup(filp, mode_cmd.handles[0]); + if (!obj) return ERR_PTR(-ENOENT); fb = intel_framebuffer_create(dev, &mode_cmd, obj); diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c index 3212d8806b5a..5ca797b01ccb 100644 --- a/drivers/gpu/drm/i915/intel_overlay.c +++ b/drivers/gpu/drm/i915/intel_overlay.c @@ -1122,9 +1122,8 @@ int intel_overlay_put_image_ioctl(struct drm_device *dev, void *data, } crtc = to_intel_crtc(drmmode_crtc); - new_bo = to_intel_bo(drm_gem_object_lookup(file_priv, - put_image_rec->bo_handle)); - if (&new_bo->base == NULL) { + new_bo = i915_gem_object_lookup(file_priv, put_image_rec->bo_handle); + if (!new_bo) { ret = -ENOENT; goto out_free; }
On 20/07/16 09:59, Chris Wilson wrote: > For symmetry with a forthcoming i915_gem_object_get() and > i915_gem_object_pu(). > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_drv.h | 20 +++++++++++- > drivers/gpu/drm/i915/i915_gem.c | 58 +++++++++++++++++----------------- > drivers/gpu/drm/i915/i915_gem_tiling.c | 8 ++--- > drivers/gpu/drm/i915/intel_display.c | 4 +-- > drivers/gpu/drm/i915/intel_overlay.c | 5 ++- > 5 files changed, 56 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 2131e7f1e57a..47f244f9c64e 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2286,7 +2286,25 @@ struct drm_i915_gem_object { > } userptr; > }; > }; > -#define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base) > + > +static inline struct drm_i915_gem_object * > +to_intel_bo(struct drm_gem_object *gem) > +{ > + /* Assert that to_intel_bo(NULL) == NULL */ > + BUILD_BUG_ON(offsetof(struct drm_i915_gem_object, base)); > + > + return container_of(gem, struct drm_i915_gem_object, base); > +} Yes, I think this is a much better way of doing the conversion, 'cos it's type-safe and we get that all-important check that 'base' is still at offset 0. In my variant on this, I also introduced the reverse mapping, to_gem_object(), so that we could completely abstract away the conversion back-and-forth between the two classes. Do you think that would be worth adding here too? > +static inline struct drm_i915_gem_object * > +i915_gem_object_lookup(struct drm_file *file, u32 handle) > +{ > + return to_intel_bo(drm_gem_object_lookup(file, handle)); > +} > + > +__deprecated > +extern struct drm_gem_object * > +drm_gem_object_lookup(struct drm_file *file, u32 handle); > > static inline bool > i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj) > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index d825db7b763d..970496419098 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -864,8 +864,8 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data, > if (ret) > return ret; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) { Yes, this construct was not only bizarre, but could actually cause problems on some compilers. See https://lists.freedesktop.org/archives/mesa-dev/2012-April/020514.html Just writing 'obj' is much nicer :) > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) { > ret = -ENOENT; > goto unlock; > } > @@ -1280,8 +1280,8 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, > if (ret) > goto put_rpm; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) { > ret = -ENOENT; > goto unlock; > } > @@ -1497,8 +1497,8 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, > if (ret) > return ret; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) { > ret = -ENOENT; > goto unlock; > } > @@ -1546,8 +1546,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, > if (ret) > return ret; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) { > ret = -ENOENT; > goto unlock; > } > @@ -1587,7 +1587,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > struct drm_file *file) > { > struct drm_i915_gem_mmap *args = data; > - struct drm_gem_object *obj; > + struct drm_i915_gem_object *obj; > unsigned long addr; > > if (args->flags & ~(I915_MMAP_WC)) > @@ -1596,19 +1596,19 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > if (args->flags & I915_MMAP_WC && !boot_cpu_has(X86_FEATURE_PAT)) > return -ENODEV; > > - obj = drm_gem_object_lookup(file, args->handle); > - if (obj == NULL) > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) > return -ENOENT; > > /* prime objects have no backing filp to GEM mmap > * pages from. > */ > - if (!obj->filp) { > - drm_gem_object_unreference_unlocked(obj); > + if (!obj->base.filp) { > + drm_gem_object_unreference_unlocked(&obj->base); > return -EINVAL; > } > > - addr = vm_mmap(obj->filp, 0, args->size, > + addr = vm_mmap(obj->base.filp, 0, args->size, > PROT_READ | PROT_WRITE, MAP_SHARED, > args->offset); > if (args->flags & I915_MMAP_WC) { > @@ -1616,7 +1616,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > struct vm_area_struct *vma; > > if (down_write_killable(&mm->mmap_sem)) { > - drm_gem_object_unreference_unlocked(obj); > + drm_gem_object_unreference_unlocked(&obj->base); > return -EINTR; > } > vma = find_vma(mm, addr); > @@ -1628,9 +1628,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > up_write(&mm->mmap_sem); > > /* This may race, but that's ok, it only gets set */ > - WRITE_ONCE(to_intel_bo(obj)->has_wc_mmap, true); > + WRITE_ONCE(obj->has_wc_mmap, true); > } > - drm_gem_object_unreference_unlocked(obj); > + drm_gem_object_unreference_unlocked(&obj->base); > if (IS_ERR((void *)addr)) > return addr; > > @@ -1968,8 +1968,8 @@ i915_gem_mmap_gtt(struct drm_file *file, > if (ret) > return ret; > > - obj = to_intel_bo(drm_gem_object_lookup(file, handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file, handle); > + if (!obj) { > ret = -ENOENT; > goto unlock; > } > @@ -2792,8 +2792,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > if (ret) > return ret; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->bo_handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file, args->bo_handle); > + if (!obj) { > mutex_unlock(&dev->struct_mutex); > return -ENOENT; > } > @@ -3596,8 +3596,8 @@ int i915_gem_get_caching_ioctl(struct drm_device *dev, void *data, > struct drm_i915_gem_caching *args = data; > struct drm_i915_gem_object *obj; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) > return -ENOENT; > > switch (obj->cache_level) { > @@ -3657,8 +3657,8 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data, > if (ret) > goto rpm_put; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) { > ret = -ENOENT; > goto unlock; > } > @@ -4026,8 +4026,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > if (ret) > return ret; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) { > ret = -ENOENT; > goto unlock; > } > @@ -4091,8 +4091,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, > if (ret) > return ret; > > - obj = to_intel_bo(drm_gem_object_lookup(file_priv, args->handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file_priv, args->handle); > + if (!obj) { > ret = -ENOENT; > goto unlock; > } > diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c > index 8030199731db..46e80f30afcd 100644 > --- a/drivers/gpu/drm/i915/i915_gem_tiling.c > +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c > @@ -166,8 +166,8 @@ i915_gem_set_tiling(struct drm_device *dev, void *data, > struct drm_i915_gem_object *obj; > int ret = 0; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) > return -ENOENT; > > if (!i915_tiling_ok(dev, > @@ -297,8 +297,8 @@ i915_gem_get_tiling(struct drm_device *dev, void *data, > struct drm_i915_private *dev_priv = to_i915(dev); > struct drm_i915_gem_object *obj; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) > return -ENOENT; > > mutex_lock(&dev->struct_mutex); > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 2f7af855a585..77d320584478 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15089,8 +15089,8 @@ intel_user_framebuffer_create(struct drm_device *dev, > struct drm_i915_gem_object *obj; > struct drm_mode_fb_cmd2 mode_cmd = *user_mode_cmd; > > - obj = to_intel_bo(drm_gem_object_lookup(filp, mode_cmd.handles[0])); > - if (&obj->base == NULL) > + obj = i915_gem_object_lookup(filp, mode_cmd.handles[0]); > + if (!obj) > return ERR_PTR(-ENOENT); > > fb = intel_framebuffer_create(dev, &mode_cmd, obj); > diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c > index 3212d8806b5a..5ca797b01ccb 100644 > --- a/drivers/gpu/drm/i915/intel_overlay.c > +++ b/drivers/gpu/drm/i915/intel_overlay.c > @@ -1122,9 +1122,8 @@ int intel_overlay_put_image_ioctl(struct drm_device *dev, void *data, > } > crtc = to_intel_crtc(drmmode_crtc); > > - new_bo = to_intel_bo(drm_gem_object_lookup(file_priv, > - put_image_rec->bo_handle)); > - if (&new_bo->base == NULL) { > + new_bo = i915_gem_object_lookup(file_priv, put_image_rec->bo_handle); > + if (!new_bo) { > ret = -ENOENT; > goto out_free; > } LGTM. Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
On ke, 2016-07-20 at 09:59 +0100, Chris Wilson wrote: > For symmetry with a forthcoming i915_gem_object_get() and > i915_gem_object_pu(). > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_drv.h | 20 +++++++++++- > drivers/gpu/drm/i915/i915_gem.c | 58 +++++++++++++++++----------------- > drivers/gpu/drm/i915/i915_gem_tiling.c | 8 ++--- > drivers/gpu/drm/i915/intel_display.c | 4 +-- > drivers/gpu/drm/i915/intel_overlay.c | 5 ++- > 5 files changed, 56 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 2131e7f1e57a..47f244f9c64e 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2286,7 +2286,25 @@ struct drm_i915_gem_object { > } userptr; > }; > }; > -#define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base) > + > +static inline struct drm_i915_gem_object * > +to_intel_bo(struct drm_gem_object *gem) > +{ > + /* Assert that to_intel_bo(NULL) == NULL */ > + BUILD_BUG_ON(offsetof(struct drm_i915_gem_object, base)); > + > + return container_of(gem, struct drm_i915_gem_object, base); > +} > + > +static inline struct drm_i915_gem_object * > +i915_gem_object_lookup(struct drm_file *file, u32 handle) > +{ > + return to_intel_bo(drm_gem_object_lookup(file, handle)); > +} > + > +__deprecated > +extern struct drm_gem_object * _deprecated seems to be placed just before the function name elsewhere, for easier spotting, I guess. So lets stick to it. Also I'm little wary about doing this deprecate another function when we introduce a new one, especially header level, but I guess there is no scenario when somebody would like to include i915_drv.h and use drm_gem_object_lookup... > +drm_gem_object_lookup(struct drm_file *file, u32 handle); > > static inline bool > i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj) > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index d825db7b763d..970496419098 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -864,8 +864,8 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data, > if (ret) > return ret; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) { > ret = -ENOENT; > goto unlock; > } > @@ -1280,8 +1280,8 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, > if (ret) > goto put_rpm; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) { > ret = -ENOENT; > goto unlock; > } > @@ -1497,8 +1497,8 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, > if (ret) > return ret; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) { > ret = -ENOENT; > goto unlock; > } > @@ -1546,8 +1546,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, > if (ret) > return ret; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) { > ret = -ENOENT; > goto unlock; > } > @@ -1587,7 +1587,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > struct drm_file *file) > { > struct drm_i915_gem_mmap *args = data; > - struct drm_gem_object *obj; > + struct drm_i915_gem_object *obj; > unsigned long addr; > > if (args->flags & ~(I915_MMAP_WC)) > @@ -1596,19 +1596,19 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > if (args->flags & I915_MMAP_WC && !boot_cpu_has(X86_FEATURE_PAT)) > return -ENODEV; > > - obj = drm_gem_object_lookup(file, args->handle); > - if (obj == NULL) > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) > return -ENOENT; > > /* prime objects have no backing filp to GEM mmap > * pages from. > */ > - if (!obj->filp) { > - drm_gem_object_unreference_unlocked(obj); > + if (!obj->base.filp) { > + drm_gem_object_unreference_unlocked(&obj->base); > return -EINVAL; > } > > - addr = vm_mmap(obj->filp, 0, args->size, > + addr = vm_mmap(obj->base.filp, 0, args->size, > PROT_READ | PROT_WRITE, MAP_SHARED, > args->offset); > if (args->flags & I915_MMAP_WC) { > @@ -1616,7 +1616,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > struct vm_area_struct *vma; > > if (down_write_killable(&mm->mmap_sem)) { > - drm_gem_object_unreference_unlocked(obj); > + drm_gem_object_unreference_unlocked(&obj->base); > return -EINTR; > } > vma = find_vma(mm, addr); > @@ -1628,9 +1628,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > up_write(&mm->mmap_sem); > > /* This may race, but that's ok, it only gets set */ > - WRITE_ONCE(to_intel_bo(obj)->has_wc_mmap, true); > + WRITE_ONCE(obj->has_wc_mmap, true); > } > - drm_gem_object_unreference_unlocked(obj); > + drm_gem_object_unreference_unlocked(&obj->base); > if (IS_ERR((void *)addr)) > return addr; > > @@ -1968,8 +1968,8 @@ i915_gem_mmap_gtt(struct drm_file *file, > if (ret) > return ret; > > - obj = to_intel_bo(drm_gem_object_lookup(file, handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file, handle); > + if (!obj) { > ret = -ENOENT; > goto unlock; > } > @@ -2792,8 +2792,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > if (ret) > return ret; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->bo_handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file, args->bo_handle); > + if (!obj) { > mutex_unlock(&dev->struct_mutex); > return -ENOENT; > } > @@ -3596,8 +3596,8 @@ int i915_gem_get_caching_ioctl(struct drm_device *dev, void *data, > struct drm_i915_gem_caching *args = data; > struct drm_i915_gem_object *obj; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) > return -ENOENT; > > switch (obj->cache_level) { > @@ -3657,8 +3657,8 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data, > if (ret) > goto rpm_put; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) { > ret = -ENOENT; > goto unlock; > } > @@ -4026,8 +4026,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > if (ret) > return ret; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) { > ret = -ENOENT; > goto unlock; > } > @@ -4091,8 +4091,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, > if (ret) > return ret; > > - obj = to_intel_bo(drm_gem_object_lookup(file_priv, args->handle)); > - if (&obj->base == NULL) { > + obj = i915_gem_object_lookup(file_priv, args->handle); > + if (!obj) { > ret = -ENOENT; > goto unlock; > } > diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c > index 8030199731db..46e80f30afcd 100644 > --- a/drivers/gpu/drm/i915/i915_gem_tiling.c > +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c > @@ -166,8 +166,8 @@ i915_gem_set_tiling(struct drm_device *dev, void *data, > struct drm_i915_gem_object *obj; > int ret = 0; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) > return -ENOENT; > > if (!i915_tiling_ok(dev, > @@ -297,8 +297,8 @@ i915_gem_get_tiling(struct drm_device *dev, void *data, > struct drm_i915_private *dev_priv = to_i915(dev); > struct drm_i915_gem_object *obj; > > - obj = to_intel_bo(drm_gem_object_lookup(file, args->handle)); > - if (&obj->base == NULL) > + obj = i915_gem_object_lookup(file, args->handle); > + if (!obj) > return -ENOENT; > > mutex_lock(&dev->struct_mutex); > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 2f7af855a585..77d320584478 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15089,8 +15089,8 @@ intel_user_framebuffer_create(struct drm_device *dev, > struct drm_i915_gem_object *obj; > struct drm_mode_fb_cmd2 mode_cmd = *user_mode_cmd; > > - obj = to_intel_bo(drm_gem_object_lookup(filp, mode_cmd.handles[0])); > - if (&obj->base == NULL) > + obj = i915_gem_object_lookup(filp, mode_cmd.handles[0]); > + if (!obj) > return ERR_PTR(-ENOENT); > > fb = intel_framebuffer_create(dev, &mode_cmd, obj); > diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c > index 3212d8806b5a..5ca797b01ccb 100644 > --- a/drivers/gpu/drm/i915/intel_overlay.c > +++ b/drivers/gpu/drm/i915/intel_overlay.c > @@ -1122,9 +1122,8 @@ int intel_overlay_put_image_ioctl(struct drm_device *dev, void *data, > } > crtc = to_intel_crtc(drmmode_crtc); > > - new_bo = to_intel_bo(drm_gem_object_lookup(file_priv, > - put_image_rec->bo_handle)); > - if (&new_bo->base == NULL) { > + new_bo = i915_gem_object_lookup(file_priv, put_image_rec->bo_handle); > + if (!new_bo) { > ret = -ENOENT; > goto out_free; > } LGTM, unless I missed some non-mechanic changes. Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas
On Wed, Jul 20, 2016 at 12:28:16PM +0100, Dave Gordon wrote: > On 20/07/16 09:59, Chris Wilson wrote: > >For symmetry with a forthcoming i915_gem_object_get() and > >i915_gem_object_pu(). > > > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >--- > > drivers/gpu/drm/i915/i915_drv.h | 20 +++++++++++- > > drivers/gpu/drm/i915/i915_gem.c | 58 +++++++++++++++++----------------- > > drivers/gpu/drm/i915/i915_gem_tiling.c | 8 ++--- > > drivers/gpu/drm/i915/intel_display.c | 4 +-- > > drivers/gpu/drm/i915/intel_overlay.c | 5 ++- > > 5 files changed, 56 insertions(+), 39 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >index 2131e7f1e57a..47f244f9c64e 100644 > >--- a/drivers/gpu/drm/i915/i915_drv.h > >+++ b/drivers/gpu/drm/i915/i915_drv.h > >@@ -2286,7 +2286,25 @@ struct drm_i915_gem_object { > > } userptr; > > }; > > }; > >-#define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base) > >+ > >+static inline struct drm_i915_gem_object * > >+to_intel_bo(struct drm_gem_object *gem) > >+{ > >+ /* Assert that to_intel_bo(NULL) == NULL */ > >+ BUILD_BUG_ON(offsetof(struct drm_i915_gem_object, base)); > >+ > >+ return container_of(gem, struct drm_i915_gem_object, base); > >+} > > Yes, I think this is a much better way of doing the conversion, 'cos > it's type-safe and we get that all-important check that 'base' is > still at offset 0. > > In my variant on this, I also introduced the reverse mapping, > to_gem_object(), so that we could completely abstract away the > conversion back-and-forth between the two classes. Do you think that > would be worth adding here too? The convention is that the downcast is tidied away, but as the upcast is just &foo->bar that is easy enough on the eyes. There is also the problem of a limited namespace, so picking who should be to_foo() and to_bar() is tricky. (Without delving into huge polymorphic macros.) -Chris
On Wed, Jul 20, 2016 at 02:38:19PM +0300, Joonas Lahtinen wrote: > On ke, 2016-07-20 at 09:59 +0100, Chris Wilson wrote: > > For symmetry with a forthcoming i915_gem_object_get() and > > i915_gem_object_pu(). > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 20 +++++++++++- > > drivers/gpu/drm/i915/i915_gem.c | 58 +++++++++++++++++----------------- > > drivers/gpu/drm/i915/i915_gem_tiling.c | 8 ++--- > > drivers/gpu/drm/i915/intel_display.c | 4 +-- > > drivers/gpu/drm/i915/intel_overlay.c | 5 ++- > > 5 files changed, 56 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 2131e7f1e57a..47f244f9c64e 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -2286,7 +2286,25 @@ struct drm_i915_gem_object { > > } userptr; > > }; > > }; > > -#define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base) > > + > > +static inline struct drm_i915_gem_object * > > +to_intel_bo(struct drm_gem_object *gem) > > +{ > > + /* Assert that to_intel_bo(NULL) == NULL */ > > + BUILD_BUG_ON(offsetof(struct drm_i915_gem_object, base)); > > + > > + return container_of(gem, struct drm_i915_gem_object, base); > > +} > > + > > +static inline struct drm_i915_gem_object * > > +i915_gem_object_lookup(struct drm_file *file, u32 handle) > > +{ > > + return to_intel_bo(drm_gem_object_lookup(file, handle)); > > +} > > + > > +__deprecated > > +extern struct drm_gem_object * > > _deprecated seems to be placed just before the function name elsewhere, > for easier spotting, I guess. So lets stick to it. I was trying to make this look exactly like the extern it was deprecating, so I settled on putting it on the line before. I wanted it tight against the previous function, so that I had static inline new_function() { ) __deprecates extern old_function() But it is not the verb and checkpatch moans about the lack of newline. > Also I'm little wary about doing this deprecate another function when > we introduce a new one, especially header level, but I guess there is > no scenario when somebody would like to include i915_drv.h and use > drm_gem_object_lookup... Exactly. That would be bad and needs to be caught. -Chris
For symmetry with a forthcoming i915_gem_object_get() and i915_gem_object_pu(). Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.h | 20 +++++++++++- drivers/gpu/drm/i915/i915_gem.c | 58 +++++++++++++++++----------------- drivers/gpu/drm/i915/i915_gem_tiling.c | 8 ++--- drivers/gpu/drm/i915/intel_display.c | 4 +-- drivers/gpu/drm/i915/intel_overlay.c | 5 ++- 5 files changed, 56 insertions(+), 39 deletions(-)