[RFC] Repurpose V4L2_CID_PIXEL_RATE for the sampling rate in the pixel array

[RFC] Repurpose V4L2_CID_PIXEL_RATE for the sampling rate in the pixel array

[Andrey Konovalov]

Greetings,

I was checking how V4L2_CID_PIXEL_RATE and V4L2_CID_LINK_FREQ controls are documented and used in recent
media_tree kernel, and found that the current usage of V4L2_CID_PIXEL_RATE is not consistent.

This topic seems to be a bit wider, but I'll focus on camera sensors and the corresponding receivers.

Not only the receiver drivers often use V4L2_CID_PIXEL_RATE to calculate the link frequency from (this is
much more common than using V4L2_CID_LINK_FREQ alone, or with fallback to V4L2_CID_PIXEL_RATE if
V4L2_CID_LINK_FREQ is not implemented).
The bigger issue is that V4L2_CID_PIXEL_RATE was intended to give the rate at which pixels are transmitted
(e.g. over CSI-2 link), while it is also seems to be used for the sampling rate in the pixel array.
The problem is that these two rates are different things, and may have different values.

The proposal is to repurpose V4L2_CID_PIXEL_RATE to report the sampling rate in the pixel array, and to
calculate the symbol rate at which pixels are transmitted over the link from V4L2_CID_LINK_FREQ value.
The receiver drivers currently using V4L2_CID_PIXEL_RATE to get the link frequency can be switched to use
the existing v4l2_get_link_rate() helper. We also need to push the sensor drivers to implement
V4L2_CID_LINK_FREQ, as quite a few of them do V4L2_CID_PIXEL_RATE only.

I am not sure about the userspace cases, but guess that the userspace is much more interested in frames
per second and exposure values than in link details. So it would rather use V4L2_CID_PIXEL_RATE for the
sampling rate in the pixel array anyway.

More details below.

1. V4L2_CID_PIXEL_RATE vs V4L2_CID_LINK_FREQ.
---------------------------------------------

According to [1]:
- V4L2_CID_LINK_FREQ control can be used to get or to set the frequency of the bus between transmitter
   (e.g. sensor) and receiver (e.g. RX phy on an SOC), This control is r/w by default, but is often made
   read-only by the sensor drivers,
- V4L2_CID_PIXEL_RATE control can be used to get pixel rate in the source pads of the subdev. This control
   is read-only by default.

[2] is even more explicit:
- V4L2_CID_LINK_FREQ control tells the receiver driver the frequency (and not the symbol rate) of the link,
- V4L2_CID_PIXEL_RATE control may be used by the receiver to obtain the pixel rate the transmitter uses.
   If the link is the MIPI CSI-2 D-PHY one, the value of the V4L2_CID_PIXEL_RATE
   is calculated as follows:
     pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample		(*)

[2] also requires the sensor driver connected through CSI-2 or parallel (BT.601 or BT.656) bus to implement
V4L2_CID_LINK_FREQ control, and for raw sensors V4L2_CID_PIXEL_RATE is also said to be mandatory.
Still only half of sensor drivers under driver/media/i2c in the current media tree have V4L2_CID_PIXEL_RATE
support and even less implement V4L2_CID_LINK_FREQ control.

(*) is valid for MIPI CSI-2 D-PHY only, but it is widely used by the receiver drivers to get CSI-2 link
frequency from V4L2_CID_PIXEL_RATE [3], [4], [5], [6].
For other bus types link frequency can also be calculated from the pixel rate and some other parameters.
There exists a helper function in drivers/media/v4l2-core/v4l2-common.c for that already [7]:
   s64 v4l2_get_link_rate(struct v4l2_ctrl_handler *handler, unsigned int mul, unsigned int div)
which tries to get the link rate value using V4L2_CID_LINK_FREQ, and if it not supported by the sub-device,
it tries V4L2_CID_PIXEL_RATE and calculates the link frequency from the pixel rate using the multiplier and
the divider provided by the caller.
Unfortunately only one driver [8] uses v4l2_get_link_rate(). [9] implements a similar approach (error
handling is different) but on its own.

[10] and [11] use V4L2_CID_LINK_FREQ directly.

Only two drivers use V4L2_CID_PIXEL_RATE for something else than getting the link frequency [12], [13].

So in most of the cases the receiver drivers need to know the link frequency.
Often this link frequency is calculated from the pixel rate, but changing the drivers to use
V4L2_CID_LINK_FREQ directly looks fairly easy.


2. V4L2_CID_PIXEL_RATE: the rate at which pixels are transmitted, or the sampling rate in the pixel array?
----------------------------------------------------------------------------------------------------------

Let's start from the example: "imx219 8-bit raw modes are broken for camss, the 8MP@15fps one included".

The imx219 driver supports only 2 data lanes.
The link frequency is fixed at 456MHz.
At 8MP the fps is 15, which the driver refers to as 8MP@15fps mode.

Originally the driver supported 10-bit raw format only, so the pixel rate was:

   IMX219_pixel_rate = IMX219_link_freq * 2 * nr_of_lanes / bits_per_sample =
                       456 * 2 * 2 / 10 = 182.4 Mpix/sec

The line length (IMX219_PPL_DEFAULT) is set to 3448 (h blanking included), the frame length in this
mode (IMX219_VTS_15FPS) is 3526 (v blanking included). Then

   IMX219_frame_rate_8MP = IMX219_pixel_rate / IMX219_PPL_DEFAULT / IMX219_VTS_15FPS =
                           182400000 / 3448 / 3526 = 15.003 Hz
- OK, as expected.

The value for read-only V4L2_CID_HBLANK control is calculated as:
         hblank = IMX219_PPL_DEFAULT - mode->width;
- and can only change as the result of switching the modes.

Then 8-bit raw format was added. And it was done such that switching between 10 and 8 bits affects the
output data domain only - the pixel read domain continues to work just as before:
   - the number of bits per sample to read from the FIFO is set to 8 or 10
     (there is a FIFO between the "pixel read" and the "MIPI data output" halves of the sensor)
   - the divider for the "read from the FIFO" clock is set to 8 or 10 - this clock is 91.2 MHz for 10 bits
     and 114 MHz for 8 bits. So that the link frequency remains the same.

The important thing to note is that the clock at which the samples are written
to the FIFO remains at 91.2 MHz for both 10 and 8 bits.

V4L2_CID_PIXEL_RATE control handling was not changed - its value is always reported as 182.4 Mpix/sec.
This immediately broke the receivers which use V4L2_CID_PIXEL_RATE to configure their phy -
e.g. capturing in 8 bit modes doesn't work on Qualcomm boards using camss driver.

So my first impression was that this is a bug in the imx219 driver. But in fact this is a more generic
API problem: if we update V4L2_CID_PIXEL_RATE to fix the issue with the drivers calculating the link
frequency from it, from the equation below [14]:

   frame interval = (analogue crop width + horizontal blanking) *
                  (analogue crop height + vertical blanking) / pixel_rate	(**)

- we would see (if we treat the h blanking as it is treated in imx219 driver) that the fps
changed from 15 to 18.75 after switching from 10-bit to 8-bit raw format.
While in fact the fps is 15 in the both cases. If we changed camss to use V4L2_CID_LINK_FREQ (not
changed between 8 and 10 bits in this case), and repurposed V4L2_CID_PIXEL_RATE to report the sampling
rate in the pixel array (not changed between 8 and 10 bits in this case), then (**) would give
the correct frame interval and 8-bit modes would work with camss driver just fine.

As a summary, this issue happened because the equation (*) assumes that pixel_rate is the rate at
which pixels are transmitted (belongs to the "MIPI data output" domain), while the imx219 driver
and equation (**) assume that the pixel_rate is the sampling rate in the pixel array. And while
in case of the imx219 driver these two (different by nature) rates are the same for 10-bit raw
formats, for 8-bit raw formats the sampling rate in the pixel array is lower than the rate at
which pixels are transmitted. Because switching from 10 to 8 bits reconfigures the "MIPI data output"
domain *only*. And this implementation does make sense: this is quite natural for a user to see
no change in fps and exposure (in absolute units, not in lines) when he/she decides to use more
or less bits per pixel.

(As a side note - different clocks for reading the pixels from the image array and for sending them
out to MIPI transmitter is a documented feature of IMX219 sensor.)

If we return to (**) again, as I see it, this equation makes sense in the "pixel sampling" domain only.
And this is how it seems to be used in the camera sensor drivers. Also [15] does say "The unit of horizontal
blanking is pixels" about V4L2_CID_HBLANK - this is way easier to work with in the "pixel sampling" domain.
Even though h/v blanking is rather the notion of "MIPI data output" domain imho. So if we look at what
is happening on the CSI link when imx219 driver switches from 10-bit row to 8-bit row, we will see the
same link frequency, the increased symbol rate and hence the shorter time between packet header and packet
footer followed by increased line blanking interval for the lines with pixel data. While V4L2_CID_PIXEL_RATE
and V4L2_CID_HBLANK would report no changes at all (as current imx219 driver uses them as though they both
belong to the "pixel sampling" domain).

And again, if the rate at which pixels are transmitted and the sampling rate in the pixel array are the same,
the "pixel sampling" domain and the "data out" domain are no difference.


Thanks,
Andrey


[1] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/ext-ctrls-image-process.html
[2] https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/csi2.html?#transmitter-drivers
[3] drivers/media/platform/ti-vpe/cal-camerarx.c
[4] drivers/staging/media/omap4iss/
[5] drivers/media/platform/rcar-vin/rcar-csi2.c
[6] drivers/media/platform/qcom/camss/camss-csiphy.c
[7] https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-common.html#c.v4l2_get_link_rate
[8] drivers/media/pci/intel/ipu3/ipu3-cio2.c
[9] drivers/media/i2c/st-mipid02.c
[10] drivers/staging/media/imx/imx6-mipi-csi2.c
[11] drivers/staging/media/atomisp/pci/
[12] drivers/media/platform/omap3isp/ispvideo.c
[13] drivers/staging/media/tegra-video/csi.c
[14] https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/camera-sensor.html?#raw-camera-sensors
[15] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/ext-ctrls-image-source.html

Re: [RFC] Repurpose V4L2_CID_PIXEL_RATE for the sampling rate in the pixel array

[Sakari Ailus]

Hi Andrey,

Thanks for your e-mail.

Could you wrap the lines before 80 characters? It's a bit hard to read...

On Tue, Dec 08, 2020 at 03:09:25PM +0300, Andrey Konovalov wrote:
> Greetings,
> 
> I was checking how V4L2_CID_PIXEL_RATE and V4L2_CID_LINK_FREQ controls
> are documented and used in recent media_tree kernel, and found that the
> current usage of V4L2_CID_PIXEL_RATE is not consistent.
> 
> This topic seems to be a bit wider, but I'll focus on camera sensors and
> the corresponding receivers.
> 
> Not only the receiver drivers often use V4L2_CID_PIXEL_RATE to calculate
> the link frequency from (this is much more common than using
> V4L2_CID_LINK_FREQ alone, or with fallback to V4L2_CID_PIXEL_RATE if
> V4L2_CID_LINK_FREQ is not implemented). The bigger issue is that
> V4L2_CID_PIXEL_RATE was intended to give the rate at which pixels are
> transmitted (e.g. over CSI-2 link), while it is also seems to be used for
> the sampling rate in the pixel array. The problem is that these two rates
> are different things, and may have different values.

The PIXEL_RATE control indeed has signified, well, the pixel rate, and
pixel rate may be different things depending on where it is found.

The original use has been both on pixel matrix as well as on the
CSI-2 transmitter but of course this is a problem if the two are the same
sub-device.

> 
> The proposal is to repurpose V4L2_CID_PIXEL_RATE to report the sampling
> rate in the pixel array, and to calculate the symbol rate at which pixels
> are transmitted over the link from V4L2_CID_LINK_FREQ value. The receiver
> drivers currently using V4L2_CID_PIXEL_RATE to get the link frequency can
> be switched to use the existing v4l2_get_link_rate() helper. We also need
> to push the sensor drivers to implement V4L2_CID_LINK_FREQ, as quite a
> few of them do V4L2_CID_PIXEL_RATE only.

Indeed. That has been the idea for some time actually, but first all the
receiver drivers need to be converted to use LINK_FREQ. This is easiest
done by using the wrapper the ipu3-cio2 driver already uses as it still
works even if the sensor driver only implemented PIXEL_RATE.

> 
> I am not sure about the userspace cases, but guess that the userspace is
> much more interested in frames per second and exposure values than in
> link details. So it would rather use V4L2_CID_PIXEL_RATE for the sampling
> rate in the pixel array anyway.

That's the case if the sensor driver exposes the rest of the configuration
as the device's native parameters (such as analogue crop rectangle and
horizontal and vertical blanking). The majority of drivers do not do that,
at least currently, but instead use SUBDEV_[GS]_FRAME_INTERVAL. This is not
a native interface for camera sensors, and it is not well suited for
conveying what other frame intervals are available.

> 
> More details below.
> 
> 1. V4L2_CID_PIXEL_RATE vs V4L2_CID_LINK_FREQ.
> ---------------------------------------------
> 
> According to [1]: - V4L2_CID_LINK_FREQ control can be used to get or to
> set the frequency of the bus between transmitter
>   (e.g. sensor) and receiver (e.g. RX phy on an SOC), This control is r/w
>   by default, but is often made read-only by the sensor drivers,
> - V4L2_CID_PIXEL_RATE control can be used to get pixel rate in the source pads of the subdev. This control
>   is read-only by default.
> 
> [2] is even more explicit: - V4L2_CID_LINK_FREQ control tells the
> receiver driver the frequency (and not the symbol rate) of the link, -
> V4L2_CID_PIXEL_RATE control may be used by the receiver to obtain the
> pixel rate the transmitter uses.
>   If the link is the MIPI CSI-2 D-PHY one, the value of the
>   V4L2_CID_PIXEL_RATE is calculated as follows:
>     pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample
> (*)
> 
> [2] also requires the sensor driver connected through CSI-2 or parallel
> (BT.601 or BT.656) bus to implement V4L2_CID_LINK_FREQ control, and for
> raw sensors V4L2_CID_PIXEL_RATE is also said to be mandatory. Still only
> half of sensor drivers under driver/media/i2c in the current media tree
> have V4L2_CID_PIXEL_RATE support and even less implement
> V4L2_CID_LINK_FREQ control.
> 
> (*) is valid for MIPI CSI-2 D-PHY only, but it is widely used by the

Correct. The only driver just got support for CSI-2 C-PHY, so the
documentation needs to be adjusted, too. I actually recently sent a patch
to address that.

> receiver drivers to get CSI-2 link frequency from V4L2_CID_PIXEL_RATE
> [3], [4], [5], [6]. For other bus types link frequency can also be
> calculated from the pixel rate and some other parameters. There exists a
> helper function in drivers/media/v4l2-core/v4l2-common.c for that already
> [7]:
>   s64 v4l2_get_link_rate(struct v4l2_ctrl_handler *handler, unsigned int
> mul, unsigned int div) which tries to get the link rate value using
> V4L2_CID_LINK_FREQ, and if it not supported by the sub-device, it tries
> V4L2_CID_PIXEL_RATE and calculates the link frequency from the pixel rate
> using the multiplier and the divider provided by the caller.
> Unfortunately only one driver [8] uses v4l2_get_link_rate(). [9]
> implements a similar approach (error handling is different) but on its
> own.
> 
> [10] and [11] use V4L2_CID_LINK_FREQ directly.
> 
> Only two drivers use V4L2_CID_PIXEL_RATE for something else than getting
> the link frequency [12], [13].
> 
> So in most of the cases the receiver drivers need to know the link
> frequency. Often this link frequency is calculated from the pixel rate,
> but changing the drivers to use V4L2_CID_LINK_FREQ directly looks fairly
> easy.
> 
> 
> 2. V4L2_CID_PIXEL_RATE: the rate at which pixels are transmitted, or the
> sampling rate in the pixel array?
> ----------------------------------------------------------------------------------------------------------
> 
> Let's start from the example: "imx219 8-bit raw modes are broken for
> camss, the 8MP@15fps one included".
> 
> The imx219 driver supports only 2 data lanes. The link frequency is fixed
> at 456MHz. At 8MP the fps is 15, which the driver refers to as 8MP@15fps
> mode.
> 
> Originally the driver supported 10-bit raw format only, so the pixel rate
> was:
> 
>   IMX219_pixel_rate = IMX219_link_freq * 2 * nr_of_lanes /
>                       bits_per_sample = 456 * 2 * 2 / 10 = 182.4 Mpix/sec
> 
> The line length (IMX219_PPL_DEFAULT) is set to 3448 (h blanking
> included), the frame length in this mode (IMX219_VTS_15FPS) is 3526 (v
> blanking included). Then
> 
>   IMX219_frame_rate_8MP = IMX219_pixel_rate / IMX219_PPL_DEFAULT /
>                           IMX219_VTS_15FPS = 182400000 / 3448 / 3526 =
>                           15.003 Hz - OK, as
> expected.
> 
> The value for read-only V4L2_CID_HBLANK control is calculated as: hblank
>         = IMX219_PPL_DEFAULT - mode->width; - and can only change
> as the result of switching the modes.
> 
> Then 8-bit raw format was added. And it was done such that switching
> between 10 and 8 bits affects the output data domain only - the pixel
> read domain continues to work just as before:
>   - the number of bits per sample to read from the FIFO is set to 8 or 10
>     (there is a FIFO between the "pixel read" and the "MIPI data output"
>   halves of the sensor) - the divider for the "read from the FIFO" clock
>   is set to 8 or 10 - this clock is 91.2 MHz for 10 bits
>     and 114 MHz for 8 bits. So that the link frequency remains the same.
> 
> The important thing to note is that the clock at which the samples are
> written to the FIFO remains at 91.2 MHz for both 10 and 8 bits.
> 
> V4L2_CID_PIXEL_RATE control handling was not changed - its value is
> always reported as 182.4 Mpix/sec. This immediately broke the receivers
> which use V4L2_CID_PIXEL_RATE to configure their phy - e.g. capturing in
> 8 bit modes doesn't work on Qualcomm boards using camss driver.
> 
> So my first impression was that this is a bug in the imx219 driver. But
> in fact this is a more generic API problem: if we update
> V4L2_CID_PIXEL_RATE to fix the issue with the drivers calculating the
> link frequency from it, from the equation below [14]:
> 
>   frame interval = (analogue crop width + horizontal blanking) *
>                  (analogue crop height + vertical blanking) / pixel_rate
> (**)
> 
> - we would see (if we treat the h blanking as it is treated in imx219
> driver) that the fps changed from 15 to 18.75 after switching from 10-bit
> to 8-bit raw format. While in fact the fps is 15 in the both cases. If we
> changed camss to use V4L2_CID_LINK_FREQ (not changed between 8 and 10
> bits in this case), and repurposed V4L2_CID_PIXEL_RATE to report the
> sampling rate in the pixel array (not changed between 8 and 10 bits in
> this case), then (**) would give the correct frame interval and 8-bit
> modes would work with camss driver just fine.
> 
> As a summary, this issue happened because the equation (*) assumes that
> pixel_rate is the rate at which pixels are transmitted (belongs to the
> "MIPI data output" domain), while the imx219 driver and equation (**)
> assume that the pixel_rate is the sampling rate in the pixel array. And
> while in case of the imx219 driver these two (different by nature) rates
> are the same for 10-bit raw formats, for 8-bit raw formats the sampling
> rate in the pixel array is lower than the rate at which pixels are
> transmitted. Because switching from 10 to 8 bits reconfigures the "MIPI
> data output" domain *only*. And this implementation does make sense: this
> is quite natural for a user to see no change in fps and exposure (in
> absolute units, not in lines) when he/she decides to use more or less
> bits per pixel.
> 
> (As a side note - different clocks for reading the pixels from the image
> array and for sending them out to MIPI transmitter is a documented
> feature of IMX219 sensor.)
> 
> If we return to (**) again, as I see it, this equation makes sense in the
> "pixel sampling" domain only. And this is how it seems to be used in the
> camera sensor drivers. Also [15] does say "The unit of horizontal
> blanking is pixels" about V4L2_CID_HBLANK - this is way easier to work
> with in the "pixel sampling" domain. Even though h/v blanking is rather
> the notion of "MIPI data output" domain imho. So if we look at what is
> happening on the CSI link when imx219 driver switches from 10-bit row to
> 8-bit row, we will see the same link frequency, the increased symbol rate
> and hence the shorter time between packet header and packet footer
> followed by increased line blanking interval for the lines with pixel
> data. While V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK would report no
> changes at all (as current imx219 driver uses them as though they both
> belong to the "pixel sampling" domain).
> 
> And again, if the rate at which pixels are transmitted and the sampling
> rate in the pixel array are the same, the "pixel sampling" domain and the
> "data out" domain are no difference.

