Wrapper API for static bus_dma allocations

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Wrapper API for static bus_dma allocations

John Baldwin
The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
descriptor rings) can be a bit obtuse to use in that it require a bit of
boilerplate to create a tag, allocate memory for the tag, then load it to get
the bus address.  Similarly, freeing it also requires several steps.  In
addition, some of the steps are a bit subtle (e.g. the need to check for an
error in the bus_dma callback instead of from bus_dmamap_load()) and not all
drivers get those correct.

To try to make this simpler I've written a little wrapper API that tries to
provide a single call to allocate a buffer and a single call to free a buffer.  
Each buffer is described by a structure defined by the API, and if the call to
allocate a buffer succeeds, the structure contains both a pointer to the
buffer in the kernel's address space as well as a bus address of the buffer.

In the interests of simplicity, this API does not allow the buffer to be quite
as fully configured as the underlying bus_dma API, instead it aims to handle
the most common cases that are used in most drivers.  As such, it assumes that
the buffer must be one contiguous range of DMA address space, and the only
parameters that can be specified are the parent tag, the alignment of the
buffer, the lowaddr parameter, the size of the buffer to allocate, and the
flags parameter that is normally passed to bus_dmamem_alloc().  I believe that
this should be sufficient to cover the vast majority of the drivers in our
tree.

I've included below a patch that contains the wrapper API along with a sample
conversion of the ndis driver (chosen at random).  If folks like this idea I
will update the patch to include manpage changes as well.

--- //depot/vendor/freebsd/src/sys/compat/ndis/subr_ndis.c
+++ //depot/user/jhb/cleanup/sys/compat/ndis/subr_ndis.c
@@ -186,7 +186,6 @@
 static ndis_status NdisMAllocateMapRegisters(ndis_handle,
  uint32_t, uint8_t, uint32_t, uint32_t);
 static void NdisMFreeMapRegisters(ndis_handle);
-static void ndis_mapshared_cb(void *, bus_dma_segment_t *, int, int);
 static void NdisMAllocateSharedMemory(ndis_handle, uint32_t,
  uint8_t, void **, ndis_physaddr *);
 static void ndis_asyncmem_complete(device_object *, void *);
@@ -1387,23 +1386,6 @@
  bus_dma_tag_destroy(sc->ndis_mtag);
 }
 
-static void
-ndis_mapshared_cb(arg, segs, nseg, error)
- void *arg;
- bus_dma_segment_t *segs;
- int nseg;
- int error;
-{
- ndis_physaddr *p;
-
- if (error || nseg > 1)
- return;
-
- p = arg;
-
- p->np_quad = segs[0].ds_addr;
-}
-
 /*
  * This maps to bus_dmamem_alloc().
  */
@@ -1443,35 +1425,17 @@
  * than 1GB of physical memory.
  */
 
- error = bus_dma_tag_create(sc->ndis_parent_tag, 64,
-    0, NDIS_BUS_SPACE_SHARED_MAXADDR, BUS_SPACE_MAXADDR, NULL,
-    NULL, len, 1, len, BUS_DMA_ALLOCNOW, NULL, NULL,
-    &sh->ndis_stag);
+ error = bus_dma_mem_create(&sh->ndis_mem, sc->ndis_parent_tag, 64,
+    NDIS_BUS_SPACE_SHARED_MAXADDR, len, BUS_DMA_NOWAIT | BUS_DMA_ZERO);
 
  if (error) {
  free(sh, M_DEVBUF);
  return;
  }
 
- error = bus_dmamem_alloc(sh->ndis_stag, vaddr,
-    BUS_DMA_NOWAIT | BUS_DMA_ZERO, &sh->ndis_smap);
-
- if (error) {
- bus_dma_tag_destroy(sh->ndis_stag);
- free(sh, M_DEVBUF);
- return;
- }
+ *vaddr = sh->ndis_mem.dma_vaddr;
+ paddr->np_quad = sh->ndis_mem.dma_baddr;
 
- error = bus_dmamap_load(sh->ndis_stag, sh->ndis_smap, *vaddr,
-    len, ndis_mapshared_cb, (void *)paddr, BUS_DMA_NOWAIT);
-
- if (error) {
- bus_dmamem_free(sh->ndis_stag, *vaddr, sh->ndis_smap);
- bus_dma_tag_destroy(sh->ndis_stag);
- free(sh, M_DEVBUF);
- return;
- }
-
  /*
  * Save the physical address along with the source address.
  * The AirGo MIMO driver will call NdisMFreeSharedMemory()
@@ -1482,8 +1446,6 @@
  */
 
  NDIS_LOCK(sc);
- sh->ndis_paddr.np_quad = paddr->np_quad;
- sh->ndis_saddr = *vaddr;
  InsertHeadList((&sc->ndis_shlist), (&sh->ndis_list));
  NDIS_UNLOCK(sc);
 }
@@ -1581,13 +1543,13 @@
  l = sc->ndis_shlist.nle_flink;
  while (l != &sc->ndis_shlist) {
  sh = CONTAINING_RECORD(l, struct ndis_shmem, ndis_list);
- if (sh->ndis_saddr == vaddr)
+ if (sh->ndis_mem.dma_vaddr == vaddr)
  break;
  /*
  * Check the physaddr too, just in case the driver lied
  * about the virtual address.
  */
- if (sh->ndis_paddr.np_quad == paddr.np_quad)
+ if (sh->ndis_mem.dma_baddr == paddr.np_quad)
  break;
  l = l->nle_flink;
  }
@@ -1604,9 +1566,7 @@
 
  NDIS_UNLOCK(sc);
 
- bus_dmamap_unload(sh->ndis_stag, sh->ndis_smap);
- bus_dmamem_free(sh->ndis_stag, sh->ndis_saddr, sh->ndis_smap);
- bus_dma_tag_destroy(sh->ndis_stag);
+ bus_dma_mem_free(&sh->ndis_mem);
 
  free(sh, M_DEVBUF);
 }
--- //depot/vendor/freebsd/src/sys/dev/if_ndis/if_ndisvar.h
+++ //depot/user/jhb/cleanup/sys/dev/if_ndis/if_ndisvar.h
@@ -66,10 +66,7 @@
 
 struct ndis_shmem {
  list_entry ndis_list;
- bus_dma_tag_t ndis_stag;
- bus_dmamap_t ndis_smap;
- void *ndis_saddr;
- ndis_physaddr ndis_paddr;
+ struct bus_dmamem ndis_mem;
 };
 
 struct ndis_cfglist {
--- //depot/vendor/freebsd/src/sys/kern/subr_bus_dma.c
+++ //depot/user/jhb/cleanup/sys/kern/subr_bus_dma.c
@@ -540,3 +540,66 @@
 
  return (0);
 }
+
+struct bus_dma_mem_cb_data {
+ struct bus_dmamem *mem;
+ int error;
+};
+
+static void
+bus_dma_mem_cb(void *arg, bus_dma_segment_t *segs, int nseg, int error)
+{
+ struct bus_dma_mem_cb_data *d;
+
+ d = arg;
+ d->error = error;
+ if (error)
+ return;
+ d->mem->dma_baddr = segs[0].ds_addr;
+}
+
+int
+bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
+    bus_size_t alignment, bus_addr_t lowaddr, bus_size_t len, int flags)
+{
+ struct bus_dma_mem_cb_data d;
+ int error;
+
+ bzero(mem, sizeof(*mem));
+ error = bus_dma_tag_create(parent, alignment, 0, lowaddr,
+    BUS_SPACE_MAXADDR, NULL, NULL, len, 1, len, 0, NULL, NULL,
+    &mem->dma_tag);
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ error = bus_dmamem_alloc(mem->dma_tag, &mem->dma_vaddr, flags,
+    &mem->dma_map);
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ d.mem = mem;
+ error = bus_dmamap_load(mem->dma_tag, mem->dma_map, mem->dma_vaddr, len,
+    bus_dma_mem_cb, &d, BUS_DMA_NOWAIT);
+ if (error == 0)
+ error = d.error;
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ return (0);
+}
+
+void
+bus_dma_mem_free(struct bus_dmamem *mem)
+{
+
+ if (mem->dma_baddr != 0)
+ bus_dmamap_unload(mem->dma_tag, mem->dma_map);
+ if (mem->dma_vaddr != NULL)
+ bus_dmamem_free(mem->dma_tag, mem->dma_vaddr, mem->dma_map);
+ if (mem->dma_tag != NULL)
+ bus_dma_tag_destroy(mem->dma_tag);
+ bzero(mem, sizeof(*mem));
+}
--- //depot/vendor/freebsd/src/sys/sys/bus_dma.h
+++ //depot/user/jhb/cleanup/sys/sys/bus_dma.h
@@ -337,4 +337,29 @@
 
 #endif /* __sparc64__ */
 
