| Message ID | 20240417120230.4086364-1-niklas.soderlund+renesas@ragnatech.se |
|---|---|
| State | Accepted |
| Commit | 1b7e7240eaf39aeebed08e09d0aae86f5f207286 |
| Headers | show |
| Series | media: rcar-vin: Add support for RAW10 | expand |
Hi Geert, On Wed, Apr 17, 2024 at 03:34:36PM +0200, Geert Uytterhoeven wrote: > On Wed, Apr 17, 2024 at 2:06 PM Niklas Söderlund wrote: > > Some R-Car SoCs are capable of capturing RAW10. Add support for it > > using the V4L2_PIX_FMT_Y10 pixel format, which I think is the correct > > format to express RAW10 unpacked to users. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > Thanks for your patch! > > I am no VIN or V4L2 expert, but the register bits LGTM, so > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > @@ -780,6 +782,9 @@ static int rvin_setup(struct rvin_dev *vin) > > case MEDIA_BUS_FMT_Y8_1X8: > > vnmc |= VNMC_INF_RAW8; > > break; > > + case MEDIA_BUS_FMT_Y10_1X10: > > + vnmc |= VNMC_INF_RGB666; > > The actual meaning of this bit is not uniform across all SoCs. > On R-Car V3U it means (partial) 16 bpp, on R-Car Gen3 it means 18 bpp. The INF bits have different meanings depending on the VIN input. What you refer to above for V3U is for the CSI-2 input, while for the rest of Gen3 you quote the values for the parallel input. Value 111 is documented as "prohibit" for the CSI-2 input on the rest of Gen3. > > + break; > > default: > > break; > > }
On Tue, Jun 18, 2024 at 05:41:03PM +0300, Laurent Pinchart wrote: > Hi Geert, > > On Wed, Apr 17, 2024 at 03:34:36PM +0200, Geert Uytterhoeven wrote: > > On Wed, Apr 17, 2024 at 2:06 PM Niklas Söderlund wrote: > > > Some R-Car SoCs are capable of capturing RAW10. Add support for it > > > using the V4L2_PIX_FMT_Y10 pixel format, which I think is the correct > > > format to express RAW10 unpacked to users. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > Thanks for your patch! > > > > I am no VIN or V4L2 expert, but the register bits LGTM, so > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > > @@ -780,6 +782,9 @@ static int rvin_setup(struct rvin_dev *vin) > > > case MEDIA_BUS_FMT_Y8_1X8: > > > vnmc |= VNMC_INF_RAW8; > > > break; > > > + case MEDIA_BUS_FMT_Y10_1X10: > > > + vnmc |= VNMC_INF_RGB666; > > > > The actual meaning of this bit is not uniform across all SoCs. > > On R-Car V3U it means (partial) 16 bpp, on R-Car Gen3 it means 18 bpp. > > The INF bits have different meanings depending on the VIN input. What > you refer to above for V3U is for the CSI-2 input, while for the rest of > Gen3 you quote the values for the parallel input. Value 111 is > documented as "prohibit" for the CSI-2 input on the rest of Gen3. To be precise, for V3U the documentation indicates "Input from Channel Selector", not CSI-2. V3U has no parallel input. The macros for the INF bits mix names for different types of inputs, it could be a good idea to clean this up. > > > + break; > > > default: > > > break; > > > }
Hi Niklas, Thank you for the patch. On Wed, Apr 17, 2024 at 02:02:30PM +0200, Niklas Söderlund wrote: > Some R-Car SoCs are capable of capturing RAW10. Add support for it > using the V4L2_PIX_FMT_Y10 pixel format, which I think is the correct > format to express RAW10 unpacked to users. MEDIA_BUS_FMT_Y10_1X10 and V4L2_PIX_FMT_Y10 are correct if the input geenrates monochrome data. If the source produces bayer data, you need a bayer pixel format and media bus code. The bayer media bus codes should be easy to handle (you just need to add them to the switch/case in rvin_setup()). The pixel formats are a bit more annoying, as you'll need to update rvin_mc_validate_format() and rvin_enum_fmt_vid_cap(). I don't like how V4L2 handles bayer formats, and that's something that is on Sakari's and my radar to fix, probably sooner than later, by introducing RAWx media bus codes and pixel formats. > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > * Changes since RFC > - Fix spelling in rcar-vin.h > --- > drivers/media/platform/renesas/rcar-vin/rcar-core.c | 1 + > drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 12 ++++++++++++ > drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c | 8 ++++++++ > drivers/media/platform/renesas/rcar-vin/rcar-vin.h | 4 +++- > 4 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c > index 809c3a38cc4a..e9675cb8faa2 100644 > --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c > @@ -1279,6 +1279,7 @@ static const struct rvin_info rcar_info_r8a779a0 = { > .use_mc = true, > .use_isp = true, > .nv12 = true, > + .raw10 = true, > .max_width = 4096, > .max_height = 4096, > }; > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > index e2c40abc6d3d..dd290054dfe7 100644 > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > @@ -123,7 +123,9 @@ > /* Video n Data Mode Register bits */ > #define VNDMR_A8BIT(n) (((n) & 0xff) << 24) > #define VNDMR_A8BIT_MASK (0xff << 24) > +#define VNDMR_RMODE_RAW10 (2 << 19) > #define VNDMR_YMODE_Y8 (1 << 12) > +#define VNDMR_YC_THR (1 << 11) > #define VNDMR_EXRGB (1 << 8) > #define VNDMR_BPSM (1 << 4) > #define VNDMR_ABIT (1 << 2) > @@ -780,6 +782,9 @@ static int rvin_setup(struct rvin_dev *vin) > case MEDIA_BUS_FMT_Y8_1X8: > vnmc |= VNMC_INF_RAW8; > break; > + case MEDIA_BUS_FMT_Y10_1X10: > + vnmc |= VNMC_INF_RGB666; > + break; > default: > break; > } > @@ -888,6 +893,9 @@ static int rvin_setup(struct rvin_dev *vin) > dmr = 0; > } > break; > + case V4L2_PIX_FMT_Y10: > + dmr = VNDMR_RMODE_RAW10 | VNDMR_YC_THR; > + break; > default: > vin_err(vin, "Invalid pixelformat (0x%x)\n", > vin->format.pixelformat); > @@ -1270,6 +1278,10 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd, > if (vin->format.pixelformat != V4L2_PIX_FMT_GREY) > return -EPIPE; > break; > + case MEDIA_BUS_FMT_Y10_1X10: > + if (vin->format.pixelformat != V4L2_PIX_FMT_Y10) > + return -EPIPE; > + break; > default: > return -EPIPE; > } > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c > index bb4b07bed28d..e7298688541d 100644 > --- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c > @@ -86,6 +86,10 @@ static const struct rvin_video_format rvin_formats[] = { > .fourcc = V4L2_PIX_FMT_GREY, > .bpp = 1, > }, > + { > + .fourcc = V4L2_PIX_FMT_Y10, > + .bpp = 4, Isn't Y10 stored in 2 bytes per pixel, not 4 ? > + }, > }; > > const struct rvin_video_format *rvin_format_from_pixel(struct rvin_dev *vin, > @@ -106,6 +110,10 @@ const struct rvin_video_format *rvin_format_from_pixel(struct rvin_dev *vin, > if (!vin->info->nv12 || !(BIT(vin->id) & 0x3333)) > return NULL; > break; > + case V4L2_PIX_FMT_Y10: > + if (!vin->info->raw10) > + return NULL; > + break; > default: > break; > } You need to update rvin_enum_fmt_vid_cap() too. > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h > index 997a66318a29..f87d4bc9e53e 100644 > --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h > @@ -151,7 +151,8 @@ struct rvin_group_route { > * @model: VIN model > * @use_mc: use media controller instead of controlling subdevice > * @use_isp: the VIN is connected to the ISP and not to the CSI-2 > - * @nv12: support outputing NV12 pixel format > + * @nv12: support outputting NV12 pixel format > + * @raw10: support outputting RAW10 pixel format > * @max_width: max input width the VIN supports > * @max_height: max input height the VIN supports > * @routes: list of possible routes from the CSI-2 recivers to > @@ -163,6 +164,7 @@ struct rvin_info { > bool use_mc; > bool use_isp; > bool nv12; > + bool raw10; > > unsigned int max_width; > unsigned int max_height;
Hi Laurent, Thanks for your review. On 2024-06-18 17:51:13 +0300, Laurent Pinchart wrote: > On Tue, Jun 18, 2024 at 05:41:03PM +0300, Laurent Pinchart wrote: > > Hi Geert, > > > > On Wed, Apr 17, 2024 at 03:34:36PM +0200, Geert Uytterhoeven wrote: > > > On Wed, Apr 17, 2024 at 2:06 PM Niklas Söderlund wrote: > > > > Some R-Car SoCs are capable of capturing RAW10. Add support for it > > > > using the V4L2_PIX_FMT_Y10 pixel format, which I think is the correct > > > > format to express RAW10 unpacked to users. > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > > > Thanks for your patch! > > > > > > I am no VIN or V4L2 expert, but the register bits LGTM, so > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > > > @@ -780,6 +782,9 @@ static int rvin_setup(struct rvin_dev *vin) > > > > case MEDIA_BUS_FMT_Y8_1X8: > > > > vnmc |= VNMC_INF_RAW8; > > > > break; > > > > + case MEDIA_BUS_FMT_Y10_1X10: > > > > + vnmc |= VNMC_INF_RGB666; > > > > > > The actual meaning of this bit is not uniform across all SoCs. > > > On R-Car V3U it means (partial) 16 bpp, on R-Car Gen3 it means 18 bpp. > > > > The INF bits have different meanings depending on the VIN input. What > > you refer to above for V3U is for the CSI-2 input, while for the rest of > > Gen3 you quote the values for the parallel input. Value 111 is > > documented as "prohibit" for the CSI-2 input on the rest of Gen3. > > To be precise, for V3U the documentation indicates "Input from Channel > Selector", not CSI-2. V3U has no parallel input. Yes it's getting a tad complex, but there is no issue here is there? This patch extends struct struct rvin_info with a new raw10 bool which indicates if raw10 is supported, or not. If it's not supported the driver rejects the MEDIA_BUS_FMT_Y10_1X10 in format validation. > > The macros for the INF bits mix names for different types of inputs, it > could be a good idea to clean this up. There are so many things in this driver I would like to clean up and are working on. The first step is to clean up the async and VIN group mess, there are patches for that on the list. Once that is done I'm planing to refactor the init functions and defines, one per generation in different files to make it more clear how things look on the different generations. > > > > > + break; > > > > default: > > > > break; > > > > } > > -- > Regards, > > Laurent Pinchart
On Tue, Jun 18, 2024 at 05:01:14PM +0200, Niklas Söderlund wrote: > Hi Laurent, > > Thanks for your review. > > On 2024-06-18 17:51:13 +0300, Laurent Pinchart wrote: > > On Tue, Jun 18, 2024 at 05:41:03PM +0300, Laurent Pinchart wrote: > > > Hi Geert, > > > > > > On Wed, Apr 17, 2024 at 03:34:36PM +0200, Geert Uytterhoeven wrote: > > > > On Wed, Apr 17, 2024 at 2:06 PM Niklas Söderlund wrote: > > > > > Some R-Car SoCs are capable of capturing RAW10. Add support for it > > > > > using the V4L2_PIX_FMT_Y10 pixel format, which I think is the correct > > > > > format to express RAW10 unpacked to users. > > > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > > > > > Thanks for your patch! > > > > > > > > I am no VIN or V4L2 expert, but the register bits LGTM, so > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > > > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > > > > @@ -780,6 +782,9 @@ static int rvin_setup(struct rvin_dev *vin) > > > > > case MEDIA_BUS_FMT_Y8_1X8: > > > > > vnmc |= VNMC_INF_RAW8; > > > > > break; > > > > > + case MEDIA_BUS_FMT_Y10_1X10: > > > > > + vnmc |= VNMC_INF_RGB666; > > > > > > > > The actual meaning of this bit is not uniform across all SoCs. > > > > On R-Car V3U it means (partial) 16 bpp, on R-Car Gen3 it means 18 bpp. > > > > > > The INF bits have different meanings depending on the VIN input. What > > > you refer to above for V3U is for the CSI-2 input, while for the rest of > > > Gen3 you quote the values for the parallel input. Value 111 is > > > documented as "prohibit" for the CSI-2 input on the rest of Gen3. > > > > To be precise, for V3U the documentation indicates "Input from Channel > > Selector", not CSI-2. V3U has no parallel input. > > Yes it's getting a tad complex, but there is no issue here is there? > This patch extends struct struct rvin_info with a new raw10 bool which > indicates if raw10 is supported, or not. If it's not supported the > driver rejects the MEDIA_BUS_FMT_Y10_1X10 in format validation. Apart from the naming causing some confusion, I don't see any issue. Functionally this part of the patch seems correct. > > The macros for the INF bits mix names for different types of inputs, it > > could be a good idea to clean this up. > > There are so many things in this driver I would like to clean up and are > working on. The first step is to clean up the async and VIN group mess, > there are patches for that on the list. Once that is done I'm planing to > refactor the init functions and defines, one per generation in different > files to make it more clear how things look on the different generations. I'm looking forward to that :-) > > > > > + break; > > > > > default: > > > > > break; > > > > > }
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c index 809c3a38cc4a..e9675cb8faa2 100644 --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c @@ -1279,6 +1279,7 @@ static const struct rvin_info rcar_info_r8a779a0 = { .use_mc = true, .use_isp = true, .nv12 = true, + .raw10 = true, .max_width = 4096, .max_height = 4096, }; diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c index e2c40abc6d3d..dd290054dfe7 100644 --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c @@ -123,7 +123,9 @@ /* Video n Data Mode Register bits */ #define VNDMR_A8BIT(n) (((n) & 0xff) << 24) #define VNDMR_A8BIT_MASK (0xff << 24) +#define VNDMR_RMODE_RAW10 (2 << 19) #define VNDMR_YMODE_Y8 (1 << 12) +#define VNDMR_YC_THR (1 << 11) #define VNDMR_EXRGB (1 << 8) #define VNDMR_BPSM (1 << 4) #define VNDMR_ABIT (1 << 2) @@ -780,6 +782,9 @@ static int rvin_setup(struct rvin_dev *vin) case MEDIA_BUS_FMT_Y8_1X8: vnmc |= VNMC_INF_RAW8; break; + case MEDIA_BUS_FMT_Y10_1X10: + vnmc |= VNMC_INF_RGB666; + break; default: break; } @@ -888,6 +893,9 @@ static int rvin_setup(struct rvin_dev *vin) dmr = 0; } break; + case V4L2_PIX_FMT_Y10: + dmr = VNDMR_RMODE_RAW10 | VNDMR_YC_THR; + break; default: vin_err(vin, "Invalid pixelformat (0x%x)\n", vin->format.pixelformat); @@ -1270,6 +1278,10 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd, if (vin->format.pixelformat != V4L2_PIX_FMT_GREY) return -EPIPE; break; + case MEDIA_BUS_FMT_Y10_1X10: + if (vin->format.pixelformat != V4L2_PIX_FMT_Y10) + return -EPIPE; + break; default: return -EPIPE; } diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c index bb4b07bed28d..e7298688541d 100644 --- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c +++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c @@ -86,6 +86,10 @@ static const struct rvin_video_format rvin_formats[] = { .fourcc = V4L2_PIX_FMT_GREY, .bpp = 1, }, + { + .fourcc = V4L2_PIX_FMT_Y10, + .bpp = 4, + }, }; const struct rvin_video_format *rvin_format_from_pixel(struct rvin_dev *vin, @@ -106,6 +110,10 @@ const struct rvin_video_format *rvin_format_from_pixel(struct rvin_dev *vin, if (!vin->info->nv12 || !(BIT(vin->id) & 0x3333)) return NULL; break; + case V4L2_PIX_FMT_Y10: + if (!vin->info->raw10) + return NULL; + break; default: break; } diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h index 997a66318a29..f87d4bc9e53e 100644 --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h @@ -151,7 +151,8 @@ struct rvin_group_route { * @model: VIN model * @use_mc: use media controller instead of controlling subdevice * @use_isp: the VIN is connected to the ISP and not to the CSI-2 - * @nv12: support outputing NV12 pixel format + * @nv12: support outputting NV12 pixel format + * @raw10: support outputting RAW10 pixel format * @max_width: max input width the VIN supports * @max_height: max input height the VIN supports * @routes: list of possible routes from the CSI-2 recivers to @@ -163,6 +164,7 @@ struct rvin_info { bool use_mc; bool use_isp; bool nv12; + bool raw10; unsigned int max_width; unsigned int max_height;
Some R-Car SoCs are capable of capturing RAW10. Add support for it using the V4L2_PIX_FMT_Y10 pixel format, which I think is the correct format to express RAW10 unpacked to users. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- * Changes since RFC - Fix spelling in rcar-vin.h --- drivers/media/platform/renesas/rcar-vin/rcar-core.c | 1 + drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 12 ++++++++++++ drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c | 8 ++++++++ drivers/media/platform/renesas/rcar-vin/rcar-vin.h | 4 +++- 4 files changed, 24 insertions(+), 1 deletion(-)