Re: [PATCH] staging/vc04_services/bcm2835-camera: distinct numeration and names for devices

[Dave Stevenson]

Hi Hans

On Fri, 10 Jan 2020 at 13:25, Hans Verkuil  wrote:
>
> Hi Michael, Kay,
>
> On 12/6/19 9:54 AM, Michael Kupfer wrote:
> > Create a static atomic counter for numerating cameras.
> > Use the Media Subsystem Kernel Internal API to create distinct
> > device-names, so that the camera-number (given by the counter)
> > matches the camera-name.
> >
> > Co-developed-by: Kay Friedrich 
> > Signed-off-by: Kay Friedrich 
> > Signed-off-by: Michael Kupfer 
> > ---
> >  .../vc04_services/bcm2835-camera/bcm2835-camera.c| 9 ++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c 
> > b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
> > index beb6a0063bb8..be5f90a8b49d 100644
> > --- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
> > +++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
> > @@ -60,6 +60,9 @@ MODULE_PARM_DESC(max_video_width, "Threshold for video 
> > mode");
> >  module_param(max_video_height, int, 0644);
> >  MODULE_PARM_DESC(max_video_height, "Threshold for video mode");
> >
> > +/* camera instance counter */
> > +static atomic_t camera_instance = ATOMIC_INIT(0);
> > +
> >  /* global device data array */
> >  static struct bm2835_mmal_dev *gdev[MAX_BCM2835_CAMERAS];
> >
> > @@ -1870,7 +1873,6 @@ static int bcm2835_mmal_probe(struct platform_device 
> > *pdev)
> >
> >   /* v4l2 core mutex used to protect all fops and v4l2 ioctls. 
> > */
> >   mutex_init(&dev->mutex);
> > - dev->camera_num = camera;
> >   dev->max_width = resolutions[camera][0];
> >   dev->max_height = resolutions[camera][1];
> >
> > @@ -1886,8 +1888,9 @@ static int bcm2835_mmal_probe(struct platform_device 
> > *pdev)
> >   dev->capture.fmt = &formats[3]; /* JPEG */
> >
> >   /* v4l device registration */
> > - snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name),
> > -  "%s", BM2835_MMAL_MODULE_NAME);
> > + dev->camera_num = v4l2_device_set_name(&dev->v4l2_dev,
> > +
> > BM2835_MMAL_MODULE_NAME,
> > +&camera_instance);
> >   ret = v4l2_device_register(NULL, &dev->v4l2_dev);
> >   if (ret) {
> >   dev_err(&pdev->dev, "%s: could not register V4L2 
> > device: %d\n",
> >
>
> Actually, in this specific case I would not use v4l2_device_set_name().
>
> Instead just use:
>
> snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name),
>  "%s-%u", BM2835_MMAL_MODULE_NAME, camera);
>
> It would be even better if there would be just one top-level v4l2_device used
> for all the camera instances. After all, there really is just one platform
> device for all of the cameras, and I would expect to see just a single
> v4l2_device as well.
>
> It doesn't hurt to have multiple v4l2_device structs, but it introduces a
> slight memory overhead since one would have been sufficient.

Doesn't that make all controls for all cameras common? The struct
v4l2_ctrl_handler is part of struct v4l2_device.

Or do we:
- ditch the use of ctrl_handler in struct v4l2_device
- create and initialise a ctrl_handler per camera on an internal
structure so we retain the control state
- assign ctrl_handler in struct v4l2_fh to it every time a file handle
on the device is opened?

And if we only have one struct v4l2_device then is there the
possibility of the unique names that Michael and Kay are trying to
introduce?

I'm a little confused as to whether there really is a gain in having a
single v4l2_device. In this case the two cameras are independent
devices, even if they are loaded by a single platform driver.

  Dave

> v4l2_device_set_name() is meant for pci-like devices. And it really
> is a bit overkill to have it as a helper function.
>
> Regards,
>
> Hans
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/31] staging: bcm2835-camera: Improvements

[Dave Stevenson]

Hi Stefan

On Fri, 28 Jun 2019 at 17:57, Stefan Wahren  wrote:
>
> Hi Hans,
>
> Am 28.06.19 um 10:06 schrieb Hans Verkuil:
> > Hi Stefan,
> >
> > On 6/27/19 8:55 PM, Stefan Wahren wrote:
> >> This is an attempt to help Dave Stevenson to get all the fixes and
> >> improvements of the bcm2835-camera driver into mainline.
> >>
> >> Mostly i only polished the commit logs for upstream.
> >>
> >> The series based on the latest bugfix V2 of staging: bcm2835-camera: Resto=
> >> re
> >> return behavior of ctrl_set_bitrate().
> > Thank you for working on this.
> >
> > Three high-level questions:
> >
> > 1) Can you post the output of 'v4l2-compliance -s' using the latest 
> > v4l2-compliance
> >from https://git.linuxtv.org/v4l-utils.git ?  I'm interested to see what 
> > the
> >status is of this driver w.r.t. the compliance tests.
>
> Before this series (Raspberry Pi 3, Camera 1.3, Linux
> 5.2.0-rc3-next-20190607, multi_v7_defconfig):
>
> v4l2-compliance SHA: b16f9e945d74aa552abdd6f873821cb77faaf13a, 32 bits
>
> Compliance test for bm2835 mmal device /dev/video0:
>
> Driver Info:
> Driver name  : bm2835 mmal
> Card type: mmal service 16.1
> Bus info : platform:bcm2835-v4l2
> Driver version   : 5.2.0
> Capabilities : 0x8525
> Video Capture
> Video Overlay
> Read/Write
> Streaming
> Extended Pix Format
> Device Capabilities
> Device Caps  : 0x0525
> Video Capture
> Video Overlay
> Read/Write
> Streaming
> Extended Pix Format
>
> Required ioctls:
> test VIDIOC_QUERYCAP: OK
>
> Allow for multiple opens:
> test second /dev/video0 open: OK
> test VIDIOC_QUERYCAP: OK
> test VIDIOC_G/S_PRIORITY: OK
> test for unlimited opens: OK
>
> Debug ioctls:
> test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
> test VIDIOC_LOG_STATUS: OK
>
> Input ioctls:
> test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
> test VIDIOC_ENUMAUDIO: OK (Not Supported)
> test VIDIOC_G/S/ENUMINPUT: OK
> test VIDIOC_G/S_AUDIO: OK (Not Supported)
> Inputs: 1 Audio Inputs: 0 Tuners: 0
>
> Output ioctls:
> test VIDIOC_G/S_MODULATOR: OK (Not Supported)
> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> test VIDIOC_ENUMAUDOUT: OK (Not Supported)
> test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
> test VIDIOC_G/S_AUDOUT: OK (Not Supported)
> Outputs: 0 Audio Outputs: 0 Modulators: 0
>
> Input/Output configuration ioctls:
> test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
> test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
> test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
> test VIDIOC_G/S_EDID: OK (Not Supported)
>
> Control ioctls (Input 0):
> test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
> test VIDIOC_QUERYCTRL: OK
> test VIDIOC_G/S_CTRL: OK
> test VIDIOC_G/S/TRY_EXT_CTRLS: OK
> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
> test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
> Standard Controls: 33 Private Controls: 0
>
> Format ioctls (Input 0):
> test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> test VIDIOC_G/S_PARM: OK
> test VIDIOC_G_FBUF: OK
> test VIDIOC_G_FMT: OK
> test VIDIOC_TRY_FMT: OK
> test VIDIOC_S_FMT: OK
> test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> test Cropping: OK (Not Supported)
> test Composing: OK (Not Supported)
> test Scaling: OK
>
> Codec ioctls (Input 0):
> test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
> test VIDIOC_G_ENC_INDEX: OK (Not Supported)
> test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
>
> Buffer ioctls (Input 0):
> fail: v4l2-test-buffers.cpp(715): q.create_bufs(node, 1, &fmt)
> != EINVAL
> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL
> test VIDIOC_EXPBUF: OK (Not Supported)
> test Requests: OK (Not Supported)
>
> Test input 0:
>
> Streaming ioctls:
> test read/write: OK
> fail: v4l2-test-buffers.cpp(2145): node->streamon(q.g_type())
> fail: v4l2-test-buffers.cpp(2224): testBlockingDQBuf(node, q)
> test blocking wait: FAIL
> fail: v4l2-test-buffers.cpp(1294): q.create_bufs(node, 1, &fmt)
> != EINVAL
> test MMAP (no poll): FAIL
> fail: v4l2-test-buffers.cpp(1294): q.create_bufs(node, 1, &fmt)
> != EINVAL
> test MMA

Re: [PATCH 00/31] staging: bcm2835-camera: Improvements

[Dave Stevenson]

Hi Stefan

Firstly a huge thank you for picking this up - it's been on my to-do
list for ages, and just hasn't made it to the top.

On Fri, 28 Jun 2019 at 09:06, Hans Verkuil  wrote:
>
> Hi Stefan,
>
> On 6/27/19 8:55 PM, Stefan Wahren wrote:
> > This is an attempt to help Dave Stevenson to get all the fixes and
> > improvements of the bcm2835-camera driver into mainline.
> >
> > Mostly i only polished the commit logs for upstream.
> >
> > The series based on the latest bugfix V2 of staging: bcm2835-camera: Resto=
> > re
> > return behavior of ctrl_set_bitrate().
>
> Thank you for working on this.
>
> Three high-level questions:
>
> 1) Can you post the output of 'v4l2-compliance -s' using the latest 
> v4l2-compliance
>from https://git.linuxtv.org/v4l-utils.git ?  I'm interested to see what 
> the
>status is of this driver w.r.t. the compliance tests.

Hi Hans.

Running it against the downstream driver (which should be nearly
identical based on this set of patches), 4.19, on a Pi4 I get
pi@raspberrypi:~/v4l-utils/utils/v4l2-compliance $ ./v4l2-compliance -s
v4l2-compliance SHA: b16f9e945d74aa552abdd6f873821cb77faaf13a, 32 bits

Compliance test for bm2835 mmal device /dev/video0:

Driver Info:
Driver name  : bm2835 mmal
Card type: mmal service 16.1
Bus info : platform:bcm2835-v4l2
Driver version   : 4.19.56
Capabilities : 0x8525
Video Capture
Video Overlay
Read/Write
Streaming
Extended Pix Format
Device Capabilities
Device Caps  : 0x0525
Video Capture
Video Overlay
Read/Write
Streaming
Extended Pix Format

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second /dev/video0 open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK
test for unlimited opens: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
test VIDIOC_LOG_STATUS: OK

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls (Input 0):
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
test VIDIOC_G/S/TRY_EXT_CTRLS: OK
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 33 Private Controls: 0

Format ioctls (Input 0):
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
test Cropping: OK (Not Supported)
test Composing: OK (Not Supported)
test Scaling: OK

Codec ioctls (Input 0):
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
test VIDIOC_G_ENC_INDEX: OK (Not Supported)
test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK (Not Supported)
test Requests: OK (Not Supported)

Test input 0:

