[Intel-gfx] [PATCH 4/6] drm/i915: Add support for stealing purgable stolen pages

[Chris Wilson]

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.
 
> >+	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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre