[libcamera-devel] PinePhone needs S_FRAME_INTERVAL
diff mbox series

Message ID 20220704221548.GB8002@amd
State New
Headers show
Series
  • [libcamera-devel] PinePhone needs S_FRAME_INTERVAL
Related show

Commit Message

Pavel Machek July 4, 2022, 10:15 p.m. UTC 
Hi!

It looks I solved another mystery: Top resolution was not available,
and depending on order of commands, sometimes 1280x720 was not
available, either.

So I took a look into megapixels, and mystery solved -- libcamera is
not setting frame intervals at all.

AFAICT that needs to happen. PinePhone needs lower fps for top
resolution, but an use high fps for lower resolutions...

This is obviosly a hack, but we need something like that.

Best regards,
							Pavel

commit 66616c78007c08fcd3fde66888c2210a392b184e
Author: Pavel Machek <pavel@ucw.cz>
Date:   Mon Jul 4 23:54:22 2022 +0200

    S_FRAME_INTERVAL is needed, or things don't work.

index fba90e20..0ee6ac30 100644

Comments

Laurent Pinchart July 4, 2022, 10:28 p.m. UTC | #1 
Hi Pavel,

On Tue, Jul 05, 2022 at 12:15:49AM +0200, Pavel Machek wrote:
> Hi!
> 
> It looks I solved another mystery: Top resolution was not available,
> and depending on order of commands, sometimes 1280x720 was not
> available, either.
> 
> So I took a look into megapixels, and mystery solved -- libcamera is
> not setting frame intervals at all.
> 
> AFAICT that needs to happen. PinePhone needs lower fps for top
> resolution, but an use high fps for lower resolutions...
> 
> This is obviosly a hack, but we need something like that.

VIDIOC_SUBDEV_S_FRAME_INTERVAL is deprecated for camera sensors and
should never have been used. Frame rates are controlled through the
pixel clock and the horizontal and vertical blanking values. If a sensor
driver requires VIDIOC_SUBDEV_S_FRAME_INTERVAL, then it should be fixed.

> commit 66616c78007c08fcd3fde66888c2210a392b184e
> Author: Pavel Machek <pavel@ucw.cz>
> Date:   Mon Jul 4 23:54:22 2022 +0200
> 
>     S_FRAME_INTERVAL is needed, or things don't work.
> 
> index fba90e20..0ee6ac30 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -452,6 +452,21 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>  			     Whence whence)
>  {
>  	struct v4l2_subdev_format subdevFmt = {};
> +
> +	{
> +	  struct v4l2_subdev_frame_interval interval = {};
> +                interval.pad = 0;
> +                interval.interval.numerator = 1;
> +                interval.interval.denominator = 10;
> +                if (int ret = ioctl(VIDIOC_SUBDEV_S_FRAME_INTERVAL,
> +                           &interval) == -1) {
> +			LOG(V4L2, Error)
> +			  << "Unable to set frame interval on pad " << pad
> +			  << ": " << strerror(-ret);
> +                        return ret;
> +                }
> +
> +	}
>  	subdevFmt.which = whence == ActiveFormat ? V4L2_SUBDEV_FORMAT_ACTIVE
>  			: V4L2_SUBDEV_FORMAT_TRY;
>  	subdevFmt.pad = pad;
>
Jacopo Mondi July 5, 2022, 7:17 a.m. UTC | #2 
Hi Pavel,

On Tue, Jul 05, 2022 at 12:15:49AM +0200, Pavel Machek via libcamera-devel wrote:
> Hi!
>
> It looks I solved another mystery: Top resolution was not available,
> and depending on order of commands, sometimes 1280x720 was not
> available, either.
>
> So I took a look into megapixels, and mystery solved -- libcamera is
> not setting frame intervals at all.
>
> AFAICT that needs to happen. PinePhone needs lower fps for top
> resolution, but an use high fps for lower resolutions...
>
> This is obviosly a hack, but we need something like that.
>

Can I ask a stupid question ? Is the sun6i a parallel or CSI-2
receiver ? If you're using my ov5640 branch (patches will land
upstream in 5.20) the need to set frame_interval to work around a
driver deficiency is now only for parallel setups. CSI-2 platforms have
been switched to use VBLANK to control the frame rate.

For reference:
https://git.sr.ht/~jmondi_/linux/tree/jmondi/media-master/ov5640-v7