Streaming ioctls:
test read/write: OK
test blocking wait: OK
warn: v4l2-test-buffers.cpp(1429): Can free buffers even
if still mmap()ed
test MMAP (no poll): OK
warn: v4l2-test-buffers.cpp(1429): Can free buffers even
if still mmap()ed
test MMAP (select): OK
warn: v4l2-test-buffers.cpp(1429): Can free buffers even
if still mmap()ed
test MMAP (epoll): OK
test USERPTR (no poll): OK
test USERPTR (select): OK
test DMABUF (no poll): OK (Not Supported)
test DMABUF (select): OK (Not Supported)

Total for bm2835 mmal device /dev/video0: 53, Succeeded: 53, Failed:
0, Warnings: 3

The warnings are because downstream we have an early version of
"media: vb2: Allow reqbufs(0) with "in use" MMAP buffers" that doesn't
set the flag to userspace. I need to revert that and apply the
accepted one (it's not a clean cherrypick though).

I do try and run compliance

Re: [PATCH 01/31] staging: bcm2835-camera: Ensure H264 header bytes get a sensible timestamp

[Dave Stevenson]

Hi Nicolas

On Thu, 27 Jun 2019 at 20:55, Nicolas Dufresne  wrote:
>
> Hi Dave,
>
> Le jeudi 27 juin 2019 à 20:55 +0200, Stefan Wahren a écrit :
> > From: Dave Stevenson 
> >
> > H264 header come from VC with 0 timestamps, which means they get a
> > strange timestamp when processed with VC/kernel start times,
> > particularly if used with the inline header option.
> > Remember the last frame timestamp and use that if set, or otherwise
> > use the kernel start time.
>
> Normally H264 headers are considered to be part of the following frame.
> Giving it the timestamp of the previous frame will likely confuse some
> userspace and cause an off-by-one in timestamp. I understand this is a
> workaround, but am wondering if this can be improved.

Sorry, slight ambiguity in how I'm reading your comment.

Are you saying that the header bytes want to be in the same buffer as
the following frame?
I thought this had also been discussed in the V4L2 stateful codec API
threads along with how many encoded frames were allowed in a single
V4L2 buffer. I certainly hadn't seen a statement about the header
bytes being combined with the next frame.
If the behaviour required by V4L2 is that header bytes and following
frame are in the same buffer, then that is relatively easy to achieve
in the firmware. This workaround can remain for older firmware as it
will never trigger if the firmware has combined the frames.


Or are you saying that the header bytes remain in their own buffer,
but the timestamp wants to be the same as the next frame? That is
harder to achieve in the firmware, but could probably be done in the
kernel driver by holding on to the header bytes frame until the next
buffer had been received, at which point the timestamp can be copied
across. Possible, but just needs slightly careful handling to ensure
we don't lose buffers accidentally.

  Dave

> >
> > Link: https://github.com/raspberrypi/linux/issues/1836
> > Signed-off-by: Dave Stevenson 
> > ---
> >  .../staging/vc04_services/bcm2835-camera/bcm2835-camera.c  | 14 
> > --
> >  .../staging/vc04_services/bcm2835-camera/bcm2835-camera.h  |  2 ++
> >  2 files changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c 
> > b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
> > index dce6e6d..0c04815 100644
> > --- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
> > +++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
> > @@ -359,7 +359,9 @@ static void buffer_cb(struct vchiq_mmal_instance 
> > *instance,
> >   }
> >   } else {
> >   if (dev->capture.frame_count) {
> > - if (dev->capture.vc_start_timestamp != -1 && pts) {
> > + if (dev->capture.vc_start_timestamp != -1) {
> > + buf->vb.vb2_buf.timestamp = ktime_get_ns();
> > + } else if (pts) {
> >   ktime_t timestamp;
> >   s64 runtime_us = pts -
> >   dev->capture.vc_start_timestamp;
> > @@ -372,8 +374,15 @@ static void buffer_cb(struct vchiq_mmal_instance 
> > *instance,
> >ktime_to_ns(timestamp));
> >   buf->vb.vb2_buf.timestamp = 
> > ktime_to_ns(timestamp);
> >   } else {
> > - buf->vb.vb2_buf.timestamp = ktime_get_ns();
> > + if (dev->capture.last_timestamp) {
> > + buf->vb.vb2_buf.timestamp =
> > + dev->capture.last_timestamp;
> > + } else {
> > + buf->vb.vb2_buf.timestamp =
> > + 
> > ktime_to_ns(dev->capture.kernel_start_ts);
> > + }
> >   }
> > + dev->capture.last_timestamp = 
> > buf->vb.vb2_buf.timestamp;
> >
> >   vb2_set_plane_payload(&buf->vb.vb2_buf, 0, length);
> >   vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> > @@ -541,6 +550,7 @@ static int start_streaming(struct vb2_queue *vq, 
> > unsigned int count)
> >dev->capture.vc_start_timestamp, parameter_size);
> >
> >   dev->capture.kernel_start_ts = ktime_get();
> > + dev->capture.l

Re: [PATCH] staging: bcm2835-camera: Restore return behavior of ctrl_set_bitrate()

[Dave Stevenson]

Hi Stefan.

On Tue, 25 Jun 2019 at 17:30, Stefan Wahren  wrote:
>
> Hi Dan,
> hi Dave,
>
> Am 25.06.19 um 09:55 schrieb Dan Carpenter:
> > On Tue, Jun 25, 2019 at 12:13:15AM +0200, Stefan Wahren wrote:
> >> The commit 52c4dfcead49 ("Staging: vc04_services: Cleanup in
> >> ctrl_set_bitrate()") changed the return behavior of ctrl_set_bitrate().
> >> This breaks probing of bcm2835-camera:
> >>
> >> bcm2835-v4l2: mmal_init: failed to set all camera controls: -3
> >> Cleanup: Destroy video encoder
> >> Cleanup: Destroy image encoder
> >> Cleanup: Destroy video render
> >> Cleanup: Destroy camera
> >> bcm2835-v4l2: bcm2835_mmal_probe: mmal init failed: -3
> >> bcm2835-camera: probe of bcm2835-camera failed with error -3
> >>
> >> So restore the old behavior and fix this issue.
> >>
> >> Fixes: 52c4dfcead49 ("Staging: vc04_services: Cleanup in 
> >> ctrl_set_bitrate()")
> >> Signed-off-by: Stefan Wahren 
> > I feel like this papers over the issue.  It would be better to figure
> > out why this is failing and fix it properly.  -3 is -ESRCH and when I
> > grep for ESRCH I only see it used in the ioctl so that can't be it.
> >
> > I think it must be -MMAL_MSG_STATUS_EINVAL actually, but it comes from
> > the firmware or something so we can't grep for it.
> yes this is a result from the VC4 firmware, there is nothing i can do
> about it. Even the firmware has been fixed, the driver must be
> compatible with older firmware version.
> > Can we do some more digging to find out why it's failing or otherwise
> > we could add a comment.
> >
> >   /*
> >* FIXME:  port_parameter_set() sometimes fails with
> >* -MMAL_MSG_STATUS_EINVAL and we don't know why so we're
> >* ignoring those errors for now.
> >*
> >*/
> >   return 0;
>
> I will add a comment and a v4l2_dbg entry.
>
> @Dave
>
> I used a Raspberry Pi 3 with a V1.3 camera and get this with an
> additional v4l2_dbg in ctrl_set_bitrate()
>
> [1.472805] raspberrypi-firmware soc:firmware: Attached to firmware
> from 2019-03-27 15:48
> ...
> [7.441639] videodev: Linux video capture interface: v2.00
> [7.511659] bcm2835_v4l2: module is from the staging directory, the
> quality is unknown, you have been warned.
> ...
> [8.162504] bcm2835-v4l2: Set fps range to 3/1000 to 3/1000
> [8.163104] bcm2835-v4l2: Set fps range to 3/1000 to 3/1000
> [8.163624] bcm2835-v4l2: Set fps range to 3/1000 to 3/1000
> [8.164395] bcm2835-v4l2: mmal_ctrl:eecd7187 ctrl id:0x98091f ctrl
> val:0 imagefx:0x0 color_effect:false u:0 v:0 ret 0(0)
> [8.164493] bcm2835-v4l2: ctrl_set_colfx: After: mmal_ctrl:1ec18e37
> ctrl id:0x98092a ctrl val:32896 ret 0(0)
> [8.165413] bcm2835-v4l2: ctrl_set_bitrate: After: mmal_ctrl:b01a3b48
> ctrl id:0x9909cf ctrl val:1000 ret -3(-22)
> [8.165872] bcm2835-v4l2: scene mode selected 0, was 0
> [8.166249] bcm2835-v4l2: V4L2 device registered as video0 - stills
> mode > 1280x720
> [8.166596] bcm2835-v4l2: vid_cap - set up encode comp
> [8.171208] bcm2835-v4l2: JPG - buf size now 786432 was 786432
> [8.171220] bcm2835-v4l2: vid_cap - cur_buf.size set to 786432
> [8.171228] bcm2835-v4l2: Set dev->capture.fmt 4745504A, 1024x768,
> stride 0, size 786432
> [8.171234] bcm2835-v4l2: Broadcom 2835 MMAL video capture ver 0.0.2
> loaded.
>
> Looks like the firmware dislike val:1000
>
> Any thoughts?

I'd had a quick dig yesterday, but forgot to hit send :-/
It looks like the firmware is getting upset over the ordering of
things in setting the default bitrate and the bitrate mode. Setting
the bitrate mode to the default of VBR fails (return code is ignored)
as the bitrate is 0 at that point. Setting the bitrate then fails as
the firmware default mode is "disabled" (ie specify the Qp
parameters).

Setting the bitrate is also done via the MMAL port format when
enabling the stream, so I believe it's only the setting of the default
values that fails.

I'll sort a firmware fix, but a comment here along the lines you
propose is definitely worth it.
 /*
  * Older firmware versions (pre July 2019) have a bug in handling
  * MMAL_PARAMETER_VIDEO_BIT_RATE that result in the call
  * returning -MMAL_MSG_STATUS_EINVAL.
  * Ignore errors from this call.
  */
  return 0;
(apologies for the formatting).

  Dave

PS Is linux-rpi-kernel actually behaving for other people? I didn't
see this patch when it was submitted, and it isn't showing in the list
archive either.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC 00/11] staging: vc04_services: Improve driver load/unload

[Dave Stevenson]

Hi Peter

On Fri, 11 Jan 2019 at 06:10, Peter Robinson  wrote:
>
> Hi Stefan,
>
> > > > > I get difference results with 5.0-rc1 but neither of the above apps
> > > > > work either, will follow up based on the rest of the thread there.
> > > > >
> > > >
> > > > My first step with Raspbian is to enable the Camera interface which 
> > > > results into an appending of the following lines to config.txt:
> > > >
> > > > start_x=1
> > > > gpu_mem=128
> > > >
> > > > AFAIK a smaller value for gpu_mem wont work. Please provide your 
> > > > settings which results in this crash.
> > >
> > > start_x=1
> > > gpu_mem=64
> >
> > even with those settings i'm getting a picture in qv4l2 (v1.12.3) and no 
> > crash.
> >
> > According to dmesg i also have 64M reserved for CMA. How many do you have?
>
> I have 192Mb of CMA with LXDE, the vc4 driver uses CMA rather than the
> gpu_mem via the firmware so that's what we set it to (and to 256Mb for
> GNOME)

As Stefan says, with Raspbian the default gpu_mem to use the camera is 128MB.
The memory required depends on your use case as it includes the
buffers for the output images.
Checking on a running system, a V2 camera streaming 1080P YU12 with 3
V4L2 buffers is using 58MB of gpu_mem for the camera stack. H264
encode isntead of YU12 and it's around 63MB.
With the vc4 driver loaded there's only a small number of other
allocations left from the gpu_mem heap, so it's around the 70MB mark
that will be the minimum to get the camera running.

> > Does your qc4l2 make use of OpenGL (not in my case)?
>
> Yes, mine does. The crash with qv4l2 was only when I tried Cheese
> first, if I reboot and just use qv4l2 it works fine when configured
> with 128Mb without any crash. Which display driver are you using? Are
> you using vc4 or the proprietary closed one? With vc4 using cma rather
> than gpu_mem I wonder if we can reduce the amount needed there, but in
> the interim I've at least now got picture output when using purely
> qv4l2 and a reserve of 128Mb
>
> I'm not sure quite what gstreamer1/cheese is doing to cause that crash.

Can you confirm what resolution and format they are using in your
failure case? "v4l2-ctl -V" after they've been run will tell you.

Your earlier request:
> As a side note it would be a useful debug feature from a support PoV
> if the following line could also note which firmware is loaded:
>
> [8.087691] raspberrypi-firmware soc:firmware: Attached to firmware
> from 2019-01-09 20:07
>
> Something like "attached to extended/reduced/whatecer firmware from 
> -XX-XX"

A very valid suggestion.
I've made the firmware changes to advertise the build variant and
firmware hash via the mailbox service, and that should be in the next
firmware release.
Pull request https://github.com/raspberrypi/linux/pull/2806 has added
the Linux kernel changes to our kernel rpi-4.19.y branch.
With the updated firmware, "vcgencmd version" will also report the
build variant for you.

  Dave
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC 00/11] staging: vc04_services: Improve driver load/unload

[Dave Stevenson]

Hi Peter

On Tue, 8 Jan 2019 at 07:21, Peter Robinson  wrote:
>
> Hi Stefan,
>
> > This patch series improves the load/unload of bcm2835 camera and audio
> > drivers. It has been tested with Raspberry Pi 3 B and a camera module V1.
> >
> > This series based on current linux-next and Phil Elwell's series ("Improve 
> > VCHIQ
> > cache line size handling"). After Nicolas' series ("staging: vc04_services:
> > Some dead code removal") has been applied, i will rebase my series.
>
> I tried testing this series applied to 4.20 with the camera module. I
> tried with qv4l2 (from v4l-utils) and using cheese, which in turn uses
> gstreamer. I basically get the same crash for both options. Desktop is
> LXDE on 32 bit Fedora 29.
>
> I've not yet tried with 5.0-rc1 but it looks like it has this patch
> series and some other bits for the vchiq drivers in staging.

I'm trying to sort the patches I have on our kernel tree and get them
in a shape to get merged to staging.
I've built for 5.0.0-rc1 and also see the same error using:
v4l2-ctl -v width=640,height=480,pixelformat=YU12
v4l2-ctl --stream-mmap=3 --stream-to=/dev/null --stream-count=300

It's three independent things:
- The firmware has failed the failed to enable the camera for reasons unknown.
- The error path then hasn't returned all the buffers to videobuf2,
hence the warning from videobuf2-core.c:1468
- The driver has then tried to pass some buffers to MMAL / VCHI which
tries adding them to an invalid list.

I'm investigating why the firmware is failing to enable the camera
initially, and then look at cleaning up the error handling.

  Dave

> Peter
>
> [  231.121704] bcm2835-v4l2: Failed enabling camera, ret -2
> [  231.127168] bcm2835-v4l2: Failed to enable camera
> [  231.132030] [ cut here ]
> [  231.136852] WARNING: CPU: 2 PID: 1473 at
> drivers/media/common/videobuf2/videobuf2-core.c:1468
> vb2_start_streaming+0xb4/0x12c [videobuf2_common]
> [  231.149967] Modules linked in: fuse ip6t_rpfilter ip6t_REJECT
> nf_reject_ipv6 xt_conntrack ebtable_nat ip6table_nat nf_nat_ipv6
> ip6table_mangle ip6table_raw ip6table_security iptable_nat nf_nat_ipv4
> nf_nat iptable_mangle iptable_raw iptable_security nf_conntrack
> nf_defrag_ipv6 nf_defrag_ipv4 libcrc32c ip_set nfnetlink
> ebtable_filter ebtables ip6table_filter ip6_tables cmac bnep sunrpc
> vfat fat brcmfmac brcmutil vc4 cfg80211 snd_soc_core bcm2835_v4l2(C)
> ac97_bus snd_bcm2835(C) videobuf2_vmalloc snd_pcm_dmaengine
> videobuf2_memops snd_seq videobuf2_v4l2 videobuf2_common
> snd_seq_device v4l2_common snd_pcm videodev media snd_timer snd
> soundcore drm_kms_helper drm hci_uart btqca joydev btbcm btintel
> fb_sys_fops syscopyarea bluetooth sysfillrect gpio_raspberrypi_exp
> sysimgblt raspberrypi_hwmon bcm2835_thermal ecdh_generic vchiq(C)
> rfkill bcm2835_wdt bcm2835_rng leds_gpio cpufreq_dt lz4 lz4_compress
> zram hid_logitech_hidpp hid_logitech_dj smsc95xx usbnet mii mmc_block
> dwc2 sdhci_iproc crc32_arm_ce
> [  231.150208]  sdhci_pltfm udc_core sdhci bcm2835 pwm_bcm2835
> i2c_bcm2835 bcm2835_dma phy_generic
> [  231.248013] CPU: 2 PID: 1473 Comm: qv4l2 Tainted: G C
>  4.20.0-1.fc30.armv7hl #1
> [  231.256663] Hardware name: BCM2835
> [  231.260144] [] (unwind_backtrace) from []
> (show_stack+0x18/0x1c)
> [  231.268013] [] (show_stack) from []
> (dump_stack+0x80/0xa0)
> [  231.275357] [] (dump_stack) from [] (__warn+0xdc/0xf8)
> [  231.282349] [] (__warn) from []
> (warn_slowpath_null+0x40/0x4c)
> [  231.290067] [] (warn_slowpath_null) from []
> (vb2_start_streaming+0xb4/0x12c [videobuf2_common])
> [  231.300747] [] (vb2_start_streaming [videobuf2_common])
> from [] (vb2_core_streamon+0x110/0x138 [videobuf2_common])
> [  231.313166] [] (vb2_core_streamon [videobuf2_common])
> from [] (__video_do_ioctl+0x35c/0x494 [videodev])
> [  231.324681] [] (__video_do_ioctl [videodev]) from
> [] (video_usercopy+0x508/0x5d4 [videodev])
> [  231.335120] [] (video_usercopy [videodev]) from
> [] (vfs_ioctl+0x28/0x3c)
> [  231.343697] [] (vfs_ioctl) from []
> (do_vfs_ioctl+0x8c/0x838)
> [  231.351212] [] (do_vfs_ioctl) from []
> (ksys_ioctl+0x58/0x74)
> [  231.358726] [] (ksys_ioctl) from []
> (ret_fast_syscall+0x0/0x54)
> [  231.366493] Exception stack(0xd7d53fa8 to 0xd7d53ff0)
> [  231.371623] 3fa0:   4a1bf700 b5edd000 000c
> 40045612 be905378 0001
> [  231.379930] 3fc0: 4a1bf700 b5edd000 40045612 0036 b5e40e40
> 000c b6f00508 
> [  231.388230] 3fe0: be905378 be905368 b5ec6804 b5b9a1f0
> [  231.393434] ---[ end trace c5943cec7bb25669 ]---
> [  237.695591] list_add corruption. prev->next should be next
> (ea4493e0), but was efeb6638. (prev=e126fa78).
> [  237.705362] [ cut here ]
> [  237.710056] kernel BUG at lib/list_debug.c:28!
> [  237.714569] Internal error: Oops - BUG: 0 [#1] SMP ARM
> [  237.719785] Modules linked in: fuse ip6t_rpfilter ip6t_REJECT
> nf_reject_ipv6 xt_conntrack ebtable_nat ip6table_nat nf_nat_ipv6
> ip

Re: [PATCH 0/7] staging: vc04_services: Some dead code removal

[Dave Stevenson]

Hi Stefan

On Sun, 28 Oct 2018 at 08:31, Stefan Wahren  wrote:
>
> Hi Dave,
>
> > Dave Stevenson  hat am 26. Oktober 2018 um 
> > 19:15 geschrieben:
> >
> >
> > Thanks Stefan.
> > I've picked up your latest patches which mean I can get the driver
> > loaded via the (almost) approved method.
> > I do seem to still have issues with not getting the expected address
> > ranges, so the driver/VPU was trying to map cached alias memory. As
> > your patches only came through yesterday I haven't had a chance to dig
> > through why yet. I've done a temporary hack to ensure we always map
> > the uncached alias, but that can't persist.
>
> does it mean with DT probing it worked before and with platform change it's 
> broken?
> Or anything else cause this regression in 4.19?

Yes, probing via DT with the node under soc gave me the correct DMA
addresses (uncached alias). With the platform changes I get the cached
alias.
Both were under 4.14. I'll try again on a later branch.

> > The networking issue has been resolved :-)
> >
> > I've pushed where I've got to to
> > https://github.com/6by9/linux/tree/rpi-4.14.y-codecs-push-pt2b
> > It's a touch messy due to integrating in your patches in the last 24
> > hours. It needs a full rebase so that my changes are on top of yours
> > rather than haphazard.
> > As we're moving to 4.19 fairly soon I may well abandon my 4.14 tree
> > and jump to either that or directly on staging. I'll see where I get
> > to early next week.
>
> Sorry, but there is no need for a quick shot against a downstream 4.14. I 
> assumed you make your changes against upstream linux-next + Phil's and my 
> patches.
>
> You can use https://github.com/anholt/linux/commits/bcm2835-audio until 
> 4.20-rc1 is out.
> Using 4.14 or 4.19 doesn't make any sense to me.

As an employee of Raspberry Pi Trading my first responsibilty is to
them, and that means LTS releases feeding in to the downstream kernel.
If these drivers can be pushed upstream then that's a win as it avoids
divergence, but it is not my main goal. At least 4.19 and 4.20 aren't
far apart so there are likely to be fewer differences.

I had it all working on 4.14, therefore it made sense to see whether
your changes allowed me to load as a platform driver. It did, but with
this little niggle over cache aliases. I'll try again on a later
kernel and try to get some more info.

  Dave
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/7] staging: vc04_services: Some dead code removal

[Dave Stevenson]

On Thu, 18 Oct 2018 at 10:38, Stefan Wahren  wrote:
>
> Am 18.10.2018 um 11:22 schrieb Dave Stevenson:
> > On Wed, 17 Oct 2018 at 17:51, Peter Robinson  wrote:
> >>>>>> Drop various pieces of dead code from here and there to get rid of
> >>>>>> the remaining users of VCHI_CONNECTION_T. After that we get to drop
> >>>>>> entire header files worth of unused code.
> >>>>>>
> >>>>>> I've tested on a Raspberry Pi Model B (bcm2835_defconfig) that
> >>>>>> snd-bcm2835 can still play analog audio just fine.
> >>>>>>
> >>>>> thanks and i'm fine with your patch series:
> >>>>>
> >>>>> Acked-by: Stefan Wahren 
> >>>>>
> >>>>> Unfortunately this would break compilation of the downstream vchi
> >>>>> drivers like vcsm [1]. Personally i don't want to maintain another
> >>>>> one, because i cannot see the gain of the resulting effort.
> >>>>>
> >>>>> [1] - 
> >>>>> https://github.com/raspberrypi/linux/tree/rpi-4.14.y/drivers/char/broadcom/vc_sm
> >>>>
> >>>> I feel like everyone else already knows the answer but why don't we just
> >>>> merge that code into staging?
> >>> Dave's been working on a new VCSM service where the firmware can call
> >>> back into Linux to allocate (instead of just having a permanent carveout
> >>> of system memory that the firmware allocates from), and lets us make
> >>> dma-bufs out of those buffers.  That driver makes a no-copies v4l2 media
> >>> decode driver possible, which would then let Kodi and similar projects
> >>> switch from downstream kernels with closed graphics to upstream kernels
> >>> with open graphics.
> >>>
> >>> Given that the new VCSM service is a rewrite, it's not clear to me that
> >>> importing the old VCSM driver is a win.  But maybe we should go raid
> >>> https://github.com/6by9/linux/tree/rpi-4.14.y-codecs-push-pt2a and grab
> >>> the new drivers.  Upstreaming the VCHI audio driver to staging has
> >>> clearly been a win for it, so maybe other eyes on the new v4l2 codec
> >>> could help Dave along in stabilizing it.
> >> I think that makes sense as long as the firmware side changes are in
> >> place so it can actually be used.
> > The firmware has supported the necessary for dmabuf import since Sept 2017.
> >
> > The new vcsm driver currently only supports importing from other
> > kernel modules as I cut it back to the bare minimum to ease
> > upstreaming. To be a complete replacement of the existing then it
> > needs to support userspace alloc/free/import/mmap. I did have most of
> > that working, but will add it in stages.
> > The codec code is working for decode but something is off for setting
> > formats on encode.
> > Both drivers are loading through DT at the moment as I couldn't get
> > Eric's platform driver stuff working. IIRC A combination of modules
> > not getting loaded and getting the appropriate coherent DMA mask set
> > (being under soc in DT gives the correct mappings, but being a
> > platform driver didn't).
>
> I'm working on these issues and i will post a proper solution soon.
>
> In case you need a hack in order to test your stuff, i can prepare a
> branch for you.

Thanks Stefan.
I've picked up your latest patches which mean I can get the driver
loaded via the (almost) approved method.
I do seem to still have issues with not getting the expected address
ranges, so the driver/VPU was trying to map cached alias memory. As
your patches only came through yesterday I haven't had a chance to dig
through why yet. I've done a temporary hack to ensure we always map
the uncached alias, but that can't persist.

> >
> > I'm fire-fighting a networking issue at the moment, but hope to be
> > back on codecs next week.
> > Could you hold off raiding my trees until say Fri 26th Oct so I can
> > ensure they are fully up to date? If I get a chance then I'll start
> > the work of porting into staging before then.
>
> The merge window will open soon, so i don't see the need to hurry.

The networking issue has been resolved :-)

I've pushed where I've got to to
https://github.com/6by9/linux/tree/rpi-4.14.y-codecs-push-pt2b
It's a touch messy due to integrating in your patches in the last 24
hours. It needs a full rebase so that my changes are on top of yours
rather than haphazard.
As we're moving to 4.19 fairly soon I may well abandon my 4.14 tree
and jump to either that or directly on staging. I'll see where I get
to early next week.

  Dave
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/7] staging: vc04_services: Some dead code removal

[Dave Stevenson]

On Wed, 17 Oct 2018 at 17:51, Peter Robinson  wrote:
>
> > >> > Drop various pieces of dead code from here and there to get rid of
> > >> > the remaining users of VCHI_CONNECTION_T. After that we get to drop
> > >> > entire header files worth of unused code.
> > >> >
> > >> > I've tested on a Raspberry Pi Model B (bcm2835_defconfig) that
> > >> > snd-bcm2835 can still play analog audio just fine.
> > >> >
> > >>
> > >> thanks and i'm fine with your patch series:
> > >>
> > >> Acked-by: Stefan Wahren 
> > >>
> > >> Unfortunately this would break compilation of the downstream vchi
> > >> drivers like vcsm [1]. Personally i don't want to maintain another
> > >> one, because i cannot see the gain of the resulting effort.
> > >>
> > >> [1] - 
> > >> https://github.com/raspberrypi/linux/tree/rpi-4.14.y/drivers/char/broadcom/vc_sm
> > >
> > >
> > > I feel like everyone else already knows the answer but why don't we just
> > > merge that code into staging?
> >
> > Dave's been working on a new VCSM service where the firmware can call
> > back into Linux to allocate (instead of just having a permanent carveout
> > of system memory that the firmware allocates from), and lets us make
> > dma-bufs out of those buffers.  That driver makes a no-copies v4l2 media
> > decode driver possible, which would then let Kodi and similar projects
> > switch from downstream kernels with closed graphics to upstream kernels
> > with open graphics.
> >
> > Given that the new VCSM service is a rewrite, it's not clear to me that
> > importing the old VCSM driver is a win.  But maybe we should go raid
> > https://github.com/6by9/linux/tree/rpi-4.14.y-codecs-push-pt2a and grab
> > the new drivers.  Upstreaming the VCHI audio driver to staging has
> > clearly been a win for it, so maybe other eyes on the new v4l2 codec
> > could help Dave along in stabilizing it.
>
> I think that makes sense as long as the firmware side changes are in
> place so it can actually be used.

The firmware has supported the necessary for dmabuf import since Sept 2017.

The new vcsm driver currently only supports importing from other
kernel modules as I cut it back to the bare minimum to ease
upstreaming. To be a complete replacement of the existing then it
needs to support userspace alloc/free/import/mmap. I did have most of
that working, but will add it in stages.
The codec code is working for decode but something is off for setting
formats on encode.
Both drivers are loading through DT at the moment as I couldn't get
Eric's platform driver stuff working. IIRC A combination of modules
not getting loaded and getting the appropriate coherent DMA mask set
(being under soc in DT gives the correct mappings, but being a
platform driver didn't).

I'm fire-fighting a networking issue at the moment, but hope to be
back on codecs next week.
Could you hold off raiding my trees until say Fri 26th Oct so I can
ensure they are fully up to date? If I get a chance then I'll start
the work of porting into staging before then.

  Dave
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/7] staging: vc04_services: Some dead code removal

[Dave Stevenson]

On Mon, 15 Oct 2018 at 17:27, Eric Anholt  wrote:
>
> Stefan Wahren  writes:
>
> > Hi Tuomas,
> >
> >> Tuomas Tynkkynen  hat am 4. Oktober 2018 um 11:37 
> >> geschrieben:
> >>
> >>
> >> Drop various pieces of dead code from here and there to get rid of
> >> the remaining users of VCHI_CONNECTION_T. After that we get to drop
> >> entire header files worth of unused code.
> >>
> >> I've tested on a Raspberry Pi Model B (bcm2835_defconfig) that
> >> snd-bcm2835 can still play analog audio just fine.
> >>
> >
> > thanks and i'm fine with your patch series:
> >
> > Acked-by: Stefan Wahren 
> >
> > Unfortunately this would break compilation of the downstream vchi
> > drivers like vcsm [1]. Personally i don't want to maintain another
> > one, because i cannot see the gain of the resulting effort.
> >
> > [1] - 
> > https://github.com/raspberrypi/linux/tree/rpi-4.14.y/drivers/char/broadcom/vc_sm

I'm happy enough to work around these changes. Once the change is in a
released kernel we can merge a modified version of vc_sm into the
downstream kernel branch. It's not as if it requires big changes.

> I think the main concern would be if we removed things necessary for
> 6by9's new vcsm (the one that will let us do dma-buf sharing between
> media decode and DRM).

The new vcsm uses the same VCHI service as the existing downstream vc_sm driver.
The video codec driver don't use any VCHI functionality over and above
the camera. It goes via a slightly extended version of the
mmal-vchiq.c, which I have split out into a shared module.

> On the other hand, git revert is a thing, so it's not like we actually
> lose anything.

:-)

  Dave
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: bcm2835-camera: remove extraneous setting of dev->colourfx.enable

[Dave Stevenson]

 Hi Stefan.

Thanks for forwarding as the linux-rpi-kernel list hasn't sent it to me as yet.

On Mon, 8 Oct 2018 at 16:48, Stefan Wahren  wrote:
>
> Hi Colin,
>
> Am 08.10.2018 um 16:50 schrieb Colin King:
> > From: Colin Ian King 
> >
> > Currently dev->colourfx.enable is being set twice, hence the first
> > occurrance is redundant and can be removed, so remove it. This minor
> > issue was introduced by commit 7b3ad5abf027 ("staging: Import the
> > BCM2835 MMAL-based V4L2 camera driver.").
> >
> > Detected by CoverityScan CID#1419711 ("Unused value")
> >
> > Signed-off-by: Colin Ian King 
> > ---
> >  drivers/staging/vc04_services/bcm2835-camera/controls.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c 
> > b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > index a2c55cb2192a..99831dd4365d 100644
> > --- a/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > +++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > @@ -588,7 +588,6 @@ static int ctrl_set_colfx(struct bm2835_mmal_dev *dev,
> >
> >   control = &dev->component[MMAL_COMPONENT_CAMERA]->control;
> >
> > - dev->colourfx.enable = (ctrl->val & 0xff00) >> 8;
> >   dev->colourfx.enable = ctrl->val & 0xff;
> >
> >   ret = vchiq_mmal_port_parameter_set(dev->instance, control,
>
> as long as the current behavior is correct, i'm okay with this patch.
> But the byte masking looks suspicious, so i hope Dave can clarify that.

It's writing to the wrong structure member.
The function is used on the V4L2 control V4L2_CID_COLORFX_CBCR, which
is defined as:
V4L2_CID_COLORFX_CBCR (integer)
Determines the Cb and Cr coefficients for V4L2_COLORFX_SET_CBCR color
effect. Bits [7:0] of the supplied 32 bit value are interpreted as Cr
component, bits [15:8] as Cb component and bits [31:16] must be zero.

ctrl->val should therefore be setting dev->colourfx.u and
dev->colourfx.v with those masks. dev->colourfx.enable field should
have been set when going through ctrl_set_image_effect for contol
V4L2_CID_COLORFX set to V4L2_COLORFX_SET_CBCR.

I've confirmed with qv4l2 that that is what is wrong. That's only been
lurking there for almost 5 years - shows how often these weirder
effects get used.
I don't mind whether Colin's patch gets merged and then fixed up, or
drop Colin's patch and apply the correct logic via a new patch.

  Dave
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: bcm2835-camera: remove extraneous setting of dev->colourfx.enable

[Dave Stevenson]

On Mon, 8 Oct 2018 at 18:09, Stefan Wahren  wrote:
>
> Hi Dave,
>
> > Dave Stevenson  hat am 8. Oktober 2018 um 
> > 18:51 geschrieben:
> >
> >
> >  Hi Stefan.
> >
> > Thanks for forwarding as the linux-rpi-kernel list hasn't sent it to me as 
> > yet.
>
> AFAIK every mail with more than 5 recipients will be delayed.
>
> >
> > On Mon, 8 Oct 2018 at 16:48, Stefan Wahren  wrote:
> > >
> > > Hi Colin,
> > >
> > > Am 08.10.2018 um 16:50 schrieb Colin King:
> > > > From: Colin Ian King 
> > > >
> > > > Currently dev->colourfx.enable is being set twice, hence the first
> > > > occurrance is redundant and can be removed, so remove it. This minor
> > > > issue was introduced by commit 7b3ad5abf027 ("staging: Import the
> > > > BCM2835 MMAL-based V4L2 camera driver.").
> > > >
> > > > Detected by CoverityScan CID#1419711 ("Unused value")
> > > >
> > > > Signed-off-by: Colin Ian King 
> > > > ---
> > > >  drivers/staging/vc04_services/bcm2835-camera/controls.c | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c 
> > > > b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > > > index a2c55cb2192a..99831dd4365d 100644
> > > > --- a/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > > > +++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > > > @@ -588,7 +588,6 @@ static int ctrl_set_colfx(struct bm2835_mmal_dev 
> > > > *dev,
> > > >
> > > >   control = &dev->component[MMAL_COMPONENT_CAMERA]->control;
> > > >
> > > > - dev->colourfx.enable = (ctrl->val & 0xff00) >> 8;
> > > >   dev->colourfx.enable = ctrl->val & 0xff;
> > > >
> > > >   ret = vchiq_mmal_port_parameter_set(dev->instance, control,
> > >
> > > as long as the current behavior is correct, i'm okay with this patch.
> > > But the byte masking looks suspicious, so i hope Dave can clarify that.
> >
> > It's writing to the wrong structure member.
> > The function is used on the V4L2 control V4L2_CID_COLORFX_CBCR, which
> > is defined as:
> > V4L2_CID_COLORFX_CBCR (integer)
> > Determines the Cb and Cr coefficients for V4L2_COLORFX_SET_CBCR color
> > effect. Bits [7:0] of the supplied 32 bit value are interpreted as Cr
> > component, bits [15:8] as Cb component and bits [31:16] must be zero.
> >
> > ctrl->val should therefore be setting dev->colourfx.u and
> > dev->colourfx.v with those masks. dev->colourfx.enable field should
> > have been set when going through ctrl_set_image_effect for contol
> > V4L2_CID_COLORFX set to V4L2_COLORFX_SET_CBCR.
> >
> > I've confirmed with qv4l2 that that is what is wrong. That's only been
> > lurking there for almost 5 years - shows how often these weirder
> > effects get used.
> > I don't mind whether Colin's patch gets merged and then fixed up, or
> > drop Colin's patch and apply the correct logic via a new patch.
>
> no this patch shouldn't be applied. Does it mean you want to send the proper 
> one?

Want to - not really. Ought to - I guess so.
I've got a bunch of checkpatch and other fixes to come too, so once
I've got a mainline kernel build back up and running I'll roll them
all up and blitz the list.

  Dave
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: bcm2835-camera: Avoid unneeded internal declaration warning

[Dave Stevenson]

Hi Nate

Thanks for the patch.

On Fri, 28 Sep 2018 at 01:53, Nathan Chancellor
 wrote:
>
> Clang warns:
>
> drivers/staging/vc04_services/bcm2835-camera/controls.c:59:18: warning:
> variable 'mains_freq_qmenu' is not needed and will not be emitted
> [-Wunneeded-internal-declaration]
> static const s64 mains_freq_qmenu[] = {
>  ^
> 1 warning generated.
>
> This is because mains_freq_qmenu is currently only used in an ARRAY_SIZE
> macro, which is a compile time evaluation in this case. Avoid this by
> adding mains_freq_qmenu as the imenu member of this structure, which
> matches all other controls that uses the ARRAY_SIZE macro in v4l2_ctrls.
> This turns out to be a no-op because V4L2_CID_MPEG_VIDEO_BITRATE_MODE is
> defined as a MMAL_CONTROL_TYPE_STD_MENU, which does not pass the imenu
> definition along to v4l2_ctrl_new in bm2835_mmal_init_controls.

There's a slight confusion betwen V4L2_CID_MPEG_VIDEO_BITRATE_MODE and
V4L2_CID_POWER_LINE_FREQUENCY in your description.

However you're right that MMAL_CONTROL_TYPE_STD_MENU calls
v4l2_ctrl_new_std_menu which doesn't need a menu array (it's
v4l2_ctrl_new_int_menu that does). That means the correct fix is to
define the max value using the relevant enum (in this case
V4L2_CID_POWER_LINE_FREQUENCY_AUTO) and remove the array.

The same is true for V4L2_CID_MPEG_VIDEO_BITRATE_MODE - max should be
V4L2_MPEG_VIDEO_BITRATE_MODE_CBR, and remove the bitrate_mode_qmenu
array.

Thanks,
  Dave

> Link: https://github.com/ClangBuiltLinux/linux/issues/122
> Signed-off-by: Nathan Chancellor 
> ---
>  drivers/staging/vc04_services/bcm2835-camera/controls.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c 
> b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> index cff7b1e07153..a2c55cb2192a 100644
> --- a/drivers/staging/vc04_services/bcm2835-camera/controls.c
> +++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> @@ -1106,7 +1106,7 @@ static const struct bm2835_mmal_v4l2_ctrl 
> v4l2_ctrls[V4L2_CTRL_COUNT] = {
> {
> V4L2_CID_POWER_LINE_FREQUENCY, MMAL_CONTROL_TYPE_STD_MENU,
> 0, ARRAY_SIZE(mains_freq_qmenu) - 1,
> -   1, 1, NULL,
> +   1, 1, mains_freq_qmenu,
> MMAL_PARAMETER_FLICKER_AVOID,
> &ctrl_set_flicker_avoidance,
> false
> --
> 2.19.0
>
>
> ___
> linux-rpi-kernel mailing list
> linux-rpi-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: bcm2835-camera: Remove sleep at streaming start

[Dave Stevenson]

Hi Stefan.

On 23 May 2018 at 21:30, Stefan Schake  wrote:
> This checks the preview component for overlay functionality, but that has
> little to do with streaming (you don't need to enable the overlay to do
> streaming). We shouldn't withhold frames from userspace because of an
> estimate for AGC stabilization time in any case.

I'm not sure I agree with you there.
With your patch if you open the camera and take one (eg JPEG) frame
you'll get an incorrectly exposed image. That's a regression over the
current behaviour. Something like Motion running on those first few
frames is likely to see the change in exposure as significant changes
and therefore give false triggers.

The reason for the check for the overlay being enabled is that if it
is running then AE/AGC will then have already had a chance to process
some frames, therefore the capture image should be reasonable.

Have you tried it with "v4l2-ctl --stream-mmap=3 --stream-count=1
--stream-to=capture.jpg" and looked at the output? In all but fairly
bright conditions I'd expect to get a very dark image.

  Dave

> Signed-off-by: Stefan Schake 
> ---
>  drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c | 7 ---
>  1 file changed, 7 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c 
> b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
> index ce26741..fbc23b8 100644
> --- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
> +++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
> @@ -22,7 +22,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>
>  #include "mmal-common.h"
> @@ -511,12 +510,6 @@ static int start_streaming(struct vb2_queue *vq, 
> unsigned int count)
> /* enable frame capture */
> dev->capture.frame_count = 1;
>
> -   /* if the preview is not already running, wait for a few frames for 
> AGC
> -* to settle down.
> -*/
> -   if (!dev->component[MMAL_COMPONENT_PREVIEW]->enabled)
> -   msleep(300);
> -
> /* enable the connection from camera to encoder (if applicable) */
> if (dev->capture.camera_port != dev->capture.port
> && dev->capture.camera_port) {
> --
> 2.7.4
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: bcm2835-camera: Add TODO for removing overlay support

[Dave Stevenson]

On 11 May 2018 at 00:12, Stefan Schake  wrote:
> The overlay code is non-functional since it relies on firmware control
> of the HVS.
>
> Signed-off-by: Stefan Schake 
> ---
> Dave, does this match your understanding?

Yes, the overlay only works if you are not running the vc4 driver,
therefore dropping it is sensible for the mainline kernel.

>  drivers/staging/vc04_services/bcm2835-camera/TODO | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-camera/TODO 
> b/drivers/staging/vc04_services/bcm2835-camera/TODO
> index 0ab9e88..770fea6 100644
> --- a/drivers/staging/vc04_services/bcm2835-camera/TODO
> +++ b/drivers/staging/vc04_services/bcm2835-camera/TODO
> @@ -32,3 +32,11 @@ We should have VCHI create a platform device once it's 
> initialized,
>  and have this driver bind to it, so that we automatically load the
>  v4l2 module after VCHI loads.
>
> +5) Remove overlay support.
> +
> +We advertise support for a V4L2 overlay implemented through the
> +ril.video_render component. This component is non-functional with an
> +upstream kernel since the VC4 DRM driver controls the HVS, not the
> +firmware. Once we have dma-buf support, the same functionality with
> +improved control can be achieved through the DRM interface. We should
> +therefore remove all V4L2 overlay functionality from the driver.
> --
> 2.7.4
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] bcm2835-v4l2: Fix buffer overflow problem

[Dave Stevenson]

On 16 March 2017 at 05:11, Eric Anholt  wrote:
> Greg KH  writes:
>
>> On Wed, Mar 15, 2017 at 03:32:56PM +, Dave Stevenson wrote:
>>> You've got a reason. It's GPLv2 licenced code so I have no control
>>> over what happens to it.
>>> Everywhere I have worked, when a patch has issues it is better to "-1"
>>> to stop/delay the merge even (or particularly) on your own patches,
>>> but this is your playground so your rules.
>>
>> It's fine to say "don't apply, for _THIS REASON_".  You didn't specify a
>> reason, which is why I complained.
>>
>>> Anyway, I'll go back to working with the downstream tree (pull request
>>> for the full fix there) and stop bothering you.
>>
>> Ah, fun, we will diverge even more in the future, not good.  Any reason
>> why I shouldn't just delete this whole in-kernel tree if you are going
>> to spend time maintaining a different version?
>>
>> If you want to maintain this driver, in the kernel tree, by all means I
>> would love the help as I don't have hardware or the ability to test
>> anything.  Having two different trees for the source guarantees that
>> there are going to be constant problems here, and that's not good for
>> anyone.
>
> If Dave doesn't do the upstreaming work for his future work, I'll be
> forced to.  The rpi tree still hasn't diverged, yet (other than this
> patch and the related one being discussed).

Firstly, sorry. Bad day and frustrations vented inappropriately.
I'll consider this my baptism of fire in the ways of the mainline
kernel. Lessons learnt: Justify everything, provide defined timelines
for fixes, and develop a thick skin.

Upstreaming this driver isn't my main task at the moment, but I do
intend to come back to it, in particular to address the issues raised
on linux-media. Timescales of 4-6 weeks all being well (defined
timeline!).
There are no current developments on this driver that I am aware of,
so that delay shouldn't be critical for divergence. My next set of
changes to it are dependent on the current tasks.
I will also be undertaking backporting the changes made in staging to
the out-of-tree driver, so divergence will be minimised. Out-of-tree
is currently 4.4 but about to adopt 4.9, so is not in a position to
take the current staging as-is.

Michael's request was an interrupt serviced badly. Having no existing
tree to test staging with it was taking me a little while after
getting the code reviewed internally to get a tree working and confirm
all was good (at least a basic compile test). To suddenly find that
wasted was annoying and I reacted :-(


Take this patch as is, and I'll liase with Michael so that one of us
creates and sends the follow up patch to complete the fix.

I'm learning :-)

  Dave
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] bcm2835-v4l2: Fix buffer overflow problem

[Dave Stevenson]

On 15 March 2017 at 10:36, Dan Carpenter  wrote:
> On Wed, Mar 15, 2017 at 10:06:11AM +0000, Dave Stevenson wrote:
>> On 15 March 2017 at 05:08, Greg KH  wrote:
>> > On Tue, Mar 14, 2017 at 03:32:43PM +0000, Dave Stevenson wrote:
>> >> NACK.
>> >> Phil asked for a couple of changes, although functionally identical.
>> >> I'll send a patch when I get a chance.
>> >
>> > What do you mean, "when I get a chance"?  What's wrong with this one?
>>
>> I was intending on doing it today.
>> Michael had noticed a variant of this patch in the downstream kernel
>> and asked me if I was intending to submit upstream, in particular
>> asking for it to come from raspberrypi becasue it was an API subtlety.
>
> This description is too vague...
>
> Your previous email was like "This patch is correct but there is an
> unspecified problem that I'm going to fix at an unspecified time." and
> now you're like "If you guys like applying patches with problems that's
> fine with me but there is an unspecified problem with this one."  I
> guess we'll wait until later today to see what the issue is.  :P

OK, part of this is me being annoyed at Michael for asking for
assistance and then jumping the gun sending this upstream.

Full description:
mmal_vchiq is reimplementing parts of the userside MMAL library in kernel space.
The expected behaviour of port_parameter_get is that it takes the size
of storage for the parameter value, and returns the amount actually
used. If the storage is too small, it returns error and the size
required.
Before this patch it wasn't updating size on success and was returning
required size+8 on error. Double whammy.
With this patch it is still not updating size on success, but is
returning the required size correctly on error.
The full fix for both paths obsoletes this patch, and came out of
Michael asking for me to get the change reviewed internally.

You've got a reason. It's GPLv2 licenced code so I have no control
over what happens to it.
Everywhere I have worked, when a patch has issues it is better to "-1"
to stop/delay the merge even (or particularly) on your own patches,
but this is your playground so your rules.

Anyway, I'll go back to working with the downstream tree (pull request
for the full fix there) and stop bothering you.

  Dave
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] bcm2835-v4l2: Fix buffer overflow problem

[Dave Stevenson]

On 15 March 2017 at 05:08, Greg KH  wrote:
> On Tue, Mar 14, 2017 at 03:32:43PM +0000, Dave Stevenson wrote:
>> NACK.
>> Phil asked for a couple of changes, although functionally identical.
>> I'll send a patch when I get a chance.
>
> What do you mean, "when I get a chance"?  What's wrong with this one?

I was intending on doing it today.
Michael had noticed a variant of this patch in the downstream kernel
and asked me if I was intending to submit upstream, in particular
asking for it to come from raspberrypi becasue it was an API subtlety.
I was, and have had the patch reviewed internally with mods
requested/made, but he's jumped ahead of me.

>> Your existing workaround has removed the immediate issue of the
>> overflow, this was only cleaning things up to actually match the
>> original API.
>
> Ok, so this patch is correct?  I don't see why I shouldn't take it as it
> fixes the issue.  What am I missing here, why exactly are you NACKing
> this?

The existing patch that is already applied
85b1ac7 staging: bcm2835-camera: Fix buffer overflow calculation on
query of camera properties
has fixed the issue of the actual overflow.

Perhaps this is me not used to the way staging works with large
numbers of patches reworking stuff multiple times, in which case
accept it.
I'm more used to trying to get things completely right earlier.

  Dave
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] bcm2835-v4l2: Fix buffer overflow problem

[Dave Stevenson]

NACK.
Phil asked for a couple of changes, although functionally identical.
I'll send a patch when I get a chance.

Your existing workaround has removed the immediate issue of the
overflow, this was only cleaning things up to actually match the
original API.

  Dave

On 14 March 2017 at 15:10, Michael Zoran  wrote:
> From: Dave Stevenson 
>
> https://github.com/raspberrypi/linux/issues/1447
> port_parameter_get() failed to account for the header
> (u32 id and u32 size) in the size before memcpying
> the response into the response buffer, so overrunning
> the provided buffer by 8 bytes.
>
> Account for those bytes, and also a belt-and-braces
> check to ensure we never copy more than *value_size
> bytes into value.
>
> Signed-off-by: Dave Stevenson 
> Signed-off-by: Michael Zoran 
> Tested-by: Michael Zoran 
>
> ---
>  drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c 
> b/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
> index fc1076db0f82..ccb2ee547055 100644
> --- a/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
> @@ -1445,7 +1445,12 @@ static int port_parameter_get(struct 
> vchiq_mmal_instance *instance,
> }
>
> ret = -rmsg->u.port_parameter_get_reply.status;
> -   if (ret || (rmsg->u.port_parameter_get_reply.size > *value_size)) {
> +   /* port_parameter_get_reply.size includes the header,
> +* whilst *value_size doesn't.
> +*/
> +   rmsg->u.port_parameter_get_reply.size -= (2 * sizeof(u32));
> +
> +   if (ret || rmsg->u.port_parameter_get_reply.size > *value_size) {
> /* Copy only as much as we have space for
>  * but report true size of parameter
>  */
> --
> 2.11.0
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 08/10] staging: bcm2835-camera: Fix buffer overflow calculation on query of camera properties

[Dave Stevenson]

On 10 March 2017 at 11:16, Michael Zoran  wrote:
> On Fri, 2017-03-10 at 11:11 +0000, Dave Stevenson wrote:
>> On 10 March 2017 at 05:08, Michael Zoran  wrote:
>> > The code that queries properties on the camera has a check
>> > for buffer overruns if the firmware sends too much data.  This
>> > check is incorrect, and during testing I was seeing stack
>> > corruption.
>> >
>> > I believe this error can actually happen in normal use, just for
>> > some reason it doesn't appear on 32 bit as often.  So perhaps
>> > it's best for the check to be fixed.
>>
>> That sounds like it is related to a report I couldn't nail down as it
>> only happened on ArchLinux -
>> https://github.com/raspberrypi/linux/issues/1447
>>
>> Comparing against the original MMAL implementation at
>> https://github.com/raspberrypi/userland/blob/master/interface/mmal/vc
>> /mmal_vc_api.c#L1187
>> there are a few subtle differences. Your patch is a belt and braces
>> to
>> ensure that it never copies too much, so reasonable in that regard.
>>
>> I'm actually more worried that it implies we have a structure
>> mismatch
>> against the firmware, but I currently can't see where it is. A load
>> of
>> debug prints for sizeof(struct) required
>>
>> > Signed-off-by: Michael Zoran 
>> > ---
>> >  drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/staging/vc04_services/bcm2835-camera/mmal-
>> > vchiq.c b/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
>> > index 41de8956e421..976aa08365f2 100644
>> > --- a/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
>> > +++ b/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
>> > @@ -1442,7 +1442,7 @@ static int port_parameter_get(struct
>> > vchiq_mmal_instance *instance,
>> > }
>> >
>> > ret = -rmsg->u.port_parameter_get_reply.status;
>> > -   if (ret) {
>> > +   if (ret || (rmsg->u.port_parameter_get_reply.size >
>> > *value_size)) {
>> > /* Copy only as much as we have space for
>> >  * but report true size of parameter
>> >  */
>> > --
>> > 2.11.0
>> >
>
> I actually tracked it while debugging down to these structures.  I was
> going to let you know, but I went with the band-aid for now.
>
> The firmware thinks the structure should be 8 bytes longer.  I'm
> wondering if it should be 4 cameras and 4 flashes. That would explain
> the extra 8 bytes.

The userland repo is the extraction of the relevant IPC files from the
firmware source tree. What you see there is the same code the firmware
is being built from. It's only a 32bit compiler so I can't see why it
would suddenly be aligning the flash structures to 64bits.

I'll do some debugging on the firmware side to work out how/where it
is going wrong, but this patch is valid regardless.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 08/10] staging: bcm2835-camera: Fix buffer overflow calculation on query of camera properties

[Dave Stevenson]

On 10 March 2017 at 05:08, Michael Zoran  wrote:
> The code that queries properties on the camera has a check
> for buffer overruns if the firmware sends too much data.  This
> check is incorrect, and during testing I was seeing stack corruption.
>
> I believe this error can actually happen in normal use, just for
> some reason it doesn't appear on 32 bit as often.  So perhaps
> it's best for the check to be fixed.

That sounds like it is related to a report I couldn't nail down as it
only happened on ArchLinux -
https://github.com/raspberrypi/linux/issues/1447

Comparing against the original MMAL implementation at
https://github.com/raspberrypi/userland/blob/master/interface/mmal/vc/mmal_vc_api.c#L1187
there are a few subtle differences. Your patch is a belt and braces to
ensure that it never copies too much, so reasonable in that regard.

I'm actually more worried that it implies we have a structure mismatch
against the firmware, but I currently can't see where it is. A load of
debug prints for sizeof(struct) required

> Signed-off-by: Michael Zoran 
> ---
>  drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c 
> b/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
> index 41de8956e421..976aa08365f2 100644
> --- a/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
> @@ -1442,7 +1442,7 @@ static int port_parameter_get(struct 
> vchiq_mmal_instance *instance,
> }
>
> ret = -rmsg->u.port_parameter_get_reply.status;
> -   if (ret) {
> +   if (ret || (rmsg->u.port_parameter_get_reply.size > *value_size)) {
> /* Copy only as much as we have space for
>  * but report true size of parameter
>  */
> --
> 2.11.0
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: bcm2835-camera: Remove explicit cache flush operations

[Dave Stevenson]

On 8 March 2017 at 13:46, Michael Zoran  wrote:
> On Wed, 2017-03-08 at 13:19 +0000, Dave Stevenson wrote:
>
> Hi Michael.
>
> The original issue was on Pi1 with H264 or MJPEG streams. Buffer sizes were
> small enough to fit in the L1 cache and the relevant sections weren't being
> flushed. The resulting stream ended up being corrupt due to a block of stale
> data from the L1. (L2 is common between VideoCore and ARM on 2835, so not an
> issue. IIRC Videocore is generally using uncached access anyway).
>
> It won't show up with any of the raw image formats as they are all too large
> to fit in L1. Low bitrate, low resolution H264 is the most likely to exhibit
> issues as buffers will be a few kB in size.
>
> I haven't looked at recent vc04_services changes, but iff it handles that
> cleanly then there are no qualms from me.
>
>   Dave
>
> vc04_services now uses dma_map_sg/dma_unmap_sg for bulk which is the
> multi-platform method to hand the buffer back and forth. I'm looking at both
> arm and arm64 code in mm, and it appears to have some rather aggressive code
> to deal with flushing the caches. It's also tries to deal with bounce
> buffers that may be needed and any iommu devices on the bus.

So theoretically it does the right thing, although may be over
aggressive in flushing in this context.

> I'll try some different formats, but I gave away my RPI 1 awhile ago. I do
> have a few RPI 2's laying around.

BCM2835 (Pi0/1) is the only processor in the family where the L2 is
shared between VC4 and ARM.
BCM2836 & 2837 (Pi2 & 3) the ARMs have their own L2 cache so I doubt
they would exhibit this issue anyway.

If you need a Pi0/1 then shout and we can find you one. (Email me directly).

  Dave

> On 8 March 2017 at 12:21, Michael Zoran  wrote:
>
> The camera code has an explicit cache flush operation
> which is not portable.  Now that vc04_services is using portable
> DMA APIs that already do the cache flushing, explicit flushes
> should no longer be needed.
>
> The one call to __cpuc_flush_dcache_area has been removed.
>
> Testing:
> The offical V2 camera for the RPI was tested on a RPI 3
> running in 32 bit mode(armhf).  The cheese application
> and ffmpeg was used to view and stream video from the
> camera.  Nothing new seems to be broken without the
> cache flushing.
>
> Signed-off-by: Michael Zoran 
> ---
>  drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c | 4 
>  1 file changed, 4 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
> b/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
> index ca6e9ebc0e81..a57eb829c353 100644
> --- a/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
> @@ -276,10 +276,6 @@ static int bulk_receive(struct vchiq_mmal_instance
> *instance,
> msg_context->u.bulk.dts = msg->u.buffer_from_host.buffer_header.dts;
> msg_context->u.bulk.pts = msg->u.buffer_from_host.buffer_header.pts;
>
> -   // only need to flush L1 cache here, as VCHIQ takes care of the L2
> -   // cache.
> -   __cpuc_flush_dcache_area(msg_context->u.bulk.buffer->buffer,
> rd_len);
> -
> /* queue the bulk submission */
> vchi_service_use(instance->handle);
> ret = vchi_bulk_queue_receive(instance->handle,
> --
> 2.11.0
>
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/6] staging: Import the BCM2835 MMAL-based V4L2 camera driver.

[Dave Stevenson]

Hi Hans.

On 06/02/17 12:58, Hans Verkuil wrote:

On 02/06/2017 12:37 PM, Dave Stevenson wrote:

Hi Hans.

On 06/02/17 09:08, Hans Verkuil wrote:

Hi Eric,

Great to see this driver appearing for upstream merging!

See below for my review comments, focusing mostly on V4L2 specifics.






+   f->fmt.pix.pixelformat = dev->capture.fmt->fourcc;
+   f->fmt.pix.bytesperline = dev->capture.stride;
+   f->fmt.pix.sizeimage = dev->capture.buffersize;
+
+   if (dev->capture.fmt->fourcc == V4L2_PIX_FMT_RGB24)
+   f->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
+   else if (dev->capture.fmt->fourcc == V4L2_PIX_FMT_JPEG)
+   f->fmt.pix.colorspace = V4L2_COLORSPACE_JPEG;
+   else
+   f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;


Colorspace has nothing to do with the pixel format. It should come from the
sensor/video receiver.

If this information is not available, then COLORSPACE_SRGB is generally a
good fallback.


I would if I could, but then I fail v4l2-compliance on V4L2_PIX_FMT_JPEG
https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-test-formats.cpp#n329
The special case for JPEG therefore has to remain.


Correct. Sorry, my fault, I forgot about that.



It looks like I tripped over the subtlety between V4L2_COLORSPACE_,
V4L2_XFER_FUNC_, V4L2_YCBCR_ENC_, and V4L2_QUANTIZATION_, and Y'CbCr
encoding vs colourspace.

The ISP coefficients are set up for BT601 limited range, and any
conversion back to RGB is done based on that. That seemed to fit
SMPTE170M rather than SRGB.


Colorspace refers to the primary colors + whitepoint that are used to
create the colors (basically this answers the question to which colors
R, G and B exactly refer to). The SMPTE170M has different primaries
compared to sRGB (and a different default transfer function as well).

RGB vs Y'CbCr is just an encoding and it doesn't change the underlying
colorspace. Unfortunately, the term 'colorspace' is often abused to just
refer to RGB vs Y'CbCr.

If the colorspace is SRGB, then when the pixelformat is a Y'CbCr encoding,
then the BT601 limited range encoding is implied, unless overridden via
the ycbcr_enc and/or quantization fields in struct v4l2_pix_format.

In other words, this does already the right thing.


https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/pixfmt-007.html#colorspace-srgb-v4l2-colorspace-srgb
"The default transfer function is V4L2_XFER_FUNC_SRGB. The default 
Y’CbCr encoding is V4L2_YCBCR_ENC_601. The default Y’CbCr quantization 
is full range."

So full range or limited?


The JPEG colorspace is a short-hand for V4L2_COLORSPACE_SRGB, V4L2_YCBCR_ENC_601
and V4L2_QUANTIZATION_FULL_RANGE. It's historical that this colorspace exists.

If I would redesign this this JPEG colorspace would be dropped.

For a lot more colorspace information see:

https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces.html



I do note that as there is now support for more RGB formats (BGR24 and
BGR32) the first "if" needs extending to cover those. Or I don't care
and only special case JPEG with all others just reporting SRGB.



Only special case JPEG.

But as I said, this information really needs to come from the sensor or
video receiver since this driver has no knowledge of this.


OK.






This is IMHO unnecessarily complex.

My recommendation is that controls are added with a set of v4l2_ctrl_new_std* 
calls
or if you really want to by walking a struct v4l2_ctrl_config array and adding 
controls
via v4l2_ctrl_new_custom.

The s_ctrl is a switch that calls the 'setter' function.

No need for arrays, callbacks, etc. Just keep it simple.


I can look into that, but I'm not sure I fully follow what you are
suggesting.

In the current implementation things like V4L2_CID_SATURATION,
V4L2_CID_SHARPNESS, V4L2_CID_CONTRAST, and V4L2_CID_BRIGHTNESS all use
the one common ctrl_set_rational setter function because the only thing
different in setting is the MMAL_PARAMETER_xxx value. I guess that could
move into the common setter based on V4L2_CID_xxx, but then the control
configuration is split between multiple places which feels less well
contained.


See e.g. samples/v4l/v4l2-pci-skeleton.c: in the probe function (or in a
function called from there if there are a lot of controls) you add the
controls, and in s_ctrl you handle them.

But this is just my preference.

So in s_ctrl you would see something like this:

switch (ctrl->id) {
case V4L2_CID_SATURATION:
ctrl_set_rational(ctrl->val, MMAL_PARAMETER_SAT);
break;
case V4L2_CID_BRIGHTNESS:
ctrl_set_rational(ctrl->val, MMAL_PARAMETER_BRIGHTNESS);
break;
...
}


OK, thanks for the clarification. That can be done.






Final question: did you run v4l2-compliance over this driver? Before this 
driver can
be moved out of staging it should pass the compliance 

Re: [PATCH 1/6] staging: Import the BCM2835 MMAL-based V4L2 camera driver.

[Dave Stevenson]

Hi Mauro.

Can I just say that I'm not attempting to upstream this, others are. 
I've just answered questions raised.


On 06/02/17 12:37, Mauro Carvalho Chehab wrote:

Em Sun, 5 Feb 2017 22:15:21 +
Dave Stevenson  escreveu:


If the goal was to protect some IP related to the sensors, I guess
this is not going to protect anything, as there are recent driver
submissions on linux-media for the ov5647 driver:

https://patchwork.kernel.org/patch/9472441/

There are also open source drivers for the Sony imx219 camera
floating around for android and chromeOS:


https://chromium.googlesource.com/chromiumos/third_party/kernel/+/factory-ryu-6486.14.B-chromeos-3.14/drivers/media/i2c/soc_camera/imx219.c

https://android.googlesource.com/kernel/bcm/+/android-bcm-tetra-3.10-lollipop-wear-release/drivers/media/video/imx219.c

Plus, there's a datasheet (with another driver) available at:
https://github.com/rellimmot/Sony-IMX219-Raspberry-Pi-V2-CMOS

So, you're not really protecting IP here.


None of the ISP processing, control loops, or tuning are open.
Yes there are kernel drivers kicking around for OV5647 and IMX219 now, 
but very little will consume raw Bayer.

That is also Omnivision or Sony IP, not Broadcom IP.


If the goal is to protect some proprietary algorithm meant to enhance
the camera captured streams, doing things like (auto-focus, auto-white
adjustments, scaling, etc), and/or implementing codec encoders, you should,
instead, restructure such codecs as mem2mem V4L2 drivers. There are a bunch
of such codecs already for other SoC where such functions are implemented
on GPU.

If you add DMABUF capabilities and media controller to the capture
driver and to the mem2mem drivers, userspace can use the V4L2 APIs
to use those modules using the arrangements they need, without
performance impacts.

So, by obfuscating the code, you're not protecting anything. Just making
harder for RPi customers to use, as you're providing a driver that is
limited.








Greg was kick on merging it on staging ;) Anyway, the real review
will happen when the driver becomes ready to be promoted out of
staging. When you address the existing issues and get it ready to
merge, please send the patch with such changes to linux-media ML.
I'll do a full review on it by then.


Is that even likely given the dependence on VCHI? I wasn't expecting
VCHI to leave staging, which would force this to remain too.


I didn't analyze the VCHI driver. As I said before, if you rewrite
the driver in a way that the Kernel can actually see the sensors
via an I2C interface, you probably can get rid of the VCHI interface
for the capture part.

You could take a look on the other mem2mem drivers and see if are
there some way to provide an interface for the GPU encoders in a
similar way to what those drivers do.


I will be looking at a sub dev driver for just the CCP2/CSI1/CSI2 
receiver as Broadcom did manage to release (probably unintentionally) a 
bastardised soc-camera driver for it and therefore the info is in the 
public domain.


It would be possible to write a second sub dev driver that was similar 
to this in using VCHI/MMAL to create another mem2mem device to wrap the 
ISP, but that would just be an image processing pipe - control loops 
would all be elsewhere (userspace).
I haven't seen a sensible mechanism within V4L2 for exporting image 
statistics for AE, AWB, AF, or histograms to userspace so that 
appropriate algorithms can be run there. Have I missed them, or do they 
not exist?






That seems a terrible hack! let the user specify the resolution via
modprobe parameter... That should depend on the hardware capabilities
instead.


This is sitting on top of an OpenMaxIL style camera component (though
accessed via MMAL - long story, but basically MMAL removed a bundle of
the ugly/annoying parts of IL).
It has the extension above V1.1.2 that you have a preview port, video
capture port, and stills capture port. Stills captures have additional
processing stages to improve image quality, whilst video has to maintain
framerate.


I see.


If you're asked for YUV or RGB frame, how do you choose between video or
stills? That's what is being set with these parameters, not the sensor
resolution. Having independent stills and video processing options
doesn't appear to be something that is supported in V4L2, but I'm open
to suggestions.


At the capture stage:

Assuming that the user wants to use different resolutions for video and
stills (for example, you're seeing a real time video, then you "click"
to capture a still image), you can create two buffer groups, one for
low-res video and another one for high-res image. When the button is
clicked, it will stop the low-res stream, set the parameters for the
high-res image and capture it.

For post-processing stage:

Switch the media pipeline via the media controller adding the post
processing cod

Re: [PATCH 1/6] staging: Import the BCM2835 MMAL-based V4L2 camera driver.

[Dave Stevenson]

Hi Hans.

On 06/02/17 09:08, Hans Verkuil wrote:

Hi Eric,

Great to see this driver appearing for upstream merging!

See below for my review comments, focusing mostly on V4L2 specifics.

On 01/27/2017 10:54 PM, Eric Anholt wrote:

- Supports raw YUV capture, preview, JPEG and H264.
- Uses videobuf2 for data transfer, using dma_buf.
- Uses 3.6.10 timestamping
- Camera power based on use
- Uses immutable input mode on video encoder

This code comes from the Raspberry Pi kernel tree (rpi-4.9.y) as of
a15ba877dab4e61ea3fc7b006e2a73828b083c52.

Signed-off-by: Eric Anholt 
---
 .../media/platform/bcm2835/bcm2835-camera.c| 2016 
 .../media/platform/bcm2835/bcm2835-camera.h|  145 ++
 drivers/staging/media/platform/bcm2835/controls.c  | 1345 +
 .../staging/media/platform/bcm2835/mmal-common.h   |   53 +
 .../media/platform/bcm2835/mmal-encodings.h|  127 ++
 .../media/platform/bcm2835/mmal-msg-common.h   |   50 +
 .../media/platform/bcm2835/mmal-msg-format.h   |   81 +
 .../staging/media/platform/bcm2835/mmal-msg-port.h |  107 ++
 drivers/staging/media/platform/bcm2835/mmal-msg.h  |  404 
 .../media/platform/bcm2835/mmal-parameters.h   |  689 +++
 .../staging/media/platform/bcm2835/mmal-vchiq.c| 1916 +++
 .../staging/media/platform/bcm2835/mmal-vchiq.h|  178 ++
 12 files changed, 7111 insertions(+)
 create mode 100644 drivers/staging/media/platform/bcm2835/bcm2835-camera.c
 create mode 100644 drivers/staging/media/platform/bcm2835/bcm2835-camera.h
 create mode 100644 drivers/staging/media/platform/bcm2835/controls.c
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-common.h
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-encodings.h
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-msg-common.h
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-msg-format.h
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-msg-port.h
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-msg.h
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-parameters.h
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-vchiq.c
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-vchiq.h

diff --git a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c 
b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
new file mode 100644
index ..4f03949aecf3
--- /dev/null
+++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
@@ -0,0 +1,2016 @@





+static int start_streaming(struct vb2_queue *vq, unsigned int count)
+{
+   struct bm2835_mmal_dev *dev = vb2_get_drv_priv(vq);
+   int ret;
+   int parameter_size;
+
+   v4l2_dbg(1, bcm2835_v4l2_debug, &dev->v4l2_dev, "%s: dev:%p\n",
+__func__, dev);
+
+   /* ensure a format has actually been set */
+   if (dev->capture.port == NULL)
+   return -EINVAL;


Standard mistake. If start_streaming returns an error, then it should call 
vb2_buffer_done
for all queued buffers with state VB2_BUF_STATE_QUEUED. Otherwise the buffer 
administration
gets unbalanced.


OK.
This is an error path that I'm not convinced can ever be followed, just 
defensive programming. It may be a candidate for just removing, but yes 
otherwise it needs to be fixed to do the right thing.



+
+   if (enable_camera(dev) < 0) {
+   v4l2_err(&dev->v4l2_dev, "Failed to enable camera\n");
+   return -EINVAL;
+   }
+
+   /*init_completion(&dev->capture.frame_cmplt); */
+
+   /* enable frame capture */
+   dev->capture.frame_count = 1;
+
+   /* if the preview is not already running, wait for a few frames for AGC
+* to settle down.
+*/
+   if (!dev->component[MMAL_COMPONENT_PREVIEW]->enabled)
+   msleep(300);
+
+   /* enable the connection from camera to encoder (if applicable) */
+   if (dev->capture.camera_port != dev->capture.port
+   && dev->capture.camera_port) {
+   ret = vchiq_mmal_port_enable(dev->instance,
+dev->capture.camera_port, NULL);
+   if (ret) {
+   v4l2_err(&dev->v4l2_dev,
+"Failed to enable encode tunnel - error %d\n",
+ret);
+   return -1;


Use a proper error, not -1.


+   }
+   }
+
+   /* Get VC timestamp at this point in time */
+   parameter_size = sizeof(dev->capture.vc_start_timestamp);
+   if (vchiq_mmal_port_parameter_get(dev->instance,
+ dev->capture.camera_port,
+ MMAL_PARAMETER_SYSTEM_TIME,
+ &dev->capture.vc_start_timestamp,
+ ¶meter_size)) {
+   v4l2_err(&dev->v4l2

Re: [PATCH 1/6] staging: Import the BCM2835 MMAL-based V4L2 camera driver.

[Dave Stevenson]

bcm2835-camera.c
new file mode 100644
index ..4f03949aecf3
--- /dev/null
+++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
@@ -0,0 +1,2016 @@
+/*
+ * Broadcom BM2835 V4L2 driver
+ *
+ * Copyright © 2013 Raspberry Pi (Trading) Ltd.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file COPYING in the main directory of this archive
+ * for more details.
+ *
+ * Authors: Vincent Sanders 
+ *  Dave Stevenson 
+ *  Simon Mellor 
+ *  Luke Diamand 


All of these are now dead email addresses.
Mine could be updated to dave.steven...@raspberrypi.org, but the others 
should probably be deleted.



+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "mmal-common.h"
+#include "mmal-encodings.h"
+#include "mmal-vchiq.h"
+#include "mmal-msg.h"
+#include "mmal-parameters.h"
+#include "bcm2835-camera.h"
+
+#define BM2835_MMAL_VERSION "0.0.2"
+#define BM2835_MMAL_MODULE_NAME "bcm2835-v4l2"
+#define MIN_WIDTH 32
+#define MIN_HEIGHT 32
+#define MIN_BUFFER_SIZE (80*1024)
+
+#define MAX_VIDEO_MODE_WIDTH 1280
+#define MAX_VIDEO_MODE_HEIGHT 720


Hmm... Doesn't the max resolution depend on the sensor?


+
+#define MAX_BCM2835_CAMERAS 2
+
+MODULE_DESCRIPTION("Broadcom 2835 MMAL video capture");
+MODULE_AUTHOR("Vincent Sanders");
+MODULE_LICENSE("GPL");
+MODULE_VERSION(BM2835_MMAL_VERSION);
+
+int bcm2835_v4l2_debug;
+module_param_named(debug, bcm2835_v4l2_debug, int, 0644);
+MODULE_PARM_DESC(bcm2835_v4l2_debug, "Debug level 0-2");
+
+#define UNSET (-1)
+static int video_nr[] = {[0 ... (MAX_BCM2835_CAMERAS - 1)] = UNSET };
+module_param_array(video_nr, int, NULL, 0644);
+MODULE_PARM_DESC(video_nr, "videoX start numbers, -1 is autodetect");
+
+static int max_video_width = MAX_VIDEO_MODE_WIDTH;
+static int max_video_height = MAX_VIDEO_MODE_HEIGHT;
+module_param(max_video_width, int, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+MODULE_PARM_DESC(max_video_width, "Threshold for video mode");
+module_param(max_video_height, int, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+MODULE_PARM_DESC(max_video_height, "Threshold for video mode");


That seems a terrible hack! let the user specify the resolution via
modprobe parameter... That should depend on the hardware capabilities
instead.


This is sitting on top of an OpenMaxIL style camera component (though 
accessed via MMAL - long story, but basically MMAL removed a bundle of 
the ugly/annoying parts of IL).
It has the extension above V1.1.2 that you have a preview port, video 
capture port, and stills capture port. Stills captures have additional 
processing stages to improve image quality, whilst video has to maintain 
framerate.


If you're asked for YUV or RGB frame, how do you choose between video or 
stills? That's what is being set with these parameters, not the sensor 
resolution. Having independent stills and video processing options 
doesn't appear to be something that is supported in V4L2, but I'm open 
to suggestions.
There were thoughts that they could be exposed as different /dev/videoN 
devices, but that then poses a quandry to the client app as to which 
node to open, so complicates the client significantly. On the plus side 
it would then allow for things like zero shutter lag captures, and 
stills during video, where you want multiple streams (apparently) 
simultaneously, but is that worth the complexity? The general view was no.



+
+/* Gstreamer bug https://bugzilla.gnome.org/show_bug.cgi?id=726521
+ * v4l2src does bad (and actually wrong) things when the vidioc_enum_framesizes
+ * function says type V4L2_FRMSIZE_TYPE_STEPWISE, which we do by default.
+ * It's happier if we just don't say anything at all, when it then
+ * sets up a load of defaults that it thinks might work.
+ * If gst_v4l2src_is_broken is non-zero, then we remove the function from
+ * our function table list (actually switch to an alternate set, but same
+ * result).
+ */
+static int gst_v4l2src_is_broken;
+module_param(gst_v4l2src_is_broken, int, S_IRUSR | S_IWUSR | S_IRGRP | 
S_IROTH);
+MODULE_PARM_DESC(gst_v4l2src_is_broken, "If non-zero, enable workaround for 
Gstreamer");


Not sure if I liked this hack here. AFAIKT, GStreamer fixed the bug with
V4L2_FRMSIZE_TYPE_STEPWISE already.


I will double check on Monday. The main Raspberry Pi distribution is 
based on Debian, so packages can be quite out of date. This bug 
certainly affected Wheezy, but I don't know for certain about Jessie. 
Sid still hasn't been adopted.


Also be aware that exactly the same issue of not supporting 
V4L2_FRMSIZE_TYPE_STEPWISE affects Chromium for WebRTC, and they seem 
not to be too bothered about fixing it