+/*
+ * A wrapper API to simplify management of static mappings.
+ */
+
+struct bus_dmamem {
+ bus_dma_tag_t dma_tag;
+ bus_dmamap_t dma_map;
+ void *dma_vaddr;
+ bus_addr_t dma_baddr;
+};
+
+/*
+ * Allocate a mapping.  On success, zero is returned and the 'dma_vaddr'
+ * and 'dma_baddr' fields are populated with the virtual and bus addresses,
+ * respectively, of the mapping.
+ */
+int bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
+       bus_size_t alignment, bus_addr_t lowaddr,
+       bus_size_t len, int flags);
+
+/*
+ * Release a mapping created by bus_dma_mem_create().
+ */
+void bus_dma_mem_free(struct bus_dmamem *mem);
+
 #endif /* _BUS_DMA_H_ */


--
John Baldwin
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Wrapper API for static bus_dma allocations

Poul-Henning Kamp
--------
In message <[hidden email]>, John Baldwin writes:

>The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
>descriptor rings) can be a bit obtuse [...]

Isn't it time we take a good hard stare at all of the bus_dma API,
and refactor it into something a lot more compact ?

--
Poul-Henning Kamp       | UNIX since Zilog Zeus 3.20
[hidden email]         | TCP/IP since RFC 956
FreeBSD committer       | BSD since 4.3-tahoe    
Never attribute to malice what can adequately be explained by incompetence.
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Wrapper API for static bus_dma allocations

John Baldwin
On 1/29/15 4:54 PM, Poul-Henning Kamp wrote:
> --------
> In message <[hidden email]>, John Baldwin writes:
>
>> The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
>> descriptor rings) can be a bit obtuse [...]
>
> Isn't it time we take a good hard stare at all of the bus_dma API,
> and refactor it into something a lot more compact ?

Given the amount of oddball hardware out there I don't think there is a
lot you can cut out.  The filter function might be something we can lose
(and losing it would simplify the implementation), but all the other
weird constraints are actually used by something AFAIK.  I do think we
can provide some simpler wrappers for some of the more common cases, but
there will be some hardware for which those wrappers do not work.

One suggestion Scott has had is to at least make it easier to extend the
API by using getter/setter routines on the tag to work with tag
attributes instead of passing them all in bus_dma_tag_create().

--
John Baldwin
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Wrapper API for static bus_dma allocations

Konstantin Belousov
On Fri, Jan 30, 2015 at 09:56:31AM -0500, John Baldwin wrote:

> On 1/29/15 4:54 PM, Poul-Henning Kamp wrote:
> > --------
> > In message <[hidden email]>, John Baldwin writes:
> >
> >> The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
> >> descriptor rings) can be a bit obtuse [...]
> >
> > Isn't it time we take a good hard stare at all of the bus_dma API,
> > and refactor it into something a lot more compact ?
>
> Given the amount of oddball hardware out there I don't think there is a
> lot you can cut out.  The filter function might be something we can lose
> (and losing it would simplify the implementation), but all the other
> weird constraints are actually used by something AFAIK.  I do think we
> can provide some simpler wrappers for some of the more common cases, but
> there will be some hardware for which those wrappers do not work.
>
> One suggestion Scott has had is to at least make it easier to extend the
> API by using getter/setter routines on the tag to work with tag
> attributes instead of passing them all in bus_dma_tag_create().

BTW, filter function is useless.  It can deny specific bus address from
being used, but it does not provide the busdma implementation even a hint
what other address should be (tried to) used.  In dmar busdma, I simply
ignored it.  And there is no real users of filter in the tree.
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Wrapper API for static bus_dma allocations

John Baldwin
On Friday, January 30, 2015 05:21:50 PM Konstantin Belousov wrote:

> On Fri, Jan 30, 2015 at 09:56:31AM -0500, John Baldwin wrote:
> > On 1/29/15 4:54 PM, Poul-Henning Kamp wrote:
> > > --------
> > >
> > > In message <[hidden email]>, John Baldwin writes:
> > >> The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
> > >> descriptor rings) can be a bit obtuse [...]
> > >
> > > Isn't it time we take a good hard stare at all of the bus_dma API,
> > > and refactor it into something a lot more compact ?
> >
> > Given the amount of oddball hardware out there I don't think there is a
> > lot you can cut out.  The filter function might be something we can lose
> > (and losing it would simplify the implementation), but all the other
> > weird constraints are actually used by something AFAIK.  I do think we
> > can provide some simpler wrappers for some of the more common cases, but
> > there will be some hardware for which those wrappers do not work.
> >
> > One suggestion Scott has had is to at least make it easier to extend the
> > API by using getter/setter routines on the tag to work with tag
> > attributes instead of passing them all in bus_dma_tag_create().
>
> BTW, filter function is useless.  It can deny specific bus address from
> being used, but it does not provide the busdma implementation even a hint
> what other address should be (tried to) used.  In dmar busdma, I simply
> ignored it.  And there is no real users of filter in the tree.

Yes, it is very annoying.  I think some old ISA SCSI HBA driver might have
used it to skip over some low-memory hole (i.e. there were two valid DMA
ranges and this was the kludge instead of having two sets of lowaddr/highaddr
exclusions).  (That is one part of the API we could rototill is to just remove
the highaddr arg just use a single arg which is effectively lowaddr.  I think
all drivers always set highaddr to BUS_SPACE_MAXADDR.)

--
John Baldwin
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Wrapper API for static bus_dma allocations

Warner Losh

> On Jan 30, 2015, at 2:31 PM, John Baldwin <[hidden email]> wrote:
>
> On Friday, January 30, 2015 05:21:50 PM Konstantin Belousov wrote:
>> On Fri, Jan 30, 2015 at 09:56:31AM -0500, John Baldwin wrote:
>>> On 1/29/15 4:54 PM, Poul-Henning Kamp wrote:
>>>> --------
>>>>
>>>> In message <[hidden email]>, John Baldwin writes:
>>>>> The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
>>>>> descriptor rings) can be a bit obtuse [...]
>>>>
>>>> Isn't it time we take a good hard stare at all of the bus_dma API,
>>>> and refactor it into something a lot more compact ?
>>>
>>> Given the amount of oddball hardware out there I don't think there is a
>>> lot you can cut out.  The filter function might be something we can lose
>>> (and losing it would simplify the implementation), but all the other
>>> weird constraints are actually used by something AFAIK.  I do think we
>>> can provide some simpler wrappers for some of the more common cases, but
>>> there will be some hardware for which those wrappers do not work.
>>>
>>> One suggestion Scott has had is to at least make it easier to extend the
>>> API by using getter/setter routines on the tag to work with tag
>>> attributes instead of passing them all in bus_dma_tag_create().
>>
>> BTW, filter function is useless.  It can deny specific bus address from
>> being used, but it does not provide the busdma implementation even a hint
>> what other address should be (tried to) used.  In dmar busdma, I simply
>> ignored it.  And there is no real users of filter in the tree.
>
> Yes, it is very annoying.  I think some old ISA SCSI HBA driver might have
> used it to skip over some low-memory hole (i.e. there were two valid DMA
> ranges and this was the kludge instead of having two sets of lowaddr/highaddr
> exclusions).  (That is one part of the API we could rototill is to just remove
> the highaddr arg just use a single arg which is effectively lowaddr.  I think
> all drivers always set highaddr to BUS_SPACE_MAXADDR.)

Not all. There’s some PCI cards that can’t do 64-bit cycles that pass in the 32-bit
value on 64-bit systems. There’s 386 instances of this in the tree. But that may be
lowaddr only. It’s hard to grep for this to be sure.

Wanrer
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Wrapper API for static bus_dma allocations

John Baldwin
On Friday, January 30, 2015 09:07:52 PM Warner Losh wrote:

> > On Jan 30, 2015, at 2:31 PM, John Baldwin <[hidden email]> wrote:
> >
> > On Friday, January 30, 2015 05:21:50 PM Konstantin Belousov wrote:
> >> On Fri, Jan 30, 2015 at 09:56:31AM -0500, John Baldwin wrote:
> >>> On 1/29/15 4:54 PM, Poul-Henning Kamp wrote:
> >>>> --------
> >>>>
> >>>> In message <[hidden email]>, John Baldwin writes:
> >>>>> The bus_dma API to allocate a chunk of static DMA'able memory (e.g.
> >>>>> for
> >>>>> descriptor rings) can be a bit obtuse [...]
> >>>>
> >>>> Isn't it time we take a good hard stare at all of the bus_dma API,
> >>>> and refactor it into something a lot more compact ?
> >>>
> >>> Given the amount of oddball hardware out there I don't think there is a
> >>> lot you can cut out.  The filter function might be something we can lose
> >>> (and losing it would simplify the implementation), but all the other
> >>> weird constraints are actually used by something AFAIK.  I do think we
> >>> can provide some simpler wrappers for some of the more common cases, but
> >>> there will be some hardware for which those wrappers do not work.
> >>>
> >>> One suggestion Scott has had is to at least make it easier to extend the
> >>> API by using getter/setter routines on the tag to work with tag
> >>> attributes instead of passing them all in bus_dma_tag_create().
> >>
> >> BTW, filter function is useless.  It can deny specific bus address from
> >> being used, but it does not provide the busdma implementation even a hint
> >> what other address should be (tried to) used.  In dmar busdma, I simply
> >> ignored it.  And there is no real users of filter in the tree.
> >
> > Yes, it is very annoying.  I think some old ISA SCSI HBA driver might have
> > used it to skip over some low-memory hole (i.e. there were two valid DMA
> > ranges and this was the kludge instead of having two sets of
> > lowaddr/highaddr exclusions).  (That is one part of the API we could
> > rototill is to just remove the highaddr arg just use a single arg which
> > is effectively lowaddr.  I think all drivers always set highaddr to
> > BUS_SPACE_MAXADDR.)
>
> Not all. There’s some PCI cards that can’t do 64-bit cycles that pass in the
> 32-bit value on 64-bit systems. There’s 386 instances of this in the tree.
> But that may be lowaddr only. It’s hard to grep for this to be sure.

That is lowaddr only, not the filter callback.

However, even if we remove the filter and highaddr arguments from tags, you
are still stuck with creating a tag, allocating memory, and loading it to get
a bus address (and tracking the associated pointers, etc.).  I still think a
wrapper API for the common case (static DMA allocations) would be useful.

Orthogonally I can explore removing the filter along with highaddr (it is
always BUS_SPACE_MAXADDR).

--
John Baldwin
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Wrapper API for static bus_dma allocations

Ian Lepore-3
In reply to this post by John Baldwin
On Thu, 2015-01-29 at 15:37 -0500, John Baldwin wrote:

> The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
> descriptor rings) can be a bit obtuse to use in that it require a bit of
> boilerplate to create a tag, allocate memory for the tag, then load it to get
> the bus address.  Similarly, freeing it also requires several steps.  In
> addition, some of the steps are a bit subtle (e.g. the need to check for an
> error in the bus_dma callback instead of from bus_dmamap_load()) and not all
> drivers get those correct.
>
> To try to make this simpler I've written a little wrapper API that tries to
> provide a single call to allocate a buffer and a single call to free a buffer.  
> Each buffer is described by a structure defined by the API, and if the call to
> allocate a buffer succeeds, the structure contains both a pointer to the
> buffer in the kernel's address space as well as a bus address of the buffer.
>
> In the interests of simplicity, this API does not allow the buffer to be quite
> as fully configured as the underlying bus_dma API, instead it aims to handle
> the most common cases that are used in most drivers.  As such, it assumes that
> the buffer must be one contiguous range of DMA address space, and the only
> parameters that can be specified are the parent tag, the alignment of the
> buffer, the lowaddr parameter, the size of the buffer to allocate, and the
> flags parameter that is normally passed to bus_dmamem_alloc().  I believe that
> this should be sufficient to cover the vast majority of the drivers in our
> tree.
>
> I've included below a patch that contains the wrapper API along with a sample
> conversion of the ndis driver (chosen at random).  If folks like this idea I
> will update the patch to include manpage changes as well.
>
> --- //depot/vendor/freebsd/src/sys/compat/ndis/subr_ndis.c
> +++ //depot/user/jhb/cleanup/sys/compat/ndis/subr_ndis.c
[...]

> +struct bus_dmamem {
> + bus_dma_tag_t dma_tag;
> + bus_dmamap_t dma_map;
> + void *dma_vaddr;
> + bus_addr_t dma_baddr;
> +};
> +
> +/*
> + * Allocate a mapping.  On success, zero is returned and the 'dma_vaddr'
> + * and 'dma_baddr' fields are populated with the virtual and bus addresses,
> + * respectively, of the mapping.
> + */
> +int bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
> +       bus_size_t alignment, bus_addr_t lowaddr,
> +       bus_size_t len, int flags);
> +
> +/*
> + * Release a mapping created by bus_dma_mem_create().
> + */
> +void bus_dma_mem_free(struct bus_dmamem *mem);
> +
>  #endif /* _BUS_DMA_H_ */
>
>

You introduce new functions named bus_dma_mem_xxxx(), and a new
structure bus_dmamem.  It's already hard enough to remember the oddball
mix of underbars in busdma naming, can the new struct be bus_dma_mem?

Also, the combo of create/free is most unsatisfying.  Typically
alloc/free come in pairs, as do create/destroy.  These pairings are used
pretty consistantly in busdma now, it'd be nice to not have to remember
this pair is an exception.  (I'm agnostic about whether the new
functionality should be alloc/delete or create/destroy.)

-- Ian


_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Wrapper API for static bus_dma allocations

Ian Lepore-3
In reply to this post by John Baldwin
On Thu, 2015-01-29 at 15:37 -0500, John Baldwin wrote:
[...]

> +int
> +bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
> +    bus_size_t alignment, bus_addr_t lowaddr, bus_size_t len, int flags)
> +{
> + struct bus_dma_mem_cb_data d;
> + int error;
> +
> + bzero(mem, sizeof(*mem));
> + error = bus_dma_tag_create(parent, alignment, 0, lowaddr,
> +    BUS_SPACE_MAXADDR, NULL, NULL, len, 1, len, 0, NULL, NULL,
> +    &mem->dma_tag);
> + if (error) {
> + bus_dma_mem_free(mem);
> + return (error);
> + }
> + error = bus_dmamem_alloc(mem->dma_tag, &mem->dma_vaddr, flags,
> +    &mem->dma_map);
> + if (error) {
> + bus_dma_mem_free(mem);
> + return (error);
> + }
> + d.mem = mem;
> + error = bus_dmamap_load(mem->dma_tag, mem->dma_map, mem->dma_vaddr, len,
> +    bus_dma_mem_cb, &d, BUS_DMA_NOWAIT);
> + if (error == 0)
> + error = d.error;
> + if (error) {
> + bus_dma_mem_free(mem);
> + return (error);
> + }
> + return (0);
> +}
> +[...]

