Commit 19656430 authored by Oscar Mateo's avatar Oscar Mateo Committed by Daniel Vetter

drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display

Otherwise, we do a NULL pointer dereference.

I've seen this happen while handling an error in
i915_gem_object_pin_to_display_plane():

If i915_gem_object_set_cache_level() fails, we call is_pin_display()
to handle the error. At this point, the object is still not pinned
to GGTT and maybe not even bound, so we have to check before we
dereference its GGTT vma.

The IGT kms_flip/bo-too-big tests for this bug.

v2: Chris Wilson says restoring the old value is easier, but that
is_pin_display is useful as a theory of operation. Take the solomonic
decision: at least this way is_pin_display is a little more robust
(until Chris can kill it off).

v3: Chris suggests the WARN in i915_gem_obj_to_ggtt has outlived its
usefulness: add a reminder to remove it.

Issue: VIZ-3772
Signed-off-by: default avatarOscar Mateo <oscar.mateo@intel.com>
Testcase: igt/kms_flip/bo-too-big
Reviewed-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
parent 4271b753
...@@ -3641,6 +3641,15 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data, ...@@ -3641,6 +3641,15 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
static bool is_pin_display(struct drm_i915_gem_object *obj) static bool is_pin_display(struct drm_i915_gem_object *obj)
{ {
struct i915_vma *vma;
if (list_empty(&obj->vma_list))
return false;
vma = i915_gem_obj_to_ggtt(obj);
if (!vma)
return false;
/* There are 3 sources that pin objects: /* There are 3 sources that pin objects:
* 1. The display engine (scanouts, sprites, cursors); * 1. The display engine (scanouts, sprites, cursors);
* 2. Reservations for execbuffer; * 2. Reservations for execbuffer;
...@@ -3652,7 +3661,7 @@ static bool is_pin_display(struct drm_i915_gem_object *obj) ...@@ -3652,7 +3661,7 @@ static bool is_pin_display(struct drm_i915_gem_object *obj)
* subtracting the potential reference by the user, any pin_count * subtracting the potential reference by the user, any pin_count
* remains, it must be due to another use by the display engine. * remains, it must be due to another use by the display engine.
*/ */
return i915_gem_obj_to_ggtt(obj)->pin_count - !!obj->user_pin_count; return vma->pin_count - !!obj->user_pin_count;
} }
/* /*
...@@ -3666,6 +3675,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj, ...@@ -3666,6 +3675,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
struct intel_ring_buffer *pipelined) struct intel_ring_buffer *pipelined)
{ {
u32 old_read_domains, old_write_domain; u32 old_read_domains, old_write_domain;
bool was_pin_display;
int ret; int ret;
if (pipelined != obj->ring) { if (pipelined != obj->ring) {
...@@ -3677,6 +3687,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj, ...@@ -3677,6 +3687,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
/* Mark the pin_display early so that we account for the /* Mark the pin_display early so that we account for the
* display coherency whilst setting up the cache domains. * display coherency whilst setting up the cache domains.
*/ */
was_pin_display = obj->pin_display;
obj->pin_display = true; obj->pin_display = true;
/* The display engine is not coherent with the LLC cache on gen6. As /* The display engine is not coherent with the LLC cache on gen6. As
...@@ -3719,7 +3730,8 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj, ...@@ -3719,7 +3730,8 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
return 0; return 0;
err_unpin_display: err_unpin_display:
obj->pin_display = is_pin_display(obj); WARN_ON(was_pin_display != is_pin_display(obj));
obj->pin_display = was_pin_display;
return ret; return ret;
} }
...@@ -5115,6 +5127,9 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj) ...@@ -5115,6 +5127,9 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
{ {
struct i915_vma *vma; struct i915_vma *vma;
/* This WARN has probably outlived its usefulness (callers already
* WARN if they don't find the GGTT vma they expect). When removing,
* remember to remove the pre-check in is_pin_display() as well */
if (WARN_ON(list_empty(&obj->vma_list))) if (WARN_ON(list_empty(&obj->vma_list)))
return NULL; return NULL;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment