| Message ID | 20221005190613.394277-6-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ar0521: Add analog gain, rework clock tree | expand |
Hi Jacopo On Wed, 5 Oct 2022 at 20:07, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Add support for V4L2_CID_LINK_FREQ which currently reports a single > hard-coded frequency which depends on the fixed pixel clock. > > This will change in the next patches where the pixel rate will be > computed from the desired link_frequency. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Looks valid based on the current pixel rate of 184MPix/s, 8bpp, divided by 4 lanes, and DDR. > --- > drivers/media/i2c/ar0521.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > index 21649aecf442..c5410b091654 100644 > --- a/drivers/media/i2c/ar0521.c > +++ b/drivers/media/i2c/ar0521.c > @@ -90,6 +90,10 @@ static const char * const ar0521_supply_names[] = { > "vaa", /* Analog (2.7V) supply */ > }; > > +static const s64 ar0521_link_frequencies[] = { > + 184000000, > +}; > + > struct ar0521_ctrls { > struct v4l2_ctrl_handler handler; > struct v4l2_ctrl *ana_gain; > @@ -104,6 +108,7 @@ struct ar0521_ctrls { > }; > struct v4l2_ctrl *pixrate; > struct v4l2_ctrl *exposure; > + struct v4l2_ctrl *link_freq; > struct v4l2_ctrl *test_pattern; > }; > > @@ -655,6 +660,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor) > ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0, > 65535, 1, 360); > > + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, > + ARRAY_SIZE(ar0521_link_frequencies) - 1, > + 0, ar0521_link_frequencies); > + Admittedly there is only one entry, but did you want to make it a read only control? With no case for it in s_ctrl, you'll get errors thrown from the control handler framework. Dave > ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops, > V4L2_CID_TEST_PATTERN, > ARRAY_SIZE(test_pattern_menu) - 1, > -- > 2.37.3 >
On Thu, Oct 06, 2022 at 04:10:10PM +0100, Dave Stevenson wrote: > On Wed, 5 Oct 2022 at 20:07, Jacopo Mondi wrote: > > > > Add support for V4L2_CID_LINK_FREQ which currently reports a single > > hard-coded frequency which depends on the fixed pixel clock. > > > > This will change in the next patches where the pixel rate will be > > computed from the desired link_frequency. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > Looks valid based on the current pixel rate of 184MPix/s, 8bpp, > divided by 4 lanes, and DDR. > > > --- > > drivers/media/i2c/ar0521.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > > index 21649aecf442..c5410b091654 100644 > > --- a/drivers/media/i2c/ar0521.c > > +++ b/drivers/media/i2c/ar0521.c > > @@ -90,6 +90,10 @@ static const char * const ar0521_supply_names[] = { > > "vaa", /* Analog (2.7V) supply */ > > }; > > > > +static const s64 ar0521_link_frequencies[] = { > > + 184000000, > > +}; > > + > > struct ar0521_ctrls { > > struct v4l2_ctrl_handler handler; > > struct v4l2_ctrl *ana_gain; > > @@ -104,6 +108,7 @@ struct ar0521_ctrls { > > }; > > struct v4l2_ctrl *pixrate; > > struct v4l2_ctrl *exposure; > > + struct v4l2_ctrl *link_freq; > > struct v4l2_ctrl *test_pattern; > > }; > > > > @@ -655,6 +660,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor) > > ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0, > > 65535, 1, 360); > > > > + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, > > + ARRAY_SIZE(ar0521_link_frequencies) - 1, > > + 0, ar0521_link_frequencies); > > + > > Admittedly there is only one entry, but did you want to make it a read > only control? With no case for it in s_ctrl, you'll get errors thrown > from the control handler framework. I'd make it writable even if there's a single entry, so that userspace won't need special logic. It will also prepare for support of multiple entries in the future. > > ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops, > > V4L2_CID_TEST_PATTERN, > > ARRAY_SIZE(test_pattern_menu) - 1,
On Fri, 7 Oct 2022 at 15:01, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Thu, Oct 06, 2022 at 04:10:10PM +0100, Dave Stevenson wrote: > > On Wed, 5 Oct 2022 at 20:07, Jacopo Mondi wrote: > > > > > > Add support for V4L2_CID_LINK_FREQ which currently reports a single > > > hard-coded frequency which depends on the fixed pixel clock. > > > > > > This will change in the next patches where the pixel rate will be > > > computed from the desired link_frequency. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Looks valid based on the current pixel rate of 184MPix/s, 8bpp, > > divided by 4 lanes, and DDR. > > > > > --- > > > drivers/media/i2c/ar0521.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > > > index 21649aecf442..c5410b091654 100644 > > > --- a/drivers/media/i2c/ar0521.c > > > +++ b/drivers/media/i2c/ar0521.c > > > @@ -90,6 +90,10 @@ static const char * const ar0521_supply_names[] = { > > > "vaa", /* Analog (2.7V) supply */ > > > }; > > > > > > +static const s64 ar0521_link_frequencies[] = { > > > + 184000000, > > > +}; > > > + > > > struct ar0521_ctrls { > > > struct v4l2_ctrl_handler handler; > > > struct v4l2_ctrl *ana_gain; > > > @@ -104,6 +108,7 @@ struct ar0521_ctrls { > > > }; > > > struct v4l2_ctrl *pixrate; > > > struct v4l2_ctrl *exposure; > > > + struct v4l2_ctrl *link_freq; > > > struct v4l2_ctrl *test_pattern; > > > }; > > > > > > @@ -655,6 +660,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor) > > > ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0, > > > 65535, 1, 360); > > > > > > + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, > > > + ARRAY_SIZE(ar0521_link_frequencies) - 1, > > > + 0, ar0521_link_frequencies); > > > + > > > > Admittedly there is only one entry, but did you want to make it a read > > only control? With no case for it in s_ctrl, you'll get errors thrown > > from the control handler framework. > > I'd make it writable even if there's a single entry, so that userspace > won't need special logic. It will also prepare for support of multiple > entries in the future. Do you really see a situation where userspace will be configuring link frequency instead of DT / ACPI? A quick search seems to imply that only 1 current driver supports a r/w link frequency - mt9v032. That would imply that having a controllable link frequency would require the special logic in userspace. I'm always very cautious about drivers that are linking PIXEL_RATE and LINK_FREQ - most of the sensors are tending to have 2 (or more) PLLs, and there is a FIFO between the image sensor (PIXEL_RATE) and the MIPI block (LINK_FREQ). imx290 is certainly wrong (pixel rate does not change with mode, but link freq does), and I'm fairly certain that ov7251 is as well (pixel rate is 48MPix/s whether at 240 or 319.2MHz link frequency). Patches coming soon for both. Dave > > > ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops, > > > V4L2_CID_TEST_PATTERN, > > > ARRAY_SIZE(test_pattern_menu) - 1, > > -- > Regards, > > Laurent Pinchart
Hi Dave, On Fri, Oct 07, 2022 at 03:26:55PM +0100, Dave Stevenson wrote: > On Fri, 7 Oct 2022 at 15:01, Laurent Pinchart wrote: > > On Thu, Oct 06, 2022 at 04:10:10PM +0100, Dave Stevenson wrote: > > > On Wed, 5 Oct 2022 at 20:07, Jacopo Mondi wrote: > > > > > > > > Add support for V4L2_CID_LINK_FREQ which currently reports a single > > > > hard-coded frequency which depends on the fixed pixel clock. > > > > > > > > This will change in the next patches where the pixel rate will be > > > > computed from the desired link_frequency. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > Looks valid based on the current pixel rate of 184MPix/s, 8bpp, > > > divided by 4 lanes, and DDR. > > > > > > > --- > > > > drivers/media/i2c/ar0521.c | 9 +++++++++ > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > > > > index 21649aecf442..c5410b091654 100644 > > > > --- a/drivers/media/i2c/ar0521.c > > > > +++ b/drivers/media/i2c/ar0521.c > > > > @@ -90,6 +90,10 @@ static const char * const ar0521_supply_names[] = { > > > > "vaa", /* Analog (2.7V) supply */ > > > > }; > > > > > > > > +static const s64 ar0521_link_frequencies[] = { > > > > + 184000000, > > > > +}; > > > > + > > > > struct ar0521_ctrls { > > > > struct v4l2_ctrl_handler handler; > > > > struct v4l2_ctrl *ana_gain; > > > > @@ -104,6 +108,7 @@ struct ar0521_ctrls { > > > > }; > > > > struct v4l2_ctrl *pixrate; > > > > struct v4l2_ctrl *exposure; > > > > + struct v4l2_ctrl *link_freq; > > > > struct v4l2_ctrl *test_pattern; > > > > }; > > > > > > > > @@ -655,6 +660,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor) > > > > ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0, > > > > 65535, 1, 360); > > > > > > > > + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, > > > > + ARRAY_SIZE(ar0521_link_frequencies) - 1, > > > > + 0, ar0521_link_frequencies); > > > > + > > > > > > Admittedly there is only one entry, but did you want to make it a read > > > only control? With no case for it in s_ctrl, you'll get errors thrown > > > from the control handler framework. > > > > I'd make it writable even if there's a single entry, so that userspace > > won't need special logic. It will also prepare for support of multiple > > entries in the future. > > Do you really see a situation where userspace will be configuring link > frequency instead of DT / ACPI? > A quick search seems to imply that only 1 current driver supports a > r/w link frequency - mt9v032. That would imply that having a > controllable link frequency would require the special logic in > userspace. Looking at the mainline kernel only, the N9, N950 and Librem5 all specify multiple link frequencies, and so does the sdm845-db845c development board. This indeed requires specific logic in userspace, and to be honest, I haven't really thought about how it would be implemented. Sakari has more experience than me here, he may be able to shed some light. > I'm always very cautious about drivers that are linking PIXEL_RATE and > LINK_FREQ - most of the sensors are tending to have 2 (or more) PLLs, > and there is a FIFO between the image sensor (PIXEL_RATE) and the MIPI > block (LINK_FREQ). imx290 is certainly wrong (pixel rate does not > change with mode, but link freq does), and I'm fairly certain that > ov7251 is as well (pixel rate is 48MPix/s whether at 240 or 319.2MHz > link frequency). Patches coming soon for both. That's a good point, different link frequencies may or may not result in different pixel rates. > > > > ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops, > > > > V4L2_CID_TEST_PATTERN, > > > > ARRAY_SIZE(test_pattern_menu) - 1,
Hi Dave On Fri, Oct 07, 2022 at 03:26:55PM +0100, Dave Stevenson wrote: > On Fri, 7 Oct 2022 at 15:01, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > On Thu, Oct 06, 2022 at 04:10:10PM +0100, Dave Stevenson wrote: > > > On Wed, 5 Oct 2022 at 20:07, Jacopo Mondi wrote: > > > > > > > > Add support for V4L2_CID_LINK_FREQ which currently reports a single > > > > hard-coded frequency which depends on the fixed pixel clock. > > > > > > > > This will change in the next patches where the pixel rate will be > > > > computed from the desired link_frequency. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > Looks valid based on the current pixel rate of 184MPix/s, 8bpp, > > > divided by 4 lanes, and DDR. > > > > > > > --- > > > > drivers/media/i2c/ar0521.c | 9 +++++++++ > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > > > > index 21649aecf442..c5410b091654 100644 > > > > --- a/drivers/media/i2c/ar0521.c > > > > +++ b/drivers/media/i2c/ar0521.c > > > > @@ -90,6 +90,10 @@ static const char * const ar0521_supply_names[] = { > > > > "vaa", /* Analog (2.7V) supply */ > > > > }; > > > > > > > > +static const s64 ar0521_link_frequencies[] = { > > > > + 184000000, > > > > +}; > > > > + > > > > struct ar0521_ctrls { > > > > struct v4l2_ctrl_handler handler; > > > > struct v4l2_ctrl *ana_gain; > > > > @@ -104,6 +108,7 @@ struct ar0521_ctrls { > > > > }; > > > > struct v4l2_ctrl *pixrate; > > > > struct v4l2_ctrl *exposure; > > > > + struct v4l2_ctrl *link_freq; > > > > struct v4l2_ctrl *test_pattern; > > > > }; > > > > > > > > @@ -655,6 +660,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor) > > > > ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0, > > > > 65535, 1, 360); > > > > > > > > + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, > > > > + ARRAY_SIZE(ar0521_link_frequencies) - 1, > > > > + 0, ar0521_link_frequencies); > > > > + > > > > > > Admittedly there is only one entry, but did you want to make it a read > > > only control? With no case for it in s_ctrl, you'll get errors thrown > > > from the control handler framework. > > > > I'd make it writable even if there's a single entry, so that userspace > > won't need special logic. It will also prepare for support of multiple > > entries in the future. > > Do you really see a situation where userspace will be configuring link > frequency instead of DT / ACPI? > A quick search seems to imply that only 1 current driver supports a > r/w link frequency - mt9v032. That would imply that having a > controllable link frequency would require the special logic in > userspace. > Yes it does, but we need one way or another to allow userspace to control the sampling frequency as extending (or shrinking) blankings helps up to the point you reach their limits. I was never really fond of the idea that such action should go through link frequency, which seems very much a parameter of the bus that should be negotiated between the recv and the tx parts, rather than being user configurable. > I'm always very cautious about drivers that are linking PIXEL_RATE and > LINK_FREQ - most of the sensors are tending to have 2 (or more) PLLs, > and there is a FIFO between the image sensor (PIXEL_RATE) and the MIPI > block (LINK_FREQ). imx290 is certainly wrong (pixel rate does not > change with mode, but link freq does), and I'm fairly certain that > ov7251 is as well (pixel rate is 48MPix/s whether at 240 or 319.2MHz > link frequency). Patches coming soon for both. The current definition of PIXEL_RATE indeed describes the sampling frequency on the pixel array, which might or might not reflect the output pixel rate. However most if not all usages of PIXEL_RATE I've seen (and FTR the way it is used in libcamera) is to denote the output pixel rate (ie it is used to compute the output timings given the line length and frame height) I wonder 1) The current definition of PIXEL_RATE as the sampling rate on the pixel array: what purposes does it serve ? Are there algorithms that require to know the sampling rate in the analog domain ? Are there implementations that treat PIXEL_RATE differently than the "pixel output rate" ? 2) LINK_FREQ is the closest control we have to express the output pixel rate, but to me is very specific to the bus configuration and does not express per se anything useful to userspace for computing timings based on frame/lane sizes. The fact LINK_FREQ is a menu contol reflects how much it relates to the HW configuration as it is assumed to come from DT Do we need an r/w PIXEL_OUTPUT_RATE control to replace - LINK_FREQ for userspace to configure it - PIXEL_RATE for userspace to read it LINK_FREQ should only be used in the tx/rx negotiation. It shall vary according to PIXEL_OUTPUT_RATE, possibily in the options specified in DTS (which are there because they have usually been validated for RF emissions, that's my understanding at least). PIXEL_RATE will equally vary, if required, and algorithms that need to know the sampling frequency in the analog domain will continue using it. Or maybe the original idea was to have a pixel array entity and a separate tx entity, each of them with different PIXEL_RATE control ? > > Dave > > > > > ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops, > > > > V4L2_CID_TEST_PATTERN, > > > > ARRAY_SIZE(test_pattern_menu) - 1, > > > > -- > > Regards, > > > > Laurent Pinchart
Hi Jacopo On Mon, 17 Oct 2022 at 10:24, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Dave > > On Fri, Oct 07, 2022 at 03:26:55PM +0100, Dave Stevenson wrote: > > On Fri, 7 Oct 2022 at 15:01, Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > On Thu, Oct 06, 2022 at 04:10:10PM +0100, Dave Stevenson wrote: > > > > On Wed, 5 Oct 2022 at 20:07, Jacopo Mondi wrote: > > > > > > > > > > Add support for V4L2_CID_LINK_FREQ which currently reports a single > > > > > hard-coded frequency which depends on the fixed pixel clock. > > > > > > > > > > This will change in the next patches where the pixel rate will be > > > > > computed from the desired link_frequency. > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > > Looks valid based on the current pixel rate of 184MPix/s, 8bpp, > > > > divided by 4 lanes, and DDR. > > > > > > > > > --- > > > > > drivers/media/i2c/ar0521.c | 9 +++++++++ > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > > > > > index 21649aecf442..c5410b091654 100644 > > > > > --- a/drivers/media/i2c/ar0521.c > > > > > +++ b/drivers/media/i2c/ar0521.c > > > > > @@ -90,6 +90,10 @@ static const char * const ar0521_supply_names[] = { > > > > > "vaa", /* Analog (2.7V) supply */ > > > > > }; > > > > > > > > > > +static const s64 ar0521_link_frequencies[] = { > > > > > + 184000000, > > > > > +}; > > > > > + > > > > > struct ar0521_ctrls { > > > > > struct v4l2_ctrl_handler handler; > > > > > struct v4l2_ctrl *ana_gain; > > > > > @@ -104,6 +108,7 @@ struct ar0521_ctrls { > > > > > }; > > > > > struct v4l2_ctrl *pixrate; > > > > > struct v4l2_ctrl *exposure; > > > > > + struct v4l2_ctrl *link_freq; > > > > > struct v4l2_ctrl *test_pattern; > > > > > }; > > > > > > > > > > @@ -655,6 +660,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor) > > > > > ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0, > > > > > 65535, 1, 360); > > > > > > > > > > + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, > > > > > + ARRAY_SIZE(ar0521_link_frequencies) - 1, > > > > > + 0, ar0521_link_frequencies); > > > > > + > > > > > > > > Admittedly there is only one entry, but did you want to make it a read > > > > only control? With no case for it in s_ctrl, you'll get errors thrown > > > > from the control handler framework. > > > > > > I'd make it writable even if there's a single entry, so that userspace > > > won't need special logic. It will also prepare for support of multiple > > > entries in the future. > > > > Do you really see a situation where userspace will be configuring link > > frequency instead of DT / ACPI? > > A quick search seems to imply that only 1 current driver supports a > > r/w link frequency - mt9v032. That would imply that having a > > controllable link frequency would require the special logic in > > userspace. > > > > Yes it does, but we need one way or another to allow userspace to > control the sampling frequency as extending (or shrinking) blankings > helps up to the point you reach their limits. > > I was never really fond of the idea that such action should go through > link frequency, which seems very much a parameter of the bus that > should be negotiated between the recv and the tx parts, rather than > being user configurable. Indeed - that's PIXEL_RATE, not LINK_FREQ. > > I'm always very cautious about drivers that are linking PIXEL_RATE and > > LINK_FREQ - most of the sensors are tending to have 2 (or more) PLLs, > > and there is a FIFO between the image sensor (PIXEL_RATE) and the MIPI > > block (LINK_FREQ). imx290 is certainly wrong (pixel rate does not > > change with mode, but link freq does), and I'm fairly certain that > > ov7251 is as well (pixel rate is 48MPix/s whether at 240 or 319.2MHz > > link frequency). Patches coming soon for both. > > The current definition of PIXEL_RATE indeed describes the sampling > frequency on the pixel array, which might or might not reflect the > output pixel rate. However most if not all usages of PIXEL_RATE I've > seen (and FTR the way it is used in libcamera) is to denote the output > pixel rate (ie it is used to compute the output timings given the line > length and frame height) > > I wonder > > 1) The current definition of PIXEL_RATE as the sampling rate on the > pixel array: what purposes does it serve ? Are there algorithms that > require to know the sampling rate in the analog domain ? Are there > implementations that treat PIXEL_RATE differently than the "pixel > output rate" ? Sampling rate on the array is the basis of using VBLANK and HBLANK to control frame rate. See documentation at [1] frame interval = (analogue crop width + horizontal blanking) * (analogue crop height + vertical blanking) / pixel rate This is NOT the pixel rate on the CSI link. Taking an example of IMX219 [2], section 9.1 shows the clock structure with 2 PLLs. PLL1 drives the pixel array (PIXEL_RATE). PLL2 drives the MIPI block (LINK_FREQ). There is a FIFO between the pixel array and MIPI, and therefore they can run at different rates. OV5647 is the same. IMX327/290/462 are the same, although FRSEL configures specific dividers for the PIXEL_RATE. OV9281 is the same (2 PLLs). In your case it does appear that LINK_FREQ and PIXEL_RATE are bound together. From the developer guide: "to reduce MIPI data rate, sensor pixel clock must be reduced as well" On AR0521 max frame interval is dictated by line_length_pck (0x0342) and frame_length_lines (0x0340), both of which are 16bit values. At your highest pixel rate of 414000000 I make that 10.37seconds per frame, or 0.09fps. So without altering link frequency, my calculations say you can do 0.09 to 60fps. Are you currently looking at use cases that need frame rates outside these limits? If not, why are you looking at changing the rate of anything? [1] https://www.kernel.org/doc/html/latest/driver-api/media/camera-sensor.html#raw-camera-sensors [2] https://github.com/rellimmot/Sony-IMX219-Raspberry-Pi-V2-CMOS/blob/master/RASPBERRY%20PI%20CAMERA%20V2%20DATASHEET%20IMX219PQH5_7.0.0_Datasheet_XXX.PDF [3] https://pdfcoffee.com/ov9281-datasheet-pdf-free.html section 2.8. > 2) LINK_FREQ is the closest control we have to express the output > pixel rate, but to me is very specific to the bus configuration and > does not express per se anything useful to userspace for computing > timings based on frame/lane sizes. The fact LINK_FREQ is a menu contol > reflects how much it relates to the HW configuration as it is assumed > to come from DT I'll agree - link frequency is IMHO near useless to userspace. It has a place for EMC compatibility, but I see that as coming from DT and the platform configuration, and not from userland. > Do we need an r/w PIXEL_OUTPUT_RATE control to replace > - LINK_FREQ for userspace to configure it > - PIXEL_RATE for userspace to read it No new control needed. Make PIXEL_RATE r/w, and make it modify the pixel array clock configuration. AIUI Drivers are allowed to validate controls, therefore presumably you can make it lock to discrete values instead of a full range. > LINK_FREQ should only be used in the tx/rx negotiation. It shall > vary according to PIXEL_OUTPUT_RATE, possibily in the options > specified in DTS (which are there because they have usually been > validated for RF emissions, that's my understanding at least). > > PIXEL_RATE will equally vary, if required, and algorithms that need to > know the sampling frequency in the analog domain will continue using > it. > > Or maybe the original idea was to have a pixel array entity and a > separate tx entity, each of them with different PIXEL_RATE control ? Pass over the original intent - I've not been involved in the V4L2 side of things long enough to know that. Dave > > > > Dave > > > > > > > ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops, > > > > > V4L2_CID_TEST_PATTERN, > > > > > ARRAY_SIZE(test_pattern_menu) - 1, > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart
Hi Laurent On Sun, 16 Oct 2022 at 02:53, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Dave, > > On Fri, Oct 07, 2022 at 03:26:55PM +0100, Dave Stevenson wrote: > > On Fri, 7 Oct 2022 at 15:01, Laurent Pinchart wrote: > > > On Thu, Oct 06, 2022 at 04:10:10PM +0100, Dave Stevenson wrote: > > > > On Wed, 5 Oct 2022 at 20:07, Jacopo Mondi wrote: > > > > > > > > > > Add support for V4L2_CID_LINK_FREQ which currently reports a single > > > > > hard-coded frequency which depends on the fixed pixel clock. > > > > > > > > > > This will change in the next patches where the pixel rate will be > > > > > computed from the desired link_frequency. > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > > Looks valid based on the current pixel rate of 184MPix/s, 8bpp, > > > > divided by 4 lanes, and DDR. > > > > > > > > > --- > > > > > drivers/media/i2c/ar0521.c | 9 +++++++++ > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > > > > > index 21649aecf442..c5410b091654 100644 > > > > > --- a/drivers/media/i2c/ar0521.c > > > > > +++ b/drivers/media/i2c/ar0521.c > > > > > @@ -90,6 +90,10 @@ static const char * const ar0521_supply_names[] = { > > > > > "vaa", /* Analog (2.7V) supply */ > > > > > }; > > > > > > > > > > +static const s64 ar0521_link_frequencies[] = { > > > > > + 184000000, > > > > > +}; > > > > > + > > > > > struct ar0521_ctrls { > > > > > struct v4l2_ctrl_handler handler; > > > > > struct v4l2_ctrl *ana_gain; > > > > > @@ -104,6 +108,7 @@ struct ar0521_ctrls { > > > > > }; > > > > > struct v4l2_ctrl *pixrate; > > > > > struct v4l2_ctrl *exposure; > > > > > + struct v4l2_ctrl *link_freq; > > > > > struct v4l2_ctrl *test_pattern; > > > > > }; > > > > > > > > > > @@ -655,6 +660,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor) > > > > > ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0, > > > > > 65535, 1, 360); > > > > > > > > > > + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, > > > > > + ARRAY_SIZE(ar0521_link_frequencies) - 1, > > > > > + 0, ar0521_link_frequencies); > > > > > + > > > > > > > > Admittedly there is only one entry, but did you want to make it a read > > > > only control? With no case for it in s_ctrl, you'll get errors thrown > > > > from the control handler framework. > > > > > > I'd make it writable even if there's a single entry, so that userspace > > > won't need special logic. It will also prepare for support of multiple > > > entries in the future. > > > > Do you really see a situation where userspace will be configuring link > > frequency instead of DT / ACPI? > > A quick search seems to imply that only 1 current driver supports a > > r/w link frequency - mt9v032. That would imply that having a > > controllable link frequency would require the special logic in > > userspace. > > Looking at the mainline kernel only, the N9, N950 and Librem5 all > specify multiple link frequencies, and so does the sdm845-db845c > development board. Sorry, insufficient time to go through all of those in detail. Looking at sdm845-db845c, yes the base DT lists multiple link frequencies for OV8856 [1], however the OV8856 driver then makes the LINK_FREQ control READ_ONLY[2]. So what is userspace meant to do with that information? IMX290 also requires 2 link frequencies to be listed, but the control is read only, and which is used is based on mode selection. (Hmm, I've just noticed that the DT binding example doesn't match the driver as it only lists one link-freq. OV8856 binding lists the restriction of the driver in the binding). N9 and N950 use "nokia,smia", which maps to CCS. Yes that driver does allow configuration of link frequency from userspace, but it still feels to me to be an odd control to allow userspace to control. [1] https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/qcom/sdm845-db845c.dts#L1240 [2] https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov8856.c#L1949 > This indeed requires specific logic in userspace, and to be honest, I > haven't really thought about how it would be implemented. Sakari has > more experience than me here, he may be able to shed some light. > > > I'm always very cautious about drivers that are linking PIXEL_RATE and > > LINK_FREQ - most of the sensors are tending to have 2 (or more) PLLs, > > and there is a FIFO between the image sensor (PIXEL_RATE) and the MIPI > > block (LINK_FREQ). imx290 is certainly wrong (pixel rate does not > > change with mode, but link freq does), and I'm fairly certain that > > ov7251 is as well (pixel rate is 48MPix/s whether at 240 or 319.2MHz > > link frequency). Patches coming soon for both. > > That's a good point, different link frequencies may or may not result in > different pixel rates. See my response to Jacopo. As you know I have a fair number of sensors around, and I see a fairly large number of the mainline drivers getting this wrong. I don't know whether that is down to copy/paste of drivers, or misunderstanding of the requirements, but it fouls up frame rate control and exposure calibration in almost every case. Dave > > > > ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops, > > > > > V4L2_CID_TEST_PATTERN, > > > > > ARRAY_SIZE(test_pattern_menu) - 1, > > -- > Regards, > > Laurent Pinchart
Hi Dave On Mon, Oct 17, 2022 at 12:00:29PM +0100, Dave Stevenson wrote: > Hi Jacopo > > On Mon, 17 Oct 2022 at 10:24, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Dave > > > > On Fri, Oct 07, 2022 at 03:26:55PM +0100, Dave Stevenson wrote: > > > On Fri, 7 Oct 2022 at 15:01, Laurent Pinchart > > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > > > On Thu, Oct 06, 2022 at 04:10:10PM +0100, Dave Stevenson wrote: > > > > > On Wed, 5 Oct 2022 at 20:07, Jacopo Mondi wrote: > > > > > > > > > > > > Add support for V4L2_CID_LINK_FREQ which currently reports a single > > > > > > hard-coded frequency which depends on the fixed pixel clock. > > > > > > > > > > > > This will change in the next patches where the pixel rate will be > > > > > > computed from the desired link_frequency. > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > > > > Looks valid based on the current pixel rate of 184MPix/s, 8bpp, > > > > > divided by 4 lanes, and DDR. > > > > > > > > > > > --- > > > > > > drivers/media/i2c/ar0521.c | 9 +++++++++ > > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > > > > > > index 21649aecf442..c5410b091654 100644 > > > > > > --- a/drivers/media/i2c/ar0521.c > > > > > > +++ b/drivers/media/i2c/ar0521.c > > > > > > @@ -90,6 +90,10 @@ static const char * const ar0521_supply_names[] = { > > > > > > "vaa", /* Analog (2.7V) supply */ > > > > > > }; > > > > > > > > > > > > +static const s64 ar0521_link_frequencies[] = { > > > > > > + 184000000, > > > > > > +}; > > > > > > + > > > > > > struct ar0521_ctrls { > > > > > > struct v4l2_ctrl_handler handler; > > > > > > struct v4l2_ctrl *ana_gain; > > > > > > @@ -104,6 +108,7 @@ struct ar0521_ctrls { > > > > > > }; > > > > > > struct v4l2_ctrl *pixrate; > > > > > > struct v4l2_ctrl *exposure; > > > > > > + struct v4l2_ctrl *link_freq; > > > > > > struct v4l2_ctrl *test_pattern; > > > > > > }; > > > > > > > > > > > > @@ -655,6 +660,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor) > > > > > > ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0, > > > > > > 65535, 1, 360); > > > > > > > > > > > > + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, > > > > > > + ARRAY_SIZE(ar0521_link_frequencies) - 1, > > > > > > + 0, ar0521_link_frequencies); > > > > > > + > > > > > > > > > > Admittedly there is only one entry, but did you want to make it a read > > > > > only control? With no case for it in s_ctrl, you'll get errors thrown > > > > > from the control handler framework. > > > > > > > > I'd make it writable even if there's a single entry, so that userspace > > > > won't need special logic. It will also prepare for support of multiple > > > > entries in the future. > > > > > > Do you really see a situation where userspace will be configuring link > > > frequency instead of DT / ACPI? > > > A quick search seems to imply that only 1 current driver supports a > > > r/w link frequency - mt9v032. That would imply that having a > > > controllable link frequency would require the special logic in > > > userspace. > > > > > > > Yes it does, but we need one way or another to allow userspace to > > control the sampling frequency as extending (or shrinking) blankings > > helps up to the point you reach their limits. > > > > I was never really fond of the idea that such action should go through > > link frequency, which seems very much a parameter of the bus that > > should be negotiated between the recv and the tx parts, rather than > > being user configurable. > > Indeed - that's PIXEL_RATE, not LINK_FREQ. > > > > I'm always very cautious about drivers that are linking PIXEL_RATE and > > > LINK_FREQ - most of the sensors are tending to have 2 (or more) PLLs, > > > and there is a FIFO between the image sensor (PIXEL_RATE) and the MIPI > > > block (LINK_FREQ). imx290 is certainly wrong (pixel rate does not > > > change with mode, but link freq does), and I'm fairly certain that > > > ov7251 is as well (pixel rate is 48MPix/s whether at 240 or 319.2MHz > > > link frequency). Patches coming soon for both. > > > > The current definition of PIXEL_RATE indeed describes the sampling > > frequency on the pixel array, which might or might not reflect the > > output pixel rate. However most if not all usages of PIXEL_RATE I've > > seen (and FTR the way it is used in libcamera) is to denote the output > > pixel rate (ie it is used to compute the output timings given the line > > length and frame height) > > > > I wonder > > > > 1) The current definition of PIXEL_RATE as the sampling rate on the > > pixel array: what purposes does it serve ? Are there algorithms that > > require to know the sampling rate in the analog domain ? Are there > > implementations that treat PIXEL_RATE differently than the "pixel > > output rate" ? > > Sampling rate on the array is the basis of using VBLANK and HBLANK to > control frame rate. See documentation at [1] My understanding, confirmed by your latest replies, is that the sampling rate on the array != output frame rate because of the different clocking trees, intermediate buffers etc, ... > > frame interval = (analogue crop width + horizontal blanking) * > (analogue crop height + vertical blanking) / pixel rate > ... hence it's not technically correct to uset he pixel rate to compute the frame timings on the receiver side in this way ... > This is NOT the pixel rate on the CSI link. ... but we would need something to represent the pixel output rate on the bus (and on the receiver) > > Taking an example of IMX219 [2], section 9.1 shows the clock structure > with 2 PLLs. PLL1 drives the pixel array (PIXEL_RATE). PLL2 drives the > MIPI block (LINK_FREQ). > There is a FIFO between the pixel array and MIPI, and therefore they > can run at different rates. > > OV5647 is the same. > IMX327/290/462 are the same, although FRSEL configures specific > dividers for the PIXEL_RATE. > OV9281 is the same (2 PLLs). > > In your case it does appear that LINK_FREQ and PIXEL_RATE are bound > together. From the developer guide: > "to reduce MIPI data rate, sensor pixel clock must be reduced as well" > Yes, AR0521 should be fine. Mine was a general digression on the two controls we use handling frame rate. > On AR0521 max frame interval is dictated by line_length_pck (0x0342) > and frame_length_lines (0x0340), both of which are 16bit values. > At your highest pixel rate of 414000000 I make that 10.37seconds per > frame, or 0.09fps. So without altering link frequency, my calculations > say you can do 0.09 to 60fps. Are you currently looking at use cases > that need frame rates outside these limits? If not, why are you > looking at changing the rate of anything? Not really, I just enabled changing the pixel output rate as that's something I was expecting to be "required". You're right in this case the total line lenght and total frame height can span up to 2^16-1 (according to the manual, I've not tested it thought) but that's not true for all sensors (the OV ones I interacted with usually have stricter limits) hence from an API point of view I think we should consider changing the pixel rate as a valid use case ? > > [1] https://www.kernel.org/doc/html/latest/driver-api/media/camera-sensor.html#raw-camera-sensors > [2] https://github.com/rellimmot/Sony-IMX219-Raspberry-Pi-V2-CMOS/blob/master/RASPBERRY%20PI%20CAMERA%20V2%20DATASHEET%20IMX219PQH5_7.0.0_Datasheet_XXX.PDF > [3] https://pdfcoffee.com/ov9281-datasheet-pdf-free.html section 2.8. > > > 2) LINK_FREQ is the closest control we have to express the output > > pixel rate, but to me is very specific to the bus configuration and > > does not express per se anything useful to userspace for computing > > timings based on frame/lane sizes. The fact LINK_FREQ is a menu contol > > reflects how much it relates to the HW configuration as it is assumed > > to come from DT > > I'll agree - link frequency is IMHO near useless to userspace. > It has a place for EMC compatibility, but I see that as coming from DT > and the platform configuration, and not from userland. > > > Do we need an r/w PIXEL_OUTPUT_RATE control to replace > > - LINK_FREQ for userspace to configure it > > - PIXEL_RATE for userspace to read it > > No new control needed. Make PIXEL_RATE r/w, and make it modify the PIXEL_RATE is ro by default, making it rw would require changing the definition of the control itself: drivers/media/v4l2-core/v4l2-ctrls-defs.c: case V4L2_CID_PIXEL_RATE: drivers/media/v4l2-core/v4l2-ctrls-defs.c- *type = V4L2_CTRL_TYPE_INTEGER64; drivers/media/v4l2-core/v4l2-ctrls-defs.c- *flags |= V4L2_CTRL_FLAG_READ_ONLY; Which makes me think using it as the output pixel rate means abusing its definition. > pixel array clock configuration. AIUI Drivers are allowed to validate > controls, therefore presumably you can make it lock to discrete values > instead of a full range. > Indeed the control value can be adjusted! > > LINK_FREQ should only be used in the tx/rx negotiation. It shall > > vary according to PIXEL_OUTPUT_RATE, possibily in the options > > specified in DTS (which are there because they have usually been > > validated for RF emissions, that's my understanding at least). > > > > PIXEL_RATE will equally vary, if required, and algorithms that need to > > know the sampling frequency in the analog domain will continue using > > it. > > > > Or maybe the original idea was to have a pixel array entity and a > > separate tx entity, each of them with different PIXEL_RATE control ? > > Pass over the original intent - I've not been involved in the V4L2 > side of things long enough to know that. > > Dave > > > > > > > Dave > > > > > > > > > ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops, > > > > > > V4L2_CID_TEST_PATTERN, > > > > > > ARRAY_SIZE(test_pattern_menu) - 1, > > > > > > > > -- > > > > Regards, > > > > > > > > Laurent Pinchart
diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c index 21649aecf442..c5410b091654 100644 --- a/drivers/media/i2c/ar0521.c +++ b/drivers/media/i2c/ar0521.c @@ -90,6 +90,10 @@ static const char * const ar0521_supply_names[] = { "vaa", /* Analog (2.7V) supply */ }; +static const s64 ar0521_link_frequencies[] = { + 184000000, +}; + struct ar0521_ctrls { struct v4l2_ctrl_handler handler; struct v4l2_ctrl *ana_gain; @@ -104,6 +108,7 @@ struct ar0521_ctrls { }; struct v4l2_ctrl *pixrate; struct v4l2_ctrl *exposure; + struct v4l2_ctrl *link_freq; struct v4l2_ctrl *test_pattern; }; @@ -655,6 +660,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor) ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0, 65535, 1, 360); + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, + ARRAY_SIZE(ar0521_link_frequencies) - 1, + 0, ar0521_link_frequencies); + ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN, ARRAY_SIZE(test_pattern_menu) - 1,
Add support for V4L2_CID_LINK_FREQ which currently reports a single hard-coded frequency which depends on the fixed pixel clock. This will change in the next patches where the pixel rate will be computed from the desired link_frequency. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- drivers/media/i2c/ar0521.c | 9 +++++++++ 1 file changed, 9 insertions(+)