lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240906233227.GD12915@pendragon.ideasonboard.com> Date: Sat, 7 Sep 2024 02:32:27 +0300 From: Laurent Pinchart <laurent.pinchart@...asonboard.com> To: Prabhakar <prabhakar.csengg@...il.com> Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, Hans Verkuil <hverkuil-cisco@...all.nl>, linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>, Fabrizio Castro <fabrizio.castro.jz@...esas.com>, Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com> Subject: Re: [PATCH 3/3] media: platform: rzg2l-cru: Add support to capture 8bit raw sRGB Hi Prabhakar, Thank you for the patch. On Fri, Sep 06, 2024 at 06:39:47PM +0100, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com> > > Add support to capture 8bit Bayer formats. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com> > --- > .../platform/renesas/rzg2l-cru/rzg2l-cru.h | 5 ++ > .../platform/renesas/rzg2l-cru/rzg2l-csi2.c | 7 ++- > .../platform/renesas/rzg2l-cru/rzg2l-ip.c | 9 +++- > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 51 ++++++++++++++++--- > 4 files changed, 61 insertions(+), 11 deletions(-) > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > index 174760239548..83c664cb0929 100644 > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > @@ -58,6 +58,11 @@ struct rzg2l_cru_ip { > struct v4l2_subdev *remote; > }; > > +enum rzg2l_cru_fmt { > + RZG2L_YUV = 0, > + RZG2L_RAW_BAYER, > + RZG2L_USER_DEFINED, > +}; > /** > * struct rzg2l_cru_dev - Renesas CRU device structure > * @dev: (OF) device > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c > index a7e4a0c109da..b14c92cd7c18 100644 > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c > @@ -18,6 +18,7 @@ > #include <linux/sys_soc.h> > #include <linux/units.h> > > +#include <media/mipi-csi2.h> > #include <media/v4l2-ctrls.h> > #include <media/v4l2-device.h> > #include <media/v4l2-fwnode.h> > @@ -188,7 +189,11 @@ struct rzg2l_csi2_format { > }; > > static const struct rzg2l_csi2_format rzg2l_csi2_formats[] = { > - { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = 0x1e, .bpp = 16 }, > + { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = MIPI_CSI2_DT_YUV422_8B, .bpp = 16 }, > + { .code = MEDIA_BUS_FMT_SBGGR8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, }, > + { .code = MEDIA_BUS_FMT_SGBRG8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, }, > + { .code = MEDIA_BUS_FMT_SGRBG8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, }, > + { .code = MEDIA_BUS_FMT_SRGGB8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, }, > }; > > static inline struct rzg2l_csi2 *sd_to_csi2(struct v4l2_subdev *sd) > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c > index ac8ebae4ed07..5f60be92ea85 100644 > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c > @@ -6,6 +6,7 @@ > */ > > #include <linux/delay.h> > +#include <media/mipi-csi2.h> I would add a blank line on both sides. > #include "rzg2l-cru.h" > > struct rzg2l_cru_ip_format { > @@ -15,7 +16,11 @@ struct rzg2l_cru_ip_format { > }; > > static const struct rzg2l_cru_ip_format rzg2l_cru_ip_formats[] = { > - { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = 0x1e, .bpp = 16 }, > + { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = MIPI_CSI2_DT_YUV422_8B, .bpp = 16 }, > + { .code = MEDIA_BUS_FMT_SBGGR8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, }, > + { .code = MEDIA_BUS_FMT_SGBRG8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, }, > + { .code = MEDIA_BUS_FMT_SGRBG8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, }, > + { .code = MEDIA_BUS_FMT_SRGGB8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, }, > }; > > enum rzg2l_csi2_pads { > @@ -149,7 +154,7 @@ static int rzg2l_cru_ip_enum_frame_size(struct v4l2_subdev *sd, > if (fse->index != 0) > return -EINVAL; > > - if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16) > + if (!rzg2l_cru_ip_code_to_fmt(fse->code)) > return -EINVAL; > > fse->min_width = RZG2L_CRU_MIN_INPUT_WIDTH; > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > index 6101a070e785..66a0b80e8da7 100644 > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > @@ -15,6 +15,7 @@ > #include <linux/delay.h> > #include <linux/pm_runtime.h> > > +#include <media/mipi-csi2.h> > #include <media/v4l2-ioctl.h> > #include <media/videobuf2-dma-contig.h> > > @@ -78,6 +79,7 @@ > #define ICnMC 0x208 > #define ICnMC_CSCTHR BIT(5) > #define ICnMC_INF_YUV8_422 (0x1e << 16) > +#define ICnMC_INF_RAW8 (0x2a << 16) > #define ICnMC_INF_USER (0x30 << 16) It looks like the INF field contains the MIPI data type. I would replace this with #define ICnMC_INF(x) ((x) << 16) and use it as ICnMC_INF(MIPI_CSI2_DT_RAW8) > #define ICnMC_VCSEL(x) ((x) << 22) > #define ICnMC_INF_MASK GENMASK(21, 16) > @@ -203,6 +205,10 @@ static int rzg2l_cru_mc_validate_format(struct rzg2l_cru_dev *cru, > > switch (fmt.format.code) { > case MEDIA_BUS_FMT_UYVY8_1X16: > + case MEDIA_BUS_FMT_SBGGR8_1X8: > + case MEDIA_BUS_FMT_SGBRG8_1X8: > + case MEDIA_BUS_FMT_SGRBG8_1X8: > + case MEDIA_BUS_FMT_SRGGB8_1X8: > break; > default: > return -EPIPE; > @@ -300,7 +306,7 @@ static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru) > rzg2l_cru_write(cru, AMnAXIATTR, amnaxiattr); > } > > -static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv, > +static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, enum rzg2l_cru_fmt *input_fmt, > struct v4l2_mbus_framefmt *ip_sd_fmt) > { > u32 icnmc; > @@ -308,11 +314,18 @@ static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv, > switch (ip_sd_fmt->code) { > case MEDIA_BUS_FMT_UYVY8_1X16: > icnmc = ICnMC_INF_YUV8_422; > - *input_is_yuv = true; > + *input_fmt = RZG2L_YUV; > + break; > + case MEDIA_BUS_FMT_SBGGR8_1X8: > + case MEDIA_BUS_FMT_SGBRG8_1X8: > + case MEDIA_BUS_FMT_SGRBG8_1X8: > + case MEDIA_BUS_FMT_SRGGB8_1X8: > + icnmc = ICnMC_INF_RAW8; > + *input_fmt = RZG2L_RAW_BAYER; > break; > default: > - *input_is_yuv = false; > icnmc = ICnMC_INF_USER; > + *input_fmt = RZG2L_USER_DEFINED; > break; > } > > @@ -327,17 +340,23 @@ static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv, > static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru, > struct v4l2_mbus_framefmt *ip_sd_fmt) > { > - bool output_is_yuv = false; > - bool input_is_yuv = false; > + enum rzg2l_cru_fmt input_fmt, output_fmt; > u32 icndmr; > > - rzg2l_cru_csi2_setup(cru, &input_is_yuv, ip_sd_fmt); > + rzg2l_cru_csi2_setup(cru, &input_fmt, ip_sd_fmt); > > /* Output format */ > switch (cru->format.pixelformat) { > case V4L2_PIX_FMT_UYVY: > icndmr = ICnDMR_YCMODE_UYVY; > - output_is_yuv = true; > + output_fmt = RZG2L_YUV; > + break; > + case V4L2_PIX_FMT_SBGGR8: > + case V4L2_PIX_FMT_SGBRG8: > + case V4L2_PIX_FMT_SGRBG8: > + case V4L2_PIX_FMT_SRGGB8: > + icndmr = 0; > + output_fmt = RZG2L_RAW_BAYER; > break; > default: > dev_err(cru->dev, "Invalid pixelformat (0x%x)\n", > @@ -346,7 +365,7 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru, > } > > /* If input and output use same colorspace, do bypass mode */ > - if (output_is_yuv == input_is_yuv) > + if (input_fmt == output_fmt) > rzg2l_cru_write(cru, ICnMC, > rzg2l_cru_read(cru, ICnMC) | ICnMC_CSCTHR); > else > @@ -809,6 +828,22 @@ static const struct v4l2_format_info rzg2l_cru_formats[] = { > .format = V4L2_PIX_FMT_UYVY, > .bpp[0] = 2, > }, > + { > + .format = V4L2_PIX_FMT_SBGGR8, > + .bpp[0] = 1, > + }, > + { > + .format = V4L2_PIX_FMT_SGBRG8, > + .bpp[0] = 1, > + }, > + { > + .format = V4L2_PIX_FMT_SGRBG8, > + .bpp[0] = 1, > + }, > + { > + .format = V4L2_PIX_FMT_SRGGB8, > + .bpp[0] = 1, > + }, > }; I think all of this could be simplified if you add the format to the rzg2l_cru_ip_format structure, as well as a rzg2l_cru_fmt member, and used rzg2l_cru_ip_format through the driver. rzg2l_cru_initialize_image_conv() would look up the rzg2l_cru_ip_format entry based on the code, and would pass that to rzg2l_cru_csi2_setup(), which wouldn't have to return an input_fmt. You could even add the icndmr value to the structure too, to avoid a switch/case. > > const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 format) -- Regards, Laurent Pinchart
Powered by blists - more mailing lists