Over the past 15 years, a lot of different camera sensor drivers have been
merged. The capabilities and interfaces exposed by the drivers have changed
a little over time and also sensor's capabilities and the driver writer's
use cases have had an effect on the interfaces the drivers implement. The
APIs have remained the same, while at the same time they haven't been
exactly a perfect match for the capabilities of the register list based
drivers.

So with that background, it's perhaps not surprising that in some areas
there's variation in how the subdev API is being used by the drivers.

If there's a desire to improve this, it means changing the user space
interface of a number of drivers. I don't know how they are being used
currently. Removing a control from a driver or changing its semantics does
have some potential for breaking user space.

If changes in the interface end up being done, they should be compatible
the goal of providing all the necessary information on sensor configuration
to the user space, not just the pixel rate in the pixel array. This
includes analogue crop rectangle, binning (and subsampling), scaling and
digital crop configurations. Selecting the actual configuration would still
be based on the source pad of the sub-device representing the sensor's
transmitter. This means often implementing more sub-devices, in many cases
the same way the CCS driver does already.

I'm also not sure it's worth changing drivers that already use
SUBDEV_[GS]_FRAME_INTERVAL; they're so far from the target and are most
likely for older hardware. The more obvious target are drivers that already
expose the blanking controls.

This is what I could think of to remedy the problems:

1. Make all receiver drivers use LINK_FREQ instead of PIXEL_RATE.

2. Remove PIXEL_RATE from sensor drivers in places where it is used to
convey the pixel rate on the data bus. Steps from this onward may be
implemented one driver at a time.

3. Add back the pixel rate control that represents the pixel rate in the
pixel array. The blanking controls need to be aligned with the analogue
crop size.

4. Implement more sub-devices to convey the sensor's current configuration
to the user, including analogue crop, binning, scaling and digital crop.

There's a chance of breaking something in the user space from 2. onwards.

If you skip the 4. step, the frame rate configuration could work but the
rest of the sensor configuration (cropping, binning, scaling etc.) would
remain unknown to the user space.

The above does not address selection of modes in register list based
drivers. It would be nice to have a solution for that, too, but the general
issue is that it does not fit to the model V4L2 has --- considering all the
configurations independent. They are that on hardware level but register
list based drivers do not configure them like that.

Any thoughts, opinions, comments etc. on this are very welcome.

Cc also Hans and Mauro.

-- 
Kind regards,

Sakari Ailus

Re: [RFC] Repurpose V4L2_CID_PIXEL_RATE for the sampling rate in the pixel array

[Andrey Konovalov]

Hi Sakari,

Thank you for your feedback!

On 21.12.2020 14:45, Sakari Ailus wrote:
> Hi Andrey,
> 
> Thanks for your e-mail.
> 
> Could you wrap the lines before 80 characters? It's a bit hard to read...
> 
> On Tue, Dec 08, 2020 at 03:09:25PM +0300, Andrey Konovalov wrote:
>> Greetings,
>>
>> I was checking how V4L2_CID_PIXEL_RATE and V4L2_CID_LINK_FREQ controls
>> are documented and used in recent media_tree kernel, and found that the
>> current usage of V4L2_CID_PIXEL_RATE is not consistent.
>>
>> This topic seems to be a bit wider, but I'll focus on camera sensors and
>> the corresponding receivers.
>>
>> Not only the receiver drivers often use V4L2_CID_PIXEL_RATE to calculate
>> the link frequency from (this is much more common than using
>> V4L2_CID_LINK_FREQ alone, or with fallback to V4L2_CID_PIXEL_RATE if
>> V4L2_CID_LINK_FREQ is not implemented). The bigger issue is that
>> V4L2_CID_PIXEL_RATE was intended to give the rate at which pixels are
>> transmitted (e.g. over CSI-2 link), while it is also seems to be used for
>> the sampling rate in the pixel array. The problem is that these two rates
>> are different things, and may have different values.
> 
> The PIXEL_RATE control indeed has signified, well, the pixel rate, and
> pixel rate may be different things depending on where it is found.
> 
> The original use has been both on pixel matrix as well as on the
> CSI-2 transmitter but of course this is a problem if the two are the same
> sub-device.

OK, I see.
I am very used to the "two are the same sub-device" case, but this is not
the only option indeed.

>> The proposal is to repurpose V4L2_CID_PIXEL_RATE to report the sampling
>> rate in the pixel array, and to calculate the symbol rate at which pixels
>> are transmitted over the link from V4L2_CID_LINK_FREQ value. The receiver
>> drivers currently using V4L2_CID_PIXEL_RATE to get the link frequency can
>> be switched to use the existing v4l2_get_link_rate() helper. We also need
>> to push the sensor drivers to implement V4L2_CID_LINK_FREQ, as quite a
>> few of them do V4L2_CID_PIXEL_RATE only.
> 
> Indeed. That has been the idea for some time actually, but first all the
> receiver drivers need to be converted to use LINK_FREQ. This is easiest
> done by using the wrapper the ipu3-cio2 driver already uses as it still
> works even if the sensor driver only implemented PIXEL_RATE.

Yes, v4l2_get_link_rate().

>> I am not sure about the userspace cases, but guess that the userspace is
>> much more interested in frames per second and exposure values than in
>> link details. So it would rather use V4L2_CID_PIXEL_RATE for the sampling
>> rate in the pixel array anyway.
> 
> That's the case if the sensor driver exposes the rest of the configuration
> as the device's native parameters (such as analogue crop rectangle and
> horizontal and vertical blanking).

These (analogue crop rectangle and horizontal and vertical blanking) are
in my plans too. (For some of the sensors I have access to and the datasheets
for.)

> The majority of drivers do not do that,
> at least currently, but instead use SUBDEV_[GS]_FRAME_INTERVAL. This is not
> a native interface for camera sensors, and it is not well suited for
> conveying what other frame intervals are available.
> 
>>
>> More details below.
>>
>> 1. V4L2_CID_PIXEL_RATE vs V4L2_CID_LINK_FREQ.
>> ---------------------------------------------
>>
>> According to [1]: - V4L2_CID_LINK_FREQ control can be used to get or to
>> set the frequency of the bus between transmitter
>>    (e.g. sensor) and receiver (e.g. RX phy on an SOC), This control is r/w
>>    by default, but is often made read-only by the sensor drivers,
>> - V4L2_CID_PIXEL_RATE control can be used to get pixel rate in the source pads of the subdev. This control
>>    is read-only by default.
>>
>> [2] is even more explicit: - V4L2_CID_LINK_FREQ control tells the
>> receiver driver the frequency (and not the symbol rate) of the link, -
>> V4L2_CID_PIXEL_RATE control may be used by the receiver to obtain the
>> pixel rate the transmitter uses.
>>    If the link is the MIPI CSI-2 D-PHY one, the value of the
>>    V4L2_CID_PIXEL_RATE is calculated as follows:
>>      pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample
>> (*)
>>
>> [2] also requires the sensor driver connected through CSI-2 or parallel
>> (BT.601 or BT.656) bus to implement V4L2_CID_LINK_FREQ control, and for
>> raw sensors V4L2_CID_PIXEL_RATE is also said to be mandatory. Still only
>> half of sensor drivers under driver/media/i2c in the current media tree
>> have V4L2_CID_PIXEL_RATE support and even less implement
>> V4L2_CID_LINK_FREQ control.
>>
>> (*) is valid for MIPI CSI-2 D-PHY only, but it is widely used by the
> 
> Correct. The only driver just got support for CSI-2 C-PHY, so the
> documentation needs to be adjusted, too. I actually recently sent a patch
> to address that.

