mbox series

[v12,00/12] media: rkisp1: Add support for i.MX8MP

Message ID 20240216095458.2919694-1-paul.elder@ideasonboard.com
Headers show
Series media: rkisp1: Add support for i.MX8MP | expand

Message

Paul Elder Feb. 16, 2024, 9:54 a.m. UTC
Twelfth time's the charm.

This patch series depends on the series "media: rkisp1: Fix shared
interrupt handling" [1]

This series extends the rkisp1 driver to support the ISP found in the
NXP i.MX8MP SoC.

The ISP IP cores in the Rockchip RK3399 (known as the "Rockchip ISP1")
and in the NXP i.MX8MP have the same origin, and have slightly diverged
over time as they are now independently developed (afaik) by Rockchip
and VeriSilicon. The latter is marketed under the name "ISP8000Nano",
and is close enough to the RK3399 ISP that it can easily be supported by
the same driver.

Patches 9 and 10 add support for UYVY output format, which can be
implemented on the ISP version in the i.MX8MP but not in the one in the
RK3399.

This version of the series specifically has been tested on a Polyhex
Debix model A with an imx219 (Raspberry Pi cam v2).

v6 added patch 11 to fix endinanness issues on raw streams on the
i.MX8MP.

In v12 patch 6 "media: rkisp1: Add version enum for i.MX8MP ISP" has
been split out from patch 12 "media: rkisp1: Add match data for i.MX8MP
ISP".

[1] https://lore.kernel.org/all/20231218-rkisp-shirq-fix-v1-0-173007628248@ideasonboard.com/

Laurent Pinchart (2):
  media: rkisp1: Add and use rkisp1_has_feature() macro
  media: rkisp1: Configure gasket on i.MX8MP

Paul Elder (10):
  media: rkisp1: Support setting memory stride for main path
  media: rkisp1: Support devices lacking self path
  media: rkisp1: Support devices lacking dual crop
  dt-bindings: media: rkisp1: Add i.MX8MP ISP to compatible
  media: rkisp1: Add version enum for i.MX8MP ISP
  media: rkisp1: Support i.MX8MP's 34-bit DMA
  media: rkisp1: Add YC swap capability
  media: rkisp1: Add UYVY as an output format
  media: rkisp1: Fix endianness on raw streams on i.MX8MP
  media: rkisp1: Add match data for i.MX8MP ISP

 .../bindings/media/rockchip-isp1.yaml         |  37 ++-
 .../platform/rockchip/rkisp1/rkisp1-capture.c | 219 +++++++++++++-----
 .../platform/rockchip/rkisp1/rkisp1-common.h  |  35 ++-
 .../platform/rockchip/rkisp1/rkisp1-dev.c     |  71 +++++-
 .../platform/rockchip/rkisp1/rkisp1-isp.c     | 131 ++++++++++-
 .../platform/rockchip/rkisp1/rkisp1-regs.h    |  35 +++
 .../platform/rockchip/rkisp1/rkisp1-resizer.c |  17 +-
 include/uapi/linux/rkisp1-config.h            |   2 +
 8 files changed, 467 insertions(+), 80 deletions(-)

Comments

Alexander Stein Feb. 16, 2024, 10:28 a.m. UTC | #1
Hi Paul,

thanks for updating this.

