linux-kernel - Re: [PATCH v2 2/2] media: i2c: ov02e10: add OV02E10 image sensor driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5fd2253f-0acb-4c95-b3bb-e7e065c92dd5@redhat.com>
Date: Tue, 1 Apr 2025 10:12:35 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Bryan O'Donoghue <bod@...nel.org>,
 Hans de Goede <hansg@...nel.org>, Jingjing Xiong <jingjing.xiong@...el.com>,
 Hao Yao <hao.yao@...el.com>, Jim Lai <jim.lai@...el.com>,
 You-Sheng Yang <vicamo.yang@...onical.com>,
 Alan Stern <stern@...land.harvard.edu>, linux-kernel@...r.kernel.org,
 linux-media@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 2/2] media: i2c: ov02e10: add OV02E10 image sensor
 driver

Hi Bryan, Sakari,

On 1-Apr-25 2:34 AM, Bryan O'Donoghue wrote:
> On 27/03/2025 07:36, Sakari Ailus wrote:
>>> +static u64 to_pixel_rate(u32 f_index)
>>> +{
>>> +    u64 pixel_rate = link_freq_menu_items[f_index] * 2 * OV02E10_DATA_LANES;
>>> +
>>> +    do_div(pixel_rate, OV02E10_RGB_DEPTH);
>> The pixel rate control is for the pixel rate on the pixel array, not on the
>> CSI-2 interface. Without binning or sub-sampling these may the same still,
>> but this only works in special cases really.
> 
> Hmm computer says no, I don't think I have understood this comment..
> 
> Looking at other drivers, I'd say the above pattern is pretty common - taking ov8856 as an example that's pretty much equivalent logic to the above, ov08x40 does something similar.
> 
> =>
> 
> pixel_rate == link_freq * 2 * #oflanes / RGB_DEPTH
>            => 360MHz * 2 * 2 / 10
>            => 360000000 * 2 * 2 / 10
>            => 144000000
> 
> If I'm understanding you though you mean the pixel rate for the control V4L2_CID_PIXEL_RATE expressed here should be the resolution * the FPS / bits_per_pixel

I have to agree with Bryan here that the pixelrate typically is const
and directly derived from the link-frequency. Even the
__v4l2_get_link_freq_ctrl() helper from drivers/media/v4l2-core/v4l2-common.c
assumes this.

binning / subsampling does not change anything wrt the pixelrate it
just means that either the blanking becomes much bigger keeping
vts / hts the same, or that the FPS becomes much higher.

It is not like the sensor is sending an empty pixel on the CSI
link every other pixel when binning, since there is no such
thing as an empty pixel. So the sensor must go faster when doing
horizontal binning to keep the CSI link filled effectively
doubling the FPS, or requiring a much larger hblank after having
taken only half the time sending pixels.

(and the same applies to vts when vertical binning)

> pixel_rate = wdith x height x fps / bpp
>            => 1928 * 1088 * 30 / 10
>            => 6292992
> 
> i.e. the pixel rate not related to the CSI2 link frequency ?

No the pixel-rate control includes vblank + hblank "pixels"
and is in pixels/sec so no dividing by bpp, iow it is:

vts * hts * fps

and this must match

link_freq * 2 * #oflanes / RGB_DEPTH

Regards,

Hans