Re: [PATCH 2/3] drm/ttm: Rework validation & memory space allocation

[Thomas H. <th...@sh...>]

Jerome Glisse wrote:
> This change allow driver to pass memory space preference
> on buffer object placement. Up to 15 differents places
> which should be enought. The placement order is given
> by the memory type encoded on 4bits in 64bits dword.
> First 4bits is the number of memory placement. Next
> 4bits is the prefered placement, then next 4bits is
> the second prefered placement.
>
> In order to avoid long function prototype i used a structure
> to path along driver wishes on the allocation. Beside
> the 64bits dwords describing prefered placement, there
> is flags indicating others preferences (caching attributes,
> pinning, ...), there is also fpfn & lpfn which are the
> first page and last page number btw which allocation can
> happen. This allow to place a buffer in a certain range.
> If those fields are set to 0 ttm will assume buffer can
> be put anywhere in the address space (thus it avoids putting
> a burden on the driver to always properly set those fields).
>
> This patch also factor few functions like evicting first
> entry of lru list or getting a memory space. This avoid
> code duplication.
>
> Signed-off-by: Jerome Glisse <jg...@re...>
>   
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;
}


// For core ttm use.
static inline struct ttm_placement *ttm_encode_simple_placement(u64 
desired, struct ttm_placement *placement)
{
        placement->fpfn = placement->lpfn = 0;
        placement->placements = placement->busy_placements = &desired;
        placement->num_placements = placement->num_busy_placements = 1;
}


> @@ -445,7 +468,6 @@ extern int ttm_bo_check_placement(struct ttm_buffer_object *bo,
>   *
>   * @bdev: Pointer to a ttm_bo_device struct.
>   * @mem_type: The memory type.
> - * @p_offset: offset for managed area in pages.
>   * @p_size: size managed area in pages.
>   *
>   * Initialize a manager for a given memory type.
> @@ -458,7 +480,7 @@ extern int ttm_bo_check_placement(struct ttm_buffer_object *bo,
>   */
>  
>  extern int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type,
> -			  unsigned long p_offset, unsigned long p_size);
> +				unsigned long p_size);
>   

Hmm,
Why remove p_offset here? It allows splitting a PCI memory region into 
multiple TTM memory regions.

Thanks,
Thomas