Re: [PATCH 2/3] drm/i915: Infrastructure for supporting different GGTT views per object

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Intel-gfx@lists.freedesktop.org,
	Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: Re: [PATCH 2/3] drm/i915: Infrastructure for supporting different GGTT views per object
Date: Thu, 04 Dec 2014 10:19:09 +0000	[thread overview]
Message-ID: <5480351D.3070907@linux.intel.com> (raw)
In-Reply-To: <20141204095326.GH25773@nuc-i3427.alporthouse.com>

On 12/04/2014 09:53 AM, Chris Wilson wrote:
> On Wed, Dec 03, 2014 at 02:59:25PM +0000, Tvrtko Ursulin wrote:
>> +void i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>> +		   u32 flags)
>> +{
>> +	struct sg_table *pages = i915_ggtt_view_pages(vma);
>> +
>> +	if (pages && !IS_ERR(pages)) {
>> +		vma->bind_vma(vma, pages, cache_level, flags);
>> +
>> +		if (vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL) {
>> +			sg_free_table(pages);
>> +			kfree(pages);
>> +		}
>> +	}
>> +}
>
> Stop. Even if the failure path is impossible with the present
> implementation, here you are masking the error only to go and pretend
> the binding succeeded.
>
> Don't be lazy, this is a very nasty bug that should be hit during igt -
> or else you are not testing well enough.

Fair comment, even if a bit too assuming. I actually had this as TODO 
but somehow lost it.

I don't have any ideas on how to provoke this to fail from an IGT? Even 
with future implementations it boils down to a couple of small 
allocations which would have to fail reliably.

But will add the error path for it.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx