On Tue, 3 Sept 2024 at 14:01, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > On Mon, Sep 02, 2024 at 10:55:04PM +0200, Benjamin Bara wrote: > > On Mon, 2 Sept 2024 at 21:58, Laurent Pinchart wrote: > > > On Mon, Sep 02, 2024 at 05:57:29PM +0200, bbara93@xxxxxxxxx wrote: > > > > From: Benjamin Bara <benjamin.bara@xxxxxxxxxxx> > > > > > > > > To be compliant to the V4L2 API, the driver currently "randomly" decides > > > > on one of the two supported modes which also implies a link frequency. > > > > > > > > Add a new mode and frequency which symbolize that the sensor is not in > > > > use. This can be used as a default value during probe() and enables us > > > > to avoid communication with the sensor. > > > > > > I really doin't like this change. I would like to instead move away from > > > modes and make the driver freely configurable. > > > > Which controls do you want to have freely configurable? At least on the > > imx290 the exposure limits depend on the blanking, and the blanking > > limits depend on the format. As the format is defined by the mode on > > imx290, I think this will be quite hard with the current controls. > > I want to make the format freely configurable. Isn't this partly limited by the discussion led by Jacopo in Dublin back in 2022 [1]? How do we shift from the current list of 2 modes, to a single defined mode that the selection API can then crop? Changing the list of enumerated modes is likely to break existing users of this driver, and AIUI it isn't currently permitted for selecting a new enumerated mode to update the selection rectangles. It also needs someone to sit down and fully digest the window cropping mode section of the datasheet, and hope that the sensor actually behaves as documented. I know I don't have the time to do that at present. [1] https://www.spinics.net/lists/linux-media/msg218231.html > > > Furthermore, the concept of an initial unconfigured state isn't valid > > > in V4L2. The driver must fully initialize the whole device state at > > > probe time. > > > > I understand that and it makes sense to me. But given the dependencies > > from above and the fact that the format is currently part of this > > "state", it makes the "freely configurable" intention even harder :-( > > Why can't we simply initialize the controls with limits that correspond > to the default format ? I don't understand what issue this is trying to > solve. If I'm following the full discussion correctly now, it's trying to avoid those couple of register writes during probe due to updating VBLANK and HBLANK control ranges. Adding all this "dummy mode" code seems overkill to achieve that. I see 3 simpler approaches: - Move the pm_runtime calls so that the range updates are when the sensor is powered down. Ack on that needing very careful handling of what is initialised when. - Move the lines state = v4l2_subdev_lock_and_get_active_state(&imx290->sd); imx290_ctrl_update(imx290, imx290->current_mode); v4l2_subdev_unlock_state(state); out of imx290_subdev_init to the end of imx290_probe, after the pm_runtime_put - Add a flag to the state (in addition to the pm_runtime handling) to denote that we're between stream_on and stream_off, and therefore imx290_set_ctrl should update registers. A quick test of the 2nd option (with patches 1, 3, and 6) appears to achieve that aim - I only see the one read from patch 6. IMHO trying to address the case where runtime PM is disabled is fairly redundant, but others may disagree. Option 3 would achieve that with minimal extra overhead though. Dave > > > > Signed-off-by: Benjamin Bara <benjamin.bara@xxxxxxxxxxx> > > > > --- > > > > Changes since v2: > > > > - new > > > > --- > > > > drivers/media/i2c/imx290.c | 29 +++++++++++++++++++++++------ > > > > 1 file changed, 23 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > > > > index 6812e7cb9e23..ece4d66001f5 100644 > > > > --- a/drivers/media/i2c/imx290.c > > > > +++ b/drivers/media/i2c/imx290.c > > > > @@ -425,14 +425,17 @@ static const struct imx290_csi_cfg imx290_csi_297mhz = { > > > > /* supported link frequencies */ > > > > #define FREQ_INDEX_1080P 0 > > > > #define FREQ_INDEX_720P 1 > > > > +#define FREQ_INDEX_OFF 2 > > > > static const s64 imx290_link_freq_2lanes[] = { > > > > [FREQ_INDEX_1080P] = 445500000, > > > > [FREQ_INDEX_720P] = 297000000, > > > > + [FREQ_INDEX_OFF] = 0, > > > > }; > > > > > > > > static const s64 imx290_link_freq_4lanes[] = { > > > > [FREQ_INDEX_1080P] = 222750000, > > > > [FREQ_INDEX_720P] = 148500000, > > > > + [FREQ_INDEX_OFF] = 0, > > > > }; > > > > > > > > /* > > > > @@ -552,6 +555,10 @@ static const struct imx290_mode imx290_modes_4lanes[] = { > > > > }, > > > > }; > > > > > > > > +static const struct imx290_mode imx290_mode_off = { > > > > + .link_freq_index = FREQ_INDEX_OFF, > > > > +}; > > > > + > > > > static inline const struct imx290_mode *imx290_modes_ptr(const struct imx290 *imx290) > > > > { > > > > if (imx290->nlanes == 2) > > > > @@ -876,10 +883,19 @@ static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v) > > > > static void imx290_ctrl_update(struct imx290 *imx290, > > > > const struct imx290_mode *mode) > > > > { > > > > - unsigned int hblank_min = mode->hmax_min - mode->width; > > > > - unsigned int hblank_max = IMX290_HMAX_MAX - mode->width; > > > > - unsigned int vblank_min = mode->vmax_min - mode->height; > > > > - unsigned int vblank_max = IMX290_VMAX_MAX - mode->height; > > > > + unsigned int hblank_min, hblank_max, vblank_min, vblank_max; > > > > + > > > > + if (mode == &imx290_mode_off) { > > > > + hblank_min = imx290_get_blank_min(imx290, false); > > > > + hblank_max = HBLANK_MAX; > > > > + vblank_min = imx290_get_blank_min(imx290, true); > > > > + vblank_max = VBLANK_MAX; > > > > + } else { > > > > + hblank_min = mode->hmax_min - mode->width; > > > > + hblank_max = IMX290_HMAX_MAX - mode->width; > > > > + vblank_min = mode->vmax_min - mode->height; > > > > + vblank_max = IMX290_VMAX_MAX - mode->height; > > > > + } > > > > > > > > __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index); > > > > > > > > @@ -932,7 +948,8 @@ static int imx290_ctrl_init(struct imx290 *imx290) > > > > imx290->link_freq = > > > > v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops, > > > > V4L2_CID_LINK_FREQ, > > > > - imx290_link_freqs_num(imx290) - 1, 0, > > > > + imx290_link_freqs_num(imx290) - 1, > > > > + FREQ_INDEX_OFF, > > > > imx290_link_freqs_ptr(imx290)); > > > > if (imx290->link_freq) > > > > imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > > @@ -1278,7 +1295,7 @@ static int imx290_subdev_init(struct imx290 *imx290) > > > > struct v4l2_subdev_state *state; > > > > int ret; > > > > > > > > - imx290->current_mode = &imx290_modes_ptr(imx290)[0]; > > > > + imx290->current_mode = &imx290_mode_off; > > > > > > > > v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops); > > > > imx290->sd.internal_ops = &imx290_internal_ops; > > > > > > -- > Regards, > > Laurent Pinchart >
