Hi Laurent
On Thu, Feb 03, 2022 at 01:03:50AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Jan 31, 2022 at 03:45:56PM +0100, Jacopo Mondi wrote:
> > The ov5640 driver supports different sizes for different mbus_codes.
> > In particular:
> >
> > - 8bpp modes: high resolution sizes (>= 1280x720)
> > - 16bpp modes: all sizes
> > - 24bpp modes: low resolutions sizes (< 1280x720)
> >
> > Adjust the image sizes according to the above constraints.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
> > ---
> > drivers/media/i2c/ov5640.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 2978dabd1d54..49d0df80f71a 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -2529,6 +2529,7 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
> > enum ov5640_frame_rate fr,
> > const struct ov5640_mode_info **new_mode)
> > {
> > + unsigned int bpp = ov5640_code_to_bpp(fmt->code);
> > struct ov5640_dev *sensor = to_ov5640_dev(sd);
> > const struct ov5640_mode_info *mode;
> > int i;
> > @@ -2536,6 +2537,17 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
> > mode = ov5640_find_mode(sensor, fr, fmt->width, fmt->height, true);
> > if (!mode)
> > return -EINVAL;
> > +
> > + /*
> > + * Adjust mode according to bpp:
> > + * - 8bpp modes work for resolution >= 1280x720
> > + * - 24bpp modes work resolution < 1280x720
> > + */
> > + if (bpp == 8 && mode->crop.width < 1280)
> > + mode = &ov5640_mode_data[OV5640_MODE_720P_1280_720];
> > + else if (bpp == 24 && mode->crop.width > 1024)
> > + mode = &ov5640_mode_data[OV5640_MODE_XGA_1024_768];
>
> This is in line with patch 20/21, so
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> but I'm still not sure about the limitation for 8bpp.
>
Me neither. I had on my todo to test 8bpp with different pixel clock
rates to see if it makes any difference. If only 1080p and full res
were working I would have guessed it was because those are the only
modes obtained by cropping the full pixel array without any
subsampling being applied, but as 1280x720 works too....
One thinking I had was that the pixel_rate assigned to each mode where
too slow for an 8bpp mode and that the resulting CSI-2 frequencies
were consequentially below the minimum required ones, but I would have
to re-calculate to see if that's the case.
Anyway, as said in the cover letter, I have now assigned a pixel_rate
to each mode, regardless of the image format. As the same 'mode' in
24bpp or 8bpp requires different pixel rates, I think this first
approach is good, but to fully exploit all the modes the pixel_rate
should be controlled by userspace too, in function of the bpp (and the
number of data lanes). My understanding is that it should happen not
by changing PIXEL_RATE but rather LINK_FREQ. But with the latter being
a menu control (something I still don't get the reason for) there's a
limited number of combination that could be supported there...
We'll see, I would have been happy enough if 16bpp gets actually
fixed when it comes to frame durations, with 8bpp/24bpp being usable.
We can improve on top of an hopefully now more solid base.
> > +
> > fmt->width = mode->crop.width;
> > fmt->height = mode->crop.height;
> >
>
> --
> Regards,
>
> Laurent Pinchart
