You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/07/22 12:04:03 UTC
[GitHub] [incubator-nuttx-apps] povauboin opened a new pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
povauboin opened a new pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338
this makes mkfatfs use fat_dma_alloc() when CONFIG_FAT_DMAMEMORY is
set. This is needed to ensure mkfatfs operates with boards that use
DMA for microSD
From nuttx b3dd424
## Summary
Reimport the change that was made originally in nuttx tree:
https://github.com/apache/incubator-nuttx/commit/b3dd424
When the mkfatfs tool was migrated to apps/, it seems this change has been lost.
## Impact
## Testing
Tested with:
./boards/arm/stm32h7/stm32h747i-disco/src/stm32_dma_alloc.c
CONFIG_GRAN=y
CONFIG_FAT_DMAMEMORY=y
Test to format a microSD card with FAT:
mkfatfs -F 32 /dev/mmcsd0
mount -t vfat /dev/mmcsd0 /mnt
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338#issuecomment-662455742
> Anyway what do you suggest ? Is there a fix or I just remove the PR ?
The change is not needed and is harmful. There is no reasonable way to do this without inventing a new non-standard OS interface. We will not do this so the PR must be closed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338#discussion_r458828914
##########
File path: fsutils/mkfatfs/mkfatfs.c
##########
@@ -337,7 +338,11 @@ int mkfatfs(FAR const char *pathname, FAR struct fat_format_s *fmt)
/* Allocate a buffer that will be working sector memory */
+#ifdef CONFIG_FAT_DMAMEMORY
+ var.fv_sect = (FAR uint8_t *)fat_dma_alloc(var.fv_sectorsize);
+#else
Review comment:
@povauboin the better fix is at SPI/DMA driver since the limitation origins from there. SPI/DMA driver should check whether the address is outside it's capability and if so it should utilize(alloc and copy) the special memory internally.
The design of fat_dma_alloc/fat_dma_free is totally wrong:
1.How to handle other file system(e.g. NXFFS, SmartFS, LittleFS...)
2.How to handle the block level access
We should remove them even inside the kernel.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo edited a comment on pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338#issuecomment-662477210
Linux has exactly the same issues. But people have developed Linux device drivers for allocating user-space DMA buffers. For example https://github.com/ikwzm/udmabuf
Something like that would be a POSIX compatible solution that should not get too much resistance.
Also related:
https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18842418/Linux+DMA+From+User+Space
https://stackoverflow.com/questions/5539375/linux-kernel-device-driver-to-dma-from-a-device-into-user-space-memory
https://forums.xilinx.com/xlnx/attachments/xlnx/ELINUX/10693/1/Linux%2520DMA%2520from%2520User%2520Space-public.pdf
I haven't thought through all of the details, but I suspect the the mmap() function could be exteneded to allocate user-space DMA-able memory, at least under certain circumstances. The current NuttX mmap() function is, however, really on a stub and none of the more exotic mmap() functionality is implemented. I think it would have to use a scatter-gather DMA interface to be usable since mmap() does not guarantee physically contiguous allocations.
Any solution would be a LOT of work to fix something that is not broken.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] povauboin commented on a change in pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
povauboin commented on a change in pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338#discussion_r458757404
##########
File path: fsutils/mkfatfs/mkfatfs.c
##########
@@ -337,7 +338,11 @@ int mkfatfs(FAR const char *pathname, FAR struct fat_format_s *fmt)
/* Allocate a buffer that will be working sector memory */
+#ifdef CONFIG_FAT_DMAMEMORY
+ var.fv_sect = (FAR uint8_t *)fat_dma_alloc(var.fv_sectorsize);
+#else
Review comment:
Ah, this is why you removed it while moving to apps/ then.
But fat_dma_alloc is defined in https://github.com/apache/incubator-nuttx/blob/master/include/nuttx/fs/fat.h#L115
And there are many other apps that use something like #include <nuttx/*.h>
Anyway what do you suggest ? Is there a fix or I just remove the PR ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on a change in pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338#discussion_r458751928
##########
File path: fsutils/mkfatfs/mkfatfs.c
##########
@@ -337,7 +338,11 @@ int mkfatfs(FAR const char *pathname, FAR struct fat_format_s *fmt)
/* Allocate a buffer that will be working sector memory */
+#ifdef CONFIG_FAT_DMAMEMORY
+ var.fv_sect = (FAR uint8_t *)fat_dma_alloc(var.fv_sectorsize);
+#else
Review comment:
Sorry this cannot be merged Applications ares not permitted to call internal operating system interfaces. That violates the portable POSIX interface and will break all PROTECTED and KERNEL builds that use mkfatfs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338#issuecomment-662508173
> Some history: The fat_dma_alloc() OS internal function was added by the PX4 team years ago when they first started working with the STM32F407 family. Those parts had only 192Kb of SRAM: 128 Kb of DMA-able SRAM and 64Kb of non-DMA CCM.
>
> In order to make full use of the heterogeneous memory, the PX4 team put both memories into the heap. That means that you could randomly get DMA or non-DMA memory from kmm_malloc(). In order to work seamlessly with FAT, the PX4 added checks in FAT to determine if the memory supports DMA or not. If the memory is non-DMA-able, the FAT fs will fall back to non-DMA, PIO operations. They also added the specal fat_dma_alloc() allocator to allocate DMA memory (only within the OS).
>
Ok, it's reasonable now, but I think the better name is up_dma_alloc/up_dma_free and move to arch.h, so all file system can get supported equally. I am curious why PX4 team don't use bounce buffer like Linux internally so DMA can still be used:
https://lwn.net/Articles/91870/
> Many modern ARMv7-M do permit DMA from DTCM now and the need for this STM32F4-specific change is not as great. I would not recommend investing any further effort into this kind of logic now; for most platforms it is just not needed at all.
Yes, the good hardware design definitely save the programmer life:)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on a change in pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338#discussion_r458751928
##########
File path: fsutils/mkfatfs/mkfatfs.c
##########
@@ -337,7 +338,11 @@ int mkfatfs(FAR const char *pathname, FAR struct fat_format_s *fmt)
/* Allocate a buffer that will be working sector memory */
+#ifdef CONFIG_FAT_DMAMEMORY
+ var.fv_sect = (FAR uint8_t *)fat_dma_alloc(var.fv_sectorsize);
+#else
Review comment:
Sorry this cannot be merged Applications ares not permitted to call internal to call internal operating system interfaces. That violates the porttable POSIX interface and will break all PROTECTED and KERNEL biulds that use mkfatfs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo edited a comment on pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338#issuecomment-662477210
Linux has exactly the same issues. But people have developed Linux device drivers for allocating user-space DMA buffers. For example https://github.com/ikwzm/udmabuf
Something like that would be a POSIX compatible solution that should not get too much resistance.
Also related:
https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18842418/Linux+DMA+From+User+Space
https://stackoverflow.com/questions/5539375/linux-kernel-device-driver-to-dma-from-a-device-into-user-space-memory
I haven't thought through all of the details, but I suspect the the mmap() function could be exteneded to allocate user-space DMA-able memory, at least under certain circumstances. The current NuttX mmap() function is, however, really on a stub and none of the more exotic mmap() functionality is implemented. I think it would have to use a scatter-gather DMA interface to be usable since mmap() does not guarantee physically contiguous allocations.
Any solution would be a LOT of work to fix something that is not broken.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on a change in pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338#discussion_r458751928
##########
File path: fsutils/mkfatfs/mkfatfs.c
##########
@@ -337,7 +338,11 @@ int mkfatfs(FAR const char *pathname, FAR struct fat_format_s *fmt)
/* Allocate a buffer that will be working sector memory */
+#ifdef CONFIG_FAT_DMAMEMORY
+ var.fv_sect = (FAR uint8_t *)fat_dma_alloc(var.fv_sectorsize);
+#else
Review comment:
Sorry this cannot be merged Applications ares not permitted to call internal to call internal operating system interfaces. That violates the porttable POSIX interface and will break all PROTECTED and KERNAL biulds that use mkfatfs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338#issuecomment-662499712
Some history: The fat_dma_alloc() OS internal function was added by the PX4 team years ago when they first started working with the STM32F407 family. Those parts had only 192Kb of SRAM: 128 Kb of DMA-able SRAM and 64Kb of non-DMA CCM.
In order to make full use of the heterogeneous memory, the PX4 team put both memories into the heap. That means that you could randomly get DMA or non-DMA memory from kmm_malloc(). In order to work seamlessly with FAT, the PX4 added checks in FAT to determine if the memory supports DMA or not. If the memory is non-DMA-able, the FAT fs will fall back to non-DMA, PIO operations. They also added the specal fat_dma_alloc() allocator to allocate DMA memory (only within the OS).
Many modern ARMv7-M do permit DMA from DTCM now and the need for this STM32F4-specific change is not as great. I would not recommend investing any further effort into this kind of logic now; for most platforms it is just not needed at all.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338#issuecomment-662510936
> I am curious why PX4 team don't use bounce buffer like Linux internally so DMA can still be used:
> https://lwn.net/Articles/91870/
Probably because they use the SD card for recording flight data and were **very** concerned with with SD card performance. That is why they want user-space DMA buffers. User-space DMA buffers is a very bad idea architecturally and should not generally be considered.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338#issuecomment-662514916
>
>
> > I am curious why PX4 team don't use bounce buffer like Linux internally so DMA can still be used:
> > https://lwn.net/Articles/91870/
>
> Probably because they use the SD card for recording flight data and were **very** concerned with with SD card performance. That is why they want user-space DMA buffers. User-space DMA buffers is a very bad idea architecturally and should not generally be considered.
BTW, the FAT file system already supports something like a "bounce buffer" but only for the case where DMA is not possible or where the DMA fails:
626 #ifdef CONFIG_FAT_DIRECT_RETRY
627 /* The low-level driver may return -EFAULT in the case where
628 * the transfer cannot be performed due to buffer memory
629 * constraints. It is probable that the buffer is completely
630 * un-DMA-able or improperly aligned. In this case, force
631 * indirect transfers via the sector buffer and restart the
632 * operation (unless we have already tried that).
633 */
634
635 if (ret == -EFAULT && !force_indirect)
636 {
637 ferr("ERROR: DMA read alignment error,"
638 " restarting indirect\n");
639 force_indirect = true;
640 goto fat_read_restart;
641 }
642 #endif /* CONFIG_FAT_DIRECT_RETRY */
This "indirect" mode will copy the user data into a DMA-able sector buffer to do the DMA transfer.
All of the DMA-related corner cases are handled within the FAT file system (yes, not the right place). No special DMA memory operations are ever necessary in user-space.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on a change in pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338#discussion_r458839584
##########
File path: fsutils/mkfatfs/mkfatfs.c
##########
@@ -337,7 +338,11 @@ int mkfatfs(FAR const char *pathname, FAR struct fat_format_s *fmt)
/* Allocate a buffer that will be working sector memory */
+#ifdef CONFIG_FAT_DMAMEMORY
+ var.fv_sect = (FAR uint8_t *)fat_dma_alloc(var.fv_sectorsize);
+#else
Review comment:
>
>
> @povauboin the better fix is at SPI/DMA driver since the limitation origins from there. SPI/DMA driver should check whether the address is outside it's capability and if so it can utilize(alloc and copy) the special memory internally.
> The design of fat_dma_alloc/fat_dma_free is totally wrong:
> 1.How to handle other file system(e.g. NXFFS, SmartFS, LittleFS...)
> 2.How to handle the block level access
> We should remove them even inside the kernel.
The solution is already implemented inside the OS for the case of FAT with CONFIG_FAT_DMAMEMORY. This PR is unnecessary since nothing is broken; mkfatfs works just fine without DAM. With CONFIG_FAT_DMAMEMORY, DMA will not be used and the filesystem will simply not use it.
In the PX4 case, they do not use SPI for FAT, but an MCI card driver (SD/MMC). It already implements the DMA.
I agree that the design is wrong. Different peripherals, however, have different DMA requirements, however. For example, LPC17 requires some peripheral DMAs to be in one memory bank and other peripheral DMAs to be done in other memory banks. Some peripherals have different alignment requirements. Some DMA memory must be allocated from different memory pools.
Block level access still goes through the same peripheral drivers. I am not sure if it is possible to define a common DMA interface at the block driver level. But some peripheral interfaces define memory allocation and free methods that could be used at the block driver level for custom peripheral DMA memory allocations.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338#discussion_r458828914
##########
File path: fsutils/mkfatfs/mkfatfs.c
##########
@@ -337,7 +338,11 @@ int mkfatfs(FAR const char *pathname, FAR struct fat_format_s *fmt)
/* Allocate a buffer that will be working sector memory */
+#ifdef CONFIG_FAT_DMAMEMORY
+ var.fv_sect = (FAR uint8_t *)fat_dma_alloc(var.fv_sectorsize);
+#else
Review comment:
@povauboin the better fix is at SPI/DMA driver since the limitation origins from there. SPI/DMA driver should check whether the address is outside it's capability and if so it can utilize(alloc and copy) the special memory internally.
The design of fat_dma_alloc/fat_dma_free is totally wrong:
1.How to handle other file system(e.g. NXFFS, SmartFS, LittleFS...)
2.How to handle the block level access
We should remove them even inside the kernel.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338#issuecomment-662477210
Linux has exactly the same issues. But people have developed Linux device drivers for allocating user-space DMA buffers. For example https://github.com/ikwzm/udmabuf
Something like that would be a POSIX compatible solution that should not get too much resistance.
Also related:
https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18842418/Linux+DMA+From+User+Space
https://stackoverflow.com/questions/5539375/linux-kernel-device-driver-to-dma-from-a-device-into-user-space-memory
I haven't thought through all of the details, but I suspect the the mmap() function could be exteneded to allocate user-space DMA-able memory, at least under certain circumstances. The current NuttX mmap() function is, however, really on a stub and none of the more exotic mmap() functionality is implemented. I think it would have to use a scatter-gather DMA interface to be usable since mmap() does not guarantee physically contiguous allocations.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338#discussion_r458847182
##########
File path: fsutils/mkfatfs/mkfatfs.c
##########
@@ -337,7 +338,11 @@ int mkfatfs(FAR const char *pathname, FAR struct fat_format_s *fmt)
/* Allocate a buffer that will be working sector memory */
+#ifdef CONFIG_FAT_DMAMEMORY
+ var.fv_sect = (FAR uint8_t *)fat_dma_alloc(var.fv_sectorsize);
+#else
Review comment:
The simplest approach is using bouce buffer I suggest before:
https://lwn.net/Articles/91870/
but we lose some performance due to memory copy(still better than interrupt driven). Another complex approach is exposing dma buffer allocation as Greg's suggestion to userspace or block driver.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo closed pull request #338: fsutils/mkfatfs: use DMA memory for mkfatfs when needed
Posted by GitBox <gi...@apache.org>.
patacongo closed pull request #338:
URL: https://github.com/apache/incubator-nuttx-apps/pull/338
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org