* [PATCH v2 0/3] media: vimc: Add support for {RGB,BGR,GBR}888 bus formats on debayer source pad
@ 2020-03-26 21:47 Nícolas F. R. A. Prado
2020-03-26 21:47 ` [PATCH v2 1/3] media: vimc: Support multiple media bus codes for each pixelformat Nícolas F. R. A. Prado
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Nícolas F. R. A. Prado @ 2020-03-26 21:47 UTC (permalink / raw)
To: linux-media
Cc: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, Hans Verkuil,
linux-kernel, lkcamp
This patch series adds support for the missing RGB888_*, BGR888_* and GBR888_*
media bus formats on the source pad of debayer subdevices of the vimc driver.
These additional bus formats enable a wider range of formats to be configured
on the topologies, making it possible to test more real use cases.
It also enables video to be streamed in the BGR pixelformat on /dev/video3.
The first patch makes it possible to have multiple media bus codes mapping to
the same pixelformat, so that, for example, all RGB888_* media bus formats use
the same RGB24 pixelformat.
The second patch maps the missing RGB888_*, BGR888_* and GBR888_* media bus
codes to the RGB24 and BGR24 pixelformats.
Since there isn't a GBR24 pixelformat, the GBR888_1X24 bus code maps to the
RGB24 pixelformat.
The third patch enables the source pad of the debayer subdevice to use the
added media bus formats.
This patch series passed all tests of v4l2-compliance:
$ compliance_git -m /dev/media0
v4l2-compliance SHA: 81e45d957c4db39397f893100b3d2729ef39b052, 64 bits, 64-bit time_t
Grand Total for vimc device /dev/media0: 461, Succeeded: 461, Failed: 0, Warnings: 0
As a side note, when listing the pads containing the new formats added, I
noticed that MEDIA_BUS_FMT_RGB888_3X8 doesn't have its name displayed by
v4l2-ctl, but from my understanding that should be a bug in v4l-utils.
$ v4l2-ctl -d /dev/v4l-subdev2 --list-subdev-mbus-codes 1
ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=1)
0x1014: MEDIA_BUS_FMT_GBR888_1X24
0x1013: MEDIA_BUS_FMT_BGR888_1X24
0x101b: MEDIA_BUS_FMT_BGR888_3X8
0x100a: MEDIA_BUS_FMT_RGB888_1X24
0x100b: MEDIA_BUS_FMT_RGB888_2X12_BE
0x100c: MEDIA_BUS_FMT_RGB888_2X12_LE
0x101c
0x1011: MEDIA_BUS_FMT_RGB888_1X7X4_SPWG
0x1012: MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA
0x100f: MEDIA_BUS_FMT_RGB888_1X32_PADHI
Changes in v2:
- Fix vimc_mbus_code_by_index not checking code array bounds
- Change commit messages to reflect v2 changes
- Suggested by Helen:
- Rename variables
- Fix array formatting
- Change code array size
- Add comment about vimc_mbus_code_by_index return value
- Add vimc_deb_is_src_code_valid function
- Add other BGR888 and RGB888 formats to BGR24 and RGB24 pixelformats
- Add other BGR888 and RGB888 formats to debayer source pad supported
formats
- Suggested by Ezequiel:
- Change cover letter to better explain this patch series
You can find v1 here: https://patchwork.linuxtv.org/cover/61391/
Nícolas F. R. A. Prado (3):
media: vimc: Support multiple media bus codes for each pixelformat
media: vimc: Add missing {RGB,BGR,GBR}888 media bus codes
media: vimc: deb: Add support for {RGB,BGR,GBR}888 bus formats on
source pad
drivers/media/platform/vimc/vimc-common.c | 82 +++++++++++++++-------
drivers/media/platform/vimc/vimc-common.h | 11 ++-
drivers/media/platform/vimc/vimc-debayer.c | 61 ++++++++++++----
drivers/media/platform/vimc/vimc-scaler.c | 10 ++-
drivers/media/platform/vimc/vimc-sensor.c | 6 +-
5 files changed, 126 insertions(+), 44 deletions(-)
--
2.25.2
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v2 1/3] media: vimc: Support multiple media bus codes for each pixelformat 2020-03-26 21:47 [PATCH v2 0/3] media: vimc: Add support for {RGB,BGR,GBR}888 bus formats on debayer source pad Nícolas F. R. A. Prado @ 2020-03-26 21:47 ` Nícolas F. R. A. Prado 2020-03-30 19:36 ` Helen Koike 2020-03-26 21:47 ` [PATCH v2 2/3] media: vimc: Add missing {RGB,BGR,GBR}888 media bus codes Nícolas F. R. A. Prado 2020-03-26 21:47 ` [PATCH v2 3/3] media: vimc: deb: Add support for {RGB,BGR,GBR}888 bus formats on source pad Nícolas F. R. A. Prado 2 siblings, 1 reply; 15+ messages in thread From: Nícolas F. R. A. Prado @ 2020-03-26 21:47 UTC (permalink / raw) To: linux-media Cc: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, lkcamp Change vimc_pix_map_list to allow multiple media bus codes to map to the same pixelformat, making it possible to add media bus codes for which there are no pixelformat. Signed-off-by: Nícolas F. R. A. Prado <nfraprado@protonmail.com> --- Changes in v2: - Fix vimc_mbus_code_by_index not checking code array bounds - Fix array formatting - Rename variables - Change code array size - Add comment about vimc_mbus_code_by_index return value drivers/media/platform/vimc/vimc-common.c | 70 ++++++++++++++--------- drivers/media/platform/vimc/vimc-common.h | 11 +++- drivers/media/platform/vimc/vimc-scaler.c | 10 +++- drivers/media/platform/vimc/vimc-sensor.c | 6 +- 4 files changed, 65 insertions(+), 32 deletions(-) diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c index c95c17c048f2..119846f3eaa5 100644 --- a/drivers/media/platform/vimc/vimc-common.c +++ b/drivers/media/platform/vimc/vimc-common.c @@ -19,19 +19,19 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { /* RGB formats */ { - .code = MEDIA_BUS_FMT_BGR888_1X24, + .code = { MEDIA_BUS_FMT_BGR888_1X24 }, .pixelformat = V4L2_PIX_FMT_BGR24, .bpp = 3, .bayer = false, }, { - .code = MEDIA_BUS_FMT_RGB888_1X24, + .code = { MEDIA_BUS_FMT_RGB888_1X24 }, .pixelformat = V4L2_PIX_FMT_RGB24, .bpp = 3, .bayer = false, }, { - .code = MEDIA_BUS_FMT_ARGB8888_1X32, + .code = { MEDIA_BUS_FMT_ARGB8888_1X32 }, .pixelformat = V4L2_PIX_FMT_ARGB32, .bpp = 4, .bayer = false, @@ -39,49 +39,49 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { /* Bayer formats */ { - .code = MEDIA_BUS_FMT_SBGGR8_1X8, + .code = { MEDIA_BUS_FMT_SBGGR8_1X8 }, .pixelformat = V4L2_PIX_FMT_SBGGR8, .bpp = 1, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SGBRG8_1X8, + .code = { MEDIA_BUS_FMT_SGBRG8_1X8 }, .pixelformat = V4L2_PIX_FMT_SGBRG8, .bpp = 1, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SGRBG8_1X8, + .code = { MEDIA_BUS_FMT_SGRBG8_1X8 }, .pixelformat = V4L2_PIX_FMT_SGRBG8, .bpp = 1, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SRGGB8_1X8, + .code = { MEDIA_BUS_FMT_SRGGB8_1X8 }, .pixelformat = V4L2_PIX_FMT_SRGGB8, .bpp = 1, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SBGGR10_1X10, + .code = { MEDIA_BUS_FMT_SBGGR10_1X10 }, .pixelformat = V4L2_PIX_FMT_SBGGR10, .bpp = 2, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SGBRG10_1X10, + .code = { MEDIA_BUS_FMT_SGBRG10_1X10 }, .pixelformat = V4L2_PIX_FMT_SGBRG10, .bpp = 2, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SGRBG10_1X10, + .code = { MEDIA_BUS_FMT_SGRBG10_1X10 }, .pixelformat = V4L2_PIX_FMT_SGRBG10, .bpp = 2, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SRGGB10_1X10, + .code = { MEDIA_BUS_FMT_SRGGB10_1X10 }, .pixelformat = V4L2_PIX_FMT_SRGGB10, .bpp = 2, .bayer = true, @@ -89,25 +89,25 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { /* 10bit raw bayer a-law compressed to 8 bits */ { - .code = MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8, + .code = { MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8 }, .pixelformat = V4L2_PIX_FMT_SBGGR10ALAW8, .bpp = 1, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8, + .code = { MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8 }, .pixelformat = V4L2_PIX_FMT_SGBRG10ALAW8, .bpp = 1, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8, + .code = { MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8 }, .pixelformat = V4L2_PIX_FMT_SGRBG10ALAW8, .bpp = 1, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8, + .code = { MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8 }, .pixelformat = V4L2_PIX_FMT_SRGGB10ALAW8, .bpp = 1, .bayer = true, @@ -115,49 +115,49 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { /* 10bit raw bayer DPCM compressed to 8 bits */ { - .code = MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8, + .code = { MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8 }, .pixelformat = V4L2_PIX_FMT_SBGGR10DPCM8, .bpp = 1, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8, + .code = { MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8 }, .pixelformat = V4L2_PIX_FMT_SGBRG10DPCM8, .bpp = 1, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8, + .code = { MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8 }, .pixelformat = V4L2_PIX_FMT_SGRBG10DPCM8, .bpp = 1, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8, + .code = { MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8 }, .pixelformat = V4L2_PIX_FMT_SRGGB10DPCM8, .bpp = 1, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SBGGR12_1X12, + .code = { MEDIA_BUS_FMT_SBGGR12_1X12 }, .pixelformat = V4L2_PIX_FMT_SBGGR12, .bpp = 2, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SGBRG12_1X12, + .code = { MEDIA_BUS_FMT_SGBRG12_1X12 }, .pixelformat = V4L2_PIX_FMT_SGBRG12, .bpp = 2, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SGRBG12_1X12, + .code = { MEDIA_BUS_FMT_SGRBG12_1X12 }, .pixelformat = V4L2_PIX_FMT_SGRBG12, .bpp = 2, .bayer = true, }, { - .code = MEDIA_BUS_FMT_SRGGB12_1X12, + .code = { MEDIA_BUS_FMT_SRGGB12_1X12 }, .pixelformat = V4L2_PIX_FMT_SRGGB12, .bpp = 2, .bayer = true, @@ -182,13 +182,31 @@ const struct vimc_pix_map *vimc_pix_map_by_index(unsigned int i) return &vimc_pix_map_list[i]; } +const u32 vimc_mbus_code_by_index(unsigned int index) +{ + unsigned int i, j; + + for (i = 0; i < ARRAY_SIZE(vimc_pix_map_list); i++) { + for (j = 0; j < ARRAY_SIZE(vimc_pix_map_list[i].code); j++) { + if (vimc_pix_map_list[i].code[j]) { + if (!index) + return vimc_pix_map_list[i].code[j]; + index--; + } + } + } + return 0; +} + const struct vimc_pix_map *vimc_pix_map_by_code(u32 code) { - unsigned int i; + unsigned int i, j; for (i = 0; i < ARRAY_SIZE(vimc_pix_map_list); i++) { - if (vimc_pix_map_list[i].code == code) - return &vimc_pix_map_list[i]; + for (j = 0; j < ARRAY_SIZE(vimc_pix_map_list[i].code); j++) { + if (vimc_pix_map_list[i].code[j] == code) + return &vimc_pix_map_list[i]; + } } return NULL; } diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h index 616d5a6b0754..585441694c86 100644 --- a/drivers/media/platform/vimc/vimc-common.h +++ b/drivers/media/platform/vimc/vimc-common.h @@ -69,7 +69,7 @@ do { \ * V4L2_PIX_FMT_* fourcc pixelformat and its bytes per pixel (bpp) */ struct vimc_pix_map { - unsigned int code; + unsigned int code[1]; unsigned int bpp; u32 pixelformat; bool bayer; @@ -172,6 +172,15 @@ void vimc_sen_release(struct vimc_ent_device *ved); */ const struct vimc_pix_map *vimc_pix_map_by_index(unsigned int i); +/** + * vimc_mbus_code_by_index - get mbus code by its index + * + * @index: index of the mbus code in vimc_pix_map_list + * + * Returns 0 if no mbus code is found for the given index. + */ +const u32 vimc_mbus_code_by_index(unsigned int index); + /** * vimc_pix_map_by_code - get vimc_pix_map struct by media bus code * diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c index 7521439747c5..6bac1fa65a6f 100644 --- a/drivers/media/platform/vimc/vimc-scaler.c +++ b/drivers/media/platform/vimc/vimc-scaler.c @@ -110,13 +110,19 @@ static int vimc_sca_enum_mbus_code(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_mbus_code_enum *code) { - const struct vimc_pix_map *vpix = vimc_pix_map_by_index(code->index); + const u32 mbus_code = vimc_mbus_code_by_index(code->index); + const struct vimc_pix_map *vpix; + + if (!mbus_code) + return -EINVAL; + + vpix = vimc_pix_map_by_code(mbus_code); /* We don't support bayer format */ if (!vpix || vpix->bayer) return -EINVAL; - code->code = vpix->code; + code->code = mbus_code; return 0; } diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c index 92daee58209e..b8bd430809c1 100644 --- a/drivers/media/platform/vimc/vimc-sensor.c +++ b/drivers/media/platform/vimc/vimc-sensor.c @@ -52,12 +52,12 @@ static int vimc_sen_enum_mbus_code(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_mbus_code_enum *code) { - const struct vimc_pix_map *vpix = vimc_pix_map_by_index(code->index); + const u32 mbus_code = vimc_mbus_code_by_index(code->index); - if (!vpix) + if (!mbus_code) return -EINVAL; - code->code = vpix->code; + code->code = mbus_code; return 0; } -- 2.25.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/3] media: vimc: Support multiple media bus codes for each pixelformat 2020-03-26 21:47 ` [PATCH v2 1/3] media: vimc: Support multiple media bus codes for each pixelformat Nícolas F. R. A. Prado @ 2020-03-30 19:36 ` Helen Koike 2020-04-20 20:36 ` Nícolas F. R. A. Prado 0 siblings, 1 reply; 15+ messages in thread From: Helen Koike @ 2020-03-30 19:36 UTC (permalink / raw) To: Nícolas F. R. A. Prado, linux-media Cc: Shuah Khan, Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, lkcamp Hi Nícolas, thank you for the patch. The series looks good in general, just minor comments below. On 3/26/20 6:47 PM, Nícolas F. R. A. Prado wrote: > Change vimc_pix_map_list to allow multiple media bus codes to map to the > same pixelformat, making it possible to add media bus codes for which > there are no pixelformat. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@protonmail.com> > --- > > Changes in v2: > - Fix vimc_mbus_code_by_index not checking code array bounds > - Fix array formatting > - Rename variables > - Change code array size > - Add comment about vimc_mbus_code_by_index return value > > drivers/media/platform/vimc/vimc-common.c | 70 ++++++++++++++--------- > drivers/media/platform/vimc/vimc-common.h | 11 +++- > drivers/media/platform/vimc/vimc-scaler.c | 10 +++- > drivers/media/platform/vimc/vimc-sensor.c | 6 +- > 4 files changed, 65 insertions(+), 32 deletions(-) > > diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c > index c95c17c048f2..119846f3eaa5 100644 > --- a/drivers/media/platform/vimc/vimc-common.c > +++ b/drivers/media/platform/vimc/vimc-common.c > @@ -19,19 +19,19 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { > > /* RGB formats */ > { > - .code = MEDIA_BUS_FMT_BGR888_1X24, > + .code = { MEDIA_BUS_FMT_BGR888_1X24 }, > .pixelformat = V4L2_PIX_FMT_BGR24, > .bpp = 3, > .bayer = false, > }, > { > - .code = MEDIA_BUS_FMT_RGB888_1X24, > + .code = { MEDIA_BUS_FMT_RGB888_1X24 }, > .pixelformat = V4L2_PIX_FMT_RGB24, > .bpp = 3, > .bayer = false, > }, > { > - .code = MEDIA_BUS_FMT_ARGB8888_1X32, > + .code = { MEDIA_BUS_FMT_ARGB8888_1X32 }, > .pixelformat = V4L2_PIX_FMT_ARGB32, > .bpp = 4, > .bayer = false, > @@ -39,49 +39,49 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { > > /* Bayer formats */ > { > - .code = MEDIA_BUS_FMT_SBGGR8_1X8, > + .code = { MEDIA_BUS_FMT_SBGGR8_1X8 }, > .pixelformat = V4L2_PIX_FMT_SBGGR8, > .bpp = 1, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SGBRG8_1X8, > + .code = { MEDIA_BUS_FMT_SGBRG8_1X8 }, > .pixelformat = V4L2_PIX_FMT_SGBRG8, > .bpp = 1, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SGRBG8_1X8, > + .code = { MEDIA_BUS_FMT_SGRBG8_1X8 }, > .pixelformat = V4L2_PIX_FMT_SGRBG8, > .bpp = 1, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SRGGB8_1X8, > + .code = { MEDIA_BUS_FMT_SRGGB8_1X8 }, > .pixelformat = V4L2_PIX_FMT_SRGGB8, > .bpp = 1, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SBGGR10_1X10, > + .code = { MEDIA_BUS_FMT_SBGGR10_1X10 }, > .pixelformat = V4L2_PIX_FMT_SBGGR10, > .bpp = 2, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SGBRG10_1X10, > + .code = { MEDIA_BUS_FMT_SGBRG10_1X10 }, > .pixelformat = V4L2_PIX_FMT_SGBRG10, > .bpp = 2, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SGRBG10_1X10, > + .code = { MEDIA_BUS_FMT_SGRBG10_1X10 }, > .pixelformat = V4L2_PIX_FMT_SGRBG10, > .bpp = 2, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SRGGB10_1X10, > + .code = { MEDIA_BUS_FMT_SRGGB10_1X10 }, > .pixelformat = V4L2_PIX_FMT_SRGGB10, > .bpp = 2, > .bayer = true, > @@ -89,25 +89,25 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { > > /* 10bit raw bayer a-law compressed to 8 bits */ > { > - .code = MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8, > + .code = { MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8 }, > .pixelformat = V4L2_PIX_FMT_SBGGR10ALAW8, > .bpp = 1, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8, > + .code = { MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8 }, > .pixelformat = V4L2_PIX_FMT_SGBRG10ALAW8, > .bpp = 1, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8, > + .code = { MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8 }, > .pixelformat = V4L2_PIX_FMT_SGRBG10ALAW8, > .bpp = 1, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8, > + .code = { MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8 }, > .pixelformat = V4L2_PIX_FMT_SRGGB10ALAW8, > .bpp = 1, > .bayer = true, > @@ -115,49 +115,49 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { > > /* 10bit raw bayer DPCM compressed to 8 bits */ > { > - .code = MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8, > + .code = { MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8 }, > .pixelformat = V4L2_PIX_FMT_SBGGR10DPCM8, > .bpp = 1, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8, > + .code = { MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8 }, > .pixelformat = V4L2_PIX_FMT_SGBRG10DPCM8, > .bpp = 1, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8, > + .code = { MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8 }, > .pixelformat = V4L2_PIX_FMT_SGRBG10DPCM8, > .bpp = 1, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8, > + .code = { MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8 }, > .pixelformat = V4L2_PIX_FMT_SRGGB10DPCM8, > .bpp = 1, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SBGGR12_1X12, > + .code = { MEDIA_BUS_FMT_SBGGR12_1X12 }, > .pixelformat = V4L2_PIX_FMT_SBGGR12, > .bpp = 2, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SGBRG12_1X12, > + .code = { MEDIA_BUS_FMT_SGBRG12_1X12 }, > .pixelformat = V4L2_PIX_FMT_SGBRG12, > .bpp = 2, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SGRBG12_1X12, > + .code = { MEDIA_BUS_FMT_SGRBG12_1X12 }, > .pixelformat = V4L2_PIX_FMT_SGRBG12, > .bpp = 2, > .bayer = true, > }, > { > - .code = MEDIA_BUS_FMT_SRGGB12_1X12, > + .code = { MEDIA_BUS_FMT_SRGGB12_1X12 }, > .pixelformat = V4L2_PIX_FMT_SRGGB12, > .bpp = 2, > .bayer = true, > @@ -182,13 +182,31 @@ const struct vimc_pix_map *vimc_pix_map_by_index(unsigned int i) > return &vimc_pix_map_list[i]; > } > > +const u32 vimc_mbus_code_by_index(unsigned int index) > +{ > + unsigned int i, j; > + > + for (i = 0; i < ARRAY_SIZE(vimc_pix_map_list); i++) { > + for (j = 0; j < ARRAY_SIZE(vimc_pix_map_list[i].code); j++) { > + if (vimc_pix_map_list[i].code[j]) { Can this be false? > + if (!index) > + return vimc_pix_map_list[i].code[j]; > + index--; > + } > + } > + } > + return 0; > +} > + > const struct vimc_pix_map *vimc_pix_map_by_code(u32 code) > { > - unsigned int i; > + unsigned int i, j; > > for (i = 0; i < ARRAY_SIZE(vimc_pix_map_list); i++) { > - if (vimc_pix_map_list[i].code == code) > - return &vimc_pix_map_list[i]; > + for (j = 0; j < ARRAY_SIZE(vimc_pix_map_list[i].code); j++) { > + if (vimc_pix_map_list[i].code[j] == code) > + return &vimc_pix_map_list[i]; > + } > } > return NULL; > } > diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h > index 616d5a6b0754..585441694c86 100644 > --- a/drivers/media/platform/vimc/vimc-common.h > +++ b/drivers/media/platform/vimc/vimc-common.h > @@ -69,7 +69,7 @@ do { \ > * V4L2_PIX_FMT_* fourcc pixelformat and its bytes per pixel (bpp) > */ > struct vimc_pix_map { > - unsigned int code; > + unsigned int code[1]; > unsigned int bpp; > u32 pixelformat; > bool bayer; > @@ -172,6 +172,15 @@ void vimc_sen_release(struct vimc_ent_device *ved); > */ > const struct vimc_pix_map *vimc_pix_map_by_index(unsigned int i); > > +/** > + * vimc_mbus_code_by_index - get mbus code by its index > + * > + * @index: index of the mbus code in vimc_pix_map_list > + * > + * Returns 0 if no mbus code is found for the given index. > + */ > +const u32 vimc_mbus_code_by_index(unsigned int index); > + > /** > * vimc_pix_map_by_code - get vimc_pix_map struct by media bus code > * > diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c > index 7521439747c5..6bac1fa65a6f 100644 > --- a/drivers/media/platform/vimc/vimc-scaler.c > +++ b/drivers/media/platform/vimc/vimc-scaler.c > @@ -110,13 +110,19 @@ static int vimc_sca_enum_mbus_code(struct v4l2_subdev *sd, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_mbus_code_enum *code) > { > - const struct vimc_pix_map *vpix = vimc_pix_map_by_index(code->index); > + const u32 mbus_code = vimc_mbus_code_by_index(code->index); > + const struct vimc_pix_map *vpix; > + > + if (!mbus_code) > + return -EINVAL; > + > + vpix = vimc_pix_map_by_code(mbus_code); > > /* We don't support bayer format */ > if (!vpix || vpix->bayer) > return -EINVAL; > > - code->code = vpix->code; > + code->code = mbus_code; no need to change this. > > return 0; > } > diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c > index 92daee58209e..b8bd430809c1 100644 > --- a/drivers/media/platform/vimc/vimc-sensor.c > +++ b/drivers/media/platform/vimc/vimc-sensor.c > @@ -52,12 +52,12 @@ static int vimc_sen_enum_mbus_code(struct v4l2_subdev *sd, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_mbus_code_enum *code) > { > - const struct vimc_pix_map *vpix = vimc_pix_map_by_index(code->index); > + const u32 mbus_code = vimc_mbus_code_by_index(code->index); > > - if (!vpix) > + if (!mbus_code) > return -EINVAL; > > - code->code = vpix->code; > + code->code = mbus_code; > > return 0; > } > With these changes Acked-by: Helen Koike <helen.koike@collabora.com> Regards, Helen ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/3] media: vimc: Support multiple media bus codes for each pixelformat 2020-03-30 19:36 ` Helen Koike @ 2020-04-20 20:36 ` Nícolas F. R. A. Prado 2020-04-20 20:50 ` Helen Koike 0 siblings, 1 reply; 15+ messages in thread From: Nícolas F. R. A. Prado @ 2020-04-20 20:36 UTC (permalink / raw) To: Helen Koike Cc: linux-media, Shuah Khan, Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, lkcamp Hi Helen, thanks for the review. Some comments below. On Mon, Mar 30, 2020 at 04:36:45PM -0300, Helen Koike wrote: > > Hi Nícolas, > > thank you for the patch. > > The series looks good in general, just minor comments below. > > On 3/26/20 6:47 PM, Nícolas F. R. A. Prado wrote: > > Change vimc_pix_map_list to allow multiple media bus codes to map to the > > same pixelformat, making it possible to add media bus codes for which > > there are no pixelformat. > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@protonmail.com> > > --- > > > > Changes in v2: > > - Fix vimc_mbus_code_by_index not checking code array bounds > > - Fix array formatting > > - Rename variables > > - Change code array size > > - Add comment about vimc_mbus_code_by_index return value > > > > drivers/media/platform/vimc/vimc-common.c | 70 ++++++++++++++--------- > > drivers/media/platform/vimc/vimc-common.h | 11 +++- > > drivers/media/platform/vimc/vimc-scaler.c | 10 +++- > > drivers/media/platform/vimc/vimc-sensor.c | 6 +- > > 4 files changed, 65 insertions(+), 32 deletions(-) > > > > diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c > > index c95c17c048f2..119846f3eaa5 100644 > > --- a/drivers/media/platform/vimc/vimc-common.c > > +++ b/drivers/media/platform/vimc/vimc-common.c > > @@ -19,19 +19,19 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { > > > > /* RGB formats */ > > { > > - .code = MEDIA_BUS_FMT_BGR888_1X24, > > + .code = { MEDIA_BUS_FMT_BGR888_1X24 }, > > .pixelformat = V4L2_PIX_FMT_BGR24, > > .bpp = 3, > > .bayer = false, > > }, > > { > > - .code = MEDIA_BUS_FMT_RGB888_1X24, > > + .code = { MEDIA_BUS_FMT_RGB888_1X24 }, > > .pixelformat = V4L2_PIX_FMT_RGB24, > > .bpp = 3, > > .bayer = false, > > }, > > { > > - .code = MEDIA_BUS_FMT_ARGB8888_1X32, > > + .code = { MEDIA_BUS_FMT_ARGB8888_1X32 }, > > .pixelformat = V4L2_PIX_FMT_ARGB32, > > .bpp = 4, > > .bayer = false, > > @@ -39,49 +39,49 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { > > > > /* Bayer formats */ > > { > > - .code = MEDIA_BUS_FMT_SBGGR8_1X8, > > + .code = { MEDIA_BUS_FMT_SBGGR8_1X8 }, > > .pixelformat = V4L2_PIX_FMT_SBGGR8, > > .bpp = 1, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SGBRG8_1X8, > > + .code = { MEDIA_BUS_FMT_SGBRG8_1X8 }, > > .pixelformat = V4L2_PIX_FMT_SGBRG8, > > .bpp = 1, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SGRBG8_1X8, > > + .code = { MEDIA_BUS_FMT_SGRBG8_1X8 }, > > .pixelformat = V4L2_PIX_FMT_SGRBG8, > > .bpp = 1, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SRGGB8_1X8, > > + .code = { MEDIA_BUS_FMT_SRGGB8_1X8 }, > > .pixelformat = V4L2_PIX_FMT_SRGGB8, > > .bpp = 1, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SBGGR10_1X10, > > + .code = { MEDIA_BUS_FMT_SBGGR10_1X10 }, > > .pixelformat = V4L2_PIX_FMT_SBGGR10, > > .bpp = 2, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SGBRG10_1X10, > > + .code = { MEDIA_BUS_FMT_SGBRG10_1X10 }, > > .pixelformat = V4L2_PIX_FMT_SGBRG10, > > .bpp = 2, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SGRBG10_1X10, > > + .code = { MEDIA_BUS_FMT_SGRBG10_1X10 }, > > .pixelformat = V4L2_PIX_FMT_SGRBG10, > > .bpp = 2, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SRGGB10_1X10, > > + .code = { MEDIA_BUS_FMT_SRGGB10_1X10 }, > > .pixelformat = V4L2_PIX_FMT_SRGGB10, > > .bpp = 2, > > .bayer = true, > > @@ -89,25 +89,25 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { > > > > /* 10bit raw bayer a-law compressed to 8 bits */ > > { > > - .code = MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8, > > + .code = { MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8 }, > > .pixelformat = V4L2_PIX_FMT_SBGGR10ALAW8, > > .bpp = 1, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8, > > + .code = { MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8 }, > > .pixelformat = V4L2_PIX_FMT_SGBRG10ALAW8, > > .bpp = 1, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8, > > + .code = { MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8 }, > > .pixelformat = V4L2_PIX_FMT_SGRBG10ALAW8, > > .bpp = 1, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8, > > + .code = { MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8 }, > > .pixelformat = V4L2_PIX_FMT_SRGGB10ALAW8, > > .bpp = 1, > > .bayer = true, > > @@ -115,49 +115,49 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { > > > > /* 10bit raw bayer DPCM compressed to 8 bits */ > > { > > - .code = MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8, > > + .code = { MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8 }, > > .pixelformat = V4L2_PIX_FMT_SBGGR10DPCM8, > > .bpp = 1, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8, > > + .code = { MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8 }, > > .pixelformat = V4L2_PIX_FMT_SGBRG10DPCM8, > > .bpp = 1, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8, > > + .code = { MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8 }, > > .pixelformat = V4L2_PIX_FMT_SGRBG10DPCM8, > > .bpp = 1, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8, > > + .code = { MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8 }, > > .pixelformat = V4L2_PIX_FMT_SRGGB10DPCM8, > > .bpp = 1, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SBGGR12_1X12, > > + .code = { MEDIA_BUS_FMT_SBGGR12_1X12 }, > > .pixelformat = V4L2_PIX_FMT_SBGGR12, > > .bpp = 2, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SGBRG12_1X12, > > + .code = { MEDIA_BUS_FMT_SGBRG12_1X12 }, > > .pixelformat = V4L2_PIX_FMT_SGBRG12, > > .bpp = 2, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SGRBG12_1X12, > > + .code = { MEDIA_BUS_FMT_SGRBG12_1X12 }, > > .pixelformat = V4L2_PIX_FMT_SGRBG12, > > .bpp = 2, > > .bayer = true, > > }, > > { > > - .code = MEDIA_BUS_FMT_SRGGB12_1X12, > > + .code = { MEDIA_BUS_FMT_SRGGB12_1X12 }, > > .pixelformat = V4L2_PIX_FMT_SRGGB12, > > .bpp = 2, > > .bayer = true, > > @@ -182,13 +182,31 @@ const struct vimc_pix_map *vimc_pix_map_by_index(unsigned int i) > > return &vimc_pix_map_list[i]; > > } > > > > +const u32 vimc_mbus_code_by_index(unsigned int index) > > +{ > > + unsigned int i, j; > > + > > + for (i = 0; i < ARRAY_SIZE(vimc_pix_map_list); i++) { > > + for (j = 0; j < ARRAY_SIZE(vimc_pix_map_list[i].code); j++) { > > + if (vimc_pix_map_list[i].code[j]) { > > Can this be false? Actually it can, but after you asked I realized this code could be way clearer. When vimc_pix_map_list[i].code[j] is 0, it means that this is an unused value of the array, so we should skip to the next pixelformat. I think writing it this way instead would be better, what do you think? for (i = 0; i < ARRAY_SIZE(vimc_pix_map_list); i++) { for (j = 0; j < ARRAY_SIZE(vimc_pix_map_list[i].code); j++) { if (!vimc_pix_map_list[i].code[j]) break; if (!index) return vimc_pix_map_list[i].code[j]; index--; > > > + if (!index) > > + return vimc_pix_map_list[i].code[j]; > > + index--; > > + } > > + } > > + } > > + return 0; > > +} > > + > > const struct vimc_pix_map *vimc_pix_map_by_code(u32 code) > > { > > - unsigned int i; > > + unsigned int i, j; > > > > for (i = 0; i < ARRAY_SIZE(vimc_pix_map_list); i++) { > > - if (vimc_pix_map_list[i].code == code) > > - return &vimc_pix_map_list[i]; > > + for (j = 0; j < ARRAY_SIZE(vimc_pix_map_list[i].code); j++) { > > + if (vimc_pix_map_list[i].code[j] == code) > > + return &vimc_pix_map_list[i]; > > + } > > } > > return NULL; > > } > > diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h > > index 616d5a6b0754..585441694c86 100644 > > --- a/drivers/media/platform/vimc/vimc-common.h > > +++ b/drivers/media/platform/vimc/vimc-common.h > > @@ -69,7 +69,7 @@ do { \ > > * V4L2_PIX_FMT_* fourcc pixelformat and its bytes per pixel (bpp) > > */ > > struct vimc_pix_map { > > - unsigned int code; > > + unsigned int code[1]; > > unsigned int bpp; > > u32 pixelformat; > > bool bayer; > > @@ -172,6 +172,15 @@ void vimc_sen_release(struct vimc_ent_device *ved); > > */ > > const struct vimc_pix_map *vimc_pix_map_by_index(unsigned int i); > > > > +/** > > + * vimc_mbus_code_by_index - get mbus code by its index > > + * > > + * @index: index of the mbus code in vimc_pix_map_list > > + * > > + * Returns 0 if no mbus code is found for the given index. > > + */ > > +const u32 vimc_mbus_code_by_index(unsigned int index); > > + > > /** > > * vimc_pix_map_by_code - get vimc_pix_map struct by media bus code > > * > > diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c > > index 7521439747c5..6bac1fa65a6f 100644 > > --- a/drivers/media/platform/vimc/vimc-scaler.c > > +++ b/drivers/media/platform/vimc/vimc-scaler.c > > @@ -110,13 +110,19 @@ static int vimc_sca_enum_mbus_code(struct v4l2_subdev *sd, > > struct v4l2_subdev_pad_config *cfg, > > struct v4l2_subdev_mbus_code_enum *code) > > { > > - const struct vimc_pix_map *vpix = vimc_pix_map_by_index(code->index); > > + const u32 mbus_code = vimc_mbus_code_by_index(code->index); > > + const struct vimc_pix_map *vpix; > > + > > + if (!mbus_code) > > + return -EINVAL; > > + > > + vpix = vimc_pix_map_by_code(mbus_code); > > > > /* We don't support bayer format */ > > if (!vpix || vpix->bayer) > > return -EINVAL; > > > > - code->code = vpix->code; > > + code->code = mbus_code; > > no need to change this. This change is actually needed, because after this patch, the code property of vimc_pix_map_list is an array, so there isn't a 1 to 1 relation between mbus code and pixmap format anymore. Since we already got the mbus code by index through vimc_mbus_code_by_index(code->index), we just use it. > > > > > return 0; > > } > > diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c > > index 92daee58209e..b8bd430809c1 100644 > > --- a/drivers/media/platform/vimc/vimc-sensor.c > > +++ b/drivers/media/platform/vimc/vimc-sensor.c > > @@ -52,12 +52,12 @@ static int vimc_sen_enum_mbus_code(struct v4l2_subdev *sd, > > struct v4l2_subdev_pad_config *cfg, > > struct v4l2_subdev_mbus_code_enum *code) > > { > > - const struct vimc_pix_map *vpix = vimc_pix_map_by_index(code->index); > > + const u32 mbus_code = vimc_mbus_code_by_index(code->index); > > > > - if (!vpix) > > + if (!mbus_code) > > return -EINVAL; > > > > - code->code = vpix->code; > > + code->code = mbus_code; > > > > return 0; > > } > > > > With these changes > > Acked-by: Helen Koike <helen.koike@collabora.com> > > Regards, > Helen Thank you, Nícolas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/3] media: vimc: Support multiple media bus codes for each pixelformat 2020-04-20 20:36 ` Nícolas F. R. A. Prado @ 2020-04-20 20:50 ` Helen Koike 0 siblings, 0 replies; 15+ messages in thread From: Helen Koike @ 2020-04-20 20:50 UTC (permalink / raw) To: Nícolas F. R. A. Prado Cc: linux-media, Shuah Khan, Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, lkcamp Hi Nícolas, On 4/20/20 5:36 PM, Nícolas F. R. A. Prado wrote: > Hi Helen, > > thanks for the review. > > Some comments below. > > On Mon, Mar 30, 2020 at 04:36:45PM -0300, Helen Koike wrote: >> >> Hi Nícolas, >> >> thank you for the patch. >> >> The series looks good in general, just minor comments below. >> >> On 3/26/20 6:47 PM, Nícolas F. R. A. Prado wrote: >>> Change vimc_pix_map_list to allow multiple media bus codes to map to the >>> same pixelformat, making it possible to add media bus codes for which >>> there are no pixelformat. >>> >>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@protonmail.com> >>> --- >>> >>> Changes in v2: >>> - Fix vimc_mbus_code_by_index not checking code array bounds >>> - Fix array formatting >>> - Rename variables >>> - Change code array size >>> - Add comment about vimc_mbus_code_by_index return value >>> >>> drivers/media/platform/vimc/vimc-common.c | 70 ++++++++++++++--------- >>> drivers/media/platform/vimc/vimc-common.h | 11 +++- >>> drivers/media/platform/vimc/vimc-scaler.c | 10 +++- >>> drivers/media/platform/vimc/vimc-sensor.c | 6 +- >>> 4 files changed, 65 insertions(+), 32 deletions(-) >>> >>> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c >>> index c95c17c048f2..119846f3eaa5 100644 >>> --- a/drivers/media/platform/vimc/vimc-common.c >>> +++ b/drivers/media/platform/vimc/vimc-common.c >>> @@ -19,19 +19,19 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { >>> >>> /* RGB formats */ >>> { >>> - .code = MEDIA_BUS_FMT_BGR888_1X24, >>> + .code = { MEDIA_BUS_FMT_BGR888_1X24 }, >>> .pixelformat = V4L2_PIX_FMT_BGR24, >>> .bpp = 3, >>> .bayer = false, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_RGB888_1X24, >>> + .code = { MEDIA_BUS_FMT_RGB888_1X24 }, >>> .pixelformat = V4L2_PIX_FMT_RGB24, >>> .bpp = 3, >>> .bayer = false, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_ARGB8888_1X32, >>> + .code = { MEDIA_BUS_FMT_ARGB8888_1X32 }, >>> .pixelformat = V4L2_PIX_FMT_ARGB32, >>> .bpp = 4, >>> .bayer = false, >>> @@ -39,49 +39,49 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { >>> >>> /* Bayer formats */ >>> { >>> - .code = MEDIA_BUS_FMT_SBGGR8_1X8, >>> + .code = { MEDIA_BUS_FMT_SBGGR8_1X8 }, >>> .pixelformat = V4L2_PIX_FMT_SBGGR8, >>> .bpp = 1, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SGBRG8_1X8, >>> + .code = { MEDIA_BUS_FMT_SGBRG8_1X8 }, >>> .pixelformat = V4L2_PIX_FMT_SGBRG8, >>> .bpp = 1, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SGRBG8_1X8, >>> + .code = { MEDIA_BUS_FMT_SGRBG8_1X8 }, >>> .pixelformat = V4L2_PIX_FMT_SGRBG8, >>> .bpp = 1, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SRGGB8_1X8, >>> + .code = { MEDIA_BUS_FMT_SRGGB8_1X8 }, >>> .pixelformat = V4L2_PIX_FMT_SRGGB8, >>> .bpp = 1, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SBGGR10_1X10, >>> + .code = { MEDIA_BUS_FMT_SBGGR10_1X10 }, >>> .pixelformat = V4L2_PIX_FMT_SBGGR10, >>> .bpp = 2, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SGBRG10_1X10, >>> + .code = { MEDIA_BUS_FMT_SGBRG10_1X10 }, >>> .pixelformat = V4L2_PIX_FMT_SGBRG10, >>> .bpp = 2, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SGRBG10_1X10, >>> + .code = { MEDIA_BUS_FMT_SGRBG10_1X10 }, >>> .pixelformat = V4L2_PIX_FMT_SGRBG10, >>> .bpp = 2, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SRGGB10_1X10, >>> + .code = { MEDIA_BUS_FMT_SRGGB10_1X10 }, >>> .pixelformat = V4L2_PIX_FMT_SRGGB10, >>> .bpp = 2, >>> .bayer = true, >>> @@ -89,25 +89,25 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { >>> >>> /* 10bit raw bayer a-law compressed to 8 bits */ >>> { >>> - .code = MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8, >>> + .code = { MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8 }, >>> .pixelformat = V4L2_PIX_FMT_SBGGR10ALAW8, >>> .bpp = 1, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8, >>> + .code = { MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8 }, >>> .pixelformat = V4L2_PIX_FMT_SGBRG10ALAW8, >>> .bpp = 1, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8, >>> + .code = { MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8 }, >>> .pixelformat = V4L2_PIX_FMT_SGRBG10ALAW8, >>> .bpp = 1, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8, >>> + .code = { MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8 }, >>> .pixelformat = V4L2_PIX_FMT_SRGGB10ALAW8, >>> .bpp = 1, >>> .bayer = true, >>> @@ -115,49 +115,49 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { >>> >>> /* 10bit raw bayer DPCM compressed to 8 bits */ >>> { >>> - .code = MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8, >>> + .code = { MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8 }, >>> .pixelformat = V4L2_PIX_FMT_SBGGR10DPCM8, >>> .bpp = 1, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8, >>> + .code = { MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8 }, >>> .pixelformat = V4L2_PIX_FMT_SGBRG10DPCM8, >>> .bpp = 1, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8, >>> + .code = { MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8 }, >>> .pixelformat = V4L2_PIX_FMT_SGRBG10DPCM8, >>> .bpp = 1, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8, >>> + .code = { MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8 }, >>> .pixelformat = V4L2_PIX_FMT_SRGGB10DPCM8, >>> .bpp = 1, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SBGGR12_1X12, >>> + .code = { MEDIA_BUS_FMT_SBGGR12_1X12 }, >>> .pixelformat = V4L2_PIX_FMT_SBGGR12, >>> .bpp = 2, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SGBRG12_1X12, >>> + .code = { MEDIA_BUS_FMT_SGBRG12_1X12 }, >>> .pixelformat = V4L2_PIX_FMT_SGBRG12, >>> .bpp = 2, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SGRBG12_1X12, >>> + .code = { MEDIA_BUS_FMT_SGRBG12_1X12 }, >>> .pixelformat = V4L2_PIX_FMT_SGRBG12, >>> .bpp = 2, >>> .bayer = true, >>> }, >>> { >>> - .code = MEDIA_BUS_FMT_SRGGB12_1X12, >>> + .code = { MEDIA_BUS_FMT_SRGGB12_1X12 }, >>> .pixelformat = V4L2_PIX_FMT_SRGGB12, >>> .bpp = 2, >>> .bayer = true, >>> @@ -182,13 +182,31 @@ const struct vimc_pix_map *vimc_pix_map_by_index(unsigned int i) >>> return &vimc_pix_map_list[i]; >>> } >>> >>> +const u32 vimc_mbus_code_by_index(unsigned int index) >>> +{ >>> + unsigned int i, j; >>> + >>> + for (i = 0; i < ARRAY_SIZE(vimc_pix_map_list); i++) { >>> + for (j = 0; j < ARRAY_SIZE(vimc_pix_map_list[i].code); j++) { >>> + if (vimc_pix_map_list[i].code[j]) { >> >> Can this be false? > > Actually it can, but after you asked I realized this code could be way clearer. > > When vimc_pix_map_list[i].code[j] is 0, it means that this is an unused value of > the array, so we should skip to the next pixelformat. > I think writing it this way instead would be better, what do you think? > > for (i = 0; i < ARRAY_SIZE(vimc_pix_map_list); i++) { > for (j = 0; j < ARRAY_SIZE(vimc_pix_map_list[i].code); j++) { > if (!vimc_pix_map_list[i].code[j]) > break; > > if (!index) > return vimc_pix_map_list[i].code[j]; > index--; > Looks better, I prefer reducing indentation. >> >>> + if (!index) >>> + return vimc_pix_map_list[i].code[j]; >>> + index--; >>> + } >>> + } >>> + } >>> + return 0; >>> +} >>> + >>> const struct vimc_pix_map *vimc_pix_map_by_code(u32 code) >>> { >>> - unsigned int i; >>> + unsigned int i, j; >>> >>> for (i = 0; i < ARRAY_SIZE(vimc_pix_map_list); i++) { >>> - if (vimc_pix_map_list[i].code == code) >>> - return &vimc_pix_map_list[i]; >>> + for (j = 0; j < ARRAY_SIZE(vimc_pix_map_list[i].code); j++) { >>> + if (vimc_pix_map_list[i].code[j] == code) >>> + return &vimc_pix_map_list[i]; >>> + } >>> } >>> return NULL; >>> } >>> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h >>> index 616d5a6b0754..585441694c86 100644 >>> --- a/drivers/media/platform/vimc/vimc-common.h >>> +++ b/drivers/media/platform/vimc/vimc-common.h >>> @@ -69,7 +69,7 @@ do { \ >>> * V4L2_PIX_FMT_* fourcc pixelformat and its bytes per pixel (bpp) >>> */ >>> struct vimc_pix_map { >>> - unsigned int code; >>> + unsigned int code[1]; >>> unsigned int bpp; >>> u32 pixelformat; >>> bool bayer; >>> @@ -172,6 +172,15 @@ void vimc_sen_release(struct vimc_ent_device *ved); >>> */ >>> const struct vimc_pix_map *vimc_pix_map_by_index(unsigned int i); >>> >>> +/** >>> + * vimc_mbus_code_by_index - get mbus code by its index >>> + * >>> + * @index: index of the mbus code in vimc_pix_map_list >>> + * >>> + * Returns 0 if no mbus code is found for the given index. >>> + */ >>> +const u32 vimc_mbus_code_by_index(unsigned int index); >>> + >>> /** >>> * vimc_pix_map_by_code - get vimc_pix_map struct by media bus code >>> * >>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c >>> index 7521439747c5..6bac1fa65a6f 100644 >>> --- a/drivers/media/platform/vimc/vimc-scaler.c >>> +++ b/drivers/media/platform/vimc/vimc-scaler.c >>> @@ -110,13 +110,19 @@ static int vimc_sca_enum_mbus_code(struct v4l2_subdev *sd, >>> struct v4l2_subdev_pad_config *cfg, >>> struct v4l2_subdev_mbus_code_enum *code) >>> { >>> - const struct vimc_pix_map *vpix = vimc_pix_map_by_index(code->index); >>> + const u32 mbus_code = vimc_mbus_code_by_index(code->index); >>> + const struct vimc_pix_map *vpix; >>> + >>> + if (!mbus_code) >>> + return -EINVAL; >>> + >>> + vpix = vimc_pix_map_by_code(mbus_code); >>> >>> /* We don't support bayer format */ >>> if (!vpix || vpix->bayer) >>> return -EINVAL; >>> >>> - code->code = vpix->code; >>> + code->code = mbus_code; >> >> no need to change this. > > This change is actually needed, because after this patch, the code property of > vimc_pix_map_list is an array, so there isn't a 1 to 1 relation between mbus > code and pixmap format anymore. > Since we already got the mbus code by index through > vimc_mbus_code_by_index(code->index), we just use it. Make sense, thank you for your explanation. Regards, Helen > >> >>> >>> return 0; >>> } >>> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c >>> index 92daee58209e..b8bd430809c1 100644 >>> --- a/drivers/media/platform/vimc/vimc-sensor.c >>> +++ b/drivers/media/platform/vimc/vimc-sensor.c >>> @@ -52,12 +52,12 @@ static int vimc_sen_enum_mbus_code(struct v4l2_subdev *sd, >>> struct v4l2_subdev_pad_config *cfg, >>> struct v4l2_subdev_mbus_code_enum *code) >>> { >>> - const struct vimc_pix_map *vpix = vimc_pix_map_by_index(code->index); >>> + const u32 mbus_code = vimc_mbus_code_by_index(code->index); >>> >>> - if (!vpix) >>> + if (!mbus_code) >>> return -EINVAL; >>> >>> - code->code = vpix->code; >>> + code->code = mbus_code; >>> >>> return 0; >>> } >>> >> >> With these changes >> >> Acked-by: Helen Koike <helen.koike@collabora.com> >> >> Regards, >> Helen > > Thank you, > Nícolas > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/3] media: vimc: Add missing {RGB,BGR,GBR}888 media bus codes 2020-03-26 21:47 [PATCH v2 0/3] media: vimc: Add support for {RGB,BGR,GBR}888 bus formats on debayer source pad Nícolas F. R. A. Prado 2020-03-26 21:47 ` [PATCH v2 1/3] media: vimc: Support multiple media bus codes for each pixelformat Nícolas F. R. A. Prado @ 2020-03-26 21:47 ` Nícolas F. R. A. Prado 2020-03-26 21:56 ` Shuah Khan 2020-03-26 21:47 ` [PATCH v2 3/3] media: vimc: deb: Add support for {RGB,BGR,GBR}888 bus formats on source pad Nícolas F. R. A. Prado 2 siblings, 1 reply; 15+ messages in thread From: Nícolas F. R. A. Prado @ 2020-03-26 21:47 UTC (permalink / raw) To: linux-media Cc: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, lkcamp Add missing RGB888_*, BGR888_* and GBR888_* media bus codes in the vimc_pix_map_list. Since there is no GBR24 pixelformat, use the RGB24 pixelformat for MEDIA_BUS_FMT_GBR888_1X24. Co-developed-by: Vitor Massaru Iha <vitor@massaru.org> Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@protonmail.com> --- Changes in v2: - Fix array formatting - Change commit message to reflect v2 changes - Change code array size - Add other BGR888 and RGB888 formats to BGR24 and RGB24 pixelformats drivers/media/platform/vimc/vimc-common.c | 16 ++++++++++++++-- drivers/media/platform/vimc/vimc-common.h | 2 +- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c index 119846f3eaa5..11489334cff7 100644 --- a/drivers/media/platform/vimc/vimc-common.c +++ b/drivers/media/platform/vimc/vimc-common.c @@ -19,13 +19,25 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { /* RGB formats */ { - .code = { MEDIA_BUS_FMT_BGR888_1X24 }, + .code = { + MEDIA_BUS_FMT_BGR888_1X24, + MEDIA_BUS_FMT_BGR888_3X8 + }, .pixelformat = V4L2_PIX_FMT_BGR24, .bpp = 3, .bayer = false, }, { - .code = { MEDIA_BUS_FMT_RGB888_1X24 }, + .code = { + MEDIA_BUS_FMT_RGB888_1X24, + MEDIA_BUS_FMT_RGB888_2X12_BE, + MEDIA_BUS_FMT_RGB888_2X12_LE, + MEDIA_BUS_FMT_RGB888_3X8, + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, + MEDIA_BUS_FMT_RGB888_1X32_PADHI, + MEDIA_BUS_FMT_GBR888_1X24 + }, .pixelformat = V4L2_PIX_FMT_RGB24, .bpp = 3, .bayer = false, diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h index 585441694c86..d5e0e8d32542 100644 --- a/drivers/media/platform/vimc/vimc-common.h +++ b/drivers/media/platform/vimc/vimc-common.h @@ -69,7 +69,7 @@ do { \ * V4L2_PIX_FMT_* fourcc pixelformat and its bytes per pixel (bpp) */ struct vimc_pix_map { - unsigned int code[1]; + unsigned int code[8]; unsigned int bpp; u32 pixelformat; bool bayer; -- 2.25.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] media: vimc: Add missing {RGB,BGR,GBR}888 media bus codes 2020-03-26 21:47 ` [PATCH v2 2/3] media: vimc: Add missing {RGB,BGR,GBR}888 media bus codes Nícolas F. R. A. Prado @ 2020-03-26 21:56 ` Shuah Khan 2020-03-30 19:37 ` Helen Koike 0 siblings, 1 reply; 15+ messages in thread From: Shuah Khan @ 2020-03-26 21:56 UTC (permalink / raw) To: Nícolas F. R. A. Prado, linux-media Cc: Helen Koike, Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, lkcamp, Shuah Khan On 3/26/20 3:47 PM, Nícolas F. R. A. Prado wrote: > Add missing RGB888_*, BGR888_* and GBR888_* media bus codes in the > vimc_pix_map_list. Since there is no GBR24 pixelformat, use the RGB24 > pixelformat for MEDIA_BUS_FMT_GBR888_1X24. > > Co-developed-by: Vitor Massaru Iha <vitor@massaru.org> > Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@protonmail.com> > --- > > Changes in v2: > - Fix array formatting > - Change commit message to reflect v2 changes > - Change code array size > - Add other BGR888 and RGB888 formats to BGR24 and RGB24 pixelformats > > drivers/media/platform/vimc/vimc-common.c | 16 ++++++++++++++-- > drivers/media/platform/vimc/vimc-common.h | 2 +- > 2 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c > index 119846f3eaa5..11489334cff7 100644 > --- a/drivers/media/platform/vimc/vimc-common.c > +++ b/drivers/media/platform/vimc/vimc-common.c > @@ -19,13 +19,25 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { > > /* RGB formats */ > { > - .code = { MEDIA_BUS_FMT_BGR888_1X24 }, > + .code = { > + MEDIA_BUS_FMT_BGR888_1X24, > + MEDIA_BUS_FMT_BGR888_3X8 > + }, > .pixelformat = V4L2_PIX_FMT_BGR24, > .bpp = 3, > .bayer = false, > }, > { > - .code = { MEDIA_BUS_FMT_RGB888_1X24 }, > + .code = { > + MEDIA_BUS_FMT_RGB888_1X24, > + MEDIA_BUS_FMT_RGB888_2X12_BE, > + MEDIA_BUS_FMT_RGB888_2X12_LE, > + MEDIA_BUS_FMT_RGB888_3X8, > + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, > + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, > + MEDIA_BUS_FMT_RGB888_1X32_PADHI, > + MEDIA_BUS_FMT_GBR888_1X24 > + }, > .pixelformat = V4L2_PIX_FMT_RGB24, > .bpp = 3, > .bayer = false, > diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h > index 585441694c86..d5e0e8d32542 100644 > --- a/drivers/media/platform/vimc/vimc-common.h > +++ b/drivers/media/platform/vimc/vimc-common.h > @@ -69,7 +69,7 @@ do { \ > * V4L2_PIX_FMT_* fourcc pixelformat and its bytes per pixel (bpp) > */ > struct vimc_pix_map { > - unsigned int code[1]; > + unsigned int code[8]; Please add a define for this instead of hard coded value. > unsigned int bpp; > u32 pixelformat; > bool bayer; > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] media: vimc: Add missing {RGB,BGR,GBR}888 media bus codes 2020-03-26 21:56 ` Shuah Khan @ 2020-03-30 19:37 ` Helen Koike 0 siblings, 0 replies; 15+ messages in thread From: Helen Koike @ 2020-03-30 19:37 UTC (permalink / raw) To: Shuah Khan, Nícolas F. R. A. Prado, linux-media Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, lkcamp On 3/26/20 6:56 PM, Shuah Khan wrote: > On 3/26/20 3:47 PM, Nícolas F. R. A. Prado wrote: >> Add missing RGB888_*, BGR888_* and GBR888_* media bus codes in the >> vimc_pix_map_list. Since there is no GBR24 pixelformat, use the RGB24 >> pixelformat for MEDIA_BUS_FMT_GBR888_1X24. >> >> Co-developed-by: Vitor Massaru Iha <vitor@massaru.org> >> Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> >> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@protonmail.com> >> --- >> >> Changes in v2: >> - Fix array formatting >> - Change commit message to reflect v2 changes >> - Change code array size >> - Add other BGR888 and RGB888 formats to BGR24 and RGB24 pixelformats >> >> drivers/media/platform/vimc/vimc-common.c | 16 ++++++++++++++-- >> drivers/media/platform/vimc/vimc-common.h | 2 +- >> 2 files changed, 15 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c >> index 119846f3eaa5..11489334cff7 100644 >> --- a/drivers/media/platform/vimc/vimc-common.c >> +++ b/drivers/media/platform/vimc/vimc-common.c >> @@ -19,13 +19,25 @@ static const struct vimc_pix_map vimc_pix_map_list[] = { >> /* RGB formats */ >> { >> - .code = { MEDIA_BUS_FMT_BGR888_1X24 }, >> + .code = { >> + MEDIA_BUS_FMT_BGR888_1X24, >> + MEDIA_BUS_FMT_BGR888_3X8 >> + }, >> .pixelformat = V4L2_PIX_FMT_BGR24, >> .bpp = 3, >> .bayer = false, >> }, >> { >> - .code = { MEDIA_BUS_FMT_RGB888_1X24 }, >> + .code = { >> + MEDIA_BUS_FMT_RGB888_1X24, >> + MEDIA_BUS_FMT_RGB888_2X12_BE, >> + MEDIA_BUS_FMT_RGB888_2X12_LE, >> + MEDIA_BUS_FMT_RGB888_3X8, >> + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, >> + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, >> + MEDIA_BUS_FMT_RGB888_1X32_PADHI, >> + MEDIA_BUS_FMT_GBR888_1X24 >> + }, >> .pixelformat = V4L2_PIX_FMT_RGB24, >> .bpp = 3, >> .bayer = false, >> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h >> index 585441694c86..d5e0e8d32542 100644 >> --- a/drivers/media/platform/vimc/vimc-common.h >> +++ b/drivers/media/platform/vimc/vimc-common.h >> @@ -69,7 +69,7 @@ do { \ >> * V4L2_PIX_FMT_* fourcc pixelformat and its bytes per pixel (bpp) >> */ >> struct vimc_pix_map { >> - unsigned int code[1]; > >> + unsigned int code[8]; > Please add a define for this instead of hard coded value. With this change suggested by Shuah: Acked-by: Helen Koike <helen.koike@collabora.com> Regards, Helen > > >> unsigned int bpp; >> u32 pixelformat; >> bool bayer; >> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 3/3] media: vimc: deb: Add support for {RGB,BGR,GBR}888 bus formats on source pad 2020-03-26 21:47 [PATCH v2 0/3] media: vimc: Add support for {RGB,BGR,GBR}888 bus formats on debayer source pad Nícolas F. R. A. Prado 2020-03-26 21:47 ` [PATCH v2 1/3] media: vimc: Support multiple media bus codes for each pixelformat Nícolas F. R. A. Prado 2020-03-26 21:47 ` [PATCH v2 2/3] media: vimc: Add missing {RGB,BGR,GBR}888 media bus codes Nícolas F. R. A. Prado @ 2020-03-26 21:47 ` Nícolas F. R. A. Prado 2020-03-26 22:06 ` Shuah Khan 2 siblings, 1 reply; 15+ messages in thread From: Nícolas F. R. A. Prado @ 2020-03-26 21:47 UTC (permalink / raw) To: linux-media Cc: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, lkcamp Add support for RGB888_*, BGR888_* and GBR888_* media bus formats on the source pad of debayer subdevices. Co-developed-by: Vitor Massaru Iha <vitor@massaru.org> Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@protonmail.com> --- Changes in v2: - Change commit message to reflect v2 changes - Rename variables - Fix array formatting - Add vimc_deb_is_src_code_valid function - Add other BGR888 and RGB888 formats to debayer source pad supported formats drivers/media/platform/vimc/vimc-debayer.c | 61 +++++++++++++++++----- 1 file changed, 49 insertions(+), 12 deletions(-) diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c index baf6bf9f65b5..33a9bea770bc 100644 --- a/drivers/media/platform/vimc/vimc-debayer.c +++ b/drivers/media/platform/vimc/vimc-debayer.c @@ -51,6 +51,19 @@ static const struct v4l2_mbus_framefmt sink_fmt_default = { .colorspace = V4L2_COLORSPACE_DEFAULT, }; +static const u32 vimc_deb_src_mbus_codes[] = { + MEDIA_BUS_FMT_GBR888_1X24, + MEDIA_BUS_FMT_BGR888_1X24, + MEDIA_BUS_FMT_BGR888_3X8, + MEDIA_BUS_FMT_RGB888_1X24, + MEDIA_BUS_FMT_RGB888_2X12_BE, + MEDIA_BUS_FMT_RGB888_2X12_LE, + MEDIA_BUS_FMT_RGB888_3X8, + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, + MEDIA_BUS_FMT_RGB888_1X32_PADHI, +}; + static const struct vimc_deb_pix_map vimc_deb_pix_map_list[] = { { .code = MEDIA_BUS_FMT_SBGGR8_1X8, @@ -125,6 +138,17 @@ static const struct vimc_deb_pix_map *vimc_deb_pix_map_by_code(u32 code) return NULL; } +static int vimc_deb_is_src_code_invalid(u32 code) +{ + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(vimc_deb_src_mbus_codes); i++) + if (vimc_deb_src_mbus_codes[i] == code) + return 0; + + return -EINVAL; +} + static int vimc_deb_init_cfg(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config *cfg) { @@ -148,14 +172,11 @@ static int vimc_deb_enum_mbus_code(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_mbus_code_enum *code) { - /* We only support one format for source pads */ if (VIMC_IS_SRC(code->pad)) { - struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); - - if (code->index) + if (code->index >= ARRAY_SIZE(vimc_deb_src_mbus_codes)) return -EINVAL; - code->code = vdeb->src_code; + code->code = vimc_deb_src_mbus_codes[code->index]; } else { if (code->index >= ARRAY_SIZE(vimc_deb_pix_map_list)) return -EINVAL; @@ -170,8 +191,6 @@ static int vimc_deb_enum_frame_size(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_frame_size_enum *fse) { - struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); - if (fse->index) return -EINVAL; @@ -181,7 +200,7 @@ static int vimc_deb_enum_frame_size(struct v4l2_subdev *sd, if (!vpix) return -EINVAL; - } else if (fse->code != vdeb->src_code) { + } else if (vimc_deb_is_src_code_invalid(fse->code)) { return -EINVAL; } @@ -237,6 +256,7 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, { struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); struct v4l2_mbus_framefmt *sink_fmt; + u32 *src_code; if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) { /* Do not change the format while stream is on */ @@ -244,8 +264,10 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, return -EBUSY; sink_fmt = &vdeb->sink_fmt; + src_code = &vdeb->src_code; } else { sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0); + src_code = &v4l2_subdev_get_try_format(sd, cfg, 1)->code; } /* @@ -253,9 +275,14 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, * it is propagated from the sink */ if (VIMC_IS_SRC(fmt->pad)) { + u32 code = fmt->format.code; + fmt->format = *sink_fmt; - /* TODO: Add support for other formats */ - fmt->format.code = vdeb->src_code; + + if (!vimc_deb_is_src_code_invalid(code)) + *src_code = code; + + fmt->format.code = *src_code; } else { /* Set the new format in the sink pad */ vimc_deb_adjust_sink_fmt(&fmt->format); @@ -291,11 +318,21 @@ static void vimc_deb_set_rgb_mbus_fmt_rgb888_1x24(struct vimc_deb_device *vdeb, unsigned int col, unsigned int rgb[3]) { + const struct vimc_pix_map *vpix; unsigned int i, index; + vpix = vimc_pix_map_by_code(vdeb->src_code); index = VIMC_FRAME_INDEX(lin, col, vdeb->sink_fmt.width, 3); - for (i = 0; i < 3; i++) - vdeb->src_frame[index + i] = rgb[i]; + for (i = 0; i < 3; i++) { + switch (vpix->pixelformat) { + case V4L2_PIX_FMT_RGB24: + vdeb->src_frame[index + i] = rgb[i]; + break; + case V4L2_PIX_FMT_BGR24: + vdeb->src_frame[index + i] = rgb[2-i]; + break; + } + } } static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable) -- 2.25.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] media: vimc: deb: Add support for {RGB,BGR,GBR}888 bus formats on source pad 2020-03-26 21:47 ` [PATCH v2 3/3] media: vimc: deb: Add support for {RGB,BGR,GBR}888 bus formats on source pad Nícolas F. R. A. Prado @ 2020-03-26 22:06 ` Shuah Khan 2020-03-30 19:43 ` Helen Koike 0 siblings, 1 reply; 15+ messages in thread From: Shuah Khan @ 2020-03-26 22:06 UTC (permalink / raw) To: Nícolas F. R. A. Prado, linux-media Cc: Helen Koike, Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, lkcamp, Shuah Khan On 3/26/20 3:47 PM, Nícolas F. R. A. Prado wrote: > Add support for RGB888_*, BGR888_* and GBR888_* media bus formats on > the source pad of debayer subdevices. > > Co-developed-by: Vitor Massaru Iha <vitor@massaru.org> > Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@protonmail.com> > --- > > Changes in v2: > - Change commit message to reflect v2 changes > - Rename variables > - Fix array formatting > - Add vimc_deb_is_src_code_valid function > - Add other BGR888 and RGB888 formats to debayer source pad supported > formats > > drivers/media/platform/vimc/vimc-debayer.c | 61 +++++++++++++++++----- > 1 file changed, 49 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c > index baf6bf9f65b5..33a9bea770bc 100644 > --- a/drivers/media/platform/vimc/vimc-debayer.c > +++ b/drivers/media/platform/vimc/vimc-debayer.c > @@ -51,6 +51,19 @@ static const struct v4l2_mbus_framefmt sink_fmt_default = { > .colorspace = V4L2_COLORSPACE_DEFAULT, > }; > > +static const u32 vimc_deb_src_mbus_codes[] = { > + MEDIA_BUS_FMT_GBR888_1X24, > + MEDIA_BUS_FMT_BGR888_1X24, > + MEDIA_BUS_FMT_BGR888_3X8, > + MEDIA_BUS_FMT_RGB888_1X24, > + MEDIA_BUS_FMT_RGB888_2X12_BE, > + MEDIA_BUS_FMT_RGB888_2X12_LE, > + MEDIA_BUS_FMT_RGB888_3X8, > + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, > + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, > + MEDIA_BUS_FMT_RGB888_1X32_PADHI, > +}; > + > static const struct vimc_deb_pix_map vimc_deb_pix_map_list[] = { > { > .code = MEDIA_BUS_FMT_SBGGR8_1X8, > @@ -125,6 +138,17 @@ static const struct vimc_deb_pix_map *vimc_deb_pix_map_by_code(u32 code) > return NULL; > } > > +static int vimc_deb_is_src_code_invalid(u32 code) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(vimc_deb_src_mbus_codes); i++) > + if (vimc_deb_src_mbus_codes[i] == code) > + return 0; > + > + return -EINVAL; > +} > + > static int vimc_deb_init_cfg(struct v4l2_subdev *sd, > struct v4l2_subdev_pad_config *cfg) > { > @@ -148,14 +172,11 @@ static int vimc_deb_enum_mbus_code(struct v4l2_subdev *sd, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_mbus_code_enum *code) > { > - /* We only support one format for source pads */ > if (VIMC_IS_SRC(code->pad)) { > - struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); > - > - if (code->index) > + if (code->index >= ARRAY_SIZE(vimc_deb_src_mbus_codes)) > return -EINVAL; > > - code->code = vdeb->src_code; > + code->code = vimc_deb_src_mbus_codes[code->index]; > } else { > if (code->index >= ARRAY_SIZE(vimc_deb_pix_map_list)) > return -EINVAL; > @@ -170,8 +191,6 @@ static int vimc_deb_enum_frame_size(struct v4l2_subdev *sd, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_frame_size_enum *fse) > { > - struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); > - > if (fse->index) > return -EINVAL; > > @@ -181,7 +200,7 @@ static int vimc_deb_enum_frame_size(struct v4l2_subdev *sd, > > if (!vpix) > return -EINVAL; > - } else if (fse->code != vdeb->src_code) { > + } else if (vimc_deb_is_src_code_invalid(fse->code)) { > return -EINVAL; > } > > @@ -237,6 +256,7 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, > { > struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); > struct v4l2_mbus_framefmt *sink_fmt; > + u32 *src_code; > > if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > /* Do not change the format while stream is on */ > @@ -244,8 +264,10 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, > return -EBUSY; > > sink_fmt = &vdeb->sink_fmt; > + src_code = &vdeb->src_code; > } else { > sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0); > + src_code = &v4l2_subdev_get_try_format(sd, cfg, 1)->code; > } > > /* > @@ -253,9 +275,14 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, > * it is propagated from the sink > */ > if (VIMC_IS_SRC(fmt->pad)) { > + u32 code = fmt->format.code; > + > fmt->format = *sink_fmt; > - /* TODO: Add support for other formats */ > - fmt->format.code = vdeb->src_code; > + > + if (!vimc_deb_is_src_code_invalid(code)) > + *src_code = code; > + > + fmt->format.code = *src_code; > } else { > /* Set the new format in the sink pad */ > vimc_deb_adjust_sink_fmt(&fmt->format); > @@ -291,11 +318,21 @@ static void vimc_deb_set_rgb_mbus_fmt_rgb888_1x24(struct vimc_deb_device *vdeb, > unsigned int col, > unsigned int rgb[3]) Change this to pass a pointer and size. > { > + const struct vimc_pix_map *vpix; > unsigned int i, index; > > + vpix = vimc_pix_map_by_code(vdeb->src_code); > index = VIMC_FRAME_INDEX(lin, col, vdeb->sink_fmt.width, 3); > - for (i = 0; i < 3; i++) > - vdeb->src_frame[index + i] = rgb[i]; > + for (i = 0; i < 3; i++) { > + switch (vpix->pixelformat) { > + case V4L2_PIX_FMT_RGB24: > + vdeb->src_frame[index + i] = rgb[i]; > + break; > + case V4L2_PIX_FMT_BGR24: > + vdeb->src_frame[index + i] = rgb[2-i]; > + break; > + } > + } > } > > static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable) > thanks, -- Shuah ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] media: vimc: deb: Add support for {RGB,BGR,GBR}888 bus formats on source pad 2020-03-26 22:06 ` Shuah Khan @ 2020-03-30 19:43 ` Helen Koike 2020-03-30 19:46 ` Shuah Khan 2020-04-20 21:01 ` Nícolas F. R. A. Prado 0 siblings, 2 replies; 15+ messages in thread From: Helen Koike @ 2020-03-30 19:43 UTC (permalink / raw) To: Shuah Khan, Nícolas F. R. A. Prado, linux-media Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, lkcamp Hello, On 3/26/20 7:06 PM, Shuah Khan wrote: > On 3/26/20 3:47 PM, Nícolas F. R. A. Prado wrote: >> Add support for RGB888_*, BGR888_* and GBR888_* media bus formats on >> the source pad of debayer subdevices. >> >> Co-developed-by: Vitor Massaru Iha <vitor@massaru.org> >> Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> >> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@protonmail.com> >> --- >> >> Changes in v2: >> - Change commit message to reflect v2 changes >> - Rename variables >> - Fix array formatting >> - Add vimc_deb_is_src_code_valid function >> - Add other BGR888 and RGB888 formats to debayer source pad supported >> formats >> >> drivers/media/platform/vimc/vimc-debayer.c | 61 +++++++++++++++++----- >> 1 file changed, 49 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c >> index baf6bf9f65b5..33a9bea770bc 100644 >> --- a/drivers/media/platform/vimc/vimc-debayer.c >> +++ b/drivers/media/platform/vimc/vimc-debayer.c >> @@ -51,6 +51,19 @@ static const struct v4l2_mbus_framefmt sink_fmt_default = { >> .colorspace = V4L2_COLORSPACE_DEFAULT, >> }; >> +static const u32 vimc_deb_src_mbus_codes[] = { >> + MEDIA_BUS_FMT_GBR888_1X24, >> + MEDIA_BUS_FMT_BGR888_1X24, >> + MEDIA_BUS_FMT_BGR888_3X8, >> + MEDIA_BUS_FMT_RGB888_1X24, >> + MEDIA_BUS_FMT_RGB888_2X12_BE, >> + MEDIA_BUS_FMT_RGB888_2X12_LE, >> + MEDIA_BUS_FMT_RGB888_3X8, >> + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, >> + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, >> + MEDIA_BUS_FMT_RGB888_1X32_PADHI, >> +}; >> + >> static const struct vimc_deb_pix_map vimc_deb_pix_map_list[] = { >> { >> .code = MEDIA_BUS_FMT_SBGGR8_1X8, >> @@ -125,6 +138,17 @@ static const struct vimc_deb_pix_map *vimc_deb_pix_map_by_code(u32 code) >> return NULL; >> } >> +static int vimc_deb_is_src_code_invalid(u32 code) >> +{ >> + unsigned int i; >> + >> + for (i = 0; i < ARRAY_SIZE(vimc_deb_src_mbus_codes); i++) >> + if (vimc_deb_src_mbus_codes[i] == code) >> + return 0; >> + >> + return -EINVAL; >> +} The naming is a bit confusing, since it checks if it is invalid, but returns a negative number if so. How about renaming to vimc_deb_src_code_is_valid ? >> + >> static int vimc_deb_init_cfg(struct v4l2_subdev *sd, >> struct v4l2_subdev_pad_config *cfg) >> { >> @@ -148,14 +172,11 @@ static int vimc_deb_enum_mbus_code(struct v4l2_subdev *sd, >> struct v4l2_subdev_pad_config *cfg, >> struct v4l2_subdev_mbus_code_enum *code) >> { >> - /* We only support one format for source pads */ >> if (VIMC_IS_SRC(code->pad)) { >> - struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); >> - >> - if (code->index) >> + if (code->index >= ARRAY_SIZE(vimc_deb_src_mbus_codes)) >> return -EINVAL; >> - code->code = vdeb->src_code; >> + code->code = vimc_deb_src_mbus_codes[code->index]; >> } else { >> if (code->index >= ARRAY_SIZE(vimc_deb_pix_map_list)) >> return -EINVAL; >> @@ -170,8 +191,6 @@ static int vimc_deb_enum_frame_size(struct v4l2_subdev *sd, >> struct v4l2_subdev_pad_config *cfg, >> struct v4l2_subdev_frame_size_enum *fse) >> { >> - struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); >> - >> if (fse->index) >> return -EINVAL; >> @@ -181,7 +200,7 @@ static int vimc_deb_enum_frame_size(struct v4l2_subdev *sd, >> if (!vpix) >> return -EINVAL; >> - } else if (fse->code != vdeb->src_code) { >> + } else if (vimc_deb_is_src_code_invalid(fse->code)) { >> return -EINVAL; >> } >> @@ -237,6 +256,7 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, >> { >> struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); >> struct v4l2_mbus_framefmt *sink_fmt; >> + u32 *src_code; >> if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) { >> /* Do not change the format while stream is on */ >> @@ -244,8 +264,10 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, >> return -EBUSY; >> sink_fmt = &vdeb->sink_fmt; >> + src_code = &vdeb->src_code; >> } else { >> sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0); >> + src_code = &v4l2_subdev_get_try_format(sd, cfg, 1)->code; >> } >> /* >> @@ -253,9 +275,14 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, >> * it is propagated from the sink >> */ >> if (VIMC_IS_SRC(fmt->pad)) { >> + u32 code = fmt->format.code; >> + >> fmt->format = *sink_fmt; >> - /* TODO: Add support for other formats */ >> - fmt->format.code = vdeb->src_code; >> + >> + if (!vimc_deb_is_src_code_invalid(code)) >> + *src_code = code; >> + >> + fmt->format.code = *src_code; >> } else { >> /* Set the new format in the sink pad */ >> vimc_deb_adjust_sink_fmt(&fmt->format); >> @@ -291,11 +318,21 @@ static void vimc_deb_set_rgb_mbus_fmt_rgb888_1x24(struct vimc_deb_device *vdeb, >> unsigned int col, >> unsigned int rgb[3]) > > Change this to pass a pointer and size. Hi Shuah, Modifying vimc_deb_set_rgb_mbus_fmt_rgb888_1x24() is not part of this patch, or do you mean another part of the code? Thanks for reviewing Helen > >> { >> + const struct vimc_pix_map *vpix; >> unsigned int i, index; >> + vpix = vimc_pix_map_by_code(vdeb->src_code); >> index = VIMC_FRAME_INDEX(lin, col, vdeb->sink_fmt.width, 3); >> - for (i = 0; i < 3; i++) >> - vdeb->src_frame[index + i] = rgb[i]; >> + for (i = 0; i < 3; i++) { >> + switch (vpix->pixelformat) { >> + case V4L2_PIX_FMT_RGB24: >> + vdeb->src_frame[index + i] = rgb[i]; >> + break; >> + case V4L2_PIX_FMT_BGR24: >> + vdeb->src_frame[index + i] = rgb[2-i]; >> + break; >> + } >> + } >> } >> static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable) >> > > thanks, > -- Shuah ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] media: vimc: deb: Add support for {RGB,BGR,GBR}888 bus formats on source pad 2020-03-30 19:43 ` Helen Koike @ 2020-03-30 19:46 ` Shuah Khan 2020-04-20 21:01 ` Nícolas F. R. A. Prado 1 sibling, 0 replies; 15+ messages in thread From: Shuah Khan @ 2020-03-30 19:46 UTC (permalink / raw) To: Helen Koike, Nícolas F. R. A. Prado, linux-media Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, lkcamp, Shuah Khan On 3/30/20 1:43 PM, Helen Koike wrote: > Hello, > > On 3/26/20 7:06 PM, Shuah Khan wrote: >> On 3/26/20 3:47 PM, Nícolas F. R. A. Prado wrote: >>> Add support for RGB888_*, BGR888_* and GBR888_* media bus formats on >>> the source pad of debayer subdevices. >>> >>> Co-developed-by: Vitor Massaru Iha <vitor@massaru.org> >>> Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> >>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@protonmail.com> >>> --- >>> >>> Changes in v2: >>> - Change commit message to reflect v2 changes >>> - Rename variables >>> - Fix array formatting >>> - Add vimc_deb_is_src_code_valid function >>> - Add other BGR888 and RGB888 formats to debayer source pad supported >>> formats >>> >>> drivers/media/platform/vimc/vimc-debayer.c | 61 +++++++++++++++++----- >>> 1 file changed, 49 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c >>> index baf6bf9f65b5..33a9bea770bc 100644 >>> --- a/drivers/media/platform/vimc/vimc-debayer.c >>> +++ b/drivers/media/platform/vimc/vimc-debayer.c >>> @@ -51,6 +51,19 @@ static const struct v4l2_mbus_framefmt sink_fmt_default = { >>> .colorspace = V4L2_COLORSPACE_DEFAULT, >>> }; >>> +static const u32 vimc_deb_src_mbus_codes[] = { >>> + MEDIA_BUS_FMT_GBR888_1X24, >>> + MEDIA_BUS_FMT_BGR888_1X24, >>> + MEDIA_BUS_FMT_BGR888_3X8, >>> + MEDIA_BUS_FMT_RGB888_1X24, >>> + MEDIA_BUS_FMT_RGB888_2X12_BE, >>> + MEDIA_BUS_FMT_RGB888_2X12_LE, >>> + MEDIA_BUS_FMT_RGB888_3X8, >>> + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, >>> + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, >>> + MEDIA_BUS_FMT_RGB888_1X32_PADHI, >>> +}; >>> + >>> static const struct vimc_deb_pix_map vimc_deb_pix_map_list[] = { >>> { >>> .code = MEDIA_BUS_FMT_SBGGR8_1X8, >>> @@ -125,6 +138,17 @@ static const struct vimc_deb_pix_map *vimc_deb_pix_map_by_code(u32 code) >>> return NULL; >>> } >>> +static int vimc_deb_is_src_code_invalid(u32 code) >>> +{ >>> + unsigned int i; >>> + >>> + for (i = 0; i < ARRAY_SIZE(vimc_deb_src_mbus_codes); i++) >>> + if (vimc_deb_src_mbus_codes[i] == code) >>> + return 0; >>> + >>> + return -EINVAL; >>> +} > > The naming is a bit confusing, since it checks if it is invalid, but returns a negative number if so. > > How about renaming to vimc_deb_src_code_is_valid ? > >>> + >>> static int vimc_deb_init_cfg(struct v4l2_subdev *sd, >>> struct v4l2_subdev_pad_config *cfg) >>> { >>> @@ -148,14 +172,11 @@ static int vimc_deb_enum_mbus_code(struct v4l2_subdev *sd, >>> struct v4l2_subdev_pad_config *cfg, >>> struct v4l2_subdev_mbus_code_enum *code) >>> { >>> - /* We only support one format for source pads */ >>> if (VIMC_IS_SRC(code->pad)) { >>> - struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); >>> - >>> - if (code->index) >>> + if (code->index >= ARRAY_SIZE(vimc_deb_src_mbus_codes)) >>> return -EINVAL; >>> - code->code = vdeb->src_code; >>> + code->code = vimc_deb_src_mbus_codes[code->index]; >>> } else { >>> if (code->index >= ARRAY_SIZE(vimc_deb_pix_map_list)) >>> return -EINVAL; >>> @@ -170,8 +191,6 @@ static int vimc_deb_enum_frame_size(struct v4l2_subdev *sd, >>> struct v4l2_subdev_pad_config *cfg, >>> struct v4l2_subdev_frame_size_enum *fse) >>> { >>> - struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); >>> - >>> if (fse->index) >>> return -EINVAL; >>> @@ -181,7 +200,7 @@ static int vimc_deb_enum_frame_size(struct v4l2_subdev *sd, >>> if (!vpix) >>> return -EINVAL; >>> - } else if (fse->code != vdeb->src_code) { >>> + } else if (vimc_deb_is_src_code_invalid(fse->code)) { >>> return -EINVAL; >>> } >>> @@ -237,6 +256,7 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, >>> { >>> struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); >>> struct v4l2_mbus_framefmt *sink_fmt; >>> + u32 *src_code; >>> if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) { >>> /* Do not change the format while stream is on */ >>> @@ -244,8 +264,10 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, >>> return -EBUSY; >>> sink_fmt = &vdeb->sink_fmt; >>> + src_code = &vdeb->src_code; >>> } else { >>> sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0); >>> + src_code = &v4l2_subdev_get_try_format(sd, cfg, 1)->code; >>> } >>> /* >>> @@ -253,9 +275,14 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, >>> * it is propagated from the sink >>> */ >>> if (VIMC_IS_SRC(fmt->pad)) { >>> + u32 code = fmt->format.code; >>> + >>> fmt->format = *sink_fmt; >>> - /* TODO: Add support for other formats */ >>> - fmt->format.code = vdeb->src_code; >>> + >>> + if (!vimc_deb_is_src_code_invalid(code)) >>> + *src_code = code; >>> + >>> + fmt->format.code = *src_code; >>> } else { >>> /* Set the new format in the sink pad */ >>> vimc_deb_adjust_sink_fmt(&fmt->format); >>> @@ -291,11 +318,21 @@ static void vimc_deb_set_rgb_mbus_fmt_rgb888_1x24(struct vimc_deb_device *vdeb, >>> unsigned int col, >>> unsigned int rgb[3]) >> >> Change this to pass a pointer and size. > > Hi Shuah, > > Modifying vimc_deb_set_rgb_mbus_fmt_rgb888_1x24() is not part of this patch, or do you mean another part of the code? > I know it isn't part of this patch. However, this could use improvment and pass pointer and size. Can be handled as a separate patch. thanks, -- Shuah ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] media: vimc: deb: Add support for {RGB,BGR,GBR}888 bus formats on source pad 2020-03-30 19:43 ` Helen Koike 2020-03-30 19:46 ` Shuah Khan @ 2020-04-20 21:01 ` Nícolas F. R. A. Prado 2020-04-20 21:04 ` Helen Koike 1 sibling, 1 reply; 15+ messages in thread From: Nícolas F. R. A. Prado @ 2020-04-20 21:01 UTC (permalink / raw) To: Helen Koike Cc: Shuah Khan, linux-media, Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, lkcamp Hi Helen, thanks for the review. Some comments below. On Mon, Mar 30, 2020 at 04:43:53PM -0300, Helen Koike wrote: > > Hello, > > On 3/26/20 7:06 PM, Shuah Khan wrote: > > On 3/26/20 3:47 PM, Nícolas F. R. A. Prado wrote: > >> Add support for RGB888_*, BGR888_* and GBR888_* media bus formats on > >> the source pad of debayer subdevices. > >> > >> Co-developed-by: Vitor Massaru Iha <vitor@massaru.org> > >> Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> > >> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@protonmail.com> > >> --- > >> > >> Changes in v2: > >> - Change commit message to reflect v2 changes > >> - Rename variables > >> - Fix array formatting > >> - Add vimc_deb_is_src_code_valid function > >> - Add other BGR888 and RGB888 formats to debayer source pad supported > >> formats > >> > >> drivers/media/platform/vimc/vimc-debayer.c | 61 +++++++++++++++++----- > >> 1 file changed, 49 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c > >> index baf6bf9f65b5..33a9bea770bc 100644 > >> --- a/drivers/media/platform/vimc/vimc-debayer.c > >> +++ b/drivers/media/platform/vimc/vimc-debayer.c > >> @@ -51,6 +51,19 @@ static const struct v4l2_mbus_framefmt sink_fmt_default = { > >> .colorspace = V4L2_COLORSPACE_DEFAULT, > >> }; > >> +static const u32 vimc_deb_src_mbus_codes[] = { > >> + MEDIA_BUS_FMT_GBR888_1X24, > >> + MEDIA_BUS_FMT_BGR888_1X24, > >> + MEDIA_BUS_FMT_BGR888_3X8, > >> + MEDIA_BUS_FMT_RGB888_1X24, > >> + MEDIA_BUS_FMT_RGB888_2X12_BE, > >> + MEDIA_BUS_FMT_RGB888_2X12_LE, > >> + MEDIA_BUS_FMT_RGB888_3X8, > >> + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, > >> + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, > >> + MEDIA_BUS_FMT_RGB888_1X32_PADHI, > >> +}; > >> + > >> static const struct vimc_deb_pix_map vimc_deb_pix_map_list[] = { > >> { > >> .code = MEDIA_BUS_FMT_SBGGR8_1X8, > >> @@ -125,6 +138,17 @@ static const struct vimc_deb_pix_map *vimc_deb_pix_map_by_code(u32 code) > >> return NULL; > >> } > >> +static int vimc_deb_is_src_code_invalid(u32 code) > >> +{ > >> + unsigned int i; > >> + > >> + for (i = 0; i < ARRAY_SIZE(vimc_deb_src_mbus_codes); i++) > >> + if (vimc_deb_src_mbus_codes[i] == code) > >> + return 0; > >> + > >> + return -EINVAL; > >> +} > > The naming is a bit confusing, since it checks if it is invalid, but returns a negative number if so. > > How about renaming to vimc_deb_src_code_is_valid ? I also don't like that the function is called 'is_invalid', but I gave it that name because I think it actually is less confusing when calling. For example, later in this patch I do: } else if (vimc_deb_is_src_code_invalid(fse->code)) { return -EINVAL; Which to me becomes very clear. Since the error values evaluate to True, the other alternative that I see is to call it 'is_valid', but return 0 when invalid and 1 when valid. But then we no longer return the -EINVAL value, which I think makes the function less clear. What do you think? Thank you, Nícolas > > >> + > >> static int vimc_deb_init_cfg(struct v4l2_subdev *sd, > >> struct v4l2_subdev_pad_config *cfg) > >> { > >> @@ -148,14 +172,11 @@ static int vimc_deb_enum_mbus_code(struct v4l2_subdev *sd, > >> struct v4l2_subdev_pad_config *cfg, > >> struct v4l2_subdev_mbus_code_enum *code) > >> { > >> - /* We only support one format for source pads */ > >> if (VIMC_IS_SRC(code->pad)) { > >> - struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); > >> - > >> - if (code->index) > >> + if (code->index >= ARRAY_SIZE(vimc_deb_src_mbus_codes)) > >> return -EINVAL; > >> - code->code = vdeb->src_code; > >> + code->code = vimc_deb_src_mbus_codes[code->index]; > >> } else { > >> if (code->index >= ARRAY_SIZE(vimc_deb_pix_map_list)) > >> return -EINVAL; > >> @@ -170,8 +191,6 @@ static int vimc_deb_enum_frame_size(struct v4l2_subdev *sd, > >> struct v4l2_subdev_pad_config *cfg, > >> struct v4l2_subdev_frame_size_enum *fse) > >> { > >> - struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); > >> - > >> if (fse->index) > >> return -EINVAL; > >> @@ -181,7 +200,7 @@ static int vimc_deb_enum_frame_size(struct v4l2_subdev *sd, > >> if (!vpix) > >> return -EINVAL; > >> - } else if (fse->code != vdeb->src_code) { > >> + } else if (vimc_deb_is_src_code_invalid(fse->code)) { > >> return -EINVAL; > >> } > >> @@ -237,6 +256,7 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, > >> { > >> struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); > >> struct v4l2_mbus_framefmt *sink_fmt; > >> + u32 *src_code; > >> if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > >> /* Do not change the format while stream is on */ > >> @@ -244,8 +264,10 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, > >> return -EBUSY; > >> sink_fmt = &vdeb->sink_fmt; > >> + src_code = &vdeb->src_code; > >> } else { > >> sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0); > >> + src_code = &v4l2_subdev_get_try_format(sd, cfg, 1)->code; > >> } > >> /* > >> @@ -253,9 +275,14 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, > >> * it is propagated from the sink > >> */ > >> if (VIMC_IS_SRC(fmt->pad)) { > >> + u32 code = fmt->format.code; > >> + > >> fmt->format = *sink_fmt; > >> - /* TODO: Add support for other formats */ > >> - fmt->format.code = vdeb->src_code; > >> + > >> + if (!vimc_deb_is_src_code_invalid(code)) > >> + *src_code = code; > >> + > >> + fmt->format.code = *src_code; > >> } else { > >> /* Set the new format in the sink pad */ > >> vimc_deb_adjust_sink_fmt(&fmt->format); > >> @@ -291,11 +318,21 @@ static void vimc_deb_set_rgb_mbus_fmt_rgb888_1x24(struct vimc_deb_device *vdeb, > >> unsigned int col, > >> unsigned int rgb[3]) > > > > Change this to pass a pointer and size. > > Hi Shuah, > > Modifying vimc_deb_set_rgb_mbus_fmt_rgb888_1x24() is not part of this patch, or do you mean another part of the code? > > Thanks for reviewing > Helen > > > > >> { > >> + const struct vimc_pix_map *vpix; > >> unsigned int i, index; > >> + vpix = vimc_pix_map_by_code(vdeb->src_code); > >> index = VIMC_FRAME_INDEX(lin, col, vdeb->sink_fmt.width, 3); > >> - for (i = 0; i < 3; i++) > >> - vdeb->src_frame[index + i] = rgb[i]; > >> + for (i = 0; i < 3; i++) { > >> + switch (vpix->pixelformat) { > >> + case V4L2_PIX_FMT_RGB24: > >> + vdeb->src_frame[index + i] = rgb[i]; > >> + break; > >> + case V4L2_PIX_FMT_BGR24: > >> + vdeb->src_frame[index + i] = rgb[2-i]; > >> + break; > >> + } > >> + } > >> } > >> static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable) > >> > > > > thanks, > > -- Shuah ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] media: vimc: deb: Add support for {RGB,BGR,GBR}888 bus formats on source pad 2020-04-20 21:01 ` Nícolas F. R. A. Prado @ 2020-04-20 21:04 ` Helen Koike 2020-04-27 22:12 ` Nícolas F. R. A. Prado 0 siblings, 1 reply; 15+ messages in thread From: Helen Koike @ 2020-04-20 21:04 UTC (permalink / raw) To: Nícolas F. R. A. Prado Cc: Shuah Khan, linux-media, Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, lkcamp On 4/20/20 6:01 PM, Nícolas F. R. A. Prado wrote: > Hi Helen, > > thanks for the review. > > Some comments below. > > On Mon, Mar 30, 2020 at 04:43:53PM -0300, Helen Koike wrote: >> >> Hello, >> >> On 3/26/20 7:06 PM, Shuah Khan wrote: >>> On 3/26/20 3:47 PM, Nícolas F. R. A. Prado wrote: >>>> Add support for RGB888_*, BGR888_* and GBR888_* media bus formats on >>>> the source pad of debayer subdevices. >>>> >>>> Co-developed-by: Vitor Massaru Iha <vitor@massaru.org> >>>> Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> >>>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@protonmail.com> >>>> --- >>>> >>>> Changes in v2: >>>> - Change commit message to reflect v2 changes >>>> - Rename variables >>>> - Fix array formatting >>>> - Add vimc_deb_is_src_code_valid function >>>> - Add other BGR888 and RGB888 formats to debayer source pad supported >>>> formats >>>> >>>> drivers/media/platform/vimc/vimc-debayer.c | 61 +++++++++++++++++----- >>>> 1 file changed, 49 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c >>>> index baf6bf9f65b5..33a9bea770bc 100644 >>>> --- a/drivers/media/platform/vimc/vimc-debayer.c >>>> +++ b/drivers/media/platform/vimc/vimc-debayer.c >>>> @@ -51,6 +51,19 @@ static const struct v4l2_mbus_framefmt sink_fmt_default = { >>>> .colorspace = V4L2_COLORSPACE_DEFAULT, >>>> }; >>>> +static const u32 vimc_deb_src_mbus_codes[] = { >>>> + MEDIA_BUS_FMT_GBR888_1X24, >>>> + MEDIA_BUS_FMT_BGR888_1X24, >>>> + MEDIA_BUS_FMT_BGR888_3X8, >>>> + MEDIA_BUS_FMT_RGB888_1X24, >>>> + MEDIA_BUS_FMT_RGB888_2X12_BE, >>>> + MEDIA_BUS_FMT_RGB888_2X12_LE, >>>> + MEDIA_BUS_FMT_RGB888_3X8, >>>> + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, >>>> + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, >>>> + MEDIA_BUS_FMT_RGB888_1X32_PADHI, >>>> +}; >>>> + >>>> static const struct vimc_deb_pix_map vimc_deb_pix_map_list[] = { >>>> { >>>> .code = MEDIA_BUS_FMT_SBGGR8_1X8, >>>> @@ -125,6 +138,17 @@ static const struct vimc_deb_pix_map *vimc_deb_pix_map_by_code(u32 code) >>>> return NULL; >>>> } >>>> +static int vimc_deb_is_src_code_invalid(u32 code) >>>> +{ >>>> + unsigned int i; >>>> + >>>> + for (i = 0; i < ARRAY_SIZE(vimc_deb_src_mbus_codes); i++) >>>> + if (vimc_deb_src_mbus_codes[i] == code) >>>> + return 0; >>>> + >>>> + return -EINVAL; >>>> +} >> >> The naming is a bit confusing, since it checks if it is invalid, but returns a negative number if so. >> >> How about renaming to vimc_deb_src_code_is_valid ? > > I also don't like that the function is called 'is_invalid', but I gave it that > name because I think it actually is less confusing when calling. > For example, later in this patch I do: > > } else if (vimc_deb_is_src_code_invalid(fse->code)) { > return -EINVAL; > > Which to me becomes very clear. > > Since the error values evaluate to True, the other alternative that I > see is to call it 'is_valid', but return 0 when invalid and 1 when valid. > But then we no longer return the -EINVAL value, which I think makes the function > less clear. > > What do you think? How about make the function to return bool instead of int? Regards, Helen > > Thank you, > Nícolas > >> >>>> + >>>> static int vimc_deb_init_cfg(struct v4l2_subdev *sd, >>>> struct v4l2_subdev_pad_config *cfg) >>>> { >>>> @@ -148,14 +172,11 @@ static int vimc_deb_enum_mbus_code(struct v4l2_subdev *sd, >>>> struct v4l2_subdev_pad_config *cfg, >>>> struct v4l2_subdev_mbus_code_enum *code) >>>> { >>>> - /* We only support one format for source pads */ >>>> if (VIMC_IS_SRC(code->pad)) { >>>> - struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); >>>> - >>>> - if (code->index) >>>> + if (code->index >= ARRAY_SIZE(vimc_deb_src_mbus_codes)) >>>> return -EINVAL; >>>> - code->code = vdeb->src_code; >>>> + code->code = vimc_deb_src_mbus_codes[code->index]; >>>> } else { >>>> if (code->index >= ARRAY_SIZE(vimc_deb_pix_map_list)) >>>> return -EINVAL; >>>> @@ -170,8 +191,6 @@ static int vimc_deb_enum_frame_size(struct v4l2_subdev *sd, >>>> struct v4l2_subdev_pad_config *cfg, >>>> struct v4l2_subdev_frame_size_enum *fse) >>>> { >>>> - struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); >>>> - >>>> if (fse->index) >>>> return -EINVAL; >>>> @@ -181,7 +200,7 @@ static int vimc_deb_enum_frame_size(struct v4l2_subdev *sd, >>>> if (!vpix) >>>> return -EINVAL; >>>> - } else if (fse->code != vdeb->src_code) { >>>> + } else if (vimc_deb_is_src_code_invalid(fse->code)) { >>>> return -EINVAL; >>>> } >>>> @@ -237,6 +256,7 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, >>>> { >>>> struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); >>>> struct v4l2_mbus_framefmt *sink_fmt; >>>> + u32 *src_code; >>>> if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) { >>>> /* Do not change the format while stream is on */ >>>> @@ -244,8 +264,10 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, >>>> return -EBUSY; >>>> sink_fmt = &vdeb->sink_fmt; >>>> + src_code = &vdeb->src_code; >>>> } else { >>>> sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0); >>>> + src_code = &v4l2_subdev_get_try_format(sd, cfg, 1)->code; >>>> } >>>> /* >>>> @@ -253,9 +275,14 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, >>>> * it is propagated from the sink >>>> */ >>>> if (VIMC_IS_SRC(fmt->pad)) { >>>> + u32 code = fmt->format.code; >>>> + >>>> fmt->format = *sink_fmt; >>>> - /* TODO: Add support for other formats */ >>>> - fmt->format.code = vdeb->src_code; >>>> + >>>> + if (!vimc_deb_is_src_code_invalid(code)) >>>> + *src_code = code; >>>> + >>>> + fmt->format.code = *src_code; >>>> } else { >>>> /* Set the new format in the sink pad */ >>>> vimc_deb_adjust_sink_fmt(&fmt->format); >>>> @@ -291,11 +318,21 @@ static void vimc_deb_set_rgb_mbus_fmt_rgb888_1x24(struct vimc_deb_device *vdeb, >>>> unsigned int col, >>>> unsigned int rgb[3]) >>> >>> Change this to pass a pointer and size. >> >> Hi Shuah, >> >> Modifying vimc_deb_set_rgb_mbus_fmt_rgb888_1x24() is not part of this patch, or do you mean another part of the code? >> >> Thanks for reviewing >> Helen >> >>> >>>> { >>>> + const struct vimc_pix_map *vpix; >>>> unsigned int i, index; >>>> + vpix = vimc_pix_map_by_code(vdeb->src_code); >>>> index = VIMC_FRAME_INDEX(lin, col, vdeb->sink_fmt.width, 3); >>>> - for (i = 0; i < 3; i++) >>>> - vdeb->src_frame[index + i] = rgb[i]; >>>> + for (i = 0; i < 3; i++) { >>>> + switch (vpix->pixelformat) { >>>> + case V4L2_PIX_FMT_RGB24: >>>> + vdeb->src_frame[index + i] = rgb[i]; >>>> + break; >>>> + case V4L2_PIX_FMT_BGR24: >>>> + vdeb->src_frame[index + i] = rgb[2-i]; >>>> + break; >>>> + } >>>> + } >>>> } >>>> static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable) >>>> >>> >>> thanks, >>> -- Shuah > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] media: vimc: deb: Add support for {RGB,BGR,GBR}888 bus formats on source pad 2020-04-20 21:04 ` Helen Koike @ 2020-04-27 22:12 ` Nícolas F. R. A. Prado 0 siblings, 0 replies; 15+ messages in thread From: Nícolas F. R. A. Prado @ 2020-04-27 22:12 UTC (permalink / raw) To: Helen Koike Cc: Shuah Khan, linux-media, Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, lkcamp On Mon, Apr 20, 2020 at 06:04:25PM -0300, Helen Koike wrote: > > > > On 4/20/20 6:01 PM, Nícolas F. R. A. Prado wrote: > > Hi Helen, > > > > thanks for the review. > > > > Some comments below. > > > > On Mon, Mar 30, 2020 at 04:43:53PM -0300, Helen Koike wrote: > >> > >> Hello, > >> > >> On 3/26/20 7:06 PM, Shuah Khan wrote: > >>> On 3/26/20 3:47 PM, Nícolas F. R. A. Prado wrote: > >>>> Add support for RGB888_*, BGR888_* and GBR888_* media bus formats on > >>>> the source pad of debayer subdevices. > >>>> > >>>> Co-developed-by: Vitor Massaru Iha <vitor@massaru.org> > >>>> Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> > >>>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@protonmail.com> > >>>> --- > >>>> > >>>> Changes in v2: > >>>> - Change commit message to reflect v2 changes > >>>> - Rename variables > >>>> - Fix array formatting > >>>> - Add vimc_deb_is_src_code_valid function > >>>> - Add other BGR888 and RGB888 formats to debayer source pad supported > >>>> formats > >>>> > >>>> drivers/media/platform/vimc/vimc-debayer.c | 61 +++++++++++++++++----- > >>>> 1 file changed, 49 insertions(+), 12 deletions(-) > >>>> > >>>> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c > >>>> index baf6bf9f65b5..33a9bea770bc 100644 > >>>> --- a/drivers/media/platform/vimc/vimc-debayer.c > >>>> +++ b/drivers/media/platform/vimc/vimc-debayer.c > >>>> @@ -51,6 +51,19 @@ static const struct v4l2_mbus_framefmt sink_fmt_default = { > >>>> .colorspace = V4L2_COLORSPACE_DEFAULT, > >>>> }; > >>>> +static const u32 vimc_deb_src_mbus_codes[] = { > >>>> + MEDIA_BUS_FMT_GBR888_1X24, > >>>> + MEDIA_BUS_FMT_BGR888_1X24, > >>>> + MEDIA_BUS_FMT_BGR888_3X8, > >>>> + MEDIA_BUS_FMT_RGB888_1X24, > >>>> + MEDIA_BUS_FMT_RGB888_2X12_BE, > >>>> + MEDIA_BUS_FMT_RGB888_2X12_LE, > >>>> + MEDIA_BUS_FMT_RGB888_3X8, > >>>> + MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, > >>>> + MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, > >>>> + MEDIA_BUS_FMT_RGB888_1X32_PADHI, > >>>> +}; > >>>> + > >>>> static const struct vimc_deb_pix_map vimc_deb_pix_map_list[] = { > >>>> { > >>>> .code = MEDIA_BUS_FMT_SBGGR8_1X8, > >>>> @@ -125,6 +138,17 @@ static const struct vimc_deb_pix_map *vimc_deb_pix_map_by_code(u32 code) > >>>> return NULL; > >>>> } > >>>> +static int vimc_deb_is_src_code_invalid(u32 code) > >>>> +{ > >>>> + unsigned int i; > >>>> + > >>>> + for (i = 0; i < ARRAY_SIZE(vimc_deb_src_mbus_codes); i++) > >>>> + if (vimc_deb_src_mbus_codes[i] == code) > >>>> + return 0; > >>>> + > >>>> + return -EINVAL; > >>>> +} > >> > >> The naming is a bit confusing, since it checks if it is invalid, but returns a negative number if so. > >> > >> How about renaming to vimc_deb_src_code_is_valid ? > > > > I also don't like that the function is called 'is_invalid', but I gave it that > > name because I think it actually is less confusing when calling. > > For example, later in this patch I do: > > > > } else if (vimc_deb_is_src_code_invalid(fse->code)) { > > return -EINVAL; > > > > Which to me becomes very clear. > > > > Since the error values evaluate to True, the other alternative that I > > see is to call it 'is_valid', but return 0 when invalid and 1 when valid. > > But then we no longer return the -EINVAL value, which I think makes the function > > less clear. > > > > What do you think? > > How about make the function to return bool instead of int? Oh that's better, I didn't realize there was a bool type. I'll send a v3 using bool then. Thank you, Nícolas > > Regards, > Helen > > > > > Thank you, > > Nícolas > > > >> > >>>> + > >>>> static int vimc_deb_init_cfg(struct v4l2_subdev *sd, > >>>> struct v4l2_subdev_pad_config *cfg) > >>>> { > >>>> @@ -148,14 +172,11 @@ static int vimc_deb_enum_mbus_code(struct v4l2_subdev *sd, > >>>> struct v4l2_subdev_pad_config *cfg, > >>>> struct v4l2_subdev_mbus_code_enum *code) > >>>> { > >>>> - /* We only support one format for source pads */ > >>>> if (VIMC_IS_SRC(code->pad)) { > >>>> - struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); > >>>> - > >>>> - if (code->index) > >>>> + if (code->index >= ARRAY_SIZE(vimc_deb_src_mbus_codes)) > >>>> return -EINVAL; > >>>> - code->code = vdeb->src_code; > >>>> + code->code = vimc_deb_src_mbus_codes[code->index]; > >>>> } else { > >>>> if (code->index >= ARRAY_SIZE(vimc_deb_pix_map_list)) > >>>> return -EINVAL; > >>>> @@ -170,8 +191,6 @@ static int vimc_deb_enum_frame_size(struct v4l2_subdev *sd, > >>>> struct v4l2_subdev_pad_config *cfg, > >>>> struct v4l2_subdev_frame_size_enum *fse) > >>>> { > >>>> - struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); > >>>> - > >>>> if (fse->index) > >>>> return -EINVAL; > >>>> @@ -181,7 +200,7 @@ static int vimc_deb_enum_frame_size(struct v4l2_subdev *sd, > >>>> if (!vpix) > >>>> return -EINVAL; > >>>> - } else if (fse->code != vdeb->src_code) { > >>>> + } else if (vimc_deb_is_src_code_invalid(fse->code)) { > >>>> return -EINVAL; > >>>> } > >>>> @@ -237,6 +256,7 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, > >>>> { > >>>> struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd); > >>>> struct v4l2_mbus_framefmt *sink_fmt; > >>>> + u32 *src_code; > >>>> if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > >>>> /* Do not change the format while stream is on */ > >>>> @@ -244,8 +264,10 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, > >>>> return -EBUSY; > >>>> sink_fmt = &vdeb->sink_fmt; > >>>> + src_code = &vdeb->src_code; > >>>> } else { > >>>> sink_fmt = v4l2_subdev_get_try_format(sd, cfg, 0); > >>>> + src_code = &v4l2_subdev_get_try_format(sd, cfg, 1)->code; > >>>> } > >>>> /* > >>>> @@ -253,9 +275,14 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd, > >>>> * it is propagated from the sink > >>>> */ > >>>> if (VIMC_IS_SRC(fmt->pad)) { > >>>> + u32 code = fmt->format.code; > >>>> + > >>>> fmt->format = *sink_fmt; > >>>> - /* TODO: Add support for other formats */ > >>>> - fmt->format.code = vdeb->src_code; > >>>> + > >>>> + if (!vimc_deb_is_src_code_invalid(code)) > >>>> + *src_code = code; > >>>> + > >>>> + fmt->format.code = *src_code; > >>>> } else { > >>>> /* Set the new format in the sink pad */ > >>>> vimc_deb_adjust_sink_fmt(&fmt->format); > >>>> @@ -291,11 +318,21 @@ static void vimc_deb_set_rgb_mbus_fmt_rgb888_1x24(struct vimc_deb_device *vdeb, > >>>> unsigned int col, > >>>> unsigned int rgb[3]) > >>> > >>> Change this to pass a pointer and size. > >> > >> Hi Shuah, > >> > >> Modifying vimc_deb_set_rgb_mbus_fmt_rgb888_1x24() is not part of this patch, or do you mean another part of the code? > >> > >> Thanks for reviewing > >> Helen > >> > >>> > >>>> { > >>>> + const struct vimc_pix_map *vpix; > >>>> unsigned int i, index; > >>>> + vpix = vimc_pix_map_by_code(vdeb->src_code); > >>>> index = VIMC_FRAME_INDEX(lin, col, vdeb->sink_fmt.width, 3); > >>>> - for (i = 0; i < 3; i++) > >>>> - vdeb->src_frame[index + i] = rgb[i]; > >>>> + for (i = 0; i < 3; i++) { > >>>> + switch (vpix->pixelformat) { > >>>> + case V4L2_PIX_FMT_RGB24: > >>>> + vdeb->src_frame[index + i] = rgb[i]; > >>>> + break; > >>>> + case V4L2_PIX_FMT_BGR24: > >>>> + vdeb->src_frame[index + i] = rgb[2-i]; > >>>> + break; > >>>> + } > >>>> + } > >>>> } > >>>> static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable) > >>>> > >>> > >>> thanks, > >>> -- Shuah > > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-04-27 22:12 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-26 21:47 [PATCH v2 0/3] media: vimc: Add support for {RGB,BGR,GBR}888 bus formats on debayer source pad Nícolas F. R. A. Prado
2020-03-26 21:47 ` [PATCH v2 1/3] media: vimc: Support multiple media bus codes for each pixelformat Nícolas F. R. A. Prado
2020-03-30 19:36 ` Helen Koike
2020-04-20 20:36 ` Nícolas F. R. A. Prado
2020-04-20 20:50 ` Helen Koike
2020-03-26 21:47 ` [PATCH v2 2/3] media: vimc: Add missing {RGB,BGR,GBR}888 media bus codes Nícolas F. R. A. Prado
2020-03-26 21:56 ` Shuah Khan
2020-03-30 19:37 ` Helen Koike
2020-03-26 21:47 ` [PATCH v2 3/3] media: vimc: deb: Add support for {RGB,BGR,GBR}888 bus formats on source pad Nícolas F. R. A. Prado
2020-03-26 22:06 ` Shuah Khan
2020-03-30 19:43 ` Helen Koike
2020-03-30 19:46 ` Shuah Khan
2020-04-20 21:01 ` Nícolas F. R. A. Prado
2020-04-20 21:04 ` Helen Koike
2020-04-27 22:12 ` Nícolas F. R. A. Prado
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).