[RFC] ttm merge ttm_backend & ttm_tt

[Konrad Rzeszutek Wilk]

On Tue, Nov 01, 2011 at 09:11:37PM -0400, Jerome Glisse wrote:
> Hi,
> 
> So attached is patch serie (sorry for that i am away of my normal mail
> config) which ultimately merge ttm_backend & ttm_tt it allows to get
> rid of data duplication btw the two, especialy btw ttm_tt and driver
> specific backend. So net result is less 300lines of code accross ttm
> and driver.
> 
> Konrad given some of the upstream nouveau change, your patchserie
> conflict and would lead to revert some nouveau fixes. I believe the
> intention is to get the ttm-dma code into 3.3 (3.2 seems late but
> dunno). If 3.3 is the aim than i will rebase your patch on top of this
> serie, this should lower the number of change your serie needed.
> 
> Note that this is early code, only compile tested, i just wanted to
> get feedback to know if anyone has objection on that.
> 
> For quick review, first 5 patches can be ignored, they are cleanup and
> dead code removal or small fixes. The real action is in the last
> patch.
> 
> Next set is to move page allocation through ttm_helper into the driver
> thus making the whole dma stuff a driver business, driver who don't
> care about dma (like vmw) could ignore it, while driver that have to
> would deal with it through ttm_helper.

I took a look at them (1-3 are good. Please put Reviewed-by on them). The 

[PATCH 4/6] drm/ttm: convert page allocation to use page ptr array instead of list

has a interesting side effect - previously the list was used so on the get_pages
the order of pages was:

 a,b,c,d,e,f

and when one was putting pages back, the list was iterated as so:

 for (i = 0; i < ttm->num_pages; ++i) {
   .. list_add(&cur_page->lru, &h);
   /* list_add is like a stack, items are put in the front */
 }
 ttm_put_pages(&h,..);

which meant that the list would have the items in:
 f,e,d,c,b,a

order. The TTM pool code would iterate over those in that order
and call __free_page. Which means some performance drawback when the
memory had to be iterated forward and then backwards, thought it probably
was cached in the L3 cache, so probably no biggie.

I don't know why it was done that way, but I wrote the TTM DMA code
to be optimized for that (and it has the code to reverse direction - b/c
the testcases I used initially had the a,b,c,d,e,f order when doing get_pages
and put_pages) - but I can alter the code to do it in the forward fashion instead
of the reverse fashion.. Better yet, it removes around 70 lines of code from the
TTM DMA.

Anyhow, it might be noting that in the commit description and perhaps make a bug-fix
for the put_pages being called in the error path instead of ttm_put_pages as a seperate
patch as you suggested.


[PATCH 6/6] drm/ttm: merge ttm_backend and ttm_tt

That is going to be a bit tricky. You are using the pci_map_page, which should not
be used if the pages have been allocated via the pci_alloc_coherent (they are already
mapped). Perhaps a simple check such as:

	if !(ttm->be && ttm->be->func && ttm->be->func->get_pages) {
		ttm->dma_address[i] = pci_map_page(dev->pdev, ...)
	}

That long ttm->be && ttm->be->func can probably be wrapped in a nice macro
and be put in ttm_page_alloc.h as:

diff --git a/include/drm/ttm/ttm_page_alloc.h b/include/drm/ttm/ttm_page_alloc.h
index daf5db6..39d6076 100644
--- a/include/drm/ttm/ttm_page_alloc.h
+++ b/include/drm/ttm/ttm_page_alloc.h
@@ -30,6 +30,12 @@
 #include "ttm_memory.h"
 
 #ifdef CONFIG_SWIOTLB
+static inline bool ttm_dma_inuse(struct ttm_tt *ttm)
+{
+	if (ttm && ttm->be && ttm->be->func && ttm->be->func->get_pages)
+		return true;
+	return false;
+}
 extern bool ttm_dma_override(struct ttm_backend_func *be);
 
 extern int ttm_dma_disable;
@@ -47,6 +53,11 @@ void ttm_dma_page_alloc_fini(void);
 extern int ttm_dma_page_alloc_debugfs(struct seq_file *m, void *data);
 #else
 #define ttm_dma_disable (1)
+
+static inline bool ttm_dma_inuse(struct ttm_tt *ttm)
+{
+	return false;
+}
 static inline bool ttm_dma_override(struct ttm_backend_func *be)
 {
 	return false;


Hmm, I am also not clear what you are using as base? 3.1 does not seem
to have a whole bunch of the things your code is doing. Like the
be->func->clear call from nouveau_sgdma.c.

What base should I be looking at? Is this the base that Dave has for 3.2?