Re: [PATCH v2 22/23] media: ov5640: Restrict sizes to mbus code

[Jacopo Mondi <jacopo@jmondi.org>]

On Sun, Feb 20, 2022 at 03:16:03PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Feb 10, 2022 at 12:10:43PM +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)
> >
> > Restrict the frame sizes enumerations to the above constraints.
> >
> > While at it, make sure the fse->mbus_code parameter is valid, and return
> > -EINVAL if it's not.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/i2c/ov5640.c | 20 +++++++++++++++++---
> >  1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index f894570ac53c..81bf547a923e 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -3315,14 +3315,28 @@ static int ov5640_enum_frame_size(struct v4l2_subdev *sd,
> >  				  struct v4l2_subdev_state *sd_state,
> >  				  struct v4l2_subdev_frame_size_enum *fse)
> >  {
> > +	u32 bpp = ov5640_code_to_bpp(fse->code);
> > +	unsigned int index = fse->index;
> > +
> >  	if (fse->pad != 0)
> >  		return -EINVAL;
> > -	if (fse->index >= OV5640_NUM_MODES)
> > +	if (!bpp)
> > +		return -EINVAL;
> > +
> > +	/* Only low-resolution modes are supported for 24bpp modes. */
>
> s/24bpp modes/24bpp formats/
>
>
> Do you think this is a limitation of the sensor, or an issue in the
> driver ? In the latter case, I'd write
>
> 	/* FIXME: High resolution modes don't work in 24bpp formats. */

I think it's expected. From register 0x4300 documentation:

YUV444/RGB888 (not available for full resolution)

So I would expect modes obtained by croppin the full pixel array do
not support RGB888. 1280x720 is said to be obtained by sub-sampling
(the ov5640 applies sub-sampling automatically) so it should be
possible to have it in 24bpp. Maybe it needs an higher pixel clock,
something that will be possible once LINK_FREQ is made adjustable

>
> > +	if (bpp == 24 && index >= OV5640_MODE_720P_1280_720)
> > +		return -EINVAL;
> > +
> > +	/* Only high-resolutions modes are supported for 8bpp formats. */
>
> And here
>
> 	/* FIXME: Low resolution modes don't work in 8bpp formats. */

This one puzzles me as well. The only explanation I have is again that
these modes need a -slower- pixel rate to comply with the CSI-2 link
frequency requirements. I can add a todo indeed.

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > +	if (bpp == 8)
> > +		index += OV5640_MODE_720P_1280_720;
> > +
> > +	if (index >= OV5640_NUM_MODES)
> >  		return -EINVAL;
> >
> > -	fse->min_width = ov5640_mode_data[fse->index].crop.width;
> > +	fse->min_width = ov5640_mode_data[index].crop.width;
> >  	fse->max_width = fse->min_width;
> > -	fse->min_height = ov5640_mode_data[fse->index].crop.height;
> > +	fse->min_height = ov5640_mode_data[index].crop.height;
> >  	fse->max_height = fse->min_height;
> >
> >  	return 0;
>
> --
> Regards,
>
> Laurent Pinchart