A couple more thoughts on this...

It would be good to pass the flags to both bus_dma_tag_create() and
bus_dmamem_alloc().  While the manpage seems to indicate that certain
flags are specific to tag-create or mem-alloc, there is actually no
overlap in values, so passing flags to tag-create that a given arch only
uses in mem-alloc should be harmless.  Implementations may be able
optimize things if they can see flags at tag-create time that normally
apply to allocation.

Hmmm, it just popped into my head that a new flag that means "this is
going to be a combined tag+map+buffer allocation" might allow for even
more optimizations.  For example you can avoid allocating any resources
related to bouncing if you know that the only mapping that will ever be
created with that tag+map is one belonging to a buffer that the
implementation can carefully allocate in a way that avoids bouncing.

Another way to achieve that would be to allow the whole implementation
of bus_dma_mem_create() to be supplied by the arch, rather than having
it be generic in kern.  The more I think about it, the more I like this.

Could we eliminate the alignment arg and instead document that the
alignment of the allocated buffer will be at least the same as the size
argument up to PAGE_SIZE, and will be PAGE_SIZE for anything 1 page or
larger?   There is a standard uma-based allocator available in
kern/subr_busdma_buffalloc.c that implements this behavior.  It's
currently used only by arm, but should be trivial to use more widely.
When I investigated alignment usage a couple years ago, I found nearly
all existing uses of alignment specify a value <= the size in the tag.

-- Ian


_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Wrapper API for static bus_dma allocations

John Baldwin
In reply to this post by John Baldwin
On Thursday, January 29, 2015 03:37:19 PM John Baldwin wrote:

> The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
> descriptor rings) can be a bit obtuse to use in that it require a bit of
> boilerplate to create a tag, allocate memory for the tag, then load it to get
> the bus address.  Similarly, freeing it also requires several steps.  In
> addition, some of the steps are a bit subtle (e.g. the need to check for an
> error in the bus_dma callback instead of from bus_dmamap_load()) and not all
> drivers get those correct.
>
> To try to make this simpler I've written a little wrapper API that tries to
> provide a single call to allocate a buffer and a single call to free a buffer.  
> Each buffer is described by a structure defined by the API, and if the call to
> allocate a buffer succeeds, the structure contains both a pointer to the
> buffer in the kernel's address space as well as a bus address of the buffer.
>
> In the interests of simplicity, this API does not allow the buffer to be quite
> as fully configured as the underlying bus_dma API, instead it aims to handle
> the most common cases that are used in most drivers.  As such, it assumes that
> the buffer must be one contiguous range of DMA address space, and the only
> parameters that can be specified are the parent tag, the alignment of the
> buffer, the lowaddr parameter, the size of the buffer to allocate, and the
> flags parameter that is normally passed to bus_dmamem_alloc().  I believe that
> this should be sufficient to cover the vast majority of the drivers in our
> tree.
>
> I've included below a patch that contains the wrapper API along with a sample
> conversion of the ndis driver (chosen at random).  If folks like this idea I
> will update the patch to include manpage changes as well.
>
> --- //depot/vendor/freebsd/src/sys/compat/ndis/subr_ndis.c
> +++ //depot/user/jhb/cleanup/sys/compat/ndis/subr_ndis.c
> @@ -186,7 +186,6 @@
>  static ndis_status NdisMAllocateMapRegisters(ndis_handle,
>   uint32_t, uint8_t, uint32_t, uint32_t);
>  static void NdisMFreeMapRegisters(ndis_handle);
> -static void ndis_mapshared_cb(void *, bus_dma_segment_t *, int, int);
>  static void NdisMAllocateSharedMemory(ndis_handle, uint32_t,
>   uint8_t, void **, ndis_physaddr *);
>  static void ndis_asyncmem_complete(device_object *, void *);
> @@ -1387,23 +1386,6 @@
>   bus_dma_tag_destroy(sc->ndis_mtag);
>  }
>  
> -static void
> -ndis_mapshared_cb(arg, segs, nseg, error)
> - void *arg;
> - bus_dma_segment_t *segs;
> - int nseg;
> - int error;
> -{
> - ndis_physaddr *p;
> -
> - if (error || nseg > 1)
> - return;
> -
> - p = arg;
> -
> - p->np_quad = segs[0].ds_addr;
> -}
> -
>  /*
>   * This maps to bus_dmamem_alloc().
>   */
> @@ -1443,35 +1425,17 @@
>   * than 1GB of physical memory.
>   */
>  
> - error = bus_dma_tag_create(sc->ndis_parent_tag, 64,
> -    0, NDIS_BUS_SPACE_SHARED_MAXADDR, BUS_SPACE_MAXADDR, NULL,
> -    NULL, len, 1, len, BUS_DMA_ALLOCNOW, NULL, NULL,
> -    &sh->ndis_stag);
> + error = bus_dma_mem_create(&sh->ndis_mem, sc->ndis_parent_tag, 64,
> +    NDIS_BUS_SPACE_SHARED_MAXADDR, len, BUS_DMA_NOWAIT | BUS_DMA_ZERO);
>  
>   if (error) {
>   free(sh, M_DEVBUF);
>   return;
>   }
>  
> - error = bus_dmamem_alloc(sh->ndis_stag, vaddr,
> -    BUS_DMA_NOWAIT | BUS_DMA_ZERO, &sh->ndis_smap);
> -
> - if (error) {
> - bus_dma_tag_destroy(sh->ndis_stag);
> - free(sh, M_DEVBUF);
> - return;
> - }
> + *vaddr = sh->ndis_mem.dma_vaddr;
> + paddr->np_quad = sh->ndis_mem.dma_baddr;
>  
> - error = bus_dmamap_load(sh->ndis_stag, sh->ndis_smap, *vaddr,
> -    len, ndis_mapshared_cb, (void *)paddr, BUS_DMA_NOWAIT);
> -
> - if (error) {
> - bus_dmamem_free(sh->ndis_stag, *vaddr, sh->ndis_smap);
> - bus_dma_tag_destroy(sh->ndis_stag);
> - free(sh, M_DEVBUF);
> - return;
> - }
> -
>   /*
>   * Save the physical address along with the source address.
>   * The AirGo MIMO driver will call NdisMFreeSharedMemory()
> @@ -1482,8 +1446,6 @@
>   */
>  
>   NDIS_LOCK(sc);
> - sh->ndis_paddr.np_quad = paddr->np_quad;
> - sh->ndis_saddr = *vaddr;
>   InsertHeadList((&sc->ndis_shlist), (&sh->ndis_list));
>   NDIS_UNLOCK(sc);
>  }
> @@ -1581,13 +1543,13 @@
>   l = sc->ndis_shlist.nle_flink;
>   while (l != &sc->ndis_shlist) {
>   sh = CONTAINING_RECORD(l, struct ndis_shmem, ndis_list);
> - if (sh->ndis_saddr == vaddr)
> + if (sh->ndis_mem.dma_vaddr == vaddr)
>   break;
>   /*
>   * Check the physaddr too, just in case the driver lied
>   * about the virtual address.
>   */
> - if (sh->ndis_paddr.np_quad == paddr.np_quad)
> + if (sh->ndis_mem.dma_baddr == paddr.np_quad)
>   break;
>   l = l->nle_flink;
>   }
> @@ -1604,9 +1566,7 @@
>  
>   NDIS_UNLOCK(sc);
>  
> - bus_dmamap_unload(sh->ndis_stag, sh->ndis_smap);
> - bus_dmamem_free(sh->ndis_stag, sh->ndis_saddr, sh->ndis_smap);
> - bus_dma_tag_destroy(sh->ndis_stag);
> + bus_dma_mem_free(&sh->ndis_mem);
>  
>   free(sh, M_DEVBUF);
>  }
> --- //depot/vendor/freebsd/src/sys/dev/if_ndis/if_ndisvar.h
> +++ //depot/user/jhb/cleanup/sys/dev/if_ndis/if_ndisvar.h
> @@ -66,10 +66,7 @@
>  
>  struct ndis_shmem {
>   list_entry ndis_list;
> - bus_dma_tag_t ndis_stag;
> - bus_dmamap_t ndis_smap;
> - void *ndis_saddr;
> - ndis_physaddr ndis_paddr;
> + struct bus_dmamem ndis_mem;
>  };
>  
>  struct ndis_cfglist {
> --- //depot/vendor/freebsd/src/sys/kern/subr_bus_dma.c
> +++ //depot/user/jhb/cleanup/sys/kern/subr_bus_dma.c
> @@ -540,3 +540,66 @@
>  
>   return (0);
>  }
> +
> +struct bus_dma_mem_cb_data {
> + struct bus_dmamem *mem;
> + int error;
> +};
> +
> +static void
> +bus_dma_mem_cb(void *arg, bus_dma_segment_t *segs, int nseg, int error)
> +{
> + struct bus_dma_mem_cb_data *d;
> +
> + d = arg;
> + d->error = error;
> + if (error)
> + return;
> + d->mem->dma_baddr = segs[0].ds_addr;
> +}
> +
> +int
> +bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
> +    bus_size_t alignment, bus_addr_t lowaddr, bus_size_t len, int flags)
> +{
> + struct bus_dma_mem_cb_data d;
> + int error;
> +
> + bzero(mem, sizeof(*mem));
> + error = bus_dma_tag_create(parent, alignment, 0, lowaddr,
> +    BUS_SPACE_MAXADDR, NULL, NULL, len, 1, len, 0, NULL, NULL,
> +    &mem->dma_tag);
> + if (error) {
> + bus_dma_mem_free(mem);
> + return (error);
> + }
> + error = bus_dmamem_alloc(mem->dma_tag, &mem->dma_vaddr, flags,
> +    &mem->dma_map);
> + if (error) {
> + bus_dma_mem_free(mem);
> + return (error);
> + }
> + d.mem = mem;
> + error = bus_dmamap_load(mem->dma_tag, mem->dma_map, mem->dma_vaddr, len,
> +    bus_dma_mem_cb, &d, BUS_DMA_NOWAIT);
> + if (error == 0)
> + error = d.error;
> + if (error) {
> + bus_dma_mem_free(mem);
> + return (error);
> + }
> + return (0);
> +}
> +
> +void
> +bus_dma_mem_free(struct bus_dmamem *mem)
> +{
> +
> + if (mem->dma_baddr != 0)
> + bus_dmamap_unload(mem->dma_tag, mem->dma_map);
> + if (mem->dma_vaddr != NULL)
> + bus_dmamem_free(mem->dma_tag, mem->dma_vaddr, mem->dma_map);
> + if (mem->dma_tag != NULL)
> + bus_dma_tag_destroy(mem->dma_tag);
> + bzero(mem, sizeof(*mem));
> +}
> --- //depot/vendor/freebsd/src/sys/sys/bus_dma.h
> +++ //depot/user/jhb/cleanup/sys/sys/bus_dma.h
> @@ -337,4 +337,29 @@
>  
>  #endif /* __sparc64__ */
>  
> +/*
> + * A wrapper API to simplify management of static mappings.
> + */
> +
> +struct bus_dmamem {
> + bus_dma_tag_t dma_tag;
> + bus_dmamap_t dma_map;
> + void *dma_vaddr;
> + bus_addr_t dma_baddr;
> +};
> +
> +/*
> + * Allocate a mapping.  On success, zero is returned and the 'dma_vaddr'
> + * and 'dma_baddr' fields are populated with the virtual and bus addresses,
> + * respectively, of the mapping.
> + */
> +int bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
> +       bus_size_t alignment, bus_addr_t lowaddr,
> +       bus_size_t len, int flags);
> +
> +/*
> + * Release a mapping created by bus_dma_mem_create().
> + */
> +void bus_dma_mem_free(struct bus_dmamem *mem);
> +
>  #endif /* _BUS_DMA_H_ */

