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
