Re: [PATCH] V4L - Fix videobuf_dma_contig_user_get() getting page aligned physical address

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi again Murali,

Thanks for your work on this.

On Thu, Dec 3, 2009 at 12:48 AM, Karicheri, Muralidharan
<m-karicheri2@xxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux