RE: [RFC PATCH v2 4/8] media: videobuf2: introduce VIDEOBUF2_PAGE memops

From: Marek Szyprowski <m.szyprowski@samsung.com>
To: "'Ming Lei'" <ming.lei@canonical.com>
Cc: "'Mauro Carvalho Chehab'" <mchehab@infradead.org>,
	"'Tony Lindgren'" <tony@atomide.com>,
	"'Sylwester Nawrocki'" <snjw23@gmail.com>,
	"'Alan Cox'" <alan@lxorguk.ukuu.org.uk>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
	"'Pawel Osciak'" <p.osciak@gmail.com>
Subject: RE: [RFC PATCH v2 4/8] media: videobuf2: introduce VIDEOBUF2_PAGE memops
Date: Tue, 10 Jan 2012 11:20:32 +0100	[thread overview]
Message-ID: <013701cccf81$7c0cdb90$742692b0$%szyprowski@samsung.com> (raw)
In-Reply-To: <CACVXFVMrRTS7TUtj7bqCWeF4zx11yT6mOq4syOkZv=Ejoo0LMw@mail.gmail.com>

Hello,

On Friday, December 23, 2011 1:21 PM Ming Lei wrote:

> >> >> > Your current implementation also abuses the design and api of videobuf2 memory
> >> >> > allocators. If the allocator needs to return a custom structure to the driver
> >> >>
> >> >> I think returning vaddr is enough.
> >> >>
> >> >> > you should use cookie method. vaddr is intended to provide only a pointer to
> >> >> > kernel virtual mapping, but you pass a struct page * there.
> >> >>
> >> >> No, __get_free_pages returns virtual address instead of 'struct page *'.
> >> >
> >> > Then you MUST use cookie for it. vaddr method should return kernel virtual address
> >> > to the buffer video data. Some parts of videobuf2 relies on this - it is used by file
> >> > io emulator (read(), write() calls) and mmap equivalent for non-mmu systems.
> >> >
> >> > Manual casting in the driver is also a bad idea, that's why there are helper functions
> >>
> >> I don't see any casts are needed. The dma address can be got from vaddr with
> >> dma_map_* easily in drivers, see the usage on patch 8/8(media: video: introduce
> >> omap4 face detection module driver).
> >
> > Sorry, but I won't accept such driver/allocator which abuses the defined API. I've already
> > pointed what vaddr method is used for.
> 
> Sorry, could you describe the abuse problem in a bit detail?

Videobuf2 requires memory module handlers to provide vaddr method to provide a pointer in 
kernel virtual address space to video data (buffer content). It is used for example by 
read()/write() io method emulator. Memory allocator/handler should not be specific to any
particular use case in the device driver. That's the design. Simple.

I your case you want to give pointer to struct page from the memory allocator to the 
driver. The cookie method has been introduced exactly for this purpose. Memory allocator
also provides a simple inline function to convert generic 'void *' return type from cookie
method to allocator specific structure/pointer. vb2_dma_contig_plane_dma_addr() and 
vb2_dma_sg_plane_desc() were examples how does passing allocator specific type though the
cookie method works.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center