| Message ID | 20200515142952.20163-3-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: staging: rkisp1: various fixes to capture formats | expand |
Hi Dafna, On 5/15/20 11:29 AM, Dafna Hirschfeld wrote: > The resize block of rkisp1 always get the input as yuv422. > Instead of defining the default hdiv, vdiv as macros, the > code is more clear if it takes the (hv)div from the YUV422P > info. This makes it clear where those values come from. > The patch also adds documentation to explain that. > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> Acked-by: Helen Koike <helen.koike@collabora.com> Thanks! Helen > --- > drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++---------- > 1 file changed, 12 insertions(+), 13 deletions(-) > > diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c > index d049374413dc..04a29af8cc92 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c > +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c > @@ -16,9 +16,6 @@ > #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8 > #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV > > -#define RKISP1_MBUS_FMT_HDIV 2 > -#define RKISP1_MBUS_FMT_VDIV 1 > - > enum rkisp1_shadow_regs_when { > RKISP1_SHADOW_REGS_SYNC, > RKISP1_SHADOW_REGS_ASYNC, > @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz, > static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > enum rkisp1_shadow_regs_when when) > { > - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV; > struct v4l2_rect sink_y, sink_c, src_y, src_c; > struct v4l2_mbus_framefmt *src_fmt; > struct v4l2_rect *sink_crop; > struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id]; > + const struct v4l2_format_info *yuv422_info = > + v4l2_format_info(V4L2_PIX_FMT_YUV422P); > > sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK, > V4L2_SUBDEV_FORMAT_ACTIVE); > @@ -386,8 +384,9 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > src_y.width = src_fmt->width; > src_y.height = src_fmt->height; > > - sink_c.width = sink_y.width / RKISP1_MBUS_FMT_HDIV; > - sink_c.height = sink_y.height / RKISP1_MBUS_FMT_VDIV; > + /* The input format of the resizer is always yuv422 */ > + sink_c.width = sink_y.width / yuv422_info->hdiv; > + sink_c.height = sink_y.height / yuv422_info->vdiv; > > /* > * The resizer is used not only to change the dimensions of the frame > @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > * (4:2:2 -> 4:2:0 for example). So the width/height of the CbCr > * streams should be set according to the pixel format in the capture. > * The resizer always gets the input as YUV422. If the capture format > - * is RGB then the memory input should be YUV422 so we don't change the > - * default hdiv, vdiv in that case. > + * is RGB then the memory input (the resizer output) should be YUV422 > + * so we use the hdiv, vdiv of the YUV422 info in this case. > */ > if (v4l2_is_format_yuv(cap->pix.info)) { > - hdiv = cap->pix.info->hdiv; > - vdiv = cap->pix.info->vdiv; > + src_c.width = cap->pix.info->hdiv; > + src_c.height = cap->pix.info->vdiv; > + } else { > + src_c.width = src_y.width / yuv422_info->hdiv; > + src_c.height = src_y.height / yuv422_info->vdiv; > } > > - src_c.width = src_y.width / hdiv; > - src_c.height = src_y.height / vdiv; > - > if (sink_c.width == src_c.width && sink_c.height == src_c.height) { > rkisp1_rsz_disable(rsz, when); > return; >
On 5/20/20 6:54 PM, Helen Koike wrote: > Hi Dafna, > > On 5/15/20 11:29 AM, Dafna Hirschfeld wrote: >> The resize block of rkisp1 always get the input as yuv422. >> Instead of defining the default hdiv, vdiv as macros, the >> code is more clear if it takes the (hv)div from the YUV422P >> info. This makes it clear where those values come from. >> The patch also adds documentation to explain that. >> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > > Acked-by: Helen Koike <helen.koike@collabora.com> > > Thanks! > Helen > >> --- >> drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++---------- >> 1 file changed, 12 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c >> index d049374413dc..04a29af8cc92 100644 >> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c >> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c >> @@ -16,9 +16,6 @@ >> #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8 >> #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV >> >> -#define RKISP1_MBUS_FMT_HDIV 2 >> -#define RKISP1_MBUS_FMT_VDIV 1 >> - >> enum rkisp1_shadow_regs_when { >> RKISP1_SHADOW_REGS_SYNC, >> RKISP1_SHADOW_REGS_ASYNC, >> @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz, >> static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >> enum rkisp1_shadow_regs_when when) >> { >> - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV; >> struct v4l2_rect sink_y, sink_c, src_y, src_c; >> struct v4l2_mbus_framefmt *src_fmt; >> struct v4l2_rect *sink_crop; >> struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id]; >> + const struct v4l2_format_info *yuv422_info = >> + v4l2_format_info(V4L2_PIX_FMT_YUV422P); >> >> sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK, >> V4L2_SUBDEV_FORMAT_ACTIVE); >> @@ -386,8 +384,9 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >> src_y.width = src_fmt->width; >> src_y.height = src_fmt->height; >> >> - sink_c.width = sink_y.width / RKISP1_MBUS_FMT_HDIV; >> - sink_c.height = sink_y.height / RKISP1_MBUS_FMT_VDIV; >> + /* The input format of the resizer is always yuv422 */ >> + sink_c.width = sink_y.width / yuv422_info->hdiv; >> + sink_c.height = sink_y.height / yuv422_info->vdiv; >> >> /* >> * The resizer is used not only to change the dimensions of the frame >> @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >> * (4:2:2 -> 4:2:0 for example). So the width/height of the CbCr >> * streams should be set according to the pixel format in the capture. >> * The resizer always gets the input as YUV422. If the capture format >> - * is RGB then the memory input should be YUV422 so we don't change the >> - * default hdiv, vdiv in that case. >> + * is RGB then the memory input (the resizer output) should be YUV422 >> + * so we use the hdiv, vdiv of the YUV422 info in this case. >> */ >> if (v4l2_is_format_yuv(cap->pix.info)) { >> - hdiv = cap->pix.info->hdiv; >> - vdiv = cap->pix.info->vdiv; >> + src_c.width = cap->pix.info->hdiv; >> + src_c.height = cap->pix.info->vdiv; Sorry, I just noticed you are assigning hdiv and vdiv directly to width and height, which looks wrong. It should be: src_c.width = src_y.width / cap->pix.info->hdiv; src_c.height = src_y.height / cap->pix.info->vdiv; Regards, Helen >> + } else { >> + src_c.width = src_y.width / yuv422_info->hdiv; >> + src_c.height = src_y.height / yuv422_info->vdiv; >> } >> >> - src_c.width = src_y.width / hdiv; >> - src_c.height = src_y.height / vdiv; >> - >> if (sink_c.width == src_c.width && sink_c.height == src_c.height) { >> rkisp1_rsz_disable(rsz, when); >> return; >>
On 21.05.20 00:08, Helen Koike wrote: > > > On 5/20/20 6:54 PM, Helen Koike wrote: >> Hi Dafna, >> >> On 5/15/20 11:29 AM, Dafna Hirschfeld wrote: >>> The resize block of rkisp1 always get the input as yuv422. >>> Instead of defining the default hdiv, vdiv as macros, the >>> code is more clear if it takes the (hv)div from the YUV422P >>> info. This makes it clear where those values come from. >>> The patch also adds documentation to explain that. >>> >>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >> >> Acked-by: Helen Koike <helen.koike@collabora.com> >> >> Thanks! >> Helen >> >>> --- >>> drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++---------- >>> 1 file changed, 12 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c >>> index d049374413dc..04a29af8cc92 100644 >>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c >>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c >>> @@ -16,9 +16,6 @@ >>> #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8 >>> #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV >>> >>> -#define RKISP1_MBUS_FMT_HDIV 2 >>> -#define RKISP1_MBUS_FMT_VDIV 1 >>> - >>> enum rkisp1_shadow_regs_when { >>> RKISP1_SHADOW_REGS_SYNC, >>> RKISP1_SHADOW_REGS_ASYNC, >>> @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz, >>> static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >>> enum rkisp1_shadow_regs_when when) >>> { >>> - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV; >>> struct v4l2_rect sink_y, sink_c, src_y, src_c; >>> struct v4l2_mbus_framefmt *src_fmt; >>> struct v4l2_rect *sink_crop; >>> struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id]; >>> + const struct v4l2_format_info *yuv422_info = >>> + v4l2_format_info(V4L2_PIX_FMT_YUV422P); >>> >>> sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK, >>> V4L2_SUBDEV_FORMAT_ACTIVE); >>> @@ -386,8 +384,9 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >>> src_y.width = src_fmt->width; >>> src_y.height = src_fmt->height; >>> >>> - sink_c.width = sink_y.width / RKISP1_MBUS_FMT_HDIV; >>> - sink_c.height = sink_y.height / RKISP1_MBUS_FMT_VDIV; >>> + /* The input format of the resizer is always yuv422 */ >>> + sink_c.width = sink_y.width / yuv422_info->hdiv; >>> + sink_c.height = sink_y.height / yuv422_info->vdiv; >>> >>> /* >>> * The resizer is used not only to change the dimensions of the frame >>> @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >>> * (4:2:2 -> 4:2:0 for example). So the width/height of the CbCr >>> * streams should be set according to the pixel format in the capture. >>> * The resizer always gets the input as YUV422. If the capture format >>> - * is RGB then the memory input should be YUV422 so we don't change the >>> - * default hdiv, vdiv in that case. >>> + * is RGB then the memory input (the resizer output) should be YUV422 >>> + * so we use the hdiv, vdiv of the YUV422 info in this case. >>> */ >>> if (v4l2_is_format_yuv(cap->pix.info)) { >>> - hdiv = cap->pix.info->hdiv; >>> - vdiv = cap->pix.info->vdiv; >>> + src_c.width = cap->pix.info->hdiv; >>> + src_c.height = cap->pix.info->vdiv; > > Sorry, I just noticed you are assigning hdiv and vdiv directly to width and height, which looks wrong. > > It should be: > > src_c.width = src_y.width / cap->pix.info->hdiv; > src_c.height = src_y.height / cap->pix.info->vdiv; autch, thanks for finding it, I probably concentrated only on testing the new RGB formats Thanks, Dafna > > Regards, > Helen > >>> + } else { >>> + src_c.width = src_y.width / yuv422_info->hdiv; >>> + src_c.height = src_y.height / yuv422_info->vdiv; >>> } >>> >>> - src_c.width = src_y.width / hdiv; >>> - src_c.height = src_y.height / vdiv; >>> - >>> if (sink_c.width == src_c.width && sink_c.height == src_c.height) { >>> rkisp1_rsz_disable(rsz, when); >>> return; >>>
Hi Dafna, Helen, On Fri, 2020-05-22 at 14:11 +0200, Dafna Hirschfeld wrote: > > On 21.05.20 00:08, Helen Koike wrote: > > > > On 5/20/20 6:54 PM, Helen Koike wrote: > > > Hi Dafna, > > > > > > On 5/15/20 11:29 AM, Dafna Hirschfeld wrote: > > > > The resize block of rkisp1 always get the input as yuv422. > > > > Instead of defining the default hdiv, vdiv as macros, the > > > > code is more clear if it takes the (hv)div from the YUV422P > > > > info. This makes it clear where those values come from. > > > > The patch also adds documentation to explain that. > > > > > > > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > > > > > > Acked-by: Helen Koike <helen.koike@collabora.com> > > > > > > Thanks! > > > Helen > > > > > > > --- > > > > drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++---------- > > > > 1 file changed, 12 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c > > > > index d049374413dc..04a29af8cc92 100644 > > > > --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c > > > > +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c > > > > @@ -16,9 +16,6 @@ > > > > #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8 > > > > #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV > > > > > > > > -#define RKISP1_MBUS_FMT_HDIV 2 > > > > -#define RKISP1_MBUS_FMT_VDIV 1 > > > > - > > > > enum rkisp1_shadow_regs_when { > > > > RKISP1_SHADOW_REGS_SYNC, > > > > RKISP1_SHADOW_REGS_ASYNC, > > > > @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz, > > > > static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > > > > enum rkisp1_shadow_regs_when when) > > > > { > > > > - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV; > > > > struct v4l2_rect sink_y, sink_c, src_y, src_c; > > > > struct v4l2_mbus_framefmt *src_fmt; > > > > struct v4l2_rect *sink_crop; > > > > struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id]; > > > > + const struct v4l2_format_info *yuv422_info = > > > > + v4l2_format_info(V4L2_PIX_FMT_YUV422P); > > > > Instead of hardcoding this fourcc, is there any way we can retrieve it from a configured format? Thanks, Ezequiel
Hi Dafna, On Fri, May 22, 2020 at 02:11:22PM +0200, Dafna Hirschfeld wrote: > On 21.05.20 00:08, Helen Koike wrote: > > On 5/20/20 6:54 PM, Helen Koike wrote: > >> On 5/15/20 11:29 AM, Dafna Hirschfeld wrote: > >>> The resize block of rkisp1 always get the input as yuv422. > >>> Instead of defining the default hdiv, vdiv as macros, the > >>> code is more clear if it takes the (hv)div from the YUV422P > >>> info. This makes it clear where those values come from. > >>> The patch also adds documentation to explain that. > >>> > >>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > >> > >> Acked-by: Helen Koike <helen.koike@collabora.com> > >> > >>> --- > >>> drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++---------- > >>> 1 file changed, 12 insertions(+), 13 deletions(-) > >>> > >>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c > >>> index d049374413dc..04a29af8cc92 100644 > >>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c > >>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c > >>> @@ -16,9 +16,6 @@ > >>> #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8 > >>> #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV > >>> > >>> -#define RKISP1_MBUS_FMT_HDIV 2 > >>> -#define RKISP1_MBUS_FMT_VDIV 1 > >>> - > >>> enum rkisp1_shadow_regs_when { > >>> RKISP1_SHADOW_REGS_SYNC, > >>> RKISP1_SHADOW_REGS_ASYNC, > >>> @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz, > >>> static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > >>> enum rkisp1_shadow_regs_when when) > >>> { > >>> - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV; > >>> struct v4l2_rect sink_y, sink_c, src_y, src_c; > >>> struct v4l2_mbus_framefmt *src_fmt; > >>> struct v4l2_rect *sink_crop; > >>> struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id]; > >>> + const struct v4l2_format_info *yuv422_info = > >>> + v4l2_format_info(V4L2_PIX_FMT_YUV422P); > >>> > >>> sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK, > >>> V4L2_SUBDEV_FORMAT_ACTIVE); > >>> @@ -386,8 +384,9 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > >>> src_y.width = src_fmt->width; > >>> src_y.height = src_fmt->height; > >>> > >>> - sink_c.width = sink_y.width / RKISP1_MBUS_FMT_HDIV; > >>> - sink_c.height = sink_y.height / RKISP1_MBUS_FMT_VDIV; > >>> + /* The input format of the resizer is always yuv422 */ > >>> + sink_c.width = sink_y.width / yuv422_info->hdiv; > >>> + sink_c.height = sink_y.height / yuv422_info->vdiv; > >>> > >>> /* > >>> * The resizer is used not only to change the dimensions of the frame > >>> @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > >>> * (4:2:2 -> 4:2:0 for example). So the width/height of the CbCr > >>> * streams should be set according to the pixel format in the capture. > >>> * The resizer always gets the input as YUV422. If the capture format > >>> - * is RGB then the memory input should be YUV422 so we don't change the > >>> - * default hdiv, vdiv in that case. > >>> + * is RGB then the memory input (the resizer output) should be YUV422 > >>> + * so we use the hdiv, vdiv of the YUV422 info in this case. > >>> */ > >>> if (v4l2_is_format_yuv(cap->pix.info)) { > >>> - hdiv = cap->pix.info->hdiv; > >>> - vdiv = cap->pix.info->vdiv; > >>> + src_c.width = cap->pix.info->hdiv; > >>> + src_c.height = cap->pix.info->vdiv; > > > > Sorry, I just noticed you are assigning hdiv and vdiv directly to width and height, which looks wrong. > > > > It should be: > > > > src_c.width = src_y.width / cap->pix.info->hdiv; > > src_c.height = src_y.height / cap->pix.info->vdiv; > > autch, thanks for finding it, I probably concentrated only on testing the new RGB formats Please make sure to test all supported formats :-) I really, really recommend writing a small set of test scripts that will automate the tests for you. It can be as simple as wrapping media-ctl + yavta (or v4l2-ctl), or the libcamera cam utility if it offers all the features you need, and run them for all supported formats. > >>> + } else { > >>> + src_c.width = src_y.width / yuv422_info->hdiv; > >>> + src_c.height = src_y.height / yuv422_info->vdiv; > >>> } > >>> > >>> - src_c.width = src_y.width / hdiv; > >>> - src_c.height = src_y.height / vdiv; > >>> - > >>> if (sink_c.width == src_c.width && sink_c.height == src_c.height) { > >>> rkisp1_rsz_disable(rsz, when); > >>> return; > >>>
On 22.05.20 15:31, Ezequiel Garcia wrote: > Hi Dafna, Helen, > > On Fri, 2020-05-22 at 14:11 +0200, Dafna Hirschfeld wrote: >> >> On 21.05.20 00:08, Helen Koike wrote: >>> >>> On 5/20/20 6:54 PM, Helen Koike wrote: >>>> Hi Dafna, >>>> >>>> On 5/15/20 11:29 AM, Dafna Hirschfeld wrote: >>>>> The resize block of rkisp1 always get the input as yuv422. >>>>> Instead of defining the default hdiv, vdiv as macros, the >>>>> code is more clear if it takes the (hv)div from the YUV422P >>>>> info. This makes it clear where those values come from. >>>>> The patch also adds documentation to explain that. >>>>> >>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>>> >>>> Acked-by: Helen Koike <helen.koike@collabora.com> >>>> >>>> Thanks! >>>> Helen >>>> >>>>> --- >>>>> drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++---------- >>>>> 1 file changed, 12 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c >>>>> index d049374413dc..04a29af8cc92 100644 >>>>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c >>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c >>>>> @@ -16,9 +16,6 @@ >>>>> #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8 >>>>> #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV >>>>> >>>>> -#define RKISP1_MBUS_FMT_HDIV 2 >>>>> -#define RKISP1_MBUS_FMT_VDIV 1 >>>>> - >>>>> enum rkisp1_shadow_regs_when { >>>>> RKISP1_SHADOW_REGS_SYNC, >>>>> RKISP1_SHADOW_REGS_ASYNC, >>>>> @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz, >>>>> static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >>>>> enum rkisp1_shadow_regs_when when) >>>>> { >>>>> - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV; >>>>> struct v4l2_rect sink_y, sink_c, src_y, src_c; >>>>> struct v4l2_mbus_framefmt *src_fmt; >>>>> struct v4l2_rect *sink_crop; >>>>> struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id]; >>>>> + const struct v4l2_format_info *yuv422_info = >>>>> + v4l2_format_info(V4L2_PIX_FMT_YUV422P); >>>>> > > Instead of hardcoding this fourcc, is there any way we can > retrieve it from a configured format? > What do you mean? If the configured format is bayer then the resizer is disabled. Otherwise the resizer always get the input as yuv422, this is why it is hard coded. Thanks, Dafna > Thanks, > Ezequiel >
On 22.05.20 15:57, Laurent Pinchart wrote: > Hi Dafna, > > On Fri, May 22, 2020 at 02:11:22PM +0200, Dafna Hirschfeld wrote: >> On 21.05.20 00:08, Helen Koike wrote: >>> On 5/20/20 6:54 PM, Helen Koike wrote: >>>> On 5/15/20 11:29 AM, Dafna Hirschfeld wrote: >>>>> The resize block of rkisp1 always get the input as yuv422. >>>>> Instead of defining the default hdiv, vdiv as macros, the >>>>> code is more clear if it takes the (hv)div from the YUV422P >>>>> info. This makes it clear where those values come from. >>>>> The patch also adds documentation to explain that. >>>>> >>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>>> >>>> Acked-by: Helen Koike <helen.koike@collabora.com> >>>> >>>>> --- >>>>> drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++---------- >>>>> 1 file changed, 12 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c >>>>> index d049374413dc..04a29af8cc92 100644 >>>>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c >>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c >>>>> @@ -16,9 +16,6 @@ >>>>> #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8 >>>>> #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV >>>>> >>>>> -#define RKISP1_MBUS_FMT_HDIV 2 >>>>> -#define RKISP1_MBUS_FMT_VDIV 1 >>>>> - >>>>> enum rkisp1_shadow_regs_when { >>>>> RKISP1_SHADOW_REGS_SYNC, >>>>> RKISP1_SHADOW_REGS_ASYNC, >>>>> @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz, >>>>> static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >>>>> enum rkisp1_shadow_regs_when when) >>>>> { >>>>> - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV; >>>>> struct v4l2_rect sink_y, sink_c, src_y, src_c; >>>>> struct v4l2_mbus_framefmt *src_fmt; >>>>> struct v4l2_rect *sink_crop; >>>>> struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id]; >>>>> + const struct v4l2_format_info *yuv422_info = >>>>> + v4l2_format_info(V4L2_PIX_FMT_YUV422P); >>>>> >>>>> sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK, >>>>> V4L2_SUBDEV_FORMAT_ACTIVE); >>>>> @@ -386,8 +384,9 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >>>>> src_y.width = src_fmt->width; >>>>> src_y.height = src_fmt->height; >>>>> >>>>> - sink_c.width = sink_y.width / RKISP1_MBUS_FMT_HDIV; >>>>> - sink_c.height = sink_y.height / RKISP1_MBUS_FMT_VDIV; >>>>> + /* The input format of the resizer is always yuv422 */ >>>>> + sink_c.width = sink_y.width / yuv422_info->hdiv; >>>>> + sink_c.height = sink_y.height / yuv422_info->vdiv; >>>>> >>>>> /* >>>>> * The resizer is used not only to change the dimensions of the frame >>>>> @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >>>>> * (4:2:2 -> 4:2:0 for example). So the width/height of the CbCr >>>>> * streams should be set according to the pixel format in the capture. >>>>> * The resizer always gets the input as YUV422. If the capture format >>>>> - * is RGB then the memory input should be YUV422 so we don't change the >>>>> - * default hdiv, vdiv in that case. >>>>> + * is RGB then the memory input (the resizer output) should be YUV422 >>>>> + * so we use the hdiv, vdiv of the YUV422 info in this case. >>>>> */ >>>>> if (v4l2_is_format_yuv(cap->pix.info)) { >>>>> - hdiv = cap->pix.info->hdiv; >>>>> - vdiv = cap->pix.info->vdiv; >>>>> + src_c.width = cap->pix.info->hdiv; >>>>> + src_c.height = cap->pix.info->vdiv; >>> >>> Sorry, I just noticed you are assigning hdiv and vdiv directly to width and height, which looks wrong. >>> >>> It should be: >>> >>> src_c.width = src_y.width / cap->pix.info->hdiv; >>> src_c.height = src_y.height / cap->pix.info->vdiv; >> >> autch, thanks for finding it, I probably concentrated only on testing the new RGB formats > > Please make sure to test all supported formats :-)Yes, I don't how I missed that :/ > > I really, really recommend writing a small set of test scripts that will > automate the tests for you. It can be as simple as wrapping media-ctl + > yavta (or v4l2-ctl), or the libcamera cam utility if it offers all the > features you need, and run them for all supported formats. We already have python scripts that wrap media-ctl/v4l2-ctl https://gitlab.collabora.com/dafna/v4l-utils/-/blob/rkisp1-simult-stream/contrib/test/test-rkisp1.py For example, you can run python3 test-rkisp1.py -t CUSTOM_RKISP1_TEST_stream -o /root -m YUYV8_2X8 -p sp -P YU12 -S -v -c --isp-dim 1300x1500 --resizer-dim 1200x700 Then: '-m YUYV8_2X8' is the media bus for the output of the isp '-p sp' is for streaming from selfpath video node '-P YU12' is for format yuv420p '--sip-dim 1300x1500' is the values to set the sensor and the crop of the sink pad of the isp entity '--resizer-dim 1200x700' is the output dimensions of the resizer Thanks, Dafna > >>>>> + } else { >>>>> + src_c.width = src_y.width / yuv422_info->hdiv; >>>>> + src_c.height = src_y.height / yuv422_info->vdiv; >>>>> } >>>>> >>>>> - src_c.width = src_y.width / hdiv; >>>>> - src_c.height = src_y.height / vdiv; >>>>> - >>>>> if (sink_c.width == src_c.width && sink_c.height == src_c.height) { >>>>> rkisp1_rsz_disable(rsz, when); >>>>> return; >>>>> >
Hi Dafna, On Fri, May 22, 2020 at 04:54:24PM +0200, Dafna Hirschfeld wrote: > On 22.05.20 15:57, Laurent Pinchart wrote: > > On Fri, May 22, 2020 at 02:11:22PM +0200, Dafna Hirschfeld wrote: > >> On 21.05.20 00:08, Helen Koike wrote: > >>> On 5/20/20 6:54 PM, Helen Koike wrote: > >>>> On 5/15/20 11:29 AM, Dafna Hirschfeld wrote: > >>>>> The resize block of rkisp1 always get the input as yuv422. > >>>>> Instead of defining the default hdiv, vdiv as macros, the > >>>>> code is more clear if it takes the (hv)div from the YUV422P > >>>>> info. This makes it clear where those values come from. > >>>>> The patch also adds documentation to explain that. > >>>>> > >>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > >>>> > >>>> Acked-by: Helen Koike <helen.koike@collabora.com> > >>>> > >>>>> --- > >>>>> drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++---------- > >>>>> 1 file changed, 12 insertions(+), 13 deletions(-) > >>>>> > >>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c > >>>>> index d049374413dc..04a29af8cc92 100644 > >>>>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c > >>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c > >>>>> @@ -16,9 +16,6 @@ > >>>>> #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8 > >>>>> #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV > >>>>> > >>>>> -#define RKISP1_MBUS_FMT_HDIV 2 > >>>>> -#define RKISP1_MBUS_FMT_VDIV 1 > >>>>> - > >>>>> enum rkisp1_shadow_regs_when { > >>>>> RKISP1_SHADOW_REGS_SYNC, > >>>>> RKISP1_SHADOW_REGS_ASYNC, > >>>>> @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz, > >>>>> static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > >>>>> enum rkisp1_shadow_regs_when when) > >>>>> { > >>>>> - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV; > >>>>> struct v4l2_rect sink_y, sink_c, src_y, src_c; > >>>>> struct v4l2_mbus_framefmt *src_fmt; > >>>>> struct v4l2_rect *sink_crop; > >>>>> struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id]; > >>>>> + const struct v4l2_format_info *yuv422_info = > >>>>> + v4l2_format_info(V4L2_PIX_FMT_YUV422P); > >>>>> > >>>>> sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK, > >>>>> V4L2_SUBDEV_FORMAT_ACTIVE); > >>>>> @@ -386,8 +384,9 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > >>>>> src_y.width = src_fmt->width; > >>>>> src_y.height = src_fmt->height; > >>>>> > >>>>> - sink_c.width = sink_y.width / RKISP1_MBUS_FMT_HDIV; > >>>>> - sink_c.height = sink_y.height / RKISP1_MBUS_FMT_VDIV; > >>>>> + /* The input format of the resizer is always yuv422 */ > >>>>> + sink_c.width = sink_y.width / yuv422_info->hdiv; > >>>>> + sink_c.height = sink_y.height / yuv422_info->vdiv; > >>>>> > >>>>> /* > >>>>> * The resizer is used not only to change the dimensions of the frame > >>>>> @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > >>>>> * (4:2:2 -> 4:2:0 for example). So the width/height of the CbCr > >>>>> * streams should be set according to the pixel format in the capture. > >>>>> * The resizer always gets the input as YUV422. If the capture format > >>>>> - * is RGB then the memory input should be YUV422 so we don't change the > >>>>> - * default hdiv, vdiv in that case. > >>>>> + * is RGB then the memory input (the resizer output) should be YUV422 > >>>>> + * so we use the hdiv, vdiv of the YUV422 info in this case. > >>>>> */ > >>>>> if (v4l2_is_format_yuv(cap->pix.info)) { > >>>>> - hdiv = cap->pix.info->hdiv; > >>>>> - vdiv = cap->pix.info->vdiv; > >>>>> + src_c.width = cap->pix.info->hdiv; > >>>>> + src_c.height = cap->pix.info->vdiv; > >>> > >>> Sorry, I just noticed you are assigning hdiv and vdiv directly to width and height, which looks wrong. > >>> > >>> It should be: > >>> > >>> src_c.width = src_y.width / cap->pix.info->hdiv; > >>> src_c.height = src_y.height / cap->pix.info->vdiv; > >> > >> autch, thanks for finding it, I probably concentrated only on testing the new RGB formats > > > > Please make sure to test all supported formats :-) > > Yes, I don't how I missed that :/ > > > I really, really recommend writing a small set of test scripts that will > > automate the tests for you. It can be as simple as wrapping media-ctl + > > yavta (or v4l2-ctl), or the libcamera cam utility if it offers all the > > features you need, and run them for all supported formats. > > We already have python scripts that wrap media-ctl/v4l2-ctl > https://gitlab.collabora.com/dafna/v4l-utils/-/blob/rkisp1-simult-stream/contrib/test/test-rkisp1.py > > For example, you can run > python3 test-rkisp1.py -t CUSTOM_RKISP1_TEST_stream -o /root -m YUYV8_2X8 -p sp -P YU12 -S -v -c --isp-dim 1300x1500 --resizer-dim 1200x700 > > Then: > '-m YUYV8_2X8' is the media bus for the output of the isp > '-p sp' is for streaming from selfpath video node > '-P YU12' is for format yuv420p > '--sip-dim 1300x1500' is the values to set the sensor and the crop of the sink pad of the isp entity > '--resizer-dim 1200x700' is the output dimensions of the resizer I recommend wrapping that into a script that will test all supported formats, and give a pass/fail result. For instance, we have developed unit test scripts for the Renesas VSP (a memory-to-memory video processing engine supported by a V4L2 driver), available at http://git.ideasonboard.com/renesas/vsp-tests.git, that test lots of different pipelines, including checks on the output image. Checking the output image is more difficult in your case, as the input to the rkisp1 is a camera sensor and not memory, but maybe there's a test pattern mode that could be leveraged ? In any case, validation of the image content is likely a long term project, short term I recommend test cases that will try all the supported formats in various resolutions, to ensure there's no crash, and you can limit the verification to the captured buffer size for instance. > >>>>> + } else { > >>>>> + src_c.width = src_y.width / yuv422_info->hdiv; > >>>>> + src_c.height = src_y.height / yuv422_info->vdiv; > >>>>> } > >>>>> > >>>>> - src_c.width = src_y.width / hdiv; > >>>>> - src_c.height = src_y.height / vdiv; > >>>>> - > >>>>> if (sink_c.width == src_c.width && sink_c.height == src_c.height) { > >>>>> rkisp1_rsz_disable(rsz, when); > >>>>> return; > >>>>>
On Fri, 2020-05-22 at 16:15 +0200, Dafna Hirschfeld wrote: > > On 22.05.20 15:31, Ezequiel Garcia wrote: > > Hi Dafna, Helen, > > > > On Fri, 2020-05-22 at 14:11 +0200, Dafna Hirschfeld wrote: > > > On 21.05.20 00:08, Helen Koike wrote: > > > > On 5/20/20 6:54 PM, Helen Koike wrote: > > > > > Hi Dafna, > > > > > > > > > > On 5/15/20 11:29 AM, Dafna Hirschfeld wrote: > > > > > > The resize block of rkisp1 always get the input as yuv422. > > > > > > Instead of defining the default hdiv, vdiv as macros, the > > > > > > code is more clear if it takes the (hv)div from the YUV422P > > > > > > info. This makes it clear where those values come from. > > > > > > The patch also adds documentation to explain that. > > > > > > > > > > > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > > > > > > > > > > Acked-by: Helen Koike <helen.koike@collabora.com> > > > > > > > > > > Thanks! > > > > > Helen > > > > > > > > > > > --- > > > > > > drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++---------- > > > > > > 1 file changed, 12 insertions(+), 13 deletions(-) > > > > > > > > > > > > diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c > > > > > > index d049374413dc..04a29af8cc92 100644 > > > > > > --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c > > > > > > +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c > > > > > > @@ -16,9 +16,6 @@ > > > > > > #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8 > > > > > > #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV > > > > > > > > > > > > -#define RKISP1_MBUS_FMT_HDIV 2 > > > > > > -#define RKISP1_MBUS_FMT_VDIV 1 > > > > > > - > > > > > > enum rkisp1_shadow_regs_when { > > > > > > RKISP1_SHADOW_REGS_SYNC, > > > > > > RKISP1_SHADOW_REGS_ASYNC, > > > > > > @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz, > > > > > > static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > > > > > > enum rkisp1_shadow_regs_when when) > > > > > > { > > > > > > - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV; > > > > > > struct v4l2_rect sink_y, sink_c, src_y, src_c; > > > > > > struct v4l2_mbus_framefmt *src_fmt; > > > > > > struct v4l2_rect *sink_crop; > > > > > > struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id]; > > > > > > + const struct v4l2_format_info *yuv422_info = > > > > > > + v4l2_format_info(V4L2_PIX_FMT_YUV422P); > > > > > > > > > > Instead of hardcoding this fourcc, is there any way we can > > retrieve it from a configured format? > > > What do you mean? > If the configured format is bayer then the resizer is disabled. > Otherwise the resizer always get the input as yuv422, this is why it is hard coded. > I don't like to rely on these assumptions/knowledge. It's much cleaner to retrieve the format, and avoiding hardcoding things as much as you can. Hope I'm making sense :-) Ezequiel
On 22.05.20 17:04, Ezequiel Garcia wrote: > On Fri, 2020-05-22 at 16:15 +0200, Dafna Hirschfeld wrote: >> >> On 22.05.20 15:31, Ezequiel Garcia wrote: >>> Hi Dafna, Helen, >>> >>> On Fri, 2020-05-22 at 14:11 +0200, Dafna Hirschfeld wrote: >>>> On 21.05.20 00:08, Helen Koike wrote: >>>>> On 5/20/20 6:54 PM, Helen Koike wrote: >>>>>> Hi Dafna, >>>>>> >>>>>> On 5/15/20 11:29 AM, Dafna Hirschfeld wrote: >>>>>>> The resize block of rkisp1 always get the input as yuv422. >>>>>>> Instead of defining the default hdiv, vdiv as macros, the >>>>>>> code is more clear if it takes the (hv)div from the YUV422P >>>>>>> info. This makes it clear where those values come from. >>>>>>> The patch also adds documentation to explain that. >>>>>>> >>>>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>>>>> >>>>>> Acked-by: Helen Koike <helen.koike@collabora.com> >>>>>> >>>>>> Thanks! >>>>>> Helen >>>>>> >>>>>>> --- >>>>>>> drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++---------- >>>>>>> 1 file changed, 12 insertions(+), 13 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c >>>>>>> index d049374413dc..04a29af8cc92 100644 >>>>>>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c >>>>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c >>>>>>> @@ -16,9 +16,6 @@ >>>>>>> #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8 >>>>>>> #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV >>>>>>> >>>>>>> -#define RKISP1_MBUS_FMT_HDIV 2 >>>>>>> -#define RKISP1_MBUS_FMT_VDIV 1 >>>>>>> - >>>>>>> enum rkisp1_shadow_regs_when { >>>>>>> RKISP1_SHADOW_REGS_SYNC, >>>>>>> RKISP1_SHADOW_REGS_ASYNC, >>>>>>> @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz, >>>>>>> static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >>>>>>> enum rkisp1_shadow_regs_when when) >>>>>>> { >>>>>>> - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV; >>>>>>> struct v4l2_rect sink_y, sink_c, src_y, src_c; >>>>>>> struct v4l2_mbus_framefmt *src_fmt; >>>>>>> struct v4l2_rect *sink_crop; >>>>>>> struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id]; >>>>>>> + const struct v4l2_format_info *yuv422_info = >>>>>>> + v4l2_format_info(V4L2_PIX_FMT_YUV422P); >>>>>>> >>> >>> Instead of hardcoding this fourcc, is there any way we can >>> retrieve it from a configured format? >>> >> What do you mean? >> If the configured format is bayer then the resizer is disabled. >> Otherwise the resizer always get the input as yuv422, this is why it is hard coded. >> > > I don't like to rely on these assumptions/knowledge.> It's much cleaner to retrieve the format, and avoiding > hardcoding things as much as you can. > > Hope I'm making sense :-) hmm, not yet, If the input is a constant why not hardcode it? Not sure what kind of code would you expect to replace it? You mean a function that get the sink mbus as an input and returns "v4l2_format_info(V4L2_PIX_FMT_YUV422P)" if the mbus is MEDIA_BUS_FMT_YUYV8_2X8? Thanks, Dafna > > Ezequiel >
On Fri, May 22, 2020 at 5:05 PM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > On Fri, 2020-05-22 at 16:15 +0200, Dafna Hirschfeld wrote: > > > > On 22.05.20 15:31, Ezequiel Garcia wrote: > > > Hi Dafna, Helen, > > > > > > On Fri, 2020-05-22 at 14:11 +0200, Dafna Hirschfeld wrote: > > > > On 21.05.20 00:08, Helen Koike wrote: > > > > > On 5/20/20 6:54 PM, Helen Koike wrote: > > > > > > Hi Dafna, > > > > > > > > > > > > On 5/15/20 11:29 AM, Dafna Hirschfeld wrote: > > > > > > > The resize block of rkisp1 always get the input as yuv422. > > > > > > > Instead of defining the default hdiv, vdiv as macros, the > > > > > > > code is more clear if it takes the (hv)div from the YUV422P > > > > > > > info. This makes it clear where those values come from. > > > > > > > The patch also adds documentation to explain that. > > > > > > > > > > > > > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > > > > > > > > > > > > Acked-by: Helen Koike <helen.koike@collabora.com> > > > > > > > > > > > > Thanks! > > > > > > Helen > > > > > > > > > > > > > --- > > > > > > > drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++---------- > > > > > > > 1 file changed, 12 insertions(+), 13 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c > > > > > > > index d049374413dc..04a29af8cc92 100644 > > > > > > > --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c > > > > > > > +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c > > > > > > > @@ -16,9 +16,6 @@ > > > > > > > #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8 > > > > > > > #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV > > > > > > > > > > > > > > -#define RKISP1_MBUS_FMT_HDIV 2 > > > > > > > -#define RKISP1_MBUS_FMT_VDIV 1 > > > > > > > - > > > > > > > enum rkisp1_shadow_regs_when { > > > > > > > RKISP1_SHADOW_REGS_SYNC, > > > > > > > RKISP1_SHADOW_REGS_ASYNC, > > > > > > > @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz, > > > > > > > static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > > > > > > > enum rkisp1_shadow_regs_when when) > > > > > > > { > > > > > > > - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV; > > > > > > > struct v4l2_rect sink_y, sink_c, src_y, src_c; > > > > > > > struct v4l2_mbus_framefmt *src_fmt; > > > > > > > struct v4l2_rect *sink_crop; > > > > > > > struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id]; > > > > > > > + const struct v4l2_format_info *yuv422_info = > > > > > > > + v4l2_format_info(V4L2_PIX_FMT_YUV422P); > > > > > > > > > > > > > Instead of hardcoding this fourcc, is there any way we can > > > retrieve it from a configured format? > > > > > What do you mean? > > If the configured format is bayer then the resizer is disabled. > > Otherwise the resizer always get the input as yuv422, this is why it is hard coded. > > > > I don't like to rely on these assumptions/knowledge. > It's much cleaner to retrieve the format, and avoiding > hardcoding things as much as you can. It would indeed be cleaner if we could retrieve this format from somewhere, but where would that be? In theory we could assign a YUV4:2:2 mbus format to the resizer input pad, but I think there is no similar data available for mbs formats, is there? Actually, if we look at this a bit more strictly, V4L2_PIX_FMT_YUV422P is not exactly what the resizer gets at its input. V4L2_PIX_FMT_YUV422P is a specific memory representation and the corresponding v4l2_format_info struct contains data about the memory layout. The resizer gets an unspecified YUV 4:2:2 pixel stream. Making the code suggest that it's V4L2_PIX_FMT_YUV422P might make it more confusing in another way. Perhaps the way forward would be to simply add a comment explaining where the 2 and 1 dividers come from? Best regards, Tomasz
Hi Dafna, On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld <dafna.hirschfeld@collabora.com> wrote: > > The resize block of rkisp1 always get the input as yuv422. > Instead of defining the default hdiv, vdiv as macros, the > code is more clear if it takes the (hv)div from the YUV422P > info. This makes it clear where those values come from. > The patch also adds documentation to explain that. > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++---------- > 1 file changed, 12 insertions(+), 13 deletions(-) > Thank you for the effort on improving this driver! Please see my comments below. > diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c > index d049374413dc..04a29af8cc92 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c > +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c > @@ -16,9 +16,6 @@ > #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8 > #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV > > -#define RKISP1_MBUS_FMT_HDIV 2 > -#define RKISP1_MBUS_FMT_VDIV 1 > - > enum rkisp1_shadow_regs_when { > RKISP1_SHADOW_REGS_SYNC, > RKISP1_SHADOW_REGS_ASYNC, > @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz, > static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > enum rkisp1_shadow_regs_when when) > { > - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV; > struct v4l2_rect sink_y, sink_c, src_y, src_c; > struct v4l2_mbus_framefmt *src_fmt; > struct v4l2_rect *sink_crop; > struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id]; > + const struct v4l2_format_info *yuv422_info = > + v4l2_format_info(V4L2_PIX_FMT_YUV422P); As I mentioned in another reply, this is a memory format, but we're using it to configure a local (i.e. non-DMA) input. > > sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK, > V4L2_SUBDEV_FORMAT_ACTIVE); > @@ -386,8 +384,9 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > src_y.width = src_fmt->width; > src_y.height = src_fmt->height; > > - sink_c.width = sink_y.width / RKISP1_MBUS_FMT_HDIV; > - sink_c.height = sink_y.height / RKISP1_MBUS_FMT_VDIV; > + /* The input format of the resizer is always yuv422 */ > + sink_c.width = sink_y.width / yuv422_info->hdiv; > + sink_c.height = sink_y.height / yuv422_info->vdiv; I'd honestly go the opposite way and just make it: /* The resizer input is always YUV 4:2:2 and so horizontally subsampled by 2. */ sink_c.width = sink_y.width / 2; sink_c.height = sink_y.height; > > /* > * The resizer is used not only to change the dimensions of the frame > @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > * (4:2:2 -> 4:2:0 for example). So the width/height of the CbCr > * streams should be set according to the pixel format in the capture. > * The resizer always gets the input as YUV422. If the capture format > - * is RGB then the memory input should be YUV422 so we don't change the > - * default hdiv, vdiv in that case. > + * is RGB then the memory input (the resizer output) should be YUV422 It could be just me, but "memory input" sounds like there was an input DMA involved, which is not the case. How about "Memory Interface input"? Also, if already amending this, I would also fix the overuse of "should". How about the following? "According to the hardware pipeline, the resizer input is always YUV 4:2:2. For YUV formats, the Memory Interface requires its input to be already properly subsampled. We can achieve subsampling factors other than YUV 4:2:2 by scaling the planes appropriately. For RGB formats, the Memory Interface requires YUV 4:2:2 input, so no additional scaling is needed." > + * so we use the hdiv, vdiv of the YUV422 info in this case. > */ > if (v4l2_is_format_yuv(cap->pix.info)) { > - hdiv = cap->pix.info->hdiv; > - vdiv = cap->pix.info->vdiv; > + src_c.width = cap->pix.info->hdiv; > + src_c.height = cap->pix.info->vdiv; > + } else { How about making it an explicit else if (v4l2_is_format_rgb(...))? > + src_c.width = src_y.width / yuv422_info->hdiv; > + src_c.height = src_y.height / yuv422_info->vdiv; And then: /* YUV 4:2:2 output to Memory Interface */ src_c.width = src_y.width / 2; src_c.height = src_y.height; > } > > - src_c.width = src_y.width / hdiv; > - src_c.height = src_y.height / vdiv; > - > if (sink_c.width == src_c.width && sink_c.height == src_c.height) { > rkisp1_rsz_disable(rsz, when); > return; > -- > 2.17.1 >
On 22.05.20 16:59, Laurent Pinchart wrote: > Hi Dafna, > > On Fri, May 22, 2020 at 04:54:24PM +0200, Dafna Hirschfeld wrote: >> On 22.05.20 15:57, Laurent Pinchart wrote: >>> On Fri, May 22, 2020 at 02:11:22PM +0200, Dafna Hirschfeld wrote: >>>> On 21.05.20 00:08, Helen Koike wrote: >>>>> On 5/20/20 6:54 PM, Helen Koike wrote: >>>>>> On 5/15/20 11:29 AM, Dafna Hirschfeld wrote: >>>>>>> The resize block of rkisp1 always get the input as yuv422. >>>>>>> Instead of defining the default hdiv, vdiv as macros, the >>>>>>> code is more clear if it takes the (hv)div from the YUV422P >>>>>>> info. This makes it clear where those values come from. >>>>>>> The patch also adds documentation to explain that. >>>>>>> >>>>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>>>>> >>>>>> Acked-by: Helen Koike <helen.koike@collabora.com> >>>>>> >>>>>>> --- >>>>>>> drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++---------- >>>>>>> 1 file changed, 12 insertions(+), 13 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c >>>>>>> index d049374413dc..04a29af8cc92 100644 >>>>>>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c >>>>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c >>>>>>> @@ -16,9 +16,6 @@ >>>>>>> #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8 >>>>>>> #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV >>>>>>> >>>>>>> -#define RKISP1_MBUS_FMT_HDIV 2 >>>>>>> -#define RKISP1_MBUS_FMT_VDIV 1 >>>>>>> - >>>>>>> enum rkisp1_shadow_regs_when { >>>>>>> RKISP1_SHADOW_REGS_SYNC, >>>>>>> RKISP1_SHADOW_REGS_ASYNC, >>>>>>> @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz, >>>>>>> static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >>>>>>> enum rkisp1_shadow_regs_when when) >>>>>>> { >>>>>>> - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV; >>>>>>> struct v4l2_rect sink_y, sink_c, src_y, src_c; >>>>>>> struct v4l2_mbus_framefmt *src_fmt; >>>>>>> struct v4l2_rect *sink_crop; >>>>>>> struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id]; >>>>>>> + const struct v4l2_format_info *yuv422_info = >>>>>>> + v4l2_format_info(V4L2_PIX_FMT_YUV422P); >>>>>>> >>>>>>> sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK, >>>>>>> V4L2_SUBDEV_FORMAT_ACTIVE); >>>>>>> @@ -386,8 +384,9 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >>>>>>> src_y.width = src_fmt->width; >>>>>>> src_y.height = src_fmt->height; >>>>>>> >>>>>>> - sink_c.width = sink_y.width / RKISP1_MBUS_FMT_HDIV; >>>>>>> - sink_c.height = sink_y.height / RKISP1_MBUS_FMT_VDIV; >>>>>>> + /* The input format of the resizer is always yuv422 */ >>>>>>> + sink_c.width = sink_y.width / yuv422_info->hdiv; >>>>>>> + sink_c.height = sink_y.height / yuv422_info->vdiv; >>>>>>> >>>>>>> /* >>>>>>> * The resizer is used not only to change the dimensions of the frame >>>>>>> @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >>>>>>> * (4:2:2 -> 4:2:0 for example). So the width/height of the CbCr >>>>>>> * streams should be set according to the pixel format in the capture. >>>>>>> * The resizer always gets the input as YUV422. If the capture format >>>>>>> - * is RGB then the memory input should be YUV422 so we don't change the >>>>>>> - * default hdiv, vdiv in that case. >>>>>>> + * is RGB then the memory input (the resizer output) should be YUV422 >>>>>>> + * so we use the hdiv, vdiv of the YUV422 info in this case. >>>>>>> */ >>>>>>> if (v4l2_is_format_yuv(cap->pix.info)) { >>>>>>> - hdiv = cap->pix.info->hdiv; >>>>>>> - vdiv = cap->pix.info->vdiv; >>>>>>> + src_c.width = cap->pix.info->hdiv; >>>>>>> + src_c.height = cap->pix.info->vdiv; >>>>> >>>>> Sorry, I just noticed you are assigning hdiv and vdiv directly to width and height, which looks wrong. >>>>> >>>>> It should be: >>>>> >>>>> src_c.width = src_y.width / cap->pix.info->hdiv; >>>>> src_c.height = src_y.height / cap->pix.info->vdiv; >>>> >>>> autch, thanks for finding it, I probably concentrated only on testing the new RGB formats >>> >>> Please make sure to test all supported formats :-) >> >> Yes, I don't how I missed that :/ >> >>> I really, really recommend writing a small set of test scripts that will >>> automate the tests for you. It can be as simple as wrapping media-ctl + >>> yavta (or v4l2-ctl), or the libcamera cam utility if it offers all the >>> features you need, and run them for all supported formats. >> >> We already have python scripts that wrap media-ctl/v4l2-ctl >> https://gitlab.collabora.com/dafna/v4l-utils/-/blob/rkisp1-simult-stream/contrib/test/test-rkisp1.py >> >> For example, you can run >> python3 test-rkisp1.py -t CUSTOM_RKISP1_TEST_stream -o /root -m YUYV8_2X8 -p sp -P YU12 -S -v -c --isp-dim 1300x1500 --resizer-dim 1200x700 >> >> Then: >> '-m YUYV8_2X8' is the media bus for the output of the isp >> '-p sp' is for streaming from selfpath video node >> '-P YU12' is for format yuv420p >> '--sip-dim 1300x1500' is the values to set the sensor and the crop of the sink pad of the isp entity >> '--resizer-dim 1200x700' is the output dimensions of the resizer > > I recommend wrapping that into a script that will test all supported > formats, and give a pass/fail result. For instance, we have developed > unit test scripts for the Renesas VSP (a memory-to-memory video > processing engine supported by a V4L2 driver), available at > http://git.ideasonboard.com/renesas/vsp-tests.git, that test lots of > different pipelines, including checks on the output image. > > Checking the output image is more difficult in your case, as the input > to the rkisp1 is a camera sensor and not memory, but maybe there's a > test pattern mode that could be leveraged ? > > In any case, validation of the image content is likely a long term > project, short term I recommend test cases that will try all the > supported formats in various resolutions, to ensure there's no crash, > and you can limit the verification to the captured buffer size for > instance. Thanks for the suggestion, I wrote this simple script for now that just iterates all video formats supported by selfpath and mainpath and streams them and then test that the resulted file matches the expected size. I will extend the tests in the future. https://gitlab.collabora.com/dafna/v4l-utils/-/blob/rkisp1-unit-tests/contrib/test/rkisp1-unit-tests.sh Thanks, Dafna > >>>>>>> + } else { >>>>>>> + src_c.width = src_y.width / yuv422_info->hdiv; >>>>>>> + src_c.height = src_y.height / yuv422_info->vdiv; >>>>>>> } >>>>>>> >>>>>>> - src_c.width = src_y.width / hdiv; >>>>>>> - src_c.height = src_y.height / vdiv; >>>>>>> - >>>>>>> if (sink_c.width == src_c.width && sink_c.height == src_c.height) { >>>>>>> rkisp1_rsz_disable(rsz, when); >>>>>>> return; >>>>>>> >
On 27.05.20 00:44, Tomasz Figa wrote: > Hi Dafna, > > On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld > <dafna.hirschfeld@collabora.com> wrote: >> >> The resize block of rkisp1 always get the input as yuv422. >> Instead of defining the default hdiv, vdiv as macros, the >> code is more clear if it takes the (hv)div from the YUV422P >> info. This makes it clear where those values come from. >> The patch also adds documentation to explain that. >> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >> --- >> drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++---------- >> 1 file changed, 12 insertions(+), 13 deletions(-) >> > > Thank you for the effort on improving this driver! Please see my comments below. > >> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c >> index d049374413dc..04a29af8cc92 100644 >> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c >> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c >> @@ -16,9 +16,6 @@ >> #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8 >> #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV >> >> -#define RKISP1_MBUS_FMT_HDIV 2 >> -#define RKISP1_MBUS_FMT_VDIV 1 >> - >> enum rkisp1_shadow_regs_when { >> RKISP1_SHADOW_REGS_SYNC, >> RKISP1_SHADOW_REGS_ASYNC, >> @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz, >> static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >> enum rkisp1_shadow_regs_when when) >> { >> - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV; >> struct v4l2_rect sink_y, sink_c, src_y, src_c; >> struct v4l2_mbus_framefmt *src_fmt; >> struct v4l2_rect *sink_crop; >> struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id]; >> + const struct v4l2_format_info *yuv422_info = >> + v4l2_format_info(V4L2_PIX_FMT_YUV422P); > > As I mentioned in another reply, this is a memory format, but we're > using it to configure a local (i.e. non-DMA) input. > >> >> sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK, >> V4L2_SUBDEV_FORMAT_ACTIVE); >> @@ -386,8 +384,9 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >> src_y.width = src_fmt->width; >> src_y.height = src_fmt->height; >> >> - sink_c.width = sink_y.width / RKISP1_MBUS_FMT_HDIV; >> - sink_c.height = sink_y.height / RKISP1_MBUS_FMT_VDIV; >> + /* The input format of the resizer is always yuv422 */ >> + sink_c.width = sink_y.width / yuv422_info->hdiv; >> + sink_c.height = sink_y.height / yuv422_info->vdiv; > > I'd honestly go the opposite way and just make it: > > /* The resizer input is always YUV 4:2:2 and so horizontally subsampled by 2. */ > sink_c.width = sink_y.width / 2; > sink_c.height = sink_y.height; > >> >> /* >> * The resizer is used not only to change the dimensions of the frame >> @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, >> * (4:2:2 -> 4:2:0 for example). So the width/height of the CbCr >> * streams should be set according to the pixel format in the capture. >> * The resizer always gets the input as YUV422. If the capture format >> - * is RGB then the memory input should be YUV422 so we don't change the >> - * default hdiv, vdiv in that case. >> + * is RGB then the memory input (the resizer output) should be YUV422 > > It could be just me, but "memory input" sounds like there was an input > DMA involved, which is not the case. How about "Memory Interface > input"? > > Also, if already amending this, I would also fix the overuse of > "should". How about the following? > > "According to the hardware pipeline, the resizer input is always YUV > 4:2:2. For YUV formats, the Memory Interface requires its input to be > already properly subsampled. We can achieve subsampling factors other > than YUV 4:2:2 by scaling the planes appropriately. For RGB formats, > the Memory Interface requires YUV 4:2:2 input, so no additional > scaling is needed." > >> + * so we use the hdiv, vdiv of the YUV422 info in this case. >> */ >> if (v4l2_is_format_yuv(cap->pix.info)) { >> - hdiv = cap->pix.info->hdiv; >> - vdiv = cap->pix.info->vdiv; >> + src_c.width = cap->pix.info->hdiv; >> + src_c.height = cap->pix.info->vdiv; >> + } else { > > How about making it an explicit else if (v4l2_is_format_rgb(...))? Actually the right way to deal with YUV downscaling is to support MEDIA_BUS_FMT_YUYV8_1_5X8 (which is YUV420) on the src format and not to look on what is configured on the capture. Thanks, Dafna > >> + src_c.width = src_y.width / yuv422_info->hdiv; >> + src_c.height = src_y.height / yuv422_info->vdiv; > > And then: > > /* YUV 4:2:2 output to Memory Interface */ > src_c.width = src_y.width / 2; > src_c.height = src_y.height; > >> } >> >> - src_c.width = src_y.width / hdiv; >> - src_c.height = src_y.height / vdiv; >> - >> if (sink_c.width == src_c.width && sink_c.height == src_c.height) { >> rkisp1_rsz_disable(rsz, when); >> return; >> -- >> 2.17.1 >>
On Wed, Jun 10, 2020 at 7:01 PM Dafna Hirschfeld <dafna.hirschfeld@collabora.com> wrote: > > > > On 27.05.20 00:44, Tomasz Figa wrote: > > Hi Dafna, > > > > On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld > > <dafna.hirschfeld@collabora.com> wrote: > >> > >> The resize block of rkisp1 always get the input as yuv422. > >> Instead of defining the default hdiv, vdiv as macros, the > >> code is more clear if it takes the (hv)div from the YUV422P > >> info. This makes it clear where those values come from. > >> The patch also adds documentation to explain that. > >> > >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > >> --- > >> drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++---------- > >> 1 file changed, 12 insertions(+), 13 deletions(-) > >> > > > > Thank you for the effort on improving this driver! Please see my comments below. > > > >> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c > >> index d049374413dc..04a29af8cc92 100644 > >> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c > >> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c > >> @@ -16,9 +16,6 @@ > >> #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8 > >> #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV > >> > >> -#define RKISP1_MBUS_FMT_HDIV 2 > >> -#define RKISP1_MBUS_FMT_VDIV 1 > >> - > >> enum rkisp1_shadow_regs_when { > >> RKISP1_SHADOW_REGS_SYNC, > >> RKISP1_SHADOW_REGS_ASYNC, > >> @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz, > >> static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > >> enum rkisp1_shadow_regs_when when) > >> { > >> - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV; > >> struct v4l2_rect sink_y, sink_c, src_y, src_c; > >> struct v4l2_mbus_framefmt *src_fmt; > >> struct v4l2_rect *sink_crop; > >> struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id]; > >> + const struct v4l2_format_info *yuv422_info = > >> + v4l2_format_info(V4L2_PIX_FMT_YUV422P); > > > > As I mentioned in another reply, this is a memory format, but we're > > using it to configure a local (i.e. non-DMA) input. > > > >> > >> sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK, > >> V4L2_SUBDEV_FORMAT_ACTIVE); > >> @@ -386,8 +384,9 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > >> src_y.width = src_fmt->width; > >> src_y.height = src_fmt->height; > >> > >> - sink_c.width = sink_y.width / RKISP1_MBUS_FMT_HDIV; > >> - sink_c.height = sink_y.height / RKISP1_MBUS_FMT_VDIV; > >> + /* The input format of the resizer is always yuv422 */ > >> + sink_c.width = sink_y.width / yuv422_info->hdiv; > >> + sink_c.height = sink_y.height / yuv422_info->vdiv; > > > > I'd honestly go the opposite way and just make it: > > > > /* The resizer input is always YUV 4:2:2 and so horizontally subsampled by 2. */ > > sink_c.width = sink_y.width / 2; > > sink_c.height = sink_y.height; > > > >> > >> /* > >> * The resizer is used not only to change the dimensions of the frame > >> @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, > >> * (4:2:2 -> 4:2:0 for example). So the width/height of the CbCr > >> * streams should be set according to the pixel format in the capture. > >> * The resizer always gets the input as YUV422. If the capture format > >> - * is RGB then the memory input should be YUV422 so we don't change the > >> - * default hdiv, vdiv in that case. > >> + * is RGB then the memory input (the resizer output) should be YUV422 > > > > It could be just me, but "memory input" sounds like there was an input > > DMA involved, which is not the case. How about "Memory Interface > > input"? > > > > Also, if already amending this, I would also fix the overuse of > > "should". How about the following? > > > > "According to the hardware pipeline, the resizer input is always YUV > > 4:2:2. For YUV formats, the Memory Interface requires its input to be > > already properly subsampled. We can achieve subsampling factors other > > than YUV 4:2:2 by scaling the planes appropriately. For RGB formats, > > the Memory Interface requires YUV 4:2:2 input, so no additional > > scaling is needed." > > > >> + * so we use the hdiv, vdiv of the YUV422 info in this case. > >> */ > >> if (v4l2_is_format_yuv(cap->pix.info)) { > >> - hdiv = cap->pix.info->hdiv; > >> - vdiv = cap->pix.info->vdiv; > >> + src_c.width = cap->pix.info->hdiv; > >> + src_c.height = cap->pix.info->vdiv; > >> + } else { > > > > How about making it an explicit else if (v4l2_is_format_rgb(...))? > Actually the right way to deal with YUV downscaling is to support > MEDIA_BUS_FMT_YUYV8_1_5X8 (which is YUV420) on the src format > and not to look on what is configured on the capture. Good point. Since we're dealing with the resizer configuration, it should be contained within the resizer subdev. My mind was still stuck with our downstream topology. Best regards, Tomasz
diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c index d049374413dc..04a29af8cc92 100644 --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c @@ -16,9 +16,6 @@ #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8 #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV -#define RKISP1_MBUS_FMT_HDIV 2 -#define RKISP1_MBUS_FMT_VDIV 1 - enum rkisp1_shadow_regs_when { RKISP1_SHADOW_REGS_SYNC, RKISP1_SHADOW_REGS_ASYNC, @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz, static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, enum rkisp1_shadow_regs_when when) { - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV; struct v4l2_rect sink_y, sink_c, src_y, src_c; struct v4l2_mbus_framefmt *src_fmt; struct v4l2_rect *sink_crop; struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id]; + const struct v4l2_format_info *yuv422_info = + v4l2_format_info(V4L2_PIX_FMT_YUV422P); sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK, V4L2_SUBDEV_FORMAT_ACTIVE); @@ -386,8 +384,9 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, src_y.width = src_fmt->width; src_y.height = src_fmt->height; - sink_c.width = sink_y.width / RKISP1_MBUS_FMT_HDIV; - sink_c.height = sink_y.height / RKISP1_MBUS_FMT_VDIV; + /* The input format of the resizer is always yuv422 */ + sink_c.width = sink_y.width / yuv422_info->hdiv; + sink_c.height = sink_y.height / yuv422_info->vdiv; /* * The resizer is used not only to change the dimensions of the frame @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz, * (4:2:2 -> 4:2:0 for example). So the width/height of the CbCr * streams should be set according to the pixel format in the capture. * The resizer always gets the input as YUV422. If the capture format - * is RGB then the memory input should be YUV422 so we don't change the - * default hdiv, vdiv in that case. + * is RGB then the memory input (the resizer output) should be YUV422 + * so we use the hdiv, vdiv of the YUV422 info in this case. */ if (v4l2_is_format_yuv(cap->pix.info)) { - hdiv = cap->pix.info->hdiv; - vdiv = cap->pix.info->vdiv; + src_c.width = cap->pix.info->hdiv; + src_c.height = cap->pix.info->vdiv; + } else { + src_c.width = src_y.width / yuv422_info->hdiv; + src_c.height = src_y.height / yuv422_info->vdiv; } - src_c.width = src_y.width / hdiv; - src_c.height = src_y.height / vdiv; - if (sink_c.width == src_c.width && sink_c.height == src_c.height) { rkisp1_rsz_disable(rsz, when); return;
The resize block of rkisp1 always get the input as yuv422. Instead of defining the default hdiv, vdiv as macros, the code is more clear if it takes the (hv)div from the YUV422P info. This makes it clear where those values come from. The patch also adds documentation to explain that. Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-)