[Intel-gfx] [PATCH 12/41] drm/i915: Introduce an internal allocator for disposable private objects

[Tvrtko Ursulin]

On 21/10/2016 08:50, Chris Wilson wrote:
> On Fri, Oct 21, 2016 at 08:21:08AM +0100, Tvrtko Ursulin wrote:
>>
>> On 20/10/2016 21:36, Chris Wilson wrote:
>>> On Thu, Oct 20, 2016 at 05:22:23PM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 20/10/2016 16:03, Chris Wilson wrote:
>>>>> Quite a few of our objects used for internal hardware programming do not
>>>>> benefit from being swappable or from being zero initialised. As such
>>>>> they do not benefit from using a shmemfs backing storage and since they
>>>>> are internal and never directly exposed to the user, we do not need to
>>>>> worry about providing a filp. For these we can use an
>>>>> drm_i915_gem_object wrapper around a sg_table of plain struct page. They
>>>>> are not swap backed and not automatically pinned. If they are reaped
>>>>> by the shrinker, the pages are released and the contents discarded. For
>>>>> the internal use case, this is fine as for example, ringbuffers are
>>>>> pinned from being written by a request to be read by the hardware. Once
>>>>> they are idle, they can be discarded entirely. As such they are a good
>>>>> match for execlist ringbuffers and a small variety of other internal
>>>>> objects.
>>>>>
>>>>> In the first iteration, this is limited to the scratch batch buffers we
>>>>> use (for command parsing and state initialisation).
>>>>
>>>> And the status page.
>>>
>>> Yeah, I was just thinking of the runtime allocated blocks where the
>>> change can be measured.
>>>
>>>>> +	max_order = MAX_ORDER;
>>>>> +#ifdef CONFIG_SWIOTLB
>>>>> +	if (swiotlb_nr_tbl())
>>>>> +		max_order = min(max_order, ilog2(IO_TLB_SEGSIZE));
>>>>> +#endif
>>>>
>>>> I couldn't figure out what IO_TLB_SEGSIZE actually is in some
>>>> minutes of cross-referencing. Did not seem to be in units of bytes
>>>> according to swiotlb.h.
>>>
>>> Pages.
>>>
>>>> In either case my question is - why use different parameters than
>>>> swiotlb_max_size you recently added to i915_gem.c?
>>>
>>> I was trying to exploit the compile time constants, and I did not care
>>> to grow the search for even higher orders.
>>
>> Sorry pages, but it is not AFAICS, but it is units of IO_TLB_SHIFT
>> size which is 2k. Happens to be fine since it is smaller (5) than
>> MAX_ORDER (11), however to me it still looks like a unrelated
>> number.
>>
>> ilog2(IO_TLB_SEGSIZE) + 1 would be the same units. It would still
>> look arbitrary but I suppose it would be passable.
>
> Oh, no! I do remember that. I guess that shows the importance of
> comments since I lost that fact during the chase to find a way of
> writing fls() as a compiletime constant.
>
> Let's
>
> /* convert swiotlb segment size into sensible units! */
> #define IO_TLB_SEGPAGES (IO_TLB_SEGSIZE << IO_TLB_SHIFT >> PAGE_SHIFT)

Thats even better, R-b on that. :)

Regards,

Tvrtko