On Tue July 31 2012 22:17:06 Federico Vaga wrote:
> As suggested I moved the Video Buffer Input (VIP) of the STA2X11 board to the
> videobuf2. This patch series is an RFC.
Thank you very much for working on this! Much appreciated!
> The first patch is just an update to the adv7180 because the VIP (the only
> user) now use the control framework so query{g_|s_|ctrl} are not necessery.
For this patch:
Acked-by: Hans Verkuil <[email protected]>
> The second patch adds a new memory allocator for the videobuf2. I name it
> videobuf2-dma-streaming but I think "streaming" is not the best choice, so
> suggestions are welcome. My inspiration for this buffer come from
> videobuf-dma-contig (cached) version. After I made this buffer I found the
> videobuf2-dma-nc made by Jonathan Corbet and I improve the allocator with
> some suggestions (http://patchwork.linuxtv.org/patch/7441/). The VIP doesn't
> work with videobu2-dma-contig and I think this solution is easier the sg.
I leave this to the vb2 experts. It's not obvious to me why we would need
a fourth memory allocator.
> The third patch updates the VIP to videobuf2 and control framework. I made also
> some restyling to the driver and change some mechanism so I take the ownership
> of the driver and I add the copyright of ST Microelectronics. Some trivial
> code is unchanged. The patch probably needs some extra update.
> I add the control framework to the VIP but without any control. I add it to
> inherit controls from adv7180.
Did you run the latest v4l2-compliance tool from the v4l-utils.git repository
over your driver? I'm sure you didn't since VIP is missing support for control
events and v4l2-compliance would certainly complain about that.
Always check with v4l2-compliance whenever you make changes! It's continuously
improved as well, so a periodic check wouldn't hurt.
Also take a look at the new vb2 helper functions in media/videobuf2-core.h:
it is likely that you can use those to simplify your driver. They are used in
e.g. vivi, so take a look there.
Regards,
Hans
On Wed, 1 Aug 2012 08:41:56 +0200
Hans Verkuil <[email protected]> wrote:
> > The second patch adds a new memory allocator for the videobuf2. I name it
> > videobuf2-dma-streaming but I think "streaming" is not the best choice, so
> > suggestions are welcome. My inspiration for this buffer come from
> > videobuf-dma-contig (cached) version. After I made this buffer I found the
> > videobuf2-dma-nc made by Jonathan Corbet and I improve the allocator with
> > some suggestions (http://patchwork.linuxtv.org/patch/7441/). The VIP doesn't
> > work with videobu2-dma-contig and I think this solution is easier the sg.
>
> I leave this to the vb2 experts. It's not obvious to me why we would need
> a fourth memory allocator.
I first wrote my version after observing that performance dropped by a
factor of three on the OLPC XO 1.75 when using contiguous, coherent
memory. If the architecture needs to turn off caching, things really slow
down, to the point that it's unusable. There's no real reason why a V4L2
device shouldn't be able to use streaming mappings in this situation; it
performs better and doesn't eat into the limited amounts of coherent DMA
space available on some systems.
I never got my version into a mergeable state only because I finally
figured out how to bludgeon the hardware into doing s/g DMA and didn't
need it anymore. But I think it's a worthwhile functionality to have,
and, with CMA, it could be the preferred mode for a number of devices.
jon
Hi Hans,
> Did you run the latest v4l2-compliance tool from the v4l-utils.git
> repository over your driver? I'm sure you didn't since VIP is missing
> support for control events and v4l2-compliance would certainly
> complain about that.
>
> Always check with v4l2-compliance whenever you make changes! It's
> continuously improved as well, so a periodic check wouldn't hurt.
I applied all your suggestions, and some extra simplification; now I'm
running v4l2-compliance but I have this error:
Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
fail: v4l2-compliance.cpp(322): doioctl(node,
VIDIOC_G_PRIORITY, &prio)
test VIDIOC_G/S_PRIORITY: FAIL
which I don't undestand. I don't have vidio_{g|s}_priority functions in
my implementation. And I'm using the V4L2_FL_USE_FH_PRIO flag as
suggested in the documentation:
---------------
- flags: optional. Set to V4L2_FL_USE_FH_PRIO if you want to let the
framework handle the VIDIOC_G/S_PRIORITY ioctls. This requires that you
use struct v4l2_fh. Eventually this flag will disappear once all drivers
use the core priority handling. But for now it has to be set explicitly.
--------------
--
Federico Vaga
On Sun August 5 2012 19:11:19 Federico Vaga wrote:
> Hi Hans,
>
> > Did you run the latest v4l2-compliance tool from the v4l-utils.git
> > repository over your driver? I'm sure you didn't since VIP is missing
> > support for control events and v4l2-compliance would certainly
> > complain about that.
> >
> > Always check with v4l2-compliance whenever you make changes! It's
> > continuously improved as well, so a periodic check wouldn't hurt.
>
> I applied all your suggestions, and some extra simplification; now I'm
> running v4l2-compliance but I have this error:
>
>
> Allow for multiple opens:
> test second video open: OK
> test VIDIOC_QUERYCAP: OK
> fail: v4l2-compliance.cpp(322): doioctl(node,
> VIDIOC_G_PRIORITY, &prio)
> test VIDIOC_G/S_PRIORITY: FAIL
>
>
> which I don't undestand. I don't have vidio_{g|s}_priority functions in
> my implementation. And I'm using the V4L2_FL_USE_FH_PRIO flag as
> suggested in the documentation:
>
> ---------------
> - flags: optional. Set to V4L2_FL_USE_FH_PRIO if you want to let the
> framework handle the VIDIOC_G/S_PRIORITY ioctls. This requires that you
> use struct v4l2_fh.
^^^^^^^^^^^^^^^^^^
Are you using struct v4l2_fh? The version you posted didn't. You need this
anyway to implement control events.
Regards,
Hans
> Eventually this flag will disappear once all drivers
> use the core priority handling. But for now it has to be set explicitly.
> --------------
>
>
> > I applied all your suggestions, and some extra simplification;
> > [...]
> > ---------------
> > - flags: optional. Set to V4L2_FL_USE_FH_PRIO if you want to let the
> > framework handle the VIDIOC_G/S_PRIORITY ioctls. This requires that
> > you use struct v4l2_fh.
>
> ^^^^^^^^^^^^^^^^^^
>
> Are you using struct v4l2_fh? The version you posted didn't. You need
> this anyway to implement control events.
Yes I'm using it now, it is part of the extra simplification that I did.
--
Federico Vaga
On Mon August 6 2012 09:38:10 Federico Vaga wrote:
>
> > > I applied all your suggestions, and some extra simplification;
> > > [...]
>
> > > ---------------
> > > - flags: optional. Set to V4L2_FL_USE_FH_PRIO if you want to let the
> > > framework handle the VIDIOC_G/S_PRIORITY ioctls. This requires that
> > > you use struct v4l2_fh.
> >
> > ^^^^^^^^^^^^^^^^^^
> >
> > Are you using struct v4l2_fh? The version you posted didn't. You need
> > this anyway to implement control events.
>
> Yes I'm using it now, it is part of the extra simplification that I did.
In that case I need to see your latest version of the source code to see
why it doesn't work.
Regards,
Hans
> In that case I need to see your latest version of the source code to
> see why it doesn't work.
I send it as patch v2 of the previous one
--
Federico Vaga
In data mercoled? 1 agosto 2012 07:04:18, Jonathan Corbet ha scritto:
> On Wed, 1 Aug 2012 08:41:56 +0200
>
> Hans Verkuil <[email protected]> wrote:
> > > The second patch adds a new memory allocator for the videobuf2. I
> > > name it videobuf2-dma-streaming but I think "streaming" is not
> > > the best choice, so suggestions are welcome. My inspiration for
> > > this buffer come from videobuf-dma-contig (cached) version. After
> > > I made this buffer I found the videobuf2-dma-nc made by Jonathan
> > > Corbet and I improve the allocator with some suggestions
> > > (http://patchwork.linuxtv.org/patch/7441/). The VIP doesn't work
> > > with videobu2-dma-contig and I think this solution is easier the
> > > sg.>
> > I leave this to the vb2 experts. It's not obvious to me why we would
> > need a fourth memory allocator.
>
> I first wrote my version after observing that performance dropped by a
> factor of three on the OLPC XO 1.75 when using contiguous, coherent
> memory. If the architecture needs to turn off caching, things really
> slow down, to the point that it's unusable. There's no real reason
> why a V4L2 device shouldn't be able to use streaming mappings in this
> situation; it performs better and doesn't eat into the limited
> amounts of coherent DMA space available on some systems.
>
> I never got my version into a mergeable state only because I finally
> figured out how to bludgeon the hardware into doing s/g DMA and didn't
> need it anymore. But I think it's a worthwhile functionality to
> have, and, with CMA, it could be the preferred mode for a number of
> devices.
>
> jon
I think that the memory allocator is the most questionable patch, but if
there are not any other comments I will send my three patches for the
inclusion. It is summer, time for vacation, so I'll wait for another
week or two for critical comments and then I will send patches.
--
Federico Vaga