On 08/10/15 12:09, Chris Wilson wrote:
On Thu, Oct 08, 2015 at 11:43:29AM +0100, Tvrtko Ursulin wrote:
-struct drm_i915_gem_object *
-i915_gem_object_create_stolen(struct drm_device *dev, u64 size)
+static bool
+mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind)
+{
+ BUG_ON(obj->stolen == NULL);
I am fundamentally opposed to BUG_ONs which can be avoided. In this
I see no value in hanging the machine while we could WARN_ON and
return false.
Don't bother with the WARN_ON. Either take the BUG_ON or accept that to
get to this point the machine is dead anyway and a warning here doesn't
help identify the root cause (better off with list debugging and memory
debugging). I have personally been converting these asserts over to a
dev-only compiletime option as I still find the BUGs more useful than
WARNs in the GEM code.
This is one of the ones which are to be expected in development only. At
that time I much prefer a WARN_ON since it potentially saves you one
reboot cycle and there aren't really any downsides to it. Especially if,
as you say, machine is dead already.
+ if (obj->madv != I915_MADV_DONTNEED)
+ return false;
+
+ if (obj->pin_display)
+ return false;
+
+ list_add(&obj->tmp_link, unwind);
+ return drm_mm_scan_add_block(&obj->stolen->base);
+}
@@ -520,17 +609,59 @@ i915_gem_object_create_stolen(struct drm_device *dev, u64 size)
if (!stolen)
return ERR_PTR(-ENOMEM);
- ret = i915_gem_stolen_insert_node(dev_priv, stolen, size, 4096);
+ ret = i915_gem_stolen_insert_node(dev_priv, &stolen->base, size, 4096);
+ if (ret == 0)
+ goto out;
+
+ /* No more stolen memory available, or too fragmented.
+ * Try evicting purgeable objects and search again.
+ */
+ ret = stolen_evict(dev_priv, size);
I have raised this question of struct_mutex in the previous round.
Correct me if I am wrong, but I thought there was some effort made
to make stolen object allocation run without struct mutex?
Correct. But note that we do GEM operations inside the eviction logic
and the struct_mutex is the only one we have for them.
With this change it requires it again. At the moment callers seem to
hold it anyway. But I think lockdep_assert_held is needed now at
least to document the requirement, probably in top level
i915_gem_object_create_stolen.
And a comment as to why, might as well also try and document the logic
behind such decisions.
Agreed.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
