Re: [PATCH 01/11] drm/i915: Add support for mapping an object page by page

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	ankitprasad.r.sharma@intel.com, intel-gfx@lists.freedesktop.org,
	akash.goel@intel.com, shashidhar.hiremath@intel.com
Subject: Re: [PATCH 01/11] drm/i915: Add support for mapping an object page by page
Date: Fri, 15 Jan 2016 15:15:56 +0000	[thread overview]
Message-ID: <56990D2C.8060502@linux.intel.com> (raw)
In-Reply-To: <20160115114433.GC8308@nuc-i3427.alporthouse.com>

On 15/01/16 11:44, Chris Wilson wrote:
> On Fri, Jan 15, 2016 at 09:53:46AM +0000, Tvrtko Ursulin wrote:
>>
>> On 14/01/16 13:34, Chris Wilson wrote:
>>> On Thu, Jan 14, 2016 at 10:32:11AM +0000, Tvrtko Ursulin wrote:
>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
>>>>> index b448ad8..5f86596 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
>>>>> @@ -317,6 +317,11 @@ struct i915_address_space {
>>>>>   			    uint64_t start,
>>>>>   			    uint64_t length,
>>>>>   			    bool use_scratch);
>>>>> +	void (*insert_page)(struct i915_address_space *vm,
>>>>> +			    dma_addr_t addr,
>>>>> +			    uint64_t offset,
>>>>> +			    enum i915_cache_level cache_level,
>>>>> +			    u32 flags);
>>>>>   	void (*insert_entries)(struct i915_address_space *vm,
>>>>>   			       struct sg_table *st,
>>>>>   			       uint64_t start,
>>>>
>>>> Why not extend the current API to support start page offset and
>>>> number of pages? Could default to full object like today if zero.
>>>> Eg:
>>>>
>>>>   void (*insert_entries)(struct i915_address_space *vm,
>>>> 			struct sg_table *st,
>>>> +			unsigned page_offset,
>>>> +			unsigned num_pages,
>>>
>>> Ouch. That would be quite slow for the insert_page() use case of
>>> page-by-page looping.
>>
>> It could use the same last page caching trick so why would be be so slow?
>
> We don't have the convenient struct? I presumed you were thinking of
> passing the offset through to sg_page_iter
>
> for_each_sg_page(st->sgl, &sg_iter, st->nents, page_offset) {
> 	if (--num_pages)
> 		break;
> 	
> 	/* original pt insertion code */
> }
>
> Is it worth expanding struct sg_table for this shortcut?
>
> There are some games where we spend more time in sg_page_iter_next() than
> anything else in the kernel. The games have more textures than GTT
> space, they aren't playable right now nor will they ever likely to be. :(
> Improving these loops would definitely be a boon nevertheless.

Okay, dead end, mission abort. :)

Ankit please just sync the patch in next respin with the RPM changes 
which happened in the meantime.

Regards,

Tvrtko

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