Am Freitag, 16. Februar 2024, 10:54:57 CET schrieb Paul Elder:
> The i.MX8MP has extra register fields in the memory interface control
> register for setting the output format, which work with the output
> alignment format register for byte-swapping and LSB/MSB alignment.
> 
> With processed and 8-bit raw streams, it doesn't cause any problems to
> not set these, but with raw streams of higher bit depth the endianness
> is swapped and the data is not aligned properly.
> 
> Add support for settings these registers and plumb them in to fix this.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
> Changes in v12:
> - replace MP_OUTPUT_FORMAT feature flag with MAIN_STRIDE
> 
> New in v6
> ---
>  .../platform/rockchip/rkisp1/rkisp1-capture.c | 93 ++++++++++++++-----
>  .../platform/rockchip/rkisp1/rkisp1-regs.h    |  8 ++
>  2 files changed, 78 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c index
> 64b1d1104e20..28a99b31581b 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> @@ -48,16 +48,20 @@ enum rkisp1_plane {
>   * @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
> + * @byte_swap: if byte pairs are swapped, for raw
>   * @write_format: defines how YCbCr self picture data is written to memory
> - * @output_format: defines sp output format
> + * @output_format_mp: defines mp output format
> + * @output_format_sp: 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;
>  	u32 uv_swap : 1;
>  	u32 yc_swap : 1;
> +	u32 byte_swap : 1;
>  	u32 write_format;
> -	u32 output_format;
> +	u32 output_format_mp;
> +	u32 output_format_sp;
>  	u32 mbus;
>  };
> 
> @@ -96,42 +100,50 @@ static const struct rkisp1_capture_fmt_cfg
> rkisp1_mp_fmts[] = { .fourcc = V4L2_PIX_FMT_YUYV,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422,
>  		.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,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YUV422P,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV16,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV61,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV16M,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV61M,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YVU422M,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
>  	/* yuv400 */
> @@ -139,6 +151,7 @@ static const struct rkisp1_capture_fmt_cfg
> rkisp1_mp_fmts[] = { .fourcc = V4L2_PIX_FMT_GREY,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV400,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
>  	/* yuv420 */
> @@ -146,81 +159,107 @@ static const struct rkisp1_capture_fmt_cfg
> rkisp1_mp_fmts[] = { .fourcc = V4L2_PIX_FMT_NV21,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV420,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV12,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV420,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV21M,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV420,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV12M,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV420,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YUV420,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV420,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YVU420,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV420,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	},
>  	/* raw */
>  	{
>  		.fourcc = V4L2_PIX_FMT_SRGGB8,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW8,
>  		.mbus = MEDIA_BUS_FMT_SRGGB8_1X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGRBG8,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW8,
>  		.mbus = MEDIA_BUS_FMT_SGRBG8_1X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGBRG8,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW8,
>  		.mbus = MEDIA_BUS_FMT_SGBRG8_1X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SBGGR8,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW8,
>  		.mbus = MEDIA_BUS_FMT_SBGGR8_1X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SRGGB10,
> +		.byte_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW10,
>  		.mbus = MEDIA_BUS_FMT_SRGGB10_1X10,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGRBG10,
> +		.byte_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW10,
>  		.mbus = MEDIA_BUS_FMT_SGRBG10_1X10,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGBRG10,
> +		.byte_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW10,
>  		.mbus = MEDIA_BUS_FMT_SGBRG10_1X10,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SBGGR10,
> +		.byte_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW10,
>  		.mbus = MEDIA_BUS_FMT_SBGGR10_1X10,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SRGGB12,
> +		.byte_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW12,
>  		.mbus = MEDIA_BUS_FMT_SRGGB12_1X12,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGRBG12,
> +		.byte_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW12,
>  		.mbus = MEDIA_BUS_FMT_SGRBG12_1X12,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGBRG12,
> +		.byte_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW12,
>  		.mbus = MEDIA_BUS_FMT_SGBRG12_1X12,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SBGGR12,
> +		.byte_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW12,
>  		.mbus = MEDIA_BUS_FMT_SBGGR12_1X12,
>  	},
>  };
> @@ -235,50 +274,50 @@ static const struct rkisp1_capture_fmt_cfg
> rkisp1_sp_fmts[] = { .fourcc = V4L2_PIX_FMT_YUYV,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.output_format_sp = 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,
> +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YUV422P,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV16,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV61,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV16M,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV61M,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YVU422M,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
>  	/* yuv400 */
> @@ -286,19 +325,19 @@ static const struct rkisp1_capture_fmt_cfg
> rkisp1_sp_fmts[] = { .fourcc = V4L2_PIX_FMT_GREY,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
>  	/* rgb */
>  	{
>  		.fourcc = V4L2_PIX_FMT_XBGR32,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
> +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_RGB565,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
> +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
>  	/* yuv420 */
> @@ -306,37 +345,37 @@ static const struct rkisp1_capture_fmt_cfg
> rkisp1_sp_fmts[] = { .fourcc = V4L2_PIX_FMT_NV21,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV12,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV21M,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV12M,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YUV420,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YVU420,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	},
>  };
> @@ -484,10 +523,12 @@ static void rkisp1_mp_config(struct rkisp1_capture
> *cap) */
>  	if (rkisp1_has_feature(rkisp1, MAIN_STRIDE)) {
>  		reg = rkisp1_read(rkisp1, 
RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT);
> -		if (cap->pix.cfg->yc_swap)
> +		if (cap->pix.cfg->yc_swap || cap->pix.cfg->byte_swap)
>  			reg |= 
RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES;
>  		else
>  			reg &= 
~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES;
> +
> +		reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_LSB_ALIGNMENT;
>  		rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, 
reg);
>  	}
> 
> @@ -554,7 +595,7 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap)
> mi_ctrl &= ~RKISP1_MI_CTRL_SP_FMT_MASK;
>  	mi_ctrl |= cap->pix.cfg->write_format |
>  		   RKISP1_MI_CTRL_SP_INPUT_YUV422 |
> -		   cap->pix.cfg->output_format |
> +		   cap->pix.cfg->output_format_sp |
>  		   RKISP1_CIF_MI_SP_AUTOUPDATE_ENABLE;
>  	rkisp1_write(rkisp1, RKISP1_CIF_MI_CTRL, mi_ctrl);
>  }
> @@ -946,6 +987,7 @@ static void rkisp1_cap_stream_enable(struct
> rkisp1_capture *cap) struct rkisp1_device *rkisp1 = cap->rkisp1;
>  	struct rkisp1_capture *other = &rkisp1->capture_devs[cap->id ^ 1];
>  	bool has_self_path = rkisp1_has_feature(rkisp1, SELF_PATH);
> +	u32 reg;
> 
>  	cap->ops->set_data_path(cap);
>  	cap->ops->config(cap);
> @@ -965,8 +1007,13 @@ static void rkisp1_cap_stream_enable(struct
> rkisp1_capture *cap) */
>  	if (!has_self_path || !other->is_streaming) {
>  		/* force cfg update */
> -		rkisp1_write(rkisp1, RKISP1_CIF_MI_INIT,
> -			     RKISP1_CIF_MI_INIT_SOFT_UPD);
> +		reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_INIT);
> +
> +		if (rkisp1_has_feature(rkisp1, MAIN_STRIDE))
> +			reg |= cap->pix.cfg->output_format_mp;

I don't have any documents regarding that ISP, but shouldn't you clear the 
bits for output_format_mp before OR'ing the new ones on top?

Best regards,
Alexander

> +
> +		reg |= RKISP1_CIF_MI_INIT_SOFT_UPD;
> +		rkisp1_write(rkisp1, RKISP1_CIF_MI_INIT, reg);
>  		rkisp1_set_next_buf(cap);
>  	}
>  	spin_unlock_irq(&cap->buf.lock);
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
> b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h index
> 3b19c8411360..762243016f05 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
> @@ -144,6 +144,14 @@
>  /* MI_INIT */
>  #define RKISP1_CIF_MI_INIT_SKIP				BIT(2)
>  #define RKISP1_CIF_MI_INIT_SOFT_UPD			BIT(4)
> +#define RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV400		(0 << 5)
> +#define RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV420		(1 << 5)
> +#define RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422		(2 << 5)
> +#define RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV444		(3 << 5)
> +#define RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW12		(4 << 5)
> +#define RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW8		(5 << 5)
> +#define RKISP1_CIF_MI_INIT_MP_OUTPUT_JPEG		(6 << 5)
> +#define RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW10		(7 << 5)
> 
>  /* MI_CTRL_SHD */
>  #define RKISP1_CIF_MI_CTRL_SHD_MP_IN_ENABLED		BIT(0)
Alexander Stein Feb. 16, 2024, 10:31 a.m. UTC | #2
Hi Paul,

