Hi Laurent!
On Mon, 2 Sept 2024 at 22:06, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> On Mon, Sep 02, 2024 at 09:43:36PM +0200, Benjamin Bara wrote:
> > On Mon, 2 Sept 2024 at 20:00, Dave Stevenson
> > <dave.stevenson@xxxxxxxxxxxxxxx> wrote:
> > > On Mon, 2 Sept 2024 at 16:58, <bbara93@xxxxxxxxx> wrote:
> > > >
> > > > From: Benjamin Bara <benjamin.bara@xxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxx>
> > > > ---
> > > > 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.
Thanks for the feedback but I will drop this patch. As we need to decide
on a link frequency, we need to decide on one of the two "all pixel"
modes the imx290 supports and therefore already know the mode-specific,
tight ranges starting from probe time.
Kind regards
Benjamin
> > > > +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
