drm/i915: check for ERR_PTR from i915_gem_object_pin_map()

Commit Message

Dave Gordon April 12, 2016, 1:46 p.m. UTC

The newly-introduced function i915_gem_object_pin_map() returns an
ERR_PTR (not NULL) if the pin-and-map opertaion fails, so that's what we
must check for. And it's nicer not to assign such a pointer-or-error to
a structure being filled in until after it's been validated, so we
should keep it local and avoid exporting a bogus pointer. Also, for
clarity and symmetry, we should clear 'virtual_start' along with 'vma'
when unmapping a ringbuffer.

Signed-off-by: Dave Gordon <[e-mail]>
Cc: Chris Wilson <[e-mail]>
Cc: Tvrtko Ursulin <[e-mail]>

---
 drivers/gpu/drm/i915/i915_drv.h         |  6 ++++--
 drivers/gpu/drm/i915/intel_ringbuffer.c | 15 +++++++++------
 2 files changed, 13 insertions(+), 8 deletions(-)

Comments

Chris Wilson April 12, 2016, 1:53 p.m. UTC | #1

On Tue, Apr 12, 2016 at 02:46:16PM +0100, Dave Gordon wrote:
> The newly-introduced function i915_gem_object_pin_map() returns an
> ERR_PTR (not NULL) if the pin-and-map opertaion fails, so that's what we
> must check for. And it's nicer not to assign such a pointer-or-error to
> a structure being filled in until after it's been validated, so we
> should keep it local and avoid exporting a bogus pointer. Also, for
> clarity and symmetry, we should clear 'virtual_start' along with 'vma'
> when unmapping a ringbuffer.
> 
> Signed-off-by: Dave Gordon <[e-mail]>
> Cc: Chris Wilson <[e-mail]>
> Cc: Tvrtko Ursulin <[e-mail]>
Reviewed-by: Chris Wilson <[e-mail]>
-Chris

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a9c8211..bc8f0a3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3000,9 +3000,11 @@  static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
  * pages and then returns a contiguous mapping of the backing storage into
  * the kernel address space.
  *
- * The caller must hold the struct_mutex.
+ * The caller must hold the struct_mutex, and is responsible for calling
+ * i915_gem_object_unpin_map() when the mapping is no longer required.
  *
- * Returns the pointer through which to access the backing storage.
+ * Returns the pointer through which to access the mapped object, or an
+ * ERR_PTR() on error.
  */
 void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 41b604e..35231ff 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2086,6 +2086,7 @@  void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
 		i915_gem_object_unpin_map(ringbuf->obj);
 	else
 		iounmap(ringbuf->virtual_start);
+	ringbuf->virtual_start = NULL;
 	ringbuf->vma = NULL;
 	i915_gem_object_ggtt_unpin(ringbuf->obj);
 }
@@ -2096,6 +2097,7 @@  int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct drm_i915_gem_object *obj = ringbuf->obj;
+	void *addr;
 	int ret;
 
 	if (HAS_LLC(dev_priv) && !obj->stolen) {
@@ -2107,9 +2109,9 @@  int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 		if (ret)
 			goto err_unpin;
 
-		ringbuf->virtual_start = i915_gem_object_pin_map(obj);
-		if (ringbuf->virtual_start == NULL) {
-			ret = -ENOMEM;
+		addr = i915_gem_object_pin_map(obj);
+		if (IS_ERR(addr)) {
+			ret = PTR_ERR(addr);
 			goto err_unpin;
 		}
 	} else {
@@ -2124,14 +2126,15 @@  int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 		/* Access through the GTT requires the device to be awake. */
 		assert_rpm_wakelock_held(dev_priv);
 
-		ringbuf->virtual_start = ioremap_wc(ggtt->mappable_base +
-						    i915_gem_obj_ggtt_offset(obj), ringbuf->size);
-		if (ringbuf->virtual_start == NULL) {
+		addr = ioremap_wc(ggtt->mappable_base +
+				  i915_gem_obj_ggtt_offset(obj), ringbuf->size);
+		if (addr == NULL) {
 			ret = -ENOMEM;
 			goto err_unpin;
 		}
 	}
 
+	ringbuf->virtual_start = addr;
 	ringbuf->vma = i915_gem_obj_to_ggtt(obj);
 	return 0;