Re: [PATCH v2 07/13] MEDIA: PLATFORM: MEDIATEK: ADD ISP_7X CAM-RAW UNIT

["CK Hu (胡俊光)" <ck.hu@mediatek.com>]

On Mon, 2025-07-07 at 09:31 +0800, shangyao lin wrote:
> From: "shangyao.lin" <shangyao.lin@mediatek.com>
> 
> Introduces the ISP pipeline driver for the MediaTek ISP raw and yuv
> modules. Key functionalities include data processing, V4L2 integration,
> resource management, debug support, and various control operations.
> Additionally, IRQ handling, platform device management, and MediaTek
> ISP DMA format support are also included.
> 
> Changes in v2:
> 
> - Removed mtk_cam-raw.c and mtk_cam-raw.h, along with related code
> - Removed M2M related code
> - Various fixes per review comments
> 
> Question for CK
> 
> Hi CK,
> 
> May I ask if it is acceptable to keep the id field for the following reason?
> When there is more than one RAW engine on the platform, having an id makes it
> much easier to manage and coordinate different RAW pipelines. It allows us to
> correctly associate events and interrupts with the corresponding RAW device
> context, which improves code maintainability and scalability.
> 
> Would you agree with keeping the id for this purpose, or do you have a preferred
> alternative?

You could keep it now. I would try to review where use this id.
If every where is not necessary, this id would finally be removed.

> 
> Explanation:
> 
> Reply to CK: "Remove resource calculation related code"
> 
> Thank you for your suggestion. The resource calculation code is retained even
> for the unprocessed IMGO path. All image streams, including IMGO unprocessed,
> pass through the raw pipeline and require proper ISP hardware configuration.

It's better to have a comment to explain the pipeline and function block in the pipeline.
When more information, I would give better advice.

> 
> Reply to CK: "It seems yuv is an independent function. If so, separate yuv
> function to an independent patch."
> 
> Thank you for your comment. The raw and yuv functions are not separated into
> independent patches because, in our hardware design, both are handled by the
> same hardware block and share the same register set. The yuv function is just a
> different mode of operation within the same unit, not an independent hardware
> module. Splitting them would not reflect the actual hardware architecture and
> could make the code harder to maintain.

I just want you to separate yuv function to another patch.
You could add it back in later patch.
Finally the code would be the same as now.
Finally it would show hardware architecture and you could maintain the same code.

> 
> Please let me know if you have any further suggestions. Thank you!
> 
> Signed-off-by: shangyao.lin <shangyao.lin@mediatek.com>
> ---

[snip]

> +	{
> +		.id = MTK_RAW_RAWI_2_IN,
> +		.name = "rawi 2",
> +		.cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE,
> +		.buf_type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,

For camera, capture device is basic function and output device is advanced function.
If this series just keep basic function, drop the output device code.

Regards,
CK

> +		.link_flags = MEDIA_LNK_FL_ENABLED |  MEDIA_LNK_FL_IMMUTABLE,
> +		.image = true,
> +		.smem_alloc = false,
> +		.dma_port = MTKCAM_ISP_RAWI_2,
> +		.fmts = stream_out_fmts,
> +		.num_fmts = ARRAY_SIZE(stream_out_fmts),
> +		.default_fmt_idx = 0,
> +		.ioctl_ops = &mtk_cam_v4l2_vout_ioctl_ops,
> +		.frmsizes = &(struct v4l2_frmsizeenum) {
> +			.index = 0,
> +			.type = V4L2_FRMSIZE_TYPE_CONTINUOUS,
> +			.stepwise = {
> +				.max_width = IMG_MAX_WIDTH,
> +				.min_width = IMG_MIN_WIDTH,
> +				.max_height = IMG_MAX_HEIGHT,
> +				.min_height = IMG_MIN_HEIGHT,
> +				.step_height = 1,
> +				.step_width = 1,
> +			},
> +		},
> +	},
> +	{
> +		.id = MTK_RAW_RAWI_3_IN,
> +		.name = "rawi 3",
> +		.cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE,
> +		.buf_type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> +		.link_flags = 0,
> +		.image = true,
> +		.smem_alloc = false,
> +		.dma_port = MTKCAM_ISP_RAWI_3,
> +		.fmts = stream_out_fmts,
> +		.num_fmts = ARRAY_SIZE(stream_out_fmts),
> +		.default_fmt_idx = 0,
> +		.ioctl_ops = &mtk_cam_v4l2_vout_ioctl_ops,
> +		.frmsizes = &(struct v4l2_frmsizeenum) {
> +			.index = 0,
> +			.type = V4L2_FRMSIZE_TYPE_CONTINUOUS,
> +			.stepwise = {
> +				.max_width = IMG_MAX_WIDTH,
> +				.min_width = IMG_MIN_WIDTH,
> +				.max_height = IMG_MAX_HEIGHT,
> +				.min_height = IMG_MIN_HEIGHT,
> +				.step_height = 1,
> +				.step_width = 1,
> +			},
> +		},
> +	},
> +	{
> +		.id = MTK_RAW_RAWI_4_IN,
> +		.name = "rawi 4",
> +		.cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE,
> +		.buf_type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> +		.link_flags = 0,
> +		.image = true,
> +		.smem_alloc = false,
> +		.dma_port = MTKCAM_ISP_RAWI_3, /* align backend RAWI_3 */
> +		.fmts = stream_out_fmts,
> +		.num_fmts = ARRAY_SIZE(stream_out_fmts),
> +		.default_fmt_idx = 0,
> +		.ioctl_ops = &mtk_cam_v4l2_vout_ioctl_ops,
> +		.frmsizes = &(struct v4l2_frmsizeenum) {
> +			.index = 0,
> +			.type = V4L2_FRMSIZE_TYPE_CONTINUOUS,
> +			.stepwise = {
> +				.max_width = IMG_MAX_WIDTH,
> +				.min_width = IMG_MIN_WIDTH,
> +				.max_height = IMG_MAX_HEIGHT,
> +				.min_height = IMG_MIN_HEIGHT,
> +				.step_height = 1,
> +				.step_width = 1,
> +			},
> +		},
> +	}
> +};
> +