Ping.  The last time I brought this up we ended up off in the weeds a bit
about changes to the bus dma API in general.  However, I think that even if
we reduce the number of args to bus_dma_create_tag you are still left with
managing a tag per static allocation etc.  I'm working on porting a driver
today where I basically just copied this into the driver directly rather
than having to implement it all by hand for the umpteenth time.  Are folks
seriously opposed to having a single function you can call to allocate a
static DMA mapping?

--
John Baldwin
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Wrapper API for static bus_dma allocations

Warner Losh
On Fri, Feb 26, 2016 at 6:10 PM, John Baldwin <[hidden email]> wrote:

> On Thursday, January 29, 2015 03:37:19 PM John Baldwin wrote:
> > The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
> > descriptor rings) can be a bit obtuse to use in that it require a bit of
> > boilerplate to create a tag, allocate memory for the tag, then load it
> to get
> > the bus address.  Similarly, freeing it also requires several steps.  In
> > addition, some of the steps are a bit subtle (e.g. the need to check for
> an
> > error in the bus_dma callback instead of from bus_dmamap_load()) and not
> all
> > drivers get those correct.
> >
> > To try to make this simpler I've written a little wrapper API that tries
> to
> > provide a single call to allocate a buffer and a single call to free a
> buffer.
> > Each buffer is described by a structure defined by the API, and if the
> call to
> > allocate a buffer succeeds, the structure contains both a pointer to the
> > buffer in the kernel's address space as well as a bus address of the
> buffer.
> >
> > In the interests of simplicity, this API does not allow the buffer to be
> quite
> > as fully configured as the underlying bus_dma API, instead it aims to
> handle
> > the most common cases that are used in most drivers.  As such, it
> assumes that
> > the buffer must be one contiguous range of DMA address space, and the
> only
> > parameters that can be specified are the parent tag, the alignment of the
> > buffer, the lowaddr parameter, the size of the buffer to allocate, and
> the
> > flags parameter that is normally passed to bus_dmamem_alloc().  I
> believe that
> > this should be sufficient to cover the vast majority of the drivers in
> our
> > tree.
> >
> > I've included below a patch that contains the wrapper API along with a
> sample
> > conversion of the ndis driver (chosen at random).  If folks like this
> idea I
> > will update the patch to include manpage changes as well.
> >
> > --- //depot/vendor/freebsd/src/sys/compat/ndis/subr_ndis.c
> > +++ //depot/user/jhb/cleanup/sys/compat/ndis/subr_ndis.c
> > @@ -186,7 +186,6 @@
> >  static ndis_status NdisMAllocateMapRegisters(ndis_handle,
> >       uint32_t, uint8_t, uint32_t, uint32_t);
> >  static void NdisMFreeMapRegisters(ndis_handle);
> > -static void ndis_mapshared_cb(void *, bus_dma_segment_t *, int, int);
> >  static void NdisMAllocateSharedMemory(ndis_handle, uint32_t,
> >       uint8_t, void **, ndis_physaddr *);
> >  static void ndis_asyncmem_complete(device_object *, void *);
> > @@ -1387,23 +1386,6 @@
> >       bus_dma_tag_destroy(sc->ndis_mtag);
> >  }
> >
> > -static void
> > -ndis_mapshared_cb(arg, segs, nseg, error)
> > -     void                    *arg;
> > -     bus_dma_segment_t       *segs;
> > -     int                     nseg;
> > -     int                     error;
> > -{
> > -     ndis_physaddr           *p;
> > -
> > -     if (error || nseg > 1)
> > -             return;
> > -
> > -     p = arg;
> > -
> > -     p->np_quad = segs[0].ds_addr;
> > -}
> > -
> >  /*
> >   * This maps to bus_dmamem_alloc().
> >   */
> > @@ -1443,35 +1425,17 @@
> >        * than 1GB of physical memory.
> >        */
> >
> > -     error = bus_dma_tag_create(sc->ndis_parent_tag, 64,
> > -         0, NDIS_BUS_SPACE_SHARED_MAXADDR, BUS_SPACE_MAXADDR, NULL,
> > -         NULL, len, 1, len, BUS_DMA_ALLOCNOW, NULL, NULL,
> > -         &sh->ndis_stag);
> > +     error = bus_dma_mem_create(&sh->ndis_mem, sc->ndis_parent_tag, 64,
> > +         NDIS_BUS_SPACE_SHARED_MAXADDR, len, BUS_DMA_NOWAIT |
> BUS_DMA_ZERO);
> >
> >       if (error) {
> >               free(sh, M_DEVBUF);
> >               return;
> >       }
> >
> > -     error = bus_dmamem_alloc(sh->ndis_stag, vaddr,
> > -         BUS_DMA_NOWAIT | BUS_DMA_ZERO, &sh->ndis_smap);
> > -
> > -     if (error) {
> > -             bus_dma_tag_destroy(sh->ndis_stag);
> > -             free(sh, M_DEVBUF);
> > -             return;
> > -     }
> > +     *vaddr = sh->ndis_mem.dma_vaddr;
> > +     paddr->np_quad = sh->ndis_mem.dma_baddr;
> >
> > -     error = bus_dmamap_load(sh->ndis_stag, sh->ndis_smap, *vaddr,
> > -         len, ndis_mapshared_cb, (void *)paddr, BUS_DMA_NOWAIT);
> > -
> > -     if (error) {
> > -             bus_dmamem_free(sh->ndis_stag, *vaddr, sh->ndis_smap);
> > -             bus_dma_tag_destroy(sh->ndis_stag);
> > -             free(sh, M_DEVBUF);
> > -             return;
> > -     }
> > -
> >       /*
> >        * Save the physical address along with the source address.
> >        * The AirGo MIMO driver will call NdisMFreeSharedMemory()
> > @@ -1482,8 +1446,6 @@
> >        */
> >
> >       NDIS_LOCK(sc);
> > -     sh->ndis_paddr.np_quad = paddr->np_quad;
> > -     sh->ndis_saddr = *vaddr;
> >       InsertHeadList((&sc->ndis_shlist), (&sh->ndis_list));
> >       NDIS_UNLOCK(sc);
> >  }
> > @@ -1581,13 +1543,13 @@
> >       l = sc->ndis_shlist.nle_flink;
> >       while (l != &sc->ndis_shlist) {
> >               sh = CONTAINING_RECORD(l, struct ndis_shmem, ndis_list);
> > -             if (sh->ndis_saddr == vaddr)
> > +             if (sh->ndis_mem.dma_vaddr == vaddr)
> >                       break;
> >               /*
> >                * Check the physaddr too, just in case the driver lied
> >                * about the virtual address.
> >                */
> > -             if (sh->ndis_paddr.np_quad == paddr.np_quad)
> > +             if (sh->ndis_mem.dma_baddr == paddr.np_quad)
> >                       break;
> >               l = l->nle_flink;
> >       }
> > @@ -1604,9 +1566,7 @@
> >
> >       NDIS_UNLOCK(sc);
> >
> > -     bus_dmamap_unload(sh->ndis_stag, sh->ndis_smap);
> > -     bus_dmamem_free(sh->ndis_stag, sh->ndis_saddr, sh->ndis_smap);
> > -     bus_dma_tag_destroy(sh->ndis_stag);
> > +     bus_dma_mem_free(&sh->ndis_mem);
> >
> >       free(sh, M_DEVBUF);
> >  }
> > --- //depot/vendor/freebsd/src/sys/dev/if_ndis/if_ndisvar.h
> > +++ //depot/user/jhb/cleanup/sys/dev/if_ndis/if_ndisvar.h
> > @@ -66,10 +66,7 @@
> >
> >  struct ndis_shmem {
> >       list_entry              ndis_list;
> > -     bus_dma_tag_t           ndis_stag;
> > -     bus_dmamap_t            ndis_smap;
> > -     void                    *ndis_saddr;
> > -     ndis_physaddr           ndis_paddr;
> > +     struct bus_dmamem       ndis_mem;
> >  };
> >
> >  struct ndis_cfglist {
> > --- //depot/vendor/freebsd/src/sys/kern/subr_bus_dma.c
> > +++ //depot/user/jhb/cleanup/sys/kern/subr_bus_dma.c
> > @@ -540,3 +540,66 @@
> >
> >       return (0);
> >  }
> > +
> > +struct bus_dma_mem_cb_data {
> > +     struct bus_dmamem *mem;
> > +     int     error;
> > +};
> > +
> > +static void
> > +bus_dma_mem_cb(void *arg, bus_dma_segment_t *segs, int nseg, int error)
> > +{
> > +     struct bus_dma_mem_cb_data *d;
> > +
> > +     d = arg;
> > +     d->error = error;
> > +     if (error)
> > +             return;
> > +     d->mem->dma_baddr = segs[0].ds_addr;
> > +}
> > +
> > +int
> > +bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
> > +    bus_size_t alignment, bus_addr_t lowaddr, bus_size_t len, int flags)
> > +{
> > +     struct bus_dma_mem_cb_data d;
> > +     int error;
> > +
> > +     bzero(mem, sizeof(*mem));
> > +     error = bus_dma_tag_create(parent, alignment, 0, lowaddr,
> > +         BUS_SPACE_MAXADDR, NULL, NULL, len, 1, len, 0, NULL, NULL,
> > +         &mem->dma_tag);
> > +     if (error) {
> > +             bus_dma_mem_free(mem);
> > +             return (error);
> > +     }
> > +     error = bus_dmamem_alloc(mem->dma_tag, &mem->dma_vaddr, flags,
> > +         &mem->dma_map);
> > +     if (error) {
> > +             bus_dma_mem_free(mem);
> > +             return (error);
> > +     }
> > +     d.mem = mem;
> > +     error = bus_dmamap_load(mem->dma_tag, mem->dma_map,
> mem->dma_vaddr, len,
> > +         bus_dma_mem_cb, &d, BUS_DMA_NOWAIT);
> > +     if (error == 0)
> > +             error = d.error;
> > +     if (error) {
> > +             bus_dma_mem_free(mem);
> > +             return (error);
> > +     }
> > +     return (0);
> > +}
> > +
> > +void
> > +bus_dma_mem_free(struct bus_dmamem *mem)
> > +{
> > +
> > +     if (mem->dma_baddr != 0)
> > +             bus_dmamap_unload(mem->dma_tag, mem->dma_map);
> > +     if (mem->dma_vaddr != NULL)
> > +             bus_dmamem_free(mem->dma_tag, mem->dma_vaddr,
> mem->dma_map);
> > +     if (mem->dma_tag != NULL)
> > +             bus_dma_tag_destroy(mem->dma_tag);
> > +     bzero(mem, sizeof(*mem));
> > +}
> > --- //depot/vendor/freebsd/src/sys/sys/bus_dma.h
> > +++ //depot/user/jhb/cleanup/sys/sys/bus_dma.h
> > @@ -337,4 +337,29 @@
> >
> >  #endif /* __sparc64__ */
> >
> > +/*
> > + * A wrapper API to simplify management of static mappings.
> > + */
> > +
> > +struct bus_dmamem {
> > +     bus_dma_tag_t   dma_tag;
> > +     bus_dmamap_t    dma_map;
> > +     void            *dma_vaddr;
> > +     bus_addr_t      dma_baddr;
> > +};
> > +
> > +/*
> > + * Allocate a mapping.  On success, zero is returned and the 'dma_vaddr'
> > + * and 'dma_baddr' fields are populated with the virtual and bus
> addresses,
> > + * respectively, of the mapping.
> > + */
> > +int bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
> > +                    bus_size_t alignment, bus_addr_t lowaddr,
> > +                    bus_size_t len, int flags);
> > +
> > +/*
> > + * Release a mapping created by bus_dma_mem_create().
> > + */
> > +void bus_dma_mem_free(struct bus_dmamem *mem);
> > +
> >  #endif /* _BUS_DMA_H_ */
>
> Ping.  The last time I brought this up we ended up off in the weeds a bit
> about changes to the bus dma API in general.  However, I think that even if
> we reduce the number of args to bus_dma_create_tag you are still left with
> managing a tag per static allocation etc.  I'm working on porting a driver
> today where I basically just copied this into the driver directly rather
> than having to implement it all by hand for the umpteenth time.  Are folks
> seriously opposed to having a single function you can call to allocate a
> static DMA mapping


