| Message ID | 1561414395-12518-1-git-send-email-wahrenst@gmx.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: bcm2835-camera: Restore return behavior of ctrl_set_bitrate() | expand |
On Mon, Jun 24, 2019 at 11:13 PM Stefan Wahren <wahrenst@gmx.net> 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 <wahrenst@gmx.net> Tested-by: Peter Robinson <pbrobinson@gmail.com> Thanks Stefan, I can confirm this resolves the issue I have seen with the camera on 5.2 but hadn't had the time to bisect it yet. Tested with a v2.1 camera module attached to a RPI3A+ Regards, Peter > --- > drivers/staging/vc04_services/bcm2835-camera/controls.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c b/drivers/staging/vc04_services/bcm2835-camera/controls.c > index d60e378..1c4c9e8 100644 > --- a/drivers/staging/vc04_services/bcm2835-camera/controls.c > +++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c > @@ -610,9 +610,11 @@ static int ctrl_set_bitrate(struct bm2835_mmal_dev *dev, > > encoder_out = &dev->component[MMAL_COMPONENT_VIDEO_ENCODE]->output[0]; > > - return vchiq_mmal_port_parameter_set(dev->instance, encoder_out, > - mmal_ctrl->mmal_id, &ctrl->val, > - sizeof(ctrl->val)); > + vchiq_mmal_port_parameter_set(dev->instance, encoder_out, > + mmal_ctrl->mmal_id, &ctrl->val, > + sizeof(ctrl->val)); > + > + return 0; > } > > static int ctrl_set_bitrate_mode(struct bm2835_mmal_dev *dev, > -- > 2.7.4 > > > _______________________________________________ > linux-rpi-kernel mailing list > linux-rpi-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel
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 <wahrenst@gmx.net> 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. 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; regards, dan carpenter
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 <wahrenst@gmx.net> > 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 30000/1000 to 30000/1000 [ 8.163104] bcm2835-v4l2: Set fps range to 30000/1000 to 30000/1000 [ 8.163624] bcm2835-v4l2: Set fps range to 30000/1000 to 30000/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:10000000 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:10000000 Any thoughts? > > > regards, > dan carpenter > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Stefan. On Tue, 25 Jun 2019 at 17:30, Stefan Wahren <wahrenst@gmx.net> 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 <wahrenst@gmx.net> > > 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 30000/1000 to 30000/1000 > [ 8.163104] bcm2835-v4l2: Set fps range to 30000/1000 to 30000/1000 > [ 8.163624] bcm2835-v4l2: Set fps range to 30000/1000 to 30000/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:10000000 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:10000000 > > 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.
> 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. No, but it never really has for me, it's always been weird in what it allows through by default and the admin has to approve a lot of things so sometimes you'll get 100s of mails at once when who ever the admin is catches up.
diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c b/drivers/staging/vc04_services/bcm2835-camera/controls.c index d60e378..1c4c9e8 100644 --- a/drivers/staging/vc04_services/bcm2835-camera/controls.c +++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c @@ -610,9 +610,11 @@ static int ctrl_set_bitrate(struct bm2835_mmal_dev *dev, encoder_out = &dev->component[MMAL_COMPONENT_VIDEO_ENCODE]->output[0]; - return vchiq_mmal_port_parameter_set(dev->instance, encoder_out, - mmal_ctrl->mmal_id, &ctrl->val, - sizeof(ctrl->val)); + vchiq_mmal_port_parameter_set(dev->instance, encoder_out, + mmal_ctrl->mmal_id, &ctrl->val, + sizeof(ctrl->val)); + + return 0; } static int ctrl_set_bitrate_mode(struct bm2835_mmal_dev *dev,
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 <wahrenst@gmx.net> --- drivers/staging/vc04_services/bcm2835-camera/controls.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) -- 2.7.4