mbox series

[v21,0/4] media: mediatek: support mdp3 on mt8183 platform

Message ID 20220706075454.15569-1-moudy.ho@mediatek.com (mailing list archive)
Headers show
Series media: mediatek: support mdp3 on mt8183 platform | expand

Message

Moudy Ho (何宗原) July 6, 2022, 7:54 a.m. UTC
Change since v20:
- Rebase on linux-next.
- Move the MDP3 GCE events to the corresponding node and adjust the
  relevant driver settings.

Change since v19:
- Rebase on linux-next.
- Export the function "mdp_cmdq_send" suggected by CK.
- Fix "Macro argument reuse" reported by checkpatch.pl

Change since v18:
- Rebase on linux-next.
- Adjust copyright date of MDP3 driver.
- Functions renaming as follows:
  [1] is_output_disable() => is_output_disabled()
  [2] mdp_component_init() => mdp_comp_config()
  [3] mdp_component_deinit() => mdp_comp_destroy()
  [4] mdp_comp_ctx_init() => mdp_comp_ctx_config()
  [5] mdp_sub_comps_create() => mdp_comp_sub_create()
- Document MDP3 10-bit format descriptions in "mtk-mdp3-regs.c".
- Add error control for functions mdp_comp_clocks_on and mdp_comp_clock_on.
- Moved function "mtk_mutex_put" from function
  "mdp_comp_destroy"(renamed from mdp_component_deinit) to avoid semantic ambiguity.
- For some allocated parameters, assign a value of NULL after freeing
  to avoid the possibility of repeated use.
- Removed unnecessary timestamp pass flow.
- About parameters passed by the user in function "mdp_try_fmt_mplane", add relevant checks to
  clamp them in a reasonable range to avoid the possibility of overflow

Change since v17:
- Depend on:
  [1] https://patchwork.kernel.org/project/linux-mediatek/list/?series=649104
- In response to future CMDQ api changes listed below:
  https://patchwork.kernel.org/project/linux-mediatek/patch/20220608144055.27562-1-chunkuang.hu@kernel.org/
  adjust CMDQ flush and callback flow in MDP3.

Change since v16:
- Rebased on v5.19-rc1
- Depend on:
  [1] https://patchwork.kernel.org/project/linux-mediatek/list/?series=646131
- In response to MUTEX changes, adjust API naming and parameters when
  used in function "mdp_path_subfrm_require".
- Remove unnecessary MDP3 phandle in 8183 dts.

Change since v15:
- Depend on:
  [1] https://patchwork.kernel.org/project/linux-mediatek/list/?series=640926
- Split the bindings under ./soc/mediatek into a separate patch.
- Fix data abort in "mdp_auto_release_work"
- Adjust the steps in the function "mdp_cmdq_send" to make the error handling
  more reasonable

Change since v14:
- Rebase on v5.18-rc6
- Depend on:
  [1] https://patchwork.kernel.org/project/linux-mediatek/list/?series=640926
- In response to CMDQ API change, replace the function "cmdq_pkt_flush_async"
  with the standard APIs of mbox
- Fix the description of "mediatek,gce-client-reg" property in MDP3-related
  bindings

Change since v13:
- Rebase on v5.18-rc4
- Depend on:
  [1] https://patchwork.kernel.org/project/linux-mediatek/list/?series=636041
- Remove advanced functionality about ISP settings for direct link cases.
- Remove the software designation in the mt8183 dts and
  revise corresponding bindings.

Change since v12:
- Rebase on linux-next
- Depend on:
  [1] https://patchwork.kernel.org/project/linux-mediatek/list/?series=630948
- Remove messages related to routing information in MDP3, and leave the related
  settings in MMSYS.
- Remove unnecessary phandle and redundant property in RDMA dt-binding and
  adjust the corresponding driver.
- Revise MDP3 node name in dts. 
- Removed unnecessary functions, mutex and work queue in MDP3 driver
- Fixed format mapping error for V4L2_PIX_FMT_RGB565X

Change since v11:
- Rebase on linux-next tag:next-20220316
- Depend on:
  [1] https://patchwork.kernel.org/project/linux-mediatek/list/?series=624281
- Remove redundant hardware index in data-binding suggested by Rob Herring.
- Referring to Rob Herring's suggestion to improve some descriptions in the
  RDMA dt-binding
- Move MDP3 file folder from "./drive/media/platform/mtk-mdp3" to
  "./driver/media/platform/mediatek/mdp3"
- Fixed the V4L2 and MDP color format mapping error in RGB565 which
  checked by Benjamin Gaignard

Change since v10:
- The routing table needs to be discarded, and the calculation result
  on the SCP side is used to write a suitable mux setting for
  1 input port and 2 output ports.
- Adjust dts parsing flow to remove redundant HW IDs.
- Fix memory leak caused by no free path information in function "mdp_cmdq_send".

Change since v9:
- Keep only the MDP3 driver patches and split the remaining mmsys and
  mutex patches into another mail.
- Move mutex mod settings to corresponding driver and make relevant adjustments
  for this in MDP3 driver.
- Fix compile warning reported by kernel test robot.

Change since v8:
- Rebase on v5.16-rc2.
- Refer to Angelo's suggestion, adjust the register writing format to increase
  readability and significance.
- Refer to Angelo's suggestion, adjust or reduce inappropriate debugging
  messages.
- Refer to Rob Herring's suggestion to correct the the binding file
  to make it with the specification.
- Fix compile warning reported by kernel test robot.

Change since v7:
- Rebase on v5.15-rc6.
- Revise several V4L2 M2M settings to pass v4l2-compliance test.
- Integrate those same component dt-binding documents of DRM and MDP, and
  move them under the MMSYS domain.
- Split MMSYS and MUTEX into two different files according to
  their functional properties.

Changes since v6:
- Refactor GCE event to corresponding node.
- Fix dt_binding_check fail.
- Fix compilation errors.

Changes since v5:
- Rebase on v5.14-rc6.
- Move MMSYS/Mutex settings to corresponding driver.
- Revise the software license description and copyright.
- Remove unnecessary enum. or definitions.
- Optimize platform/chip definition conditions.
- Use general printing functions instead of MDP3 private ones.
- Fix compile warning.

Changes since v4:
- Rebase on v5.13-rc1.
- Remove the CMDQ flush flow to match the CMDQ API change.
- Integrate four of MDP's direct-link subcomponents into MDP controller node
  from syscon node to avoid illegal clock usage.
- Rewrite dt-binding in a JSON compatible subset of YAML
- Fix a bit of macro argument precedence.

Changes since v3:
- Rebase on v5.9-rc1.
- modify code for review comment from Rob Herring, cancel multiple nodes using
  same register base situation.
- control IOMMU port through pm runtime get/put to DMA components' device.
- SCP(VPU) driver revision.
- stop queuing jobs(remove flush_workqueue()) after mdp_m2m_release().
- add computation of plane address with data_offset.
- fix scale ratio check issue.
- add default v4l2_format setting.

Changes since v2:
- modify code for review comment from Tomasz Figa & Alexandre Courbot
- review comment from Rob Herring will offer code revision in v4, due to
  it's related to device node modification, will need to modify code
  architecture

Changes since v1:
- modify code for CMDQ v3 API support
- EC ipi cmd migration
- fix compliance test fail item (m2m cmd with -f) due to there is two problem in
  runing all format(-f) cmd:
1. out of memory before test complete
        Due to capture buffer mmap (refcount + 1) after reqbuf but seems
        no corresponding munmap called before device close.
        There are total 12XX items(formats) in format test and each format
        alloc 8 capture/output buffers.
2. unceasingly captureBufs() (randomly)
        Seems the break statement didn't catch the count == 0 situation:
        In v4l2-test-buffers.cpp, function: captureBufs()
                        ...
                        count--;
                        if (!node->is_m2m && !count)
                                break;
        Log is as attachment

Hi,

This patch is used to present Media Data Path 3 (MDP3)
which provided scaling and color format conversion.
support using GCE to write register in critical time limitation.
support V4L2 m2m device control.

Moudy Ho (4):
  dt-binding: mediatek: add bindings for MediaTek MDP3 components
  dt-binding: mediatek: add bindings for MediaTek CCORR and WDMA
  arm64: dts: mt8183: add Mediatek MDP3 nodes
  media: platform: mtk-mdp3: add Mediatek MDP3 driver

 .../bindings/media/mediatek,mdp3-rdma.yaml    |   95 ++
 .../bindings/media/mediatek,mdp3-rsz.yaml     |   77 ++
 .../bindings/media/mediatek,mdp3-wrot.yaml    |   80 ++
 .../bindings/soc/mediatek/mediatek,ccorr.yaml |   68 ++
 .../bindings/soc/mediatek/mediatek,wdma.yaml  |   81 ++
 arch/arm64/boot/dts/mediatek/mt8183.dtsi      |   63 +
 drivers/media/platform/mediatek/Kconfig       |    1 +
 drivers/media/platform/mediatek/Makefile      |    1 +
 drivers/media/platform/mediatek/mdp3/Kconfig  |   20 +
 drivers/media/platform/mediatek/mdp3/Makefile |    6 +
 .../platform/mediatek/mdp3/mdp_reg_ccorr.h    |   19 +
 .../platform/mediatek/mdp3/mdp_reg_rdma.h     |   65 ++
 .../platform/mediatek/mdp3/mdp_reg_rsz.h      |   39 +
 .../platform/mediatek/mdp3/mdp_reg_wdma.h     |   47 +
 .../platform/mediatek/mdp3/mdp_reg_wrot.h     |   55 +
 .../platform/mediatek/mdp3/mtk-img-ipi.h      |  290 +++++
 .../platform/mediatek/mdp3/mtk-mdp3-cmdq.c    |  465 ++++++++
 .../platform/mediatek/mdp3/mtk-mdp3-cmdq.h    |   43 +
 .../platform/mediatek/mdp3/mtk-mdp3-comp.c    | 1030 +++++++++++++++++
 .../platform/mediatek/mdp3/mtk-mdp3-comp.h    |  186 +++
 .../platform/mediatek/mdp3/mtk-mdp3-core.c    |  357 ++++++
 .../platform/mediatek/mdp3/mtk-mdp3-core.h    |   93 ++
 .../platform/mediatek/mdp3/mtk-mdp3-m2m.c     |  769 ++++++++++++
 .../platform/mediatek/mdp3/mtk-mdp3-m2m.h     |   48 +
 .../platform/mediatek/mdp3/mtk-mdp3-regs.c    |  742 ++++++++++++
 .../platform/mediatek/mdp3/mtk-mdp3-regs.h    |  373 ++++++
 .../platform/mediatek/mdp3/mtk-mdp3-vpu.c     |  312 +++++
 .../platform/mediatek/mdp3/mtk-mdp3-vpu.h     |   78 ++
 28 files changed, 5503 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml
 create mode 100644 Documentation/devicetree/bindings/media/mediatek,mdp3-rsz.yaml
 create mode 100644 Documentation/devicetree/bindings/media/mediatek,mdp3-wrot.yaml
 create mode 100644 Documentation/devicetree/bindings/soc/mediatek/mediatek,ccorr.yaml
 create mode 100644 Documentation/devicetree/bindings/soc/mediatek/mediatek,wdma.yaml
 create mode 100644 drivers/media/platform/mediatek/mdp3/Kconfig
 create mode 100644 drivers/media/platform/mediatek/mdp3/Makefile
 create mode 100644 drivers/media/platform/mediatek/mdp3/mdp_reg_ccorr.h
 create mode 100644 drivers/media/platform/mediatek/mdp3/mdp_reg_rdma.h
 create mode 100644 drivers/media/platform/mediatek/mdp3/mdp_reg_rsz.h
 create mode 100644 drivers/media/platform/mediatek/mdp3/mdp_reg_wdma.h
 create mode 100644 drivers/media/platform/mediatek/mdp3/mdp_reg_wrot.h
 create mode 100644 drivers/media/platform/mediatek/mdp3/mtk-img-ipi.h
 create mode 100644 drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
 create mode 100644 drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.h
 create mode 100644 drivers/media/platform/mediatek/mdp3/mtk-mdp3-comp.c
 create mode 100644 drivers/media/platform/mediatek/mdp3/mtk-mdp3-comp.h
 create mode 100644 drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
 create mode 100644 drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
 create mode 100644 drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.c
 create mode 100644 drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.h
 create mode 100644 drivers/media/platform/mediatek/mdp3/mtk-mdp3-regs.c
 create mode 100644 drivers/media/platform/mediatek/mdp3/mtk-mdp3-regs.h
 create mode 100644 drivers/media/platform/mediatek/mdp3/mtk-mdp3-vpu.c
 create mode 100644 drivers/media/platform/mediatek/mdp3/mtk-mdp3-vpu.h

Comments

Hans Verkuil July 8, 2022, 8:08 a.m. UTC | #1
Hi Moudy,

Some comments below:

On 7/6/22 09:54, Moudy Ho wrote:
> This patch adds driver for Mediatek's Media Data Path ver.3 (MDP3).
> It provides the following functions:
>   color transform, format conversion, resize, crop, rotate, flip
>   and additional image quality enhancement.
> 
> The MDP3 driver is mainly used for Google Chromebook products to
> import the new architecture to set the HW settings as shown below:
>   User -> V4L2 framework
>     -> MDP3 driver -> SCP (setting calculations)
>       -> MDP3 driver -> CMDQ (GCE driver) -> HW
> 
> Each modules' related operation control is sited in mtk-mdp3-comp.c
> Each modules' register table is defined in file with "mdp_reg_" prefix
> GCE related API, operation control  sited in mtk-mdp3-cmdq.c
> V4L2 m2m device functions are implemented in mtk-mdp3-m2m.c
> Probe, power, suspend/resume, system level functions are defined in
> mtk-mdp3-core.c
> 
> v4l2-compliance 1.22.1, 32 bits, 32-bit time_t
> 
> Compliance test for mtk-mdp3 device /dev/video2:
> 
> Driver Info:
> 	Driver name      : mtk-mdp3
> 	Card type        : 14001000.mdp3-rdma0

Card type is expected to be a human readable name, and that's not the case
here.

> 	Bus info         : platform:mtk-mdp3

<snip>

> diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.c b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.c
> new file mode 100644
> index 000000000000..9d2b8acc303f
> --- /dev/null
> +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.c
> @@ -0,0 +1,769 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022 MediaTek Inc.
> + * Author: Ping-Hsun Wu <ping-hsun.wu@mediatek.com>
> + */
> +
> +#include <linux/platform_device.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-event.h>
> +#include <media/videobuf2-dma-contig.h>
> +#include "mtk-mdp3-m2m.h"
> +
> +static inline struct mdp_m2m_ctx *fh_to_ctx(struct v4l2_fh *fh)
> +{
> +	return container_of(fh, struct mdp_m2m_ctx, fh);
> +}
> +
> +static inline struct mdp_m2m_ctx *ctrl_to_ctx(struct v4l2_ctrl *ctrl)
> +{
> +	return container_of(ctrl->handler, struct mdp_m2m_ctx, ctrl_handler);
> +}
> +
> +static inline struct mdp_frame *ctx_get_frame(struct mdp_m2m_ctx *ctx,
> +					      enum v4l2_buf_type type)
> +{
> +	if (V4L2_TYPE_IS_OUTPUT(type))
> +		return &ctx->curr_param.output;
> +	else
> +		return &ctx->curr_param.captures[0];
> +}
> +
> +static void mdp_m2m_ctx_set_state(struct mdp_m2m_ctx *ctx, u32 state)
> +{
> +	atomic_or(state, &ctx->curr_param.state);
> +}
> +
> +static bool mdp_m2m_ctx_is_state_set(struct mdp_m2m_ctx *ctx, u32 mask)
> +{
> +	bool ret;
> +
> +	ret = ((atomic_read(&ctx->curr_param.state) & mask) == mask);

This can just do 'return' so you can drop the 'ret' variable.

> +
> +	return ret;
> +}
> +
> +static void mdp_m2m_process_done(void *priv, int vb_state)
> +{
> +	struct mdp_m2m_ctx *ctx = priv;
> +	struct vb2_v4l2_buffer *src_vbuf, *dst_vbuf;
> +
> +	src_vbuf = (struct vb2_v4l2_buffer *)
> +			v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
> +	dst_vbuf = (struct vb2_v4l2_buffer *)
> +			v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
> +	ctx->curr_param.frame_no = ctx->frame_count[MDP_M2M_SRC];
> +	src_vbuf->sequence = ctx->frame_count[MDP_M2M_SRC]++;
> +	dst_vbuf->sequence = ctx->frame_count[MDP_M2M_DST]++;
> +	v4l2_m2m_buf_copy_metadata(src_vbuf, dst_vbuf, true);
> +
> +	v4l2_m2m_buf_done(src_vbuf, vb_state);
> +	v4l2_m2m_buf_done(dst_vbuf, vb_state);
> +	v4l2_m2m_job_finish(ctx->mdp_dev->m2m_dev, ctx->m2m_ctx);
> +}
> +
> +static void mdp_m2m_device_run(void *priv)
> +{
> +	struct mdp_m2m_ctx *ctx = priv;
> +	struct mdp_frame *frame;
> +	struct vb2_v4l2_buffer *src_vb, *dst_vb;
> +	struct img_ipi_frameparam param = {0};
> +	struct mdp_cmdq_param task = {0};

Just use '= {}', it's cleaner.

> +	enum vb2_buffer_state vb_state = VB2_BUF_STATE_ERROR;
> +	int ret;
> +
> +	if (mdp_m2m_ctx_is_state_set(ctx, MDP_M2M_CTX_ERROR)) {
> +		dev_err(&ctx->mdp_dev->pdev->dev,
> +			"mdp_m2m_ctx is in error state\n");
> +		goto worker_end;
> +	}
> +
> +	param.frame_no = ctx->curr_param.frame_no;
> +	param.type = ctx->curr_param.type;
> +	param.num_inputs = 1;
> +	param.num_outputs = 1;
> +
> +	frame = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
> +	src_vb = v4l2_m2m_next_src_buf(ctx->m2m_ctx);
> +	mdp_set_src_config(&param.inputs[0], frame, &src_vb->vb2_buf);
> +
> +	frame = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> +	dst_vb = v4l2_m2m_next_dst_buf(ctx->m2m_ctx);
> +	mdp_set_dst_config(&param.outputs[0], frame, &dst_vb->vb2_buf);
> +
> +	ret = mdp_vpu_process(&ctx->vpu, &param);
> +	if (ret) {
> +		dev_err(&ctx->mdp_dev->pdev->dev,
> +			"VPU MDP process failed: %d\n", ret);
> +		goto worker_end;
> +	}
> +
> +	task.config = ctx->vpu.config;
> +	task.param = &param;
> +	task.composes[0] = &frame->compose;
> +	task.cmdq_cb = NULL;
> +	task.cb_data = NULL;
> +	task.mdp_ctx = ctx;
> +
> +	ret = mdp_cmdq_send(ctx->mdp_dev, &task);
> +	if (ret) {
> +		dev_err(&ctx->mdp_dev->pdev->dev,
> +			"CMDQ sendtask failed: %d\n", ret);
> +		goto worker_end;
> +	}
> +
> +	return;
> +
> +worker_end:
> +	mdp_m2m_process_done(ctx, vb_state);
> +}
> +
> +static int mdp_m2m_start_streaming(struct vb2_queue *q, unsigned int count)
> +{
> +	struct mdp_m2m_ctx *ctx = vb2_get_drv_priv(q);
> +
> +	ctx->frame_count[MDP_M2M_SRC] = 0;
> +	ctx->frame_count[MDP_M2M_DST] = 0;

Don't set both, check which queue it is first.

> +
> +	return 0;
> +}
> +
> +static struct vb2_v4l2_buffer *mdp_m2m_buf_remove(struct mdp_m2m_ctx *ctx,
> +						  unsigned int type)
> +{
> +	if (V4L2_TYPE_IS_OUTPUT(type))
> +		return (struct vb2_v4l2_buffer *)
> +			v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
> +	else
> +		return (struct vb2_v4l2_buffer *)
> +			v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
> +}
> +
> +static void mdp_m2m_stop_streaming(struct vb2_queue *q)
> +{
> +	struct mdp_m2m_ctx *ctx = vb2_get_drv_priv(q);
> +	struct vb2_v4l2_buffer *vb;
> +
> +	vb = mdp_m2m_buf_remove(ctx, q->type);
> +	while (vb) {
> +		v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);
> +		vb = mdp_m2m_buf_remove(ctx, q->type);
> +	}
> +}
> +
> +static int mdp_m2m_queue_setup(struct vb2_queue *q,
> +			       unsigned int *num_buffers,
> +			       unsigned int *num_planes, unsigned int sizes[],
> +			       struct device *alloc_devs[])
> +{
> +	struct mdp_m2m_ctx *ctx = vb2_get_drv_priv(q);
> +	struct v4l2_pix_format_mplane *pix_mp;
> +	struct device *dev = &ctx->mdp_dev->pdev->dev;
> +	u32 i;
> +
> +	pix_mp = &ctx_get_frame(ctx, q->type)->format.fmt.pix_mp;
> +
> +	/* from VIDIOC_CREATE_BUFS */
> +	if (*num_planes) {
> +		if (*num_planes != pix_mp->num_planes)
> +			return -EINVAL;
> +		for (i = 0; i < pix_mp->num_planes; ++i)
> +			if (sizes[i] < pix_mp->plane_fmt[i].sizeimage)
> +				return -EINVAL;
> +	} else {/* from VIDIOC_REQBUFS */
> +		*num_planes = pix_mp->num_planes;
> +		for (i = 0; i < pix_mp->num_planes; ++i)
> +			sizes[i] = pix_mp->plane_fmt[i].sizeimage;
> +	}
> +
> +	dev_info(dev, "[%d] type:%d, planes:%u, buffers:%u, size:%u,%u,%u",

This should be dropped or changed to dev_dbg. You don't want logging
when doing normal things, that will just spam the kernel log.

> +		 ctx->id, q->type, *num_planes, *num_buffers,
> +		 sizes[0], sizes[1], sizes[2]);
> +	return 0;
> +}
> +
> +static int mdp_m2m_buf_prepare(struct vb2_buffer *vb)
> +{
> +	struct mdp_m2m_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> +	struct v4l2_pix_format_mplane *pix_mp;
> +	struct vb2_v4l2_buffer *v4l2_buf = to_vb2_v4l2_buffer(vb);
> +	u32 i;
> +
> +	v4l2_buf->field = V4L2_FIELD_NONE;
> +
> +	if (!V4L2_TYPE_IS_OUTPUT(vb->type)) {
> +		pix_mp = &ctx_get_frame(ctx, vb->type)->format.fmt.pix_mp;
> +		for (i = 0; i < pix_mp->num_planes; ++i) {
> +			vb2_set_plane_payload(vb, i,
> +					      pix_mp->plane_fmt[i].sizeimage);
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int mdp_m2m_buf_out_validate(struct vb2_buffer *vb)
> +{
> +	struct vb2_v4l2_buffer *v4l2_buf = to_vb2_v4l2_buffer(vb);
> +
> +	v4l2_buf->field = V4L2_FIELD_NONE;
> +
> +	return 0;
> +}
> +
> +static void mdp_m2m_buf_queue(struct vb2_buffer *vb)
> +{
> +	struct mdp_m2m_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> +	struct vb2_v4l2_buffer *v4l2_buf = to_vb2_v4l2_buffer(vb);
> +
> +	v4l2_buf->field = V4L2_FIELD_NONE;
> +
> +	v4l2_m2m_buf_queue(ctx->m2m_ctx, to_vb2_v4l2_buffer(vb));
> +}
> +
> +static const struct vb2_ops mdp_m2m_qops = {
> +	.queue_setup	= mdp_m2m_queue_setup,
> +	.wait_prepare	= vb2_ops_wait_prepare,
> +	.wait_finish	= vb2_ops_wait_finish,
> +	.buf_prepare	= mdp_m2m_buf_prepare,
> +	.start_streaming = mdp_m2m_start_streaming,
> +	.stop_streaming	= mdp_m2m_stop_streaming,
> +	.buf_queue	= mdp_m2m_buf_queue,
> +	.buf_out_validate = mdp_m2m_buf_out_validate,
> +};
> +
> +static int mdp_m2m_querycap(struct file *file, void *fh,
> +			    struct v4l2_capability *cap)
> +{
> +	struct mdp_m2m_ctx *ctx = fh_to_ctx(fh);
> +
> +	strscpy(cap->driver, MDP_MODULE_NAME, sizeof(cap->driver));
> +	strscpy(cap->card, ctx->mdp_dev->pdev->name, sizeof(cap->card));

As mentioned at the top, this is not a suitable name for cap->card.

> +	strscpy(cap->bus_info, "platform:mtk-mdp3", sizeof(cap->bus_info));
> +
> +	return 0;
> +}
> +
> +static int mdp_m2m_enum_fmt_mplane(struct file *file, void *fh,
> +				   struct v4l2_fmtdesc *f)
> +{
> +	return mdp_enum_fmt_mplane(f);
> +}
> +
> +static int mdp_m2m_g_fmt_mplane(struct file *file, void *fh,
> +				struct v4l2_format *f)
> +{
> +	struct mdp_m2m_ctx *ctx = fh_to_ctx(fh);
> +	struct mdp_frame *frame;
> +	struct v4l2_pix_format_mplane *pix_mp;
> +	struct device *dev = &ctx->mdp_dev->pdev->dev;
> +
> +	frame = ctx_get_frame(ctx, f->type);
> +	*f = frame->format;
> +	pix_mp = &f->fmt.pix_mp;
> +	pix_mp->colorspace = ctx->curr_param.colorspace;
> +	pix_mp->xfer_func = ctx->curr_param.xfer_func;
> +	pix_mp->ycbcr_enc = ctx->curr_param.ycbcr_enc;
> +	pix_mp->quantization = ctx->curr_param.quant;
> +
> +	dev_info(dev, "[%d] type:%d, frame:%ux%u colorspace=%d", ctx->id, f->type,
> +		 f->fmt.pix_mp.width, f->fmt.pix_mp.height,
> +		 f->fmt.pix_mp.colorspace);

Drop this, you're returning this info to userspace anyway, so why spam the
kernel log?

> +	return 0;
> +}
> +
> +static int mdp_m2m_s_fmt_mplane(struct file *file, void *fh,
> +				struct v4l2_format *f)
> +{
> +	struct mdp_m2m_ctx *ctx = fh_to_ctx(fh);
> +	struct mdp_frame *frame = ctx_get_frame(ctx, f->type);
> +	struct mdp_frame *capture;
> +	const struct mdp_format *fmt;
> +	struct vb2_queue *vq;
> +	struct device *dev = &ctx->mdp_dev->pdev->dev;
> +
> +	dev_info(dev, "[%d] type:%d", ctx->id, f->type);

dev_dbg. If dev_info is used elsewhere in similar situations, then replace that
with dev_dbg as well. Regular usage of this driver must not produce kernel logging.

> +
> +	fmt = mdp_try_fmt_mplane(f, &ctx->curr_param, ctx->id);
> +	if (!fmt) {
> +		dev_info(dev, "[%d] try_fmt failed, type:%d", ctx->id, f->type);

Drop this, the error code says enough.

> +		return -EINVAL;
> +	}
> +
> +	vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type);
> +	if (vb2_is_streaming(vq)) {

This must be vb2_is_busy(): changing formats is prohibited once buffers are
allocated (vb2_is_busy() is true in that case).

> +		dev_info(&ctx->mdp_dev->pdev->dev, "Queue %d busy\n", f->type);

Drop this, the error code says enough.

> +		return -EBUSY;
> +	}
> +
> +	frame->format = *f;
> +	frame->mdp_fmt = fmt;
> +	frame->ycbcr_prof = mdp_map_ycbcr_prof_mplane(f, fmt->mdp_color);
> +	frame->usage = V4L2_TYPE_IS_OUTPUT(f->type) ?
> +		MDP_BUFFER_USAGE_HW_READ : MDP_BUFFER_USAGE_MDP;
> +
> +	capture = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> +	if (V4L2_TYPE_IS_OUTPUT(f->type)) {
> +		capture->crop.c.left = 0;
> +		capture->crop.c.top = 0;
> +		capture->crop.c.width = f->fmt.pix_mp.width;
> +		capture->crop.c.height = f->fmt.pix_mp.height;
> +		ctx->curr_param.colorspace = f->fmt.pix_mp.colorspace;
> +		ctx->curr_param.ycbcr_enc = f->fmt.pix_mp.ycbcr_enc;
> +		ctx->curr_param.quant = f->fmt.pix_mp.quantization;
> +		ctx->curr_param.xfer_func = f->fmt.pix_mp.xfer_func;
> +	} else {
> +		capture->compose.left = 0;
> +		capture->compose.top = 0;
> +		capture->compose.width = f->fmt.pix_mp.width;
> +		capture->compose.height = f->fmt.pix_mp.height;
> +	}
> +
> +	ctx->frame_count[MDP_M2M_SRC] = 0;
> +	ctx->frame_count[MDP_M2M_DST] = 0;

Changing the format doesn't mean you need to zero this. Just drop these two
line.

> +
> +	dev_info(dev, "[%d] type:%d, frame:%ux%u", ctx->id, f->type,
> +		 f->fmt.pix_mp.width, f->fmt.pix_mp.height);

Drop this.

> +	return 0;
> +}
> +
> +static int mdp_m2m_try_fmt_mplane(struct file *file, void *fh,
> +				  struct v4l2_format *f)
> +{
> +	struct mdp_m2m_ctx *ctx = fh_to_ctx(fh);
> +
> +	if (!mdp_try_fmt_mplane(f, &ctx->curr_param, ctx->id))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int mdp_m2m_streamon(struct file *file, void *fh,
> +			    enum v4l2_buf_type type)
> +{
> +	struct mdp_m2m_ctx *ctx = fh_to_ctx(fh);
> +	struct mdp_frame *capture;
> +	int ret;
> +	bool out_streaming, cap_streaming;
> +
> +	capture = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> +	out_streaming = ctx->m2m_ctx->out_q_ctx.q.streaming;
> +	cap_streaming = ctx->m2m_ctx->cap_q_ctx.q.streaming;

Use vb2_is_streaming() rather than accessing q.streaming directly.

> +
> +	/* Check to see if scaling ratio is within supported range */
> +	if ((V4L2_TYPE_IS_OUTPUT(type) && cap_streaming) ||
> +	    (!V4L2_TYPE_IS_OUTPUT(type) && out_streaming)) {
> +		ret = mdp_check_scaling_ratio(&capture->crop.c,
> +					      &capture->compose,
> +					      capture->rotation,
> +					      ctx->curr_param.limit);
> +		if (ret) {
> +			dev_info(&ctx->mdp_dev->pdev->dev,
> +				 "Out of scaling range\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (!mdp_m2m_ctx_is_state_set(ctx, MDP_VPU_INIT)) {
> +		ret = mdp_vpu_get_locked(ctx->mdp_dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = mdp_vpu_ctx_init(&ctx->vpu, &ctx->mdp_dev->vpu,
> +				       MDP_DEV_M2M);
> +		if (ret) {
> +			dev_err(&ctx->mdp_dev->pdev->dev,
> +				"VPU init failed %d\n", ret);
> +			return -EINVAL;
> +		}
> +		mdp_m2m_ctx_set_state(ctx, MDP_VPU_INIT);
> +	}
> +
> +	return v4l2_m2m_streamon(file, ctx->m2m_ctx, type);
> +}

There really is no reason to override streamon. You can also do this in
start_streaming(). I recommend moving these tests there. Internally
streamon() will call start_streaming(), so any error you return in that
callback function will be the error return of the streamon as well.

> +
> +static int mdp_m2m_g_selection(struct file *file, void *fh,
> +			       struct v4l2_selection *s)
> +{
> +	struct mdp_m2m_ctx *ctx = fh_to_ctx(fh);
> +	struct mdp_frame *frame;
> +	struct device *dev = &ctx->mdp_dev->pdev->dev;
> +	bool valid = false;
> +
> +	if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> +		valid = mdp_target_is_crop(s->target);
> +	else if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		valid = mdp_target_is_compose(s->target);
> +
> +	if (!valid) {
> +		dev_err(dev, "[%s:%d] invalid type:%u target:%u", __func__, ctx->id,
> +			s->type, s->target);

Drop this. User errors should never cause spamming in the kernel log.

Please check your code: you can use dev_dbg to help debugging, but dev_info
should be used very sparingly (typically only in probe() and remove()), and
dev_warn/err only for hardware/driver warnings/errors, and never due to
userspace inputs.

> +		return -EINVAL;
> +	}
> +
> +	switch (s->target) {
> +	case V4L2_SEL_TGT_CROP:
> +		if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> +			frame = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> +			s->r = frame->crop.c;
> +			return 0;
> +		}

It's easier to read if you swap the tests:

		if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
			return -EINVAL;
		frame = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
		s->r = frame->crop.c;
		return 0;

Same below.

> +		break;
> +	case V4L2_SEL_TGT_COMPOSE:
> +		if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> +			frame = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> +			s->r = frame->compose;
> +			return 0;
> +		}
> +		break;
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +		if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> +			frame = ctx_get_frame(ctx, s->type);
> +			s->r.left = 0;
> +			s->r.top = 0;
> +			s->r.width = frame->format.fmt.pix_mp.width;
> +			s->r.height = frame->format.fmt.pix_mp.height;
> +			return 0;
> +		}
> +		break;
> +	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> +		if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> +			frame = ctx_get_frame(ctx, s->type);
> +			s->r.left = 0;
> +			s->r.top = 0;
> +			s->r.width = frame->format.fmt.pix_mp.width;
> +			s->r.height = frame->format.fmt.pix_mp.height;
> +			return 0;
> +		}
> +		break;
> +	}
> +	return -EINVAL;
> +}
> +
> +static int mdp_m2m_s_selection(struct file *file, void *fh,
> +			       struct v4l2_selection *s)
> +{
> +	struct mdp_m2m_ctx *ctx = fh_to_ctx(fh);
> +	struct mdp_frame *frame = ctx_get_frame(ctx, s->type);
> +	struct mdp_frame *capture;
> +	struct v4l2_rect r;
> +	struct device *dev = &ctx->mdp_dev->pdev->dev;
> +	bool valid = false;
> +	int ret;
> +
> +	if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> +		valid = (s->target == V4L2_SEL_TGT_CROP);
> +	else if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		valid = (s->target == V4L2_SEL_TGT_COMPOSE);
> +
> +	if (!valid) {
> +		dev_err(dev, "[%s:%d] invalid type:%u target:%u", __func__, ctx->id,
> +			s->type, s->target);
> +		return -EINVAL;
> +	}
> +
> +	ret = mdp_try_crop(ctx, &r, s, frame);
> +	if (ret)
> +		return ret;
> +	capture = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> +
> +	if (mdp_target_is_crop(s->target))
> +		capture->crop.c = r;
> +	else
> +		capture->compose = r;
> +
> +	s->r = r;
> +	memset(s->reserved, 0, sizeof(s->reserved));

No need for this memset, the v4l2 core will clear this for you.

> +
> +	ctx->frame_count[MDP_M2M_SRC] = 0;
> +	ctx->frame_count[MDP_M2M_DST] = 0;

Drop this!

> +	return 0;
> +}
> +
> +static const struct v4l2_ioctl_ops mdp_m2m_ioctl_ops = {
> +	.vidioc_querycap		= mdp_m2m_querycap,
> +	.vidioc_enum_fmt_vid_cap	= mdp_m2m_enum_fmt_mplane,
> +	.vidioc_enum_fmt_vid_out	= mdp_m2m_enum_fmt_mplane,
> +	.vidioc_g_fmt_vid_cap_mplane	= mdp_m2m_g_fmt_mplane,
> +	.vidioc_g_fmt_vid_out_mplane	= mdp_m2m_g_fmt_mplane,
> +	.vidioc_s_fmt_vid_cap_mplane	= mdp_m2m_s_fmt_mplane,
> +	.vidioc_s_fmt_vid_out_mplane	= mdp_m2m_s_fmt_mplane,
> +	.vidioc_try_fmt_vid_cap_mplane	= mdp_m2m_try_fmt_mplane,
> +	.vidioc_try_fmt_vid_out_mplane	= mdp_m2m_try_fmt_mplane,
> +	.vidioc_reqbufs			= v4l2_m2m_ioctl_reqbufs,
> +	.vidioc_querybuf		= v4l2_m2m_ioctl_querybuf,
> +	.vidioc_qbuf			= v4l2_m2m_ioctl_qbuf,
> +	.vidioc_expbuf			= v4l2_m2m_ioctl_expbuf,
> +	.vidioc_dqbuf			= v4l2_m2m_ioctl_dqbuf,
> +	.vidioc_create_bufs		= v4l2_m2m_ioctl_create_bufs,
> +	.vidioc_streamon		= mdp_m2m_streamon,
> +	.vidioc_streamoff		= v4l2_m2m_ioctl_streamoff,
> +	.vidioc_g_selection		= mdp_m2m_g_selection,
> +	.vidioc_s_selection		= mdp_m2m_s_selection,
> +	.vidioc_subscribe_event		= v4l2_ctrl_subscribe_event,
> +	.vidioc_unsubscribe_event	= v4l2_event_unsubscribe,
> +};
> +
> +static int mdp_m2m_queue_init(void *priv,
> +			      struct vb2_queue *src_vq,
> +			      struct vb2_queue *dst_vq)
> +{
> +	struct mdp_m2m_ctx *ctx = priv;
> +	int ret;
> +
> +	src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> +	src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> +	src_vq->ops = &mdp_m2m_qops;
> +	src_vq->mem_ops = &vb2_dma_contig_memops;
> +	src_vq->drv_priv = ctx;
> +	src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> +	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> +	src_vq->dev = &ctx->mdp_dev->pdev->dev;
> +	src_vq->lock = &ctx->ctx_lock;
> +
> +	ret = vb2_queue_init(src_vq);
> +	if (ret)
> +		return ret;
> +
> +	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> +	dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> +	dst_vq->ops = &mdp_m2m_qops;
> +	dst_vq->mem_ops = &vb2_dma_contig_memops;
> +	dst_vq->drv_priv = ctx;
> +	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> +	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> +	dst_vq->dev = &ctx->mdp_dev->pdev->dev;
> +	dst_vq->lock = &ctx->ctx_lock;
> +
> +	return vb2_queue_init(dst_vq);
> +}
> +
> +static int mdp_m2m_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct mdp_m2m_ctx *ctx = ctrl_to_ctx(ctrl);
> +	struct mdp_frame *capture;
> +
> +	if (ctrl->flags & V4L2_CTRL_FLAG_INACTIVE)
> +		return 0;

Why this test? As far as I can tell this flag is never set, so there
is no need to check against it.

> +
> +	capture = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> +	switch (ctrl->id) {
> +	case V4L2_CID_HFLIP:
> +		capture->hflip = ctrl->val;
> +		break;
> +	case V4L2_CID_VFLIP:
> +		capture->vflip = ctrl->val;
> +		break;
> +	case V4L2_CID_ROTATE:
> +		capture->rotation = ctrl->val;
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_ctrl_ops mdp_m2m_ctrl_ops = {
> +	.s_ctrl	= mdp_m2m_s_ctrl,
> +};
> +
> +static int mdp_m2m_ctrls_create(struct mdp_m2m_ctx *ctx)
> +{
> +	v4l2_ctrl_handler_init(&ctx->ctrl_handler, MDP_MAX_CTRLS);
> +	ctx->ctrls.hflip = v4l2_ctrl_new_std(&ctx->ctrl_handler,
> +					     &mdp_m2m_ctrl_ops, V4L2_CID_HFLIP,
> +					     0, 1, 1, 0);
> +	ctx->ctrls.vflip = v4l2_ctrl_new_std(&ctx->ctrl_handler,
> +					     &mdp_m2m_ctrl_ops, V4L2_CID_VFLIP,
> +					     0, 1, 1, 0);
> +	ctx->ctrls.rotate = v4l2_ctrl_new_std(&ctx->ctrl_handler,
> +					      &mdp_m2m_ctrl_ops,
> +					      V4L2_CID_ROTATE, 0, 270, 90, 0);
> +
> +	if (ctx->ctrl_handler.error) {
> +		int err = ctx->ctrl_handler.error;
> +
> +		v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> +		dev_err(&ctx->mdp_dev->pdev->dev,
> +			"Failed to register controls\n");
> +		return err;
> +	}
> +	return 0;
> +}
> +
> +static int mdp_m2m_open(struct file *file)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +	struct mdp_dev *mdp = video_get_drvdata(vdev);
> +	struct mdp_m2m_ctx *ctx;
> +	struct device *dev = &mdp->pdev->dev;
> +	int ret;
> +	struct v4l2_format default_format;

Just add '= {}' to avoid the memset later on.

> +
> +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	if (mutex_lock_interruptible(&mdp->m2m_lock)) {
> +		ret = -ERESTARTSYS;
> +		goto err_free_ctx;
> +	}
> +
> +	ctx->id = ida_alloc(&mdp->mdp_ida, GFP_KERNEL);
> +	ctx->mdp_dev = mdp;
> +
> +	v4l2_fh_init(&ctx->fh, vdev);
> +	vdev->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING;

This doesn't belong in open(). It is already done when the video device is registered.

> +	file->private_data = &ctx->fh;
> +	ret = mdp_m2m_ctrls_create(ctx);
> +	if (ret)
> +		goto err_exit_fh;
> +
> +	/* Use separate control handler per file handle */
> +	ctx->fh.ctrl_handler = &ctx->ctrl_handler;
> +	v4l2_fh_add(&ctx->fh);
> +
> +	mutex_init(&ctx->ctx_lock);
> +	ctx->m2m_ctx = v4l2_m2m_ctx_init(mdp->m2m_dev, ctx, mdp_m2m_queue_init);
> +	if (IS_ERR(ctx->m2m_ctx)) {
> +		dev_err(dev, "Failed to initialize m2m context\n");
> +		ret = PTR_ERR(ctx->m2m_ctx);
> +		goto err_release_handler;
> +	}
> +	ctx->fh.m2m_ctx = ctx->m2m_ctx;
> +
> +	ctx->curr_param.ctx = ctx;
> +	ret = mdp_frameparam_init(&ctx->curr_param);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize mdp parameter\n");
> +		goto err_release_m2m_ctx;
> +	}
> +
> +	mutex_unlock(&mdp->m2m_lock);
> +
> +	/* Default format */
> +	memset(&default_format, 0, sizeof(default_format));

This can be dropped if default_format is inited with = {}.

> +	default_format.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> +	default_format.fmt.pix_mp.width = 32;
> +	default_format.fmt.pix_mp.height = 32;
> +	default_format.fmt.pix_mp.pixelformat = V4L2_PIX_FMT_YUV420M;
> +	mdp_m2m_s_fmt_mplane(file, &ctx->fh, &default_format);
> +	default_format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> +	mdp_m2m_s_fmt_mplane(file, &ctx->fh, &default_format);
> +
> +	dev_dbg(dev, "%s:[%d]", __func__, ctx->id);
> +
> +	return 0;
> +
> +err_release_m2m_ctx:
> +	v4l2_m2m_ctx_release(ctx->m2m_ctx);
> +err_release_handler:
> +	v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> +	v4l2_fh_del(&ctx->fh);
> +err_exit_fh:
> +	v4l2_fh_exit(&ctx->fh);
> +	mutex_unlock(&mdp->m2m_lock);
> +err_free_ctx:
> +	kfree(ctx);
> +
> +	return ret;
> +}
> +
> +static int mdp_m2m_release(struct file *file)
> +{
> +	struct mdp_m2m_ctx *ctx = fh_to_ctx(file->private_data);
> +	struct mdp_dev *mdp = video_drvdata(file);
> +	struct device *dev = &mdp->pdev->dev;
> +
> +	mutex_lock(&mdp->m2m_lock);
> +	v4l2_m2m_ctx_release(ctx->m2m_ctx);
> +	if (mdp_m2m_ctx_is_state_set(ctx, MDP_VPU_INIT)) {
> +		mdp_vpu_ctx_deinit(&ctx->vpu);
> +		mdp_vpu_put_locked(mdp);
> +	}
> +
> +	v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> +	v4l2_fh_del(&ctx->fh);
> +	v4l2_fh_exit(&ctx->fh);
> +	ida_free(&mdp->mdp_ida, ctx->id);
> +	mutex_unlock(&mdp->m2m_lock);
> +
> +	dev_dbg(dev, "%s:[%d]", __func__, ctx->id);
> +	kfree(ctx);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_file_operations mdp_m2m_fops = {
> +	.owner		= THIS_MODULE,
> +	.poll		= v4l2_m2m_fop_poll,
> +	.unlocked_ioctl	= video_ioctl2,
> +	.mmap		= v4l2_m2m_fop_mmap,
> +	.open		= mdp_m2m_open,
> +	.release	= mdp_m2m_release,
> +};
> +
> +static const struct v4l2_m2m_ops mdp_m2m_ops = {
> +	.device_run	= mdp_m2m_device_run,
> +};
> +
> +int mdp_m2m_device_register(struct mdp_dev *mdp)
> +{
> +	struct device *dev = &mdp->pdev->dev;
> +	int ret = 0;
> +
> +	mdp->m2m_vdev = video_device_alloc();
> +	if (!mdp->m2m_vdev) {
> +		dev_err(dev, "Failed to allocate video device\n");
> +		ret = -ENOMEM;
> +		goto err_video_alloc;
> +	}
> +	mdp->m2m_vdev->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE |
> +		V4L2_CAP_STREAMING;
> +	mdp->m2m_vdev->fops = &mdp_m2m_fops;
> +	mdp->m2m_vdev->ioctl_ops = &mdp_m2m_ioctl_ops;
> +	mdp->m2m_vdev->release = mdp_video_device_release;
> +	mdp->m2m_vdev->lock = &mdp->m2m_lock;
> +	mdp->m2m_vdev->vfl_dir = VFL_DIR_M2M;
> +	mdp->m2m_vdev->v4l2_dev = &mdp->v4l2_dev;
> +	snprintf(mdp->m2m_vdev->name, sizeof(mdp->m2m_vdev->name), "%s:m2m",
> +		 MDP_MODULE_NAME);
> +	video_set_drvdata(mdp->m2m_vdev, mdp);
> +
> +	mdp->m2m_dev = v4l2_m2m_init(&mdp_m2m_ops);
> +	if (IS_ERR(mdp->m2m_dev)) {
> +		dev_err(dev, "Failed to initialize v4l2-m2m device\n");
> +		ret = PTR_ERR(mdp->m2m_dev);
> +		goto err_m2m_init;
> +	}
> +
> +	ret = video_register_device(mdp->m2m_vdev, VFL_TYPE_VIDEO, 2);

Why 2? Just use -1 to pick the first free device number.

> +	if (ret) {
> +		dev_err(dev, "Failed to register video device\n");
> +		goto err_video_register;
> +	}
> +
> +	v4l2_info(&mdp->v4l2_dev, "Driver registered as /dev/video%d",
> +		  mdp->m2m_vdev->num);
> +	return 0;
> +
> +err_video_register:
> +	v4l2_m2m_release(mdp->m2m_dev);
> +err_m2m_init:
> +	video_device_release(mdp->m2m_vdev);
> +err_video_alloc:
> +
> +	return ret;
> +}
> +
> +void mdp_m2m_device_unregister(struct mdp_dev *mdp)
> +{
> +	video_unregister_device(mdp->m2m_vdev);
> +}
> +
> +void mdp_m2m_job_finish(struct mdp_m2m_ctx *ctx)
> +{
> +	enum vb2_buffer_state vb_state = VB2_BUF_STATE_DONE;
> +
> +	mdp_m2m_process_done(ctx, vb_state);
> +}
> diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.h b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.h
> new file mode 100644
> index 000000000000..61ddbaf1bf13
> --- /dev/null
> +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2022 MediaTek Inc.
> + * Author: Ping-Hsun Wu <ping-hsun.wu@mediatek.com>
> + */
> +
> +#ifndef __MTK_MDP3_M2M_H__
> +#define __MTK_MDP3_M2M_H__
> +
> +#include <media/v4l2-ctrls.h>
> +#include "mtk-mdp3-core.h"
> +#include "mtk-mdp3-vpu.h"
> +#include "mtk-mdp3-regs.h"
> +
> +#define MDP_MAX_CTRLS	10
> +
> +enum {
> +	MDP_M2M_SRC = 0,
> +	MDP_M2M_DST = 1,
> +	MDP_M2M_MAX,
> +};
> +
> +struct mdp_m2m_ctrls {
> +	struct v4l2_ctrl	*hflip;
> +	struct v4l2_ctrl	*vflip;
> +	struct v4l2_ctrl	*rotate;
> +};
> +
> +struct mdp_m2m_ctx {
> +	u32				id;
> +	struct mdp_dev			*mdp_dev;
> +	struct v4l2_fh			fh;
> +	struct v4l2_ctrl_handler	ctrl_handler;
> +	struct mdp_m2m_ctrls		ctrls;
> +	struct v4l2_m2m_ctx		*m2m_ctx;
> +	struct mdp_vpu_ctx		vpu;
> +	u32				frame_count[MDP_M2M_MAX];
> +
> +	struct mdp_frameparam		curr_param;
> +	/* synchronization protect for mdp m2m context */
> +	struct mutex			ctx_lock;
> +};
> +
> +int mdp_m2m_device_register(struct mdp_dev *mdp);
> +void mdp_m2m_device_unregister(struct mdp_dev *mdp);
> +void mdp_m2m_job_finish(struct mdp_m2m_ctx *ctx);
> +
> +#endif  /* __MTK_MDP3_M2M_H__ */
> diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-regs.c b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-regs.c
> new file mode 100644
> index 000000000000..18874eb7851c
> --- /dev/null
> +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-regs.c
> @@ -0,0 +1,742 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022 MediaTek Inc.
> + * Author: Ping-Hsun Wu <ping-hsun.wu@mediatek.com>
> + */
> +
> +#include <media/v4l2-common.h>
> +#include <media/videobuf2-v4l2.h>
> +#include <media/videobuf2-dma-contig.h>
> +#include "mtk-mdp3-core.h"
> +#include "mtk-mdp3-regs.h"
> +#include "mtk-mdp3-m2m.h"
> +
> +/*
> + * All 10-bit related formats are not added in the basic format list,
> + * please add the corresponding format settings before use.
> + */
> +static const struct mdp_format mdp_formats[] = {
> +	{
> +		.pixelformat	= V4L2_PIX_FMT_GREY,
> +		.mdp_color	= MDP_COLOR_GREY,
> +		.depth		= { 8 },
> +		.row_depth	= { 8 },
> +		.num_planes	= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_RGB565X,
> +		.mdp_color	= MDP_COLOR_BGR565,
> +		.depth		= { 16 },
> +		.row_depth	= { 16 },
> +		.num_planes	= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_RGB565,
> +		.mdp_color	= MDP_COLOR_RGB565,
> +		.depth		= { 16 },
> +		.row_depth	= { 16 },
> +		.num_planes	= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_RGB24,
> +		.mdp_color	= MDP_COLOR_RGB888,
> +		.depth		= { 24 },
> +		.row_depth	= { 24 },
> +		.num_planes	= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_BGR24,
> +		.mdp_color	= MDP_COLOR_BGR888,
> +		.depth		= { 24 },
> +		.row_depth	= { 24 },
> +		.num_planes	= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_ABGR32,
> +		.mdp_color	= MDP_COLOR_BGRA8888,
> +		.depth		= { 32 },
> +		.row_depth	= { 32 },
> +		.num_planes	= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_ARGB32,
> +		.mdp_color	= MDP_COLOR_ARGB8888,
> +		.depth		= { 32 },
> +		.row_depth	= { 32 },
> +		.num_planes	= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_UYVY,
> +		.mdp_color	= MDP_COLOR_UYVY,
> +		.depth		= { 16 },
> +		.row_depth	= { 16 },
> +		.num_planes	= 1,
> +		.walign		= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_VYUY,
> +		.mdp_color	= MDP_COLOR_VYUY,
> +		.depth		= { 16 },
> +		.row_depth	= { 16 },
> +		.num_planes	= 1,
> +		.walign		= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_YUYV,
> +		.mdp_color	= MDP_COLOR_YUYV,
> +		.depth		= { 16 },
> +		.row_depth	= { 16 },
> +		.num_planes	= 1,
> +		.walign		= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_YVYU,
> +		.mdp_color	= MDP_COLOR_YVYU,
> +		.depth		= { 16 },
> +		.row_depth	= { 16 },
> +		.num_planes	= 1,
> +		.walign		= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_YUV420,
> +		.mdp_color	= MDP_COLOR_I420,
> +		.depth		= { 12 },
> +		.row_depth	= { 8 },
> +		.num_planes	= 1,
> +		.walign		= 1,
> +		.halign		= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_YVU420,
> +		.mdp_color	= MDP_COLOR_YV12,
> +		.depth		= { 12 },
> +		.row_depth	= { 8 },
> +		.num_planes	= 1,
> +		.walign		= 1,
> +		.halign		= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_NV12,
> +		.mdp_color	= MDP_COLOR_NV12,
> +		.depth		= { 12 },
> +		.row_depth	= { 8 },
> +		.num_planes	= 1,
> +		.walign		= 1,
> +		.halign		= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_NV21,
> +		.mdp_color	= MDP_COLOR_NV21,
> +		.depth		= { 12 },
> +		.row_depth	= { 8 },
> +		.num_planes	= 1,
> +		.walign		= 1,
> +		.halign		= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_NV16,
> +		.mdp_color	= MDP_COLOR_NV16,
> +		.depth		= { 16 },
> +		.row_depth	= { 8 },
> +		.num_planes	= 1,
> +		.walign		= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_NV61,
> +		.mdp_color	= MDP_COLOR_NV61,
> +		.depth		= { 16 },
> +		.row_depth	= { 8 },
> +		.num_planes	= 1,
> +		.walign		= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_NV24,
> +		.mdp_color	= MDP_COLOR_NV24,
> +		.depth		= { 24 },
> +		.row_depth	= { 8 },
> +		.num_planes	= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_NV42,
> +		.mdp_color	= MDP_COLOR_NV42,
> +		.depth		= { 24 },
> +		.row_depth	= { 8 },
> +		.num_planes	= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_MT21C,
> +		.mdp_color	= MDP_COLOR_420_BLK_UFO,
> +		.depth		= { 8, 4 },
> +		.row_depth	= { 8, 8 },
> +		.num_planes	= 2,
> +		.walign		= 4,
> +		.halign		= 5,
> +		.flags		= MDP_FMT_FLAG_OUTPUT,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_MM21,
> +		.mdp_color	= MDP_COLOR_420_BLK,
> +		.depth		= { 8, 4 },
> +		.row_depth	= { 8, 8 },
> +		.num_planes	= 2,
> +		.walign		= 4,
> +		.halign		= 5,
> +		.flags		= MDP_FMT_FLAG_OUTPUT,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_NV12M,
> +		.mdp_color	= MDP_COLOR_NV12,
> +		.depth		= { 8, 4 },
> +		.row_depth	= { 8, 8 },
> +		.num_planes	= 2,
> +		.walign		= 1,
> +		.halign		= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_NV21M,
> +		.mdp_color	= MDP_COLOR_NV21,
> +		.depth		= { 8, 4 },
> +		.row_depth	= { 8, 8 },
> +		.num_planes	= 2,
> +		.walign		= 1,
> +		.halign		= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_NV16M,
> +		.mdp_color	= MDP_COLOR_NV16,
> +		.depth		= { 8, 8 },
> +		.row_depth	= { 8, 8 },
> +		.num_planes	= 2,
> +		.walign		= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_NV61M,
> +		.mdp_color	= MDP_COLOR_NV61,
> +		.depth		= { 8, 8 },
> +		.row_depth	= { 8, 8 },
> +		.num_planes	= 2,
> +		.walign		= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_YUV420M,
> +		.mdp_color	= MDP_COLOR_I420,
> +		.depth		= { 8, 2, 2 },
> +		.row_depth	= { 8, 4, 4 },
> +		.num_planes	= 3,
> +		.walign		= 1,
> +		.halign		= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}, {
> +		.pixelformat	= V4L2_PIX_FMT_YVU420M,
> +		.mdp_color	= MDP_COLOR_YV12,
> +		.depth		= { 8, 2, 2 },
> +		.row_depth	= { 8, 4, 4 },
> +		.num_planes	= 3,
> +		.walign		= 1,
> +		.halign		= 1,
> +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> +	}
> +};
> +
> +static const struct mdp_limit mdp_def_limit = {
> +	.out_limit = {
> +		.wmin	= 16,
> +		.hmin	= 16,
> +		.wmax	= 8176,
> +		.hmax	= 8176,
> +	},
> +	.cap_limit = {
> +		.wmin	= 2,
> +		.hmin	= 2,
> +		.wmax	= 8176,
> +		.hmax	= 8176,
> +	},
> +	.h_scale_up_max = 32,
> +	.v_scale_up_max = 32,
> +	.h_scale_down_max = 20,
> +	.v_scale_down_max = 128,
> +};
> +
> +static const struct mdp_format *mdp_find_fmt(u32 pixelformat, u32 type)
> +{
> +	u32 i, flag;
> +
> +	flag = V4L2_TYPE_IS_OUTPUT(type) ? MDP_FMT_FLAG_OUTPUT :
> +					MDP_FMT_FLAG_CAPTURE;
> +	for (i = 0; i < ARRAY_SIZE(mdp_formats); ++i) {
> +		if (!(mdp_formats[i].flags & flag))
> +			continue;
> +		if (mdp_formats[i].pixelformat == pixelformat)
> +			return &mdp_formats[i];
> +	}
> +	return NULL;
> +}
> +
> +static const struct mdp_format *mdp_find_fmt_by_index(u32 index, u32 type)
> +{
> +	u32 i, flag, num = 0;
> +
> +	flag = V4L2_TYPE_IS_OUTPUT(type) ? MDP_FMT_FLAG_OUTPUT :
> +					MDP_FMT_FLAG_CAPTURE;
> +	for (i = 0; i < ARRAY_SIZE(mdp_formats); ++i) {
> +		if (!(mdp_formats[i].flags & flag))
> +			continue;
> +		if (index == num)
> +			return &mdp_formats[i];
> +		num++;
> +	}
> +	return NULL;
> +}
> +
> +enum mdp_ycbcr_profile mdp_map_ycbcr_prof_mplane(struct v4l2_format *f,
> +						 u32 mdp_color)
> +{
> +	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> +
> +	if (MDP_COLOR_IS_RGB(mdp_color))
> +		return MDP_YCBCR_PROFILE_FULL_BT601;
> +
> +	switch (pix_mp->colorspace) {
> +	case V4L2_COLORSPACE_JPEG:
> +		return MDP_YCBCR_PROFILE_JPEG;
> +	case V4L2_COLORSPACE_REC709:
> +	case V4L2_COLORSPACE_DCI_P3:
> +		if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> +			return MDP_YCBCR_PROFILE_FULL_BT709;
> +		return MDP_YCBCR_PROFILE_BT709;
> +	case V4L2_COLORSPACE_BT2020:
> +		if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> +			return MDP_YCBCR_PROFILE_FULL_BT2020;
> +		return MDP_YCBCR_PROFILE_BT2020;
> +	default:
> +		if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> +			return MDP_YCBCR_PROFILE_FULL_BT601;
> +		return MDP_YCBCR_PROFILE_BT601;
> +	}
> +}
> +
> +static void mdp_bound_align_image(u32 *w, u32 *h,
> +				  struct v4l2_frmsize_stepwise *s,
> +				  unsigned int salign)
> +{
> +	unsigned int org_w, org_h;
> +
> +	org_w = *w;
> +	org_h = *h;
> +	v4l_bound_align_image(w, s->min_width, s->max_width, s->step_width,
> +			      h, s->min_height, s->max_height, s->step_height,
> +			      salign);
> +
> +	s->min_width = org_w;
> +	s->min_height = org_h;
> +	v4l2_apply_frmsize_constraints(w, h, s);
> +}
> +
> +static int mdp_clamp_align(s32 *x, int min, int max, unsigned int align)
> +{
> +	unsigned int mask;
> +
> +	if (min < 0 || max < 0)
> +		return -ERANGE;
> +
> +	/* Bits that must be zero to be aligned */
> +	mask = ~((1 << align) - 1);
> +
> +	min = 0 ? 0 : ((min + ~mask) & mask);
> +	max = max & mask;
> +	if ((unsigned int)min > (unsigned int)max)
> +		return -ERANGE;
> +
> +	/* Clamp to aligned min and max */
> +	*x = clamp(*x, min, max);
> +
> +	/* Round to nearest aligned value */
> +	if (align)
> +		*x = (*x + (1 << (align - 1))) & mask;
> +	return 0;
> +}
> +
> +int mdp_enum_fmt_mplane(struct v4l2_fmtdesc *f)
> +{
> +	const struct mdp_format *fmt;
> +
> +	fmt = mdp_find_fmt_by_index(f->index, f->type);
> +	if (!fmt)
> +		return -EINVAL;
> +
> +	f->pixelformat = fmt->pixelformat;
> +	return 0;
> +}
> +
> +const struct mdp_format *mdp_try_fmt_mplane(struct v4l2_format *f,
> +					    struct mdp_frameparam *param,
> +					    u32 ctx_id)
> +{
> +	struct device *dev = &param->ctx->mdp_dev->pdev->dev;
> +	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> +	const struct mdp_format *fmt;
> +	const struct mdp_pix_limit *pix_limit;
> +	struct v4l2_frmsize_stepwise s;
> +	u32 org_w, org_h;
> +	unsigned int i;
> +
> +	fmt = mdp_find_fmt(pix_mp->pixelformat, f->type);
> +	if (!fmt) {
> +		fmt = mdp_find_fmt_by_index(0, f->type);
> +		if (!fmt) {
> +			dev_dbg(dev, "%d: pixelformat %c%c%c%c invalid", ctx_id,
> +				(pix_mp->pixelformat & 0xff),
> +				(pix_mp->pixelformat >>  8) & 0xff,
> +				(pix_mp->pixelformat >> 16) & 0xff,
> +				(pix_mp->pixelformat >> 24) & 0xff);
> +			return NULL;
> +		}
> +	}
> +
> +	pix_mp->field = V4L2_FIELD_NONE;
> +	pix_mp->flags = 0;
> +	pix_mp->pixelformat = fmt->pixelformat;
> +	if (!V4L2_TYPE_IS_OUTPUT(f->type)) {
> +		pix_mp->colorspace = param->colorspace;
> +		pix_mp->xfer_func = param->xfer_func;
> +		pix_mp->ycbcr_enc = param->ycbcr_enc;
> +		pix_mp->quantization = param->quant;
> +	}
> +
> +	pix_limit = V4L2_TYPE_IS_OUTPUT(f->type) ? &param->limit->out_limit :
> +						&param->limit->cap_limit;
> +	s.min_width = pix_limit->wmin;
> +	s.max_width = pix_limit->wmax;
> +	s.step_width = fmt->walign;
> +	s.min_height = pix_limit->hmin;
> +	s.max_height = pix_limit->hmax;
> +	s.step_height = fmt->halign;
> +	org_w = pix_mp->width;
> +	org_h = pix_mp->height;
> +
> +	mdp_bound_align_image(&pix_mp->width, &pix_mp->height, &s, fmt->salign);
> +	if (org_w != pix_mp->width || org_h != pix_mp->height)
> +		dev_dbg(dev, "%d: size change: %ux%u to %ux%u", ctx_id,
> +			org_w, org_h, pix_mp->width, pix_mp->height);
> +
> +	if (pix_mp->num_planes && pix_mp->num_planes != fmt->num_planes)
> +		dev_dbg(dev, "%d num of planes change: %u to %u", ctx_id,
> +			pix_mp->num_planes, fmt->num_planes);
> +	pix_mp->num_planes = fmt->num_planes;
> +
> +	for (i = 0; i < pix_mp->num_planes; ++i) {
> +		u32 min_bpl = (pix_mp->width * fmt->row_depth[i]) >> 3;
> +		u32 max_bpl = (pix_limit->wmax * fmt->row_depth[i]) >> 3;
> +		u32 bpl = pix_mp->plane_fmt[i].bytesperline;
> +		u32 min_si, max_si;
> +		u32 si = pix_mp->plane_fmt[i].sizeimage;
> +
> +		bpl = clamp(bpl, min_bpl, max_bpl);
> +		pix_mp->plane_fmt[i].bytesperline = bpl;
> +
> +		min_si = (bpl * pix_mp->height * fmt->depth[i]) / fmt->row_depth[i];
> +		max_si = (bpl * s.max_height * fmt->depth[i]) / fmt->row_depth[i];
> +
> +		si = clamp(si, min_si, max_si);
> +		pix_mp->plane_fmt[i].sizeimage = si;
> +
> +		dev_dbg(dev, "%d: p%u, bpl:%u [%u, %u], sizeimage:%u [%u, %u]", ctx_id,
> +			i, bpl, min_bpl, max_bpl, si, min_si, max_si);
> +	}
> +
> +	return fmt;
> +}
> +
> +static int mdp_clamp_start(s32 *x, int min, int max, unsigned int align,
> +			   u32 flags)
> +{
> +	if (flags & V4L2_SEL_FLAG_GE)
> +		max = *x;
> +	if (flags & V4L2_SEL_FLAG_LE)
> +		min = *x;
> +	return mdp_clamp_align(x, min, max, align);
> +}
> +
> +static int mdp_clamp_end(s32 *x, int min, int max, unsigned int align,
> +			 u32 flags)
> +{
> +	if (flags & V4L2_SEL_FLAG_GE)
> +		min = *x;
> +	if (flags & V4L2_SEL_FLAG_LE)
> +		max = *x;
> +	return mdp_clamp_align(x, min, max, align);
> +}
> +
> +int mdp_try_crop(struct mdp_m2m_ctx *ctx, struct v4l2_rect *r,
> +		 const struct v4l2_selection *s, struct mdp_frame *frame)
> +{
> +	struct device *dev = &ctx->mdp_dev->pdev->dev;
> +	s32 left, top, right, bottom;
> +	u32 framew, frameh, walign, halign;
> +	int ret;
> +
> +	dev_dbg(dev, "%d target:%d, set:(%d,%d) %ux%u", ctx->id,
> +		s->target, s->r.left, s->r.top, s->r.width, s->r.height);
> +
> +	left = s->r.left;
> +	top = s->r.top;
> +	right = s->r.left + s->r.width;
> +	bottom = s->r.top + s->r.height;
> +	framew = frame->format.fmt.pix_mp.width;
> +	frameh = frame->format.fmt.pix_mp.height;
> +
> +	if (mdp_target_is_crop(s->target)) {
> +		walign = 1;
> +		halign = 1;
> +	} else {
> +		walign = frame->mdp_fmt->walign;
> +		halign = frame->mdp_fmt->halign;
> +	}
> +
> +	dev_dbg(dev, "%d align:%u,%u, bound:%ux%u", ctx->id,
> +		walign, halign, framew, frameh);
> +
> +	ret = mdp_clamp_start(&left, 0, right, walign, s->flags);
> +	if (ret)
> +		return ret;
> +	ret = mdp_clamp_start(&top, 0, bottom, halign, s->flags);
> +	if (ret)
> +		return ret;
> +	ret = mdp_clamp_end(&right, left, framew, walign, s->flags);
> +	if (ret)
> +		return ret;
> +	ret = mdp_clamp_end(&bottom, top, frameh, halign, s->flags);
> +	if (ret)
> +		return ret;
> +
> +	r->left = left;
> +	r->top = top;
> +	r->width = right - left;
> +	r->height = bottom - top;
> +
> +	dev_dbg(dev, "%d crop:(%d,%d) %ux%u", ctx->id,
> +		r->left, r->top, r->width, r->height);
> +	return 0;
> +}
> +
> +int mdp_check_scaling_ratio(const struct v4l2_rect *crop,
> +			    const struct v4l2_rect *compose, s32 rotation,
> +	const struct mdp_limit *limit)
> +{
> +	u32 crop_w, crop_h, comp_w, comp_h;
> +
> +	crop_w = crop->width;
> +	crop_h = crop->height;
> +	if (90 == rotation || 270 == rotation) {
> +		comp_w = compose->height;
> +		comp_h = compose->width;
> +	} else {
> +		comp_w = compose->width;
> +		comp_h = compose->height;
> +	}
> +
> +	if ((crop_w / comp_w) > limit->h_scale_down_max ||
> +	    (crop_h / comp_h) > limit->v_scale_down_max ||
> +	    (comp_w / crop_w) > limit->h_scale_up_max ||
> +	    (comp_h / crop_h) > limit->v_scale_up_max)
> +		return -ERANGE;
> +	return 0;
> +}
> +
> +/* Stride that is accepted by MDP HW */
> +static u32 mdp_fmt_get_stride(const struct mdp_format *fmt,
> +			      u32 bytesperline, unsigned int plane)
> +{
> +	enum mdp_color c = fmt->mdp_color;
> +	u32 stride;
> +
> +	stride = (bytesperline * MDP_COLOR_BITS_PER_PIXEL(c))
> +		/ fmt->row_depth[0];
> +	if (plane == 0)
> +		return stride;
> +	if (plane < MDP_COLOR_GET_PLANE_COUNT(c)) {
> +		if (MDP_COLOR_IS_BLOCK_MODE(c))
> +			stride = stride / 2;
> +		return stride;
> +	}
> +	return 0;
> +}
> +
> +/* Stride that is accepted by MDP HW of format with contiguous planes */
> +static u32 mdp_fmt_get_stride_contig(const struct mdp_format *fmt,
> +				     u32 pix_stride, unsigned int plane)
> +{
> +	enum mdp_color c = fmt->mdp_color;
> +	u32 stride = pix_stride;
> +
> +	if (plane == 0)
> +		return stride;
> +	if (plane < MDP_COLOR_GET_PLANE_COUNT(c)) {
> +		stride = stride >> MDP_COLOR_GET_H_SUBSAMPLE(c);
> +		if (MDP_COLOR_IS_UV_COPLANE(c) && !MDP_COLOR_IS_BLOCK_MODE(c))
> +			stride = stride * 2;
> +		return stride;
> +	}
> +	return 0;
> +}
> +
> +/* Plane size that is accepted by MDP HW */
> +static u32 mdp_fmt_get_plane_size(const struct mdp_format *fmt,
> +				  u32 stride, u32 height, unsigned int plane)
> +{
> +	enum mdp_color c = fmt->mdp_color;
> +	u32 bytesperline;
> +
> +	bytesperline = (stride * fmt->row_depth[0])
> +		/ MDP_COLOR_BITS_PER_PIXEL(c);
> +	if (plane == 0)
> +		return bytesperline * height;
> +	if (plane < MDP_COLOR_GET_PLANE_COUNT(c)) {
> +		height = height >> MDP_COLOR_GET_V_SUBSAMPLE(c);
> +		if (MDP_COLOR_IS_BLOCK_MODE(c))
> +			bytesperline = bytesperline * 2;
> +		return bytesperline * height;
> +	}
> +	return 0;
> +}
> +
> +static void mdp_prepare_buffer(struct img_image_buffer *b,
> +			       struct mdp_frame *frame, struct vb2_buffer *vb)
> +{
> +	struct v4l2_pix_format_mplane *pix_mp = &frame->format.fmt.pix_mp;
> +	unsigned int i;
> +
> +	b->format.colorformat = frame->mdp_fmt->mdp_color;
> +	b->format.ycbcr_prof = frame->ycbcr_prof;
> +	for (i = 0; i < pix_mp->num_planes; ++i) {
> +		u32 stride = mdp_fmt_get_stride(frame->mdp_fmt,
> +			pix_mp->plane_fmt[i].bytesperline, i);
> +
> +		b->format.plane_fmt[i].stride = stride;
> +		/*
> +		 * TODO : The way to pass an offset within a DMA-buf
> +		 * is not defined in V4L2 specification, so we abuse
> +		 * data_offset for now. Fix it when we have the right interface,
> +		 * including any necessary validation and potential alignment
> +		 * issues.
> +		 */
> +		b->format.plane_fmt[i].size =
> +			mdp_fmt_get_plane_size(frame->mdp_fmt, stride,
> +					       pix_mp->height, i) -
> +					       vb->planes[i].data_offset;
> +		b->iova[i] = vb2_dma_contig_plane_dma_addr(vb, i) +
> +			     vb->planes[i].data_offset;

Why do you need data_offset? What is the use case? Obviously I can't
merge a driver that abuses an API.

> +	}
> +	for (; i < MDP_COLOR_GET_PLANE_COUNT(b->format.colorformat); ++i) {
> +		u32 stride = mdp_fmt_get_stride_contig(frame->mdp_fmt,
> +			b->format.plane_fmt[0].stride, i);
> +
> +		b->format.plane_fmt[i].stride = stride;
> +		b->format.plane_fmt[i].size =
> +			mdp_fmt_get_plane_size(frame->mdp_fmt, stride,
> +					       pix_mp->height, i);
> +		b->iova[i] = b->iova[i - 1] + b->format.plane_fmt[i - 1].size;
> +	}
> +	b->usage = frame->usage;
> +}
> +
> +void mdp_set_src_config(struct img_input *in,
> +			struct mdp_frame *frame, struct vb2_buffer *vb)
> +{
> +	in->buffer.format.width = frame->format.fmt.pix_mp.width;
> +	in->buffer.format.height = frame->format.fmt.pix_mp.height;
> +	mdp_prepare_buffer(&in->buffer, frame, vb);
> +}
> +
> +static u32 mdp_to_fixed(u32 *r, struct v4l2_fract *f)
> +{
> +	u32 q;
> +
> +	if (f->denominator == 0) {
> +		*r = 0;
> +		return 0;
> +	}
> +
> +	q = f->numerator / f->denominator;
> +	*r = div_u64(((u64)f->numerator - q * f->denominator) <<
> +		     IMG_SUBPIXEL_SHIFT, f->denominator);
> +	return q;
> +}
> +
> +static void mdp_set_src_crop(struct img_crop *c, struct mdp_crop *crop)
> +{
> +	c->left = crop->c.left
> +		+ mdp_to_fixed(&c->left_subpix, &crop->left_subpix);
> +	c->top = crop->c.top
> +		+ mdp_to_fixed(&c->top_subpix, &crop->top_subpix);
> +	c->width = crop->c.width
> +		+ mdp_to_fixed(&c->width_subpix, &crop->width_subpix);
> +	c->height = crop->c.height
> +		+ mdp_to_fixed(&c->height_subpix, &crop->height_subpix);
> +}
> +
> +static void mdp_set_orientation(struct img_output *out,
> +				s32 rotation, bool hflip, bool vflip)
> +{
> +	u8 flip = 0;
> +
> +	if (hflip)
> +		flip ^= 1;
> +	if (vflip) {
> +		/*
> +		 * A vertical flip is equivalent to
> +		 * a 180-degree rotation with a horizontal flip
> +		 */
> +		rotation += 180;
> +		flip ^= 1;
> +	}
> +
> +	out->rotation = rotation % 360;
> +	if (flip != 0)
> +		out->flags |= IMG_CTRL_FLAG_HFLIP;
> +	else
> +		out->flags &= ~IMG_CTRL_FLAG_HFLIP;
> +}
> +
> +void mdp_set_dst_config(struct img_output *out,
> +			struct mdp_frame *frame, struct vb2_buffer *vb)
> +{
> +	out->buffer.format.width = frame->compose.width;
> +	out->buffer.format.height = frame->compose.height;
> +	mdp_prepare_buffer(&out->buffer, frame, vb);
> +	mdp_set_src_crop(&out->crop, &frame->crop);
> +	mdp_set_orientation(out, frame->rotation, frame->hflip, frame->vflip);
> +}
> +
> +int mdp_frameparam_init(struct mdp_frameparam *param)
> +{
> +	struct mdp_frame *frame;
> +
> +	if (!param)
> +		return -EINVAL;
> +
> +	INIT_LIST_HEAD(&param->list);
> +	param->limit = &mdp_def_limit;
> +	param->type = MDP_STREAM_TYPE_BITBLT;
> +
> +	frame = &param->output;
> +	frame->format.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> +	frame->mdp_fmt = mdp_try_fmt_mplane(&frame->format, param, 0);
> +	frame->ycbcr_prof =
> +		mdp_map_ycbcr_prof_mplane(&frame->format,
> +					  frame->mdp_fmt->mdp_color);
> +	frame->usage = MDP_BUFFER_USAGE_HW_READ;
> +
> +	param->num_captures = 1;
> +	frame = &param->captures[0];
> +	frame->format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> +	frame->mdp_fmt = mdp_try_fmt_mplane(&frame->format, param, 0);
> +	frame->ycbcr_prof =
> +		mdp_map_ycbcr_prof_mplane(&frame->format,
> +					  frame->mdp_fmt->mdp_color);
> +	frame->usage = MDP_BUFFER_USAGE_MDP;
> +	frame->crop.c.width = param->output.format.fmt.pix_mp.width;
> +	frame->crop.c.height = param->output.format.fmt.pix_mp.height;
> +	frame->compose.width = frame->format.fmt.pix_mp.width;
> +	frame->compose.height = frame->format.fmt.pix_mp.height;
> +
> +	return 0;
> +}

Regards,

	Hans
Laurent Pinchart July 8, 2022, 8:20 a.m. UTC | #2
On Fri, Jul 08, 2022 at 10:08:44AM +0200, Hans Verkuil wrote:
> Hi Moudy,
> 
> Some comments below:

And one more

> On 7/6/22 09:54, Moudy Ho wrote:
> > This patch adds driver for Mediatek's Media Data Path ver.3 (MDP3).
> > It provides the following functions:
> >   color transform, format conversion, resize, crop, rotate, flip
> >   and additional image quality enhancement.
> > 
> > The MDP3 driver is mainly used for Google Chromebook products to
> > import the new architecture to set the HW settings as shown below:
> >   User -> V4L2 framework
> >     -> MDP3 driver -> SCP (setting calculations)
> >       -> MDP3 driver -> CMDQ (GCE driver) -> HW
> > 
> > Each modules' related operation control is sited in mtk-mdp3-comp.c
> > Each modules' register table is defined in file with "mdp_reg_" prefix
> > GCE related API, operation control  sited in mtk-mdp3-cmdq.c
> > V4L2 m2m device functions are implemented in mtk-mdp3-m2m.c
> > Probe, power, suspend/resume, system level functions are defined in
> > mtk-mdp3-core.c
> > 
> > v4l2-compliance 1.22.1, 32 bits, 32-bit time_t
> > 
> > Compliance test for mtk-mdp3 device /dev/video2:
> > 
> > Driver Info:
> > 	Driver name      : mtk-mdp3
> > 	Card type        : 14001000.mdp3-rdma0
> 
> Card type is expected to be a human readable name, and that's not the case
> here.
> 
> > 	Bus info         : platform:mtk-mdp3
> 
> <snip>
> 
> > diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.c b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.c
> > new file mode 100644
> > index 000000000000..9d2b8acc303f
> > --- /dev/null
> > +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.c
> > @@ -0,0 +1,769 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Ping-Hsun Wu <ping-hsun.wu@mediatek.com>
> > + */
> > +
> > +#include <linux/platform_device.h>
> > +#include <media/v4l2-ioctl.h>
> > +#include <media/v4l2-event.h>
> > +#include <media/videobuf2-dma-contig.h>
> > +#include "mtk-mdp3-m2m.h"
> > +
> > +static inline struct mdp_m2m_ctx *fh_to_ctx(struct v4l2_fh *fh)
> > +{
> > +	return container_of(fh, struct mdp_m2m_ctx, fh);
> > +}
> > +
> > +static inline struct mdp_m2m_ctx *ctrl_to_ctx(struct v4l2_ctrl *ctrl)
> > +{
> > +	return container_of(ctrl->handler, struct mdp_m2m_ctx, ctrl_handler);
> > +}
> > +
> > +static inline struct mdp_frame *ctx_get_frame(struct mdp_m2m_ctx *ctx,
> > +					      enum v4l2_buf_type type)
> > +{
> > +	if (V4L2_TYPE_IS_OUTPUT(type))
> > +		return &ctx->curr_param.output;
> > +	else
> > +		return &ctx->curr_param.captures[0];
> > +}
> > +
> > +static void mdp_m2m_ctx_set_state(struct mdp_m2m_ctx *ctx, u32 state)
> > +{
> > +	atomic_or(state, &ctx->curr_param.state);
> > +}
> > +
> > +static bool mdp_m2m_ctx_is_state_set(struct mdp_m2m_ctx *ctx, u32 mask)
> > +{
> > +	bool ret;
> > +
> > +	ret = ((atomic_read(&ctx->curr_param.state) & mask) == mask);
> 
> This can just do 'return' so you can drop the 'ret' variable.
> 
> > +
> > +	return ret;
> > +}
> > +
> > +static void mdp_m2m_process_done(void *priv, int vb_state)
> > +{
> > +	struct mdp_m2m_ctx *ctx = priv;
> > +	struct vb2_v4l2_buffer *src_vbuf, *dst_vbuf;
> > +
> > +	src_vbuf = (struct vb2_v4l2_buffer *)
> > +			v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
> > +	dst_vbuf = (struct vb2_v4l2_buffer *)
> > +			v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
> > +	ctx->curr_param.frame_no = ctx->frame_count[MDP_M2M_SRC];
> > +	src_vbuf->sequence = ctx->frame_count[MDP_M2M_SRC]++;
> > +	dst_vbuf->sequence = ctx->frame_count[MDP_M2M_DST]++;
> > +	v4l2_m2m_buf_copy_metadata(src_vbuf, dst_vbuf, true);
> > +
> > +	v4l2_m2m_buf_done(src_vbuf, vb_state);
> > +	v4l2_m2m_buf_done(dst_vbuf, vb_state);
> > +	v4l2_m2m_job_finish(ctx->mdp_dev->m2m_dev, ctx->m2m_ctx);
> > +}
> > +
> > +static void mdp_m2m_device_run(void *priv)
> > +{
> > +	struct mdp_m2m_ctx *ctx = priv;
> > +	struct mdp_frame *frame;
> > +	struct vb2_v4l2_buffer *src_vb, *dst_vb;
> > +	struct img_ipi_frameparam param = {0};
> > +	struct mdp_cmdq_param task = {0};
> 
> Just use '= {}', it's cleaner.
> 
> > +	enum vb2_buffer_state vb_state = VB2_BUF_STATE_ERROR;
> > +	int ret;
> > +
> > +	if (mdp_m2m_ctx_is_state_set(ctx, MDP_M2M_CTX_ERROR)) {
> > +		dev_err(&ctx->mdp_dev->pdev->dev,
> > +			"mdp_m2m_ctx is in error state\n");
> > +		goto worker_end;
> > +	}
> > +
> > +	param.frame_no = ctx->curr_param.frame_no;
> > +	param.type = ctx->curr_param.type;
> > +	param.num_inputs = 1;
> > +	param.num_outputs = 1;
> > +
> > +	frame = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
> > +	src_vb = v4l2_m2m_next_src_buf(ctx->m2m_ctx);
> > +	mdp_set_src_config(&param.inputs[0], frame, &src_vb->vb2_buf);
> > +
> > +	frame = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> > +	dst_vb = v4l2_m2m_next_dst_buf(ctx->m2m_ctx);
> > +	mdp_set_dst_config(&param.outputs[0], frame, &dst_vb->vb2_buf);
> > +
> > +	ret = mdp_vpu_process(&ctx->vpu, &param);
> > +	if (ret) {
> > +		dev_err(&ctx->mdp_dev->pdev->dev,
> > +			"VPU MDP process failed: %d\n", ret);
> > +		goto worker_end;
> > +	}
> > +
> > +	task.config = ctx->vpu.config;
> > +	task.param = &param;
> > +	task.composes[0] = &frame->compose;
> > +	task.cmdq_cb = NULL;
> > +	task.cb_data = NULL;
> > +	task.mdp_ctx = ctx;
> > +
> > +	ret = mdp_cmdq_send(ctx->mdp_dev, &task);
> > +	if (ret) {
> > +		dev_err(&ctx->mdp_dev->pdev->dev,
> > +			"CMDQ sendtask failed: %d\n", ret);
> > +		goto worker_end;
> > +	}
> > +
> > +	return;
> > +
> > +worker_end:
> > +	mdp_m2m_process_done(ctx, vb_state);
> > +}
> > +
> > +static int mdp_m2m_start_streaming(struct vb2_queue *q, unsigned int count)
> > +{
> > +	struct mdp_m2m_ctx *ctx = vb2_get_drv_priv(q);
> > +
> > +	ctx->frame_count[MDP_M2M_SRC] = 0;
> > +	ctx->frame_count[MDP_M2M_DST] = 0;
> 
> Don't set both, check which queue it is first.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static struct vb2_v4l2_buffer *mdp_m2m_buf_remove(struct mdp_m2m_ctx *ctx,
> > +						  unsigned int type)
> > +{
> > +	if (V4L2_TYPE_IS_OUTPUT(type))
> > +		return (struct vb2_v4l2_buffer *)
> > +			v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
> > +	else
> > +		return (struct vb2_v4l2_buffer *)
> > +			v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
> > +}
> > +
> > +static void mdp_m2m_stop_streaming(struct vb2_queue *q)
> > +{
> > +	struct mdp_m2m_ctx *ctx = vb2_get_drv_priv(q);
> > +	struct vb2_v4l2_buffer *vb;
> > +
> > +	vb = mdp_m2m_buf_remove(ctx, q->type);
> > +	while (vb) {
> > +		v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);
> > +		vb = mdp_m2m_buf_remove(ctx, q->type);
> > +	}
> > +}
> > +
> > +static int mdp_m2m_queue_setup(struct vb2_queue *q,
> > +			       unsigned int *num_buffers,
> > +			       unsigned int *num_planes, unsigned int sizes[],
> > +			       struct device *alloc_devs[])
> > +{
> > +	struct mdp_m2m_ctx *ctx = vb2_get_drv_priv(q);
> > +	struct v4l2_pix_format_mplane *pix_mp;
> > +	struct device *dev = &ctx->mdp_dev->pdev->dev;
> > +	u32 i;
> > +
> > +	pix_mp = &ctx_get_frame(ctx, q->type)->format.fmt.pix_mp;
> > +
> > +	/* from VIDIOC_CREATE_BUFS */
> > +	if (*num_planes) {
> > +		if (*num_planes != pix_mp->num_planes)
> > +			return -EINVAL;
> > +		for (i = 0; i < pix_mp->num_planes; ++i)
> > +			if (sizes[i] < pix_mp->plane_fmt[i].sizeimage)
> > +				return -EINVAL;
> > +	} else {/* from VIDIOC_REQBUFS */
> > +		*num_planes = pix_mp->num_planes;
> > +		for (i = 0; i < pix_mp->num_planes; ++i)
> > +			sizes[i] = pix_mp->plane_fmt[i].sizeimage;
> > +	}
> > +
> > +	dev_info(dev, "[%d] type:%d, planes:%u, buffers:%u, size:%u,%u,%u",
> 
> This should be dropped or changed to dev_dbg. You don't want logging
> when doing normal things, that will just spam the kernel log.
> 
> > +		 ctx->id, q->type, *num_planes, *num_buffers,
> > +		 sizes[0], sizes[1], sizes[2]);
> > +	return 0;
> > +}
> > +
> > +static int mdp_m2m_buf_prepare(struct vb2_buffer *vb)
> > +{
> > +	struct mdp_m2m_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > +	struct v4l2_pix_format_mplane *pix_mp;
> > +	struct vb2_v4l2_buffer *v4l2_buf = to_vb2_v4l2_buffer(vb);
> > +	u32 i;
> > +
> > +	v4l2_buf->field = V4L2_FIELD_NONE;
> > +
> > +	if (!V4L2_TYPE_IS_OUTPUT(vb->type)) {
> > +		pix_mp = &ctx_get_frame(ctx, vb->type)->format.fmt.pix_mp;
> > +		for (i = 0; i < pix_mp->num_planes; ++i) {
> > +			vb2_set_plane_payload(vb, i,
> > +					      pix_mp->plane_fmt[i].sizeimage);
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int mdp_m2m_buf_out_validate(struct vb2_buffer *vb)
> > +{
> > +	struct vb2_v4l2_buffer *v4l2_buf = to_vb2_v4l2_buffer(vb);
> > +
> > +	v4l2_buf->field = V4L2_FIELD_NONE;
> > +
> > +	return 0;
> > +}
> > +
> > +static void mdp_m2m_buf_queue(struct vb2_buffer *vb)
> > +{
> > +	struct mdp_m2m_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > +	struct vb2_v4l2_buffer *v4l2_buf = to_vb2_v4l2_buffer(vb);
> > +
> > +	v4l2_buf->field = V4L2_FIELD_NONE;
> > +
> > +	v4l2_m2m_buf_queue(ctx->m2m_ctx, to_vb2_v4l2_buffer(vb));
> > +}
> > +
> > +static const struct vb2_ops mdp_m2m_qops = {
> > +	.queue_setup	= mdp_m2m_queue_setup,
> > +	.wait_prepare	= vb2_ops_wait_prepare,
> > +	.wait_finish	= vb2_ops_wait_finish,
> > +	.buf_prepare	= mdp_m2m_buf_prepare,
> > +	.start_streaming = mdp_m2m_start_streaming,
> > +	.stop_streaming	= mdp_m2m_stop_streaming,
> > +	.buf_queue	= mdp_m2m_buf_queue,
> > +	.buf_out_validate = mdp_m2m_buf_out_validate,
> > +};
> > +
> > +static int mdp_m2m_querycap(struct file *file, void *fh,
> > +			    struct v4l2_capability *cap)
> > +{
> > +	struct mdp_m2m_ctx *ctx = fh_to_ctx(fh);
> > +
> > +	strscpy(cap->driver, MDP_MODULE_NAME, sizeof(cap->driver));
> > +	strscpy(cap->card, ctx->mdp_dev->pdev->name, sizeof(cap->card));
> 
> As mentioned at the top, this is not a suitable name for cap->card.
> 
> > +	strscpy(cap->bus_info, "platform:mtk-mdp3", sizeof(cap->bus_info));

And don't override bus_info, the value isn't right. The V4L2 core should
do the right thing for platform devices now.

> > +	return 0;
> > +}
> > +
> > +static int mdp_m2m_enum_fmt_mplane(struct file *file, void *fh,
> > +				   struct v4l2_fmtdesc *f)
> > +{
> > +	return mdp_enum_fmt_mplane(f);
> > +}
> > +
> > +static int mdp_m2m_g_fmt_mplane(struct file *file, void *fh,
> > +				struct v4l2_format *f)
> > +{
> > +	struct mdp_m2m_ctx *ctx = fh_to_ctx(fh);
> > +	struct mdp_frame *frame;
> > +	struct v4l2_pix_format_mplane *pix_mp;
> > +	struct device *dev = &ctx->mdp_dev->pdev->dev;
> > +
> > +	frame = ctx_get_frame(ctx, f->type);
> > +	*f = frame->format;
> > +	pix_mp = &f->fmt.pix_mp;
> > +	pix_mp->colorspace = ctx->curr_param.colorspace;
> > +	pix_mp->xfer_func = ctx->curr_param.xfer_func;
> > +	pix_mp->ycbcr_enc = ctx->curr_param.ycbcr_enc;
> > +	pix_mp->quantization = ctx->curr_param.quant;
> > +
> > +	dev_info(dev, "[%d] type:%d, frame:%ux%u colorspace=%d", ctx->id, f->type,
> > +		 f->fmt.pix_mp.width, f->fmt.pix_mp.height,
> > +		 f->fmt.pix_mp.colorspace);
> 
> Drop this, you're returning this info to userspace anyway, so why spam the
> kernel log?
> 
> > +	return 0;
> > +}
> > +
> > +static int mdp_m2m_s_fmt_mplane(struct file *file, void *fh,
> > +				struct v4l2_format *f)
> > +{
> > +	struct mdp_m2m_ctx *ctx = fh_to_ctx(fh);
> > +	struct mdp_frame *frame = ctx_get_frame(ctx, f->type);
> > +	struct mdp_frame *capture;
> > +	const struct mdp_format *fmt;
> > +	struct vb2_queue *vq;
> > +	struct device *dev = &ctx->mdp_dev->pdev->dev;
> > +
> > +	dev_info(dev, "[%d] type:%d", ctx->id, f->type);
> 
> dev_dbg. If dev_info is used elsewhere in similar situations, then replace that
> with dev_dbg as well. Regular usage of this driver must not produce kernel logging.
> 
> > +
> > +	fmt = mdp_try_fmt_mplane(f, &ctx->curr_param, ctx->id);
> > +	if (!fmt) {
> > +		dev_info(dev, "[%d] try_fmt failed, type:%d", ctx->id, f->type);
> 
> Drop this, the error code says enough.
> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type);
> > +	if (vb2_is_streaming(vq)) {
> 
> This must be vb2_is_busy(): changing formats is prohibited once buffers are
> allocated (vb2_is_busy() is true in that case).
> 
> > +		dev_info(&ctx->mdp_dev->pdev->dev, "Queue %d busy\n", f->type);
> 
> Drop this, the error code says enough.
> 
> > +		return -EBUSY;
> > +	}
> > +
> > +	frame->format = *f;
> > +	frame->mdp_fmt = fmt;
> > +	frame->ycbcr_prof = mdp_map_ycbcr_prof_mplane(f, fmt->mdp_color);
> > +	frame->usage = V4L2_TYPE_IS_OUTPUT(f->type) ?
> > +		MDP_BUFFER_USAGE_HW_READ : MDP_BUFFER_USAGE_MDP;
> > +
> > +	capture = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> > +	if (V4L2_TYPE_IS_OUTPUT(f->type)) {
> > +		capture->crop.c.left = 0;
> > +		capture->crop.c.top = 0;
> > +		capture->crop.c.width = f->fmt.pix_mp.width;
> > +		capture->crop.c.height = f->fmt.pix_mp.height;
> > +		ctx->curr_param.colorspace = f->fmt.pix_mp.colorspace;
> > +		ctx->curr_param.ycbcr_enc = f->fmt.pix_mp.ycbcr_enc;
> > +		ctx->curr_param.quant = f->fmt.pix_mp.quantization;
> > +		ctx->curr_param.xfer_func = f->fmt.pix_mp.xfer_func;
> > +	} else {
> > +		capture->compose.left = 0;
> > +		capture->compose.top = 0;
> > +		capture->compose.width = f->fmt.pix_mp.width;
> > +		capture->compose.height = f->fmt.pix_mp.height;
> > +	}
> > +
> > +	ctx->frame_count[MDP_M2M_SRC] = 0;
> > +	ctx->frame_count[MDP_M2M_DST] = 0;
> 
> Changing the format doesn't mean you need to zero this. Just drop these two
> line.
> 
> > +
> > +	dev_info(dev, "[%d] type:%d, frame:%ux%u", ctx->id, f->type,
> > +		 f->fmt.pix_mp.width, f->fmt.pix_mp.height);
> 
> Drop this.
> 
> > +	return 0;
> > +}
> > +
> > +static int mdp_m2m_try_fmt_mplane(struct file *file, void *fh,
> > +				  struct v4l2_format *f)
> > +{
> > +	struct mdp_m2m_ctx *ctx = fh_to_ctx(fh);
> > +
> > +	if (!mdp_try_fmt_mplane(f, &ctx->curr_param, ctx->id))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mdp_m2m_streamon(struct file *file, void *fh,
> > +			    enum v4l2_buf_type type)
> > +{
> > +	struct mdp_m2m_ctx *ctx = fh_to_ctx(fh);
> > +	struct mdp_frame *capture;
> > +	int ret;
> > +	bool out_streaming, cap_streaming;
> > +
> > +	capture = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> > +	out_streaming = ctx->m2m_ctx->out_q_ctx.q.streaming;
> > +	cap_streaming = ctx->m2m_ctx->cap_q_ctx.q.streaming;
> 
> Use vb2_is_streaming() rather than accessing q.streaming directly.
> 
> > +
> > +	/* Check to see if scaling ratio is within supported range */
> > +	if ((V4L2_TYPE_IS_OUTPUT(type) && cap_streaming) ||
> > +	    (!V4L2_TYPE_IS_OUTPUT(type) && out_streaming)) {
> > +		ret = mdp_check_scaling_ratio(&capture->crop.c,
> > +					      &capture->compose,
> > +					      capture->rotation,
> > +					      ctx->curr_param.limit);
> > +		if (ret) {
> > +			dev_info(&ctx->mdp_dev->pdev->dev,
> > +				 "Out of scaling range\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	if (!mdp_m2m_ctx_is_state_set(ctx, MDP_VPU_INIT)) {
> > +		ret = mdp_vpu_get_locked(ctx->mdp_dev);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = mdp_vpu_ctx_init(&ctx->vpu, &ctx->mdp_dev->vpu,
> > +				       MDP_DEV_M2M);
> > +		if (ret) {
> > +			dev_err(&ctx->mdp_dev->pdev->dev,
> > +				"VPU init failed %d\n", ret);
> > +			return -EINVAL;
> > +		}
> > +		mdp_m2m_ctx_set_state(ctx, MDP_VPU_INIT);
> > +	}
> > +
> > +	return v4l2_m2m_streamon(file, ctx->m2m_ctx, type);
> > +}
> 
> There really is no reason to override streamon. You can also do this in
> start_streaming(). I recommend moving these tests there. Internally
> streamon() will call start_streaming(), so any error you return in that
> callback function will be the error return of the streamon as well.
> 
> > +
> > +static int mdp_m2m_g_selection(struct file *file, void *fh,
> > +			       struct v4l2_selection *s)
> > +{
> > +	struct mdp_m2m_ctx *ctx = fh_to_ctx(fh);
> > +	struct mdp_frame *frame;
> > +	struct device *dev = &ctx->mdp_dev->pdev->dev;
> > +	bool valid = false;
> > +
> > +	if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > +		valid = mdp_target_is_crop(s->target);
> > +	else if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > +		valid = mdp_target_is_compose(s->target);
> > +
> > +	if (!valid) {
> > +		dev_err(dev, "[%s:%d] invalid type:%u target:%u", __func__, ctx->id,
> > +			s->type, s->target);
> 
> Drop this. User errors should never cause spamming in the kernel log.
> 
> Please check your code: you can use dev_dbg to help debugging, but dev_info
> should be used very sparingly (typically only in probe() and remove()), and
> dev_warn/err only for hardware/driver warnings/errors, and never due to
> userspace inputs.
> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	switch (s->target) {
> > +	case V4L2_SEL_TGT_CROP:
> > +		if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> > +			frame = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> > +			s->r = frame->crop.c;
> > +			return 0;
> > +		}
> 
> It's easier to read if you swap the tests:
> 
> 		if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> 			return -EINVAL;
> 		frame = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> 		s->r = frame->crop.c;
> 		return 0;
> 
> Same below.
> 
> > +		break;
> > +	case V4L2_SEL_TGT_COMPOSE:
> > +		if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> > +			frame = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> > +			s->r = frame->compose;
> > +			return 0;
> > +		}
> > +		break;
> > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > +		if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> > +			frame = ctx_get_frame(ctx, s->type);
> > +			s->r.left = 0;
> > +			s->r.top = 0;
> > +			s->r.width = frame->format.fmt.pix_mp.width;
> > +			s->r.height = frame->format.fmt.pix_mp.height;
> > +			return 0;
> > +		}
> > +		break;
> > +	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> > +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> > +		if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> > +			frame = ctx_get_frame(ctx, s->type);
> > +			s->r.left = 0;
> > +			s->r.top = 0;
> > +			s->r.width = frame->format.fmt.pix_mp.width;
> > +			s->r.height = frame->format.fmt.pix_mp.height;
> > +			return 0;
> > +		}
> > +		break;
> > +	}
> > +	return -EINVAL;
> > +}
> > +
> > +static int mdp_m2m_s_selection(struct file *file, void *fh,
> > +			       struct v4l2_selection *s)
> > +{
> > +	struct mdp_m2m_ctx *ctx = fh_to_ctx(fh);
> > +	struct mdp_frame *frame = ctx_get_frame(ctx, s->type);
> > +	struct mdp_frame *capture;
> > +	struct v4l2_rect r;
> > +	struct device *dev = &ctx->mdp_dev->pdev->dev;
> > +	bool valid = false;
> > +	int ret;
> > +
> > +	if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > +		valid = (s->target == V4L2_SEL_TGT_CROP);
> > +	else if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > +		valid = (s->target == V4L2_SEL_TGT_COMPOSE);
> > +
> > +	if (!valid) {
> > +		dev_err(dev, "[%s:%d] invalid type:%u target:%u", __func__, ctx->id,
> > +			s->type, s->target);
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = mdp_try_crop(ctx, &r, s, frame);
> > +	if (ret)
> > +		return ret;
> > +	capture = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> > +
> > +	if (mdp_target_is_crop(s->target))
> > +		capture->crop.c = r;
> > +	else
> > +		capture->compose = r;
> > +
> > +	s->r = r;
> > +	memset(s->reserved, 0, sizeof(s->reserved));
> 
> No need for this memset, the v4l2 core will clear this for you.
> 
> > +
> > +	ctx->frame_count[MDP_M2M_SRC] = 0;
> > +	ctx->frame_count[MDP_M2M_DST] = 0;
> 
> Drop this!
> 
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_ioctl_ops mdp_m2m_ioctl_ops = {
> > +	.vidioc_querycap		= mdp_m2m_querycap,
> > +	.vidioc_enum_fmt_vid_cap	= mdp_m2m_enum_fmt_mplane,
> > +	.vidioc_enum_fmt_vid_out	= mdp_m2m_enum_fmt_mplane,
> > +	.vidioc_g_fmt_vid_cap_mplane	= mdp_m2m_g_fmt_mplane,
> > +	.vidioc_g_fmt_vid_out_mplane	= mdp_m2m_g_fmt_mplane,
> > +	.vidioc_s_fmt_vid_cap_mplane	= mdp_m2m_s_fmt_mplane,
> > +	.vidioc_s_fmt_vid_out_mplane	= mdp_m2m_s_fmt_mplane,
> > +	.vidioc_try_fmt_vid_cap_mplane	= mdp_m2m_try_fmt_mplane,
> > +	.vidioc_try_fmt_vid_out_mplane	= mdp_m2m_try_fmt_mplane,
> > +	.vidioc_reqbufs			= v4l2_m2m_ioctl_reqbufs,
> > +	.vidioc_querybuf		= v4l2_m2m_ioctl_querybuf,
> > +	.vidioc_qbuf			= v4l2_m2m_ioctl_qbuf,
> > +	.vidioc_expbuf			= v4l2_m2m_ioctl_expbuf,
> > +	.vidioc_dqbuf			= v4l2_m2m_ioctl_dqbuf,
> > +	.vidioc_create_bufs		= v4l2_m2m_ioctl_create_bufs,
> > +	.vidioc_streamon		= mdp_m2m_streamon,
> > +	.vidioc_streamoff		= v4l2_m2m_ioctl_streamoff,
> > +	.vidioc_g_selection		= mdp_m2m_g_selection,
> > +	.vidioc_s_selection		= mdp_m2m_s_selection,
> > +	.vidioc_subscribe_event		= v4l2_ctrl_subscribe_event,
> > +	.vidioc_unsubscribe_event	= v4l2_event_unsubscribe,
> > +};
> > +
> > +static int mdp_m2m_queue_init(void *priv,
> > +			      struct vb2_queue *src_vq,
> > +			      struct vb2_queue *dst_vq)
> > +{
> > +	struct mdp_m2m_ctx *ctx = priv;
> > +	int ret;
> > +
> > +	src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> > +	src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> > +	src_vq->ops = &mdp_m2m_qops;
> > +	src_vq->mem_ops = &vb2_dma_contig_memops;
> > +	src_vq->drv_priv = ctx;
> > +	src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> > +	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > +	src_vq->dev = &ctx->mdp_dev->pdev->dev;
> > +	src_vq->lock = &ctx->ctx_lock;
> > +
> > +	ret = vb2_queue_init(src_vq);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> > +	dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> > +	dst_vq->ops = &mdp_m2m_qops;
> > +	dst_vq->mem_ops = &vb2_dma_contig_memops;
> > +	dst_vq->drv_priv = ctx;
> > +	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> > +	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > +	dst_vq->dev = &ctx->mdp_dev->pdev->dev;
> > +	dst_vq->lock = &ctx->ctx_lock;
> > +
> > +	return vb2_queue_init(dst_vq);
> > +}
> > +
> > +static int mdp_m2m_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct mdp_m2m_ctx *ctx = ctrl_to_ctx(ctrl);
> > +	struct mdp_frame *capture;
> > +
> > +	if (ctrl->flags & V4L2_CTRL_FLAG_INACTIVE)
> > +		return 0;
> 
> Why this test? As far as I can tell this flag is never set, so there
> is no need to check against it.
> 
> > +
> > +	capture = ctx_get_frame(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> > +	switch (ctrl->id) {
> > +	case V4L2_CID_HFLIP:
> > +		capture->hflip = ctrl->val;
> > +		break;
> > +	case V4L2_CID_VFLIP:
> > +		capture->vflip = ctrl->val;
> > +		break;
> > +	case V4L2_CID_ROTATE:
> > +		capture->rotation = ctrl->val;
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops mdp_m2m_ctrl_ops = {
> > +	.s_ctrl	= mdp_m2m_s_ctrl,
> > +};
> > +
> > +static int mdp_m2m_ctrls_create(struct mdp_m2m_ctx *ctx)
> > +{
> > +	v4l2_ctrl_handler_init(&ctx->ctrl_handler, MDP_MAX_CTRLS);
> > +	ctx->ctrls.hflip = v4l2_ctrl_new_std(&ctx->ctrl_handler,
> > +					     &mdp_m2m_ctrl_ops, V4L2_CID_HFLIP,
> > +					     0, 1, 1, 0);
> > +	ctx->ctrls.vflip = v4l2_ctrl_new_std(&ctx->ctrl_handler,
> > +					     &mdp_m2m_ctrl_ops, V4L2_CID_VFLIP,
> > +					     0, 1, 1, 0);
> > +	ctx->ctrls.rotate = v4l2_ctrl_new_std(&ctx->ctrl_handler,
> > +					      &mdp_m2m_ctrl_ops,
> > +					      V4L2_CID_ROTATE, 0, 270, 90, 0);
> > +
> > +	if (ctx->ctrl_handler.error) {
> > +		int err = ctx->ctrl_handler.error;
> > +
> > +		v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> > +		dev_err(&ctx->mdp_dev->pdev->dev,
> > +			"Failed to register controls\n");
> > +		return err;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int mdp_m2m_open(struct file *file)
> > +{
> > +	struct video_device *vdev = video_devdata(file);
> > +	struct mdp_dev *mdp = video_get_drvdata(vdev);
> > +	struct mdp_m2m_ctx *ctx;
> > +	struct device *dev = &mdp->pdev->dev;
> > +	int ret;
> > +	struct v4l2_format default_format;
> 
> Just add '= {}' to avoid the memset later on.
> 
> > +
> > +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> > +	if (!ctx)
> > +		return -ENOMEM;
> > +
> > +	if (mutex_lock_interruptible(&mdp->m2m_lock)) {
> > +		ret = -ERESTARTSYS;
> > +		goto err_free_ctx;
> > +	}
> > +
> > +	ctx->id = ida_alloc(&mdp->mdp_ida, GFP_KERNEL);
> > +	ctx->mdp_dev = mdp;
> > +
> > +	v4l2_fh_init(&ctx->fh, vdev);
> > +	vdev->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING;
> 
> This doesn't belong in open(). It is already done when the video device is registered.
> 
> > +	file->private_data = &ctx->fh;
> > +	ret = mdp_m2m_ctrls_create(ctx);
> > +	if (ret)
> > +		goto err_exit_fh;
> > +
> > +	/* Use separate control handler per file handle */
> > +	ctx->fh.ctrl_handler = &ctx->ctrl_handler;
> > +	v4l2_fh_add(&ctx->fh);
> > +
> > +	mutex_init(&ctx->ctx_lock);
> > +	ctx->m2m_ctx = v4l2_m2m_ctx_init(mdp->m2m_dev, ctx, mdp_m2m_queue_init);
> > +	if (IS_ERR(ctx->m2m_ctx)) {
> > +		dev_err(dev, "Failed to initialize m2m context\n");
> > +		ret = PTR_ERR(ctx->m2m_ctx);
> > +		goto err_release_handler;
> > +	}
> > +	ctx->fh.m2m_ctx = ctx->m2m_ctx;
> > +
> > +	ctx->curr_param.ctx = ctx;
> > +	ret = mdp_frameparam_init(&ctx->curr_param);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to initialize mdp parameter\n");
> > +		goto err_release_m2m_ctx;
> > +	}
> > +
> > +	mutex_unlock(&mdp->m2m_lock);
> > +
> > +	/* Default format */
> > +	memset(&default_format, 0, sizeof(default_format));
> 
> This can be dropped if default_format is inited with = {}.
> 
> > +	default_format.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> > +	default_format.fmt.pix_mp.width = 32;
> > +	default_format.fmt.pix_mp.height = 32;
> > +	default_format.fmt.pix_mp.pixelformat = V4L2_PIX_FMT_YUV420M;
> > +	mdp_m2m_s_fmt_mplane(file, &ctx->fh, &default_format);
> > +	default_format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> > +	mdp_m2m_s_fmt_mplane(file, &ctx->fh, &default_format);
> > +
> > +	dev_dbg(dev, "%s:[%d]", __func__, ctx->id);
> > +
> > +	return 0;
> > +
> > +err_release_m2m_ctx:
> > +	v4l2_m2m_ctx_release(ctx->m2m_ctx);
> > +err_release_handler:
> > +	v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> > +	v4l2_fh_del(&ctx->fh);
> > +err_exit_fh:
> > +	v4l2_fh_exit(&ctx->fh);
> > +	mutex_unlock(&mdp->m2m_lock);
> > +err_free_ctx:
> > +	kfree(ctx);
> > +
> > +	return ret;
> > +}
> > +
> > +static int mdp_m2m_release(struct file *file)
> > +{
> > +	struct mdp_m2m_ctx *ctx = fh_to_ctx(file->private_data);
> > +	struct mdp_dev *mdp = video_drvdata(file);
> > +	struct device *dev = &mdp->pdev->dev;
> > +
> > +	mutex_lock(&mdp->m2m_lock);
> > +	v4l2_m2m_ctx_release(ctx->m2m_ctx);
> > +	if (mdp_m2m_ctx_is_state_set(ctx, MDP_VPU_INIT)) {
> > +		mdp_vpu_ctx_deinit(&ctx->vpu);
> > +		mdp_vpu_put_locked(mdp);
> > +	}
> > +
> > +	v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> > +	v4l2_fh_del(&ctx->fh);
> > +	v4l2_fh_exit(&ctx->fh);
> > +	ida_free(&mdp->mdp_ida, ctx->id);
> > +	mutex_unlock(&mdp->m2m_lock);
> > +
> > +	dev_dbg(dev, "%s:[%d]", __func__, ctx->id);
> > +	kfree(ctx);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_file_operations mdp_m2m_fops = {
> > +	.owner		= THIS_MODULE,
> > +	.poll		= v4l2_m2m_fop_poll,
> > +	.unlocked_ioctl	= video_ioctl2,
> > +	.mmap		= v4l2_m2m_fop_mmap,
> > +	.open		= mdp_m2m_open,
> > +	.release	= mdp_m2m_release,
> > +};
> > +
> > +static const struct v4l2_m2m_ops mdp_m2m_ops = {
> > +	.device_run	= mdp_m2m_device_run,
> > +};
> > +
> > +int mdp_m2m_device_register(struct mdp_dev *mdp)
> > +{
> > +	struct device *dev = &mdp->pdev->dev;
> > +	int ret = 0;
> > +
> > +	mdp->m2m_vdev = video_device_alloc();
> > +	if (!mdp->m2m_vdev) {
> > +		dev_err(dev, "Failed to allocate video device\n");
> > +		ret = -ENOMEM;
> > +		goto err_video_alloc;
> > +	}
> > +	mdp->m2m_vdev->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE |
> > +		V4L2_CAP_STREAMING;
> > +	mdp->m2m_vdev->fops = &mdp_m2m_fops;
> > +	mdp->m2m_vdev->ioctl_ops = &mdp_m2m_ioctl_ops;
> > +	mdp->m2m_vdev->release = mdp_video_device_release;
> > +	mdp->m2m_vdev->lock = &mdp->m2m_lock;
> > +	mdp->m2m_vdev->vfl_dir = VFL_DIR_M2M;
> > +	mdp->m2m_vdev->v4l2_dev = &mdp->v4l2_dev;
> > +	snprintf(mdp->m2m_vdev->name, sizeof(mdp->m2m_vdev->name), "%s:m2m",
> > +		 MDP_MODULE_NAME);
> > +	video_set_drvdata(mdp->m2m_vdev, mdp);
> > +
> > +	mdp->m2m_dev = v4l2_m2m_init(&mdp_m2m_ops);
> > +	if (IS_ERR(mdp->m2m_dev)) {
> > +		dev_err(dev, "Failed to initialize v4l2-m2m device\n");
> > +		ret = PTR_ERR(mdp->m2m_dev);
> > +		goto err_m2m_init;
> > +	}
> > +
> > +	ret = video_register_device(mdp->m2m_vdev, VFL_TYPE_VIDEO, 2);
> 
> Why 2? Just use -1 to pick the first free device number.
> 
> > +	if (ret) {
> > +		dev_err(dev, "Failed to register video device\n");
> > +		goto err_video_register;
> > +	}
> > +
> > +	v4l2_info(&mdp->v4l2_dev, "Driver registered as /dev/video%d",
> > +		  mdp->m2m_vdev->num);
> > +	return 0;
> > +
> > +err_video_register:
> > +	v4l2_m2m_release(mdp->m2m_dev);
> > +err_m2m_init:
> > +	video_device_release(mdp->m2m_vdev);
> > +err_video_alloc:
> > +
> > +	return ret;
> > +}
> > +
> > +void mdp_m2m_device_unregister(struct mdp_dev *mdp)
> > +{
> > +	video_unregister_device(mdp->m2m_vdev);
> > +}
> > +
> > +void mdp_m2m_job_finish(struct mdp_m2m_ctx *ctx)
> > +{
> > +	enum vb2_buffer_state vb_state = VB2_BUF_STATE_DONE;
> > +
> > +	mdp_m2m_process_done(ctx, vb_state);
> > +}
> > diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.h b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.h
> > new file mode 100644
> > index 000000000000..61ddbaf1bf13
> > --- /dev/null
> > +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.h
> > @@ -0,0 +1,48 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Ping-Hsun Wu <ping-hsun.wu@mediatek.com>
> > + */
> > +
> > +#ifndef __MTK_MDP3_M2M_H__
> > +#define __MTK_MDP3_M2M_H__
> > +
> > +#include <media/v4l2-ctrls.h>
> > +#include "mtk-mdp3-core.h"
> > +#include "mtk-mdp3-vpu.h"
> > +#include "mtk-mdp3-regs.h"
> > +
> > +#define MDP_MAX_CTRLS	10
> > +
> > +enum {
> > +	MDP_M2M_SRC = 0,
> > +	MDP_M2M_DST = 1,
> > +	MDP_M2M_MAX,
> > +};
> > +
> > +struct mdp_m2m_ctrls {
> > +	struct v4l2_ctrl	*hflip;
> > +	struct v4l2_ctrl	*vflip;
> > +	struct v4l2_ctrl	*rotate;
> > +};
> > +
> > +struct mdp_m2m_ctx {
> > +	u32				id;
> > +	struct mdp_dev			*mdp_dev;
> > +	struct v4l2_fh			fh;
> > +	struct v4l2_ctrl_handler	ctrl_handler;
> > +	struct mdp_m2m_ctrls		ctrls;
> > +	struct v4l2_m2m_ctx		*m2m_ctx;
> > +	struct mdp_vpu_ctx		vpu;
> > +	u32				frame_count[MDP_M2M_MAX];
> > +
> > +	struct mdp_frameparam		curr_param;
> > +	/* synchronization protect for mdp m2m context */
> > +	struct mutex			ctx_lock;
> > +};
> > +
> > +int mdp_m2m_device_register(struct mdp_dev *mdp);
> > +void mdp_m2m_device_unregister(struct mdp_dev *mdp);
> > +void mdp_m2m_job_finish(struct mdp_m2m_ctx *ctx);
> > +
> > +#endif  /* __MTK_MDP3_M2M_H__ */
> > diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-regs.c b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-regs.c
> > new file mode 100644
> > index 000000000000..18874eb7851c
> > --- /dev/null
> > +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-regs.c
> > @@ -0,0 +1,742 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Ping-Hsun Wu <ping-hsun.wu@mediatek.com>
> > + */
> > +
> > +#include <media/v4l2-common.h>
> > +#include <media/videobuf2-v4l2.h>
> > +#include <media/videobuf2-dma-contig.h>
> > +#include "mtk-mdp3-core.h"
> > +#include "mtk-mdp3-regs.h"
> > +#include "mtk-mdp3-m2m.h"
> > +
> > +/*
> > + * All 10-bit related formats are not added in the basic format list,
> > + * please add the corresponding format settings before use.
> > + */
> > +static const struct mdp_format mdp_formats[] = {
> > +	{
> > +		.pixelformat	= V4L2_PIX_FMT_GREY,
> > +		.mdp_color	= MDP_COLOR_GREY,
> > +		.depth		= { 8 },
> > +		.row_depth	= { 8 },
> > +		.num_planes	= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_RGB565X,
> > +		.mdp_color	= MDP_COLOR_BGR565,
> > +		.depth		= { 16 },
> > +		.row_depth	= { 16 },
> > +		.num_planes	= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_RGB565,
> > +		.mdp_color	= MDP_COLOR_RGB565,
> > +		.depth		= { 16 },
> > +		.row_depth	= { 16 },
> > +		.num_planes	= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_RGB24,
> > +		.mdp_color	= MDP_COLOR_RGB888,
> > +		.depth		= { 24 },
> > +		.row_depth	= { 24 },
> > +		.num_planes	= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_BGR24,
> > +		.mdp_color	= MDP_COLOR_BGR888,
> > +		.depth		= { 24 },
> > +		.row_depth	= { 24 },
> > +		.num_planes	= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_ABGR32,
> > +		.mdp_color	= MDP_COLOR_BGRA8888,
> > +		.depth		= { 32 },
> > +		.row_depth	= { 32 },
> > +		.num_planes	= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_ARGB32,
> > +		.mdp_color	= MDP_COLOR_ARGB8888,
> > +		.depth		= { 32 },
> > +		.row_depth	= { 32 },
> > +		.num_planes	= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_UYVY,
> > +		.mdp_color	= MDP_COLOR_UYVY,
> > +		.depth		= { 16 },
> > +		.row_depth	= { 16 },
> > +		.num_planes	= 1,
> > +		.walign		= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_VYUY,
> > +		.mdp_color	= MDP_COLOR_VYUY,
> > +		.depth		= { 16 },
> > +		.row_depth	= { 16 },
> > +		.num_planes	= 1,
> > +		.walign		= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_YUYV,
> > +		.mdp_color	= MDP_COLOR_YUYV,
> > +		.depth		= { 16 },
> > +		.row_depth	= { 16 },
> > +		.num_planes	= 1,
> > +		.walign		= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_YVYU,
> > +		.mdp_color	= MDP_COLOR_YVYU,
> > +		.depth		= { 16 },
> > +		.row_depth	= { 16 },
> > +		.num_planes	= 1,
> > +		.walign		= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_YUV420,
> > +		.mdp_color	= MDP_COLOR_I420,
> > +		.depth		= { 12 },
> > +		.row_depth	= { 8 },
> > +		.num_planes	= 1,
> > +		.walign		= 1,
> > +		.halign		= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_YVU420,
> > +		.mdp_color	= MDP_COLOR_YV12,
> > +		.depth		= { 12 },
> > +		.row_depth	= { 8 },
> > +		.num_planes	= 1,
> > +		.walign		= 1,
> > +		.halign		= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_NV12,
> > +		.mdp_color	= MDP_COLOR_NV12,
> > +		.depth		= { 12 },
> > +		.row_depth	= { 8 },
> > +		.num_planes	= 1,
> > +		.walign		= 1,
> > +		.halign		= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_NV21,
> > +		.mdp_color	= MDP_COLOR_NV21,
> > +		.depth		= { 12 },
> > +		.row_depth	= { 8 },
> > +		.num_planes	= 1,
> > +		.walign		= 1,
> > +		.halign		= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_NV16,
> > +		.mdp_color	= MDP_COLOR_NV16,
> > +		.depth		= { 16 },
> > +		.row_depth	= { 8 },
> > +		.num_planes	= 1,
> > +		.walign		= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_NV61,
> > +		.mdp_color	= MDP_COLOR_NV61,
> > +		.depth		= { 16 },
> > +		.row_depth	= { 8 },
> > +		.num_planes	= 1,
> > +		.walign		= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_NV24,
> > +		.mdp_color	= MDP_COLOR_NV24,
> > +		.depth		= { 24 },
> > +		.row_depth	= { 8 },
> > +		.num_planes	= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_NV42,
> > +		.mdp_color	= MDP_COLOR_NV42,
> > +		.depth		= { 24 },
> > +		.row_depth	= { 8 },
> > +		.num_planes	= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_MT21C,
> > +		.mdp_color	= MDP_COLOR_420_BLK_UFO,
> > +		.depth		= { 8, 4 },
> > +		.row_depth	= { 8, 8 },
> > +		.num_planes	= 2,
> > +		.walign		= 4,
> > +		.halign		= 5,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_MM21,
> > +		.mdp_color	= MDP_COLOR_420_BLK,
> > +		.depth		= { 8, 4 },
> > +		.row_depth	= { 8, 8 },
> > +		.num_planes	= 2,
> > +		.walign		= 4,
> > +		.halign		= 5,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_NV12M,
> > +		.mdp_color	= MDP_COLOR_NV12,
> > +		.depth		= { 8, 4 },
> > +		.row_depth	= { 8, 8 },
> > +		.num_planes	= 2,
> > +		.walign		= 1,
> > +		.halign		= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_NV21M,
> > +		.mdp_color	= MDP_COLOR_NV21,
> > +		.depth		= { 8, 4 },
> > +		.row_depth	= { 8, 8 },
> > +		.num_planes	= 2,
> > +		.walign		= 1,
> > +		.halign		= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_NV16M,
> > +		.mdp_color	= MDP_COLOR_NV16,
> > +		.depth		= { 8, 8 },
> > +		.row_depth	= { 8, 8 },
> > +		.num_planes	= 2,
> > +		.walign		= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_NV61M,
> > +		.mdp_color	= MDP_COLOR_NV61,
> > +		.depth		= { 8, 8 },
> > +		.row_depth	= { 8, 8 },
> > +		.num_planes	= 2,
> > +		.walign		= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_YUV420M,
> > +		.mdp_color	= MDP_COLOR_I420,
> > +		.depth		= { 8, 2, 2 },
> > +		.row_depth	= { 8, 4, 4 },
> > +		.num_planes	= 3,
> > +		.walign		= 1,
> > +		.halign		= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}, {
> > +		.pixelformat	= V4L2_PIX_FMT_YVU420M,
> > +		.mdp_color	= MDP_COLOR_YV12,
> > +		.depth		= { 8, 2, 2 },
> > +		.row_depth	= { 8, 4, 4 },
> > +		.num_planes	= 3,
> > +		.walign		= 1,
> > +		.halign		= 1,
> > +		.flags		= MDP_FMT_FLAG_OUTPUT | MDP_FMT_FLAG_CAPTURE,
> > +	}
> > +};
> > +
> > +static const struct mdp_limit mdp_def_limit = {
> > +	.out_limit = {
> > +		.wmin	= 16,
> > +		.hmin	= 16,
> > +		.wmax	= 8176,
> > +		.hmax	= 8176,
> > +	},
> > +	.cap_limit = {
> > +		.wmin	= 2,
> > +		.hmin	= 2,
> > +		.wmax	= 8176,
> > +		.hmax	= 8176,
> > +	},
> > +	.h_scale_up_max = 32,
> > +	.v_scale_up_max = 32,
> > +	.h_scale_down_max = 20,
> > +	.v_scale_down_max = 128,
> > +};
> > +
> > +static const struct mdp_format *mdp_find_fmt(u32 pixelformat, u32 type)
> > +{
> > +	u32 i, flag;
> > +
> > +	flag = V4L2_TYPE_IS_OUTPUT(type) ? MDP_FMT_FLAG_OUTPUT :
> > +					MDP_FMT_FLAG_CAPTURE;
> > +	for (i = 0; i < ARRAY_SIZE(mdp_formats); ++i) {
> > +		if (!(mdp_formats[i].flags & flag))
> > +			continue;
> > +		if (mdp_formats[i].pixelformat == pixelformat)
> > +			return &mdp_formats[i];
> > +	}
> > +	return NULL;
> > +}
> > +
> > +static const struct mdp_format *mdp_find_fmt_by_index(u32 index, u32 type)
> > +{
> > +	u32 i, flag, num = 0;
> > +
> > +	flag = V4L2_TYPE_IS_OUTPUT(type) ? MDP_FMT_FLAG_OUTPUT :
> > +					MDP_FMT_FLAG_CAPTURE;
> > +	for (i = 0; i < ARRAY_SIZE(mdp_formats); ++i) {
> > +		if (!(mdp_formats[i].flags & flag))
> > +			continue;
> > +		if (index == num)
> > +			return &mdp_formats[i];
> > +		num++;
> > +	}
> > +	return NULL;
> > +}
> > +
> > +enum mdp_ycbcr_profile mdp_map_ycbcr_prof_mplane(struct v4l2_format *f,
> > +						 u32 mdp_color)
> > +{
> > +	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > +
> > +	if (MDP_COLOR_IS_RGB(mdp_color))
> > +		return MDP_YCBCR_PROFILE_FULL_BT601;
> > +
> > +	switch (pix_mp->colorspace) {
> > +	case V4L2_COLORSPACE_JPEG:
> > +		return MDP_YCBCR_PROFILE_JPEG;
> > +	case V4L2_COLORSPACE_REC709:
> > +	case V4L2_COLORSPACE_DCI_P3:
> > +		if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> > +			return MDP_YCBCR_PROFILE_FULL_BT709;
> > +		return MDP_YCBCR_PROFILE_BT709;
> > +	case V4L2_COLORSPACE_BT2020:
> > +		if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> > +			return MDP_YCBCR_PROFILE_FULL_BT2020;
> > +		return MDP_YCBCR_PROFILE_BT2020;
> > +	default:
> > +		if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> > +			return MDP_YCBCR_PROFILE_FULL_BT601;
> > +		return MDP_YCBCR_PROFILE_BT601;
> > +	}
> > +}
> > +
> > +static void mdp_bound_align_image(u32 *w, u32 *h,
> > +				  struct v4l2_frmsize_stepwise *s,
> > +				  unsigned int salign)
> > +{
> > +	unsigned int org_w, org_h;
> > +
> > +	org_w = *w;
> > +	org_h = *h;
> > +	v4l_bound_align_image(w, s->min_width, s->max_width, s->step_width,
> > +			      h, s->min_height, s->max_height, s->step_height,
> > +			      salign);
> > +
> > +	s->min_width = org_w;
> > +	s->min_height = org_h;
> > +	v4l2_apply_frmsize_constraints(w, h, s);
> > +}
> > +
> > +static int mdp_clamp_align(s32 *x, int min, int max, unsigned int align)
> > +{
> > +	unsigned int mask;
> > +
> > +	if (min < 0 || max < 0)
> > +		return -ERANGE;
> > +
> > +	/* Bits that must be zero to be aligned */
> > +	mask = ~((1 << align) - 1);
> > +
> > +	min = 0 ? 0 : ((min + ~mask) & mask);
> > +	max = max & mask;
> > +	if ((unsigned int)min > (unsigned int)max)
> > +		return -ERANGE;
> > +
> > +	/* Clamp to aligned min and max */
> > +	*x = clamp(*x, min, max);
> > +
> > +	/* Round to nearest aligned value */
> > +	if (align)
> > +		*x = (*x + (1 << (align - 1))) & mask;
> > +	return 0;
> > +}
> > +
> > +int mdp_enum_fmt_mplane(struct v4l2_fmtdesc *f)
> > +{
> > +	const struct mdp_format *fmt;
> > +
> > +	fmt = mdp_find_fmt_by_index(f->index, f->type);
> > +	if (!fmt)
> > +		return -EINVAL;
> > +
> > +	f->pixelformat = fmt->pixelformat;
> > +	return 0;
> > +}
> > +
> > +const struct mdp_format *mdp_try_fmt_mplane(struct v4l2_format *f,
> > +					    struct mdp_frameparam *param,
> > +					    u32 ctx_id)
> > +{
> > +	struct device *dev = &param->ctx->mdp_dev->pdev->dev;
> > +	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > +	const struct mdp_format *fmt;
> > +	const struct mdp_pix_limit *pix_limit;
> > +	struct v4l2_frmsize_stepwise s;
> > +	u32 org_w, org_h;
> > +	unsigned int i;
> > +
> > +	fmt = mdp_find_fmt(pix_mp->pixelformat, f->type);
> > +	if (!fmt) {
> > +		fmt = mdp_find_fmt_by_index(0, f->type);
> > +		if (!fmt) {
> > +			dev_dbg(dev, "%d: pixelformat %c%c%c%c invalid", ctx_id,
> > +				(pix_mp->pixelformat & 0xff),
> > +				(pix_mp->pixelformat >>  8) & 0xff,
> > +				(pix_mp->pixelformat >> 16) & 0xff,
> > +				(pix_mp->pixelformat >> 24) & 0xff);
> > +			return NULL;
> > +		}
> > +	}
> > +
> > +	pix_mp->field = V4L2_FIELD_NONE;
> > +	pix_mp->flags = 0;
> > +	pix_mp->pixelformat = fmt->pixelformat;
> > +	if (!V4L2_TYPE_IS_OUTPUT(f->type)) {
> > +		pix_mp->colorspace = param->colorspace;
> > +		pix_mp->xfer_func = param->xfer_func;
> > +		pix_mp->ycbcr_enc = param->ycbcr_enc;
> > +		pix_mp->quantization = param->quant;
> > +	}
> > +
> > +	pix_limit = V4L2_TYPE_IS_OUTPUT(f->type) ? &param->limit->out_limit :
> > +						&param->limit->cap_limit;
> > +	s.min_width = pix_limit->wmin;
> > +	s.max_width = pix_limit->wmax;
> > +	s.step_width = fmt->walign;
> > +	s.min_height = pix_limit->hmin;
> > +	s.max_height = pix_limit->hmax;
> > +	s.step_height = fmt->halign;
> > +	org_w = pix_mp->width;
> > +	org_h = pix_mp->height;
> > +
> > +	mdp_bound_align_image(&pix_mp->width, &pix_mp->height, &s, fmt->salign);
> > +	if (org_w != pix_mp->width || org_h != pix_mp->height)
> > +		dev_dbg(dev, "%d: size change: %ux%u to %ux%u", ctx_id,
> > +			org_w, org_h, pix_mp->width, pix_mp->height);
> > +
> > +	if (pix_mp->num_planes && pix_mp->num_planes != fmt->num_planes)
> > +		dev_dbg(dev, "%d num of planes change: %u to %u", ctx_id,
> > +			pix_mp->num_planes, fmt->num_planes);
> > +	pix_mp->num_planes = fmt->num_planes;
> > +
> > +	for (i = 0; i < pix_mp->num_planes; ++i) {
> > +		u32 min_bpl = (pix_mp->width * fmt->row_depth[i]) >> 3;
> > +		u32 max_bpl = (pix_limit->wmax * fmt->row_depth[i]) >> 3;
> > +		u32 bpl = pix_mp->plane_fmt[i].bytesperline;
> > +		u32 min_si, max_si;
> > +		u32 si = pix_mp->plane_fmt[i].sizeimage;
> > +
> > +		bpl = clamp(bpl, min_bpl, max_bpl);
> > +		pix_mp->plane_fmt[i].bytesperline = bpl;
> > +
> > +		min_si = (bpl * pix_mp->height * fmt->depth[i]) / fmt->row_depth[i];
> > +		max_si = (bpl * s.max_height * fmt->depth[i]) / fmt->row_depth[i];
> > +
> > +		si = clamp(si, min_si, max_si);
> > +		pix_mp->plane_fmt[i].sizeimage = si;
> > +
> > +		dev_dbg(dev, "%d: p%u, bpl:%u [%u, %u], sizeimage:%u [%u, %u]", ctx_id,
> > +			i, bpl, min_bpl, max_bpl, si, min_si, max_si);
> > +	}
> > +
> > +	return fmt;
> > +}
> > +
> > +static int mdp_clamp_start(s32 *x, int min, int max, unsigned int align,
> > +			   u32 flags)
> > +{
> > +	if (flags & V4L2_SEL_FLAG_GE)
> > +		max = *x;
> > +	if (flags & V4L2_SEL_FLAG_LE)
> > +		min = *x;
> > +	return mdp_clamp_align(x, min, max, align);
> > +}
> > +
> > +static int mdp_clamp_end(s32 *x, int min, int max, unsigned int align,
> > +			 u32 flags)
> > +{
> > +	if (flags & V4L2_SEL_FLAG_GE)
> > +		min = *x;
> > +	if (flags & V4L2_SEL_FLAG_LE)
> > +		max = *x;
> > +	return mdp_clamp_align(x, min, max, align);
> > +}
> > +
> > +int mdp_try_crop(struct mdp_m2m_ctx *ctx, struct v4l2_rect *r,
> > +		 const struct v4l2_selection *s, struct mdp_frame *frame)
> > +{
> > +	struct device *dev = &ctx->mdp_dev->pdev->dev;
> > +	s32 left, top, right, bottom;
> > +	u32 framew, frameh, walign, halign;
> > +	int ret;
> > +
> > +	dev_dbg(dev, "%d target:%d, set:(%d,%d) %ux%u", ctx->id,
> > +		s->target, s->r.left, s->r.top, s->r.width, s->r.height);
> > +
> > +	left = s->r.left;
> > +	top = s->r.top;
> > +	right = s->r.left + s->r.width;
> > +	bottom = s->r.top + s->r.height;
> > +	framew = frame->format.fmt.pix_mp.width;
> > +	frameh = frame->format.fmt.pix_mp.height;
> > +
> > +	if (mdp_target_is_crop(s->target)) {
> > +		walign = 1;
> > +		halign = 1;
> > +	} else {
> > +		walign = frame->mdp_fmt->walign;
> > +		halign = frame->mdp_fmt->halign;
> > +	}
> > +
> > +	dev_dbg(dev, "%d align:%u,%u, bound:%ux%u", ctx->id,
> > +		walign, halign, framew, frameh);
> > +
> > +	ret = mdp_clamp_start(&left, 0, right, walign, s->flags);
> > +	if (ret)
> > +		return ret;
> > +	ret = mdp_clamp_start(&top, 0, bottom, halign, s->flags);
> > +	if (ret)
> > +		return ret;
> > +	ret = mdp_clamp_end(&right, left, framew, walign, s->flags);
> > +	if (ret)
> > +		return ret;
> > +	ret = mdp_clamp_end(&bottom, top, frameh, halign, s->flags);
> > +	if (ret)
> > +		return ret;
> > +
> > +	r->left = left;
> > +	r->top = top;
> > +	r->width = right - left;
> > +	r->height = bottom - top;
> > +
> > +	dev_dbg(dev, "%d crop:(%d,%d) %ux%u", ctx->id,
> > +		r->left, r->top, r->width, r->height);
> > +	return 0;
> > +}
> > +
> > +int mdp_check_scaling_ratio(const struct v4l2_rect *crop,
> > +			    const struct v4l2_rect *compose, s32 rotation,
> > +	const struct mdp_limit *limit)
> > +{
> > +	u32 crop_w, crop_h, comp_w, comp_h;
> > +
> > +	crop_w = crop->width;
> > +	crop_h = crop->height;
> > +	if (90 == rotation || 270 == rotation) {
> > +		comp_w = compose->height;
> > +		comp_h = compose->width;
> > +	} else {
> > +		comp_w = compose->width;
> > +		comp_h = compose->height;
> > +	}
> > +
> > +	if ((crop_w / comp_w) > limit->h_scale_down_max ||
> > +	    (crop_h / comp_h) > limit->v_scale_down_max ||
> > +	    (comp_w / crop_w) > limit->h_scale_up_max ||
> > +	    (comp_h / crop_h) > limit->v_scale_up_max)
> > +		return -ERANGE;
> > +	return 0;
> > +}
> > +
> > +/* Stride that is accepted by MDP HW */
> > +static u32 mdp_fmt_get_stride(const struct mdp_format *fmt,
> > +			      u32 bytesperline, unsigned int plane)
> > +{
> > +	enum mdp_color c = fmt->mdp_color;
> > +	u32 stride;
> > +
> > +	stride = (bytesperline * MDP_COLOR_BITS_PER_PIXEL(c))
> > +		/ fmt->row_depth[0];
> > +	if (plane == 0)
> > +		return stride;
> > +	if (plane < MDP_COLOR_GET_PLANE_COUNT(c)) {
> > +		if (MDP_COLOR_IS_BLOCK_MODE(c))
> > +			stride = stride / 2;
> > +		return stride;
> > +	}
> > +	return 0;
> > +}
> > +
> > +/* Stride that is accepted by MDP HW of format with contiguous planes */
> > +static u32 mdp_fmt_get_stride_contig(const struct mdp_format *fmt,
> > +				     u32 pix_stride, unsigned int plane)
> > +{
> > +	enum mdp_color c = fmt->mdp_color;
> > +	u32 stride = pix_stride;
> > +
> > +	if (plane == 0)
> > +		return stride;
> > +	if (plane < MDP_COLOR_GET_PLANE_COUNT(c)) {
> > +		stride = stride >> MDP_COLOR_GET_H_SUBSAMPLE(c);
> > +		if (MDP_COLOR_IS_UV_COPLANE(c) && !MDP_COLOR_IS_BLOCK_MODE(c))
> > +			stride = stride * 2;
> > +		return stride;
> > +	}
> > +	return 0;
> > +}
> > +
> > +/* Plane size that is accepted by MDP HW */
> > +static u32 mdp_fmt_get_plane_size(const struct mdp_format *fmt,
> > +				  u32 stride, u32 height, unsigned int plane)
> > +{
> > +	enum mdp_color c = fmt->mdp_color;
> > +	u32 bytesperline;
> > +
> > +	bytesperline = (stride * fmt->row_depth[0])
> > +		/ MDP_COLOR_BITS_PER_PIXEL(c);
> > +	if (plane == 0)
> > +		return bytesperline * height;
> > +	if (plane < MDP_COLOR_GET_PLANE_COUNT(c)) {
> > +		height = height >> MDP_COLOR_GET_V_SUBSAMPLE(c);
> > +		if (MDP_COLOR_IS_BLOCK_MODE(c))
> > +			bytesperline = bytesperline * 2;
> > +		return bytesperline * height;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static void mdp_prepare_buffer(struct img_image_buffer *b,
> > +			       struct mdp_frame *frame, struct vb2_buffer *vb)
> > +{
> > +	struct v4l2_pix_format_mplane *pix_mp = &frame->format.fmt.pix_mp;
> > +	unsigned int i;
> > +
> > +	b->format.colorformat = frame->mdp_fmt->mdp_color;
> > +	b->format.ycbcr_prof = frame->ycbcr_prof;
> > +	for (i = 0; i < pix_mp->num_planes; ++i) {
> > +		u32 stride = mdp_fmt_get_stride(frame->mdp_fmt,
> > +			pix_mp->plane_fmt[i].bytesperline, i);
> > +
> > +		b->format.plane_fmt[i].stride = stride;
> > +		/*
> > +		 * TODO : The way to pass an offset within a DMA-buf
> > +		 * is not defined in V4L2 specification, so we abuse
> > +		 * data_offset for now. Fix it when we have the right interface,
> > +		 * including any necessary validation and potential alignment
> > +		 * issues.
> > +		 */
> > +		b->format.plane_fmt[i].size =
> > +			mdp_fmt_get_plane_size(frame->mdp_fmt, stride,
> > +					       pix_mp->height, i) -
> > +					       vb->planes[i].data_offset;
> > +		b->iova[i] = vb2_dma_contig_plane_dma_addr(vb, i) +
> > +			     vb->planes[i].data_offset;
> 
> Why do you need data_offset? What is the use case? Obviously I can't
> merge a driver that abuses an API.
> 
> > +	}
> > +	for (; i < MDP_COLOR_GET_PLANE_COUNT(b->format.colorformat); ++i) {
> > +		u32 stride = mdp_fmt_get_stride_contig(frame->mdp_fmt,
> > +			b->format.plane_fmt[0].stride, i);
> > +
> > +		b->format.plane_fmt[i].stride = stride;
> > +		b->format.plane_fmt[i].size =
> > +			mdp_fmt_get_plane_size(frame->mdp_fmt, stride,
> > +					       pix_mp->height, i);
> > +		b->iova[i] = b->iova[i - 1] + b->format.plane_fmt[i - 1].size;
> > +	}
> > +	b->usage = frame->usage;
> > +}
> > +
> > +void mdp_set_src_config(struct img_input *in,
> > +			struct mdp_frame *frame, struct vb2_buffer *vb)
> > +{
> > +	in->buffer.format.width = frame->format.fmt.pix_mp.width;
> > +	in->buffer.format.height = frame->format.fmt.pix_mp.height;
> > +	mdp_prepare_buffer(&in->buffer, frame, vb);
> > +}
> > +
> > +static u32 mdp_to_fixed(u32 *r, struct v4l2_fract *f)
> > +{
> > +	u32 q;
> > +
> > +	if (f->denominator == 0) {
> > +		*r = 0;
> > +		return 0;
> > +	}
> > +
> > +	q = f->numerator / f->denominator;
> > +	*r = div_u64(((u64)f->numerator - q * f->denominator) <<
> > +		     IMG_SUBPIXEL_SHIFT, f->denominator);
> > +	return q;
> > +}
> > +
> > +static void mdp_set_src_crop(struct img_crop *c, struct mdp_crop *crop)
> > +{
> > +	c->left = crop->c.left
> > +		+ mdp_to_fixed(&c->left_subpix, &crop->left_subpix);
> > +	c->top = crop->c.top
> > +		+ mdp_to_fixed(&c->top_subpix, &crop->top_subpix);
> > +	c->width = crop->c.width
> > +		+ mdp_to_fixed(&c->width_subpix, &crop->width_subpix);
> > +	c->height = crop->c.height
> > +		+ mdp_to_fixed(&c->height_subpix, &crop->height_subpix);
> > +}
> > +
> > +static void mdp_set_orientation(struct img_output *out,
> > +				s32 rotation, bool hflip, bool vflip)
> > +{
> > +	u8 flip = 0;
> > +
> > +	if (hflip)
> > +		flip ^= 1;
> > +	if (vflip) {
> > +		/*
> > +		 * A vertical flip is equivalent to
> > +		 * a 180-degree rotation with a horizontal flip
> > +		 */
> > +		rotation += 180;
> > +		flip ^= 1;
> > +	}
> > +
> > +	out->rotation = rotation % 360;
> > +	if (flip != 0)
> > +		out->flags |= IMG_CTRL_FLAG_HFLIP;
> > +	else
> > +		out->flags &= ~IMG_CTRL_FLAG_HFLIP;
> > +}
> > +
> > +void mdp_set_dst_config(struct img_output *out,
> > +			struct mdp_frame *frame, struct vb2_buffer *vb)
> > +{
> > +	out->buffer.format.width = frame->compose.width;
> > +	out->buffer.format.height = frame->compose.height;
> > +	mdp_prepare_buffer(&out->buffer, frame, vb);
> > +	mdp_set_src_crop(&out->crop, &frame->crop);
> > +	mdp_set_orientation(out, frame->rotation, frame->hflip, frame->vflip);
> > +}
> > +
> > +int mdp_frameparam_init(struct mdp_frameparam *param)
> > +{
> > +	struct mdp_frame *frame;
> > +
> > +	if (!param)
> > +		return -EINVAL;
> > +
> > +	INIT_LIST_HEAD(&param->list);
> > +	param->limit = &mdp_def_limit;
> > +	param->type = MDP_STREAM_TYPE_BITBLT;
> > +
> > +	frame = &param->output;
> > +	frame->format.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> > +	frame->mdp_fmt = mdp_try_fmt_mplane(&frame->format, param, 0);
> > +	frame->ycbcr_prof =
> > +		mdp_map_ycbcr_prof_mplane(&frame->format,
> > +					  frame->mdp_fmt->mdp_color);
> > +	frame->usage = MDP_BUFFER_USAGE_HW_READ;
> > +
> > +	param->num_captures = 1;
> > +	frame = &param->captures[0];
> > +	frame->format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> > +	frame->mdp_fmt = mdp_try_fmt_mplane(&frame->format, param, 0);
> > +	frame->ycbcr_prof =
> > +		mdp_map_ycbcr_prof_mplane(&frame->format,
> > +					  frame->mdp_fmt->mdp_color);
> > +	frame->usage = MDP_BUFFER_USAGE_MDP;
> > +	frame->crop.c.width = param->output.format.fmt.pix_mp.width;
> > +	frame->crop.c.height = param->output.format.fmt.pix_mp.height;
> > +	frame->compose.width = frame->format.fmt.pix_mp.width;
> > +	frame->compose.height = frame->format.fmt.pix_mp.height;
> > +
> > +	return 0;
> > +}
Moudy Ho (何宗原) July 11, 2022, 8:11 a.m. UTC | #3
Hi Hans,

Thanks for your review, and appreciate for all comments and
suggestions.
I'll go through each "dev_info" one by one and replace the unnecessary
with "dev_dbg" or just remove it.
In addition, I will adjust other inappropriate settings as suggested.

On Fri, 2022-07-08 at 10:08 +0200, Hans Verkuil wrote:
> Hi Moudy,
> 
> Some comments below:
> 
> On 7/6/22 09:54, Moudy Ho wrote:
> > This patch adds driver for Mediatek's Media Data Path ver.3 (MDP3).
> > It provides the following functions:
> >   color transform, format conversion, resize, crop, rotate, flip
> >   and additional image quality enhancement.
> > 
> > The MDP3 driver is mainly used for Google Chromebook products to
> > import the new architecture to set the HW settings as shown below:
> >   User -> V4L2 framework
> >     -> MDP3 driver -> SCP (setting calculations)
> >       -> MDP3 driver -> CMDQ (GCE driver) -> HW
> > 
> > Each modules' related operation control is sited in mtk-mdp3-comp.c
> > Each modules' register table is defined in file with "mdp_reg_"
> > prefix
> > GCE related API, operation control  sited in mtk-mdp3-cmdq.c
> > V4L2 m2m device functions are implemented in mtk-mdp3-m2m.c
> > Probe, power, suspend/resume, system level functions are defined in
> > mtk-mdp3-core.c
> > 
> > v4l2-compliance 1.22.1, 32 bits, 32-bit time_t
> > 
> > Compliance test for mtk-mdp3 device /dev/video2:
> > 
> > Driver Info:
> > 	Driver name      : mtk-mdp3
> > 	Card type        : 14001000.mdp3-rdma0
> 
> Card type is expected to be a human readable name, and that's not the
> case
> here.
> 
> > 	Bus info         : platform:mtk-mdp3
> 
> <snip>

[snip]

> > diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-regs.c
> > b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-regs.c
> > new file mode 100644
> > index 000000000000..18874eb7851c
> > --- /dev/null
> > +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-regs.c

[snip]

> > +/* Plane size that is accepted by MDP HW */
> > +static u32 mdp_fmt_get_plane_size(const struct mdp_format *fmt,
> > +				  u32 stride, u32 height, unsigned int
> > plane)
> > +{
> > +	enum mdp_color c = fmt->mdp_color;
> > +	u32 bytesperline;
> > +
> > +	bytesperline = (stride * fmt->row_depth[0])
> > +		/ MDP_COLOR_BITS_PER_PIXEL(c);
> > +	if (plane == 0)
> > +		return bytesperline * height;
> > +	if (plane < MDP_COLOR_GET_PLANE_COUNT(c)) {
> > +		height = height >> MDP_COLOR_GET_V_SUBSAMPLE(c);
> > +		if (MDP_COLOR_IS_BLOCK_MODE(c))
> > +			bytesperline = bytesperline * 2;
> > +		return bytesperline * height;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static void mdp_prepare_buffer(struct img_image_buffer *b,
> > +			       struct mdp_frame *frame, struct
> > vb2_buffer *vb)
> > +{
> > +	struct v4l2_pix_format_mplane *pix_mp = &frame-
> > >format.fmt.pix_mp;
> > +	unsigned int i;
> > +
> > +	b->format.colorformat = frame->mdp_fmt->mdp_color;
> > +	b->format.ycbcr_prof = frame->ycbcr_prof;
> > +	for (i = 0; i < pix_mp->num_planes; ++i) {
> > +		u32 stride = mdp_fmt_get_stride(frame->mdp_fmt,
> > +			pix_mp->plane_fmt[i].bytesperline, i);
> > +
> > +		b->format.plane_fmt[i].stride = stride;
> > +		/*
> > +		 * TODO : The way to pass an offset within a DMA-buf
> > +		 * is not defined in V4L2 specification, so we abuse
> > +		 * data_offset for now. Fix it when we have the right
> > interface,
> > +		 * including any necessary validation and potential
> > alignment
> > +		 * issues.
> > +		 */
> > +		b->format.plane_fmt[i].size =
> > +			mdp_fmt_get_plane_size(frame->mdp_fmt, stride,
> > +					       pix_mp->height, i) -
> > +					       vb-
> > >planes[i].data_offset;
> > +		b->iova[i] = vb2_dma_contig_plane_dma_addr(vb, i) +
> > +			     vb->planes[i].data_offset;
> 
> Why do you need data_offset? What is the use case? Obviously I can't
> merge a driver that abuses an API.
> 

Apologize for not incorporating the previously discussed results into
this version due to my carelessness, I will correct it in the next
version.

https://patchwork.kernel.org/project/linux-mediatek/patch/20210623091457.18002-1-moudy.ho@mediatek.com/

> > +	}
> > +	for (; i < MDP_COLOR_GET_PLANE_COUNT(b->format.colorformat);
> > ++i) {
> > +		u32 stride = mdp_fmt_get_stride_contig(frame->mdp_fmt,
> > +			b->format.plane_fmt[0].stride, i);
> > +
> > +		b->format.plane_fmt[i].stride = stride;
> > +		b->format.plane_fmt[i].size =
> > +			mdp_fmt_get_plane_size(frame->mdp_fmt, stride,
> > +					       pix_mp->height, i);
> > +		b->iova[i] = b->iova[i - 1] + b->format.plane_fmt[i -
> > 1].size;
> > +	}
> > +	b->usage = frame->usage;
> > +}
> > +
> > +void mdp_set_src_config(struct img_input *in,
> > +			struct mdp_frame *frame, struct vb2_buffer *vb)
> > +{
> > +	in->buffer.format.width = frame->format.fmt.pix_mp.width;
> > +	in->buffer.format.height = frame->format.fmt.pix_mp.height;
> > +	mdp_prepare_buffer(&in->buffer, frame, vb);
> > +}

[snip]

> 
> Regards,
> 
> 	Hans

Many thanks,
Moudy
Moudy Ho (何宗原) July 11, 2022, 8:25 a.m. UTC | #4
On Fri, 2022-07-08 at 11:20 +0300, Laurent Pinchart wrote:
> On Fri, Jul 08, 2022 at 10:08:44AM +0200, Hans Verkuil wrote:
> > Hi Moudy,
> > 
> > Some comments below:
> 
> And one more
> 
> > On 7/6/22 09:54, Moudy Ho wrote:
> > > This patch adds driver for Mediatek's Media Data Path ver.3
> > > (MDP3).
> > > It provides the following functions:
> > >   color transform, format conversion, resize, crop, rotate, flip
> > >   and additional image quality enhancement.
> > > 
> > > The MDP3 driver is mainly used for Google Chromebook products to
> > > import the new architecture to set the HW settings as shown
> > > below:
> > >   User -> V4L2 framework
> > >     -> MDP3 driver -> SCP (setting calculations)
> > >       -> MDP3 driver -> CMDQ (GCE driver) -> HW
> > > 
> > > Each modules' related operation control is sited in mtk-mdp3-
> > > comp.c
> > > Each modules' register table is defined in file with "mdp_reg_"
> > > prefix
> > > GCE related API, operation control  sited in mtk-mdp3-cmdq.c
> > > V4L2 m2m device functions are implemented in mtk-mdp3-m2m.c
> > > Probe, power, suspend/resume, system level functions are defined
> > > in
> > > mtk-mdp3-core.c
> > > 
> > > v4l2-compliance 1.22.1, 32 bits, 32-bit time_t
> > > 
> > > Compliance test for mtk-mdp3 device /dev/video2:
> > > 
> > > Driver Info:
> > > 	Driver name      : mtk-mdp3
> > > 	Card type        : 14001000.mdp3-rdma0
> > 
> > Card type is expected to be a human readable name, and that's not
> > the case
> > here.
> > 
> > > 	Bus info         : platform:mtk-mdp3
> > 
> > <snip>
> > 
> > > diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.c
> > > b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.c
> > > new file mode 100644

[snip]

> > > +static const struct vb2_ops mdp_m2m_qops = {
> > > +	.queue_setup	= mdp_m2m_queue_setup,
> > > +	.wait_prepare	= vb2_ops_wait_prepare,
> > > +	.wait_finish	= vb2_ops_wait_finish,
> > > +	.buf_prepare	= mdp_m2m_buf_prepare,
> > > +	.start_streaming = mdp_m2m_start_streaming,
> > > +	.stop_streaming	= mdp_m2m_stop_streaming,
> > > +	.buf_queue	= mdp_m2m_buf_queue,
> > > +	.buf_out_validate = mdp_m2m_buf_out_validate,
> > > +};
> > > +
> > > +static int mdp_m2m_querycap(struct file *file, void *fh,
> > > +			    struct v4l2_capability *cap)
> > > +{
> > > +	struct mdp_m2m_ctx *ctx = fh_to_ctx(fh);
> > > +
> > > +	strscpy(cap->driver, MDP_MODULE_NAME, sizeof(cap->driver));
> > > +	strscpy(cap->card, ctx->mdp_dev->pdev->name, sizeof(cap-
> > > >card));
> > 
> > As mentioned at the top, this is not a suitable name for cap->card.
> > 
> > > +	strscpy(cap->bus_info, "platform:mtk-mdp3", sizeof(cap-
> > > >bus_info));
> 
> And don't override bus_info, the value isn't right. The V4L2 core
> should
> do the right thing for platform devices now.
> 

Hi Laurent,

Thanks for the reminder, I'll fix it in the next version along with
what Hans mentioned earlier.

Regards,
Moudy

> > > +	return 0;
> > > +}
> > > +
> > > +static int mdp_m2m_enum_fmt_mplane(struct file *file, void *fh,
> > > +				   struct v4l2_fmtdesc *f)
> > > +{
> > > +	return mdp_enum_fmt_mplane(f);
> > > +}