Re: [PATCH 2/9] [media] media: omap_vout: Convert omap_vout_uservirt_to_phys() to use get_vaddr_pfns()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

[-- Attachment #1: Type: text/plain, Size: 3581 bytes --]

Hi Tomi,

On Friday 12 June 2015 12:21:13 Tomi Valkeinen wrote:
> On 11/06/15 07:21, Laurent Pinchart wrote:
> > On Wednesday 10 June 2015 06:20:45 Mauro Carvalho Chehab wrote:
> >> From: Jan Kara <jack@suse.cz>
> >> 
> >> Convert omap_vout_uservirt_to_phys() to use get_vaddr_pfns() instead of
> >> hand made mapping of virtual address to physical address. Also the
> >> function leaked page reference from get_user_pages() so fix that by
> >> properly release the reference when omap_vout_buffer_release() is
> >> called.
> >> 
> >> Signed-off-by: Jan Kara <jack@suse.cz>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> [hans.verkuil@cisco.com: remove unused struct omap_vout_device *vout
> >> variable]
> >> 
> >> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> >> 
> >> diff --git a/drivers/media/platform/omap/omap_vout.c
> >> b/drivers/media/platform/omap/omap_vout.c index
> >> f09c5f17a42f..7feb6394f111
> >> 100644
> >> --- a/drivers/media/platform/omap/omap_vout.c
> >> +++ b/drivers/media/platform/omap/omap_vout.c
> >> @@ -195,46 +195,34 @@ static int omap_vout_try_format(struct
> >> v4l2_pix_format *pix) }
> >> 
> >>  /*
> >> 
> >> - * omap_vout_uservirt_to_phys: This inline function is used to convert
> >> user - * space virtual address to physical address.
> >> + * omap_vout_get_userptr: Convert user space virtual address to physical
> >> + * address.
> >> 
> >>   */
> >> 
> >> -static unsigned long omap_vout_uservirt_to_phys(unsigned long virtp)
> >> +static int omap_vout_get_userptr(struct videobuf_buffer *vb, u32 virtp,
> >> +				 u32 *physp)
> >> 
> >>  {
> >> 
> >> -	unsigned long physp = 0;
> >> -	struct vm_area_struct *vma;
> >> -	struct mm_struct *mm = current->mm;
> >> +	struct frame_vector *vec;
> >> +	int ret;
> >> 
> >>  	/* For kernel direct-mapped memory, take the easy way */
> >> 
> >> -	if (virtp >= PAGE_OFFSET)
> >> -		return virt_to_phys((void *) virtp);
> >> -
> >> -	down_read(&current->mm->mmap_sem);
> >> -	vma = find_vma(mm, virtp);
> >> -	if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
> >> -		/* this will catch, kernel-allocated, mmaped-to-usermode
> >> -		   addresses */
> >> -		physp = (vma->vm_pgoff << PAGE_SHIFT) + (virtp - vma->vm_start);
> >> -		up_read(&current->mm->mmap_sem);
> >> -	} else {
> >> -		/* otherwise, use get_user_pages() for general userland pages */
> >> -		int res, nr_pages = 1;
> >> -		struct page *pages;
> >> +	if (virtp >= PAGE_OFFSET) {
> >> +		*physp = virt_to_phys((void *)virtp);
> > 
> > Lovely. virtp comes from userspace and as far as I know it arrives here
> > completely unchecked. The problem isn't introduced by this patch, but
> > omap_vout buffer management seems completely broken to me, and nobody
> > seems to care about the driver. Given that omapdrm should now provide the
> > video output capabilities that are missing from omapfb and resulted in
> > the development of omap_vout, shouldn't we drop the omap_vout driver ?
> > 
> > Tomi, any opinion on this ? Do you see any omap_vout capability missing
> > from omapdrm ?
> 
> I've never used omap_vout, so I don't even know what it offers. I do
> know it supports VRFB rotation (omap3), which we don't have on omapdrm,
> though. Whether anyone uses that (or omap_vout), I have no idea...
> 
> I'd personally love to get rid of omap_vout, as it causes complications
> on omapfb/omapdss side.

How difficult would it be to implement VRFB rotation in omapdrm ?

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]