| Message ID | 20220223104034.91550-27-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov5640: Rework the clock tree programming for MIPI | expand |
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@jmondi.org> > --- > 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. > - 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; Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > } > > static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
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@jmondi.org> > > --- > > 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@ideasonboard.com> Thanks > > > } > > > > static int ov5640_s_stream(struct v4l2_subdev *sd, int enable) > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Wed, Feb 23, 2022 at 03:22:17PM +0100, Jacopo Mondi wrote: > On Wed, Feb 23, 2022 at 02:17:30PM +0200, Laurent Pinchart wrote: > > 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@jmondi.org> > > > --- > > > 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 ? Only when called from userspace, not when called from another subdev. That shouldn't happen though, and the right solution for that is to create wrapper functions for subdev operations that perform sanity checks. I'm fine dropping it here. > > > - 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@ideasonboard.com> > > Thanks > > > > } > > > > > > static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
Hi Laurent, Jacopo, On Wed, Feb 23, 2022 at 04:36:48PM +0200, Laurent Pinchart wrote: > > > > @@ -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 ? > > Only when called from userspace, not when called from another subdev. This was historically true but changed in 2019, as the checks when calling the subdev ops from the kernel and userspace were aligned. This was done by commit a8fa55078a7784a99a2ce389b5d7456a3be9a941 .
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) { + if (formats[i].code == code) + return &formats[i]; + ++i; + } + + 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; - 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; } static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
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@jmondi.org> --- drivers/media/i2c/ov5640.c | 125 +++++++++++++++++++++++++------------ 1 file changed, 86 insertions(+), 39 deletions(-)