Date: Thu, 12 Jul 2012 09:37:13 -0700 (PDT) From: Scott Long <scott4long@yahoo.com> To: John Baldwin <jhb@freebsd.org>, "current@freebsd.org" <current@freebsd.org> Cc: Peter Jeremy <peter@rulingia.com> Subject: Re: Adding support for WC (write-combining) memory to bus_dma Message-ID: <1342111033.198.YahooMailNeo@web45716.mail.sp1.yahoo.com> In-Reply-To: <201207121040.27116.jhb@freebsd.org> References: <201207121040.27116.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
=0A=0A=0A=0A----- Original Message -----=0A> From: John Baldwin <jhb@freebs= d.org>=0A> To: current@freebsd.org=0A> Cc: scottl@freebsd.org; Peter Jeremy= <peter@rulingia.com>=0A> Sent: Thursday, July 12, 2012 7:40 AM=0A> Subject= : Adding support for WC (write-combining) memory to bus_dma=0A> =0A> I have= a need to allocate static DMA memory via bus_dmamem_alloc() that is =0A> a= lso WC (for a PCI-e device so it can use "nosnoop" transactions).=A0 =0A> T= his is =0A> similar to what the nvidia driver needs, but in my case it is m= uch cleaner to =0A> allocate the memory via bus dma since the existing code= I am extending all =0A> uses busdma.=0A> =0A> I have a patch to implement = this on 8.x for amd64 that I can port to HEAD if =0A> folks don't object.= =A0 What I would really like to do is add a new paramter to =0A> =0A> bus_d= mamem_alloc() to specify the memory attribute to use, but I am hesitant =0A= > to break that API.=A0 Instead, I added a new flag similar to the existing= =0A> BUS_DMA_NOCACHE used to allocate UC memory.=0A>=A0=0A=0APlease don't = add new parameters. =A0Now that I'm carefully documenting the=0Aevolution o= f the=A0APIs, it's becoming glaringly apparent how sloppy we are=0Awith API= design and=A0interface compatibility. =A0I'm just as guilty of it as anyon= e,=0Abut I'd really like to=A0see less instances of call signature changes = in existing=0Afunctions; they make=A0driver maintenance tedious and are har= d to effectively=0Adocument. =A0Some options I can think of:=0A=0A1. =A0cre= ate bus_dmamem_alloc_attr(). =A0I don't really like leafy API growth like= =0Athis either,=A0but=A0it's not a horrible solution.=0A2. =A0There are exi= sting placeholder flags, BUS_DMA_BUS[1234] that could be=0Aaliased and repu= rposed to hold 4 bits of attribute information for this function.=0AThe 3 a= nd 4 variants are already in use, but I haven't looked closely to see=0Athe= ir scope.=0A3. =A0Reserve the top 16 bits of the flags for attribute inform= ation.=0A4. =A0Move the attribute information into the tag and create new s= etter/getter=0Aaccessors for attribute information. =A0This would probably = be the cleanest,=0Athough it breaks the existing sloppiness of allowing dif= ferent pseudo-attributes=0Afor different allocations under the same tag. = =A0I've wanted to break down the=0Aexisting bus_dma_tag_create() into finer= -grained setter/getters for a while in=0Aany case.=0A5. =A0Move the attribu= te information into the map and force everyone to start=0Acreating maps for= static memory allocations. =A0This would actually add some=0Amissing unifo= rmity to the API and might actually be cleaner that option 4.=0A=0A> While = doing this, I ran into an old bug, which is that if you were to call =0A> b= us_dmamem_alloc() with BUS_DMA_NOCACHE but a tag that otherwise fell throug= h =0A> to using malloc() instead of contigmalloc(), bus_dmamem_alloc() woul= d actually=0A> change the state of the entire page.=A0 This seems wrong.=A0= Instead, I think that =0A> any request for a non-default memory attribute = should always use =0A> contigmalloc().=A0 In fact, even better is to call k= mem_alloc_contig() directly=0A> rather than using contigmalloc().=A0 Howeve= r, if you change this, then =0A> bus_dmamem_free() won't always DTRT as it = doesn't have enough state to =0A> know if=0A> a small allocation should be = free'd via free() or contigfree() (the latter =0A> would be required if it = used a non-default memory attribute).=A0 The fix I used =0A> for this was t= o create a new dummy dmamap that is returned by bus_dmamem_alloc =0A> if it= uses contigmalloc().=A0 bus_dmamem_free() then checks the passed in map = =0A> pointer to decide which type of free to perform.=A0 Once this is fixed= , the =0A> actual WC support is rather trivial as it merely consists of pas= sing a =0A> different argument to kmem_alloc_contig().=0A=0AYup, this is a = problem, and I like your fix; this kind of state is exactly what=0Abelongs = in the map.=0A=0A> =0A> Oh, and using kmem_alloc_contig() instead of the pm= ap_change_attr() hack is=0A> required if you want to be able to export the = same pages to userland via=0A> mmap (e.g. using an OBJT_SG object). :)=0A> = =0A> Peter, this is somewhat orthognal (but related) to your bus_dma patch = which is=0A> what prompted me to post this.=0A> =0A> Patch for 8 is below.= =A0 Porting it to HEAD should be fairly trivial and direct.=0A> =0A> Index:= amd64/amd64/busdma_machdep.c=0A=0AThanks,=0AScott=0A
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1342111033.198.YahooMailNeo>