Skip site navigation (1)Skip section navigation (2)
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>