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]

 




Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
phone: 301-407-9583
email: m-karicheri2@xxxxxx

>-----Original Message-----
>From: Magnus Damm [mailto:magnus.damm@xxxxxxxxx]
>Sent: Friday, December 04, 2009 6:06 AM
>To: Karicheri, Muralidharan
>Cc: linux-media@xxxxxxxxxxxxxxx; mchehab@xxxxxxxxxxxxx; davinci-linux-open-
>source@xxxxxxxxxxxxxxxxxxxx
>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-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.

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 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