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.
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.
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.
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.
Thomas thanks a lot for your review.
Cheers,
Jerome
------------------------------------------------------------------------------
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
