Re: [PATCH 05/10] media: ar0521: Add LINK_FREQ control

[Dave Stevenson <dave.stevenson@raspberrypi.com>]

Hi Laurent

On Sun, 16 Oct 2022 at 02:53, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Dave,
>
> On Fri, Oct 07, 2022 at 03:26:55PM +0100, Dave Stevenson wrote:
> > On Fri, 7 Oct 2022 at 15:01, Laurent Pinchart wrote:
> > > On Thu, Oct 06, 2022 at 04:10:10PM +0100, Dave Stevenson wrote:
> > > > On Wed, 5 Oct 2022 at 20:07, Jacopo Mondi wrote:
> > > > >
> > > > > Add support for V4L2_CID_LINK_FREQ which currently reports a single
> > > > > hard-coded frequency which depends on the fixed pixel clock.
> > > > >
> > > > > This will change in the next patches where the pixel rate will be
> > > > > computed from the desired link_frequency.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > >
> > > > Looks valid based on the current pixel rate of 184MPix/s, 8bpp,
> > > > divided by 4 lanes, and DDR.
> > > >
> > > > > ---
> > > > >  drivers/media/i2c/ar0521.c | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > >
> > > > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> > > > > index 21649aecf442..c5410b091654 100644
> > > > > --- a/drivers/media/i2c/ar0521.c
> > > > > +++ b/drivers/media/i2c/ar0521.c
> > > > > @@ -90,6 +90,10 @@ static const char * const ar0521_supply_names[] = {
> > > > >         "vaa",          /* Analog (2.7V) supply */
> > > > >  };
> > > > >
> > > > > +static const s64 ar0521_link_frequencies[] = {
> > > > > +       184000000,
> > > > > +};
> > > > > +
> > > > >  struct ar0521_ctrls {
> > > > >         struct v4l2_ctrl_handler handler;
> > > > >         struct v4l2_ctrl *ana_gain;
> > > > > @@ -104,6 +108,7 @@ struct ar0521_ctrls {
> > > > >         };
> > > > >         struct v4l2_ctrl *pixrate;
> > > > >         struct v4l2_ctrl *exposure;
> > > > > +       struct v4l2_ctrl *link_freq;
> > > > >         struct v4l2_ctrl *test_pattern;
> > > > >  };
> > > > >
> > > > > @@ -655,6 +660,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
> > > > >         ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0,
> > > > >                                             65535, 1, 360);
> > > > >
> > > > > +       ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
> > > > > +                                       ARRAY_SIZE(ar0521_link_frequencies) - 1,
> > > > > +                                       0, ar0521_link_frequencies);
> > > > > +
> > > >
> > > > Admittedly there is only one entry, but did you want to make it a read
> > > > only control? With no case for it in s_ctrl, you'll get errors thrown
> > > > from the control handler framework.
> > >
> > > I'd make it writable even if there's a single entry, so that userspace
> > > won't need special logic. It will also prepare for support of multiple
> > > entries in the future.
> >
> > Do you really see a situation where userspace will be configuring link
> > frequency instead of DT / ACPI?
> > A quick search seems to imply that only 1 current driver supports a
> > r/w link frequency - mt9v032. That would imply that having a
> > controllable link frequency would require the special logic in
> > userspace.
>
> Looking at the mainline kernel only, the N9, N950 and Librem5 all
> specify multiple link frequencies, and so does the sdm845-db845c
> development board.

Sorry, insufficient time to go through all of those in detail.

Looking at sdm845-db845c, yes the base DT lists multiple link
frequencies for OV8856 [1], however the OV8856 driver then makes the
LINK_FREQ control READ_ONLY[2].
So what is userspace meant to do with that information?

IMX290 also requires 2 link frequencies to be listed, but the control
is read only, and which is used is based on mode selection.
(Hmm, I've just noticed that the DT binding example doesn't match the
driver as it only lists one link-freq. OV8856 binding lists the
restriction of the driver in the binding).

N9 and N950 use "nokia,smia", which maps to CCS. Yes that driver does
allow configuration of link frequency from userspace, but it still
feels to me to be an odd control to allow userspace to control.

[1] https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/qcom/sdm845-db845c.dts#L1240
[2] https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov8856.c#L1949

> This indeed requires specific logic in userspace, and to be honest, I
> haven't really thought about how it would be implemented. Sakari has
> more experience than me here, he may be able to shed some light.
>
> > I'm always very cautious about drivers that are linking PIXEL_RATE and
> > LINK_FREQ - most of the sensors are tending to have 2 (or more) PLLs,
> > and there is a FIFO between the image sensor (PIXEL_RATE) and the MIPI
> > block (LINK_FREQ). imx290 is certainly wrong (pixel rate does not
> > change with mode, but link freq does), and I'm fairly certain that
> > ov7251 is as well (pixel rate is 48MPix/s whether at 240 or 319.2MHz
> > link frequency). Patches coming soon for both.
>
> That's a good point, different link frequencies may or may not result in
> different pixel rates.

See my response to Jacopo.

As you know I have a fair number of sensors around, and I see a fairly
large number of the mainline drivers getting this wrong. I don't know
whether that is down to copy/paste of drivers, or misunderstanding of
the requirements, but it fouls up frame rate control and exposure
calibration in almost every case.

  Dave

> > > >         ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl, ops,
> > > > >                                         V4L2_CID_TEST_PATTERN,
> > > > >                                         ARRAY_SIZE(test_pattern_menu) - 1,
>
> --
> Regards,
>
> Laurent Pinchart