I have no objections to this.

Warner
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Wrapper API for static bus_dma allocations

Marius Strobl-2
In reply to this post by John Baldwin
On Fri, Feb 26, 2016 at 05:10:53PM -0800, John Baldwin wrote:

> On Thursday, January 29, 2015 03:37:19 PM John Baldwin wrote:
> > The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
> > descriptor rings) can be a bit obtuse to use in that it require a bit of
> > boilerplate to create a tag, allocate memory for the tag, then load it to get
> > the bus address.  Similarly, freeing it also requires several steps.  In
> > addition, some of the steps are a bit subtle (e.g. the need to check for an
> > error in the bus_dma callback instead of from bus_dmamap_load()) and not all
> > drivers get those correct.
> >
> > To try to make this simpler I've written a little wrapper API that tries to
> > provide a single call to allocate a buffer and a single call to free a buffer.  
> > Each buffer is described by a structure defined by the API, and if the call to
> > allocate a buffer succeeds, the structure contains both a pointer to the
> > buffer in the kernel's address space as well as a bus address of the buffer.
> >
> > In the interests of simplicity, this API does not allow the buffer to be quite
> > as fully configured as the underlying bus_dma API, instead it aims to handle
> > the most common cases that are used in most drivers.  As such, it assumes that
> > the buffer must be one contiguous range of DMA address space, and the only
> > parameters that can be specified are the parent tag, the alignment of the
> > buffer, the lowaddr parameter, the size of the buffer to allocate, and the
> > flags parameter that is normally passed to bus_dmamem_alloc().  I believe that
> > this should be sufficient to cover the vast majority of the drivers in our
> > tree.
> >
> > I've included below a patch that contains the wrapper API along with a sample
> > conversion of the ndis driver (chosen at random).  If folks like this idea I
> > will update the patch to include manpage changes as well.
> >
> > --- //depot/vendor/freebsd/src/sys/compat/ndis/subr_ndis.c
> > +++ //depot/user/jhb/cleanup/sys/compat/ndis/subr_ndis.c
> > @@ -186,7 +186,6 @@
> >  static ndis_status NdisMAllocateMapRegisters(ndis_handle,
> >   uint32_t, uint8_t, uint32_t, uint32_t);
> >  static void NdisMFreeMapRegisters(ndis_handle);
> > -static void ndis_mapshared_cb(void *, bus_dma_segment_t *, int, int);
> >  static void NdisMAllocateSharedMemory(ndis_handle, uint32_t,
> >   uint8_t, void **, ndis_physaddr *);
> >  static void ndis_asyncmem_complete(device_object *, void *);
> > @@ -1387,23 +1386,6 @@
> >   bus_dma_tag_destroy(sc->ndis_mtag);
> >  }
> >  
> > -static void
> > -ndis_mapshared_cb(arg, segs, nseg, error)
> > - void *arg;
> > - bus_dma_segment_t *segs;
> > - int nseg;
> > - int error;
> > -{
> > - ndis_physaddr *p;
> > -
> > - if (error || nseg > 1)
> > - return;
> > -
> > - p = arg;
> > -
> > - p->np_quad = segs[0].ds_addr;
> > -}
> > -
> >  /*
> >   * This maps to bus_dmamem_alloc().
> >   */
> > @@ -1443,35 +1425,17 @@
> >   * than 1GB of physical memory.
> >   */
> >  
> > - error = bus_dma_tag_create(sc->ndis_parent_tag, 64,
> > -    0, NDIS_BUS_SPACE_SHARED_MAXADDR, BUS_SPACE_MAXADDR, NULL,
> > -    NULL, len, 1, len, BUS_DMA_ALLOCNOW, NULL, NULL,
> > -    &sh->ndis_stag);
> > + error = bus_dma_mem_create(&sh->ndis_mem, sc->ndis_parent_tag, 64,
> > +    NDIS_BUS_SPACE_SHARED_MAXADDR, len, BUS_DMA_NOWAIT | BUS_DMA_ZERO);
> >  
> >   if (error) {
> >   free(sh, M_DEVBUF);
> >   return;
> >   }
> >  
> > - error = bus_dmamem_alloc(sh->ndis_stag, vaddr,
> > -    BUS_DMA_NOWAIT | BUS_DMA_ZERO, &sh->ndis_smap);
> > -
> > - if (error) {
> > - bus_dma_tag_destroy(sh->ndis_stag);
> > - free(sh, M_DEVBUF);
> > - return;
> > - }
> > + *vaddr = sh->ndis_mem.dma_vaddr;
> > + paddr->np_quad = sh->ndis_mem.dma_baddr;
> >  
> > - error = bus_dmamap_load(sh->ndis_stag, sh->ndis_smap, *vaddr,
> > -    len, ndis_mapshared_cb, (void *)paddr, BUS_DMA_NOWAIT);
> > -
> > - if (error) {
> > - bus_dmamem_free(sh->ndis_stag, *vaddr, sh->ndis_smap);
> > - bus_dma_tag_destroy(sh->ndis_stag);
> > - free(sh, M_DEVBUF);
> > - return;
> > - }
> > -
> >   /*
> >   * Save the physical address along with the source address.
> >   * The AirGo MIMO driver will call NdisMFreeSharedMemory()
> > @@ -1482,8 +1446,6 @@
> >   */
> >  
> >   NDIS_LOCK(sc);
> > - sh->ndis_paddr.np_quad = paddr->np_quad;
> > - sh->ndis_saddr = *vaddr;
> >   InsertHeadList((&sc->ndis_shlist), (&sh->ndis_list));
> >   NDIS_UNLOCK(sc);
> >  }
> > @@ -1581,13 +1543,13 @@
> >   l = sc->ndis_shlist.nle_flink;
> >   while (l != &sc->ndis_shlist) {
> >   sh = CONTAINING_RECORD(l, struct ndis_shmem, ndis_list);
> > - if (sh->ndis_saddr == vaddr)
> > + if (sh->ndis_mem.dma_vaddr == vaddr)
> >   break;
> >   /*
> >   * Check the physaddr too, just in case the driver lied
> >   * about the virtual address.
> >   */
> > - if (sh->ndis_paddr.np_quad == paddr.np_quad)
> > + if (sh->ndis_mem.dma_baddr == paddr.np_quad)
> >   break;
> >   l = l->nle_flink;
> >   }
> > @@ -1604,9 +1566,7 @@
> >  
> >   NDIS_UNLOCK(sc);
> >  
> > - bus_dmamap_unload(sh->ndis_stag, sh->ndis_smap);
> > - bus_dmamem_free(sh->ndis_stag, sh->ndis_saddr, sh->ndis_smap);
> > - bus_dma_tag_destroy(sh->ndis_stag);
> > + bus_dma_mem_free(&sh->ndis_mem);
> >  
> >   free(sh, M_DEVBUF);
> >  }
> > --- //depot/vendor/freebsd/src/sys/dev/if_ndis/if_ndisvar.h
> > +++ //depot/user/jhb/cleanup/sys/dev/if_ndis/if_ndisvar.h
> > @@ -66,10 +66,7 @@
> >  
> >  struct ndis_shmem {
> >   list_entry ndis_list;
> > - bus_dma_tag_t ndis_stag;
> > - bus_dmamap_t ndis_smap;
> > - void *ndis_saddr;
> > - ndis_physaddr ndis_paddr;
> > + struct bus_dmamem ndis_mem;
> >  };
> >  
> >  struct ndis_cfglist {
> > --- //depot/vendor/freebsd/src/sys/kern/subr_bus_dma.c
> > +++ //depot/user/jhb/cleanup/sys/kern/subr_bus_dma.c
> > @@ -540,3 +540,66 @@
> >  
> >   return (0);
> >  }
> > +
> > +struct bus_dma_mem_cb_data {
> > + struct bus_dmamem *mem;
> > + int error;
> > +};
> > +
> > +static void
> > +bus_dma_mem_cb(void *arg, bus_dma_segment_t *segs, int nseg, int error)
> > +{
> > + struct bus_dma_mem_cb_data *d;
> > +
> > + d = arg;
> > + d->error = error;
> > + if (error)
> > + return;
> > + d->mem->dma_baddr = segs[0].ds_addr;
> > +}
> > +
> > +int
> > +bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
> > +    bus_size_t alignment, bus_addr_t lowaddr, bus_size_t len, int flags)
> > +{
> > + struct bus_dma_mem_cb_data d;
> > + int error;
> > +
> > + bzero(mem, sizeof(*mem));
> > + error = bus_dma_tag_create(parent, alignment, 0, lowaddr,
> > +    BUS_SPACE_MAXADDR, NULL, NULL, len, 1, len, 0, NULL, NULL,
> > +    &mem->dma_tag);
> > + if (error) {
> > + bus_dma_mem_free(mem);
> > + return (error);
> > + }
> > + error = bus_dmamem_alloc(mem->dma_tag, &mem->dma_vaddr, flags,
> > +    &mem->dma_map);
> > + if (error) {
> > + bus_dma_mem_free(mem);
> > + return (error);
> > + }
> > + d.mem = mem;
> > + error = bus_dmamap_load(mem->dma_tag, mem->dma_map, mem->dma_vaddr, len,
> > +    bus_dma_mem_cb, &d, BUS_DMA_NOWAIT);
> > + if (error == 0)
> > + error = d.error;
> > + if (error) {
> > + bus_dma_mem_free(mem);
> > + return (error);
> > + }
> > + return (0);
> > +}
> > +
> > +void
> > +bus_dma_mem_free(struct bus_dmamem *mem)
> > +{
> > +
> > + if (mem->dma_baddr != 0)
> > + bus_dmamap_unload(mem->dma_tag, mem->dma_map);
> > + if (mem->dma_vaddr != NULL)
> > + bus_dmamem_free(mem->dma_tag, mem->dma_vaddr, mem->dma_map);
> > + if (mem->dma_tag != NULL)
> > + bus_dma_tag_destroy(mem->dma_tag);
> > + bzero(mem, sizeof(*mem));
> > +}
> > --- //depot/vendor/freebsd/src/sys/sys/bus_dma.h
> > +++ //depot/user/jhb/cleanup/sys/sys/bus_dma.h
> > @@ -337,4 +337,29 @@
> >  
> >  #endif /* __sparc64__ */
> >  
> > +/*
> > + * A wrapper API to simplify management of static mappings.
> > + */
> > +
> > +struct bus_dmamem {
> > + bus_dma_tag_t dma_tag;
> > + bus_dmamap_t dma_map;
> > + void *dma_vaddr;
> > + bus_addr_t dma_baddr;
> > +};
> > +
> > +/*
> > + * Allocate a mapping.  On success, zero is returned and the 'dma_vaddr'
> > + * and 'dma_baddr' fields are populated with the virtual and bus addresses,
> > + * respectively, of the mapping.
> > + */
> > +int bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
> > +       bus_size_t alignment, bus_addr_t lowaddr,
> > +       bus_size_t len, int flags);
> > +
> > +/*
> > + * Release a mapping created by bus_dma_mem_create().
> > + */
> > +void bus_dma_mem_free(struct bus_dmamem *mem);
> > +
> >  #endif /* _BUS_DMA_H_ */
>
> Ping.  The last time I brought this up we ended up off in the weeds a bit
> about changes to the bus dma API in general.  However, I think that even if
> we reduce the number of args to bus_dma_create_tag you are still left with
> managing a tag per static allocation etc.  I'm working on porting a driver
> today where I basically just copied this into the driver directly rather
> than having to implement it all by hand for the umpteenth time.  Are folks
> seriously opposed to having a single function you can call to allocate a
> static DMA mapping?

