Re: [PATCH v3 06/10] media: staging: rkisp1: add a helper function to enumerate supported mbus formats on capture

[Dafna Hirschfeld <dafna3@gmail.com>]

(resending as plain text)

On Sun, Aug 30, 2020 at 4:43 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> Hi Dafna,
>
> On Thu, Jul 23, 2020 at 03:20:10PM +0200, Dafna Hirschfeld wrote:
> > Add a function 'rkisp1_cap_enum_mbus_codes' that receive
> > a pointer to 'v4l2_subdev_mbus_code_enum' and returns the
> > next supported mbus format of the capture. The function
> > assumes that pixel formats with identical 'mbus' are grouped
> > together in the hardcoded arrays, therefore the order of the
> > entries in the array 'rkisp1_sp_fmts' are adjusted.
> > This function is a helper for the media bus enumeration of
> > the source pad of the resizer entity.
> >
> > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > ---
> >  drivers/staging/media/rkisp1/rkisp1-capture.c | 77 ++++++++++++-------
> >  drivers/staging/media/rkisp1/rkisp1-common.h  |  8 ++
> >  2 files changed, 58 insertions(+), 27 deletions(-)
> >
>
> Thank you for the patch. Please see my comments inline.
>
> > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> > index 5dd91b5f82a4..4dabd07a3da9 100644
> > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> > @@ -112,6 +112,13 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
> >               .write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> >               .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >       },
> > +     /* yuv400 */
> > +     {
> > +             .fourcc = V4L2_PIX_FMT_GREY,
> > +             .uv_swap = 0,
> > +             .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> > +             .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > +     },
> >       /* yuv420 */
> >       {
> >               .fourcc = V4L2_PIX_FMT_NV21,
> > @@ -144,13 +151,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
> >               .write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> >               .mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> >       },
> > -     /* yuv400 */
> > -     {
> > -             .fourcc = V4L2_PIX_FMT_GREY,
> > -             .uv_swap = 0,
> > -             .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> > -             .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > -     },
> >       /* raw */
> >       {
> >               .fourcc = V4L2_PIX_FMT_SRGGB8,
> > @@ -236,6 +236,26 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
> >               .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> >               .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >       },
> > +     /* yuv400 */
> > +     {
> > +             .fourcc = V4L2_PIX_FMT_GREY,
> > +             .uv_swap = 0,
> > +             .write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
> > +             .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
> > +             .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > +     },
> > +     /* rgb */
> > +     {
> > +             .fourcc = V4L2_PIX_FMT_XBGR32,
> > +             .write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > +             .output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
> > +             .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > +     }, {
> > +             .fourcc = V4L2_PIX_FMT_RGB565,
> > +             .write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > +             .output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
> > +             .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > +     },
>
> Is there any reason to move RGB formats here rather than keeping them at
> the end of the list, after all YUV formats?

Hi,
yes, the new function 'rkisp1_cap_enum_mbus_codes' assumes that
pixelformats with
identical mbus codes are grouped together. It uses it to iterate the
formats and increment
a counter when encountering a new mbus.

>
> >       /* yuv420 */
> >       {
> >               .fourcc = V4L2_PIX_FMT_NV21,
> > @@ -274,26 +294,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
> >               .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> >               .mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> >       },
> > -     /* yuv400 */
> > -     {
> > -             .fourcc = V4L2_PIX_FMT_GREY,
> > -             .uv_swap = 0,
> > -             .write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
> > -             .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
> > -             .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > -     },
> > -     /* rgb */
> > -     {
> > -             .fourcc = V4L2_PIX_FMT_XBGR32,
> > -             .write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > -             .output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
> > -             .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > -     }, {
> > -             .fourcc = V4L2_PIX_FMT_RGB565,
> > -             .write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > -             .output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
> > -             .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > -     },
> >  };
> >
> >  static const struct rkisp1_capture_config rkisp1_capture_config_mp = {
> > @@ -334,6 +334,29 @@ rkisp1_vdev_to_node(struct video_device *vdev)
> >       return container_of(vdev, struct rkisp1_vdev_node, vdev);
> >  }
> >
> > +int rkisp1_cap_enum_mbus_codes(struct rkisp1_capture *cap,
> > +                            struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > +     const struct rkisp1_capture_fmt_cfg *fmts = cap->config->fmts;
> > +     u32 curr_mbus = fmts[0].mbus;
> > +     int i, n = 0;
> > +
> > +     if (code->index == 0) {
> > +             code->code = fmts[0].mbus;
> > +             return 0;
> > +     }
> > +
> > +     for (i = 1; i < cap->config->fmt_size; i++)
>
> How about just initializing curr_mbus to MEDIA_BUS_FMT_FIXED? This is not
> supposed to be found in the array, so is a safe initial value, which would
> allow removing the explicit handling of index == 0.

yes, I'll do that.

>
> > +             if (fmts[i].mbus != curr_mbus) {
>
> As Helen mentioned, we could use the continue keyword to skip the iteration
> early and make it obvious that rest of the loop is entirely for the case
> where mbus != curr_mbus, removing one level of nesting.
>

that too,

> > +                     curr_mbus = fmts[i].mbus;
> > +                     if (++n == code->index) {
> > +                             code->code = curr_mbus;
> > +                             return 0;
> > +                     }
> > +             }
>
> According to the kernel coding style guidelines [1], this for loop should
> have braces.
>
> [1] https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces
>
thanks, I didn't know that.



Thanks,
Dafna

> Best regards,
> Tomasz