Re: [PATCH] V4L - Fix videobuf_dma_contig_user_get() getting page aligned physical address
Hi again Murali, Thanks for your work on this. On Thu, Dec 3, 2009 at 12:48 AM, Karicheri, Muralidharan m-kariche...@ti.com wrote: Magnus, Thanks for the patch. For non-page aligned user space pointers I agree that a fix is needed. Don't you think the while loop in videobuf_dma_contig_user_get() also needs to be adjusted to include the last page? I think the while loop checks one page too little in the non-aligned case today. Thanks for reviewing my patch. It had worked for non-aligned address in my testing. If I understand this code correctly, the physical address of the user page start is determined in the first loop (pages_done == 0) and additional loops are run to make sure the memory is physically contiguous. Initially the mem-size is set to number of pages aligned to page size. Assume we pass 4097 bytes as size. mem-size = PAGE_ALIGN(vb-size); = 2 Inside the loop, iteration is done for 0 to pages-1. pages_done (mem-size 12) = pages_done 2 = iterate 2 times For size of 4096, we iterate once. For size of 4095, we iterate once. So IMO the loop is already iterate one more time when we pass non-aligned address since size is aligned to include the last page. So based on this could you ack my patch so that we could ask Mauro to merge it with priority? I think your observations are correct, but I also think there is one more hidden issue. In the case where the offset within the page is other than 0 then we should loop once more to also check the final page. Right now no one is checking if the last page is contiguous or not in the case on non-page-aligned offset.. So in your case with a 4096 or 4095 size, but if the offset withing the page is non-zero then we should loop twice to make sure the pages really are physically contiguous. Today we only loop once based on the size. We should also include the offset in the calculation of number of pages to check. If you can include that fix in your patch that would be great. If not then i'll fix it up myself. Thanks! / magnus -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] V4L - Fix videobuf_dma_contig_user_get() getting page aligned physical address
Murali Karicheri Software Design Engineer Texas Instruments Inc. Germantown, MD 20874 phone: 301-407-9583 email: m-kariche...@ti.com -Original Message- From: Magnus Damm [mailto:magnus.d...@gmail.com] Sent: Friday, December 04, 2009 6:06 AM To: Karicheri, Muralidharan Cc: linux-media@vger.kernel.org; mche...@infradead.org; davinci-linux-open- sou...@linux.davincidsp.com Subject: Re: [PATCH] V4L - Fix videobuf_dma_contig_user_get() getting page aligned physical address Hi again Murali, Thanks for your work on this. On Thu, Dec 3, 2009 at 12:48 AM, Karicheri, Muralidharan m-kariche...@ti.com wrote: Magnus, Thanks for the patch. For non-page aligned user space pointers I agree that a fix is needed. Don't you think the while loop in videobuf_dma_contig_user_get() also needs to be adjusted to include the last page? I think the while loop checks one page too little in the non-aligned case today. Thanks for reviewing my patch. It had worked for non-aligned address in my testing. If I understand this code correctly, the physical address of the user page start is determined in the first loop (pages_done == 0) and additional loops are run to make sure the memory is physically contiguous. Initially the mem-size is set to number of pages aligned to page size. Assume we pass 4097 bytes as size. mem-size = PAGE_ALIGN(vb-size); = 2 Inside the loop, iteration is done for 0 to pages-1. pages_done (mem-size 12) = pages_done 2 = iterate 2 times For size of 4096, we iterate once. For size of 4095, we iterate once. So IMO the loop is already iterate one more time when we pass non-aligned address since size is aligned to include the last page. So based on this could you ack my patch so that we could ask Mauro to merge it with priority? I think your observations are correct, but I also think there is one more hidden issue. In the case where the offset within the page is other than 0 then we should loop once more to also check the final page. Right now no one is checking if the last page is contiguous or not in the case on non-page-aligned offset.. So in your case with a 4096 or 4095 size, but if the offset withing the page is non-zero then we should loop twice to make sure the pages really are physically contiguous. Today we only loop once based on the size. We should also include the offset in the calculation of number of pages to check. Yes. You are right. For offsets that are non-aligned we need to check for the last one. Probably unsigned int offset = vb-baddr ~PAGE_MASK; mem-size = PAGE_ALIGN(vb-size + offset); .. if (pages_done == 0) mem-handle = (this_pfn PAGE_SHIFT) + offset; If this is fine, I can send you a updated patch. If you can fix it that is fine too. regards, Murali If you can include that fix in your patch that would be great. If not then i'll fix it up myself. If you could do this it will be great! Thanks! / magnus -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] V4L - Fix videobuf_dma_contig_user_get() getting page aligned physical address
Magnus, Thanks for the patch. For non-page aligned user space pointers I agree that a fix is needed. Don't you think the while loop in videobuf_dma_contig_user_get() also needs to be adjusted to include the last page? I think the while loop checks one page too little in the non-aligned case today. Cheers, / magnus Thanks for reviewing my patch. It had worked for non-aligned address in my testing. If I understand this code correctly, the physical address of the user page start is determined in the first loop (pages_done == 0) and additional loops are run to make sure the memory is physically contiguous. Initially the mem-size is set to number of pages aligned to page size. Assume we pass 4097 bytes as size. mem-size = PAGE_ALIGN(vb-size); = 2 Inside the loop, iteration is done for 0 to pages-1. pages_done (mem-size 12) = pages_done 2 = iterate 2 times For size of 4096, we iterate once. For size of 4095, we iterate once. So IMO the loop is already iterate one more time when we pass non-aligned address since size is aligned to include the last page. So based on this could you ack my patch so that we could ask Mauro to merge it with priority? Murali Karicheri Software Design Engineer Texas Instruments Inc. Germantown, MD 20874 phone: 301-407-9583 email: m-kariche...@ti.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] V4L - Fix videobuf_dma_contig_user_get() getting page aligned physical address
From: Muralidharan Karicheri m-kariche...@ti.com If a USERPTR address that is not aligned to page boundary is passed to the videobuf_dma_contig_user_get() function, it saves a page aligned address to the dma_handle. This is not correct. This issue is observed when using USERPTR IO machism for buffer exchange. Signed-off-by: Muralidharan Karicheri m-kariche...@ti.com --- Applies to V4L-DVB linux-next branch drivers/media/video/videobuf-dma-contig.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/media/video/videobuf-dma-contig.c b/drivers/media/video/videobuf-dma-contig.c index d25f284..928dfa1 100644 --- a/drivers/media/video/videobuf-dma-contig.c +++ b/drivers/media/video/videobuf-dma-contig.c @@ -166,7 +166,8 @@ static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem, break; if (pages_done == 0) - mem-dma_handle = this_pfn PAGE_SHIFT; + mem-dma_handle = (this_pfn PAGE_SHIFT) + + (vb-baddr ~PAGE_MASK); else if (this_pfn != (prev_pfn + 1)) ret = -EFAULT; -- 1.6.0.4 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
