Hi Laurent
On Wed, Feb 23, 2022 at 02:17:30PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Feb 23, 2022 at 11:40:33AM +0100, Jacopo Mondi wrote:
> > The format enumeration list is shared between CSI-2 and DVP modes.
> > This lead to the enumeration of unsupported format variants in both
> > modes.
> >
> > Separate the list of DVP and CSI-2 formats and create helpers to access
> > the correct one.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
> > ---
> > drivers/media/i2c/ov5640.c | 125 +++++++++++++++++++++++++------------
> > 1 file changed, 86 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index a709e7f73364..f35006bcea3a 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -188,11 +188,13 @@ enum ov5640_format_mux {
> > OV5640_FMT_MUX_RAW_CIP,
> > };
> >
> > -static const struct ov5640_pixfmt {
> > +struct ov5640_pixfmt {
> > u32 code;
> > u32 colorspace;
> > u8 bpp;
> > -} ov5640_formats[] = {
> > +};
> > +
> > +static const struct ov5640_pixfmt ov5640_dvp_formats[] = {
> > {
> > .code = MEDIA_BUS_FMT_JPEG_1X8,
> > .colorspace = V4L2_COLORSPACE_JPEG,
> > @@ -202,23 +204,48 @@ static const struct ov5640_pixfmt {
> > .colorspace = V4L2_COLORSPACE_SRGB,
> > .bpp = 16,
> > }, {
> > - .code = MEDIA_BUS_FMT_UYVY8_1X16,
> > + .code = MEDIA_BUS_FMT_YUYV8_2X8,
> > .colorspace = V4L2_COLORSPACE_SRGB,
> > .bpp = 16,
> > }, {
> > - .code = MEDIA_BUS_FMT_YUYV8_2X8,
> > + .code = MEDIA_BUS_FMT_RGB565_2X8_LE,
> > .colorspace = V4L2_COLORSPACE_SRGB,
> > .bpp = 16,
> > }, {
> > - .code = MEDIA_BUS_FMT_YUYV8_1X16,
> > + .code = MEDIA_BUS_FMT_RGB565_2X8_BE,
> > .colorspace = V4L2_COLORSPACE_SRGB,
> > .bpp = 16,
> > }, {
> > - .code = MEDIA_BUS_FMT_RGB565_2X8_LE,
> > + .code = MEDIA_BUS_FMT_SBGGR8_1X8,
> > .colorspace = V4L2_COLORSPACE_SRGB,
> > + .bpp = 8,
> > + }, {
> > + .code = MEDIA_BUS_FMT_SGBRG8_1X8,
> > + .colorspace = V4L2_COLORSPACE_SRGB,
> > + .bpp = 8
> > + }, {
> > + .code = MEDIA_BUS_FMT_SGRBG8_1X8,
> > + .colorspace = V4L2_COLORSPACE_SRGB,
> > + .bpp = 8,
> > + }, {
> > + .code = MEDIA_BUS_FMT_SRGGB8_1X8,
> > + .colorspace = V4L2_COLORSPACE_SRGB,
> > + .bpp = 8,
> > + },
> > + { /* sentinel */ }
> > +};
> > +
> > +static const struct ov5640_pixfmt ov5640_csi2_formats[] = {
> > + {
> > + .code = MEDIA_BUS_FMT_JPEG_1X8,
> > + .colorspace = V4L2_COLORSPACE_JPEG,
> > .bpp = 16,
> > }, {
> > - .code = MEDIA_BUS_FMT_RGB565_2X8_BE,
> > + .code = MEDIA_BUS_FMT_UYVY8_1X16,
> > + .colorspace = V4L2_COLORSPACE_SRGB,
> > + .bpp = 16,
> > + }, {
> > + .code = MEDIA_BUS_FMT_YUYV8_1X16,
> > .colorspace = V4L2_COLORSPACE_SRGB,
> > .bpp = 16,
> > }, {
> > @@ -246,20 +273,9 @@ static const struct ov5640_pixfmt {
> > .colorspace = V4L2_COLORSPACE_SRGB,
> > .bpp = 8,
> > },
> > + { /* sentinel */ }
> > };
> >
> > -static u32 ov5640_code_to_bpp(u32 code)
> > -{
> > - unsigned int i;
> > -
> > - for (i = 0; i < ARRAY_SIZE(ov5640_formats); ++i) {
> > - if (ov5640_formats[i].code == code)
> > - return ov5640_formats[i].bpp;
> > - }
> > -
> > - return 0;
> > -}
> > -
> > /*
> > * FIXME: remove this when a subdev API becomes available
> > * to set the MIPI CSI-2 virtual channel.
> > @@ -408,6 +424,33 @@ static inline bool ov5640_is_csi2(const struct ov5640_dev *sensor)
> > return sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY;
> > }
> >
> > +static inline const struct ov5640_pixfmt *ov5640_formats(struct ov5640_dev *sensor)
> > +{
> > + return ov5640_is_csi2(sensor) ? ov5640_csi2_formats : ov5640_dvp_formats;
> > +}
> > +
> > +static const struct ov5640_pixfmt *ov5640_code_to_pixfmt(struct ov5640_dev *sensor,
> > + u32 code)
> > +{
> > + const struct ov5640_pixfmt *formats = ov5640_formats(sensor);
> > + unsigned int i = 0;
> > +
> > + while (formats[i].code) {
>
> for (i = 0; formats[i].code; ++i) {
>
> > + if (formats[i].code == code)
> > + return &formats[i];
> > + ++i;
>
> and drop this one.
>
> > + }
> > +
> > + return &formats[0];
> > +}
> > +
> > +static u32 ov5640_code_to_bpp(struct ov5640_dev *sensor, u32 code)
> > +{
> > + const struct ov5640_pixfmt *format = ov5640_code_to_pixfmt(sensor, code);
> > +
> > + return format->bpp;
> > +}
> > +
> > /*
> > * FIXME: all of these register tables are likely filled with
> > * entries that set the register to their power-on default values,
> > @@ -1391,7 +1434,7 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor)
> > * (0x01=0.5ns).
> > */
> > sample_rate = ov5640_pixel_rates[sensor->current_mode->pixel_rate]
> > - * (ov5640_code_to_bpp(fmt->code) / 8);
> > + * (ov5640_code_to_bpp(sensor, fmt->code) / 8);
> > pclk_period = 2000000000U / sample_rate;
> >
> > /* Program the clock tree registers. */
> > @@ -1456,7 +1499,7 @@ static int ov5640_set_dvp_pclk(struct ov5640_dev *sensor)
> > int ret;
> >
> > rate = ov5640_calc_pixel_rate(sensor);
> > - rate *= ov5640_code_to_bpp(sensor->fmt.code);
> > + rate *= ov5640_code_to_bpp(sensor, sensor->fmt.code);
> > rate /= sensor->ep.bus.parallel.bus_width;
> >
> > ov5640_calc_pclk(sensor, rate, &prediv, &mult, &sysdiv, &pll_rdiv,
> > @@ -2690,15 +2733,18 @@ 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;
> > + const struct ov5640_pixfmt *pixfmt;
> > + unsigned int bpp;
> >
> > mode = ov5640_find_mode(sensor, fr, fmt->width, fmt->height, true);
> > if (!mode)
> > return -EINVAL;
> >
> > + pixfmt = ov5640_code_to_pixfmt(sensor, fmt->code);
> > + bpp = pixfmt->bpp;
> > +
> > /*
> > * Adjust mode according to bpp:
> > * - 8bpp modes work for resolution >= 1280x720
> > @@ -2715,14 +2761,8 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
> > if (new_mode)
> > *new_mode = mode;
> >
> > - for (i = 0; i < ARRAY_SIZE(ov5640_formats); i++)
> > - if (ov5640_formats[i].code == fmt->code)
> > - break;
> > - if (i >= ARRAY_SIZE(ov5640_formats))
> > - i = 0;
> > -
> > - fmt->code = ov5640_formats[i].code;
> > - fmt->colorspace = ov5640_formats[i].colorspace;
> > + fmt->code = pixfmt->code;
> > + fmt->colorspace = pixfmt->colorspace;
> > fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> > fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> > @@ -2764,7 +2804,7 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor)
> > * progressively slow it down if it exceeds 1GHz.
> > */
> > num_lanes = sensor->ep.bus.mipi_csi2.num_data_lanes;
> > - bpp = ov5640_code_to_bpp(fmt->code);
> > + bpp = ov5640_code_to_bpp(sensor, fmt->code);
> > do {
> > pixel_rate = ov5640_pixel_rates[pixel_rate_id];
> > link_freq = pixel_rate * bpp / (2 * num_lanes);
> > @@ -3462,7 +3502,8 @@ 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);
> > + struct ov5640_dev *sensor = to_ov5640_dev(sd);
> > + u32 bpp = ov5640_code_to_bpp(sensor, fse->code);
> > unsigned int index = fse->index;
> >
> > if (fse->pad != 0)
> > @@ -3589,13 +3630,19 @@ static int ov5640_enum_mbus_code(struct v4l2_subdev *sd,
> > struct v4l2_subdev_state *sd_state,
> > struct v4l2_subdev_mbus_code_enum *code)
> > {
> > - if (code->pad != 0)
> > - return -EINVAL;
>
> You've lost this check.
>
The driver has a single pad, doesn't core verify that the pad argument
is valid ?
> > - if (code->index >= ARRAY_SIZE(ov5640_formats))
> > - return -EINVAL;
> > + struct ov5640_dev *sensor = to_ov5640_dev(sd);
> > + const struct ov5640_pixfmt *formats = ov5640_formats(sensor);
> > + unsigned int i = 0;
> >
> > - code->code = ov5640_formats[code->index].code;
> > - return 0;
> > + while (formats[i].code) {
> > + if (i == code->index) {
> > + code->code = formats[i].code;
> > + return 0;
> > + }
> > + ++i;
> > + }
> > +
> > + return -EINVAL;
>
> That's quite inefficient.
>
> struct ov5640_dev *sensor = to_ov5640_dev(sd);
> const struct ov5640_pixfmt *formats;
> unsigned int num_formats;
>
> if (ov5640_is_csi2(sensor)) {
> formats = ov5640_csi2_formats;
> num_formats = ARRAY_SIZE(ov5640_csi2_formats) - 1;
> } else {
> formats = ov5640_dvp_formats;
> num_formats = ARRAY_SIZE(ov5640_dvp_formats) - 1;
> }
>
> if (code->pad != 0 || code->index >= num_formats)
> return -EINVAL;
>
> code->code = formats[code->index].code;
> return 0;
ack
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Thanks
>
> > }
> >
> > static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>
> --
> Regards,
>
> Laurent Pinchart
