Hi Dave!
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
> > +#define MINIMUM_WIDTH WIDTH_720P
> > +#define MINIMUM_HEIGHT HEIGHT_720P
> > 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)
> > +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"
I think I couldn't really transport my intention behind this function
and going to try to explain my thought process a little bit:
My basic understanding of sensors and blanking times is that the sensors
cannot "just forward image data" but also need some time for internal
stuff. My expectation would be that this time should be the same for two
different modes (720p and 1080p) of the same type ("all pixel readout").
I would even expect that in this type of mode, most of the "internal
time" is used for transferring data, therefore I would also expect that
the minimum blanking times for 720p to be lower (or at least equal) than
1080p. This would make my intention quite easy: Just activate the lowest
resolution during probe, because it has the widest allowed blanking time
ranges, and then we can "freely configure", independent of the mode
later used.
However, this is not the case here: The minimum VBLANK of the 1080p mode
is lower than the minimum VBLANK of the 720p mode. Together with
HBLANK_min_720p < HBLANK_min_1080p, we need a "virtual, invalid" mode to
support both minimas.
What would make things simpler here would be allowing us to control the
total times instead of the blanking times. In this case, we can directly
use the minimum defined {H,V}MAX of the mode and default to the mode
with the lowest possible resolution instead. This would enable a freely
configurable frame duration with full supported ranges and valid modes.
Kind regards
Benjamin
> > + 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
>
> 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)
> > {
> >
> > --
> > 2.46.0
> >
> >