No, I'm fine with your proposal.

Marius

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Wrapper API for static bus_dma allocations

John Baldwin
In reply to this post by John Baldwin
On Friday, February 26, 2016 05:10:53 PM John Baldwin wrote:

> On Thursday, January 29, 2015 03:37:19 PM John Baldwin wrote:
> > The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
> > descriptor rings) can be a bit obtuse to use in that it require a bit of
> > boilerplate to create a tag, allocate memory for the tag, then load it to get
> > the bus address.  Similarly, freeing it also requires several steps.  In
> > addition, some of the steps are a bit subtle (e.g. the need to check for an
> > error in the bus_dma callback instead of from bus_dmamap_load()) and not all
> > drivers get those correct.
> >
> > To try to make this simpler I've written a little wrapper API that tries to
> > provide a single call to allocate a buffer and a single call to free a buffer.  
> > Each buffer is described by a structure defined by the API, and if the call to
> > allocate a buffer succeeds, the structure contains both a pointer to the
> > buffer in the kernel's address space as well as a bus address of the buffer.
> >
> > In the interests of simplicity, this API does not allow the buffer to be quite
> > as fully configured as the underlying bus_dma API, instead it aims to handle
> > the most common cases that are used in most drivers.  As such, it assumes that
> > the buffer must be one contiguous range of DMA address space, and the only
> > parameters that can be specified are the parent tag, the alignment of the
> > buffer, the lowaddr parameter, the size of the buffer to allocate, and the
> > flags parameter that is normally passed to bus_dmamem_alloc().  I believe that
> > this should be sufficient to cover the vast majority of the drivers in our
> > tree.

