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

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

[Stefan Wahren]

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(-)

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peter Robinson]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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.

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Stefan Wahren]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 <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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peter Robinson]

>   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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel