Re: [PATCH 21/21] media: ov5640: Adjust format to bpp in s_fmt

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

On Mon, Feb 07, 2022 at 05:07:07PM +0100, Jacopo Mondi wrote:
> On Thu, Feb 03, 2022 at 01:03:50AM +0200, Laurent Pinchart wrote:
> > 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@jmondi.org>
> > > ---
> > >  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@ideasonboard.com>
> >
> > 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.

That's something I considered too.

> 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...

That's correct, PIXEL_RATE is read-only while LINK_FREQ is read-write.

LINK_FREQ is a menu control because link frequencies need to be selected
with the whole system taken into consideration, to avoid EMC issues. The
possible link frequencies are thus expected to be specified in DT. I'm
not a big fan of this. While the concept make sense, it's confusing and
under-documented. We're especially missing documentation for sensor
driver developers.

> 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.

Sure.

> > > +
> > >  	fmt->width = mode->crop.width;
> > >  	fmt->height = mode->crop.height;
> > >

-- 
Regards,

Laurent Pinchart