thanks for the update.

Am Freitag, 16. Februar 2024, 10:54:54 CET schrieb Paul Elder:
> On the ISP that is integrated in the i.MX8MP, DMA addresses have been
> extended to 34 bits, with the 32 MSBs stored in the DMA address
> registers and the 2 LSBs set to 0.
> 
> To support this:
> - Shift the addresses to the right by 2 when writing to registers
> - Set the dma mask to 34 bits
> - Use dma_addr_t instead of u32 when storing the addresses
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> Tested-by: Adam Ford <aford173@gmail.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> Changes since v5:
> 
> - Improve the commit message
> 
> Changes since v4:
> 
> - Squash in fix from Tomi:
>   -
> https://gitlab.com/ideasonboard/nxp/linux/-/commit/d6477fe673b1c0d05d12ae21
> d8db9a03b07e7fea
> 
> Changes since v2:
> 
> - Document the RKISP1_FEATURE_DMA_34BIT bit
> - Use the rkisp1_has_feature() macro
> ---
>  .../platform/rockchip/rkisp1/rkisp1-capture.c | 20 ++++++++++---------
>  .../platform/rockchip/rkisp1/rkisp1-common.h  |  4 +++-
>  .../platform/rockchip/rkisp1/rkisp1-dev.c     |  8 ++++++++
>  3 files changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c index
> ca95f62822fa..1ee7639c42b7 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> @@ -648,11 +648,13 @@ static void rkisp1_dummy_buf_destroy(struct
> rkisp1_capture *cap)
> 
>  static void rkisp1_set_next_buf(struct rkisp1_capture *cap)
>  {
> +	u8 shift = rkisp1_has_feature(cap->rkisp1, DMA_34BIT) ? 2 : 0;
> +
>  	cap->buf.curr = cap->buf.next;
>  	cap->buf.next = NULL;
> 
>  	if (!list_empty(&cap->buf.queue)) {
> -		u32 *buff_addr;
> +		dma_addr_t *buff_addr;
> 
>  		cap->buf.next = list_first_entry(&cap->buf.queue, struct 
rkisp1_buffer,
> queue); list_del(&cap->buf.next->queue);
> @@ -660,7 +662,7 @@ static void rkisp1_set_next_buf(struct rkisp1_capture
> *cap) buff_addr = cap->buf.next->buff_addr;
> 
>  		rkisp1_write(cap->rkisp1, cap->config->mi.y_base_ad_init,
> -			     buff_addr[RKISP1_PLANE_Y]);
> +			     buff_addr[RKISP1_PLANE_Y] >> shift);
>  		/*
>  		 * In order to support grey format we capture
>  		 * YUV422 planar format from the camera and
> @@ -669,17 +671,17 @@ static void rkisp1_set_next_buf(struct rkisp1_capture
> *cap) if (cap->pix.cfg->fourcc == V4L2_PIX_FMT_GREY) {
>  			rkisp1_write(cap->rkisp1,
>  				     cap->config->mi.cb_base_ad_init,
> -				     cap->buf.dummy.dma_addr);
> +				     cap->buf.dummy.dma_addr >> 
shift);
>  			rkisp1_write(cap->rkisp1,
>  				     cap->config->mi.cr_base_ad_init,
> -				     cap->buf.dummy.dma_addr);
> +				     cap->buf.dummy.dma_addr >> 
shift);
>  		} else {
>  			rkisp1_write(cap->rkisp1,
>  				     cap->config->mi.cb_base_ad_init,
> -				     buff_addr[RKISP1_PLANE_CB]);
> +				     buff_addr[RKISP1_PLANE_CB] >> 
shift);
>  			rkisp1_write(cap->rkisp1,
>  				     cap->config->mi.cr_base_ad_init,
> -				     buff_addr[RKISP1_PLANE_CR]);
> +				     buff_addr[RKISP1_PLANE_CR] >> 
shift);
>  		}
>  	} else {
>  		/*
> @@ -687,11 +689,11 @@ static void rkisp1_set_next_buf(struct rkisp1_capture
> *cap) * throw data if there is no available buffer.
>  		 */
>  		rkisp1_write(cap->rkisp1, cap->config->mi.y_base_ad_init,
> -			     cap->buf.dummy.dma_addr);
> +			     cap->buf.dummy.dma_addr >> shift);
>  		rkisp1_write(cap->rkisp1, cap->config->mi.cb_base_ad_init,
> -			     cap->buf.dummy.dma_addr);
> +			     cap->buf.dummy.dma_addr >> shift);
>  		rkisp1_write(cap->rkisp1, cap->config->mi.cr_base_ad_init,
> -			     cap->buf.dummy.dma_addr);
> +			     cap->buf.dummy.dma_addr >> shift);
>  	}
> 
>  	/* Set plane offsets */
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h index
> 69940014d597..26573f6ae575 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -114,6 +114,7 @@ enum rkisp1_isp_pad {
>   * @RKISP1_FEATURE_MAIN_STRIDE: The ISP supports configurable stride on the
> main path * @RKISP1_FEATURE_SELF_PATH: The ISP has a self path
>   * @RKISP1_FEATURE_DUAL_CROP: The ISP has the dual crop block at the
> resizer input + * @RKISP1_FEATURE_DMA_34BIT: The ISP uses 34-bit DMA
> addresses
>   *
>   * The ISP features are stored in a bitmask in &rkisp1_info.features and
> allow * the driver to implement support for features present in some ISP
> versions @@ -124,6 +125,7 @@ enum rkisp1_feature {
>  	RKISP1_FEATURE_MAIN_STRIDE = BIT(1),
>  	RKISP1_FEATURE_SELF_PATH = BIT(2),
>  	RKISP1_FEATURE_DUAL_CROP = BIT(3),
> +	RKISP1_FEATURE_DMA_34BIT = BIT(4),
>  };
> 
>  #define rkisp1_has_feature(rkisp1, feature) \
> @@ -239,7 +241,7 @@ struct rkisp1_vdev_node {
>  struct rkisp1_buffer {
>  	struct vb2_v4l2_buffer vb;
>  	struct list_head queue;
> -	u32 buff_addr[VIDEO_MAX_PLANES];
> +	dma_addr_t buff_addr[VIDEO_MAX_PLANES];
>  };
> 
>  /*
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c index
> d0a3a13d9dd7..54a62487a4e8 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -552,6 +552,7 @@ static int rkisp1_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct rkisp1_device *rkisp1;
>  	struct v4l2_device *v4l2_dev;
> +	unsigned long long dma_mask;

The signature for dma_set_mask_and_coherent() uses u64 for the mask, so I 
would use the same type here as well.

Best regards,
Alexander

>  	unsigned int i;
>  	int ret, irq;
>  	u32 cif_id;
> @@ -566,6 +567,13 @@ static int rkisp1_probe(struct platform_device *pdev)
>  	dev_set_drvdata(dev, rkisp1);
>  	rkisp1->dev = dev;
> 
> +	dma_mask = rkisp1_has_feature(rkisp1, DMA_34BIT) ? DMA_BIT_MASK(34) 
:
> +							   
DMA_BIT_MASK(32);
> +
> +	ret = dma_set_mask_and_coherent(dev, dma_mask);
> +	if (ret)
> +		return ret;
> +
>  	mutex_init(&rkisp1->stream_lock);
> 
>  	rkisp1->base_addr = devm_platform_ioremap_resource(pdev, 0);
Laurent Pinchart Feb. 18, 2024, 5:45 p.m. UTC | #3
Hi Paul,

Thank you for the patch.

On Fri, Feb 16, 2024 at 06:54:50PM +0900, Paul Elder wrote:
> Some versions of the ISP supported by the rkisp1 driver, such as the ISP
> in the i.MX8MP, lack the dual crop registers and don't support cropping
> at the resizer input. They instead rely on cropping in the Image
> Stabilization module, at the output of the ISP, to modify the resizer
> input size and implement digital zoom.
> 
> Add a dual crop feature flag to distinguish the versions of the ISP that
> support dual crop from those that don't, and make sure that the sink
> crop is set to the sink format when dual crop is not supported.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> Tested-by: Adam Ford <aford173@gmail.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> Changes in v12:
> - Remove mention of moving resizer input crop to image stabilizer from
>   commit message
> - Make sure the sink crop is set to the sink format when dual crop is
>   not supported
> ---
>  .../media/platform/rockchip/rkisp1/rkisp1-common.h    |  2 ++
>  drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c   |  6 ++++--
>  .../media/platform/rockchip/rkisp1/rkisp1-resizer.c   | 11 +++++++----
>  3 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index f7c251f79aa9..219d4a2547aa 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -112,6 +112,7 @@ enum rkisp1_isp_pad {
>   * @RKISP1_FEATURE_MIPI_CSI2: The ISP has an internal MIPI CSI-2 receiver
>   * @RKISP1_FEATURE_MAIN_STRIDE: The ISP supports configurable stride on the main path
>   * @RKISP1_FEATURE_SELF_PATH: The ISP has a self path
> + * @RKISP1_FEATURE_DUAL_CROP: The ISP has the dual crop block at the resizer input
>   *
>   * The ISP features are stored in a bitmask in &rkisp1_info.features and allow
>   * the driver to implement support for features present in some ISP versions
> @@ -121,6 +122,7 @@ enum rkisp1_feature {
>  	RKISP1_FEATURE_MIPI_CSI2 = BIT(0),
>  	RKISP1_FEATURE_MAIN_STRIDE = BIT(1),
>  	RKISP1_FEATURE_SELF_PATH = BIT(2),
> +	RKISP1_FEATURE_DUAL_CROP = BIT(3),
>  };
>  
>  #define rkisp1_has_feature(rkisp1, feature) \
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index f48a21985b18..2e40c376cab5 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -507,7 +507,8 @@ static const struct rkisp1_info px30_isp_info = {
>  	.isr_size = ARRAY_SIZE(px30_isp_isrs),
>  	.isp_ver = RKISP1_V12,
>  	.features = RKISP1_FEATURE_MIPI_CSI2
> -		  | RKISP1_FEATURE_SELF_PATH,
> +		  | RKISP1_FEATURE_SELF_PATH
> +		  | RKISP1_FEATURE_DUAL_CROP,
>  };
>  
>  static const char * const rk3399_isp_clks[] = {
> @@ -527,7 +528,8 @@ static const struct rkisp1_info rk3399_isp_info = {
>  	.isr_size = ARRAY_SIZE(rk3399_isp_isrs),
>  	.isp_ver = RKISP1_V10,
>  	.features = RKISP1_FEATURE_MIPI_CSI2
> -		  | RKISP1_FEATURE_SELF_PATH,
> +		  | RKISP1_FEATURE_SELF_PATH
> +		  | RKISP1_FEATURE_DUAL_CROP,
>  };
>  
>  static const struct of_device_id rkisp1_of_match[] = {
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
> index dd77a31e6014..755d319b568e 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
> @@ -447,8 +447,9 @@ static void rkisp1_rsz_set_sink_crop(struct rkisp1_resizer *rsz,
>  	/* Not crop for MP bayer raw data */

	/* Not crop for MP bayer raw data, or for devices lacking dual crop. */

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I can handle this when applying.

>  	mbus_info = rkisp1_mbus_info_get_by_code(sink_fmt->code);
>  
> -	if (rsz->id == RKISP1_MAINPATH &&
> -	    mbus_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
> +	if ((rsz->id == RKISP1_MAINPATH &&
> +	     mbus_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) ||
> +	    !rkisp1_has_feature(rsz->rkisp1, DUAL_CROP)) {
>  		sink_crop->left = 0;
>  		sink_crop->top = 0;
>  		sink_crop->width = sink_fmt->width;
> @@ -635,7 +636,8 @@ static int rkisp1_rsz_s_stream(struct v4l2_subdev *sd, int enable)
>  	struct v4l2_subdev_state *sd_state;
>  
>  	if (!enable) {
> -		rkisp1_dcrop_disable(rsz, RKISP1_SHADOW_REGS_ASYNC);
> +		if (rkisp1_has_feature(rkisp1, DUAL_CROP))
> +			rkisp1_dcrop_disable(rsz, RKISP1_SHADOW_REGS_ASYNC);
>  		rkisp1_rsz_disable(rsz, RKISP1_SHADOW_REGS_ASYNC);
>  		return 0;
>  	}
> @@ -646,7 +648,8 @@ static int rkisp1_rsz_s_stream(struct v4l2_subdev *sd, int enable)
>  	sd_state = v4l2_subdev_lock_and_get_active_state(sd);
>  
>  	rkisp1_rsz_config(rsz, sd_state, when);
> -	rkisp1_dcrop_config(rsz, sd_state);
> +	if (rkisp1_has_feature(rkisp1, DUAL_CROP))
> +		rkisp1_dcrop_config(rsz, sd_state);
>  
>  	v4l2_subdev_unlock_state(sd_state);
>
Laurent Pinchart Feb. 18, 2024, 6:27 p.m. UTC | #4
Hi Paul,

Thank you for the patch.

On Fri, Feb 16, 2024 at 06:54:52PM +0900, Paul Elder wrote:
> Add to the version enum an entry for the i.MX8MP ISP.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
> New in v12:
> - split out from "media: rkisp1: Add match data for i.MX8MP ISP"
> - changed the version enum name
> ---
>  include/uapi/linux/rkisp1-config.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> index 730673ecc63d..2d1c448a6ab8 100644
> --- a/include/uapi/linux/rkisp1-config.h
> +++ b/include/uapi/linux/rkisp1-config.h
> @@ -179,12 +179,14 @@
>   * @RKISP1_V11: declared in the original vendor code, but not used
>   * @RKISP1_V12: used at least in rk3326 and px30
>   * @RKISP1_V13: used at least in rk1808
> + * @RKISP1_V_IMX8MP: used in at least imx8mp

We need to also update the comments that references version numbers in
the same file. I'll send a v12.1 as a reply.

>   */
>  enum rkisp1_cif_isp_version {
>  	RKISP1_V10 = 10,
>  	RKISP1_V11,
>  	RKISP1_V12,
>  	RKISP1_V13,
> +	RKISP1_V_IMX8MP,
>  };
>  
>  enum rkisp1_cif_isp_histogram_mode {
Laurent Pinchart Feb. 18, 2024, 6:53 p.m. UTC | #5
Hi Alexander,

On Fri, Feb 16, 2024 at 11:31:46AM +0100, Alexander Stein wrote:
> Hi Paul,
> 
> thanks for the update.
> 
> Am Freitag, 16. Februar 2024, 10:54:54 CET schrieb Paul Elder:
> > On the ISP that is integrated in the i.MX8MP, DMA addresses have been
> > extended to 34 bits, with the 32 MSBs stored in the DMA address
> > registers and the 2 LSBs set to 0.
> > 
> > To support this:
> > - Shift the addresses to the right by 2 when writing to registers
> > - Set the dma mask to 34 bits
> > - Use dma_addr_t instead of u32 when storing the addresses
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > Tested-by: Adam Ford <aford173@gmail.com>
> > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > ---
> > Changes since v5:
> > 
> > - Improve the commit message
> > 
> > Changes since v4:
> > 
> > - Squash in fix from Tomi:
> >   -
> > https://gitlab.com/ideasonboard/nxp/linux/-/commit/d6477fe673b1c0d05d12ae21
> > d8db9a03b07e7fea
> > 
> > Changes since v2:
> > 
> > - Document the RKISP1_FEATURE_DMA_34BIT bit
> > - Use the rkisp1_has_feature() macro
> > ---
> >  .../platform/rockchip/rkisp1/rkisp1-capture.c | 20 ++++++++++---------
> >  .../platform/rockchip/rkisp1/rkisp1-common.h  |  4 +++-
> >  .../platform/rockchip/rkisp1/rkisp1-dev.c     |  8 ++++++++
> >  3 files changed, 22 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c index
> > ca95f62822fa..1ee7639c42b7 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > @@ -648,11 +648,13 @@ static void rkisp1_dummy_buf_destroy(struct
> > rkisp1_capture *cap)
> > 
> >  static void rkisp1_set_next_buf(struct rkisp1_capture *cap)
> >  {
> > +	u8 shift = rkisp1_has_feature(cap->rkisp1, DMA_34BIT) ? 2 : 0;
> > +
> >  	cap->buf.curr = cap->buf.next;
> >  	cap->buf.next = NULL;
> > 
> >  	if (!list_empty(&cap->buf.queue)) {
> > -		u32 *buff_addr;
> > +		dma_addr_t *buff_addr;
> > 
> >  		cap->buf.next = list_first_entry(&cap->buf.queue, struct 
> rkisp1_buffer,

On a side note, any chance your mail client would have an option to
avoid this kind of wrapping ? :-)

> > queue); list_del(&cap->buf.next->queue);
> > @@ -660,7 +662,7 @@ static void rkisp1_set_next_buf(struct rkisp1_capture
> > *cap) buff_addr = cap->buf.next->buff_addr;
> > 
> >  		rkisp1_write(cap->rkisp1, cap->config->mi.y_base_ad_init,
> > -			     buff_addr[RKISP1_PLANE_Y]);
> > +			     buff_addr[RKISP1_PLANE_Y] >> shift);
> >  		/*
> >  		 * In order to support grey format we capture
> >  		 * YUV422 planar format from the camera and
> > @@ -669,17 +671,17 @@ static void rkisp1_set_next_buf(struct rkisp1_capture
> > *cap) if (cap->pix.cfg->fourcc == V4L2_PIX_FMT_GREY) {
> >  			rkisp1_write(cap->rkisp1,
> >  				     cap->config->mi.cb_base_ad_init,
> > -				     cap->buf.dummy.dma_addr);
> > +				     cap->buf.dummy.dma_addr >> 
> shift);
> >  			rkisp1_write(cap->rkisp1,
> >  				     cap->config->mi.cr_base_ad_init,
> > -				     cap->buf.dummy.dma_addr);
> > +				     cap->buf.dummy.dma_addr >> 
> shift);
> >  		} else {
> >  			rkisp1_write(cap->rkisp1,
> >  				     cap->config->mi.cb_base_ad_init,
> > -				     buff_addr[RKISP1_PLANE_CB]);
> > +				     buff_addr[RKISP1_PLANE_CB] >> 
> shift);
> >  			rkisp1_write(cap->rkisp1,
> >  				     cap->config->mi.cr_base_ad_init,
> > -				     buff_addr[RKISP1_PLANE_CR]);
> > +				     buff_addr[RKISP1_PLANE_CR] >> 
> shift);
> >  		}
> >  	} else {
> >  		/*
> > @@ -687,11 +689,11 @@ static void rkisp1_set_next_buf(struct rkisp1_capture
> > *cap) * throw data if there is no available buffer.
> >  		 */
> >  		rkisp1_write(cap->rkisp1, cap->config->mi.y_base_ad_init,
> > -			     cap->buf.dummy.dma_addr);
> > +			     cap->buf.dummy.dma_addr >> shift);
> >  		rkisp1_write(cap->rkisp1, cap->config->mi.cb_base_ad_init,
> > -			     cap->buf.dummy.dma_addr);
> > +			     cap->buf.dummy.dma_addr >> shift);
> >  		rkisp1_write(cap->rkisp1, cap->config->mi.cr_base_ad_init,
> > -			     cap->buf.dummy.dma_addr);
> > +			     cap->buf.dummy.dma_addr >> shift);
> >  	}
> > 
> >  	/* Set plane offsets */
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h index
> > 69940014d597..26573f6ae575 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > @@ -114,6 +114,7 @@ enum rkisp1_isp_pad {
> >   * @RKISP1_FEATURE_MAIN_STRIDE: The ISP supports configurable stride on the
> > main path * @RKISP1_FEATURE_SELF_PATH: The ISP has a self path
> >   * @RKISP1_FEATURE_DUAL_CROP: The ISP has the dual crop block at the
> > resizer input + * @RKISP1_FEATURE_DMA_34BIT: The ISP uses 34-bit DMA
> > addresses
> >   *
> >   * The ISP features are stored in a bitmask in &rkisp1_info.features and
> > allow * the driver to implement support for features present in some ISP
> > versions @@ -124,6 +125,7 @@ enum rkisp1_feature {
> >  	RKISP1_FEATURE_MAIN_STRIDE = BIT(1),
> >  	RKISP1_FEATURE_SELF_PATH = BIT(2),
> >  	RKISP1_FEATURE_DUAL_CROP = BIT(3),
> > +	RKISP1_FEATURE_DMA_34BIT = BIT(4),
> >  };
> > 
> >  #define rkisp1_has_feature(rkisp1, feature) \
> > @@ -239,7 +241,7 @@ struct rkisp1_vdev_node {
> >  struct rkisp1_buffer {
> >  	struct vb2_v4l2_buffer vb;
> >  	struct list_head queue;
> > -	u32 buff_addr[VIDEO_MAX_PLANES];
> > +	dma_addr_t buff_addr[VIDEO_MAX_PLANES];
> >  };
> > 
> >  /*
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c index
> > d0a3a13d9dd7..54a62487a4e8 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > @@ -552,6 +552,7 @@ static int rkisp1_probe(struct platform_device *pdev)
> >  	struct device *dev = &pdev->dev;
> >  	struct rkisp1_device *rkisp1;
> >  	struct v4l2_device *v4l2_dev;
> > +	unsigned long long dma_mask;
> 
> The signature for dma_set_mask_and_coherent() uses u64 for the mask, so I 
> would use the same type here as well.

I'll do so in v13.

> >  	unsigned int i;
> >  	int ret, irq;
> >  	u32 cif_id;
> > @@ -566,6 +567,13 @@ static int rkisp1_probe(struct platform_device *pdev)
> >  	dev_set_drvdata(dev, rkisp1);
> >  	rkisp1->dev = dev;
> > 
> > +	dma_mask = rkisp1_has_feature(rkisp1, DMA_34BIT) ? DMA_BIT_MASK(34) 
> :
> > +							   
> DMA_BIT_MASK(32);
> > +
> > +	ret = dma_set_mask_and_coherent(dev, dma_mask);
> > +	if (ret)
> > +		return ret;
> > +
> >  	mutex_init(&rkisp1->stream_lock);
> > 
> >  	rkisp1->base_addr = devm_platform_ioremap_resource(pdev, 0);
Laurent Pinchart Feb. 18, 2024, 8:01 p.m. UTC | #6
On Fri, Feb 16, 2024 at 11:28:36AM +0100, Alexander Stein wrote:
> Hi Paul,
> 
> thanks for updating this.
> 
> Am Freitag, 16. Februar 2024, 10:54:57 CET schrieb Paul Elder:
> > The i.MX8MP has extra register fields in the memory interface control
> > register for setting the output format, which work with the output
> > alignment format register for byte-swapping and LSB/MSB alignment.
> > 
> > With processed and 8-bit raw streams, it doesn't cause any problems to
> > not set these, but with raw streams of higher bit depth the endianness
> > is swapped and the data is not aligned properly.
> > 
> > Add support for settings these registers and plumb them in to fix this.

s/settings/setting/

> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> > Changes in v12:
> > - replace MP_OUTPUT_FORMAT feature flag with MAIN_STRIDE
> > 
> > New in v6
> > ---
> >  .../platform/rockchip/rkisp1/rkisp1-capture.c | 93 ++++++++++++++-----
> >  .../platform/rockchip/rkisp1/rkisp1-regs.h    |  8 ++
> >  2 files changed, 78 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c index
> > 64b1d1104e20..28a99b31581b 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > @@ -48,16 +48,20 @@ enum rkisp1_plane {
> >   * @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
> > + * @byte_swap: if byte pairs are swapped, for raw
> >   * @write_format: defines how YCbCr self picture data is written to memory
> > - * @output_format: defines sp output format
> > + * @output_format_mp: defines mp output format
> > + * @output_format_sp: defines sp output format

I don't see any format that defines both output_format_mp and
output_format_sp. Unless you want to merge the rkisp1_mp_fmts and
rkisp1_sp_fmts arrays (I'm not sure it would be doable), you could use a
single output_format field. Only the description needs to be updated.

> >   * @mbus: the mbus code on the src resizer pad that matches the pixel
> > format */
> >  struct rkisp1_capture_fmt_cfg {
> >  	u32 fourcc;
> >  	u32 uv_swap : 1;
> >  	u32 yc_swap : 1;
> > +	u32 byte_swap : 1;
> >  	u32 write_format;
> > -	u32 output_format;
> > +	u32 output_format_mp;
> > +	u32 output_format_sp;
> >  	u32 mbus;
> >  };
> > 
> > @@ -96,42 +100,50 @@ static const struct rkisp1_capture_fmt_cfg
> > rkisp1_mp_fmts[] = { .fourcc = V4L2_PIX_FMT_YUYV,
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422,
> >  		.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,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_YUV422P,
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_NV16,
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_NV61,
> >  		.uv_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_NV16M,
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_NV61M,
> >  		.uv_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_YVU422M,
> >  		.uv_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	},
> >  	/* yuv400 */
> > @@ -139,6 +151,7 @@ static const struct rkisp1_capture_fmt_cfg
> > rkisp1_mp_fmts[] = { .fourcc = V4L2_PIX_FMT_GREY,
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV400,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	},
> >  	/* yuv420 */
> > @@ -146,81 +159,107 @@ static const struct rkisp1_capture_fmt_cfg
> > rkisp1_mp_fmts[] = { .fourcc = V4L2_PIX_FMT_NV21,
> >  		.uv_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV420,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_NV12,
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV420,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_NV21M,
> >  		.uv_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV420,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_NV12M,
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV420,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_YUV420,
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV420,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_YVU420,
> >  		.uv_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV420,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> >  	},
> >  	/* raw */
> >  	{
> >  		.fourcc = V4L2_PIX_FMT_SRGGB8,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW8,
> >  		.mbus = MEDIA_BUS_FMT_SRGGB8_1X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_SGRBG8,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW8,
> >  		.mbus = MEDIA_BUS_FMT_SGRBG8_1X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_SGBRG8,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW8,
> >  		.mbus = MEDIA_BUS_FMT_SGBRG8_1X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_SBGGR8,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW8,
> >  		.mbus = MEDIA_BUS_FMT_SBGGR8_1X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_SRGGB10,
> > +		.byte_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW10,
> >  		.mbus = MEDIA_BUS_FMT_SRGGB10_1X10,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_SGRBG10,
> > +		.byte_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW10,
> >  		.mbus = MEDIA_BUS_FMT_SGRBG10_1X10,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_SGBRG10,
> > +		.byte_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW10,
> >  		.mbus = MEDIA_BUS_FMT_SGBRG10_1X10,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_SBGGR10,
> > +		.byte_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW10,
> >  		.mbus = MEDIA_BUS_FMT_SBGGR10_1X10,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_SRGGB12,
> > +		.byte_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW12,
> >  		.mbus = MEDIA_BUS_FMT_SRGGB12_1X12,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_SGRBG12,
> > +		.byte_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW12,
> >  		.mbus = MEDIA_BUS_FMT_SGRBG12_1X12,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_SGBRG12,
> > +		.byte_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW12,
> >  		.mbus = MEDIA_BUS_FMT_SGBRG12_1X12,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_SBGGR12,
> > +		.byte_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> > +		.output_format_mp = RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW12,
> >  		.mbus = MEDIA_BUS_FMT_SBGGR12_1X12,
> >  	},
> >  };
> > @@ -235,50 +274,50 @@ static const struct rkisp1_capture_fmt_cfg
> > rkisp1_sp_fmts[] = { .fourcc = V4L2_PIX_FMT_YUYV,
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
> > -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> > +		.output_format_sp = 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,
> > +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_YUV422P,
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> > +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_NV16,
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
> > -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> > +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_NV61,
> >  		.uv_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
> > -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> > +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_NV16M,
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
> > -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> > +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_NV61M,
> >  		.uv_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
> > -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> > +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_YVU422M,
> >  		.uv_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> > +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	},
> >  	/* yuv400 */
> > @@ -286,19 +325,19 @@ static const struct rkisp1_capture_fmt_cfg
> > rkisp1_sp_fmts[] = { .fourcc = V4L2_PIX_FMT_GREY,
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> > +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	},
> >  	/* rgb */
> >  	{
> >  		.fourcc = V4L2_PIX_FMT_XBGR32,
> >  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
> > +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_RGB565,
> >  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
> > +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	},
> >  	/* yuv420 */
> > @@ -306,37 +345,37 @@ static const struct rkisp1_capture_fmt_cfg
> > rkisp1_sp_fmts[] = { .fourcc = V4L2_PIX_FMT_NV21,
> >  		.uv_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
> > -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> > +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_NV12,
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
> > -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> > +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_NV21M,
> >  		.uv_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
> > -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> > +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_NV12M,
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
> > -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> > +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_YUV420,
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> > +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_YVU420,
> >  		.uv_swap = 1,
> >  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> > +		.output_format_sp = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> >  	},
> >  };
> > @@ -484,10 +523,12 @@ static void rkisp1_mp_config(struct rkisp1_capture
> > *cap) */
> >  	if (rkisp1_has_feature(rkisp1, MAIN_STRIDE)) {
> >  		reg = rkisp1_read(rkisp1, 
> RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT);
> > -		if (cap->pix.cfg->yc_swap)
> > +		if (cap->pix.cfg->yc_swap || cap->pix.cfg->byte_swap)
> >  			reg |= 
> RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES;
> >  		else
> >  			reg &= 
> ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES;
> > +
> > +		reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_LSB_ALIGNMENT;
> >  		rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, 
> reg);
> >  	}
> > 
> > @@ -554,7 +595,7 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap)
> > mi_ctrl &= ~RKISP1_MI_CTRL_SP_FMT_MASK;
> >  	mi_ctrl |= cap->pix.cfg->write_format |
> >  		   RKISP1_MI_CTRL_SP_INPUT_YUV422 |
> > -		   cap->pix.cfg->output_format |
> > +		   cap->pix.cfg->output_format_sp |
> >  		   RKISP1_CIF_MI_SP_AUTOUPDATE_ENABLE;
> >  	rkisp1_write(rkisp1, RKISP1_CIF_MI_CTRL, mi_ctrl);
> >  }
> > @@ -946,6 +987,7 @@ static void rkisp1_cap_stream_enable(struct
> > rkisp1_capture *cap) struct rkisp1_device *rkisp1 = cap->rkisp1;
> >  	struct rkisp1_capture *other = &rkisp1->capture_devs[cap->id ^ 1];
> >  	bool has_self_path = rkisp1_has_feature(rkisp1, SELF_PATH);
> > +	u32 reg;
> > 
> >  	cap->ops->set_data_path(cap);
> >  	cap->ops->config(cap);
> > @@ -965,8 +1007,13 @@ static void rkisp1_cap_stream_enable(struct
> > rkisp1_capture *cap) */
> >  	if (!has_self_path || !other->is_streaming) {
> >  		/* force cfg update */
> > -		rkisp1_write(rkisp1, RKISP1_CIF_MI_INIT,
> > -			     RKISP1_CIF_MI_INIT_SOFT_UPD);
> > +		reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_INIT);
> > +
> > +		if (rkisp1_has_feature(rkisp1, MAIN_STRIDE))
> > +			reg |= cap->pix.cfg->output_format_mp;
> 
> I don't have any documents regarding that ISP, but shouldn't you clear the 
> bits for output_format_mp before OR'ing the new ones on top?