Nice, thanks!

>> receiver drivers to get CSI-2 link frequency from V4L2_CID_PIXEL_RATE
>> [3], [4], [5], [6]. For other bus types link frequency can also be
>> calculated from the pixel rate and some other parameters. There exists a
>> helper function in drivers/media/v4l2-core/v4l2-common.c for that already
>> [7]:
>>    s64 v4l2_get_link_rate(struct v4l2_ctrl_handler *handler, unsigned int
>> mul, unsigned int div) which tries to get the link rate value using
>> V4L2_CID_LINK_FREQ, and if it not supported by the sub-device, it tries
>> V4L2_CID_PIXEL_RATE and calculates the link frequency from the pixel rate
>> using the multiplier and the divider provided by the caller.
>> Unfortunately only one driver [8] uses v4l2_get_link_rate(). [9]
>> implements a similar approach (error handling is different) but on its
>> own.
>>
>> [10] and [11] use V4L2_CID_LINK_FREQ directly.
>>
>> Only two drivers use V4L2_CID_PIXEL_RATE for something else than getting
>> the link frequency [12], [13].
>>
>> So in most of the cases the receiver drivers need to know the link
>> frequency. Often this link frequency is calculated from the pixel rate,
>> but changing the drivers to use V4L2_CID_LINK_FREQ directly looks fairly
>> easy.
>>
>>
>> 2. V4L2_CID_PIXEL_RATE: the rate at which pixels are transmitted, or the
>> sampling rate in the pixel array?
>> ----------------------------------------------------------------------------------------------------------
>>
>> Let's start from the example: "imx219 8-bit raw modes are broken for
>> camss, the 8MP@15fps one included".
>>
>> The imx219 driver supports only 2 data lanes. The link frequency is fixed
>> at 456MHz. At 8MP the fps is 15, which the driver refers to as 8MP@15fps
>> mode.
>>
>> Originally the driver supported 10-bit raw format only, so the pixel rate
>> was:
>>
>>    IMX219_pixel_rate = IMX219_link_freq * 2 * nr_of_lanes /
>>                        bits_per_sample = 456 * 2 * 2 / 10 = 182.4 Mpix/sec
>>
>> The line length (IMX219_PPL_DEFAULT) is set to 3448 (h blanking
>> included), the frame length in this mode (IMX219_VTS_15FPS) is 3526 (v
>> blanking included). Then
>>
>>    IMX219_frame_rate_8MP = IMX219_pixel_rate / IMX219_PPL_DEFAULT /
>>                            IMX219_VTS_15FPS = 182400000 / 3448 / 3526 =
>>                            15.003 Hz - OK, as
>> expected.
>>
>> The value for read-only V4L2_CID_HBLANK control is calculated as: hblank
>>          = IMX219_PPL_DEFAULT - mode->width; - and can only change
>> as the result of switching the modes.
>>
>> Then 8-bit raw format was added. And it was done such that switching
>> between 10 and 8 bits affects the output data domain only - the pixel
>> read domain continues to work just as before:
>>    - the number of bits per sample to read from the FIFO is set to 8 or 10
>>      (there is a FIFO between the "pixel read" and the "MIPI data output"
>>    halves of the sensor) - the divider for the "read from the FIFO" clock
>>    is set to 8 or 10 - this clock is 91.2 MHz for 10 bits
>>      and 114 MHz for 8 bits. So that the link frequency remains the same.
>>
>> The important thing to note is that the clock at which the samples are
>> written to the FIFO remains at 91.2 MHz for both 10 and 8 bits.
>>
>> V4L2_CID_PIXEL_RATE control handling was not changed - its value is
>> always reported as 182.4 Mpix/sec. This immediately broke the receivers
>> which use V4L2_CID_PIXEL_RATE to configure their phy - e.g. capturing in
>> 8 bit modes doesn't work on Qualcomm boards using camss driver.
>>
>> So my first impression was that this is a bug in the imx219 driver. But
>> in fact this is a more generic API problem: if we update
>> V4L2_CID_PIXEL_RATE to fix the issue with the drivers calculating the
>> link frequency from it, from the equation below [14]:
>>
>>    frame interval = (analogue crop width + horizontal blanking) *
>>                   (analogue crop height + vertical blanking) / pixel_rate
>> (**)
>>
>> - we would see (if we treat the h blanking as it is treated in imx219
>> driver) that the fps changed from 15 to 18.75 after switching from 10-bit
>> to 8-bit raw format. While in fact the fps is 15 in the both cases. If we
>> changed camss to use V4L2_CID_LINK_FREQ (not changed between 8 and 10
>> bits in this case), and repurposed V4L2_CID_PIXEL_RATE to report the
>> sampling rate in the pixel array (not changed between 8 and 10 bits in
>> this case), then (**) would give the correct frame interval and 8-bit
>> modes would work with camss driver just fine.
>>
>> As a summary, this issue happened because the equation (*) assumes that
>> pixel_rate is the rate at which pixels are transmitted (belongs to the
>> "MIPI data output" domain), while the imx219 driver and equation (**)
>> assume that the pixel_rate is the sampling rate in the pixel array. And
>> while in case of the imx219 driver these two (different by nature) rates
>> are the same for 10-bit raw formats, for 8-bit raw formats the sampling
>> rate in the pixel array is lower than the rate at which pixels are
>> transmitted. Because switching from 10 to 8 bits reconfigures the "MIPI
>> data output" domain *only*. And this implementation does make sense: this
>> is quite natural for a user to see no change in fps and exposure (in
>> absolute units, not in lines) when he/she decides to use more or less
>> bits per pixel.
>>
>> (As a side note - different clocks for reading the pixels from the image
>> array and for sending them out to MIPI transmitter is a documented
>> feature of IMX219 sensor.)
>>
>> If we return to (**) again, as I see it, this equation makes sense in the
>> "pixel sampling" domain only. And this is how it seems to be used in the
>> camera sensor drivers. Also [15] does say "The unit of horizontal
>> blanking is pixels" about V4L2_CID_HBLANK - this is way easier to work
>> with in the "pixel sampling" domain. Even though h/v blanking is rather
>> the notion of "MIPI data output" domain imho. So if we look at what is
>> happening on the CSI link when imx219 driver switches from 10-bit row to
>> 8-bit row, we will see the same link frequency, the increased symbol rate
>> and hence the shorter time between packet header and packet footer
>> followed by increased line blanking interval for the lines with pixel
>> data. While V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK would report no
>> changes at all (as current imx219 driver uses them as though they both
>> belong to the "pixel sampling" domain).
>>
>> And again, if the rate at which pixels are transmitted and the sampling
>> rate in the pixel array are the same, the "pixel sampling" domain and the
>> "data out" domain are no difference.
> 
> Over the past 15 years, a lot of different camera sensor drivers have been
> merged. The capabilities and interfaces exposed by the drivers have changed
> a little over time and also sensor's capabilities and the driver writer's
> use cases have had an effect on the interfaces the drivers implement. The
> APIs have remained the same, while at the same time they haven't been
> exactly a perfect match for the capabilities of the register list based
> drivers.
> 
> So with that background, it's perhaps not surprising that in some areas
> there's variation in how the subdev API is being used by the drivers.
> 
> If there's a desire to improve this, it means changing the user space
> interface of a number of drivers. I don't know how they are being used
> currently. Removing a control from a driver or changing its semantics does
> have some potential for breaking user space.

That's correct.

> If changes in the interface end up being done, they should be compatible
> the goal of providing all the necessary information on sensor configuration
> to the user space, not just the pixel rate in the pixel array. This
> includes analogue crop rectangle, binning (and subsampling), scaling and
> digital crop configurations. Selecting the actual configuration would still
> be based on the source pad of the sub-device representing the sensor's
> transmitter. This means often implementing more sub-devices, in many cases
> the same way the CCS driver does already.

Guess this worth trying on some of the drivers. To prove the concept, to
get an idea of how much extra efforts this adds when writing a new driver,
and to see how these efforts could be minimized.
It should better be done with a new driver (not to break the user space),
but changing a recently added driver not yet widely used might work too.

> I'm also not sure it's worth changing drivers that already use
> SUBDEV_[GS]_FRAME_INTERVAL; they're so far from the target and are most
> likely for older hardware. The more obvious target are drivers that already
> expose the blanking controls.

Leaving the SUBDEV_[GS]_FRAME_INTERVAL drivers alone sounds good for me.

> This is what I could think of to remedy the problems:
> 
> 1. Make all receiver drivers use LINK_FREQ instead of PIXEL_RATE.

I am going to do that for the camss driver anyway.
So I could add the other receiver drivers to the patchset, though I wouldn't
be able to test all or most of them except the camss.

> 2. Remove PIXEL_RATE from sensor drivers in places where it is used to
> convey the pixel rate on the data bus. Steps from this onward may be
> implemented one driver at a time.

Agreed. "One driver at a time" works the best for me.

> 3. Add back the pixel rate control that represents the pixel rate in the
> pixel array. The blanking controls need to be aligned with the analogue
> crop size.
> 
> 4. Implement more sub-devices to convey the sensor's current configuration
> to the user, including analogue crop, binning, scaling and digital crop.
> 
> There's a chance of breaking something in the user space from 2. onwards.
> 
> If you skip the 4. step, the frame rate configuration could work but the
> rest of the sensor configuration (cropping, binning, scaling etc.) would
> remain unknown to the user space.

