Hi Paul, On Thu, Jul 28, 2022 at 09:52:59PM +0900, paul.elder@xxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxxx> > > > > > > --- > > > 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@xxxxxxxxxxxxxxxx> > > > > > rkisp1_mi_config_ctrl(cap); > > > > > > mi_ctrl = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL); -- Regards, Laurent Pinchart