Em Thu, 17 Apr 2014 00:33:55 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> On 04/17/2014 12:23 AM, Mauro Carvalho Chehab wrote:
> > Em Mon, 10 Mar 2014 13:20:49 +0100
> > Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> >
> >> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >>
> >> Convert the saa7134 driver to vb2.
> >>
> >> Note that while this uses the vb2-dma-sg version, the VB2_USERPTR mode is
> >> disabled. The DMA hardware only supports DMAing full pages, and in the
> >> USERPTR memory model the first and last scatter-gather buffer is almost
> >> never a full page.
> >>
> >> In practice this means that we can't use the VB2_USERPTR mode.
> >
> > Why not? Provided that the buffer is equal or bigger than the number of
> > pages required by saa7134, that should be OK.
> >
> > All the driver needs to do is to check if the USERPTR buffer condition is met,
> > returning an error otherwise (and likely printing a msg at dmesg).
>
> Yuck. Well, I'll take a look at this.
>
> It has in my view the same problem as abusing USERPTR to pass pointers to
> physically contiguous memory: yes, it 'supports' USERPTR, but it has additional
> requirements which userspace has no way of knowing or detecting.
>
> It's really not USERPTR at all, it is PAGE_ALIGNED_USERPTR.
>
> Quite different.
Hmm... If I remember well, mmapped memory (being userptr or not) are always
page aligned, at least on systems with MMU.
> I would prefer that you have to enable it explicitly through e.g. a module option.
> That way you can still do it, but you really have to know what you are doing.
>
> > I suspect that this change will break some userspace programs used
> > for video surveillance equipment.
> >
> >> This has been tested with raw video, compressed video, VBI, radio, DVB and
> >> video overlays.
> >>
> >> Unfortunately, a vb2 conversion is one of those things you cannot split
> >> up in smaller patches, it's all or nothing. This patch switches the whole
> >> driver over to vb2, using the vb2 ioctl and fop helper functions.
> >
> > Not quite true. This patch contains lots of non-vb2 stuff, like:
> > - Coding Style fixes;
> > - Removal of res_get/res_set/res_free;
> > - Functions got moved from one place to another one.
>
> I will see if there is anything sensible that I can split up. I'm not aware
> of any particular coding style issues, but I'll review it.
There are several, like:
- dprintk("buffer_finish %p\n",q->curr);
+ dprintk("buffer_finish %p\n", q->curr);
Also, it seems that you moved some functions, like:
ts_reset_encoder(struct saa7134_dev* dev) that was moved
to some other part of the code and renamed as stop_streaming().
There are several of such cases, with makes hard to really see the
VB2 changes, and what it might be some code dropped by mistake.
>
> The removal of the resource functions is not something I can split up. It
> is replaced by the resource handling that's built into the vb2 helper functions.
Well, currently, it is really hard to see that all the checks between
empress and normal video streams are still done right, as the patch
become big and messy.
Please try to break it into a more granular set of patches that
would help to check if everything is there.
Thanks,
Mauro
>
> Regards,
>
> Hans
>
> >
> > It is really hard to review it, as is, as the real changes are mixed with
> > the above code cleanups/changes.
> >
> > Please split this patch in a way that it allows reviewing the changes
> > there.
>
--
Regards,
Mauro
--
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
