| Message ID | 20200723132014.4597-7-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: staging: rkisp1: add support to V4L2_CAP_IO_MC | expand |
Hi Dafna, Thanks for the patch. On 7/23/20 10:20 AM, 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(-) > > 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[] = { I would also add a comment in the beginning of this array saying that mbus should be grouped together for the sake of enum. > .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, > + }, > /* 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; Maybe you could use code->code as the current, since in case of error -EINVAL should be returned and this value should be ignored iirc. > + int i, n = 0; unsigned > + > + if (code->index == 0) { if (!code->index) > + code->code = fmts[0].mbus; > + return 0; > + } > + > + for (i = 1; i < cap->config->fmt_size; i++) > + if (fmts[i].mbus != curr_mbus) { You can decrease one indentation level with: if (fmts[i].mbus == curr_mbus) continue; Regards, Helen > + curr_mbus = fmts[i].mbus; > + if (++n == code->index) { > + code->code = curr_mbus; > + return 0; > + } > + } > + return -EINVAL; > +} > + > /* ---------------------------------------------------------------------------- > * Stream operations for self-picture path (sp) and main-picture path (mp) > */ > diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h > index 3dc51d703f73..73e1963cc47a 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-common.h > +++ b/drivers/staging/media/rkisp1/rkisp1-common.h > @@ -297,6 +297,14 @@ static inline u32 rkisp1_read(struct rkisp1_device *rkisp1, unsigned int addr) > return readl(rkisp1->base_addr + addr); > } > > +/* > + * rkisp1_cap_enum_mbus_codes - A helper function that return the i'th supported mbus code > + * of the capture entity. This is used to enumerate the supported > + * mbus codes on the source pad of the resizer. > + */ > +int rkisp1_cap_enum_mbus_codes(struct rkisp1_capture *cap, > + struct v4l2_subdev_mbus_code_enum *code); > + > void rkisp1_sd_adjust_crop_rect(struct v4l2_rect *crop, > const struct v4l2_rect *bounds); > >
Am 04.08.20 um 01:49 schrieb Helen Koike: > Hi Dafna, > > Thanks for the patch. > > On 7/23/20 10:20 AM, 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(-) >> >> 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[] = { > > I would also add a comment in the beginning of this array saying that mbus > should be grouped together for the sake of enum. > >> .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, >> + }, >> /* 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; > > Maybe you could use code->code as the current, since in case of error > -EINVAL should be returned and this value should be ignored iirc. Hi, but code->code is an unknown value. The aim of this function is to fill it according to code->index. Thanks, Dafna > >> + int i, n = 0; > > unsigned > >> + >> + if (code->index == 0) { > > if (!code->index) > >> + code->code = fmts[0].mbus; >> + return 0; >> + } >> + >> + for (i = 1; i < cap->config->fmt_size; i++) >> + if (fmts[i].mbus != curr_mbus) { > > You can decrease one indentation level with: > > if (fmts[i].mbus == curr_mbus) > continue; > > Regards, > Helen > >> + curr_mbus = fmts[i].mbus; >> + if (++n == code->index) { >> + code->code = curr_mbus; >> + return 0; >> + } >> + } >> + return -EINVAL; >> +} >> + >> /* ---------------------------------------------------------------------------- >> * Stream operations for self-picture path (sp) and main-picture path (mp) >> */ >> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h >> index 3dc51d703f73..73e1963cc47a 100644 >> --- a/drivers/staging/media/rkisp1/rkisp1-common.h >> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h >> @@ -297,6 +297,14 @@ static inline u32 rkisp1_read(struct rkisp1_device *rkisp1, unsigned int addr) >> return readl(rkisp1->base_addr + addr); >> } >> >> +/* >> + * rkisp1_cap_enum_mbus_codes - A helper function that return the i'th supported mbus code >> + * of the capture entity. This is used to enumerate the supported >> + * mbus codes on the source pad of the resizer. >> + */ >> +int rkisp1_cap_enum_mbus_codes(struct rkisp1_capture *cap, >> + struct v4l2_subdev_mbus_code_enum *code); >> + >> void rkisp1_sd_adjust_crop_rect(struct v4l2_rect *crop, >> const struct v4l2_rect *bounds); >> >>
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? > /* 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. > + 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. > + 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 Best regards, Tomasz
(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
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, + }, /* 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++) + if (fmts[i].mbus != curr_mbus) { + curr_mbus = fmts[i].mbus; + if (++n == code->index) { + code->code = curr_mbus; + return 0; + } + } + return -EINVAL; +} + /* ---------------------------------------------------------------------------- * Stream operations for self-picture path (sp) and main-picture path (mp) */ diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h index 3dc51d703f73..73e1963cc47a 100644 --- a/drivers/staging/media/rkisp1/rkisp1-common.h +++ b/drivers/staging/media/rkisp1/rkisp1-common.h @@ -297,6 +297,14 @@ static inline u32 rkisp1_read(struct rkisp1_device *rkisp1, unsigned int addr) return readl(rkisp1->base_addr + addr); } +/* + * rkisp1_cap_enum_mbus_codes - A helper function that return the i'th supported mbus code + * of the capture entity. This is used to enumerate the supported + * mbus codes on the source pad of the resizer. + */ +int rkisp1_cap_enum_mbus_codes(struct rkisp1_capture *cap, + struct v4l2_subdev_mbus_code_enum *code); + void rkisp1_sd_adjust_crop_rect(struct v4l2_rect *crop, const struct v4l2_rect *bounds);
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(-)