I think it would be even better to set the output format in
rkisp1_mp_config(), writing the whole register there.

> > +
> > +		reg |= RKISP1_CIF_MI_INIT_SOFT_UPD;
> > +		rkisp1_write(rkisp1, RKISP1_CIF_MI_INIT, reg);
> >  		rkisp1_set_next_buf(cap);
> >  	}
> >  	spin_unlock_irq(&cap->buf.lock);
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
> > b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h index
> > 3b19c8411360..762243016f05 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
> > @@ -144,6 +144,14 @@
> >  /* MI_INIT */
> >  #define RKISP1_CIF_MI_INIT_SKIP				BIT(2)
> >  #define RKISP1_CIF_MI_INIT_SOFT_UPD			BIT(4)
> > +#define RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV400		(0 << 5)
> > +#define RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV420		(1 << 5)
> > +#define RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV422		(2 << 5)
> > +#define RKISP1_CIF_MI_INIT_MP_OUTPUT_YUV444		(3 << 5)
> > +#define RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW12		(4 << 5)
> > +#define RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW8		(5 << 5)
> > +#define RKISP1_CIF_MI_INIT_MP_OUTPUT_JPEG		(6 << 5)
> > +#define RKISP1_CIF_MI_INIT_MP_OUTPUT_RAW10		(7 << 5)
> > 
> >  /* MI_CTRL_SHD */
> >  #define RKISP1_CIF_MI_CTRL_SHD_MP_IN_ENABLED		BIT(0)
Laurent Pinchart Feb. 18, 2024, 8:03 p.m. UTC | #7
Hi Paul,

