| Message ID | 20220714112603.1117335-3-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: rkisp1: Add support for UYVY output | expand |
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);
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);
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);
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);
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(+)