Jerome Glisse wrote:
> On Sat, Dec 05, 2009 at 11:30:38PM +0100, Thomas Hellström wrote:
>
>> Jerome Glisse wrote:
>>
>>> On Sat, Dec 05, 2009 at 01:14:29PM +0100, Thomas Hellström wrote:
>>>
>>>> Jerome,
>>>>
>>>> I'm just reviewing the API change here. I'll do a more thorough
>>>> code review when the API is agreed upon.
>>>>
>>>>
>>>>> +
>>>>> +/**
>>>>> + * struct ttm_placement
>>>>> + *
>>>>> + * @fpfn: first valid page frame number to put the object
>>>>> + * @lpfn: last valid page frame number to put the object
>>>>> + * @placements: prefered placements, 4bits per placement,
>>>>> + * first 4 bits are the the number of placement
>>>>> + * @busy_placements: prefered placements when need to evict,
>>>>> + * 4bits per placement, first 4 bits are the the number of placement
>>>>> + * @flags: Additional placement flags
>>>>> + *
>>>>> + * Structure indicating the placement you request for an object.
>>>>> + */
>>>>> +struct ttm_placement {
>>>>> + unsigned fpfn;
>>>>> + unsigned lpfn;
>>>>> + u64 placements;
>>>>> + u32 flags;
>>>>> +};
>>>>> +
>>>>>
>>>> The names fpfn and lpfn are a bit misleading, since the pfn can
>>>> be confused with kernel pfns. These are really page offsets into
>>>> the managed area? At least this confused me a bit.
>>>>
>>>> For @placements and @busy_placements, I take it you encode both
>>>> the possible placements and the priority order (although you
>>>> leave out the @busy_placements in the struct. I think
>>>> @busy_placements are needed, though. Consider a situation where
>>>> you want to place a texture in vram, but if that causes a
>>>> pipeline stall due to VRAM eviction, rather place it in AGP.
>>>>
>>>> I dislike the coding of all this information in a single member,
>>>> also I think one should distinguish between allowable placement
>>>> and priority. The validation code may want to alter priority,
>>>> which is not easily done in current TTM, and core TTM basically
>>>> only wants to be able to move objects out to cached or uncached
>>>> system memory.
>>>>
>>>> What about
>>>>
>>>> struct ttm_placement {
>>>> unsigned long fpfn;
>>>> unsigned long lpfn;
>>>> const u64 *placements; //Encoded the traditional way
>>>> unsigned num_placements;
>>>> const u64 *busy_placements;
>>>> unsigned num_busy_placements;
>>>> }
>>>>
>>> I removed busy placement, i think giving placement preference is enough.
>>>
>> Obviously at least OpenChrome is using that functionality.
>> A typical use-case where this isn't enough is where you have a
>> texture that's preferrably
>> placed in VRAM but it may also be placed in TT. If both these are
>> unavailable,
>> you'd like to start evicting from TT, because that's done in an
>> instant, compared to PCI DMA from
>> VRAM which is at 60MB/s on some chipsets.
>>
>>
>>
>>> If you look at code you can see that ttm will try with first placement,
>>> than second, ... My change allow to change the priority as you provide
>>> one with each validation.
>>>
>>>
>> Yes. Allowing to change the priority for each validation is a thing
>> that's missing, and I'd like to see
>> that functionality go in.
>>
>>
>>> For instance if you prefer a bo in vram and only allow vram :
>>> placements = (VRAM << 4) | 1
>>>
>>> If you prefere bo in VRAM but the bo can also be in TT :
>>> placements = (VRAM << 4) | (TT << 8) | 2
>>>
>>> If you prefer bo in TT but can also be in vram :
>>> placements = (TT << 4) | (VRAM << 8) | 2
>>>
>>> I would like to avoid having pointer, using a quadword seems enought
>>> to fit all need i can think of (15 different placement). So my
>>> change is all about allowing driver to alter priority for each
>>> validation.
>>>
>> Packing the information in a quadword is also very ugly, and makes
>> the unpacking code
>> hard to follow. In the typical case, either the driver decides
>> placement based on previous usage history,
>> and in that case only one placement would be given. Or the legacy
>> case where a
>> driver uses a static array for priorities. Hence a pointer to an
>> array is IMHO the obvious choice.
>>
>>
>>> On the avoiding eviction thing, the plan i was thinking of is mostly
>>> to let the driver come up with an heuristic to decide when to put
>>> a bo in vram or not. This would mean driver won't set vram as first
>>> priority placement if the driver don't think it's worth evicting
>>> other bo for it. Thought adding busy might be a good idea, i can
>>> understand that it could be usefull.
>>>
>> One thing that should be considered if you want to implement an
>> advanced heuristic in the driver,
>> is to add a bool to the validate function that stops it from
>> evicting. In that case the driver could
>> do that itself, and implement whatever strategy it likes.
>>
>
> Ok so i will switch to array and add back busy stuff. I think to allow
> driver to decide what to do if eviction is needed is as simple as driver
> supplying empty busy placements this would lead to validate returning
> -ENOMEM then driver can do whatever it thinks is best.
>
Yes, that seems like a good solution.
> For radeon my plan is to have a function to mess with the lru list
> in order to try to sort a bit what we want to evict, i might do that
> along the defrag function. And i will still provide busy placements.
>
>
OK.
> Do you prefer i move argument out of the structure ? I did that
> mostly to avoid long function prototype but i don't have strong
> feeling about it.
>
The structure is OK, I think. I don't have any strong feelings about
that either.
> Last if anyone have better name for fpfn/lpfn i am more than willing
> to take them. I did use pfn as it looked like pfn use of kernel, as
> we manage space on page boundary.
>
Actually, ?pfn isn't that bad except that it's easy to think they
actually mean pfn as in the
physical page address, and get confused, as I initially did. Perhaps opn
as in offset page number?
I'll leave it for you to decide.
> Thomas thanks a lot for your review.
>
Thanks. I'll hopefully have time to do a code-review of the final patch
as well.
> Cheers,
> Jerome
>
Thanks,
Thomas
------------------------------------------------------------------------------
Join us December 9, 2009 for the Red Hat Virtual Experience,
a free event focused on virtualization and cloud computing.
Attend in-depth sessions from your desk. Your couch. Anywhere.
http://p.sf.net/sfu/redhat-sfdev2dev
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
