* [PATCH 0/2] media: rkisp1: Add support for UYVY output @ 2022-07-14 11:26 Paul Elder 2022-07-14 11:26 ` [PATCH 1/2] media: rkisp1: Add YC swap capability Paul Elder 2022-07-14 11:26 ` [PATCH 2/2] media: rkisp1: Add UYVY as an output format Paul Elder 0 siblings, 2 replies; 7+ messages in thread From: Paul Elder @ 2022-07-14 11:26 UTC (permalink / raw) To: linux-media Cc: Paul Elder, Dafna Hirschfeld, Mauro Carvalho Chehab, Heiko Stuebner, laurent.pinchart, linux-rockchip, linux-arm-kernel, linux-kernel This series adds support for UYVY as an output format in the rkisp1 driver, for devices that support it. Notably, the i.MX8MP supports it, while the rk3399 does not. This series is based on branch pinchartl/v5.19/isp/1 [1] at this repo [2]. [1] https://gitlab.com/ideasonboard/nxp/linux/-/commits/pinchartl/v5.19/isp/1 [2] https://gitlab.com/ideasonboard/nxp/linux Paul Elder (2): media: rkisp1: Add YC swap capability media: rkisp1: Add UYVY as an output format .../platform/rockchip/rkisp1/rkisp1-capture.c | 67 +++++++++++++++++-- .../platform/rockchip/rkisp1/rkisp1-common.h | 1 + .../platform/rockchip/rkisp1/rkisp1-dev.c | 3 +- 3 files changed, 63 insertions(+), 8 deletions(-) -- 2.30.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] media: rkisp1: Add YC swap capability 2022-07-14 11:26 [PATCH 0/2] media: rkisp1: Add support for UYVY output Paul Elder @ 2022-07-14 11:26 ` Paul Elder 2022-07-19 22:23 ` Laurent Pinchart 2022-07-14 11:26 ` [PATCH 2/2] media: rkisp1: Add UYVY as an output format Paul Elder 1 sibling, 1 reply; 7+ messages in thread From: Paul Elder @ 2022-07-14 11:26 UTC (permalink / raw) To: linux-media Cc: Paul Elder, Dafna Hirschfeld, Mauro Carvalho Chehab, Heiko Stuebner, laurent.pinchart, linux-rockchip, linux-arm-kernel, linux-kernel The ISP version in the i.MX8MP has an MI_OUTPUT_ALIGN_FORMAT register that the rk3399 does not have. This register allows swapping bytes, which can be used to implement UYVY from YUYV. Add a feature flag to signify the presence of this feature, and add it to the i.MX8MP match data. Also add it as a flag to the format info in the list of supported formats by the capture v4l2 devices, and update enum_fmt and s_fmt to take it into account. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- .../platform/rockchip/rkisp1/rkisp1-capture.c | 27 ++++++++++++++----- .../platform/rockchip/rkisp1/rkisp1-common.h | 1 + .../platform/rockchip/rkisp1/rkisp1-dev.c | 3 ++- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c index c494afbc21b4..85fd85fe208c 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c @@ -47,13 +47,15 @@ enum rkisp1_plane { * @fourcc: pixel format * @fmt_type: helper filed for pixel format * @uv_swap: if cb cr swapped, for yuv + * @yc_swap: if y and cb/cr swapped, for yuv * @write_format: defines how YCbCr self picture data is written to memory * @output_format: defines sp output format * @mbus: the mbus code on the src resizer pad that matches the pixel format */ struct rkisp1_capture_fmt_cfg { u32 fourcc; - u8 uv_swap; + u32 uv_swap : 1; + u32 yc_swap : 1; u32 write_format; u32 output_format; u32 mbus; @@ -1150,9 +1152,13 @@ static const struct rkisp1_capture_fmt_cfg * rkisp1_find_fmt_cfg(const struct rkisp1_capture *cap, const u32 pixelfmt) { unsigned int i; + bool yc_swap_support = + cap->rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN; for (i = 0; i < cap->config->fmt_size; i++) { - if (cap->config->fmts[i].fourcc == pixelfmt) + const struct rkisp1_capture_fmt_cfg *fmt = &cap->config->fmts[i]; + if (fmt->fourcc == pixelfmt && + (!fmt->yc_swap || yc_swap_support)) return &cap->config->fmts[i]; } return NULL; @@ -1223,22 +1229,29 @@ static int rkisp1_enum_fmt_vid_cap_mplane(struct file *file, void *priv, struct rkisp1_capture *cap = video_drvdata(file); const struct rkisp1_capture_fmt_cfg *fmt = NULL; unsigned int i, n = 0; + bool yc_swap_support = + cap->rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN; - if (!f->mbus_code) { - if (f->index >= cap->config->fmt_size) - return -EINVAL; + if (f->index >= cap->config->fmt_size) + return -EINVAL; + if (!f->mbus_code && yc_swap_support) { fmt = &cap->config->fmts[f->index]; f->pixelformat = fmt->fourcc; return 0; } for (i = 0; i < cap->config->fmt_size; i++) { - if (cap->config->fmts[i].mbus != f->mbus_code) + fmt = &cap->config->fmts[i]; + + if (!!f->mbus_code && fmt->mbus != f->mbus_code) + continue; + + if (!yc_swap_support && fmt->yc_swap) continue; if (n++ == f->index) { - f->pixelformat = cap->config->fmts[i].fourcc; + f->pixelformat = fmt->fourcc; return 0; } } diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h index 38be565341d6..b0f9221a1922 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h @@ -119,6 +119,7 @@ enum rkisp1_feature { RKISP1_FEATURE_MAIN_STRIDE = BIT(3), RKISP1_FEATURE_DMA_34BIT = BIT(4), RKISP1_FEATURE_SELF_PATH = BIT(5), + RKISP1_FEATURE_MI_OUTPUT_ALIGN = BIT(6), }; /* diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c index 3b549c07a9bb..a475933820fd 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c @@ -520,7 +520,8 @@ static const struct rkisp1_info imx8mp_isp_info = { .isp_ver = IMX8MP_V10, .features = RKISP1_FEATURE_RSZ_CROP | RKISP1_FEATURE_MAIN_STRIDE - | RKISP1_FEATURE_DMA_34BIT, + | RKISP1_FEATURE_DMA_34BIT + | RKISP1_FEATURE_MI_OUTPUT_ALIGN, }; static const struct of_device_id rkisp1_of_match[] = { -- 2.30.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] media: rkisp1: Add YC swap capability 2022-07-14 11:26 ` [PATCH 1/2] media: rkisp1: Add YC swap capability Paul Elder @ 2022-07-19 22:23 ` Laurent Pinchart 0 siblings, 0 replies; 7+ messages in thread From: Laurent Pinchart @ 2022-07-19 22:23 UTC (permalink / raw) To: Paul Elder Cc: linux-media, Dafna Hirschfeld, Mauro Carvalho Chehab, Heiko Stuebner, linux-rockchip, linux-arm-kernel, linux-kernel Hi Paul, Thank you for the patch. On Thu, Jul 14, 2022 at 08:26:02PM +0900, Paul Elder wrote: > The ISP version in the i.MX8MP has an MI_OUTPUT_ALIGN_FORMAT register > that the rk3399 does not have. This register allows swapping bytes, > which can be used to implement UYVY from YUYV. > > Add a feature flag to signify the presence of this feature, and add it > to the i.MX8MP match data. Also add it as a flag to the format info in > the list of supported formats by the capture v4l2 devices, and update > enum_fmt and s_fmt to take it into account. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > .../platform/rockchip/rkisp1/rkisp1-capture.c | 27 ++++++++++++++----- > .../platform/rockchip/rkisp1/rkisp1-common.h | 1 + > .../platform/rockchip/rkisp1/rkisp1-dev.c | 3 ++- > 3 files changed, 23 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > index c494afbc21b4..85fd85fe208c 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > @@ -47,13 +47,15 @@ enum rkisp1_plane { > * @fourcc: pixel format > * @fmt_type: helper filed for pixel format > * @uv_swap: if cb cr swapped, for yuv > + * @yc_swap: if y and cb/cr swapped, for yuv > * @write_format: defines how YCbCr self picture data is written to memory > * @output_format: defines sp output format > * @mbus: the mbus code on the src resizer pad that matches the pixel format > */ > struct rkisp1_capture_fmt_cfg { > u32 fourcc; > - u8 uv_swap; > + u32 uv_swap : 1; > + u32 yc_swap : 1; > u32 write_format; > u32 output_format; > u32 mbus; > @@ -1150,9 +1152,13 @@ static const struct rkisp1_capture_fmt_cfg * > rkisp1_find_fmt_cfg(const struct rkisp1_capture *cap, const u32 pixelfmt) > { > unsigned int i; > + bool yc_swap_support = > + cap->rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN; Could you move this before the previous line ? I'm tempted to write a patch at some point that will simplify this with a rkisp1_has_feature() that will... scratch that, https://lore.kernel.org/linux-media/20220719221751.4159-1-laurent.pinchart@ideasonboard.com :-) Could you rebase for v2 ? > > for (i = 0; i < cap->config->fmt_size; i++) { > - if (cap->config->fmts[i].fourcc == pixelfmt) > + const struct rkisp1_capture_fmt_cfg *fmt = &cap->config->fmts[i]; Missing blank line. I thought checkpatch.pl would warn about this. > + if (fmt->fourcc == pixelfmt && > + (!fmt->yc_swap || yc_swap_support)) > return &cap->config->fmts[i]; > } > return NULL; > @@ -1223,22 +1229,29 @@ static int rkisp1_enum_fmt_vid_cap_mplane(struct file *file, void *priv, > struct rkisp1_capture *cap = video_drvdata(file); > const struct rkisp1_capture_fmt_cfg *fmt = NULL; > unsigned int i, n = 0; > + bool yc_swap_support = > + cap->rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN; > > - if (!f->mbus_code) { > - if (f->index >= cap->config->fmt_size) > - return -EINVAL; > + if (f->index >= cap->config->fmt_size) > + return -EINVAL; > > + if (!f->mbus_code && yc_swap_support) { > fmt = &cap->config->fmts[f->index]; > f->pixelformat = fmt->fourcc; > return 0; > } I'm tempted to drop this optimization, as it makes the code more complex and only brings a small improvement to an ioctl that shouldn't be in a hot path, but that's up to you. > > for (i = 0; i < cap->config->fmt_size; i++) { > - if (cap->config->fmts[i].mbus != f->mbus_code) > + fmt = &cap->config->fmts[i]; > + > + if (!!f->mbus_code && fmt->mbus != f->mbus_code) You can s/!!// Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + continue; > + > + if (!yc_swap_support && fmt->yc_swap) > continue; > > if (n++ == f->index) { > - f->pixelformat = cap->config->fmts[i].fourcc; > + f->pixelformat = fmt->fourcc; > return 0; > } > } > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > index 38be565341d6..b0f9221a1922 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > @@ -119,6 +119,7 @@ enum rkisp1_feature { > RKISP1_FEATURE_MAIN_STRIDE = BIT(3), > RKISP1_FEATURE_DMA_34BIT = BIT(4), > RKISP1_FEATURE_SELF_PATH = BIT(5), > + RKISP1_FEATURE_MI_OUTPUT_ALIGN = BIT(6), > }; > > /* > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > index 3b549c07a9bb..a475933820fd 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > @@ -520,7 +520,8 @@ static const struct rkisp1_info imx8mp_isp_info = { > .isp_ver = IMX8MP_V10, > .features = RKISP1_FEATURE_RSZ_CROP > | RKISP1_FEATURE_MAIN_STRIDE > - | RKISP1_FEATURE_DMA_34BIT, > + | RKISP1_FEATURE_DMA_34BIT > + | RKISP1_FEATURE_MI_OUTPUT_ALIGN, > }; > > static const struct of_device_id rkisp1_of_match[] = { -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] media: rkisp1: Add UYVY as an output format 2022-07-14 11:26 [PATCH 0/2] media: rkisp1: Add support for UYVY output Paul Elder 2022-07-14 11:26 ` [PATCH 1/2] media: rkisp1: Add YC swap capability Paul Elder @ 2022-07-14 11:26 ` Paul Elder 2022-07-19 22:29 ` Laurent Pinchart 1 sibling, 1 reply; 7+ messages in thread From: Paul Elder @ 2022-07-14 11:26 UTC (permalink / raw) To: linux-media Cc: Paul Elder, Dafna Hirschfeld, Mauro Carvalho Chehab, Heiko Stuebner, laurent.pinchart, linux-rockchip, linux-arm-kernel, linux-kernel Add support for UYVY as an output format. The uv_swap bit in the MI_XTD_FORMAT_CTRL register that is used for the NV formats does not work for packed YUV formats. Thus, UYVY support is implemented via byte-swapping. This method clearly does not work for implementing support for YVYU and VYUY. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- UYVY for the self path has not been tested because no device supports it. The rk3399 has a self path, but does not have the MI_OUTPUT_ALIGN_FORMAT register and thus does not support UYVY. The i.MX8MP does support UYVY, but does not have a self path. --- .../platform/rockchip/rkisp1/rkisp1-capture.c | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c index 85fd85fe208c..77496ccef7ec 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c @@ -97,6 +97,12 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = { .uv_swap = 0, .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT, .mbus = MEDIA_BUS_FMT_YUYV8_2X8, + }, { + .fourcc = V4L2_PIX_FMT_UYVY, + .uv_swap = 0, + .yc_swap = 1, + .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT, + .mbus = MEDIA_BUS_FMT_YUYV8_2X8, }, { .fourcc = V4L2_PIX_FMT_YUV422P, .uv_swap = 0, @@ -231,6 +237,13 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = { .write_format = RKISP1_MI_CTRL_SP_WRITE_INT, .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422, .mbus = MEDIA_BUS_FMT_YUYV8_2X8, + }, { + .fourcc = V4L2_PIX_FMT_UYVY, + .uv_swap = 0, + .yc_swap = 1, + .write_format = RKISP1_MI_CTRL_SP_WRITE_INT, + .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422, + .mbus = MEDIA_BUS_FMT_YUYV8_2X8, }, { .fourcc = V4L2_PIX_FMT_YUV422P, .uv_swap = 0, @@ -464,6 +477,20 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap) rkisp1_write(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL, reg); } + /* + * uv swapping with the MI_XTD_FORMAT_CTRL register only works for + * NV12/NV21 and NV16/NV61, so instead use byte swap to support UYVY. + * YVYU and VYUY cannot be supported with this method. + */ + if (rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN) { + reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT); + if (cap->pix.cfg->yc_swap) + reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES; + else + reg &= ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES; + rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, reg); + } + rkisp1_mi_config_ctrl(cap); reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL); @@ -505,6 +532,19 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap) rkisp1_write(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL, reg); } + /* + * uv swapping with the MI_XTD_FORMAT_CTRL register only works for + * NV12/NV21 and NV16/NV61, so instead use byte swap to support UYVY. + * YVYU and VYUY cannot be supported with this method. + */ + if (rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN) { + reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT); + if (cap->pix.cfg->yc_swap) + reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_SP_BYTE_SWAP_BYTES; + else + reg &= ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_SP_BYTE_SWAP_BYTES; + rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, reg); + } rkisp1_mi_config_ctrl(cap); mi_ctrl = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL); -- 2.30.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] media: rkisp1: Add UYVY as an output format 2022-07-14 11:26 ` [PATCH 2/2] media: rkisp1: Add UYVY as an output format Paul Elder @ 2022-07-19 22:29 ` Laurent Pinchart 2022-07-28 12:52 ` paul.elder 0 siblings, 1 reply; 7+ messages in thread From: Laurent Pinchart @ 2022-07-19 22:29 UTC (permalink / raw) To: Paul Elder Cc: linux-media, Dafna Hirschfeld, Mauro Carvalho Chehab, Heiko Stuebner, linux-rockchip, linux-arm-kernel, linux-kernel Hi Paul, Thank you for the patch. On Thu, Jul 14, 2022 at 08:26:03PM +0900, Paul Elder wrote: > Add support for UYVY as an output format. The uv_swap bit in the > MI_XTD_FORMAT_CTRL register that is used for the NV formats does not > work for packed YUV formats. Thus, UYVY support is implemented via > byte-swapping. This method clearly does not work for implementing > support for YVYU and VYUY. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > UYVY for the self path has not been tested because no device supports > it. The rk3399 has a self path, but does not have the > MI_OUTPUT_ALIGN_FORMAT register and thus does not support UYVY. The > i.MX8MP does support UYVY, but does not have a self path. I'm tempted to drop it then, as the code below isn't correct given that you use the same register for both MP and SP. Let's address MP only for now. > --- > .../platform/rockchip/rkisp1/rkisp1-capture.c | 40 +++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > index 85fd85fe208c..77496ccef7ec 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > @@ -97,6 +97,12 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = { > .uv_swap = 0, > .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT, > .mbus = MEDIA_BUS_FMT_YUYV8_2X8, > + }, { > + .fourcc = V4L2_PIX_FMT_UYVY, > + .uv_swap = 0, > + .yc_swap = 1, > + .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT, > + .mbus = MEDIA_BUS_FMT_YUYV8_2X8, > }, { > .fourcc = V4L2_PIX_FMT_YUV422P, > .uv_swap = 0, > @@ -231,6 +237,13 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = { > .write_format = RKISP1_MI_CTRL_SP_WRITE_INT, > .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422, > .mbus = MEDIA_BUS_FMT_YUYV8_2X8, > + }, { > + .fourcc = V4L2_PIX_FMT_UYVY, > + .uv_swap = 0, > + .yc_swap = 1, > + .write_format = RKISP1_MI_CTRL_SP_WRITE_INT, > + .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422, > + .mbus = MEDIA_BUS_FMT_YUYV8_2X8, > }, { > .fourcc = V4L2_PIX_FMT_YUV422P, > .uv_swap = 0, > @@ -464,6 +477,20 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap) > rkisp1_write(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL, reg); > } > > + /* > + * uv swapping with the MI_XTD_FORMAT_CTRL register only works for s@uv@U/V@ > + * NV12/NV21 and NV16/NV61, so instead use byte swap to support UYVY. > + * YVYU and VYUY cannot be supported with this method. > + */ > + if (rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN) { > + reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT); > + if (cap->pix.cfg->yc_swap) > + reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES; > + else > + reg &= ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES; > + rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, reg); As the register is not initialized anywhere, it would be better to write it fully here instead of a read-modify-write cycle. Same comments below. > + } > + > rkisp1_mi_config_ctrl(cap); > > reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL); > @@ -505,6 +532,19 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap) > rkisp1_write(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL, reg); > } > > + /* > + * uv swapping with the MI_XTD_FORMAT_CTRL register only works for > + * NV12/NV21 and NV16/NV61, so instead use byte swap to support UYVY. > + * YVYU and VYUY cannot be supported with this method. > + */ > + if (rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN) { > + reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT); > + if (cap->pix.cfg->yc_swap) > + reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_SP_BYTE_SWAP_BYTES; > + else > + reg &= ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_SP_BYTE_SWAP_BYTES; > + rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, reg); > + } Missing blank line. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > rkisp1_mi_config_ctrl(cap); > > mi_ctrl = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL); -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] media: rkisp1: Add UYVY as an output format 2022-07-19 22:29 ` Laurent Pinchart @ 2022-07-28 12:52 ` paul.elder 2022-07-28 14:20 ` Laurent Pinchart 0 siblings, 1 reply; 7+ messages in thread From: paul.elder @ 2022-07-28 12:52 UTC (permalink / raw) To: Laurent Pinchart Cc: linux-media, Dafna Hirschfeld, Mauro Carvalho Chehab, Heiko Stuebner, linux-rockchip, linux-arm-kernel, linux-kernel Hi Laurent, On Wed, Jul 20, 2022 at 01:29:57AM +0300, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > > On Thu, Jul 14, 2022 at 08:26:03PM +0900, Paul Elder wrote: > > Add support for UYVY as an output format. The uv_swap bit in the > > MI_XTD_FORMAT_CTRL register that is used for the NV formats does not > > work for packed YUV formats. Thus, UYVY support is implemented via > > byte-swapping. This method clearly does not work for implementing > > support for YVYU and VYUY. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > UYVY for the self path has not been tested because no device supports > > it. The rk3399 has a self path, but does not have the > > MI_OUTPUT_ALIGN_FORMAT register and thus does not support UYVY. The > > i.MX8MP does support UYVY, but does not have a self path. > > I'm tempted to drop it then, as the code below isn't correct given that > you use the same register for both MP and SP. Let's address MP only for > now. The same register is used for both MP and SP. They just have different bits. Which is why we'd need the read-write cycle, assuming that there exists a device that has both an SP and the MI_OUTPUT_ALIGN_FORMAT register. Paul > > > --- > > .../platform/rockchip/rkisp1/rkisp1-capture.c | 40 +++++++++++++++++++ > > 1 file changed, 40 insertions(+) > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > > index 85fd85fe208c..77496ccef7ec 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > > @@ -97,6 +97,12 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = { > > .uv_swap = 0, > > .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT, > > .mbus = MEDIA_BUS_FMT_YUYV8_2X8, > > + }, { > > + .fourcc = V4L2_PIX_FMT_UYVY, > > + .uv_swap = 0, > > + .yc_swap = 1, > > + .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT, > > + .mbus = MEDIA_BUS_FMT_YUYV8_2X8, > > }, { > > .fourcc = V4L2_PIX_FMT_YUV422P, > > .uv_swap = 0, > > @@ -231,6 +237,13 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = { > > .write_format = RKISP1_MI_CTRL_SP_WRITE_INT, > > .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422, > > .mbus = MEDIA_BUS_FMT_YUYV8_2X8, > > + }, { > > + .fourcc = V4L2_PIX_FMT_UYVY, > > + .uv_swap = 0, > > + .yc_swap = 1, > > + .write_format = RKISP1_MI_CTRL_SP_WRITE_INT, > > + .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422, > > + .mbus = MEDIA_BUS_FMT_YUYV8_2X8, > > }, { > > .fourcc = V4L2_PIX_FMT_YUV422P, > > .uv_swap = 0, > > @@ -464,6 +477,20 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap) > > rkisp1_write(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL, reg); > > } > > > > + /* > > + * uv swapping with the MI_XTD_FORMAT_CTRL register only works for > > s@uv@U/V@ > > > + * NV12/NV21 and NV16/NV61, so instead use byte swap to support UYVY. > > + * YVYU and VYUY cannot be supported with this method. > > + */ > > + if (rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN) { > > + reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT); > > + if (cap->pix.cfg->yc_swap) > > + reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES; > > + else > > + reg &= ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES; > > + rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, reg); > > As the register is not initialized anywhere, it would be better to write > it fully here instead of a read-modify-write cycle. Same comments below. > > > + } > > + > > rkisp1_mi_config_ctrl(cap); > > > > reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL); > > @@ -505,6 +532,19 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap) > > rkisp1_write(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL, reg); > > } > > > > + /* > > + * uv swapping with the MI_XTD_FORMAT_CTRL register only works for > > + * NV12/NV21 and NV16/NV61, so instead use byte swap to support UYVY. > > + * YVYU and VYUY cannot be supported with this method. > > + */ > > + if (rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN) { > > + reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT); > > + if (cap->pix.cfg->yc_swap) > > + reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_SP_BYTE_SWAP_BYTES; > > + else > > + reg &= ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_SP_BYTE_SWAP_BYTES; > > + rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, reg); > > + } > > Missing blank line. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > rkisp1_mi_config_ctrl(cap); > > > > mi_ctrl = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL); _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] media: rkisp1: Add UYVY as an output format 2022-07-28 12:52 ` paul.elder @ 2022-07-28 14:20 ` Laurent Pinchart 0 siblings, 0 replies; 7+ messages in thread From: Laurent Pinchart @ 2022-07-28 14:20 UTC (permalink / raw) To: paul.elder Cc: linux-media, Dafna Hirschfeld, Mauro Carvalho Chehab, Heiko Stuebner, linux-rockchip, linux-arm-kernel, linux-kernel Hi Paul, On Thu, Jul 28, 2022 at 09:52:59PM +0900, paul.elder@ideasonboard.com wrote: > On Wed, Jul 20, 2022 at 01:29:57AM +0300, Laurent Pinchart wrote: > > On Thu, Jul 14, 2022 at 08:26:03PM +0900, Paul Elder wrote: > > > Add support for UYVY as an output format. The uv_swap bit in the > > > MI_XTD_FORMAT_CTRL register that is used for the NV formats does not > > > work for packed YUV formats. Thus, UYVY support is implemented via > > > byte-swapping. This method clearly does not work for implementing > > > support for YVYU and VYUY. > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > --- > > > UYVY for the self path has not been tested because no device supports > > > it. The rk3399 has a self path, but does not have the > > > MI_OUTPUT_ALIGN_FORMAT register and thus does not support UYVY. The > > > i.MX8MP does support UYVY, but does not have a self path. > > > > I'm tempted to drop it then, as the code below isn't correct given that > > you use the same register for both MP and SP. Let's address MP only for > > now. > > The same register is used for both MP and SP. They just have different > bits. Which is why we'd need the read-write cycle, assuming that there > exists a device that has both an SP and the MI_OUTPUT_ALIGN_FORMAT > register. Indeed, I had missed that. The documentation is confusing, the register is described as "Output align format for main path", has both mp_byte_swap and sp_byte_swap, but no sp equivalent to mp_lsb_alignment (maybe the self path doesn't support raw outputs though ?). I'm OK keeping support for both paths, but I think the MI_OUTPUT_ALIGN_FORMAT register should then be initialized to a default value somewhere. > > > --- > > > .../platform/rockchip/rkisp1/rkisp1-capture.c | 40 +++++++++++++++++++ > > > 1 file changed, 40 insertions(+) > > > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > > > index 85fd85fe208c..77496ccef7ec 100644 > > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > > > @@ -97,6 +97,12 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = { > > > .uv_swap = 0, > > > .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT, > > > .mbus = MEDIA_BUS_FMT_YUYV8_2X8, > > > + }, { > > > + .fourcc = V4L2_PIX_FMT_UYVY, > > > + .uv_swap = 0, > > > + .yc_swap = 1, > > > + .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT, > > > + .mbus = MEDIA_BUS_FMT_YUYV8_2X8, > > > }, { > > > .fourcc = V4L2_PIX_FMT_YUV422P, > > > .uv_swap = 0, > > > @@ -231,6 +237,13 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = { > > > .write_format = RKISP1_MI_CTRL_SP_WRITE_INT, > > > .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422, > > > .mbus = MEDIA_BUS_FMT_YUYV8_2X8, > > > + }, { > > > + .fourcc = V4L2_PIX_FMT_UYVY, > > > + .uv_swap = 0, > > > + .yc_swap = 1, > > > + .write_format = RKISP1_MI_CTRL_SP_WRITE_INT, > > > + .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422, > > > + .mbus = MEDIA_BUS_FMT_YUYV8_2X8, > > > }, { > > > .fourcc = V4L2_PIX_FMT_YUV422P, > > > .uv_swap = 0, > > > @@ -464,6 +477,20 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap) > > > rkisp1_write(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL, reg); > > > } > > > > > > + /* > > > + * uv swapping with the MI_XTD_FORMAT_CTRL register only works for > > > > s@uv@U/V@ > > > > > + * NV12/NV21 and NV16/NV61, so instead use byte swap to support UYVY. > > > + * YVYU and VYUY cannot be supported with this method. > > > + */ > > > + if (rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN) { > > > + reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT); > > > + if (cap->pix.cfg->yc_swap) > > > + reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES; > > > + else > > > + reg &= ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES; > > > + rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, reg); > > > > As the register is not initialized anywhere, it would be better to write > > it fully here instead of a read-modify-write cycle. Same comments below. > > > > > + } > > > + > > > rkisp1_mi_config_ctrl(cap); > > > > > > reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL); > > > @@ -505,6 +532,19 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap) > > > rkisp1_write(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL, reg); > > > } > > > > > > + /* > > > + * uv swapping with the MI_XTD_FORMAT_CTRL register only works for > > > + * NV12/NV21 and NV16/NV61, so instead use byte swap to support UYVY. > > > + * YVYU and VYUY cannot be supported with this method. > > > + */ > > > + if (rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN) { > > > + reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT); > > > + if (cap->pix.cfg->yc_swap) > > > + reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_SP_BYTE_SWAP_BYTES; > > > + else > > > + reg &= ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_SP_BYTE_SWAP_BYTES; > > > + rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, reg); > > > + } > > > > Missing blank line. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > rkisp1_mi_config_ctrl(cap); > > > > > > mi_ctrl = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL); -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-07-28 14:21 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-07-14 11:26 [PATCH 0/2] media: rkisp1: Add support for UYVY output Paul Elder 2022-07-14 11:26 ` [PATCH 1/2] media: rkisp1: Add YC swap capability Paul Elder 2022-07-19 22:23 ` Laurent Pinchart 2022-07-14 11:26 ` [PATCH 2/2] media: rkisp1: Add UYVY as an output format Paul Elder 2022-07-19 22:29 ` Laurent Pinchart 2022-07-28 12:52 ` paul.elder 2022-07-28 14:20 ` Laurent Pinchart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).