After some more thinking, I've altered this API to include an 'args' struct
similar to the one Konstantin used for make_dev_s() to specify constraints.
This would permit most constraints to be specified on an as-needed basis
without requiring all of them each time.  It does still assume 1 contiguous
region, but the majority of our drivers require that anyway.

I've forward ported this and converted a more typical driver (xl(4)) as a
demo of the new API.  You can find the full diff here:

https://reviews.freebsd.org/D5704

(I've not yet written manpage updates)

Here's the new code in xl(4) to allocate the TX and RX rings.  I think it
highlights the specific constraints (alignment, etc.) more clearly than the
previous version:

        ​        /*
        ​         * Now allocate a chunk of DMA-able memory for the DMA
        ​         * descriptor lists.  All of our lists are allocated as a
        ​         * contiguous block of memory.
        ​         */
        ​        bus_dma_mem_args_init(&args);
        ​        args.dma_alignment = 8;
        ​        args.dma_lowaddr = BUS_SPACE_MAXADDR_32BIT;
        ​        error = bus_dma_mem_alloc(bus_get_dma_tag(dev), XL_RX_LIST_SZ, 0, &args,
        ​            &sc->xl_ldata.xl_rx_ring);
        ​        if (error) {
        ​                device_printf(dev, "failed to allocate rx ring\n");
        ​                goto fail;
        ​        }
        ​        sc->xl_ldata.xl_rx_list = sc->xl_ldata.xl_rx_ring.dma_vaddr;
        ​
        ​        error = bus_dma_mem_alloc(bus_get_dma_tag(dev), XL_TX_LIST_SZ, 0, &args,
        ​            &sc->xl_ldata.xl_tx_ring);
        ​        if (error) {
        ​                device_printf(dev, "failed to allocate tx ring\n");
        ​                goto fail;
        ​        }
        ​        sc->xl_ldata.xl_tx_list = sc->xl_ldata.xl_tx_ring.dma_vaddr;


--
John Baldwin
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"