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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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