Re: [PATCH] drm/i915: Use the existing pages when retrying to DMA map

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

On 20/12/2016 14:14, Chris Wilson wrote:
> On Tue, Dec 20, 2016 at 01:42:47PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Rather than freeing and re-allocating the pages when DMA mapping
>> in large chunks fails, we can just rebuild the sg table with no
>> coalescing.
>
> You are freeing and reallocating the pages - I thought you meant the
> sg_table. The cost for recovering the shmemfs after allocation/swapin
> already performed in the first pass should be a look up in the radix tree.
> Not worrisome.

Maybe not, but looks a bit unsightly to me. This is simple enough and 
one re-allocation less. From both pages and sg entries, to just the sg 
entries.

>> Also change back the page counter to unsigned int because that
>> is what the sg API supports.
>
> Rather go the other way and start reporting an error when we overflow.
> Being strict all page_counts should be u64. :|

As I wrote in the other thread, that's already in 
i915_gem_object_create. Added there at the time when I spotted the 
sg_alloc_table unsigned int business.

>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>> Only compile tested!
>> ---
>>  drivers/gpu/drm/i915/i915_gem.c | 40 ++++++++++++++++++++++++++++++----------
>>  1 file changed, 30 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 5275f6248ce3..e73f9f5a5d23 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2340,12 +2340,36 @@ static void i915_sg_trim(struct sg_table *orig_st)
>>  	*orig_st = new_st;
>>  }
>>
>> +static void i915_sg_uncoalesce(struct sg_table *orig_st, unsigned long nents)
>> +{
>> +	struct sg_table new_st;
>> +	struct scatterlist *new_sg;
>> +	struct sgt_iter sgt_iter;
>> +	struct page *page;
>> +
>> +	if (sg_alloc_table(&new_st, nents, GFP_KERNEL))
>> +		return;
>
> I was hoping for a way to use the pages already allocated. Feels
> possible, just reversing the chain and walking backwards will be fun.

For a rainy day as you said. :)

>
>> +	new_sg = new_st.sgl;
>> +	for_each_sgt_page(page, sgt_iter, orig_st) {
>> +		sg_set_page(new_sg, page, PAGE_SIZE, 0);
>> +		/* called before being DMA mapped, no need to copy sg->dma_* */
>> +		new_sg = sg_next(new_sg);
>> +	}
>> +
>> +	GEM_BUG_ON(new_sg); /* Should walk exactly nents and hit the end */
>> +
>> +	sg_free_table(orig_st);
>> +
>> +	*orig_st = new_st;
>> +}
>

Regards,

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