Hi Dave
On Fri, Oct 07, 2022 at 03:26:55PM +0100, Dave Stevenson wrote:
> On Fri, 7 Oct 2022 at 15:01, Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, Oct 06, 2022 at 04:10:10PM +0100, Dave Stevenson wrote:
> > > On Wed, 5 Oct 2022 at 20:07, Jacopo Mondi wrote:
> > > >
> > > > Add support for V4L2_CID_LINK_FREQ which currently reports a single
> > > > hard-coded frequency which depends on the fixed pixel clock.
> > > >
> > > > This will change in the next patches where the pixel rate will be
> > > > computed from the desired link_frequency.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
> > >
> > > Looks valid based on the current pixel rate of 184MPix/s, 8bpp,
> > > divided by 4 lanes, and DDR.
> > >
> > > > ---
> > > > drivers/media/i2c/ar0521.c | 9 +++++++++
> > > > 1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> > > > index 21649aecf442..c5410b091654 100644
> > > > --- a/drivers/media/i2c/ar0521.c
> > > > +++ b/drivers/media/i2c/ar0521.c
> > > > @@ -90,6 +90,10 @@ static const char * const ar0521_supply_names[] = {
> > > > "vaa", /* Analog (2.7V) supply */
> > > > };
> > > >
> > > > +static const s64 ar0521_link_frequencies[] = {
> > > > + 184000000,
> > > > +};
> > > > +
> > > > struct ar0521_ctrls {
> > > > struct v4l2_ctrl_handler handler;
> > > > struct v4l2_ctrl *ana_gain;
> > > > @@ -104,6 +108,7 @@ struct ar0521_ctrls {
> > > > };
> > > > struct v4l2_ctrl *pixrate;
> > > > struct v4l2_ctrl *exposure;
> > > > + struct v4l2_ctrl *link_freq;
> > > > struct v4l2_ctrl *test_pattern;
> > > > };
> > > >
> > > > @@ -655,6 +660,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
> > > > ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0,
> > > > 65535, 1, 360);
> > > >
> > > > + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
> > > > + ARRAY_SIZE(ar0521_link_frequencies) - 1,
> > > > + 0, ar0521_link_frequencies);
> > > > +
> > >
> > > Admittedly there is only one entry, but did you want to make it a read
> > > only control? With no case for it in s_ctrl, you'll get errors thrown
> > > from the control handler framework.
> >
> > I'd make it writable even if there's a single entry, so that userspace
> > won't need special logic. It will also prepare for support of multiple
> > entries in the future.
>
> Do you really see a situation where userspace will be configuring link
> frequency instead of DT / ACPI?
> A quick search seems to imply that only 1 current driver supports a
> r/w link frequency - mt9v032. That would imply that having a
> controllable link frequency would require the special logic in
> userspace.
>
Yes it does, but we need one way or another to allow userspace to
control the sampling frequency as extending (or shrinking) blankings
helps up to the point you reach their limits.
I was never really fond of the idea that such action should go through
link frequency, which seems very much a parameter of the bus that
should be negotiated between the recv and the tx parts, rather than
being user configurable.
> I'm always very cautious about drivers that are linking PIXEL_RATE and
> LINK_FREQ - most of the sensors are tending to have 2 (or more) PLLs,
> and there is a FIFO between the image sensor (PIXEL_RATE) and the MIPI
> block (LINK_FREQ). imx290 is certainly wrong (pixel rate does not
> change with mode, but link freq does), and I'm fairly certain that
> ov7251 is as well (pixel rate is 48MPix/s whether at 240 or 319.2MHz
> link frequency). Patches coming soon for both.
The current definition of PIXEL_RATE indeed describes the sampling
frequency on the pixel array, which might or might not reflect the
output pixel rate. However most if not all usages of PIXEL_RATE I've
seen (and FTR the way it is used in libcamera) is to denote the output
pixel rate (ie it is used to compute the output timings given the line
length and frame height)
I wonder
1) The current definition of PIXEL_RATE as the sampling rate on the
pixel array: what purposes does it serve ? Are there algorithms that
require to know the sampling rate in the analog domain ? Are there
implementations that treat PIXEL_RATE differently than the "pixel
output rate" ?
2) LINK_FREQ is the closest control we have to express the output
pixel rate, but to me is very specific to the bus configuration and
does not express per se anything useful to userspace for computing
timings based on frame/lane sizes. The fact LINK_FREQ is a menu contol
reflects how much it relates to the HW configuration as it is assumed
to come from DT
Do we need an r/w PIXEL_OUTPUT_RATE control to replace
- LINK_FREQ for userspace to configure it
- PIXEL_RATE for userspace to read it
LINK_FREQ should only be used in the tx/rx negotiation. It shall
vary according to PIXEL_OUTPUT_RATE, possibily in the options
specified in DTS (which are there because they have usually been
validated for RF emissions, that's my understanding at least).
PIXEL_RATE will equally vary, if required, and algorithms that need to
know the sampling frequency in the analog domain will continue using
it.
Or maybe the original idea was to have a pixel array entity and a
separate tx entity, each of them with different PIXEL_RATE control ?
>
> Dave
>
> > > > ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops,
> > > > V4L2_CID_TEST_PATTERN,
> > > > ARRAY_SIZE(test_pattern_menu) - 1,
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
