Re: [PATCH v3 2/7] media: i2c: imx290: Define absolute control ranges

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Sep 02, 2024 at 09:43:36PM +0200, Benjamin Bara wrote:
> Hi Dave!
> 
> On Mon, 2 Sept 2024 at 20:00, Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> > On Mon, 2 Sept 2024 at 16:58, <bbara93@gmail.com> wrote:
> > >
> > > From: Benjamin Bara <benjamin.bara@skidata.com>
> > >
> > > For now, the driver activates the first mode (1080p) as current active
> > > mode in probe(). This e.g. means that one cannot set VBLANK below 45
> > > (vmax_min - height), although theoretically the minimum is 30 (720p
> > > mode). Define the absolute possible/supported ranges to have them
> > > available later.
> >
> > Currently the driver will set the ranges for VBLANK and HBLANK
> > whenever the mode changes.
> >
> > How is it helpful to fake these numbers? Seeing as they aren't
> > reflecting anything useful, they may as well all be 0.
> >
> > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > > ---
> > > Changes since v2:
> > > - new
> > > ---
> > >  drivers/media/i2c/imx290.c | 36 ++++++++++++++++++++++++++++++++----
> > >  1 file changed, 32 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 1c97f9650eb4..466492bab600 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -499,6 +499,10 @@ static const struct imx290_clk_cfg imx290_720p_clock_config[] = {
> > >  };
> > >
> > >  /* Mode configs */
> > > +#define WIDTH_720P     1280
> > > +#define HEIGHT_720P    720

That's pure obfuscation :-) Just use the values directly.

> > > +#define MINIMUM_WIDTH  WIDTH_720P
> > > +#define MINIMUM_HEIGHT HEIGHT_720P

Driver-specific macros should have a driver-specific prefix.

> > >  static const struct imx290_mode imx290_modes_2lanes[] = {
> > >         {
> > >                 .width = 1920,
> > > @@ -512,8 +516,8 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
> > >                 .clk_cfg = imx290_1080p_clock_config,
> > >         },
> > >         {
> > > -               .width = 1280,
> > > -               .height = 720,
> > > +               .width = WIDTH_720P,
> > > +               .height = HEIGHT_720P,
> > >                 .hmax_min = 3300,
> > >                 .vmax_min = 750,
> > >                 .link_freq_index = FREQ_INDEX_720P,
> > > @@ -537,8 +541,8 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> > >                 .clk_cfg = imx290_1080p_clock_config,
> > >         },
> > >         {
> > > -               .width = 1280,
> > > -               .height = 720,
> > > +               .width = WIDTH_720P,
> > > +               .height = HEIGHT_720P,
> > >                 .hmax_min = 3300,
> > >                 .vmax_min = 750,
> > >                 .link_freq_index = FREQ_INDEX_720P,
> > > @@ -846,6 +850,30 @@ static const char * const imx290_test_pattern_menu[] = {
> > >         "000/555h Toggle Pattern",
> > >  };
> > >
> > > +/* absolute supported control ranges */
> > > +#define HBLANK_MAX     (IMX290_HMAX_MAX - MINIMUM_WIDTH)
> > > +#define VBLANK_MAX     (IMX290_VMAX_MAX - MINIMUM_HEIGHT)

Here too.

> > > +static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
> > > +{
> >
> > This function is never used in this patch. I'm surprised the compiler
> > doesn't throw an error on a static function not being used.
> > You first use it in patch 4 "Introduce initial "off" mode & link freq"
> >
> > > +       const struct imx290_mode *modes = imx290_modes_ptr(imx290);
> > > +       unsigned int min = UINT_MAX;
> > > +       int i;
> > > +
> > > +       for (i = 0; i < imx290_modes_num(imx290); i++) {
> > > +               unsigned int tmp;
> > > +
> > > +               if (v)
> > > +                       tmp = modes[i].hmax_min - modes[i].width;
> >
> > if (v)
> >    return h
> >
> > With the complete series my sensor comes up with controls defined as
> > vertical_blanking 0x009e0901 (int)    : min=280 max=261423 step=1
> > default=280 value=280
> > horizontal_blanking 0x009e0902 (int)    : min=30 max=64255 step=1
> > default=30 value=30
> >
> > Set the mode to 1080p and I get
> > vertical_blanking 0x009e0901 (int)    : min=45 max=261063 step=1
> > default=45 value=1169
> > horizontal_blanking 0x009e0902 (int)    : min=280 max=63615 step=1
> > default=280 value=280
> 
> The idea here is to have VBLANK=30 available in the initial "after
> probe" state of the sensor. VBLANK=30 is a valid value for 720p mode,
> but it cannot be set after probe, because the driver (not the user)
> decided that 1080 mode is active. The idea is to relax the ranges while
> the mode is not set. Once the mode is known, the values are tightened
> to the real mode-dependent values.
> 
> Kind regards
> Benjamin
> 
> >   Dave
> >
> > > +               else
> > > +                       tmp = modes[i].vmax_min - modes[i].height;
> > > +
> > > +               if (tmp < min)
> > > +                       min = tmp;
> > > +       }
> > > +
> > > +       return min;
> > > +}
> > > +
> > >  static void imx290_ctrl_update(struct imx290 *imx290,
> > >                                const struct imx290_mode *mode)
> > >  {

-- 
Regards,

Laurent Pinchart