Thank you for the patch.

On Fri, Feb 16, 2024 at 06:54:58PM +0900, Paul Elder wrote:
> Add match data to the rkisp1 driver to match the i.MX8MP ISP.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> Tested-by: Adam Ford <aford173@gmail.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> Changes in v12:
> - move out adding the version enum
> ---
>  .../platform/rockchip/rkisp1/rkisp1-dev.c     | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index 54a62487a4e8..a6b47f0af467 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -533,6 +533,26 @@ static const struct rkisp1_info rk3399_isp_info = {
>  		  | RKISP1_FEATURE_DUAL_CROP,
>  };
>  
> +static const char * const imx8mp_isp_clks[] = {
> +	"isp",
> +	"hclk",
> +	"aclk",
> +};
> +
> +static const struct rkisp1_isr_data imx8mp_isp_isrs[] = {
> +	{ NULL, rkisp1_isr, BIT(RKISP1_IRQ_ISP) | BIT(RKISP1_IRQ_MI) },
> +};
> +
> +static const struct rkisp1_info imx8mp_isp_info = {
> +	.clks = imx8mp_isp_clks,
> +	.clk_size = ARRAY_SIZE(imx8mp_isp_clks),
> +	.isrs = imx8mp_isp_isrs,
> +	.isr_size = ARRAY_SIZE(imx8mp_isp_isrs),
> +	.isp_ver = RKISP1_V_IMX8MP,
> +	.features = RKISP1_FEATURE_MAIN_STRIDE
> +		  | RKISP1_FEATURE_DMA_34BIT,
> +};
> +
>  static const struct of_device_id rkisp1_of_match[] = {
>  	{
>  		.compatible = "rockchip,px30-cif-isp",
> @@ -542,6 +562,10 @@ static const struct of_device_id rkisp1_of_match[] = {
>  		.compatible = "rockchip,rk3399-cif-isp",
>  		.data = &rk3399_isp_info,
>  	},
> +	{
> +		.compatible = "fsl,imx8mp-isp",
> +		.data = &imx8mp_isp_info,
> +	},
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, rkisp1_of_match);