> Best regards,
> 							Pavel
>
> commit 66616c78007c08fcd3fde66888c2210a392b184e
> Author: Pavel Machek <pavel@ucw.cz>
> Date:   Mon Jul 4 23:54:22 2022 +0200
>
>     S_FRAME_INTERVAL is needed, or things don't work.
>
> index fba90e20..0ee6ac30 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -452,6 +452,21 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>  			     Whence whence)
>  {
>  	struct v4l2_subdev_format subdevFmt = {};
> +
> +	{
> +	  struct v4l2_subdev_frame_interval interval = {};
> +                interval.pad = 0;
> +                interval.interval.numerator = 1;
> +                interval.interval.denominator = 10;
> +                if (int ret = ioctl(VIDIOC_SUBDEV_S_FRAME_INTERVAL,
> +                           &interval) == -1) {
> +			LOG(V4L2, Error)
> +			  << "Unable to set frame interval on pad " << pad
> +			  << ": " << strerror(-ret);
> +                        return ret;
> +                }
> +
> +	}
>  	subdevFmt.which = whence == ActiveFormat ? V4L2_SUBDEV_FORMAT_ACTIVE
>  			: V4L2_SUBDEV_FORMAT_TRY;
>  	subdevFmt.pad = pad;
>
> --
> People of Russia, stop Putin before his war on Ukraine escalates.
Pavel Machek July 5, 2022, 8:49 p.m. UTC | #3 
Hi!

> > It looks I solved another mystery: Top resolution was not available,
> > and depending on order of commands, sometimes 1280x720 was not
> > available, either.
> > 
> > So I took a look into megapixels, and mystery solved -- libcamera is
> > not setting frame intervals at all.
> > 
> > AFAICT that needs to happen. PinePhone needs lower fps for top
> > resolution, but an use high fps for lower resolutions...
> > 
> > This is obviosly a hack, but we need something like that.
> 
> VIDIOC_SUBDEV_S_FRAME_INTERVAL is deprecated for camera sensors and
> should never have been used. Frame rates are controlled through the
> pixel clock and the horizontal and vertical blanking values. If a sensor
> driver requires VIDIOC_SUBDEV_S_FRAME_INTERVAL, then it should be
> fixed.

Hmm. I can't really change the kernel at the moment.

But... However you call frames per second, it is quite an important
parameter that needs to be controlled by the user, no? Is there plan
to include it in the stream options

  -s, --stream key=value[,key=value,...] ...            Set configuration of a camera stream
            height=integer                                Height in pixels
	    pixelformat=string                            Pixel format name
	    role=string                                   Role for the stream (viewfinder, video, still, raw)
	    width=integer                                 Width in pixels
					  
Confusing thing for me is that without S_FRAME_INTERVAL being set
explicitely, old value was being used, leading to
hard-to-understand/debug problems. I guess that is no longer going to
be a problem with fixed kernel drivers.

Best regards,
								Pavel
Pavel Machek July 5, 2022, 8:56 p.m. UTC | #4 
Hi!

> > It looks I solved another mystery: Top resolution was not available,
> > and depending on order of commands, sometimes 1280x720 was not
> > available, either.
> >
> > So I took a look into megapixels, and mystery solved -- libcamera is
> > not setting frame intervals at all.
> >
> > AFAICT that needs to happen. PinePhone needs lower fps for top
> > resolution, but an use high fps for lower resolutions...
> >
> > This is obviosly a hack, but we need something like that.
> >
> 
> Can I ask a stupid question ? Is the sun6i a parallel or CSI-2
> receiver ? If you're using my ov5640 branch (patches will land
> upstream in 5.20) the need to set frame_interval to work around a
> driver deficiency is now only for parallel setups. CSI-2 platforms have
> been switched to use VBLANK to control the frame rate.

I'm not really sure how to tell the difference. media-ctl mentions
csi:

mobian@mobian:~/g/libcamera$ sudo media-ctl -p -d /dev/media1
Media controller API version 5.15.44

Media device information
------------------------
driver          sun6i-csi
model           Allwinner Video Capture Device
serial
bus info        platform:1cb0000.csi
hw revision     0x0
driver version  5.15.44

Device topology
- entity 1: sun6i-csi (1 pad, 2 links)
 type Node subtype V4L flags 0
 device node name /dev/video2
 pad0: Sink
 <- "gc2145 3-003c":0 []
 <- "ov5640 3-004c":0 [ENABLED]
						   
I'm using "Linux mobian 5.15-sunxi64 #1 SMP Tue Jun 7 01:08:50 UTC 2022
aarch64 GNU/Linux" kernel from mobian, and can not really change it
easily at the moment.

Best regards,
								Pavel
Jacopo Mondi July 6, 2022, 6:43 a.m. UTC | #5 
Hi Pavel,

On Tue, Jul 05, 2022 at 10:56:06PM +0200, Pavel Machek wrote:
> Hi!
>
> > > It looks I solved another mystery: Top resolution was not available,
> > > and depending on order of commands, sometimes 1280x720 was not
> > > available, either.
> > >
> > > So I took a look into megapixels, and mystery solved -- libcamera is
> > > not setting frame intervals at all.
> > >
> > > AFAICT that needs to happen. PinePhone needs lower fps for top
> > > resolution, but an use high fps for lower resolutions...
> > >
> > > This is obviosly a hack, but we need something like that.
> > >
> >
> > Can I ask a stupid question ? Is the sun6i a parallel or CSI-2
> > receiver ? If you're using my ov5640 branch (patches will land
> > upstream in 5.20) the need to set frame_interval to work around a
> > driver deficiency is now only for parallel setups. CSI-2 platforms have
> > been switched to use VBLANK to control the frame rate.
>
> I'm not really sure how to tell the difference. media-ctl mentions
> csi:

