On Wed, Nov 02, 2011 at 11:04:43AM -0400, Konrad Rzeszutek Wilk wrote:
> 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.
Order in which we put them back on the list can be easily change, either
by use of add_tail or by iterating array from end to begining. i am not
sure how much this can impact things.
> 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, ...)
> }
Your dma change are not suppose to be on top of that, idea is to add yet
another callbacks populate+free_page which will do either pci map page
or just use your code (ie use dma alloc and store dma addr into the
array). So there is a missing piece here before adding your dma code.
I just wanted to keep ttm_tt & ttm_backend merge as simple as possible
without major change. Adding the populate + get_page callback sounded
like good candidate for another patch.
> 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?
Should apply on top of linus master kernel as of yesterday.
I will do some testing and add the populate+free page callback
and resend.
Cheers,
Jerome
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