For 2.-4. I could look at imx290 and ov8856 drivers first (I would have to
regardless of what is discussed in this mail thread).
Currently the both drivers use the same pixel rate on the data bus and in
the pixel array. So there is nothing crying to be fixed ASAP here.
Also for imx290 the PLL setup isn't documented in the datasheet
(just "write these 7 numbers into that 7 registers <period>") which leaves
no room for improvements. For ov8856 the situation looks better.
The both drivers have no cropping or binning support at all at the moment
(don't even report the default analogue crop rectangle) - this needs to
be addressed. (ov8856 is more interesting in this respect as it supports
binning plus both analogue, and digital crop. For imx290 digital crop can be
implemented, but analogue crop could probably not; binning isn't available.)
imx290 driver has quite some fixes in the RPi kernel tree, so the mainline
driver needs to sync up.

> The above does not address selection of modes in register list based
> drivers. It would be nice to have a solution for that, too, but the general
> issue is that it does not fit to the model V4L2 has --- considering all the
> configurations independent. They are that on hardware level but register
> list based drivers do not configure them like that.

Right. One side of the problem is that the sensor datasheets often force
people to implement register list based drivers by providing only partial
description of the hardware.

But there are register list drivers which support only one mode - hence
selection of modes is no problem.
E.g. imx334 driver being upstreamed now, and ov8856 driver.
The latter lists 4 modes currently, but 3280x2464 vs 3264x2448
and 1640x1232 vs 1632x1224 look like different settings for the same mode
(which is a bug imho; each pair should be merged into one mode), and the
16..x12.. ones are the 32..x24.. ones with binning (could be handled through
the selection API?).

> Any thoughts, opinions, comments etc. on this are very welcome.
> 
> Cc also Hans and Mauro.
> 

Thanks,
Andrey

Re: [RFC] Repurpose V4L2_CID_PIXEL_RATE for the sampling rate in the pixel array

[Andrey Konovalov]

Hi Jacopo,

On 01.02.2021 20:54, Jacopo Mondi wrote:
> Hi Andrey, Sakari,
> 
> On Mon, Dec 21, 2020 at 01:45:38PM +0200, Sakari Ailus wrote:
>> Hi Andrey,
>>
>> Thanks for your e-mail.
>>
>> Could you wrap the lines before 80 characters? It's a bit hard to read...
>>
>> On Tue, Dec 08, 2020 at 03:09:25PM +0300, Andrey Konovalov wrote:
>>> Greetings,
>>>
>>> I was checking how V4L2_CID_PIXEL_RATE and V4L2_CID_LINK_FREQ controls
>>> are documented and used in recent media_tree kernel, and found that the
>>> current usage of V4L2_CID_PIXEL_RATE is not consistent.
>>>
>>> This topic seems to be a bit wider, but I'll focus on camera sensors and
>>> the corresponding receivers.
>>>
>>> Not only the receiver drivers often use V4L2_CID_PIXEL_RATE to calculate
>>> the link frequency from (this is much more common than using
>>> V4L2_CID_LINK_FREQ alone, or with fallback to V4L2_CID_PIXEL_RATE if
>>> V4L2_CID_LINK_FREQ is not implemented). The bigger issue is that
>>> V4L2_CID_PIXEL_RATE was intended to give the rate at which pixels are
>>> transmitted (e.g. over CSI-2 link), while it is also seems to be used for
>>> the sampling rate in the pixel array. The problem is that these two rates
>>> are different things, and may have different values.
>>
>> The PIXEL_RATE control indeed has signified, well, the pixel rate, and
>> pixel rate may be different things depending on where it is found.
>>
>> The original use has been both on pixel matrix as well as on the
>> CSI-2 transmitter but of course this is a problem if the two are the same
>> sub-device.
>>
>>>
>>> The proposal is to repurpose V4L2_CID_PIXEL_RATE to report the sampling
>>> rate in the pixel array, and to calculate the symbol rate at which pixels
>>> are transmitted over the link from V4L2_CID_LINK_FREQ value. The receiver
>>> drivers currently using V4L2_CID_PIXEL_RATE to get the link frequency can
>>> be switched to use the existing v4l2_get_link_rate() helper. We also need
>>> to push the sensor drivers to implement V4L2_CID_LINK_FREQ, as quite a
>>> few of them do V4L2_CID_PIXEL_RATE only.
>>
>> Indeed. That has been the idea for some time actually, but first all the
>> receiver drivers need to be converted to use LINK_FREQ. This is easiest
>> done by using the wrapper the ipu3-cio2 driver already uses as it still
>> works even if the sensor driver only implemented PIXEL_RATE.
>>
>>>
>>> I am not sure about the userspace cases, but guess that the userspace is
>>> much more interested in frames per second and exposure values than in
>>> link details. So it would rather use V4L2_CID_PIXEL_RATE for the sampling
>>> rate in the pixel array anyway.
>>
>> That's the case if the sensor driver exposes the rest of the configuration
>> as the device's native parameters (such as analogue crop rectangle and
>> horizontal and vertical blanking). The majority of drivers do not do that,
>> at least currently, but instead use SUBDEV_[GS]_FRAME_INTERVAL. This is not
>> a native interface for camera sensors, and it is not well suited for
>> conveying what other frame intervals are available.
>>
>>>
>>> More details below.
>>>
>>> 1. V4L2_CID_PIXEL_RATE vs V4L2_CID_LINK_FREQ.
>>> ---------------------------------------------
>>>
>>> According to [1]: - V4L2_CID_LINK_FREQ control can be used to get or to
>>> set the frequency of the bus between transmitter
>>>    (e.g. sensor) and receiver (e.g. RX phy on an SOC), This control is r/w
>>>    by default, but is often made read-only by the sensor drivers,
>>> - V4L2_CID_PIXEL_RATE control can be used to get pixel rate in the source pads of the subdev. This control
>>>    is read-only by default.
>>>
>>> [2] is even more explicit: - V4L2_CID_LINK_FREQ control tells the
>>> receiver driver the frequency (and not the symbol rate) of the link, -
>>> V4L2_CID_PIXEL_RATE control may be used by the receiver to obtain the
>>> pixel rate the transmitter uses.
>>>    If the link is the MIPI CSI-2 D-PHY one, the value of the
>>>    V4L2_CID_PIXEL_RATE is calculated as follows:
>>>      pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample
>>> (*)
>>>
>>> [2] also requires the sensor driver connected through CSI-2 or parallel
>>> (BT.601 or BT.656) bus to implement V4L2_CID_LINK_FREQ control, and for
>>> raw sensors V4L2_CID_PIXEL_RATE is also said to be mandatory. Still only
>>> half of sensor drivers under driver/media/i2c in the current media tree
>>> have V4L2_CID_PIXEL_RATE support and even less implement
>>> V4L2_CID_LINK_FREQ control.
>>>
>>> (*) is valid for MIPI CSI-2 D-PHY only, but it is widely used by the
>>
>> Correct. The only driver just got support for CSI-2 C-PHY, so the
>> documentation needs to be adjusted, too. I actually recently sent a patch
>> to address that.
> 
> OT: not to deviate the conversation, but isn't the usage of
> v4l2_get_link_rate() in ipu3 ([8]) missing a / 2 to account for the
> CSI-2 DDR ? Anyway, back to the topic..

It looks like it is. v4l2_get_link_rate()'s 2nd and 3d args are just "mul"
and "div", so the 3d arg in ipu3 driver calling v4l2_get_link_rate()
should be "lanes * 2".

>>> receiver drivers to get CSI-2 link frequency from V4L2_CID_PIXEL_RATE
>>> [3], [4], [5], [6]. For other bus types link frequency can also be
>>> calculated from the pixel rate and some other parameters. There exists a
>>> helper function in drivers/media/v4l2-core/v4l2-common.c for that already
>>> [7]:
>>>    s64 v4l2_get_link_rate(struct v4l2_ctrl_handler *handler, unsigned int
>>> mul, unsigned int div) which tries to get the link rate value using
>>> V4L2_CID_LINK_FREQ, and if it not supported by the sub-device, it tries
>>> V4L2_CID_PIXEL_RATE and calculates the link frequency from the pixel rate
>>> using the multiplier and the divider provided by the caller.
>>> Unfortunately only one driver [8] uses v4l2_get_link_rate(). [9]
>>> implements a similar approach (error handling is different) but on its
>>> own.
>>>
>>> [10] and [11] use V4L2_CID_LINK_FREQ directly.
>>>
>>> Only two drivers use V4L2_CID_PIXEL_RATE for something else than getting
>>> the link frequency [12], [13].
>>>
>>> So in most of the cases the receiver drivers need to know the link
>>> frequency. Often this link frequency is calculated from the pixel rate,
>>> but changing the drivers to use V4L2_CID_LINK_FREQ directly looks fairly
>>> easy.
> 
> I don't have much to add to what you and Sakari said here, and I can't
> not agree that the usage of the PIXEL_RATE to calculate LINK_FREQUENCY
> might be misleading. I would add the I never really got why
> LINK_FREQUENCY has to be menu control. It might make sense for
> mode-based sensor drivers, where for each known mode the link
> frequency can be pre-calculated, but for freely configurable sensors I
> don't actually see the point. Also, if I were to re-design this I
> would not use a control but rather add link frequency to the
> get_mbus_config() kAPI to be used during the transmitter/receiver
> subdevices setup phase.

IIRC Sakari was saying that the link frequency affects the EMI caused
by the device, and this limits the allowed range of values for link
frequency. I take it as passing the EMI tests is much easier if only
small set of fixed frequencies is allowed.

And I am not sure if having the link frequency freely configurable in
a wider continuous range would make the sensor driver significantly more
flexible (while e.g. full binning and crop support is a big advantage over
limited number of fixed resolution modes).

>>> 2. V4L2_CID_PIXEL_RATE: the rate at which pixels are transmitted, or the
>>> sampling rate in the pixel array?
>>> ----------------------------------------------------------------------------------------------------------
>>>
>>> Let's start from the example: "imx219 8-bit raw modes are broken for
>>> camss, the 8MP@15fps one included".
>>>
>>> The imx219 driver supports only 2 data lanes. The link frequency is fixed
>>> at 456MHz. At 8MP the fps is 15, which the driver refers to as 8MP@15fps
>>> mode.
>>>
>>> Originally the driver supported 10-bit raw format only, so the pixel rate
>>> was:
>>>
>>>    IMX219_pixel_rate = IMX219_link_freq * 2 * nr_of_lanes /
>>>                        bits_per_sample = 456 * 2 * 2 / 10 = 182.4 Mpix/sec
>>>
>>> The line length (IMX219_PPL_DEFAULT) is set to 3448 (h blanking
>>> included), the frame length in this mode (IMX219_VTS_15FPS) is 3526 (v
>>> blanking included). Then
>>>
>>>    IMX219_frame_rate_8MP = IMX219_pixel_rate / IMX219_PPL_DEFAULT /
>>>                            IMX219_VTS_15FPS = 182400000 / 3448 / 3526 =
>>>                            15.003 Hz - OK, as
>>> expected.
>>>
>>> The value for read-only V4L2_CID_HBLANK control is calculated as: hblank
>>>          = IMX219_PPL_DEFAULT - mode->width; - and can only change
>>> as the result of switching the modes.
>>>
>>> Then 8-bit raw format was added. And it was done such that switching
>>> between 10 and 8 bits affects the output data domain only - the pixel
>>> read domain continues to work just as before:
>>>    - the number of bits per sample to read from the FIFO is set to 8 or 10
>>>      (there is a FIFO between the "pixel read" and the "MIPI data output"
>>>    halves of the sensor) - the divider for the "read from the FIFO" clock
>>>    is set to 8 or 10 - this clock is 91.2 MHz for 10 bits
>>>      and 114 MHz for 8 bits. So that the link frequency remains the same.
>>>
>>> The important thing to note is that the clock at which the samples are
>>> written to the FIFO remains at 91.2 MHz for both 10 and 8 bits.
>>>
>>> V4L2_CID_PIXEL_RATE control handling was not changed - its value is
>>> always reported as 182.4 Mpix/sec. This immediately broke the receivers
>>> which use V4L2_CID_PIXEL_RATE to configure their phy - e.g. capturing in
>>> 8 bit modes doesn't work on Qualcomm boards using camss driver.
>>>
>>> So my first impression was that this is a bug in the imx219 driver. But
>>> in fact this is a more generic API problem: if we update
>>> V4L2_CID_PIXEL_RATE to fix the issue with the drivers calculating the
>>> link frequency from it, from the equation below [14]:
>>>
>>>    frame interval = (analogue crop width + horizontal blanking) *
>>>                   (analogue crop height + vertical blanking) / pixel_rate
>>> (**)
>>>
>>> - we would see (if we treat the h blanking as it is treated in imx219
>>> driver) that the fps changed from 15 to 18.75 after switching from 10-bit
>>> to 8-bit raw format. While in fact the fps is 15 in the both cases. If we
>>> changed camss to use V4L2_CID_LINK_FREQ (not changed between 8 and 10
>>> bits in this case), and repurposed V4L2_CID_PIXEL_RATE to report the
>>> sampling rate in the pixel array (not changed between 8 and 10 bits in
>>> this case), then (**) would give the correct frame interval and 8-bit
>>> modes would work with camss driver just fine.
>>>
>>> As a summary, this issue happened because the equation (*) assumes that
>>> pixel_rate is the rate at which pixels are transmitted (belongs to the
>>> "MIPI data output" domain), while the imx219 driver and equation (**)
>>> assume that the pixel_rate is the sampling rate in the pixel array. And
>>> while in case of the imx219 driver these two (different by nature) rates
>>> are the same for 10-bit raw formats, for 8-bit raw formats the sampling
>>> rate in the pixel array is lower than the rate at which pixels are
>>> transmitted. Because switching from 10 to 8 bits reconfigures the "MIPI
>>> data output" domain *only*. And this implementation does make sense: this
>>> is quite natural for a user to see no change in fps and exposure (in
>>> absolute units, not in lines) when he/she decides to use more or less
>>> bits per pixel.
>>>
>>> (As a side note - different clocks for reading the pixels from the image
>>> array and for sending them out to MIPI transmitter is a documented
>>> feature of IMX219 sensor.)
>>>
>>> If we return to (**) again, as I see it, this equation makes sense in the
>>> "pixel sampling" domain only. And this is how it seems to be used in the
>>> camera sensor drivers. Also [15] does say "The unit of horizontal
>>> blanking is pixels" about V4L2_CID_HBLANK - this is way easier to work
>>> with in the "pixel sampling" domain. Even though h/v blanking is rather
>>> the notion of "MIPI data output" domain imho. So if we look at what is
>>> happening on the CSI link when imx219 driver switches from 10-bit row to
>>> 8-bit row, we will see the same link frequency, the increased symbol rate
>>> and hence the shorter time between packet header and packet footer
>>> followed by increased line blanking interval for the lines with pixel
>>> data. While V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK would report no
>>> changes at all (as current imx219 driver uses them as though they both
>>> belong to the "pixel sampling" domain).
> 
> Isn't this expected ?

Yes, this is expected.

By "h/v blanking is rather the notion of "MIPI data output" domain imho"
I just meant that V4L2_CID_HBLANK / V4L2_CID_VBLANK usually operate on
"Horizontal Total Size" (in pixels) / "Vertical Total Size" (in lines)
registers (the names vary between sensor families), there are no registers
which directly corresponded to this controls. Blanking is what
is usually shown on the MIPI CSI-2 timing diagrams instead.
For me V4L2_CID_[HV]BLANK-ing is rather a way to express how sampling is
done in the pixel array...

> The pixel rate being accessible to userspace is mostly useful to
> transform parameters expressed in abstract 'lines' (or 'pixels') such
> as the exposure time (or the horizontal blankings) to a wall clock
> time in some second sub-unit.
... and they perfectly fit this usage.

> Shouldn't they be expressed in the
> 'pixel sampling' domain then

Yes, they should.

> and let the CSI-2 transmitter adjust them
> to respect the frame rate/link frequency ratio ? (in my mental picture
> I'm missing where that adjustement happens though, and I assume it's
> encoded in some part of the mode registers list)

To my understanding (as long as we are looking at the streaming phase only)
the CSI transmitter and receiver don't care much of what is happening
between FrameEnd and FrameStart (frame blanking) and between the packet
footer and the packet header (line blanking). The CSI receiver does need
to know the link frequency to set its clock high enough, and to tune the
delays (or something - I am not an expert there), and that's it.

>>> And again, if the rate at which pixels are transmitted and the sampling
>>> rate in the pixel array are the same, the "pixel sampling" domain and the
>>> "data out" domain are no difference.
>>
>> Over the past 15 years, a lot of different camera sensor drivers have been
>> merged. The capabilities and interfaces exposed by the drivers have changed
>> a little over time and also sensor's capabilities and the driver writer's
>> use cases have had an effect on the interfaces the drivers implement. The
>> APIs have remained the same, while at the same time they haven't been
>> exactly a perfect match for the capabilities of the register list based
>> drivers.
>>
>> So with that background, it's perhaps not surprising that in some areas
>> there's variation in how the subdev API is being used by the drivers.
>>
>> If there's a desire to improve this, it means changing the user space
>> interface of a number of drivers. I don't know how they are being used
>> currently. Removing a control from a driver or changing its semantics does
>> have some potential for breaking user space.
>>
>> If changes in the interface end up being done, they should be compatible
>> the goal of providing all the necessary information on sensor configuration
>> to the user space, not just the pixel rate in the pixel array. This
>> includes analogue crop rectangle, binning (and subsampling), scaling and
>> digital crop configurations. Selecting the actual configuration would still
>> be based on the source pad of the sub-device representing the sensor's
>> transmitter. This means often implementing more sub-devices, in many cases
>> the same way the CCS driver does already.
> 
> libcamera is actually struggling with the same questions. So far we
> got away using the selection API to retrieve the analogue crop size
> and other information on the pixel array, but when it's about how to
> convey other parameters, such as the subsampling mode in use, the
> current API fall shorts. One use case for sub-sampling is to estimate
> the FOV and make it possible to identify 'modes' that have the same
> FOV but might be faster to transmit (higher frame rate).
> 
> Digital scalers could be instead worked around by using multiple
> subdevs and make the scaling step explicit between pads of different
> entities, that's my understanding at least.
> 
> Ideally, with multiple subdevices in place, and an opportune usage of
> selection targets those information might be made available with the
> current API probably, by mapping the analogue crop rectangle and the
> sub-sampling to the TGT_CROP and TGT_COMPOSE target of a dedicated
> sub-devices. Digital scalers would require yet another sudbdev..

Yes, splitting the camera sensors into multiple subdevices sounds
promising. To look how this is implemented in the ccs driver and how
this approach could be used for other camera sensor drivers is in
my todo list.

>> I'm also not sure it's worth changing drivers that already use
>> SUBDEV_[GS]_FRAME_INTERVAL; they're so far from the target and are most
>> likely for older hardware. The more obvious target are drivers that already
>> expose the blanking controls.
>>
>> This is what I could think of to remedy the problems:
>>
>> 1. Make all receiver drivers use LINK_FREQ instead of PIXEL_RATE.
> 
> could using a kAPI to convey the link frequency make the transition
> less painful? so that if there's any userspace component that relies
> on the LINK_FREQUENCY control we don't break it ?

The LINK_FREQUENCY control would not change (unless it would be replaced
with kAPI). This is PIXEL_RATE control which behavior might change for
some camera sensor drivers.
If PIXEL_RATE implementation in the sensor driver is correct, and the sensor
driver doesn't use different rates to read the pixels from the image array
and to send them over the CSI-2 link, there should be no breakage at all
(as far as I can see).

>> 2. Remove PIXEL_RATE from sensor drivers in places where it is used to
>> convey the pixel rate on the data bus.

- and replace it with LINK_FREQUENCY (quite some sensor drivers implement
PIXEL_RATE only)

>> Steps from this onward may be
>> implemented one driver at a time.
>>
>> 3. Add back the pixel rate control that represents the pixel rate in the
>> pixel array. The blanking controls need to be aligned with the analogue
>> crop size.
>>
>> 4. Implement more sub-devices to convey the sensor's current configuration
>> to the user, including analogue crop, binning, scaling and digital crop.
>>
>> There's a chance of breaking something in the user space from 2. onwards.
>>
>> If you skip the 4. step, the frame rate configuration could work but the
>> rest of the sensor configuration (cropping, binning, scaling etc.) would
>> remain unknown to the user space.
>>
>> The above does not address selection of modes in register list based
>> drivers. It would be nice to have a solution for that, too, but the general
>> issue is that it does not fit to the model V4L2 has --- considering all the
>> configurations independent. They are that on hardware level but register
>> list based drivers do not configure them like that.
>>
>> Any thoughts, opinions, comments etc. on this are very welcome.
>>
> 
> Let me point you to the libcamera component that
> handles camera sensors and which is currently limited to use a few
> selection targets and a few controls. Ideally, if we grow the
> two side together to support more complex use cases, it provides a
> place where to test any new/repurposed API.

Yes, the both sides would benefit from that.

> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/camera_sensor.cpp#n666
> https://git.linuxtv.org/libcamera.git/tree/Documentation/sensor_driver_requirements.rst

Noted, thanks!

> Thanks
>     j

Thanks,
Andrey

>> Cc also Hans and Mauro.
>>
>> --
>> Kind regards,
>>
>> Sakari Ailus
>>

Re: [RFC] Repurpose V4L2_CID_PIXEL_RATE for the sampling rate in the pixel array

[Jacopo Mondi]

Hi Andrey, Sakari,

On Mon, Dec 21, 2020 at 01:45:38PM +0200, Sakari Ailus wrote:
> Hi Andrey,
>
> Thanks for your e-mail.
>
> Could you wrap the lines before 80 characters? It's a bit hard to read...
>
> On Tue, Dec 08, 2020 at 03:09:25PM +0300, Andrey Konovalov wrote:
> > Greetings,
> >
> > I was checking how V4L2_CID_PIXEL_RATE and V4L2_CID_LINK_FREQ controls
> > are documented and used in recent media_tree kernel, and found that the
> > current usage of V4L2_CID_PIXEL_RATE is not consistent.
> >
> > This topic seems to be a bit wider, but I'll focus on camera sensors and
> > the corresponding receivers.
> >
> > Not only the receiver drivers often use V4L2_CID_PIXEL_RATE to calculate
> > the link frequency from (this is much more common than using
> > V4L2_CID_LINK_FREQ alone, or with fallback to V4L2_CID_PIXEL_RATE if
> > V4L2_CID_LINK_FREQ is not implemented). The bigger issue is that
> > V4L2_CID_PIXEL_RATE was intended to give the rate at which pixels are
> > transmitted (e.g. over CSI-2 link), while it is also seems to be used for
> > the sampling rate in the pixel array. The problem is that these two rates
> > are different things, and may have different values.
>
> The PIXEL_RATE control indeed has signified, well, the pixel rate, and
> pixel rate may be different things depending on where it is found.
>
> The original use has been both on pixel matrix as well as on the
> CSI-2 transmitter but of course this is a problem if the two are the same
> sub-device.
>
> >
> > The proposal is to repurpose V4L2_CID_PIXEL_RATE to report the sampling
> > rate in the pixel array, and to calculate the symbol rate at which pixels
> > are transmitted over the link from V4L2_CID_LINK_FREQ value. The receiver
> > drivers currently using V4L2_CID_PIXEL_RATE to get the link frequency can
> > be switched to use the existing v4l2_get_link_rate() helper. We also need
> > to push the sensor drivers to implement V4L2_CID_LINK_FREQ, as quite a
> > few of them do V4L2_CID_PIXEL_RATE only.
>
> Indeed. That has been the idea for some time actually, but first all the
> receiver drivers need to be converted to use LINK_FREQ. This is easiest
> done by using the wrapper the ipu3-cio2 driver already uses as it still
> works even if the sensor driver only implemented PIXEL_RATE.
>
> >
> > I am not sure about the userspace cases, but guess that the userspace is
> > much more interested in frames per second and exposure values than in
> > link details. So it would rather use V4L2_CID_PIXEL_RATE for the sampling
> > rate in the pixel array anyway.
>
> That's the case if the sensor driver exposes the rest of the configuration
> as the device's native parameters (such as analogue crop rectangle and
> horizontal and vertical blanking). The majority of drivers do not do that,
> at least currently, but instead use SUBDEV_[GS]_FRAME_INTERVAL. This is not
> a native interface for camera sensors, and it is not well suited for
> conveying what other frame intervals are available.
>
> >
> > More details below.
> >
> > 1. V4L2_CID_PIXEL_RATE vs V4L2_CID_LINK_FREQ.
> > ---------------------------------------------
> >
> > According to [1]: - V4L2_CID_LINK_FREQ control can be used to get or to
> > set the frequency of the bus between transmitter
> >   (e.g. sensor) and receiver (e.g. RX phy on an SOC), This control is r/w
> >   by default, but is often made read-only by the sensor drivers,
> > - V4L2_CID_PIXEL_RATE control can be used to get pixel rate in the source pads of the subdev. This control
> >   is read-only by default.
> >
> > [2] is even more explicit: - V4L2_CID_LINK_FREQ control tells the
> > receiver driver the frequency (and not the symbol rate) of the link, -
> > V4L2_CID_PIXEL_RATE control may be used by the receiver to obtain the
> > pixel rate the transmitter uses.
> >   If the link is the MIPI CSI-2 D-PHY one, the value of the
> >   V4L2_CID_PIXEL_RATE is calculated as follows:
> >     pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample
> > (*)
> >
> > [2] also requires the sensor driver connected through CSI-2 or parallel
> > (BT.601 or BT.656) bus to implement V4L2_CID_LINK_FREQ control, and for
> > raw sensors V4L2_CID_PIXEL_RATE is also said to be mandatory. Still only
> > half of sensor drivers under driver/media/i2c in the current media tree
> > have V4L2_CID_PIXEL_RATE support and even less implement
> > V4L2_CID_LINK_FREQ control.
> >
> > (*) is valid for MIPI CSI-2 D-PHY only, but it is widely used by the
>
> Correct. The only driver just got support for CSI-2 C-PHY, so the
> documentation needs to be adjusted, too. I actually recently sent a patch
> to address that.

