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

[Andrey Konovalov <andrey.konovalov@linaro.org>]

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