DM81xx - CMEM : patch attached for _pfn() to be included in the next Linux utils 3.2x.xx.xx

Hi,

I did the original patch and apparently the team is looking for a pass / fail test.

The issue this code fixes is trying to get the physical address of a video4linux2 mmap'ed video output buffer to pass directly to the DSP on a DM816x running on PSP 04.00.01.13.

The attached test program demonstrates the issue, compile it with -DUSE_CMEM and linking to the CMEM user lib. Without the patch the call to CMEM_getPhys() fails with a message "warning: failed getting physical address of video buffer ...", with the patch it succeeds and provides the correct physical address of the mmap'ed video output buffer in a message "Physical address of buffer ...".

You will probably need to adjust the compile line for your environment, but it should compile cleanly.

Note also that besides making the cmemk driver work in this scenario using follow_pfn() is much more robust than using the vm_pgoff of the vma and assuming it always contains the PFN and that the mapping is linear, as follow_pfn() uses the actual page tables to get the physical address and is part of the exported driver API.

Best,

Diego

Diego,

Thankyou very much for the enhancement to CMEM, and the test case.  We will evaluate this change and update CMEM with it should the change prove appropriate.

I do have a question about the enhancement, though.  You mention that today's existing method "assumes that vma->vm_pgoff contains the PFN and that the mapping is linear" (I paraphrased your text a little).  That word "linear" concerns me, since code calling CMEM_getPhys() is used in situations where the physical address is given to a "device" that needs contiguous physical memory.  If underlying physical memory of a contiguous virtual address range is not contiguous itself, then the "device" won't be able to take the base physical address and assume it's linear.  Does modifying this code to use follow_pfn() accomodate non-contiguous physical memory?

CMEM_getPhys() probably shouldn't succeed for non-contiguous underlying physical memory.

Regards,

- Rob

Rob,

I agree with you that CMEM_getPhys() is intended to be used with devices that use contiguous physical memory for their buffers, hence the mapping will be linear. Nevertheless, for devices that would not use contiguous physical memory you could still use CMEM_getPhys() to get the physical address of each page, in which case the user would need to call it for each page of the virtual mapping to get a correct and meaningful result. That being said, I do not know of any such drivers where CMEM_getPhys() would be useful. In any case, using follow_pfn() is more robust in that it is the kernel mm code that does the lookup and that function is meant to work reliably with user space mappings done with remap_pfn_range() and friends and return a proper error code otherwise.

On a side note, I should point out that using get_user_pages() in get_phys() is flawed (as is currently used when all else fails in get_phys()), since that would lock the page in RAM indefinitely as there is no matching page_cache_release() call. Note that get_user_pages() will succeed for user space mappings (e.g., mmap'ed file) which are initially swappable and should become swappable again when done with the buffer.

For cases where the physical address of an arbitrary user space mapping is necessary we have another patch which adds CMEM_share() and CMEM_unshare(). We use it for instance to share font files with the DSP. The font files are mmap'ed in the application (Linux side) and then we call CMEM_share() to get the physical address of each page in the virtual mapping, which we then pass to the DSP for it to use the font data. When the DSP no longer requires the font we call CMEM_unshare() which will unlock the pages in the region. It uses get_user_pages() and page_cache_release() to properly lock the pages while shared and unlock them when done. It also has proper reference counting so even a crashing app will not leave the pages indefinitely locked. As this patch is only loosely related to contiguous buffers it is not certain it really belongs in CMEM. Anyhow, if you are interested I can also push this patch.

Best,

Diego