OT: not to deviate the conversation, but isn't the usage of
v4l2_get_link_rate() in ipu3 ([8]) missing a / 2 to account for the
CSI-2 DDR ? Anyway, back to the topic..

>
> > receiver drivers to get CSI-2 link frequency from V4L2_CID_PIXEL_RATE
> > [3], [4], [5], [6]. For other bus types link frequency can also be
> > calculated from the pixel rate and some other parameters. There exists a
> > helper function in drivers/media/v4l2-core/v4l2-common.c for that already
> > [7]:
> >   s64 v4l2_get_link_rate(struct v4l2_ctrl_handler *handler, unsigned int
> > mul, unsigned int div) which tries to get the link rate value using
> > V4L2_CID_LINK_FREQ, and if it not supported by the sub-device, it tries
> > V4L2_CID_PIXEL_RATE and calculates the link frequency from the pixel rate
> > using the multiplier and the divider provided by the caller.
> > Unfortunately only one driver [8] uses v4l2_get_link_rate(). [9]
> > implements a similar approach (error handling is different) but on its
> > own.
> >
> > [10] and [11] use V4L2_CID_LINK_FREQ directly.
> >
> > Only two drivers use V4L2_CID_PIXEL_RATE for something else than getting
> > the link frequency [12], [13].
> >
> > So in most of the cases the receiver drivers need to know the link
> > frequency. Often this link frequency is calculated from the pixel rate,
> > but changing the drivers to use V4L2_CID_LINK_FREQ directly looks fairly
> > easy.

