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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
