Re: [Lkcamp] [PATCH v3 3/3] media: vimc: deb: Add support for {RGB, BGR, GBR}888 bus formats on source pad

[Helen Koike]

Hello,

On 4/28/20 4:46 AM, Dafna Hirschfeld wrote:
> hi,
> Thanks for the patches!
> 
> On 28.04.20 01:03, 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@xxxxxxxxxxx>
>> Signed-off-by: Vitor Massaru Iha <vitor@xxxxxxxxxxx>
>> Signed-off-by: NÃcolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxxx>
>> ---
>>
>> Changes in v3:
>> - Rename vimc_deb_is_src_code_invalid() to vimc_deb_src_code_is_valid()
>> - Change vimc_deb_src_code_is_valid() to return bool
>>
>> 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
>>
>> Â .../media/test-drivers/vimc/vimc-debayer.cÂÂÂ | 61 +++++++++++++++----
>> Â 1 file changed, 49 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
>> index d10aee9f84c4..7e87706d417e 100644
>> --- a/drivers/media/test-drivers/vimc/vimc-debayer.c
>> +++ b/drivers/media/test-drivers/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 bool vimc_deb_src_code_is_valid(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 true;
>> +
>> +ÂÂÂ return false;
>> +}
>> +
>> Â 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_src_code_is_valid(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_src_code_is_valid(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,
> I guess the name of the function should now change to vimc_deb_set_rgb_mbus_fmt ?
> Or better vimc_deb_process_rgb_frame.
> Also, it seems that it is a assigned as a callback so that each src_fmt have a different callback
> but you already did it with a switch case. So maybe you can add a patch to call it directly

Agreed that it should be renamed. Removing the callback could be done later (up to you NÃcolas).

With the rename, and with or without the callback removal:
Acked-by: Helen Koike <helen.koike@xxxxxxxxxxxxx>

> 
> Thanks,
> Dafna
> 
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 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)
>>
> 
> _______________________________________________
> Lkcamp mailing list
> Lkcamp@xxxxxxxxxxxxxxxxxxxxxxx
> https://lists.libreplanetbr.org/mailman/listinfo/lkcamp