I don't have much to add to what you and Sakari said here, and I can't
not agree that the usage of the PIXEL_RATE to calculate LINK_FREQUENCY
might be misleading. I would add the I never really got why
LINK_FREQUENCY has to be menu control. It might make sense for
mode-based sensor drivers, where for each known mode the link
frequency can be pre-calculated, but for freely configurable sensors I
don't actually see the point. Also, if I were to re-design this I
would not use a control but rather add link frequency to the
get_mbus_config() kAPI to be used during the transmitter/receiver
subdevices setup phase.

> >
> >
> > 2. V4L2_CID_PIXEL_RATE: the rate at which pixels are transmitted, or the
> > sampling rate in the pixel array?
> > ----------------------------------------------------------------------------------------------------------
> >
> > Let's start from the example: "imx219 8-bit raw modes are broken for
> > camss, the 8MP@15fps one included".
> >
> > The imx219 driver supports only 2 data lanes. The link frequency is fixed
> > at 456MHz. At 8MP the fps is 15, which the driver refers to as 8MP@15fps
> > mode.
> >
> > Originally the driver supported 10-bit raw format only, so the pixel rate
> > was:
> >
> >   IMX219_pixel_rate = IMX219_link_freq * 2 * nr_of_lanes /
> >                       bits_per_sample = 456 * 2 * 2 / 10 = 182.4 Mpix/sec
> >
> > The line length (IMX219_PPL_DEFAULT) is set to 3448 (h blanking
> > included), the frame length in this mode (IMX219_VTS_15FPS) is 3526 (v
> > blanking included). Then
> >
> >   IMX219_frame_rate_8MP = IMX219_pixel_rate / IMX219_PPL_DEFAULT /
> >                           IMX219_VTS_15FPS = 182400000 / 3448 / 3526 =
> >                           15.003 Hz - OK, as
> > expected.
> >
> > The value for read-only V4L2_CID_HBLANK control is calculated as: hblank
> >         = IMX219_PPL_DEFAULT - mode->width; - and can only change
> > as the result of switching the modes.
> >
> > Then 8-bit raw format was added. And it was done such that switching
> > between 10 and 8 bits affects the output data domain only - the pixel
> > read domain continues to work just as before:
> >   - the number of bits per sample to read from the FIFO is set to 8 or 10
> >     (there is a FIFO between the "pixel read" and the "MIPI data output"
> >   halves of the sensor) - the divider for the "read from the FIFO" clock
> >   is set to 8 or 10 - this clock is 91.2 MHz for 10 bits
> >     and 114 MHz for 8 bits. So that the link frequency remains the same.
> >
> > The important thing to note is that the clock at which the samples are
> > written to the FIFO remains at 91.2 MHz for both 10 and 8 bits.
> >
> > V4L2_CID_PIXEL_RATE control handling was not changed - its value is
> > always reported as 182.4 Mpix/sec. This immediately broke the receivers
> > which use V4L2_CID_PIXEL_RATE to configure their phy - e.g. capturing in
> > 8 bit modes doesn't work on Qualcomm boards using camss driver.
> >
> > So my first impression was that this is a bug in the imx219 driver. But
> > in fact this is a more generic API problem: if we update
> > V4L2_CID_PIXEL_RATE to fix the issue with the drivers calculating the
> > link frequency from it, from the equation below [14]:
> >
> >   frame interval = (analogue crop width + horizontal blanking) *
> >                  (analogue crop height + vertical blanking) / pixel_rate
> > (**)
> >
> > - we would see (if we treat the h blanking as it is treated in imx219
> > driver) that the fps changed from 15 to 18.75 after switching from 10-bit
> > to 8-bit raw format. While in fact the fps is 15 in the both cases. If we
> > changed camss to use V4L2_CID_LINK_FREQ (not changed between 8 and 10
> > bits in this case), and repurposed V4L2_CID_PIXEL_RATE to report the
> > sampling rate in the pixel array (not changed between 8 and 10 bits in
> > this case), then (**) would give the correct frame interval and 8-bit
> > modes would work with camss driver just fine.
> >
> > As a summary, this issue happened because the equation (*) assumes that
> > pixel_rate is the rate at which pixels are transmitted (belongs to the
> > "MIPI data output" domain), while the imx219 driver and equation (**)
> > assume that the pixel_rate is the sampling rate in the pixel array. And
> > while in case of the imx219 driver these two (different by nature) rates
> > are the same for 10-bit raw formats, for 8-bit raw formats the sampling
> > rate in the pixel array is lower than the rate at which pixels are
> > transmitted. Because switching from 10 to 8 bits reconfigures the "MIPI
> > data output" domain *only*. And this implementation does make sense: this
> > is quite natural for a user to see no change in fps and exposure (in
> > absolute units, not in lines) when he/she decides to use more or less
> > bits per pixel.
> >
> > (As a side note - different clocks for reading the pixels from the image
> > array and for sending them out to MIPI transmitter is a documented
> > feature of IMX219 sensor.)
> >
> > If we return to (**) again, as I see it, this equation makes sense in the
> > "pixel sampling" domain only. And this is how it seems to be used in the
> > camera sensor drivers. Also [15] does say "The unit of horizontal
> > blanking is pixels" about V4L2_CID_HBLANK - this is way easier to work
> > with in the "pixel sampling" domain. Even though h/v blanking is rather
> > the notion of "MIPI data output" domain imho. So if we look at what is
> > happening on the CSI link when imx219 driver switches from 10-bit row to
> > 8-bit row, we will see the same link frequency, the increased symbol rate
> > and hence the shorter time between packet header and packet footer
> > followed by increased line blanking interval for the lines with pixel
> > data. While V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK would report no
> > changes at all (as current imx219 driver uses them as though they both
> > belong to the "pixel sampling" domain).