No worries, if you can't update the kernel it doesn't make a
difference. I thought you were using the same kernel as Rafael with
the ov5640 patches in. If you're on the current driver version you
need the awkward s_frame_interval dance.

>
> mobian@mobian:~/g/libcamera$ sudo media-ctl -p -d /dev/media1
> Media controller API version 5.15.44
>
> Media device information
> ------------------------
> driver          sun6i-csi
> model           Allwinner Video Capture Device
> serial
> bus info        platform:1cb0000.csi
> hw revision     0x0
> driver version  5.15.44
>
> Device topology
> - entity 1: sun6i-csi (1 pad, 2 links)
>  type Node subtype V4L flags 0
>  device node name /dev/video2
>  pad0: Sink
>  <- "gc2145 3-003c":0 []
>  <- "ov5640 3-004c":0 [ENABLED]
>
> I'm using "Linux mobian 5.15-sunxi64 #1 SMP Tue Jun 7 01:08:50 UTC 2022
> aarch64 GNU/Linux" kernel from mobian, and can not really change it
> easily at the moment.
>
> Best regards,
> 								Pavel
> --
> People of Russia, stop Putin before his war on Ukraine escalates.
Pavel Machek July 6, 2022, 6:16 p.m. UTC | #6 
Hi!

> > > Can I ask a stupid question ? Is the sun6i a parallel or CSI-2
> > > receiver ? If you're using my ov5640 branch (patches will land
> > > upstream in 5.20) the need to set frame_interval to work around a
> > > driver deficiency is now only for parallel setups. CSI-2 platforms have
> > > been switched to use VBLANK to control the frame rate.
> >
> > I'm not really sure how to tell the difference. media-ctl mentions
> > csi:
> 
> No worries, if you can't update the kernel it doesn't make a
> difference. I thought you were using the same kernel as Rafael with
> the ov5640 patches in. If you're on the current driver version you
> need the awkward s_frame_interval dance.

Rafael is developing Maemo Leste, and I'm currently using Mobian.

If it is easy to boot Leste from SD card, I may try that, but not this
week. I'm actually using Leste on Droid4 and would not mind same
system on both.

Best regards,
								Pavel
Pavel Machek July 8, 2022, 12:51 p.m. UTC | #7 
Hi!

> > AFAICT that needs to happen. PinePhone needs lower fps for top
> > resolution, but an use high fps for lower resolutions...
> > 
> > This is obviosly a hack, but we need something like that.
> 
> VIDIOC_SUBDEV_S_FRAME_INTERVAL is deprecated for camera sensors and
> should never have been used. Frame rates are controlled through the
> pixel clock and the horizontal and vertical blanking values. If a sensor
> driver requires VIDIOC_SUBDEV_S_FRAME_INTERVAL, then it should be
> fixed.

I guess I know understand why VIDIOC_SUBDEV_S_FRAME_INTERVAL is bad
idea. Still we may want stream option to select lower fps when
requested.

Anyway libcamerasrc started working for me. I set
VIDIOC_SUBDEV_S_FRAME_INTERVAL to 1/60, that may be related.

export GST_DEBUG=libcamera*:7
export GST_PLUGIN_PATH=$(pwd)/build/src/gstreamer
gst-launch-1.0 libcamerasrc camera-name=/base/i2c-csi/rear-camera@4c !
'video/x-raw,format=YUY2,width=640,height=480' ! autovideosink

Produces image with obivous errors, but other settings with 1/30
worked ok. Let me verify...

gst-launch-1.0 libcamerasrc camera-name=/base/i2c-csi/rear-camera@4c !
'video/x-raw,format=YUY2,width=1280,height=720' ! autovideosink

This one works. There's slight color change between left and right
half of the image, not sure what I should think about it.

Best regards,
								Pavel

Patch
diff mbox series 

--- a/src/libcamera/v4l2_subdevice.cpp
+++ b/src/libcamera/v4l2_subdevice.cpp
@@ -452,6 +452,21 @@  int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
 			     Whence whence)
 {
 	struct v4l2_subdev_format subdevFmt = {};
+
+	{
+	  struct v4l2_subdev_frame_interval interval = {};
+                interval.pad = 0;
+                interval.interval.numerator = 1;
+                interval.interval.denominator = 10;
+                if (int ret = ioctl(VIDIOC_SUBDEV_S_FRAME_INTERVAL,
+                           &interval) == -1) {
+			LOG(V4L2, Error)
+			  << "Unable to set frame interval on pad " << pad
+			  << ": " << strerror(-ret);
+                        return ret;
+                }
+
+	}
 	subdevFmt.which = whence == ActiveFormat ? V4L2_SUBDEV_FORMAT_ACTIVE
 			: V4L2_SUBDEV_FORMAT_TRY;
 	subdevFmt.pad = pad;