Isn't this expected ?

The pixel rate being accessible to userspace is mostly useful to
transform parameters expressed in abstract 'lines' (or 'pixels') such
as the exposure time (or the horizontal blankings) to a wall clock
time in some second sub-unit. Shouldn't they be expressed in the
'pixel sampling' domain then and let the CSI-2 transmitter adjust them
to respect the frame rate/link frequency ratio ? (in my mental picture
I'm missing where that adjustement happens though, and I assume it's
encoded in some part of the mode registers list)

> >
> > And again, if the rate at which pixels are transmitted and the sampling
> > rate in the pixel array are the same, the "pixel sampling" domain and the
> > "data out" domain are no difference.
>
> Over the past 15 years, a lot of different camera sensor drivers have been
> merged. The capabilities and interfaces exposed by the drivers have changed
> a little over time and also sensor's capabilities and the driver writer's
> use cases have had an effect on the interfaces the drivers implement. The
> APIs have remained the same, while at the same time they haven't been
> exactly a perfect match for the capabilities of the register list based
> drivers.
>
> So with that background, it's perhaps not surprising that in some areas
> there's variation in how the subdev API is being used by the drivers.
>
> If there's a desire to improve this, it means changing the user space
> interface of a number of drivers. I don't know how they are being used
> currently. Removing a control from a driver or changing its semantics does
> have some potential for breaking user space.
>
> If changes in the interface end up being done, they should be compatible
> the goal of providing all the necessary information on sensor configuration
> to the user space, not just the pixel rate in the pixel array. This
> includes analogue crop rectangle, binning (and subsampling), scaling and
> digital crop configurations. Selecting the actual configuration would still
> be based on the source pad of the sub-device representing the sensor's
> transmitter. This means often implementing more sub-devices, in many cases
> the same way the CCS driver does already.

libcamera is actually struggling with the same questions. So far we
got away using the selection API to retrieve the analogue crop size
and other information on the pixel array, but when it's about how to
convey other parameters, such as the subsampling mode in use, the
current API fall shorts. One use case for sub-sampling is to estimate
the FOV and make it possible to identify 'modes' that have the same
FOV but might be faster to transmit (higher frame rate).

Digital scalers could be instead worked around by using multiple
subdevs and make the scaling step explicit between pads of different
entities, that's my understanding at least.

Ideally, with multiple subdevices in place, and an opportune usage of
selection targets those information might be made available with the
current API probably, by mapping the analogue crop rectangle and the
sub-sampling to the TGT_CROP and TGT_COMPOSE target of a dedicated
sub-devices. Digital scalers would require yet another sudbdev..

>
> I'm also not sure it's worth changing drivers that already use
> SUBDEV_[GS]_FRAME_INTERVAL; they're so far from the target and are most
> likely for older hardware. The more obvious target are drivers that already
> expose the blanking controls.
>
> This is what I could think of to remedy the problems:
>
> 1. Make all receiver drivers use LINK_FREQ instead of PIXEL_RATE.

could using a kAPI to convey the link frequency make the transition
less painful? so that if there's any userspace component that relies
on the LINK_FREQUENCY control we don't break it ?

>
> 2. Remove PIXEL_RATE from sensor drivers in places where it is used to
> convey the pixel rate on the data bus. Steps from this onward may be
> implemented one driver at a time.
>
> 3. Add back the pixel rate control that represents the pixel rate in the
> pixel array. The blanking controls need to be aligned with the analogue
> crop size.
>
> 4. Implement more sub-devices to convey the sensor's current configuration
> to the user, including analogue crop, binning, scaling and digital crop.
>
> There's a chance of breaking something in the user space from 2. onwards.
>
> If you skip the 4. step, the frame rate configuration could work but the
> rest of the sensor configuration (cropping, binning, scaling etc.) would
> remain unknown to the user space.
>
> The above does not address selection of modes in register list based
> drivers. It would be nice to have a solution for that, too, but the general
> issue is that it does not fit to the model V4L2 has --- considering all the
> configurations independent. They are that on hardware level but register
> list based drivers do not configure them like that.
>
> Any thoughts, opinions, comments etc. on this are very welcome.
>

Let me point you to the libcamera component that
handles camera sensors and which is currently limited to use a few
selection targets and a few controls. Ideally, if we grow the
two side together to support more complex use cases, it provides a
place where to test any new/repurposed API.

https://git.linuxtv.org/libcamera.git/tree/src/libcamera/camera_sensor.cpp#n666
https://git.linuxtv.org/libcamera.git/tree/Documentation/sensor_driver_requirements.rst

Thanks
   j

> Cc also Hans and Mauro.
>
> --
> Kind regards,
>
> Sakari Ailus
>