| Message ID | 20190417104511.21514-1-frederic.chen@mediatek.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC | expand |
Hi Frederic, Most reviews are already covered by Tomasz, here are some small missing pieces. Please see my comments inline. On Wed, Apr 17, 2019 at 06:45:11PM +0800, Frederic Chen wrote: > This patch adds the driver of Digital Image Processing (DIP) > unit in Mediatek ISP system, providing image format > conversion, resizing, and rotation features. > > The mtk-isp directory will contain drivers for multiple IP > blocks found in Mediatek ISP system. It will include ISP > Pass 1 driver(CAM), sensor interface driver, DIP driver and > face detection driver. > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> > --- > drivers/media/platform/mtk-isp/Makefile | 18 + > .../media/platform/mtk-isp/isp_50/Makefile | 17 + > .../platform/mtk-isp/isp_50/dip/Makefile | 32 + > .../mtk-isp/isp_50/dip/mtk_dip-ctrl.c | 124 ++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.c | 1116 +++++++++++++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.h | 321 ++++ > .../platform/mtk-isp/isp_50/dip/mtk_dip-hw.h | 167 ++ > .../mtk-isp/isp_50/dip/mtk_dip-smem.c | 322 ++++ > .../mtk-isp/isp_50/dip/mtk_dip-smem.h | 39 + > .../platform/mtk-isp/isp_50/dip/mtk_dip-sys.c | 1384 +++++++++++++++++ > .../mtk-isp/isp_50/dip/mtk_dip-v4l2.c | 1310 ++++++++++++++++ > 11 files changed, 4850 insertions(+) > create mode 100644 drivers/media/platform/mtk-isp/Makefile > create mode 100644 drivers/media/platform/mtk-isp/isp_50/Makefile > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/Makefile > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-hw.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.h > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c > > diff --git a/drivers/media/platform/mtk-isp/Makefile b/drivers/media/platform/mtk-isp/Makefile > new file mode 100644 > index 000000000000..24bc5354e2f6 > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/Makefile > @@ -0,0 +1,18 @@ > +# > +# Copyright (C) 2018 MediaTek Inc. > +# > +# This program is free software: you can redistribute it and/or modify > +# it under the terms of the GNU General Public License version 2 as > +# published by the Free Software Foundation. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > + > +obj-$(CONFIG_VIDEO_MEDIATEK_ISP_COMMON) += common/ > + > +obj-y += isp_50/ > + > +obj-$(CONFIG_VIDEO_MEDIATEK_ISP_FD_SUPPORT) += fd/ > diff --git a/drivers/media/platform/mtk-isp/isp_50/Makefile b/drivers/media/platform/mtk-isp/isp_50/Makefile > new file mode 100644 > index 000000000000..fd0e5bd3c781 > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/Makefile > @@ -0,0 +1,17 @@ > +# > +# Copyright (C) 2018 MediaTek Inc. > +# > +# This program is free software: you can redistribute it and/or modify > +# it under the terms of the GNU General Public License version 2 as > +# published by the Free Software Foundation. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > + > +ifeq ($(CONFIG_VIDEO_MEDIATEK_ISP_DIP_SUPPORT),y) > +obj-y += dip/ > +endif > + > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/Makefile b/drivers/media/platform/mtk-isp/isp_50/dip/Makefile > new file mode 100644 > index 000000000000..03137416857b > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/Makefile > @@ -0,0 +1,32 @@ > +# > +# Copyright (C) 2018 MediaTek Inc. > +# > +# This program is free software: you can redistribute it and/or modify > +# it under the terms of the GNU General Public License version 2 as > +# published by the Free Software Foundation. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +$(info $(srctree)) > +ccflags-y += -I$(srctree)/drivers/media/platform/mtk-mdp3 > + > +obj-y += mtk_dip-sys.o > + > +# To provide alloc context managing memory shared > +# between CPU and ISP coprocessor > +mtk_dip_smem-objs := \ > +mtk_dip-smem.o > + > +obj-y += mtk_dip_smem.o > + > +# Utilits to provide frame-based streaming model typo? should be Utilities. > +# with v4l2 user interfaces > +mtk_dip_util-objs := \ > +mtk_dip-dev.o \ > +mtk_dip-v4l2.o \ > +mtk_dip-ctrl.o \ > + > +obj-y += mtk_dip_util.o > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c > new file mode 100644 > index 000000000000..e35574818120 > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c > @@ -0,0 +1,124 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018 MediaTek Inc. > + * Author: Frederic Chen <frederic.chen@mediatek.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/device.h> > +#include <linux/platform_device.h> > +#include "mtk_dip-dev.h" > + > +static void handle_buf_usage_config(struct v4l2_ctrl *ctrl) > +{ > + struct mtk_dip_video_device *node = > + container_of(ctrl->handler, > + struct mtk_dip_video_device, ctrl_handler); > + > + if (ctrl->val < MTK_DIP_V4l2_BUF_USAGE_DEFAULT || > + ctrl->val >= MTK_DIP_V4l2_BUF_USAGE_NONE) { > + pr_err("Invalid buffer usage id %d", ctrl->val); > + return; > + } > + node->dev_q.buffer_usage = ctrl->val; > +} > + > +static void handle_buf_rotate_config(struct v4l2_ctrl *ctrl) > +{ > + struct mtk_dip_video_device *node = > + container_of(ctrl->handler, > + struct mtk_dip_video_device, ctrl_handler); > + > + if (ctrl->val != 0 || ctrl->val != 90 || > + ctrl->val != 180 || ctrl->val != 270) { Should we use "and" here instead of "or"? > + pr_err("Invalid buffer rotation %d", ctrl->val); > + return; > + } > + node->dev_q.rotation = ctrl->val; > +} > + > +static int mtk_dip_video_device_s_ctrl(struct v4l2_ctrl *ctrl) > +{ > + switch (ctrl->id) { > + case V4L2_CID_PRIVATE_SET_BUFFER_USAGE: > + handle_buf_usage_config(ctrl); > + break; > + case V4L2_CID_ROTATE: > + handle_buf_rotate_config(ctrl); > + break; > + default: > + break; redundant indentation? > + } > + return 0; > +} > + > +static const struct v4l2_ctrl_ops mtk_dip_video_device_ctrl_ops = { > + .s_ctrl = mtk_dip_video_device_s_ctrl, > +}; > + > +static const struct v4l2_ctrl_config mtk_dip_buf_usage_config = { > + .ops = &mtk_dip_video_device_ctrl_ops, > + .id = V4L2_CID_PRIVATE_SET_BUFFER_USAGE, > + .name = "MTK ISP SET BUFFER USAGE", > + .type = V4L2_CTRL_TYPE_INTEGER, > + .min = MTK_DIP_V4l2_BUF_USAGE_DEFAULT, > + .max = MTK_DIP_V4l2_BUF_USAGE_POSTPROC, > + .step = 1, > + .def = MTK_DIP_V4l2_BUF_USAGE_DEFAULT, > + .flags = V4L2_CTRL_FLAG_SLIDER | V4L2_CTRL_FLAG_EXECUTE_ON_WRITE, > + }; > + > +int mtk_dip_ctrl_init(struct mtk_dip_pipe *dip_pipe) > +{ > + struct v4l2_ctrl_handler *hdl = &dip_pipe->ctrl_handler; > + struct mtk_dip_video_device *node; > + int i; > + int img_nodes_to_be_init[3] = { > + MTK_DIP_VIDEO_NODE_ID_RAW_OUT, > + MTK_DIP_VIDEO_NODE_ID_MDP0_CAPTURE, > + MTK_DIP_VIDEO_NODE_ID_MDP1_CAPTURE > + }; > + > + v4l2_ctrl_handler_init(hdl, V4L2_CID_MTK_DIP_MAX); > + > + pr_debug("%s init ctrl: %p\n", __func__, hdl); > + > + if (hdl->error) { > + pr_err("Failed in v4l2_ctrl_handler_init\n"); > + return hdl->error; > + } > + > + for (i = 0; i < MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM; i++) > + v4l2_ctrl_handler_init(&dip_pipe->nodes[i].ctrl_handler, > + V4L2_CID_MTK_DIP_MAX); > + > + for (i = 0; i < ARRAY_SIZE(img_nodes_to_be_init); i++) { > + node = &dip_pipe->nodes[img_nodes_to_be_init[i]]; > + > + if (v4l2_ctrl_new_custom(&node->ctrl_handler, > + &mtk_dip_buf_usage_config, > + NULL) == NULL) > + dev_err(&dip_pipe->dip_dev->pdev->dev, > + "Node(%s) create buf_usage_config ctrl failed:(%d)", > + node->desc->name, > + node->ctrl_handler.error); > + > + if (v4l2_ctrl_new_std(&dip_pipe->ctrl_handler, > + &mtk_dip_video_device_ctrl_ops, > + V4L2_CID_ROTATE, 0, 270, 90, 0) == NULL) > + dev_err(&dip_pipe->dip_dev->pdev->dev, > + "Node(%s) create rotate ctrl failed:(%d)", > + node->desc->name, node->ctrl_handler.error); > + } > + > +return 0; missing indentation? > +} > +EXPORT_SYMBOL_GPL(mtk_dip_ctrl_init); > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c > new file mode 100644 > index 000000000000..9f450dae2820 > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c > @@ -0,0 +1,1116 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018 MediaTek Inc. > + * Author: Frederic Chen <frederic.chen@mediatek.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/module.h> > +#include <linux/device.h> > +#include <linux/platform_device.h> > +#include <media/videobuf2-dma-contig.h> > +#include <linux/device.h> > +#include <linux/platform_device.h> > +#include <media/videobuf2-dma-contig.h> > +#include <linux/dma-mapping.h> > +#include <media/v4l2-event.h> > +#include "mtk_dip-dev.h" > +#include "mtk_dip-smem.h" > +#include "mtk-mdp3-regs.h" > +#include "mtk-img-ipi.h" > + > +int mtk_dip_pipe_init(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_dev *dip_dev, > + struct mtk_dip_pipe_desc *setting, > + struct media_device *media_dev, > + struct v4l2_device *v4l2_dev, > + struct mtk_dip_smem_dev *smem_alloc_dev) > +{ > + int ret, i; > + > + dip_pipe->dip_dev = dip_dev; > + dip_pipe->desc = setting; > + dip_pipe->smem_alloc_dev = smem_alloc_dev; > + > + atomic_set(&dip_pipe->pipe_job_sequence, 0); > + spin_lock_init(&dip_pipe->job_lock); > + mutex_init(&dip_pipe->lock); > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, "init pipe(%s,%d)\n", > + dip_pipe->desc->name, > + dip_pipe->desc->id); > + > + dip_pipe->num_nodes = MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM; > + > + for (i = 0; i < MTK_DIP_VIDEO_NODE_ID_OUT_TOTAL_NUM; i++) { > + dip_pipe->nodes[i].desc = > + &dip_pipe->desc->output_queue_descs[i]; > + dip_pipe->nodes[i].immutable = 0; > + dip_pipe->nodes[i].enabled = > + dip_pipe->nodes[i].desc->default_enable; > + atomic_set(&dip_pipe->nodes[i].sequence, 0); > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "%s: init node(%s,%d)\n", > + dip_pipe->desc->name, > + dip_pipe->nodes[i].desc->name, i); > + } > + > + for (i = MTK_DIP_VIDEO_NODE_ID_OUT_TOTAL_NUM; > + i < MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM; i++) { > + int cap_idx = i - MTK_DIP_VIDEO_NODE_ID_OUT_TOTAL_NUM; > + > + dip_pipe->nodes[i].desc = > + &dip_pipe->desc->capture_queue_descs[cap_idx]; > + dip_pipe->nodes[i].immutable = 0; > + dip_pipe->nodes[i].enabled = > + dip_pipe->nodes[i].desc->default_enable; > + atomic_set(&dip_pipe->nodes[i].sequence, 0); > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "%s: init node(%s,%d)\n", > + dip_pipe->desc->name, > + dip_pipe->nodes[i].desc->name, i); > + } Can we merge these two for loops and check if i < OUT_TOTAL_NUM inside the loop to remove some duplicate code? > + > + if (dip_pipe->desc->master >= 0 && > + dip_pipe->desc->master < MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM) { > + dip_pipe->nodes[dip_pipe->desc->master].immutable = 1; > + dip_pipe->nodes[dip_pipe->desc->master].enabled = 1; > + } > + > + ret = mtk_dip_ctrl_init(dip_pipe); > + > + if (ret) { > + dev_err(&dip_pipe->dip_dev->pdev->dev, > + "%s: failed(%d) to initialize ctrls\n", > + dip_pipe->desc->name, ret); > + goto failed_ctrl; > + } > + > + ret = mtk_dip_pipe_v4l2_register(dip_pipe, media_dev, v4l2_dev); > + > + if (ret) { > + dev_err(&dip_pipe->dip_dev->pdev->dev, > + "%s: failed(%d) to create V4L2 devices\n", > + dip_pipe->desc->name, ret); > + goto failed_pipe; > + } > + > + return 0; > + > +failed_ctrl: > +failed_pipe: > + mutex_destroy(&dip_pipe->lock); > + return ret; > +} > + > +static int mtk_dip_pipe_next_job_id(struct mtk_dip_pipe *dip_pipe) > +{ > + int global_job_id = > + atomic_inc_return(&dip_pipe->pipe_job_sequence); > + > + global_job_id = > + (global_job_id & 0x0000FFFF) | > + (dip_pipe->desc->id << 16); > + > + return global_job_id; > +} > + > +int mtk_dip_pipe_init_job_infos(struct mtk_dip_pipe *dip_pipe) > +{ > + int i; > + > + spin_lock(&dip_pipe->job_lock); > + > + dip_pipe->num_pipe_job_infos = ARRAY_SIZE(dip_pipe->pipe_job_infos); > + INIT_LIST_HEAD(&dip_pipe->pipe_job_running_list); > + INIT_LIST_HEAD(&dip_pipe->pipe_job_free_list); > + > + for (i = 0; i < dip_pipe->num_pipe_job_infos; i++) { > + struct mtk_dip_pipe_job_info *pipe_job_info = > + &dip_pipe->pipe_job_infos[i]; > + list_add_tail(&pipe_job_info->list, > + &dip_pipe->pipe_job_free_list); > + } > + > + spin_unlock(&dip_pipe->job_lock); > + > + return 0; > +} > + > +static int mtk_dip_pipe_process_pipe_job_info(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_pipe_job_info > + *pipe_job_info) > +{ > + spin_lock(&dip_pipe->job_lock); > + > + list_del(&pipe_job_info->list); > + list_add_tail(&pipe_job_info->list, &dip_pipe->pipe_job_running_list); > + > + spin_unlock(&dip_pipe->job_lock); > + return 0; > +} > + > +struct mtk_dip_pipe_job_info * > +mtk_dip_pipe_get_running_job_info(struct mtk_dip_pipe *dip_pipe, > + int pipe_job_id) > +{ > + struct mtk_dip_pipe_job_info *pipe_job_info = NULL; > + > + spin_lock(&dip_pipe->job_lock); > + > + list_for_each_entry(pipe_job_info, > + &dip_pipe->pipe_job_running_list, list) { > + if (pipe_job_info->id == pipe_job_id) { > + spin_unlock(&dip_pipe->job_lock); > + return pipe_job_info; > + } > + } > + > + spin_unlock(&dip_pipe->job_lock); > + > + return NULL; > +} > + > +static int > +mtk_dip_pipe_free_job_info(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_pipe_job_info *pipe_job_info) > +{ > + spin_lock(&dip_pipe->job_lock); > + > + list_del(&pipe_job_info->list); > + list_add_tail(&pipe_job_info->list, &dip_pipe->pipe_job_free_list); > + > + spin_unlock(&dip_pipe->job_lock); > + > + return 0; > +} > + > +static struct mtk_dip_pipe_job_info * > +mtk_dip_pipe_get_free_job_info(struct mtk_dip_pipe *dip_pipe) > +{ > + struct mtk_dip_pipe_job_info *pipe_job_info = NULL; > + > + spin_lock(&dip_pipe->job_lock); > + list_for_each_entry(pipe_job_info, > + &dip_pipe->pipe_job_free_list, list) { > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, "Found free pipe job\n"); > + spin_unlock(&dip_pipe->job_lock); > + return pipe_job_info; > + } > + spin_unlock(&dip_pipe->job_lock); > + > + dev_err(&dip_pipe->dip_dev->pdev->dev, > + "%s: can't found free pipe job\n", > + dip_pipe->desc->name); > + > + return NULL; > +} > + > +static void > +mtk_dip_pipe_update_job_info(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_pipe_job_info *pipe_job_info, > + struct mtk_dip_video_device *node, > + struct mtk_dip_dev_buffer *dev_buf) > +{ > + if (!pipe_job_info || !dev_buf || !node) { > + dev_err(&dip_pipe->dip_dev->pdev->dev, > + "%s: update pipe-job(%p) failed, buf(%p),node(%p)\n", > + dip_pipe->desc->name, > + pipe_job_info, dev_buf, node); > + return; > + } > + > + if (pipe_job_info->buf_map[node->desc->id]) > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "%s:%s: buf overwrite\n", > + dip_pipe->desc->name, > + node->desc->name); > + > + if (node->desc->buf_type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) > + pipe_job_info->num_img_capture_bufs++; > + > + if (node->desc->buf_type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > + pipe_job_info->num_img_output_bufs++; > + > + if (node->desc->buf_type == V4L2_BUF_TYPE_META_OUTPUT) > + pipe_job_info->num_meta_output_bufs++; > + > + if (node->desc->buf_type == V4L2_BUF_TYPE_META_CAPTURE) > + pipe_job_info->num_meta_capture_bufs++; > + > + pipe_job_info->buf_map[node->desc->id] = dev_buf; > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "%s:%s: added buf(%p) to pipe-job(%p)\n", > + dip_pipe->desc->name, node->desc->name, dev_buf, > + pipe_job_info); > +} > + > +static void mtk_dip_pipe_debug_job(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_pipe_job_info *pipe_job_info) > +{ > + int i; > + > + if (!dip_pipe || !pipe_job_info) > + return; > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "%s: pipe-job(%p),id(%d),req(%p)buf nums(%d,%d,%d,%d)\n", > + dip_pipe->desc->name, > + pipe_job_info, > + pipe_job_info->id, > + pipe_job_info->req, > + pipe_job_info->num_img_capture_bufs, > + pipe_job_info->num_img_output_bufs, > + pipe_job_info->num_meta_capture_bufs, > + pipe_job_info->num_meta_output_bufs); > + > + for (i = 0; i < MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM ; i++) { > + if (pipe_job_info->buf_map[i]) > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "Node(%s,%d), buf(%p)\n", > + dip_pipe->nodes[i].desc->name, i, > + pipe_job_info->buf_map[i]); > + } > +} > + > +int mtk_dip_pipe_job_finish(struct mtk_dip_pipe *dip_pipe, > + unsigned int pipe_job_info_id, > + enum vb2_buffer_state vbf_state) > +{ > + int i; > + struct mtk_dip_pipe_job_info *job_info = NULL; > + const int pipe_id = > + mtk_dip_pipe_get_pipe_from_job_id(pipe_job_info_id); > + u64 timestamp = 0; > + > + if (!dip_pipe) > + pr_err("%s: pipe-job id(%d) release failed, dip_pipe is null\n", > + __func__, pipe_job_info_id); > + > + job_info = mtk_dip_pipe_get_running_job_info(dip_pipe, > + pipe_job_info_id); > + > + if (!job_info) { > + dev_err(&dip_pipe->dip_dev->pdev->dev, > + "%s:%s: can't find pipe-job id(%d)\n", > + __func__, dip_pipe->desc->name, pipe_id); > + return -EINVAL; > + } > + > + timestamp = ktime_get_ns(); > + > + for (i = 0; i < MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM; i++) { > + struct mtk_dip_dev_buffer *dev_buf = job_info->buf_map[i]; > + > + if (!dev_buf) { > + continue; > + } else { > + dev_buf->vbb.vb2_buf.timestamp = ktime_get_ns(); > + mtk_dip_v4l2_buffer_done(&dev_buf->vbb.vb2_buf, > + vbf_state); > + } > + } > + > + mtk_dip_pipe_free_job_info(dip_pipe, job_info); > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "%s:%s: finish pipe-job, id(%d), vb state(%d)\n", > + __func__, dip_pipe->desc->name, pipe_id, > + pipe_job_info_id, vbf_state); The format string does not match the arguments. There are two ids in the arguments but only one %d for them. > + > + return 0; > +} > + > +static void mtk_dip_dev_buf_fill_info(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_dev_buffer *dev_buf) > +{ > + struct vb2_v4l2_buffer *b; > + struct mtk_dip_video_device *node; > + struct mtk_dip_video_device_desc *desc; > + > + b = &dev_buf->vbb; > + node = mtk_dip_vbq_to_node(b->vb2_buf.vb2_queue); > + desc = node->desc; > + dev_buf->fmt = node->vdev_fmt; > + dev_buf->dev_fmt = node->dev_q.dev_fmt; > + dev_buf->isp_daddr = > + vb2_dma_contig_plane_dma_addr(&b->vb2_buf, 0); > + dev_buf->vaddr = vb2_plane_vaddr(&b->vb2_buf, 0); > + dev_buf->buffer_usage = node->dev_q.buffer_usage; > + dev_buf->rotation = node->dev_q.rotation; > + > + if (desc->smem_alloc) { > + dev_buf->scp_daddr = > + mtk_dip_smem_iova_to_phys > + (dip_pipe->smem_alloc_dev, > + dev_buf->isp_daddr); > + } else { > + dev_buf->scp_daddr = 0; > + } > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "%s:%s: buf type(%d), idx(%d), mem(%d), isp_daddr(%p), scp_daddr(%p)\n", > + dip_pipe->desc->name, > + desc->name, > + b->vb2_buf.type, > + b->vb2_buf.index, > + b->vb2_buf.memory, > + dev_buf->isp_daddr, > + dev_buf->scp_daddr); > +} > + > +int mtk_dip_pipe_queue_buffers(struct media_request *req, > + int initial) > +{ > + struct media_request_object *obj; > + struct mtk_dip_pipe *dip_pipe; > + struct mtk_dip_pipe_job_info *pipe_job_info = NULL; > + int ret = 0; > + > + list_for_each_entry(obj, &req->objects, list) { > + struct vb2_buffer *vb; > + > + if (vb2_request_object_is_buffer(obj)) { > + struct mtk_dip_dev_buffer *buf; > + struct mtk_dip_dev_buffer *dev_buf; > + struct mtk_dip_video_device *node; > + > + vb = container_of(obj, struct vb2_buffer, req_obj); > + node = mtk_dip_vbq_to_node(vb->vb2_queue); > + dip_pipe = vb2_get_drv_priv(vb->vb2_queue); > + dev_buf = mtk_dip_vb2_buf_to_dev_buf(vb); > + buf = dev_buf; > + > + if (!pipe_job_info) { > + pipe_job_info = mtk_dip_pipe_get_free_job_info > + (dip_pipe); > + > + if (!pipe_job_info) > + goto FAILE_JOB_NOT_TRIGGER; > + > + memset(pipe_job_info->buf_map, 0, > + sizeof(pipe_job_info->buf_map)); > + pipe_job_info->num_img_capture_bufs = 0; > + pipe_job_info->num_img_output_bufs = 0; > + pipe_job_info->num_meta_capture_bufs = 0; > + pipe_job_info->num_meta_output_bufs = 0; > + } > + > + mtk_dip_dev_buf_fill_info(dip_pipe, > + buf); > + > + mtk_dip_pipe_update_job_info(dip_pipe, > + pipe_job_info, > + node, > + buf); > + } > + } > + > + if (!pipe_job_info) > + return -EINVAL; > + > + pipe_job_info->id = > + mtk_dip_pipe_next_job_id(dip_pipe); > + > + mtk_dip_pipe_debug_job(dip_pipe, pipe_job_info); > + > + mutex_lock(&dip_pipe->lock); > + > + if (!dip_pipe->streaming) { > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "%s:%s: stream is off, no hw enqueue triggered\n", > + __func__, dip_pipe->desc->name); > + mutex_unlock(&dip_pipe->lock); > + return 0; > + } > + > + if (mtk_dip_pipe_process_pipe_job_info(dip_pipe, pipe_job_info)) { > + dev_err(&dip_pipe->dip_dev->pdev->dev, > + "%s:%s: can't start to run pipe job id(%d)\n", > + __func__, dip_pipe->desc->name, > + pipe_job_info->id); > + mutex_unlock(&dip_pipe->lock); > + goto FAILE_JOB_NOT_TRIGGER; > + } > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "%s: trigger pipe job, id(%d)\n", > + dip_pipe->desc->name, > + dip_pipe->desc->id); > + > + if (mtk_dip_pipe_job_start(dip_pipe, pipe_job_info)) { > + mutex_unlock(&dip_pipe->lock); > + goto FAILE_JOB_NOT_TRIGGER; > + } > + > + mutex_unlock(&dip_pipe->lock); > + > + return 0; > + > +FAILE_JOB_NOT_TRIGGER: The label should be snake_case. Is "FAILE" a typo of "FAILED"? > + if (initial) > + return ret; > + > + mtk_dip_pipe_job_finish(dip_pipe, pipe_job_info->id, > + VB2_BUF_STATE_ERROR); > + > + return -EINVAL; > +} > + > +int mtk_dip_pipe_release(struct mtk_dip_pipe *dip_pipe) > +{ > + mtk_dip_pipe_v4l2_unregister(dip_pipe); > + v4l2_ctrl_handler_free(&dip_pipe->ctrl_handler); > + mutex_destroy(&dip_pipe->lock); > + > + return 0; > +} > + > +static void set_img_fmt(struct v4l2_pix_format_mplane *mfmt_to_fill, > + struct mtk_dip_dev_format *dev_fmt) > +{ > + int i; > + > + mfmt_to_fill->pixelformat = dev_fmt->fmt.img.pixelformat; > + mfmt_to_fill->num_planes = dev_fmt->fmt.img.num_planes; > + mfmt_to_fill->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > + mfmt_to_fill->quantization = V4L2_QUANTIZATION_DEFAULT; > + mfmt_to_fill->colorspace = dev_fmt->fmt.img.colorspace; > + > + memset(mfmt_to_fill->reserved, 0, sizeof(mfmt_to_fill->reserved)); > + > + pr_debug("%s: Fmt(%d),w(%d),h(%d),f(%d)\n", > + __func__, > + mfmt_to_fill->pixelformat, > + mfmt_to_fill->width, > + mfmt_to_fill->height, > + mfmt_to_fill->field); > + > + for (i = 0 ; i < mfmt_to_fill->num_planes; ++i) { Please remove the space after "i = 0". > + int bpl = (mfmt_to_fill->width * > + dev_fmt->fmt.img.row_depth[i]) / 8; > + int sizeimage = (mfmt_to_fill->width * mfmt_to_fill->height * > + dev_fmt->fmt.img.depth[i]) / 8; > + > + mfmt_to_fill->plane_fmt[i].bytesperline = bpl; > + mfmt_to_fill->plane_fmt[i].sizeimage = sizeimage; > + memset(mfmt_to_fill->plane_fmt[i].reserved, > + 0, sizeof(mfmt_to_fill->plane_fmt[i].reserved)); > + > + pr_debug("plane(%d):bpl(%d),sizeimage(%u)\n", > + i, bpl, > + mfmt_to_fill->plane_fmt[i].sizeimage); > + } > +} > + > +static void set_meta_fmt(struct v4l2_meta_format *metafmt_to_fill, > + struct mtk_dip_dev_format *dev_fmt) > +{ > + metafmt_to_fill->dataformat = dev_fmt->fmt.meta.dataformat; > + > + if (dev_fmt->fmt.meta.max_buffer_size <= 0) { > + pr_debug("Invalid meta buf size(%u), use default(%u)\n", > + dev_fmt->fmt.meta.max_buffer_size, > + MTK_DIP_DEV_META_BUF_DEFAULT_SIZE); > + metafmt_to_fill->buffersize = MTK_DIP_DEV_META_BUF_DEFAULT_SIZE; > + } else { > + pr_debug("Use meta size(%u)\n", > + dev_fmt->fmt.meta.max_buffer_size); > + metafmt_to_fill->buffersize = dev_fmt->fmt.meta.max_buffer_size; > + } > +} > + > +void mtk_dip_pipe_load_default_fmt(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_video_device *node, > + struct v4l2_format *fmt_to_fill) > +{ > + struct mtk_dip_dev_format *dev_fmt; > + struct mtk_dip_video_device_desc *desc = node->desc; > + > + if (desc->num_fmts == 0) { > + pr_err("%s:%s: desc->num_fmts is 0, no format support list\n", > + __func__, desc->name); > + return; > + } > + > + if (desc->default_fmt_idx >= desc->num_fmts) { > + pr_debug("%s:%s: invalid idx(%d), must < num_fmts(%d)\n", > + __func__, desc->name, desc->default_fmt_idx, > + desc->num_fmts); > + desc->default_fmt_idx = 0; > + } > + > + dev_fmt = &desc->fmts[desc->default_fmt_idx]; > + fmt_to_fill->type = desc->buf_type; > + if (mtk_dip_buf_is_meta(desc->buf_type)) { > + set_meta_fmt(&fmt_to_fill->fmt.meta, dev_fmt); > + } else { > + fmt_to_fill->fmt.pix_mp.width = desc->default_width; > + fmt_to_fill->fmt.pix_mp.height = desc->default_height; > + fmt_to_fill->fmt.pix_mp.field = V4L2_FIELD_NONE; > + > + set_img_fmt(&fmt_to_fill->fmt.pix_mp, dev_fmt); > + } > +} > + > +struct mtk_dip_dev_format * > +mtk_dip_pipe_find_fmt(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_video_device *node, > + u32 format) > +{ > + int i; > + struct mtk_dip_dev_format *dev_fmt; > + > + struct mtk_dip_video_device_desc *desc = node->desc; > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, "fmt to find(%x)\n", format); > + > + for (i = 0; i < desc->num_fmts; i++) { > + dev_fmt = &desc->fmts[i]; > + if (!mtk_dip_buf_is_meta(desc->buf_type)) { > + if (dev_fmt->fmt.img.pixelformat == format) > + return dev_fmt; > + } else { > + if (dev_fmt->fmt.meta.dataformat == format) > + return dev_fmt; > + } > + } > + > + return NULL; > +} > + > +int mtk_dip_pipe_set_meta_fmt(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_video_device *node, > + struct v4l2_meta_format *user_fmt, > + struct v4l2_meta_format *node_fmt) > +{ > + struct mtk_dip_dev_format *dev_fmt; > + > + if (!user_fmt || !node_fmt) > + return -EINVAL; > + > + dev_fmt = mtk_dip_pipe_find_fmt(dip_pipe, node, > + user_fmt->dataformat); > + > + if (!dev_fmt) > + return -EINVAL; > + > + node->dev_q.dev_fmt = dev_fmt; > + set_meta_fmt(node_fmt, dev_fmt); > + *user_fmt = *node_fmt; > + > + return 0; > +} > + > +int mtk_dip_pipe_set_img_fmt(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_video_device *node, > + struct v4l2_pix_format_mplane *user_fmt, > + struct v4l2_pix_format_mplane *dest_fmt) > +{ > + struct mtk_dip_dev_format *dev_fmt; > + > + if (!user_fmt || !dest_fmt) > + return -EINVAL; > + > + dev_fmt = mtk_dip_pipe_find_fmt(dip_pipe, node, > + user_fmt->pixelformat); > + > + if (!dev_fmt) { > + pr_debug("%s:%s:%s: dev_fmt(%d) not found\n", > + __func__, dip_pipe->desc->name, > + node->desc->name, user_fmt->pixelformat); > + return -EINVAL; > + } > + > + node->dev_q.dev_fmt = dev_fmt; > + dest_fmt->width = user_fmt->width; > + dest_fmt->height = user_fmt->height; > + dest_fmt->field = V4L2_FIELD_NONE; > + > + set_img_fmt(dest_fmt, dev_fmt); > + > + return 0; > +} > + > +int mtk_dip_pipe_streamon(struct mtk_dip_pipe *dip_pipe) > +{ > + int ret; > + struct mtk_dip_dev *dip_dev; > + > + if (!dip_pipe) > + return -EINVAL; > + > + dip_dev = dev_get_drvdata(&dip_pipe->dip_dev->pdev->dev); > + > + mutex_lock(&dip_pipe->lock); > + > + ret = mtk_dip_hw_streamon(&dip_dev->dip_hw, > + dip_pipe->desc->id); > + > + if (ret) { > + dev_err(&dip_pipe->dip_dev->pdev->dev, > + "%s:%s:%d: failed to start hw\n", > + __func__, dip_pipe->desc->name, > + dip_pipe->desc->id); > + mutex_unlock(&dip_pipe->lock); > + return -EBUSY; > + } > + > + dip_pipe->streaming = 1; > + mutex_unlock(&dip_pipe->lock); > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "%s:%s:%d: start hw\n", > + __func__, dip_pipe->desc->name, > + dip_pipe->desc->id); > + > + return ret; > +} > + > +int mtk_dip_pipe_streamoff(struct mtk_dip_pipe *dip_pipe) > +{ > + int ret; > + struct mtk_dip_dev *dip_dev; > + > + if (!dip_pipe) > + return -EINVAL; > + > + dip_dev = dev_get_drvdata(&dip_pipe->dip_dev->pdev->dev); > + > + mutex_lock(&dip_pipe->lock); > + > + ret = mtk_dip_hw_streamoff(&dip_dev->dip_hw, > + dip_pipe->desc->id); > + > + if (ret) { > + dev_err(&dip_pipe->dip_dev->pdev->dev, > + "%s:%s:%d: failed to stop hw\n", > + __func__, dip_pipe->desc->name, > + dip_pipe->desc->id); > + mutex_unlock(&dip_pipe->lock); > + return -EBUSY; > + } > + > + dip_pipe->streaming = 0; > + mutex_unlock(&dip_pipe->lock); > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "%s:%s:%d: stop hw\n", > + __func__, dip_pipe->desc->name, > + dip_pipe->desc->id); > + > + return 0; > +} > + > +static enum mdp_ycbcr_profile > +map_ycbcr_prof_mplane(struct v4l2_pix_format_mplane *pix_mp, > + u32 mdp_color) > +{ > + 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; > + } > + /* V4L2_COLORSPACE_SRGB or else */ > + if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE) > + return MDP_YCBCR_PROFILE_FULL_BT601; > + return MDP_YCBCR_PROFILE_BT601; > +} > + > +/* Stride that is accepted by MDP HW */ > +static u32 dip_mdp_fmt_get_stride(const struct mtk_dip_dev_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 > +dip_mdp_fmt_get_stride_contig(const struct mtk_dip_dev_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 > +dip_mdp_fmt_get_plane_size(const struct mtk_dip_dev_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 int is_contig_mp_buffer(struct mtk_dip_dev_buffer *dev_buf) > +{ > + if (MDP_COLOR_GET_PLANE_COUNT(dev_buf->dev_fmt->fmt.img.mdp_color) > + == 1) > + return 0; > + else > + return 1; > +} > + > +static int fill_ipi_img_param_mp(struct mtk_dip_pipe *dip_pipe, > + struct img_image_buffer *b, > + struct mtk_dip_dev_buffer *dev_buf, > + char *buf_name) > +{ > + struct v4l2_pix_format_mplane *pix_mp; > + struct mtk_dip_dev_mdp_format *mdp_fmt; > + unsigned int i; > + unsigned int total_plane_size = 0; > + > + if (!dev_buf || !dev_buf->dev_fmt) { > + dev_err(&dip_pipe->dip_dev->pdev->dev, > + "%s: %s's dev format not set\n", > + __func__, buf_name); > + return -EINVAL; > + } > + > + pix_mp = &dev_buf->fmt.fmt.pix_mp; > + mdp_fmt = &dev_buf->dev_fmt->fmt.img; > + > + b->format.colorformat = dev_buf->dev_fmt->fmt.img.mdp_color; > + b->format.width = dev_buf->fmt.fmt.pix_mp.width; > + b->format.height = dev_buf->fmt.fmt.pix_mp.height; > + b->format.ycbcr_prof = > + map_ycbcr_prof_mplane(pix_mp, > + dev_buf->dev_fmt->fmt.img.mdp_color); > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "%s: buf(%s), IPI: w(%d),h(%d),c(0x%x)\n", > + dip_pipe->desc->name, > + buf_name, > + b->format.width, > + b->format.height, > + b->format.colorformat); > + > + for (i = 0; i < pix_mp->num_planes; ++i) { > + u32 stride = > + dip_mdp_fmt_get_stride > + (mdp_fmt, pix_mp->plane_fmt[i].bytesperline, i); > + > + b->format.plane_fmt[i].stride = stride; > + b->format.plane_fmt[i].size = > + dip_mdp_fmt_get_plane_size(mdp_fmt, > + stride, > + pix_mp->height, i); > + b->iova[i] = dev_buf->isp_daddr; > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "Contiguous-mp-buf:plane(%i),stride(%d),size(%d),iova(%p)", > + i, > + b->format.plane_fmt[i].stride, > + b->format.plane_fmt[i].size, > + b->iova[i]); iova is an u32, so should we use %x instead of %llx here? > + total_plane_size = b->format.plane_fmt[i].size; > + } > + > + for (; i < MDP_COLOR_GET_PLANE_COUNT(b->format.colorformat); ++i) { > + u32 stride = > + dip_mdp_fmt_get_stride_contig > + (mdp_fmt, b->format.plane_fmt[0].stride, i); > + > + b->format.plane_fmt[i].stride = stride; > + b->format.plane_fmt[i].size = > + dip_mdp_fmt_get_plane_size(mdp_fmt, stride, > + pix_mp->height, i); > + b->iova[i] = b->iova[i - 1] + b->format.plane_fmt[i - 1].size; > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "Contiguous-mp-buf:plane(%i),stride(%d),size(%d),iova(%p)", > + i, > + b->format.plane_fmt[i].stride, > + b->format.plane_fmt[i].size, > + b->iova[i]); iova is an u32, so should we use %x instead of %llx here? > + total_plane_size += b->format.plane_fmt[i].size; > + } > + > + b->usage = dev_buf->buffer_usage; > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "Contiguous-mp-buf(%s),v4l2-sizeimage(%d),total-plane-size(%d)\n", > + buf_name, > + pix_mp->plane_fmt[0].sizeimage, > + total_plane_size); > + > + return 0; > +} > + > +static int fill_ipi_img_param(struct mtk_dip_pipe *dip_pipe, > + struct img_image_buffer *img, > + struct mtk_dip_dev_buffer *dev_buf, > + char *buf_name) > +{ > + img->format.width = dev_buf->fmt.fmt.pix_mp.width; > + img->format.height = dev_buf->fmt.fmt.pix_mp.height; > + > + if (dev_buf && dev_buf->dev_fmt) { > + img->format.colorformat = > + dev_buf->dev_fmt->fmt.img.mdp_color; > + } else { > + dev_err(&dip_pipe->dip_dev->pdev->dev, > + "%s: %s's dev format not set\n", > + __func__, buf_name); > + return -EINVAL; > + } > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "%s: buf(%s) IPI: w(%d),h(%d),c(0x%x)\n", > + dip_pipe->desc->name, > + buf_name, > + img->format.width, > + img->format.height, > + img->format.colorformat); > + > + img->format.plane_fmt[0].size = > + dev_buf->fmt.fmt.pix_mp.plane_fmt[0].sizeimage; > + img->format.plane_fmt[0].stride = > + dev_buf->fmt.fmt.pix_mp.plane_fmt[0].bytesperline; > + img->iova[0] = dev_buf->isp_daddr; > + img->usage = dev_buf->buffer_usage; > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "size(%d), stride(%d),ycbcr(%d),iova(%p),u(%d)\n", > + img->format.plane_fmt[0].size, > + img->format.plane_fmt[0].stride, > + img->format.ycbcr_prof, > + img->iova[0], > + img->usage); > + > + return 0; > +} > + > +static int fill_input_ipi_param(struct mtk_dip_pipe *dip_pipe, > + struct img_input *iin, > + struct mtk_dip_dev_buffer *dev_buf, > + char *buf_name) > +{ > + struct img_image_buffer *img = &iin->buffer; > + > + /* Will map the vale with V4L2 color space in the future */ typo? vale => value > + img->format.ycbcr_prof = 1; > + if (is_contig_mp_buffer(dev_buf)) > + return fill_ipi_img_param_mp(dip_pipe, img, dev_buf, > + buf_name); > + else > + return fill_ipi_img_param(dip_pipe, img, dev_buf, > + buf_name); > +} > + > +static int fill_output_ipi_param(struct mtk_dip_pipe *dip_pipe, > + struct img_output *iout, > + struct mtk_dip_dev_buffer *dev_buf_out, > + struct mtk_dip_dev_buffer *dev_buf_in, > + char *buf_name) > +{ > + int ret; > + struct img_image_buffer *img = &iout->buffer; > + > + img->format.ycbcr_prof = 0; > + > + if (is_contig_mp_buffer(dev_buf_out)) > + ret = fill_ipi_img_param_mp(dip_pipe, img, dev_buf_out, > + buf_name); > + else > + ret = fill_ipi_img_param(dip_pipe, img, dev_buf_out, > + buf_name); > + > + iout->crop.left = 0; > + iout->crop.top = 0; > + iout->crop.width = dev_buf_in->fmt.fmt.pix_mp.width; > + iout->crop.height = dev_buf_in->fmt.fmt.pix_mp.height; > + iout->crop.left_subpix = 0; > + iout->crop.top_subpix = 0; > + iout->crop.width_subpix = 0; > + iout->crop.height_subpix = 0; > + iout->rotation = dev_buf_out->rotation; > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "%s: buf(%s) IPI-ext:c_l(%d),c_t(%d),c_w(%d),c_h(%d)\n", > + dip_pipe->desc->name, > + buf_name, > + iout->crop.left, > + iout->crop.top, > + iout->crop.width, > + iout->crop.height); > + > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > + "c_ls(%d),c_ts(%d),c_ws(%d),c_hs(%d),rot(%d)\n", > + iout->crop.left_subpix, > + iout->crop.top_subpix, > + iout->crop.width_subpix, > + iout->crop.height_subpix, > + iout->rotation); > + > + return ret; > +} > + > +int mtk_dip_pipe_job_start(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_pipe_job_info *pipe_job_info) > +{ > + struct platform_device *pdev = dip_pipe->dip_dev->pdev; > + int ret; > + int out_img_buf_idx; > + struct img_ipi_frameparam dip_param; > + struct mtk_dip_dev_buffer *dev_buf_in; > + struct mtk_dip_dev_buffer *dev_buf_out; > + struct mtk_dip_dev_buffer *dev_buf_tuning; > + > + if (!pipe_job_info) { > + dev_err(&pdev->dev, > + "pipe_job_info(%p) in start can't be NULL\n", > + pipe_job_info); > + return -EINVAL; > + } > + > + /* We need RAW and at least MDP0 or MDP 1 buffer */ > + if (!pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_RAW_OUT] || > + (!pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_MDP0_CAPTURE] && > + !pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_MDP1_CAPTURE])){ > + struct mtk_dip_dev_buffer **map = pipe_job_info->buf_map; > + > + dev_dbg(&pdev->dev, > + "can't trigger job: raw(%p), mdp0(%p), mdp1(%p)\n", > + map[MTK_DIP_VIDEO_NODE_ID_RAW_OUT], > + map[MTK_DIP_VIDEO_NODE_ID_MDP0_CAPTURE], > + map[MTK_DIP_VIDEO_NODE_ID_MDP1_CAPTURE]); > + return -EINVAL; > + } > + > + dev_dbg(&pdev->dev, > + "%s:%s: pipe-job id(%d)\n", > + __func__, dip_pipe->desc->name, > + pipe_job_info->id); > + > + /* Fill ipi params for DIP driver */ > + memset(&dip_param, 0, sizeof(struct img_ipi_frameparam)); > + > + dip_param.index = pipe_job_info->id; > + dip_param.num_outputs = pipe_job_info->num_img_capture_bufs; > + dip_param.num_inputs = pipe_job_info->num_img_output_bufs; > + dip_param.type = STREAM_ISP_IC; > + > + /* Tuning buffer */ > + dev_buf_tuning = > + pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_TUNING_OUT]; > + if (dev_buf_tuning) { > + dev_dbg(&pdev->dev, > + "Tuning buf queued: pa(%p),va(%p),iova(%p)\n", > + dev_buf_tuning->scp_daddr, > + dev_buf_tuning->vaddr, > + dev_buf_tuning->isp_daddr); > + dip_param.tuning_data.pa = (uint32_t)dev_buf_tuning->scp_daddr; > + dip_param.tuning_data.va = (uint64_t)dev_buf_tuning->vaddr; > + dip_param.tuning_data.iova = > + (uint32_t)dev_buf_tuning->isp_daddr; > + } else { > + dev_dbg(&pdev->dev, > + "Doesn't enqueued tuning buffer, by-pass\n"); > + dip_param.tuning_data.pa = 0; > + dip_param.tuning_data.va = 0; > + dip_param.tuning_data.iova = 0; > + } > + > + /* Raw-in buffer */ > + dev_buf_in = > + pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_RAW_OUT]; > + if (dev_buf_in) { > + struct img_input *iin = &dip_param.inputs[0]; > + > + fill_input_ipi_param(dip_pipe, iin, dev_buf_in, "RAW"); > + } > + > + out_img_buf_idx = 0; > + > + /* MDP 0 buffer */ > + dev_buf_out = > + pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_MDP0_CAPTURE]; > + if (dev_buf_out) { > + struct img_output *iout = &dip_param.outputs[out_img_buf_idx]; > + > + fill_output_ipi_param(dip_pipe, iout, dev_buf_out, > + dev_buf_in, "MDP0"); > + out_img_buf_idx++; > + } > + > + /* MDP 1 buffer */ > + dev_buf_out = > + pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_MDP1_CAPTURE]; > + if (dev_buf_out) { > + struct img_output *iout = &dip_param.outputs[out_img_buf_idx]; > + > + fill_output_ipi_param(dip_pipe, iout, dev_buf_out, > + dev_buf_in, "MDP1"); > + out_img_buf_idx++; > + } > + > + ret = mtk_dip_hw_enqueue(&dip_pipe->dip_dev->dip_hw, &dip_param); > + > + if (ret) { > + dev_dbg(&pdev->dev, > + "%s:%s: enqueue to HW failed(%d)\n", > + __func__, dip_pipe->desc->name, ret); > + return -EBUSY; > + } > + > + return ret; > +} > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h > new file mode 100644 > index 000000000000..f51f7a44379a > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h > @@ -0,0 +1,321 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2018 MediaTek Inc. > + * Author: Frederic Chen <frederic.chen@mediatek.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#ifndef _MTK_DIP_DEV_H_ > +#define _MTK_DIP_DEV_H_ > + > +#include <linux/types.h> > +#include <linux/device.h> > +#include <linux/platform_device.h> > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-subdev.h> > +#include <media/v4l2-device.h> > +#include <linux/videodev2.h> > +#include <media/videobuf2-core.h> > +#include <media/videobuf2-v4l2.h> > + > +#include "mtk_dip-hw.h" > +#include "mtk_dip-smem.h" > + > +#define MTK_DIP_PIPE_ID_PREVIEW 0 > +#define MTK_DIP_PIPE_ID_CAPTURE 1 > +#define MTK_DIP_PIPE_ID_REPROCESS 2 > +#define MTK_DIP_PIPE_ID_TOTAL_NUM 3 > + > +#define MTK_DIP_VIDEO_NODE_ID_RAW_OUT 0 > +#define MTK_DIP_VIDEO_NODE_ID_TUNING_OUT 1 > +#define MTK_DIP_VIDEO_NODE_ID_OUT_TOTAL_NUM 2 > +#define MTK_DIP_VIDEO_NODE_ID_MDP0_CAPTURE 2 > +#define MTK_DIP_VIDEO_NODE_ID_MDP1_CAPTURE 3 > +#define MTK_DIP_VIDEO_NODE_ID_CAPTURE_TOTAL_NUM 2 > +#define MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM \ > + (MTK_DIP_VIDEO_NODE_ID_OUT_TOTAL_NUM + \ > + MTK_DIP_VIDEO_NODE_ID_CAPTURE_TOTAL_NUM) > + > +#define MTK_DIP_VIDEO_NODE_ID_NO_MASTER -1 > + > +#define MTK_DIP_OUTPUT_MIN_WIDTH 2U > +#define MTK_DIP_OUTPUT_MIN_HEIGHT 2U > +#define MTK_DIP_OUTPUT_MAX_WIDTH 5376U > +#define MTK_DIP_OUTPUT_MAX_HEIGHT 4032U > +#define MTK_DIP_CAPTURE_MIN_WIDTH 2U > +#define MTK_DIP_CAPTURE_MIN_HEIGHT 2U > +#define MTK_DIP_CAPTURE_MAX_WIDTH 5376U > +#define MTK_DIP_CAPTURE_MAX_HEIGHT 4032U > + > +#define MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME "MTK-ISP-DIP-V4L2" > +#define MTK_DIP_DEV_DIP_PREVIEW_NAME \ > + MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME > +#define MTK_DIP_DEV_DIP_CAPTURE_NAME "MTK-ISP-DIP-CAP-V4L2" > +#define MTK_DIP_DEV_DIP_REPROCESS_NAME "MTK-ISP-DIP-REP-V4L2" > + > +#define MTK_DIP_DEV_META_BUF_DEFAULT_SIZE (1110 * 1024) > + > +#define V4L2_CID_PRIVATE_UT_NUM (V4L2_CID_USER_BASE | 0x1001) > +#define V4L2_CID_PRIVATE_SET_BUFFER_USAGE (V4L2_CID_PRIVATE_UT_NUM + 2) > +#define V4L2_CID_MTK_DIP_MAX 100 > + > +enum mtk_dip_v4l2_buffer_usage { > + MTK_DIP_V4l2_BUF_USAGE_DEFAULT = 0, > + MTK_DIP_V4l2_BUF_USAGE_FD, > + MTK_DIP_V4l2_BUF_USAGE_POSTPROC, > + MTK_DIP_V4l2_BUF_USAGE_NONE, > +}; The constants in enums should be capitalized. Could we use V4L2 instead of V4l2 here? > + > +/* > + * Supported format and the information used for > + * size calculation > + */ > +struct mtk_dip_dev_meta_format { > + u32 dataformat; > + u32 max_buffer_size; > + u8 flags; > +}; > + > +/* MDP part private format definitation */ > +struct mtk_dip_dev_mdp_format { > + u32 pixelformat; > + u32 mdp_color; > + u32 colorspace; > + u8 depth[VIDEO_MAX_PLANES]; > + u8 row_depth[VIDEO_MAX_PLANES]; > + u8 num_planes; > + u8 walign; > + u8 halign; > + u8 salign; > + u32 flags; > +}; > + > +struct mtk_dip_dev_format { > + union { > + struct mtk_dip_dev_meta_format meta; > + struct mtk_dip_dev_mdp_format img; > + } fmt; > +}; > + > +struct mtk_dip_pipe_job_info { > + struct media_request *req; > + int id; > + struct mtk_dip_dev_buffer* > + buf_map[MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM]; > + int num_img_capture_bufs; > + int num_img_output_bufs; > + int num_meta_capture_bufs; > + int num_meta_output_bufs; > + struct list_head list; > +}; > + > +struct mtk_dip_dev_buffer { > + struct vb2_v4l2_buffer vbb; > + struct v4l2_format fmt; > + struct mtk_dip_dev_format *dev_fmt; > + int pipe_job_id; > + void *vaddr; > + dma_addr_t isp_daddr; > + dma_addr_t scp_daddr; > + unsigned int buffer_usage; > + int rotation; > + struct list_head list; > +}; > + > +struct mtk_dip_pipe_desc { > + char *name; > + int master; > + int id; > + struct mtk_dip_video_device_desc *output_queue_descs; > + int total_output_queues; > + struct mtk_dip_video_device_desc *capture_queue_descs; > + int total_capture_queues; > +}; > + > +struct mtk_dip_video_device_desc { > + int id; > + char *name; > + u32 buf_type; > + u32 cap; > + int smem_alloc; > + int dynamic; > + int default_enable; > + struct mtk_dip_dev_format *fmts; > + int num_fmts; > + char *description; > + int default_width; > + int default_height; > + const struct v4l2_ioctl_ops *ops; > + int default_fmt_idx; > +}; > + > +struct mtk_dip_dev_queue { > + struct vb2_queue vbq; > + /* Serializes vb2 queue and video device operations */ > + struct mutex lock; > + struct mtk_dip_dev_format *dev_fmt; > + /* Firmware uses buffer_usage to select suitable DMA ports */ > + unsigned int buffer_usage; > + int rotation; > +}; > + > +struct mtk_dip_video_device { > + struct video_device vdev; > + struct mtk_dip_dev_queue dev_q; > + struct v4l2_format vdev_fmt; > + struct media_pad vdev_pad; > + struct v4l2_mbus_framefmt pad_fmt; > + struct v4l2_ctrl_handler ctrl_handler; > + int immutable; > + int enabled; > + struct mtk_dip_video_device_desc *desc; > + atomic_t sequence; > +}; > + > +struct mtk_dip_pipe { > + struct mtk_dip_dev *dip_dev; > + struct mtk_dip_video_device nodes[MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM]; > + int num_nodes; > + int streaming; > + struct media_pad *subdev_pads; > + struct media_pipeline pipeline; > + struct v4l2_subdev subdev; > + struct v4l2_subdev_fh *fh; > + struct v4l2_ctrl_handler ctrl_handler; > + struct mtk_dip_smem_dev *smem_alloc_dev; > + atomic_t pipe_job_sequence; > + struct mtk_dip_pipe_job_info pipe_job_infos[VB2_MAX_FRAME]; > + int num_pipe_job_infos; > + struct list_head pipe_job_running_list; > + struct list_head pipe_job_free_list; > + /* Serializes pipe's stream on/off and buffers enqueue operations */ > + struct mutex lock; > + spinlock_t job_lock; /* protect the pipe job list */ > + struct mtk_dip_pipe_desc *desc; > +}; > + > +struct mtk_dip_dev { > + struct platform_device *pdev; > + struct media_device mdev; > + struct v4l2_device v4l2_dev; > + struct mtk_dip_pipe dip_pipe[MTK_DIP_PIPE_ID_TOTAL_NUM]; > + struct mtk_dip_smem_dev smem_alloc_dev; > + struct mtk_dip_hw dip_hw; > +}; > + > +int mtk_dip_dev_media_register(struct device *dev, > + struct media_device *media_dev, > + const char *model); > + > +int mtk_dip_dev_v4l2_init(struct mtk_dip_dev *dip_dev); > + > +void mtk_dip_dev_v4l2_release(struct mtk_dip_dev *dip_dev); > + > +int mtk_dip_dev_v4l2_register(struct device *dev, > + struct media_device *media_dev, > + struct v4l2_device *v4l2_dev); > + > +int mtk_dip_pipe_v4l2_register(struct mtk_dip_pipe *dip_pipe, > + struct media_device *media_dev, > + struct v4l2_device *v4l2_dev); > + > +int mtk_dip_pipe_v4l2_unregister(struct mtk_dip_pipe *dip_pipe); > + > +void mtk_dip_v4l2_buffer_done(struct vb2_buffer *vb, > + enum vb2_buffer_state state); > + > +int mtk_dip_pipe_queue_buffers(struct media_request *req, int initial); > + > +int mtk_dip_pipe_init(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_dev *dip_dev, > + struct mtk_dip_pipe_desc *setting, > + struct media_device *media_dev, > + struct v4l2_device *v4l2_dev, > + struct mtk_dip_smem_dev *smem_alloc_dev); > + > +int mtk_dip_pipe_release(struct mtk_dip_pipe *dip_pipe); > + > +int mtk_dip_pipe_job_finish(struct mtk_dip_pipe *dip_pipe, > + unsigned int pipe_job_info_id, > + enum vb2_buffer_state state); > + > +int mtk_dip_pipe_job_start(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_pipe_job_info *pipe_job_info); > + > +int mtk_dip_pipe_init_job_infos(struct mtk_dip_pipe *dip_pipe); > + > +struct mtk_dip_dev_format * > +mtk_dip_pipe_find_fmt(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_video_device *node, > + u32 format); > + > +int mtk_dip_pipe_set_img_fmt(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_video_device *node, > + struct v4l2_pix_format_mplane *user_fmt, > + struct v4l2_pix_format_mplane *node_fmt); > + > +int mtk_dip_pipe_set_meta_fmt(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_video_device *node, > + struct v4l2_meta_format *user_fmt, > + struct v4l2_meta_format *node_fmt); Where do we use this function? > + > +void mtk_dip_pipe_load_default_fmt(struct mtk_dip_pipe *dip_pipe, > + struct mtk_dip_video_device *node, > + struct v4l2_format *fmt_to_fill); > + > +int mtk_dip_pipe_streamon(struct mtk_dip_pipe *dip_pipe); > + > +int mtk_dip_pipe_streamoff(struct mtk_dip_pipe *dip_pipe); > + > +int mtk_dip_ctrl_init(struct mtk_dip_pipe *dip_pipe); > + > +static inline struct mtk_dip_video_device * > +mtk_dip_file_to_node(struct file *file) > +{ > + return container_of(video_devdata(file), > + struct mtk_dip_video_device, vdev); > +} > + > +static inline struct mtk_dip_pipe * > +mtk_dip_subdev_to_pipe(struct v4l2_subdev *sd) > +{ > + return container_of(sd, struct mtk_dip_pipe, subdev); > +} > + > +static inline struct mtk_dip_video_device * > +mtk_dip_vbq_to_node(struct vb2_queue *vq) > +{ > + return container_of(vq, struct mtk_dip_video_device, dev_q.vbq); > +} > + > +static inline struct mtk_dip_dev_buffer * > +mtk_dip_vb2_buf_to_dev_buf(struct vb2_buffer *vb) > +{ > + return container_of(vb, struct mtk_dip_dev_buffer, vbb.vb2_buf); > +} > + > +static inline struct mtk_dip_dev *mtk_dip_hw_to_dev(struct mtk_dip_hw *dip_hw) > +{ > + return container_of(dip_hw, struct mtk_dip_dev, dip_hw); > +} > + > +static inline int mtk_dip_buf_is_meta(u32 type) > +{ > + return type == V4L2_BUF_TYPE_META_CAPTURE || > + type == V4L2_BUF_TYPE_META_OUTPUT; > +} > + > +static inline int mtk_dip_pipe_get_pipe_from_job_id(int pipe_job_id) > +{ > + return (pipe_job_id >> 16) & 0x0000FFFF; > +} > + > +#endif /* _MTK_DIP_DEV_H_ */ > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-hw.h b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-hw.h > new file mode 100644 > index 000000000000..d813d8b92e8b > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-hw.h > @@ -0,0 +1,167 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2018 MediaTek Inc. > + * Author: Holmes Chiou <holmes.chiou@mediatek.com> > + * > + * This program is free software: you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#ifndef _MTK_DIP_HW_H_ > +#define _MTK_DIP_HW_H_ > + > +#include <linux/clk.h> > +#include "mtk-img-ipi.h" > + > +enum STREAM_TYPE_ENUM { > + STREAM_UNKNOWN, > + STREAM_BITBLT, > + STREAM_GPU_BITBLT, > + STREAM_DUAL_BITBLT, > + STREAM_2ND_BITBLT, > + STREAM_ISP_IC, > + STREAM_ISP_VR, > + STREAM_ISP_ZSD, > + STREAM_ISP_IP, > + STREAM_ISP_VSS, > + STREAM_ISP_ZSD_SLOW, > + STREAM_WPE, > + STREAM_WPE2, > +}; > + > +enum mtk_dip_hw_user_state { > + DIP_STATE_INIT = 0, > + DIP_STATE_STREAMON, > + DIP_STATE_STREAMOFF > +}; > + > +struct mtk_dip_hw_frame_job { > + struct img_frameparam fparam; > + int sequence; > +}; > + > +struct mtk_dip_hw_user_id { > + struct list_head list_entry; > + u16 id; > + u32 num; > + u16 state; > +}; > + > +struct mtk_dip_hw_subframe { > + struct img_addr buffer; > + struct sg_table table; > + struct img_sw_addr config_data; > + struct img_addr tuning_buf; > + struct img_sw_addr frameparam; > + struct list_head list_entry; > +}; > + > +struct mtk_dip_hw_queue { > + struct list_head queue; > + struct mutex queuelock; /* protect queue and queue_cnt */ > + u32 queue_cnt; > +}; > + > +struct mtk_dip_hw_joblist { > + struct list_head queue; > + spinlock_t queuelock; /* protect job list */ > + u32 queue_cnt; > +}; > + > +struct mtk_dip_hw_thread { > + struct task_struct *thread; > + wait_queue_head_t wq; > +}; > + > +struct mtk_dip_hw_work { > + struct list_head list_entry; > + struct img_ipi_frameparam frameparams; > + struct mtk_dip_hw_user_id *user_id; > +}; > + > +struct mtk_dip_hw_submit_work { > + struct work_struct frame_work; > + struct mtk_dip_hw *dip_hw; > +}; > + > +struct mtk_dip_hw_mdpcb_work { > + struct work_struct frame_work; > + struct img_ipi_frameparam *frameparams; > +}; > + > +struct mtk_dip_hw_clk { > + struct clk *img_larb5; > + struct clk *img_dip; > +}; > + > +enum frame_state { > + FRAME_STATE_INIT = 0, > + FRAME_STATE_COMPOSING, > + FRAME_STATE_RUNNING, > + FRAME_STATE_DONE, > + FRAME_STATE_STREAMOFF, > + FRAME_STATE_ERROR, > + FRAME_STATE_HW_TIMEOUT > +}; > + > +struct mtk_dip_hw { > + struct mtk_dip_hw_clk dip_clk; > + struct device *larb_dev; > + struct mtk_dip_hw_joblist dip_gcejoblist; > + struct mtk_dip_hw_queue dip_freebufferlist; > + struct mtk_dip_hw_queue dip_usedbufferlist; > + struct mtk_dip_hw_thread dip_runner_thread; > + struct mtk_dip_hw_queue dip_useridlist; > + struct mtk_dip_hw_queue dip_worklist; > + struct workqueue_struct *composer_wq; > + struct mtk_dip_hw_submit_work submit_work; > + wait_queue_head_t composing_wq; > + wait_queue_head_t flushing_wq; > + atomic_t num_composing; /* increase after ipi */ > + /* increase after calling MDP driver */ > + atomic_t num_running; > + /*MDP/GCE callback workqueue */ Missing space after "/*". > + struct workqueue_struct *mdpcb_workqueue; > + /* for MDP driver */ > + struct platform_device *mdp_pdev; > + /* for VPU driver */ > + struct platform_device *vpu_pdev; > + struct rproc *rproc_handle; > + dma_addr_t scp_workingbuf_addr; > + /* increase after enqueue */ > + atomic_t dip_enque_cnt; > + /* increase after Stream ON, decrease when Stream OFF */ > + atomic_t dip_stream_cnt; > + /* increase after open, decrease when close */ > + atomic_t dip_user_cnt; > +}; > + > +int mtk_dip_hw_enqueue(struct mtk_dip_hw *dip_hw, > + struct img_ipi_frameparam *frameparams); > +int mtk_dip_hw_connect(struct mtk_dip_hw *dip_hw); > +int mtk_dip_hw_disconnect(struct mtk_dip_hw *dip_hw); > +int mtk_dip_hw_streamon(struct mtk_dip_hw *dip_hw, u16 id); > +int mtk_dip_hw_streamoff(struct mtk_dip_hw *dip_hw, u16 id); > + > +static inline struct mtk_dip_hw_frame_job > +*mtk_dip_fparam_to_job(struct img_frameparam *fparam) > +{ > + return container_of(fparam, struct mtk_dip_hw_frame_job, fparam); > +} > + > +static inline struct mtk_dip_hw_frame_job * > +mtk_dip_ipi_fparam_to_job(struct img_ipi_frameparam *ipi_fparam) > +{ > + return container_of(ipi_fparam, > + struct mtk_dip_hw_frame_job, > + fparam.frameparam); > +} > + > +#endif /* _MTK_DIP_HW_H_ */ > + > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.c > new file mode 100644 > index 000000000000..5456c0b54ad4 > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.c > @@ -0,0 +1,322 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018 MediaTek Inc. > + * Author: Frederic Chen <frederic.chen@mediatek.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/module.h> > +#include <linux/device.h> > +#include <linux/platform_device.h> > +#include <linux/of.h> > +#include <linux/of_fdt.h> > +#include <linux/of_reserved_mem.h> > +#include <linux/dma-contiguous.h> > +#include <linux/dma-mapping.h> > +#include <linux/slab.h> > +#include <linux/err.h> > +#include <linux/iommu.h> > +#include <asm/cacheflush.h> > +#include "mtk_dip-smem.h" > + > +#define MTK_DIP_SMEM_DEV_NAME "MTK-DIP-SMEM" > + > +static struct reserved_mem *dip_reserved_smem; > +static struct dma_map_ops smem_dma_ops; > + > +struct dma_coherent_mem { > + void *virt_base; > + dma_addr_t device_base; > + unsigned long pfn_base; > + int size; > + int flags; > + unsigned long *bitmap; > + spinlock_t spinlock; /* protect dma_coherent_mem member */ > + bool use_dev_dma_pfn_offset; > +}; > + > +static struct dma_coherent_mem *dev_get_coherent_memory(struct device *dev) > +{ > + if (dev && dev->dma_mem) > + return dev->dma_mem; > + return NULL; > +} > + > +phys_addr_t mtk_dip_smem_iova_to_phys(struct mtk_dip_smem_dev *smem_dev, > + dma_addr_t iova) > +{ > + struct iommu_domain *smem_dom; > + phys_addr_t addr; > + phys_addr_t limit; > + > + if (!smem_dev) > + return 0; > + > + smem_dom = iommu_get_domain_for_dev(smem_dev->dev.parent); > + > + if (!smem_dom) > + return 0; > + > + addr = iommu_iova_to_phys(smem_dom, iova); > + > + limit = smem_dev->smem_base + smem_dev->smem_size; > + > + if (addr < smem_dev->smem_base || addr >= limit) { > + dev_err(&smem_dev->dev, > + "Unexpected scp_daddr %pa (must >= %pa and <%pa)\n", > + &addr, &smem_dev->smem_base, &limit); > + return 0; > + } > + dev_dbg(&smem_dev->dev, "Pa verifcation pass: %pa(>=%pa, <%pa)\n", > + &addr, &smem_dev->smem_base, &limit); > + return addr; > +} > + > +/******************************************** > + * MTK DIP SMEM DMA ops * > + ********************************************/ > +static int mtk_dip_smem_get_sgtable(struct device *dev, > + struct sg_table *sgt, > + void *cpu_addr, > + dma_addr_t dma_addr, > + size_t size, unsigned long attrs) > +{ > + struct mtk_dip_smem_dev *smem_dev = dev_get_drvdata(dev); > + int n_pages_align; > + int size_align; > + int page_start; > + unsigned long long offset_p; > + > + phys_addr_t paddr = mtk_dip_smem_iova_to_phys(smem_dev, dma_addr); > + > + offset_p = (unsigned long long)paddr - > + (unsigned long long)smem_dev->smem_base; > + > + dev_dbg(dev, "%s: dma_addr(%p), cpu_addr(%p), pa(%p), size(%d)\n", > + __func__, dma_addr, cpu_addr, paddr, size); Please use %zd for size as it is a size_t. > + > + size_align = round_up(size, PAGE_SIZE); > + n_pages_align = size_align >> PAGE_SHIFT; > + page_start = offset_p >> PAGE_SHIFT; > + > + dev_dbg(dev, "%s: page_start(%d), page pa(%p), pa(%p), aligned size(%d)\n", > + __func__, > + page_start, > + page_to_phys(*(smem_dev->smem_pages + page_start)), > + paddr, > + size_align > + ); > + > + if (!smem_dev) { > + dev_err(dev, "can't get sgtable from smem_dev\n"); > + return -EINVAL; > + } We already dereference smem_dev above. Should we move this check or simply remove it? > + > + dev_dbg(dev, "%s: get sgt of the smem: %d pages\n", __func__, > + n_pages_align); > + > + return sg_alloc_table_from_pages(sgt, > + smem_dev->smem_pages + page_start, > + n_pages_align, > + 0, size_align, GFP_KERNEL); > +} > + <snip> > + > -- > 2.18.0 > Sincerely, Shik
Hi Frederic, On Wed, May 22, 2019 at 03:14:15AM +0800, Frederic Chen wrote: > Dear Tomasz, > > I appreciate your comment. It is very helpful for us. > You're welcome. Thanks for replying to all the comments. I'll skip those resolved in my reply to keep the message shorter. > > On Thu, 2019-05-09 at 18:48 +0900, Tomasz Figa wrote: > > Hi Frederic, > > > > On Wed, Apr 17, 2019 at 7:45 PM Frederic Chen <frederic.chen@mediatek.com> wrote: [snip] > > > + > > > + timestamp = ktime_get_ns(); > > > > This timestamp is unused. > > This line will be removed in the next patch. > > > > > > + > > > + for (i = 0; i < MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM; i++) { > > > + struct mtk_dip_dev_buffer *dev_buf = job_info->buf_map[i]; > > > + > > > + if (!dev_buf) { > > > + continue; > > > + } else { > > > > No need to put this code under else. > > I got it. > > > > > > + dev_buf->vbb.vb2_buf.timestamp = ktime_get_ns(); > > > > Did you mean to use the timestamp variable here? > > I would like to remove timestamp and still use > dev_buf->vbb.vb2_buf.timestamp directly. > That would result in buffers from different queues having different timestamps. However, this is a mem2mem device, so we shouldn't assign timestamps here. We should copy the timestamp from the matching input buffer. I missed that before, sorry. [snip] > > > > > + dev_buf->buffer_usage = node->dev_q.buffer_usage; > > > > There should be no buffer_usage, but rather separate queue for each usage. > > I will remove mtk_dip_dev_buffer.buffer_usage and > mtk_dip_dev_queue.buffer_usage. > > May I add the dma port information (DMA port selection hint) in each > video node's setting (mtk_dip_video_device_desc) so that we can retrieve > the hint when we need to pass the buffer to co-processor as following: > > > static int fill_ipi_img_param(struct mtk_dip_pipe *dip_pipe, > struct img_image_buffer *img, > struct mtk_dip_dev_buffer *dev_buf, > char *buf_name) > { > struct mtk_dip_video_device *vdo_dev; > // ... > vdo_dev = mtk_dip_vbq_to_node(dev_buf->vbb.vb2_buf.vb2_queue); > /* image parameter will be passed to firmware*/ > img->usage = vdo_dev->desc->dma_hint; > // ... > } > Yes, that sounds sensible, but I'm not sure "hint" is a good name here, because the purpose is precisely defined, not just a hint. Perhaps better to call it something like "dma_port"? > > > > > > + dev_buf->rotation = node->dev_q.rotation; > > > + > > > + if (desc->smem_alloc) { > > > + dev_buf->scp_daddr = > > > + mtk_dip_smem_iova_to_phys > > > + (dip_pipe->smem_alloc_dev, > > > + dev_buf->isp_daddr); > > > > Isn't this overly wrapped? The first argument should fit in the second line. > > Do you mean the modification like this: > > dev_buf->scp_daddr = mtk_dip_smem_iova_to_phys > (dip_pipe->smem_alloc_dev, > dev_buf->isp_daddr); Almost. The wrapped lines should be aligned to the right as much as possible, so it would be more like: dev_buf->scp_daddr = mtk_dip_smem_iova_to_phys( dip_pipe->smem_alloc_dev, dev_buf->isp_daddr); [snip] > > > + > > > + if (fjob->sequence == -1) { > > > + pr_err("%s: Invalid cmdq_cb_data(%p)\n", > > > + __func__, data); > > > + ipi_fparam = NULL; > > > > As far as I can see, we always get the data we explicitly gave to the cmdq > > driver together with the callback function, so it sounds like we shouldn't > > be able to get invalid data here. > > > > I'm not sure if we can always trust CMDQ callback since we had > suffered from an CMDQ bug that return an invalid address before. > Hmm, but these values don't really go to the CMDQ hardware or firmware, right? It's just the kernel CMDQ driver that looks the data pointer in its internal state. That would be a bug similar to the workqueue framework giving a bad pointer to the work function. Also, checking for -1 alone doesn't really help us too much, because we could for example get a pointer to some memory filled with random values, in case of such bug. Perhaps we should inspect the CMDQ driver design instead to make sure it's impossible to receive a bad pointer from it. [snip] > > > + > > > + if (WARN_ONCE(!mdpcb_work, "%s: frame_no(%d) is lost", > > > + __func__, frameparam->frame_no)) > > > + return; > > > + > > > + INIT_WORK(&mdpcb_work->frame_work, mdp_cb_worker); > > > + mdpcb_work->frameparams = frameparam; > > > + if (data.sta != CMDQ_CB_NORMAL) > > > + mdpcb_work->frameparams->state = FRAME_STATE_HW_TIMEOUT; > > > + > > > + queue_work(dip_hw->mdpcb_workqueue, &mdpcb_work->frame_work); > > > > Looking at mdp_cb_worker() and mtk_dip_notify() called by it, after fixing > > the locking to use spinlocks rather than mutexes and remove some dynamic > > allocations pointed out in other comments, we could just run them in atomic > > context, without the need for this workqueue. > > I will re-design this part to support the function to be run in > atomic context. > > I would like to discuss the error handling in this function > for the next patch. When we get FRAME_STATE_HW_TIMEOUT, is it > suitable to trigger a work to notify SCP with scp_ipi_send() to > dump debug messages in further? (Can this kind of debugging > function be upstream?) > Yes, we should be able to have something like that. > > > > > +} > > > + > > > +static void dip_vpu_handler(void *data, unsigned int len, void *priv) > > > +{ > > > + struct img_frameparam *framejob; > > > + struct img_ipi_frameparam *frameparam; > > > + struct mtk_dip_hw *dip_hw; > > > + struct mtk_dip_dev *dip_dev; > > > + unsigned long flags; > > > + u32 num; > > > + > > > + if (WARN_ONCE(!data, "%s: failed due to NULL data\n", __func__)) > > > + return; > > > + > > > + frameparam = (struct img_ipi_frameparam *)data; > > > + framejob = dip_create_framejob(frameparam->index); > > > > Rather than creating this dynamically, it should be already created. The > > reason you have the priv argument to this function is that you can have the > > SCP driver give to you a pointer to some struct that you gave to it when > > calling scp_ipi_register(). That struct should have a list of valid VPU jobs > > and here you should iterate through that list and find a matching job or > > fail if there is no matching one. (We need to validate here, because we > > can't trust the values coming from the firmware.) > > May I embedded img_frameparam into the extended media_request object > so that it can be managed with media_request objects? > > struct mtk_dip_request { > struct media_request request; > struct mtk_dip_pipe_job_info job_info; > struct mtk_dip_hw_work dip_work; > struct img_frameparam job; > struct mtk_dip_hw_mdpcb_timeout_work timeout_work; > }; > > Yes, I think it would make sense indeed. [snip] > > > +static int dip_runner_func(void *data) > > > +{ > > > + struct img_frameparam *framejob; > > > + struct mtk_dip_hw *dip_hw; > > > + struct mtk_dip_dev *dip_dev; > > > + struct mtk_dip_hw_user_id *user_id; > > > + unsigned long flags; > > > + bool found; > > > + u32 queuecnt, num; > > > + int ret; > > > + > > > + dip_hw = (struct mtk_dip_hw *)data; > > > + dip_dev = mtk_dip_hw_to_dev(dip_hw); > > > + > > > + while (1) { > > > + spin_lock_irqsave(&dip_hw->dip_gcejoblist.queuelock, flags); > > > + queuecnt = dip_hw->dip_gcejoblist.queue_cnt; > > > + spin_unlock_irqrestore(&dip_hw->dip_gcejoblist.queuelock, > > > + flags); > > > + > > > + ret = wait_event_interruptible_timeout > > > + (dip_hw->dip_runner_thread.wq, > > > + queuecnt || kthread_should_stop(), > > > + msecs_to_jiffies(DIP_COMPOSER_THREAD_TIMEOUT)); > > > > Checking queuecnt here is also wrong. Please check my comment to another > > wait_event_* in this patch. > > > > Also the interruptibility and timeout don't make sense here, because we just > > want to keep waiting unless something shows up or the kthread is forcefully > > woken up for stopping. > > > > However, we probably want it to be freezable(), so that we don't have to > > bring the thread down and up on suspend/resume. > > > I would like to fix it as following: > > > static bool mtk_dip_hw_qce_queuecnt(struct mtk_dip_hw *dip_hw) > { > int queuecnt; > > spin_lock_irqsave(&dip_hw->dip_gcejoblist.queuelock, flags); > queuecnt = dip_hw->dip_gcejoblist.queue_cnt; > spin_unlock_irqrestore(&dip_hw->dip_gcejoblist.queuelock, > > return queuecnt; > } > > static int dip_runner_func(void *data) > { > > //... > set_freezable(); > while(1) { > wait_event_freezable((dip_hw->dip_runner_thread.wq, > mtk_dip_hw_qce_queuecnt() || > kthread_should_stop()); > > if (kthread_should_stop()) > break; > > // Call MDP/GCE API to do HW excecution > // ... > > } > } > Sounds good to me. [snip] > > > + > > > +free_work_list: > > > + > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "%s, free: frame_no(%d),idx(0x%x),worklist cnt(%d),composing num(%d)\n", > > > + __func__, dip_work->frameparams.frame_no, > > > + dip_work->frameparams.index, len, num); > > > + > > > + kfree(dip_work); > > > > We haven't allocated this object and so we shouldn't free it here. The layer > > that allocated it should receive it back and free. (But we probably don't > > need to allocate it dynamically as per my other comments.) > > > > May I also merge dip_work into mtk_dip_request? > > struct mtk_dip_request { > struct media_request request; > struct mtk_dip_pipe_job_info job_info; > struct mtk_dip_hw_work dip_work; > struct img_frameparam job; > struct mtk_dip_hw_mdpcb_timeout_work timeout_work; > }; > Yes, it should indeed make sense. > > > Also a general note - a work can be queued only once. This means that > > current code races when two dip_works are attempted to be queued very > > quickly one after another (or even at the same time from different threads). > > > > I can think of two potential options for fixing this: > > > > 1) Loop in the work function until there is nothing to queue to the hardware > > anymore - but this needs tricky synchronization, because there is still > > short time at the end of the work function when a new dip_work could be > > added. > > > > 2) Change this to a kthread that just keeps running in a loop waiting for > > some available dip_work to show up and then sending it to the firmware. > > This should be simpler, as the kthread shouldn't have a chance to miss > > any dip_work queued. > > > > I'm personally in favor of option 2, as it should simplify the > > synchronization. > > > > I would like to re-design this part with a kthread in the next patch. Actually I missed another option. We could have 1 work_struct for 1 request and then we could keep using a workqueue. Perhaps that could be simpler than a kthread. Actually, similar approach could be used for the dip_runner_func. Instead of having a kthread looping, we could just have another workqueue and 1 dip_runner_work per 1 request. Then we wouldn't need to do the waiting loop ourselves anymore. Does it make sense? [snip] > > > + buf->buffer.iova = sg_dma_address(buf->table.sgl); > > > + buf->tuning_buf.iova = buf->buffer.iova + > > > + DIP_TUNING_OFFSET; > > > + > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "%s: buf(%d), pa(%p), iova(%p)\n", > > > + __func__, i, buf->buffer.pa, buf->buffer.iova); > > > + > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "%s: config_data(%d), pa(%p), iova(%p)\n", > > > + __func__, i, buf->config_data.pa, buf->config_data.va); > > > + > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "%s: tuning_buf(%d), pa(%p), iova(%p)\n", > > > + __func__, i, buf->tuning_buf.pa, buf->tuning_buf.iova); > > > + > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "%s: frameparam(%d), pa(%p), iova(%p)\n", > > > + __func__, i, buf->frameparam.pa, buf->frameparam.va); > > > + > > > + list_add_tail(&buf->list_entry, > > > + &dip_hw->dip_freebufferlist.queue); > > > + dip_hw->dip_freebufferlist.queue_cnt++; > > > + kfree(pages); > > > + } > > > > But actually, why aren't these buffers managed by VB2? > > > > I would like to manage the buffer with VB2, but I'm not sure what is > the right way to do that. > > These are DIP internal working buffers. The buffers are not read or > written by user application. In this case, could I add a meta capture > video device to manage them or integrate the buffers with tuning meta > output buffer directly? > Ah, if these are internal buffers, then I guess we indeed need to allocate them ourselves. [snip] > > > + frameparam.state = FRAME_STATE_INIT; > > > + dip_send(dip_hw->vpu_pdev, SCP_IPI_DIP_INIT, > > > + (void *)&frameparam, sizeof(frameparam), 0); > > > > Is the call above synchronous? If not, don't we need to wait here for SCP to > > initialize? > > > > No, it is not synchronous when passing 0 in the latest parameter. We > would like to improve it by using scp_ipi_send() directly instead of > dip_send(), and wait for the ack from SCP with 2ms timeout setting. > > ret = scp_ipi_send(dip_hw->scp_pdev SCP_IPI_DIP_INIT, > &frameparam, sizeof(frameparam), 2); > 2ms could likely fail here quite often, because even kernel code can be preempted. We should have something like 200ms here, just to avoid blocking indefinitely, but long enough not to happen even during high system load. [snip] > > > > + framework->frameparams.state = FRAME_STATE_INIT; > > > + framework->frameparams.frame_no = > > > + atomic_inc_return(&dip_hw->dip_enque_cnt); > > > > It sounds like the struct passed as the frameparams argument to this > > function and framework->frameparams should be two different structs. > > > > I'm not sure if I understand it. Would you like to elaborate on this > point? > > I would like to merge mtk_dip_hw_enqueue and mtk_dip_pipe_job_start so > that we can configure the framework->frameparams in a single function. > Ah, that comment might have been added before I noticed that the functions can be merged. Forgot to remove, sorry. Please ignore. [snip] > > > +static int __maybe_unused mtk_dip_pm_suspend(struct device *dev) > > > +{ > > > + struct mtk_dip_dev *dip_dev = dev_get_drvdata(dev); > > > + > > > + if (atomic_read(&dip_dev->dip_hw.dip_user_cnt) > 0) { > > > + mtk_dip_hw_set_clk(&dip_dev->dip_hw, false); > > > + dev_dbg(&dip_dev->pdev->dev, "%s: disable clock\n", > > > + __func__); > > > + } > > > > Don't we need to do something to prevent the driver from scheduling next > > frames here and wait for the hardware to finish processing current frame > > here? > > > > I will add some design to handle this case. Could we add a > suspend variable in mtk_dip_hw and check it in submit work kthread? > Actually, with the other changes, the kthread would have been frozen for us at this point, so maybe there isn't anything to do here! :) OR, if we change the kthread to a workqueue (as per my other comment above), we could make the workqueue freezable (by adding WQ_FREEZABLE to the flags argument at creation time) and it would be frozen for us after finishing current work struct. > > > + > > > + return 0; > > > +} > > > + > > > +static int __maybe_unused mtk_dip_pm_resume(struct device *dev) > > > +{ > > > + struct mtk_dip_dev *dip_dev = dev_get_drvdata(dev); > > > + > > > + if (atomic_read(&dip_dev->dip_hw.dip_user_cnt) > 0) { > > > + mtk_dip_hw_set_clk(&dip_dev->dip_hw, true); > > > + dev_dbg(&dip_dev->pdev->dev, "%s: enable clock\n", > > > + __func__); > > > + } > > > > Don't we need to kick off the hardware here to continue processing from > > the next frame? > > > > I will add some design to handle this case. > As per above, making the kthreads/workqueues freezable actually should solve the problem. [snip] > > > +static int mtk_dip_subdev_close(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_fh *fh) > > > +{ > > > + struct mtk_dip_pipe *dip_pipe = mtk_dip_subdev_to_pipe(sd); > > > + struct mtk_dip_dev *dip_dev = > > > + dev_get_drvdata(&dip_pipe->dip_dev->pdev->dev); > > > + > > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > > + "%s:%s: pipe(%d) disconnect to dip_hw\n", > > > + __func__, dip_pipe->desc->name, > > > + dip_pipe->desc->id); > > > + > > > + return mtk_dip_hw_disconnect(&dip_dev->dip_hw); > > > +} > > > > We shouldn't do any hardware operations just at subdev open or close. Those > > should happen once the streaming actually starts. > > > > Userspace is expected to open device nodes just to query them and that > > shouldn't have any side effects. > > > > In general I don't see why we actually should even do anything in subdev ops > > in this driver. The subdev is only exposed to bridge all the video nodes > > together and any control is done via either video ioctls or metadata > > buffers. > > > > I will move the hardware operation to streamon. We call rproc_boot() to > load frimware in mtk_dip_hw_connect(), should it be moved to streamon, > too? > I think that would normally be done in runtime PM callbacks, with autosuspend delay enabled to avoid continuous power-off and on repeateadly. Then, when there is a request to queue, the driver would call pm_runtime_get_sync() and when a request completes, pm_runtime_put_autosuspend(). [snip] > > > +static void mtk_dip_node_to_v4l2(struct mtk_dip_pipe *dip_pipe, > > > + u32 idx, > > > + struct video_device *vdev, > > > + struct v4l2_format *f) > > > +{ > > > + u32 cap; > > > + struct mtk_dip_video_device *node = &dip_pipe->nodes[idx]; > > > + > > > + cap = mtk_dip_node_get_v4l2_cap(dip_pipe, node); > > > + vdev->ioctl_ops = node->desc->ops; > > > + vdev->device_caps = V4L2_CAP_STREAMING | cap; > > > > Why not just have mtk_dip_node_get_v4l2_cap() include V4L2_CAP_STREAMING in > > the return value? > > > > I would like to set the V4L2_CAP_STREAMING in node->desc->cap directly. > > vdev->device_caps = node->desc->caps; > Sure, that's also good. Best regards, Tomasz
Dear Tomasz, Thank you for your comments. On Wed, 2019-05-22 at 19:25 +0900, Tomasz Figa wrote: > Hi Frederic, > > On Wed, May 22, 2019 at 03:14:15AM +0800, Frederic Chen wrote: > > Dear Tomasz, > > > > I appreciate your comment. It is very helpful for us. > > > > You're welcome. Thanks for replying to all the comments. I'll skip those > resolved in my reply to keep the message shorter. > > > > > On Thu, 2019-05-09 at 18:48 +0900, Tomasz Figa wrote: > > > Hi Frederic, > > > > > > On Wed, Apr 17, 2019 at 7:45 PM Frederic Chen <frederic.chen@mediatek.com> wrote: > [snip] > > > > + > > > > + timestamp = ktime_get_ns(); > > > > > > This timestamp is unused. > > > > This line will be removed in the next patch. > > > > > > > > > + > > > > + for (i = 0; i < MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM; i++) { > > > > + struct mtk_dip_dev_buffer *dev_buf = job_info->buf_map[i]; > > > > + > > > > + if (!dev_buf) { > > > > + continue; > > > > + } else { > > > > > > No need to put this code under else. > > > > I got it. > > > > > > > > > + dev_buf->vbb.vb2_buf.timestamp = ktime_get_ns(); > > > > > > Did you mean to use the timestamp variable here? > > > > I would like to remove timestamp and still use > > dev_buf->vbb.vb2_buf.timestamp directly. > > > > That would result in buffers from different queues having different > timestamps. > > However, this is a mem2mem device, so we shouldn't assign timestamps > here. We should copy the timestamp from the matching input buffer. I > missed that before, sorry. I will copy the raw input buffer's timestamps to the mdp 0 and mdp 1 buffer's ones. > > [snip] > > > > > > > > + dev_buf->buffer_usage = node->dev_q.buffer_usage; > > > > > > There should be no buffer_usage, but rather separate queue for each usage. > > > > I will remove mtk_dip_dev_buffer.buffer_usage and > > mtk_dip_dev_queue.buffer_usage. > > > > May I add the dma port information (DMA port selection hint) in each > > video node's setting (mtk_dip_video_device_desc) so that we can retrieve > > the hint when we need to pass the buffer to co-processor as following: > > > > > > static int fill_ipi_img_param(struct mtk_dip_pipe *dip_pipe, > > struct img_image_buffer *img, > > struct mtk_dip_dev_buffer *dev_buf, > > char *buf_name) > > { > > struct mtk_dip_video_device *vdo_dev; > > // ... > > vdo_dev = mtk_dip_vbq_to_node(dev_buf->vbb.vb2_buf.vb2_queue); > > /* image parameter will be passed to firmware*/ > > img->usage = vdo_dev->desc->dma_hint; > > // ... > > } > > > > Yes, that sounds sensible, but I'm not sure "hint" is a good name here, > because the purpose is precisely defined, not just a hint. Perhaps > better to call it something like "dma_port"? > Yes, I would like to use "dma_port" since it is better. > > > > > > > > > + dev_buf->rotation = node->dev_q.rotation; > > > > + > > > > + if (desc->smem_alloc) { > > > > + dev_buf->scp_daddr = > > > > + mtk_dip_smem_iova_to_phys > > > > + (dip_pipe->smem_alloc_dev, > > > > + dev_buf->isp_daddr); > > > > > > Isn't this overly wrapped? The first argument should fit in the second line. > > > > Do you mean the modification like this: > > > > dev_buf->scp_daddr = mtk_dip_smem_iova_to_phys > > (dip_pipe->smem_alloc_dev, > > dev_buf->isp_daddr); > > Almost. The wrapped lines should be aligned to the right as much as > possible, so it would be more like: > > dev_buf->scp_daddr = mtk_dip_smem_iova_to_phys( > dip_pipe->smem_alloc_dev, > dev_buf->isp_daddr); > I got it. > [snip] > > > > + > > > > + if (fjob->sequence == -1) { > > > > + pr_err("%s: Invalid cmdq_cb_data(%p)\n", > > > > + __func__, data); > > > > + ipi_fparam = NULL; > > > > > > As far as I can see, we always get the data we explicitly gave to the cmdq > > > driver together with the callback function, so it sounds like we shouldn't > > > be able to get invalid data here. > > > > > > > I'm not sure if we can always trust CMDQ callback since we had > > suffered from an CMDQ bug that return an invalid address before. > > > > Hmm, but these values don't really go to the CMDQ hardware or firmware, > right? It's just the kernel CMDQ driver that looks the data pointer in > its internal state. That would be a bug similar to the workqueue > framework giving a bad pointer to the work function. > > Also, checking for -1 alone doesn't really help us too much, because we > could for example get a pointer to some memory filled with random > values, in case of such bug. > > Perhaps we should inspect the CMDQ driver design instead to make sure > it's impossible to receive a bad pointer from it. > I agree with you. I will remove the fjob->sequence check codes. > [snip] > > > > > + > > > > + if (WARN_ONCE(!mdpcb_work, "%s: frame_no(%d) is lost", > > > > + __func__, frameparam->frame_no)) > > > > + return; > > > > + > > > > + INIT_WORK(&mdpcb_work->frame_work, mdp_cb_worker); > > > > + mdpcb_work->frameparams = frameparam; > > > > + if (data.sta != CMDQ_CB_NORMAL) > > > > + mdpcb_work->frameparams->state = FRAME_STATE_HW_TIMEOUT; > > > > + > > > > + queue_work(dip_hw->mdpcb_workqueue, &mdpcb_work->frame_work); > > > > > > Looking at mdp_cb_worker() and mtk_dip_notify() called by it, after fixing > > > the locking to use spinlocks rather than mutexes and remove some dynamic > > > allocations pointed out in other comments, we could just run them in atomic > > > context, without the need for this workqueue. > > > > I will re-design this part to support the function to be run in > > atomic context. > > > > I would like to discuss the error handling in this function > > for the next patch. When we get FRAME_STATE_HW_TIMEOUT, is it > > suitable to trigger a work to notify SCP with scp_ipi_send() to > > dump debug messages in further? (Can this kind of debugging > > function be upstream?) > > > > Yes, we should be able to have something like that. > > > > > > > > +} > > > > + > > > > +static void dip_vpu_handler(void *data, unsigned int len, void *priv) > > > > +{ > > > > + struct img_frameparam *framejob; > > > > + struct img_ipi_frameparam *frameparam; > > > > + struct mtk_dip_hw *dip_hw; > > > > + struct mtk_dip_dev *dip_dev; > > > > + unsigned long flags; > > > > + u32 num; > > > > + > > > > + if (WARN_ONCE(!data, "%s: failed due to NULL data\n", __func__)) > > > > + return; > > > > + > > > > + frameparam = (struct img_ipi_frameparam *)data; > > > > + framejob = dip_create_framejob(frameparam->index); > > > > > > Rather than creating this dynamically, it should be already created. The > > > reason you have the priv argument to this function is that you can have the > > > SCP driver give to you a pointer to some struct that you gave to it when > > > calling scp_ipi_register(). That struct should have a list of valid VPU jobs > > > and here you should iterate through that list and find a matching job or > > > fail if there is no matching one. (We need to validate here, because we > > > can't trust the values coming from the firmware.) > > > > May I embedded img_frameparam into the extended media_request object > > so that it can be managed with media_request objects? > > > > struct mtk_dip_request { > > struct media_request request; > > struct mtk_dip_pipe_job_info job_info; > > struct mtk_dip_hw_work dip_work; > > struct img_frameparam job; > > struct mtk_dip_hw_mdpcb_timeout_work timeout_work; > > }; > > > > > > Yes, I think it would make sense indeed. > > [snip] > > > > > +static int dip_runner_func(void *data) > > > > +{ > > > > + struct img_frameparam *framejob; > > > > + struct mtk_dip_hw *dip_hw; > > > > + struct mtk_dip_dev *dip_dev; > > > > + struct mtk_dip_hw_user_id *user_id; > > > > + unsigned long flags; > > > > + bool found; > > > > + u32 queuecnt, num; > > > > + int ret; > > > > + > > > > + dip_hw = (struct mtk_dip_hw *)data; > > > > + dip_dev = mtk_dip_hw_to_dev(dip_hw); > > > > + > > > > + while (1) { > > > > + spin_lock_irqsave(&dip_hw->dip_gcejoblist.queuelock, flags); > > > > + queuecnt = dip_hw->dip_gcejoblist.queue_cnt; > > > > + spin_unlock_irqrestore(&dip_hw->dip_gcejoblist.queuelock, > > > > + flags); > > > > + > > > > + ret = wait_event_interruptible_timeout > > > > + (dip_hw->dip_runner_thread.wq, > > > > + queuecnt || kthread_should_stop(), > > > > + msecs_to_jiffies(DIP_COMPOSER_THREAD_TIMEOUT)); > > > > > > Checking queuecnt here is also wrong. Please check my comment to another > > > wait_event_* in this patch. > > > > > > Also the interruptibility and timeout don't make sense here, because we just > > > want to keep waiting unless something shows up or the kthread is forcefully > > > woken up for stopping. > > > > > > However, we probably want it to be freezable(), so that we don't have to > > > bring the thread down and up on suspend/resume. > > > > > > I would like to fix it as following: > > > > > > static bool mtk_dip_hw_qce_queuecnt(struct mtk_dip_hw *dip_hw) > > { > > int queuecnt; > > > > spin_lock_irqsave(&dip_hw->dip_gcejoblist.queuelock, flags); > > queuecnt = dip_hw->dip_gcejoblist.queue_cnt; > > spin_unlock_irqrestore(&dip_hw->dip_gcejoblist.queuelock, > > > > return queuecnt; > > } > > > > static int dip_runner_func(void *data) > > { > > > > //... > > set_freezable(); > > while(1) { > > wait_event_freezable((dip_hw->dip_runner_thread.wq, > > mtk_dip_hw_qce_queuecnt() || > > kthread_should_stop()); > > > > if (kthread_should_stop()) > > break; > > > > // Call MDP/GCE API to do HW excecution > > // ... > > > > } > > } > > > > Sounds good to me. > > [snip] > > > > > + > > > > +free_work_list: > > > > + > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > + "%s, free: frame_no(%d),idx(0x%x),worklist cnt(%d),composing num(%d)\n", > > > > + __func__, dip_work->frameparams.frame_no, > > > > + dip_work->frameparams.index, len, num); > > > > + > > > > + kfree(dip_work); > > > > > > We haven't allocated this object and so we shouldn't free it here. The layer > > > that allocated it should receive it back and free. (But we probably don't > > > need to allocate it dynamically as per my other comments.) > > > > > > > May I also merge dip_work into mtk_dip_request? > > > > struct mtk_dip_request { > > struct media_request request; > > struct mtk_dip_pipe_job_info job_info; > > struct mtk_dip_hw_work dip_work; > > struct img_frameparam job; > > struct mtk_dip_hw_mdpcb_timeout_work timeout_work; > > }; > > > > Yes, it should indeed make sense. > > > > > > Also a general note - a work can be queued only once. This means that > > > current code races when two dip_works are attempted to be queued very > > > quickly one after another (or even at the same time from different threads). > > > > > > I can think of two potential options for fixing this: > > > > > > 1) Loop in the work function until there is nothing to queue to the hardware > > > anymore - but this needs tricky synchronization, because there is still > > > short time at the end of the work function when a new dip_work could be > > > added. > > > > > > 2) Change this to a kthread that just keeps running in a loop waiting for > > > some available dip_work to show up and then sending it to the firmware. > > > This should be simpler, as the kthread shouldn't have a chance to miss > > > any dip_work queued. > > > > > > I'm personally in favor of option 2, as it should simplify the > > > synchronization. > > > > > > > I would like to re-design this part with a kthread in the next patch. > > Actually I missed another option. We could have 1 work_struct for 1 > request and then we could keep using a workqueue. Perhaps that could be > simpler than a kthread. > > Actually, similar approach could be used for the dip_runner_func. > Instead of having a kthread looping, we could just have another > workqueue and 1 dip_runner_work per 1 request. Then we wouldn't need to > do the waiting loop ourselves anymore. > > Does it make sense? Yes, it make sense. Let me summarize the modification about the flow. First, we will have two work_struct in mtk_dip_request. struct mtk_dip_request { struct media_request request; //... /* Prepare DIP part hardware configurtion */ struct mtk_dip_hw_submit_work submit_work; /* Replace dip_running thread jobs*/ struct mtk_dip_hw_composing_work composing_work; /* Only for composing error handling */ struct mtk_dip_hw_mdpcb_timeout_work timeout_work; }; Second, the overall flow of handling each request is : 1. mtk_dip_hw_enqueue calls queue_work() to put submit_work into its workqueue 2. submit_work sends IMG_IPI_FRAME command to SCP to prepare DIP hardware configuration 3. dip_scp_handler receives the IMG_IPI_FRAME result from SCP 4. dip_scp_handler calls queue_work() to put composing_work (instead of original dip_running thread jobs) into its workqueue 5. composing_work calls dip_mdp_cmdq_send() to finish the mdp part tasks 6. dip_mdp_cb_func() trigged by MDP driver calls vb2_buffer_done to return the buffer (no workqueue required here) > > [snip] > > > > > + buf->buffer.iova = sg_dma_address(buf->table.sgl); > > > > + buf->tuning_buf.iova = buf->buffer.iova + > > > > + DIP_TUNING_OFFSET; > > > > + > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > + "%s: buf(%d), pa(%p), iova(%p)\n", > > > > + __func__, i, buf->buffer.pa, buf->buffer.iova); > > > > + > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > + "%s: config_data(%d), pa(%p), iova(%p)\n", > > > > + __func__, i, buf->config_data.pa, buf->config_data.va); > > > > + > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > + "%s: tuning_buf(%d), pa(%p), iova(%p)\n", > > > > + __func__, i, buf->tuning_buf.pa, buf->tuning_buf.iova); > > > > + > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > + "%s: frameparam(%d), pa(%p), iova(%p)\n", > > > > + __func__, i, buf->frameparam.pa, buf->frameparam.va); > > > > + > > > > + list_add_tail(&buf->list_entry, > > > > + &dip_hw->dip_freebufferlist.queue); > > > > + dip_hw->dip_freebufferlist.queue_cnt++; > > > > + kfree(pages); > > > > + } > > > > > > But actually, why aren't these buffers managed by VB2? > > > > > > > I would like to manage the buffer with VB2, but I'm not sure what is > > the right way to do that. > > > > These are DIP internal working buffers. The buffers are not read or > > written by user application. In this case, could I add a meta capture > > video device to manage them or integrate the buffers with tuning meta > > output buffer directly? > > > > Ah, if these are internal buffers, then I guess we indeed need to > allocate them ourselves. I got it. > > [snip] > > > > > + frameparam.state = FRAME_STATE_INIT; > > > > + dip_send(dip_hw->vpu_pdev, SCP_IPI_DIP_INIT, > > > > + (void *)&frameparam, sizeof(frameparam), 0); > > > > > > Is the call above synchronous? If not, don't we need to wait here for SCP to > > > initialize? > > > > > > > No, it is not synchronous when passing 0 in the latest parameter. We > > would like to improve it by using scp_ipi_send() directly instead of > > dip_send(), and wait for the ack from SCP with 2ms timeout setting. > > > > ret = scp_ipi_send(dip_hw->scp_pdev SCP_IPI_DIP_INIT, > > &frameparam, sizeof(frameparam), 2); > > > > 2ms could likely fail here quite often, because even kernel code can be > preempted. We should have something like 200ms here, just to avoid > blocking indefinitely, but long enough not to happen even during high > system load. > I got it. > [snip] > > > > > > > + framework->frameparams.state = FRAME_STATE_INIT; > > > > + framework->frameparams.frame_no = > > > > + atomic_inc_return(&dip_hw->dip_enque_cnt); > > > > > > It sounds like the struct passed as the frameparams argument to this > > > function and framework->frameparams should be two different structs. > > > > > > > I'm not sure if I understand it. Would you like to elaborate on this > > point? > > > > I would like to merge mtk_dip_hw_enqueue and mtk_dip_pipe_job_start so > > that we can configure the framework->frameparams in a single function. > > > > Ah, that comment might have been added before I noticed that the > functions can be merged. Forgot to remove, sorry. Please ignore. > > [snip] > > > > > +static int __maybe_unused mtk_dip_pm_suspend(struct device *dev) > > > > +{ > > > > + struct mtk_dip_dev *dip_dev = dev_get_drvdata(dev); > > > > + > > > > + if (atomic_read(&dip_dev->dip_hw.dip_user_cnt) > 0) { > > > > + mtk_dip_hw_set_clk(&dip_dev->dip_hw, false); > > > > + dev_dbg(&dip_dev->pdev->dev, "%s: disable clock\n", > > > > + __func__); > > > > + } > > > > > > Don't we need to do something to prevent the driver from scheduling next > > > frames here and wait for the hardware to finish processing current frame > > > here? > > > > > > > I will add some design to handle this case. Could we add a > > suspend variable in mtk_dip_hw and check it in submit work kthread? > > > > Actually, with the other changes, the kthread would have been frozen for > us at this point, so maybe there isn't anything to do here! :) > > OR, if we change the kthread to a workqueue (as per my other comment > above), we could make the workqueue freezable (by adding WQ_FREEZABLE to > the flags argument at creation time) and it would be frozen for us after > finishing current work struct. > Thank you for the advice. I would like to use workqueue with WQ_FREEZABLE. > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int __maybe_unused mtk_dip_pm_resume(struct device *dev) > > > > +{ > > > > + struct mtk_dip_dev *dip_dev = dev_get_drvdata(dev); > > > > + > > > > + if (atomic_read(&dip_dev->dip_hw.dip_user_cnt) > 0) { > > > > + mtk_dip_hw_set_clk(&dip_dev->dip_hw, true); > > > > + dev_dbg(&dip_dev->pdev->dev, "%s: enable clock\n", > > > > + __func__); > > > > + } > > > > > > Don't we need to kick off the hardware here to continue processing from > > > the next frame? > > > > > > > I will add some design to handle this case. > > > > As per above, making the kthreads/workqueues freezable actually should > solve the problem. > > [snip] > > > > > +static int mtk_dip_subdev_close(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_fh *fh) > > > > +{ > > > > + struct mtk_dip_pipe *dip_pipe = mtk_dip_subdev_to_pipe(sd); > > > > + struct mtk_dip_dev *dip_dev = > > > > + dev_get_drvdata(&dip_pipe->dip_dev->pdev->dev); > > > > + > > > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > > > + "%s:%s: pipe(%d) disconnect to dip_hw\n", > > > > + __func__, dip_pipe->desc->name, > > > > + dip_pipe->desc->id); > > > > + > > > > + return mtk_dip_hw_disconnect(&dip_dev->dip_hw); > > > > +} > > > > > > We shouldn't do any hardware operations just at subdev open or close. Those > > > should happen once the streaming actually starts. > > > > > > Userspace is expected to open device nodes just to query them and that > > > shouldn't have any side effects. > > > > > > In general I don't see why we actually should even do anything in subdev ops > > > in this driver. The subdev is only exposed to bridge all the video nodes > > > together and any control is done via either video ioctls or metadata > > > buffers. > > > > > > > I will move the hardware operation to streamon. We call rproc_boot() to > > load frimware in mtk_dip_hw_connect(), should it be moved to streamon, > > too? > > > > I think that would normally be done in runtime PM callbacks, with > autosuspend delay enabled to avoid continuous power-off and on > repeateadly. > > Then, when there is a request to queue, the driver would call > pm_runtime_get_sync() and when a request completes, > pm_runtime_put_autosuspend(). > I got it. I will put it in PM callbacks and use pm_runtime_get_sync() and pm_runtime_put_autosuspend() in request handling flow. > [snip] > > > > > +static void mtk_dip_node_to_v4l2(struct mtk_dip_pipe *dip_pipe, > > > > + u32 idx, > > > > + struct video_device *vdev, > > > > + struct v4l2_format *f) > > > > +{ > > > > + u32 cap; > > > > + struct mtk_dip_video_device *node = &dip_pipe->nodes[idx]; > > > > + > > > > + cap = mtk_dip_node_get_v4l2_cap(dip_pipe, node); > > > > + vdev->ioctl_ops = node->desc->ops; > > > > + vdev->device_caps = V4L2_CAP_STREAMING | cap; > > > > > > Why not just have mtk_dip_node_get_v4l2_cap() include V4L2_CAP_STREAMING in > > > the return value? > > > > > > > I would like to set the V4L2_CAP_STREAMING in node->desc->cap directly. > > > > vdev->device_caps = node->desc->caps; > > > > Sure, that's also good. > > Best regards, > Tomasz Sincerely, Frederic Chen
Hi Shilk, I appreciate your comments. On Wed, 2019-05-22 at 17:51 +0800, Shik Chen wrote: > Hi Frederic, > > Most reviews are already covered by Tomasz, here are some small missing pieces. > Please see my comments inline. > > On Wed, Apr 17, 2019 at 06:45:11PM +0800, Frederic Chen wrote: > > This patch adds the driver of Digital Image Processing (DIP) > > unit in Mediatek ISP system, providing image format > > conversion, resizing, and rotation features. > > > > The mtk-isp directory will contain drivers for multiple IP > > blocks found in Mediatek ISP system. It will include ISP > > Pass 1 driver(CAM), sensor interface driver, DIP driver and > > face detection driver. > > > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com> > > --- > > drivers/media/platform/mtk-isp/Makefile | 18 + > > .../media/platform/mtk-isp/isp_50/Makefile | 17 + > > .../platform/mtk-isp/isp_50/dip/Makefile | 32 + > > .../mtk-isp/isp_50/dip/mtk_dip-ctrl.c | 124 ++ > > .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.c | 1116 +++++++++++++ > > .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.h | 321 ++++ > > .../platform/mtk-isp/isp_50/dip/mtk_dip-hw.h | 167 ++ > > .../mtk-isp/isp_50/dip/mtk_dip-smem.c | 322 ++++ > > .../mtk-isp/isp_50/dip/mtk_dip-smem.h | 39 + > > .../platform/mtk-isp/isp_50/dip/mtk_dip-sys.c | 1384 +++++++++++++++++ > > .../mtk-isp/isp_50/dip/mtk_dip-v4l2.c | 1310 ++++++++++++++++ > > 11 files changed, 4850 insertions(+) > > create mode 100644 drivers/media/platform/mtk-isp/Makefile > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/Makefile > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/Makefile > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-hw.h > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.c > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.h > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c > > create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c > > > > diff --git a/drivers/media/platform/mtk-isp/Makefile b/drivers/media/platform/mtk-isp/Makefile > > new file mode 100644 > > index 000000000000..24bc5354e2f6 > > --- /dev/null > > +++ b/drivers/media/platform/mtk-isp/Makefile > > @@ -0,0 +1,18 @@ > > +# > > +# Copyright (C) 2018 MediaTek Inc. > > +# > > +# This program is free software: you can redistribute it and/or modify > > +# it under the terms of the GNU General Public License version 2 as > > +# published by the Free Software Foundation. > > +# > > +# This program is distributed in the hope that it will be useful, > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > +# GNU General Public License for more details. > > +# > > + > > +obj-$(CONFIG_VIDEO_MEDIATEK_ISP_COMMON) += common/ > > + > > +obj-y += isp_50/ > > + > > +obj-$(CONFIG_VIDEO_MEDIATEK_ISP_FD_SUPPORT) += fd/ > > diff --git a/drivers/media/platform/mtk-isp/isp_50/Makefile b/drivers/media/platform/mtk-isp/isp_50/Makefile > > new file mode 100644 > > index 000000000000..fd0e5bd3c781 > > --- /dev/null > > +++ b/drivers/media/platform/mtk-isp/isp_50/Makefile > > @@ -0,0 +1,17 @@ > > +# > > +# Copyright (C) 2018 MediaTek Inc. > > +# > > +# This program is free software: you can redistribute it and/or modify > > +# it under the terms of the GNU General Public License version 2 as > > +# published by the Free Software Foundation. > > +# > > +# This program is distributed in the hope that it will be useful, > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > +# GNU General Public License for more details. > > +# > > + > > +ifeq ($(CONFIG_VIDEO_MEDIATEK_ISP_DIP_SUPPORT),y) > > +obj-y += dip/ > > +endif > > + > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/Makefile b/drivers/media/platform/mtk-isp/isp_50/dip/Makefile > > new file mode 100644 > > index 000000000000..03137416857b > > --- /dev/null > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/Makefile > > @@ -0,0 +1,32 @@ > > +# > > +# Copyright (C) 2018 MediaTek Inc. > > +# > > +# This program is free software: you can redistribute it and/or modify > > +# it under the terms of the GNU General Public License version 2 as > > +# published by the Free Software Foundation. > > +# > > +# This program is distributed in the hope that it will be useful, > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > +# GNU General Public License for more details. > > +# > > +$(info $(srctree)) > > +ccflags-y += -I$(srctree)/drivers/media/platform/mtk-mdp3 > > + > > +obj-y += mtk_dip-sys.o > > + > > +# To provide alloc context managing memory shared > > +# between CPU and ISP coprocessor > > +mtk_dip_smem-objs := \ > > +mtk_dip-smem.o > > + > > +obj-y += mtk_dip_smem.o > > + > > +# Utilits to provide frame-based streaming model > > typo? should be Utilities. > I will correct it in the next patch. > > +# with v4l2 user interfaces > > +mtk_dip_util-objs := \ > > +mtk_dip-dev.o \ > > +mtk_dip-v4l2.o \ > > +mtk_dip-ctrl.o \ > > + > > +obj-y += mtk_dip_util.o > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c > > new file mode 100644 > > index 000000000000..e35574818120 > > --- /dev/null > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c > > @@ -0,0 +1,124 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2018 MediaTek Inc. > > + * Author: Frederic Chen <frederic.chen@mediatek.com> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#include <linux/device.h> > > +#include <linux/platform_device.h> > > +#include "mtk_dip-dev.h" > > + > > +static void handle_buf_usage_config(struct v4l2_ctrl *ctrl) > > +{ > > + struct mtk_dip_video_device *node = > > + container_of(ctrl->handler, > > + struct mtk_dip_video_device, ctrl_handler); > > + > > + if (ctrl->val < MTK_DIP_V4l2_BUF_USAGE_DEFAULT || > > + ctrl->val >= MTK_DIP_V4l2_BUF_USAGE_NONE) { > > + pr_err("Invalid buffer usage id %d", ctrl->val); > > + return; > > + } > > + node->dev_q.buffer_usage = ctrl->val; > > +} > > + > > +static void handle_buf_rotate_config(struct v4l2_ctrl *ctrl) > > +{ > > + struct mtk_dip_video_device *node = > > + container_of(ctrl->handler, > > + struct mtk_dip_video_device, ctrl_handler); > > + > > + if (ctrl->val != 0 || ctrl->val != 90 || > > + ctrl->val != 180 || ctrl->val != 270) { > > Should we use "and" here instead of "or"? Yes, it should be "and". I will fix it. > > > + pr_err("Invalid buffer rotation %d", ctrl->val); > > + return; > > + } > > + node->dev_q.rotation = ctrl->val; > > +} > > + > > +static int mtk_dip_video_device_s_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + switch (ctrl->id) { > > + case V4L2_CID_PRIVATE_SET_BUFFER_USAGE: > > + handle_buf_usage_config(ctrl); > > + break; > > + case V4L2_CID_ROTATE: > > + handle_buf_rotate_config(ctrl); > > + break; > > + default: > > + break; > > redundant indentation? > I will correct it. > > + } > > + return 0; > > +} > > + > > +static const struct v4l2_ctrl_ops mtk_dip_video_device_ctrl_ops = { > > + .s_ctrl = mtk_dip_video_device_s_ctrl, > > +}; > > + > > +static const struct v4l2_ctrl_config mtk_dip_buf_usage_config = { > > + .ops = &mtk_dip_video_device_ctrl_ops, > > + .id = V4L2_CID_PRIVATE_SET_BUFFER_USAGE, > > + .name = "MTK ISP SET BUFFER USAGE", > > + .type = V4L2_CTRL_TYPE_INTEGER, > > + .min = MTK_DIP_V4l2_BUF_USAGE_DEFAULT, > > + .max = MTK_DIP_V4l2_BUF_USAGE_POSTPROC, > > + .step = 1, > > + .def = MTK_DIP_V4l2_BUF_USAGE_DEFAULT, > > + .flags = V4L2_CTRL_FLAG_SLIDER | V4L2_CTRL_FLAG_EXECUTE_ON_WRITE, > > + }; > > + > > +int mtk_dip_ctrl_init(struct mtk_dip_pipe *dip_pipe) > > +{ > > + struct v4l2_ctrl_handler *hdl = &dip_pipe->ctrl_handler; > > + struct mtk_dip_video_device *node; > > + int i; > > + int img_nodes_to_be_init[3] = { > > + MTK_DIP_VIDEO_NODE_ID_RAW_OUT, > > + MTK_DIP_VIDEO_NODE_ID_MDP0_CAPTURE, > > + MTK_DIP_VIDEO_NODE_ID_MDP1_CAPTURE > > + }; > > + > > + v4l2_ctrl_handler_init(hdl, V4L2_CID_MTK_DIP_MAX); > > + > > + pr_debug("%s init ctrl: %p\n", __func__, hdl); > > + > > + if (hdl->error) { > > + pr_err("Failed in v4l2_ctrl_handler_init\n"); > > + return hdl->error; > > + } > > + > > + for (i = 0; i < MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM; i++) > > + v4l2_ctrl_handler_init(&dip_pipe->nodes[i].ctrl_handler, > > + V4L2_CID_MTK_DIP_MAX); > > + > > + for (i = 0; i < ARRAY_SIZE(img_nodes_to_be_init); i++) { > > + node = &dip_pipe->nodes[img_nodes_to_be_init[i]]; > > + > > + if (v4l2_ctrl_new_custom(&node->ctrl_handler, > > + &mtk_dip_buf_usage_config, > > + NULL) == NULL) > > + dev_err(&dip_pipe->dip_dev->pdev->dev, > > + "Node(%s) create buf_usage_config ctrl failed:(%d)", > > + node->desc->name, > > + node->ctrl_handler.error); > > + > > + if (v4l2_ctrl_new_std(&dip_pipe->ctrl_handler, > > + &mtk_dip_video_device_ctrl_ops, > > + V4L2_CID_ROTATE, 0, 270, 90, 0) == NULL) > > + dev_err(&dip_pipe->dip_dev->pdev->dev, > > + "Node(%s) create rotate ctrl failed:(%d)", > > + node->desc->name, node->ctrl_handler.error); > > + } > > + > > +return 0; > > missing indentation? > I will correct it. > > +} > > +EXPORT_SYMBOL_GPL(mtk_dip_ctrl_init); > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c > > new file mode 100644 > > index 000000000000..9f450dae2820 > > --- /dev/null > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c > > @@ -0,0 +1,1116 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2018 MediaTek Inc. > > + * Author: Frederic Chen <frederic.chen@mediatek.com> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/device.h> > > +#include <linux/platform_device.h> > > +#include <media/videobuf2-dma-contig.h> > > +#include <linux/device.h> > > +#include <linux/platform_device.h> > > +#include <media/videobuf2-dma-contig.h> > > +#include <linux/dma-mapping.h> > > +#include <media/v4l2-event.h> > > +#include "mtk_dip-dev.h" > > +#include "mtk_dip-smem.h" > > +#include "mtk-mdp3-regs.h" > > +#include "mtk-img-ipi.h" > > + > > +int mtk_dip_pipe_init(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_dev *dip_dev, > > + struct mtk_dip_pipe_desc *setting, > > + struct media_device *media_dev, > > + struct v4l2_device *v4l2_dev, > > + struct mtk_dip_smem_dev *smem_alloc_dev) > > +{ > > + int ret, i; > > + > > + dip_pipe->dip_dev = dip_dev; > > + dip_pipe->desc = setting; > > + dip_pipe->smem_alloc_dev = smem_alloc_dev; > > + > > + atomic_set(&dip_pipe->pipe_job_sequence, 0); > > + spin_lock_init(&dip_pipe->job_lock); > > + mutex_init(&dip_pipe->lock); > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, "init pipe(%s,%d)\n", > > + dip_pipe->desc->name, > > + dip_pipe->desc->id); > > + > > + dip_pipe->num_nodes = MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM; > > + > > + for (i = 0; i < MTK_DIP_VIDEO_NODE_ID_OUT_TOTAL_NUM; i++) { > > + dip_pipe->nodes[i].desc = > > + &dip_pipe->desc->output_queue_descs[i]; > > + dip_pipe->nodes[i].immutable = 0; > > + dip_pipe->nodes[i].enabled = > > + dip_pipe->nodes[i].desc->default_enable; > > + atomic_set(&dip_pipe->nodes[i].sequence, 0); > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "%s: init node(%s,%d)\n", > > + dip_pipe->desc->name, > > + dip_pipe->nodes[i].desc->name, i); > > + } > > + > > + for (i = MTK_DIP_VIDEO_NODE_ID_OUT_TOTAL_NUM; > > + i < MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM; i++) { > > + int cap_idx = i - MTK_DIP_VIDEO_NODE_ID_OUT_TOTAL_NUM; > > + > > + dip_pipe->nodes[i].desc = > > + &dip_pipe->desc->capture_queue_descs[cap_idx]; > > + dip_pipe->nodes[i].immutable = 0; > > + dip_pipe->nodes[i].enabled = > > + dip_pipe->nodes[i].desc->default_enable; > > + atomic_set(&dip_pipe->nodes[i].sequence, 0); > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "%s: init node(%s,%d)\n", > > + dip_pipe->desc->name, > > + dip_pipe->nodes[i].desc->name, i); > > + } > > Can we merge these two for loops and check if i < OUT_TOTAL_NUM inside the loop > to remove some duplicate code? Yes. I will merge them to remove duplicate code. > > > + > > + if (dip_pipe->desc->master >= 0 && > > + dip_pipe->desc->master < MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM) { > > + dip_pipe->nodes[dip_pipe->desc->master].immutable = 1; > > + dip_pipe->nodes[dip_pipe->desc->master].enabled = 1; > > + } > > + > > + ret = mtk_dip_ctrl_init(dip_pipe); > > + > > + if (ret) { > > + dev_err(&dip_pipe->dip_dev->pdev->dev, > > + "%s: failed(%d) to initialize ctrls\n", > > + dip_pipe->desc->name, ret); > > + goto failed_ctrl; > > + } > > + > > + ret = mtk_dip_pipe_v4l2_register(dip_pipe, media_dev, v4l2_dev); > > + > > + if (ret) { > > + dev_err(&dip_pipe->dip_dev->pdev->dev, > > + "%s: failed(%d) to create V4L2 devices\n", > > + dip_pipe->desc->name, ret); > > + goto failed_pipe; > > + } > > + > > + return 0; > > + > > +failed_ctrl: > > +failed_pipe: > > + mutex_destroy(&dip_pipe->lock); > > + return ret; > > +} > > + > > +static int mtk_dip_pipe_next_job_id(struct mtk_dip_pipe *dip_pipe) > > +{ > > + int global_job_id = > > + atomic_inc_return(&dip_pipe->pipe_job_sequence); > > + > > + global_job_id = > > + (global_job_id & 0x0000FFFF) | > > + (dip_pipe->desc->id << 16); > > + > > + return global_job_id; > > +} > > + > > +int mtk_dip_pipe_init_job_infos(struct mtk_dip_pipe *dip_pipe) > > +{ > > + int i; > > + > > + spin_lock(&dip_pipe->job_lock); > > + > > + dip_pipe->num_pipe_job_infos = ARRAY_SIZE(dip_pipe->pipe_job_infos); > > + INIT_LIST_HEAD(&dip_pipe->pipe_job_running_list); > > + INIT_LIST_HEAD(&dip_pipe->pipe_job_free_list); > > + > > + for (i = 0; i < dip_pipe->num_pipe_job_infos; i++) { > > + struct mtk_dip_pipe_job_info *pipe_job_info = > > + &dip_pipe->pipe_job_infos[i]; > > + list_add_tail(&pipe_job_info->list, > > + &dip_pipe->pipe_job_free_list); > > + } > > + > > + spin_unlock(&dip_pipe->job_lock); > > + > > + return 0; > > +} > > + > > +static int mtk_dip_pipe_process_pipe_job_info(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_pipe_job_info > > + *pipe_job_info) > > +{ > > + spin_lock(&dip_pipe->job_lock); > > + > > + list_del(&pipe_job_info->list); > > + list_add_tail(&pipe_job_info->list, &dip_pipe->pipe_job_running_list); > > + > > + spin_unlock(&dip_pipe->job_lock); > > + return 0; > > +} > > + > > +struct mtk_dip_pipe_job_info * > > +mtk_dip_pipe_get_running_job_info(struct mtk_dip_pipe *dip_pipe, > > + int pipe_job_id) > > +{ > > + struct mtk_dip_pipe_job_info *pipe_job_info = NULL; > > + > > + spin_lock(&dip_pipe->job_lock); > > + > > + list_for_each_entry(pipe_job_info, > > + &dip_pipe->pipe_job_running_list, list) { > > + if (pipe_job_info->id == pipe_job_id) { > > + spin_unlock(&dip_pipe->job_lock); > > + return pipe_job_info; > > + } > > + } > > + > > + spin_unlock(&dip_pipe->job_lock); > > + > > + return NULL; > > +} > > + > > +static int > > +mtk_dip_pipe_free_job_info(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_pipe_job_info *pipe_job_info) > > +{ > > + spin_lock(&dip_pipe->job_lock); > > + > > + list_del(&pipe_job_info->list); > > + list_add_tail(&pipe_job_info->list, &dip_pipe->pipe_job_free_list); > > + > > + spin_unlock(&dip_pipe->job_lock); > > + > > + return 0; > > +} > > + > > +static struct mtk_dip_pipe_job_info * > > +mtk_dip_pipe_get_free_job_info(struct mtk_dip_pipe *dip_pipe) > > +{ > > + struct mtk_dip_pipe_job_info *pipe_job_info = NULL; > > + > > + spin_lock(&dip_pipe->job_lock); > > + list_for_each_entry(pipe_job_info, > > + &dip_pipe->pipe_job_free_list, list) { > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, "Found free pipe job\n"); > > + spin_unlock(&dip_pipe->job_lock); > > + return pipe_job_info; > > + } > > + spin_unlock(&dip_pipe->job_lock); > > + > > + dev_err(&dip_pipe->dip_dev->pdev->dev, > > + "%s: can't found free pipe job\n", > > + dip_pipe->desc->name); > > + > > + return NULL; > > +} > > + > > +static void > > +mtk_dip_pipe_update_job_info(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_pipe_job_info *pipe_job_info, > > + struct mtk_dip_video_device *node, > > + struct mtk_dip_dev_buffer *dev_buf) > > +{ > > + if (!pipe_job_info || !dev_buf || !node) { > > + dev_err(&dip_pipe->dip_dev->pdev->dev, > > + "%s: update pipe-job(%p) failed, buf(%p),node(%p)\n", > > + dip_pipe->desc->name, > > + pipe_job_info, dev_buf, node); > > + return; > > + } > > + > > + if (pipe_job_info->buf_map[node->desc->id]) > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "%s:%s: buf overwrite\n", > > + dip_pipe->desc->name, > > + node->desc->name); > > + > > + if (node->desc->buf_type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) > > + pipe_job_info->num_img_capture_bufs++; > > + > > + if (node->desc->buf_type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > > + pipe_job_info->num_img_output_bufs++; > > + > > + if (node->desc->buf_type == V4L2_BUF_TYPE_META_OUTPUT) > > + pipe_job_info->num_meta_output_bufs++; > > + > > + if (node->desc->buf_type == V4L2_BUF_TYPE_META_CAPTURE) > > + pipe_job_info->num_meta_capture_bufs++; > > + > > + pipe_job_info->buf_map[node->desc->id] = dev_buf; > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "%s:%s: added buf(%p) to pipe-job(%p)\n", > > + dip_pipe->desc->name, node->desc->name, dev_buf, > > + pipe_job_info); > > +} > > + > > +static void mtk_dip_pipe_debug_job(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_pipe_job_info *pipe_job_info) > > +{ > > + int i; > > + > > + if (!dip_pipe || !pipe_job_info) > > + return; > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "%s: pipe-job(%p),id(%d),req(%p)buf nums(%d,%d,%d,%d)\n", > > + dip_pipe->desc->name, > > + pipe_job_info, > > + pipe_job_info->id, > > + pipe_job_info->req, > > + pipe_job_info->num_img_capture_bufs, > > + pipe_job_info->num_img_output_bufs, > > + pipe_job_info->num_meta_capture_bufs, > > + pipe_job_info->num_meta_output_bufs); > > + > > + for (i = 0; i < MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM ; i++) { > > + if (pipe_job_info->buf_map[i]) > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "Node(%s,%d), buf(%p)\n", > > + dip_pipe->nodes[i].desc->name, i, > > + pipe_job_info->buf_map[i]); > > + } > > +} > > + > > +int mtk_dip_pipe_job_finish(struct mtk_dip_pipe *dip_pipe, > > + unsigned int pipe_job_info_id, > > + enum vb2_buffer_state vbf_state) > > +{ > > + int i; > > + struct mtk_dip_pipe_job_info *job_info = NULL; > > + const int pipe_id = > > + mtk_dip_pipe_get_pipe_from_job_id(pipe_job_info_id); > > + u64 timestamp = 0; > > + > > + if (!dip_pipe) > > + pr_err("%s: pipe-job id(%d) release failed, dip_pipe is null\n", > > + __func__, pipe_job_info_id); > > + > > + job_info = mtk_dip_pipe_get_running_job_info(dip_pipe, > > + pipe_job_info_id); > > + > > + if (!job_info) { > > + dev_err(&dip_pipe->dip_dev->pdev->dev, > > + "%s:%s: can't find pipe-job id(%d)\n", > > + __func__, dip_pipe->desc->name, pipe_id); > > + return -EINVAL; > > + } > > + > > + timestamp = ktime_get_ns(); > > + > > + for (i = 0; i < MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM; i++) { > > + struct mtk_dip_dev_buffer *dev_buf = job_info->buf_map[i]; > > + > > + if (!dev_buf) { > > + continue; > > + } else { > > + dev_buf->vbb.vb2_buf.timestamp = ktime_get_ns(); > > + mtk_dip_v4l2_buffer_done(&dev_buf->vbb.vb2_buf, > > + vbf_state); > > + } > > + } > > + > > + mtk_dip_pipe_free_job_info(dip_pipe, job_info); > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "%s:%s: finish pipe-job, id(%d), vb state(%d)\n", > > + __func__, dip_pipe->desc->name, pipe_id, > > + pipe_job_info_id, vbf_state); > > The format string does not match the arguments. There are two ids in the > arguments but only one %d for them. > I will fix it by removing the pipe_id as following: dev_dbg(&dip_pipe->dip_dev->pdev->dev, "%s:%s: finish pipe-job, id(%d), vb state(%d)\n", __func__, dip_pipe->desc->name, pipe_job_info_id, vbf_state); > > + > > + return 0; > > +} > > + > > +static void mtk_dip_dev_buf_fill_info(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_dev_buffer *dev_buf) > > +{ > > + struct vb2_v4l2_buffer *b; > > + struct mtk_dip_video_device *node; > > + struct mtk_dip_video_device_desc *desc; > > + > > + b = &dev_buf->vbb; > > + node = mtk_dip_vbq_to_node(b->vb2_buf.vb2_queue); > > + desc = node->desc; > > + dev_buf->fmt = node->vdev_fmt; > > + dev_buf->dev_fmt = node->dev_q.dev_fmt; > > + dev_buf->isp_daddr = > > + vb2_dma_contig_plane_dma_addr(&b->vb2_buf, 0); > > + dev_buf->vaddr = vb2_plane_vaddr(&b->vb2_buf, 0); > > + dev_buf->buffer_usage = node->dev_q.buffer_usage; > > + dev_buf->rotation = node->dev_q.rotation; > > + > > + if (desc->smem_alloc) { > > + dev_buf->scp_daddr = > > + mtk_dip_smem_iova_to_phys > > + (dip_pipe->smem_alloc_dev, > > + dev_buf->isp_daddr); > > + } else { > > + dev_buf->scp_daddr = 0; > > + } > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "%s:%s: buf type(%d), idx(%d), mem(%d), isp_daddr(%p), scp_daddr(%p)\n", > > + dip_pipe->desc->name, > > + desc->name, > > + b->vb2_buf.type, > > + b->vb2_buf.index, > > + b->vb2_buf.memory, > > + dev_buf->isp_daddr, > > + dev_buf->scp_daddr); > > +} > > + > > +int mtk_dip_pipe_queue_buffers(struct media_request *req, > > + int initial) > > +{ > > + struct media_request_object *obj; > > + struct mtk_dip_pipe *dip_pipe; > > + struct mtk_dip_pipe_job_info *pipe_job_info = NULL; > > + int ret = 0; > > + > > + list_for_each_entry(obj, &req->objects, list) { > > + struct vb2_buffer *vb; > > + > > + if (vb2_request_object_is_buffer(obj)) { > > + struct mtk_dip_dev_buffer *buf; > > + struct mtk_dip_dev_buffer *dev_buf; > > + struct mtk_dip_video_device *node; > > + > > + vb = container_of(obj, struct vb2_buffer, req_obj); > > + node = mtk_dip_vbq_to_node(vb->vb2_queue); > > + dip_pipe = vb2_get_drv_priv(vb->vb2_queue); > > + dev_buf = mtk_dip_vb2_buf_to_dev_buf(vb); > > + buf = dev_buf; > > + > > + if (!pipe_job_info) { > > + pipe_job_info = mtk_dip_pipe_get_free_job_info > > + (dip_pipe); > > + > > + if (!pipe_job_info) > > + goto FAILE_JOB_NOT_TRIGGER; > > + > > + memset(pipe_job_info->buf_map, 0, > > + sizeof(pipe_job_info->buf_map)); > > + pipe_job_info->num_img_capture_bufs = 0; > > + pipe_job_info->num_img_output_bufs = 0; > > + pipe_job_info->num_meta_capture_bufs = 0; > > + pipe_job_info->num_meta_output_bufs = 0; > > + } > > + > > + mtk_dip_dev_buf_fill_info(dip_pipe, > > + buf); > > + > > + mtk_dip_pipe_update_job_info(dip_pipe, > > + pipe_job_info, > > + node, > > + buf); > > + } > > + } > > + > > + if (!pipe_job_info) > > + return -EINVAL; > > + > > + pipe_job_info->id = > > + mtk_dip_pipe_next_job_id(dip_pipe); > > + > > + mtk_dip_pipe_debug_job(dip_pipe, pipe_job_info); > > + > > + mutex_lock(&dip_pipe->lock); > > + > > + if (!dip_pipe->streaming) { > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "%s:%s: stream is off, no hw enqueue triggered\n", > > + __func__, dip_pipe->desc->name); > > + mutex_unlock(&dip_pipe->lock); > > + return 0; > > + } > > + > > + if (mtk_dip_pipe_process_pipe_job_info(dip_pipe, pipe_job_info)) { > > + dev_err(&dip_pipe->dip_dev->pdev->dev, > > + "%s:%s: can't start to run pipe job id(%d)\n", > > + __func__, dip_pipe->desc->name, > > + pipe_job_info->id); > > + mutex_unlock(&dip_pipe->lock); > > + goto FAILE_JOB_NOT_TRIGGER; > > + } > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "%s: trigger pipe job, id(%d)\n", > > + dip_pipe->desc->name, > > + dip_pipe->desc->id); > > + > > + if (mtk_dip_pipe_job_start(dip_pipe, pipe_job_info)) { > > + mutex_unlock(&dip_pipe->lock); > > + goto FAILE_JOB_NOT_TRIGGER; > > + } > > + > > + mutex_unlock(&dip_pipe->lock); > > + > > + return 0; > > + > > +FAILE_JOB_NOT_TRIGGER: > > The label should be snake_case. Is "FAILE" a typo of "FAILED"? Yes. I will correct it. > > > + if (initial) > > + return ret; > > + > > + mtk_dip_pipe_job_finish(dip_pipe, pipe_job_info->id, > > + VB2_BUF_STATE_ERROR); > > + > > + return -EINVAL; > > +} > > + > > +int mtk_dip_pipe_release(struct mtk_dip_pipe *dip_pipe) > > +{ > > + mtk_dip_pipe_v4l2_unregister(dip_pipe); > > + v4l2_ctrl_handler_free(&dip_pipe->ctrl_handler); > > + mutex_destroy(&dip_pipe->lock); > > + > > + return 0; > > +} > > + > > +static void set_img_fmt(struct v4l2_pix_format_mplane *mfmt_to_fill, > > + struct mtk_dip_dev_format *dev_fmt) > > +{ > > + int i; > > + > > + mfmt_to_fill->pixelformat = dev_fmt->fmt.img.pixelformat; > > + mfmt_to_fill->num_planes = dev_fmt->fmt.img.num_planes; > > + mfmt_to_fill->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > > + mfmt_to_fill->quantization = V4L2_QUANTIZATION_DEFAULT; > > + mfmt_to_fill->colorspace = dev_fmt->fmt.img.colorspace; > > + > > + memset(mfmt_to_fill->reserved, 0, sizeof(mfmt_to_fill->reserved)); > > + > > + pr_debug("%s: Fmt(%d),w(%d),h(%d),f(%d)\n", > > + __func__, > > + mfmt_to_fill->pixelformat, > > + mfmt_to_fill->width, > > + mfmt_to_fill->height, > > + mfmt_to_fill->field); > > + > > + for (i = 0 ; i < mfmt_to_fill->num_planes; ++i) { > > Please remove the space after "i = 0". I got it. > > > + int bpl = (mfmt_to_fill->width * > > + dev_fmt->fmt.img.row_depth[i]) / 8; > > + int sizeimage = (mfmt_to_fill->width * mfmt_to_fill->height * > > + dev_fmt->fmt.img.depth[i]) / 8; > > + > > + mfmt_to_fill->plane_fmt[i].bytesperline = bpl; > > + mfmt_to_fill->plane_fmt[i].sizeimage = sizeimage; > > + memset(mfmt_to_fill->plane_fmt[i].reserved, > > + 0, sizeof(mfmt_to_fill->plane_fmt[i].reserved)); > > + > > + pr_debug("plane(%d):bpl(%d),sizeimage(%u)\n", > > + i, bpl, > > + mfmt_to_fill->plane_fmt[i].sizeimage); > > + } > > +} > > + > > +static void set_meta_fmt(struct v4l2_meta_format *metafmt_to_fill, > > + struct mtk_dip_dev_format *dev_fmt) > > +{ > > + metafmt_to_fill->dataformat = dev_fmt->fmt.meta.dataformat; > > + > > + if (dev_fmt->fmt.meta.max_buffer_size <= 0) { > > + pr_debug("Invalid meta buf size(%u), use default(%u)\n", > > + dev_fmt->fmt.meta.max_buffer_size, > > + MTK_DIP_DEV_META_BUF_DEFAULT_SIZE); > > + metafmt_to_fill->buffersize = MTK_DIP_DEV_META_BUF_DEFAULT_SIZE; > > + } else { > > + pr_debug("Use meta size(%u)\n", > > + dev_fmt->fmt.meta.max_buffer_size); > > + metafmt_to_fill->buffersize = dev_fmt->fmt.meta.max_buffer_size; > > + } > > +} > > + > > +void mtk_dip_pipe_load_default_fmt(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_video_device *node, > > + struct v4l2_format *fmt_to_fill) > > +{ > > + struct mtk_dip_dev_format *dev_fmt; > > + struct mtk_dip_video_device_desc *desc = node->desc; > > + > > + if (desc->num_fmts == 0) { > > + pr_err("%s:%s: desc->num_fmts is 0, no format support list\n", > > + __func__, desc->name); > > + return; > > + } > > + > > + if (desc->default_fmt_idx >= desc->num_fmts) { > > + pr_debug("%s:%s: invalid idx(%d), must < num_fmts(%d)\n", > > + __func__, desc->name, desc->default_fmt_idx, > > + desc->num_fmts); > > + desc->default_fmt_idx = 0; > > + } > > + > > + dev_fmt = &desc->fmts[desc->default_fmt_idx]; > > + fmt_to_fill->type = desc->buf_type; > > + if (mtk_dip_buf_is_meta(desc->buf_type)) { > > + set_meta_fmt(&fmt_to_fill->fmt.meta, dev_fmt); > > + } else { > > + fmt_to_fill->fmt.pix_mp.width = desc->default_width; > > + fmt_to_fill->fmt.pix_mp.height = desc->default_height; > > + fmt_to_fill->fmt.pix_mp.field = V4L2_FIELD_NONE; > > + > > + set_img_fmt(&fmt_to_fill->fmt.pix_mp, dev_fmt); > > + } > > +} > > + > > +struct mtk_dip_dev_format * > > +mtk_dip_pipe_find_fmt(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_video_device *node, > > + u32 format) > > +{ > > + int i; > > + struct mtk_dip_dev_format *dev_fmt; > > + > > + struct mtk_dip_video_device_desc *desc = node->desc; > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, "fmt to find(%x)\n", format); > > + > > + for (i = 0; i < desc->num_fmts; i++) { > > + dev_fmt = &desc->fmts[i]; > > + if (!mtk_dip_buf_is_meta(desc->buf_type)) { > > + if (dev_fmt->fmt.img.pixelformat == format) > > + return dev_fmt; > > + } else { > > + if (dev_fmt->fmt.meta.dataformat == format) > > + return dev_fmt; > > + } > > + } > > + > > + return NULL; > > +} > > + > > +int mtk_dip_pipe_set_meta_fmt(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_video_device *node, > > + struct v4l2_meta_format *user_fmt, > > + struct v4l2_meta_format *node_fmt) > > +{ > > + struct mtk_dip_dev_format *dev_fmt; > > + > > + if (!user_fmt || !node_fmt) > > + return -EINVAL; > > + > > + dev_fmt = mtk_dip_pipe_find_fmt(dip_pipe, node, > > + user_fmt->dataformat); > > + > > + if (!dev_fmt) > > + return -EINVAL; > > + > > + node->dev_q.dev_fmt = dev_fmt; > > + set_meta_fmt(node_fmt, dev_fmt); > > + *user_fmt = *node_fmt; > > + > > + return 0; > > +} > > + > > +int mtk_dip_pipe_set_img_fmt(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_video_device *node, > > + struct v4l2_pix_format_mplane *user_fmt, > > + struct v4l2_pix_format_mplane *dest_fmt) > > +{ > > + struct mtk_dip_dev_format *dev_fmt; > > + > > + if (!user_fmt || !dest_fmt) > > + return -EINVAL; > > + > > + dev_fmt = mtk_dip_pipe_find_fmt(dip_pipe, node, > > + user_fmt->pixelformat); > > + > > + if (!dev_fmt) { > > + pr_debug("%s:%s:%s: dev_fmt(%d) not found\n", > > + __func__, dip_pipe->desc->name, > > + node->desc->name, user_fmt->pixelformat); > > + return -EINVAL; > > + } > > + > > + node->dev_q.dev_fmt = dev_fmt; > > + dest_fmt->width = user_fmt->width; > > + dest_fmt->height = user_fmt->height; > > + dest_fmt->field = V4L2_FIELD_NONE; > > + > > + set_img_fmt(dest_fmt, dev_fmt); > > + > > + return 0; > > +} > > + > > +int mtk_dip_pipe_streamon(struct mtk_dip_pipe *dip_pipe) > > +{ > > + int ret; > > + struct mtk_dip_dev *dip_dev; > > + > > + if (!dip_pipe) > > + return -EINVAL; > > + > > + dip_dev = dev_get_drvdata(&dip_pipe->dip_dev->pdev->dev); > > + > > + mutex_lock(&dip_pipe->lock); > > + > > + ret = mtk_dip_hw_streamon(&dip_dev->dip_hw, > > + dip_pipe->desc->id); > > + > > + if (ret) { > > + dev_err(&dip_pipe->dip_dev->pdev->dev, > > + "%s:%s:%d: failed to start hw\n", > > + __func__, dip_pipe->desc->name, > > + dip_pipe->desc->id); > > + mutex_unlock(&dip_pipe->lock); > > + return -EBUSY; > > + } > > + > > + dip_pipe->streaming = 1; > > + mutex_unlock(&dip_pipe->lock); > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "%s:%s:%d: start hw\n", > > + __func__, dip_pipe->desc->name, > > + dip_pipe->desc->id); > > + > > + return ret; > > +} > > + > > +int mtk_dip_pipe_streamoff(struct mtk_dip_pipe *dip_pipe) > > +{ > > + int ret; > > + struct mtk_dip_dev *dip_dev; > > + > > + if (!dip_pipe) > > + return -EINVAL; > > + > > + dip_dev = dev_get_drvdata(&dip_pipe->dip_dev->pdev->dev); > > + > > + mutex_lock(&dip_pipe->lock); > > + > > + ret = mtk_dip_hw_streamoff(&dip_dev->dip_hw, > > + dip_pipe->desc->id); > > + > > + if (ret) { > > + dev_err(&dip_pipe->dip_dev->pdev->dev, > > + "%s:%s:%d: failed to stop hw\n", > > + __func__, dip_pipe->desc->name, > > + dip_pipe->desc->id); > > + mutex_unlock(&dip_pipe->lock); > > + return -EBUSY; > > + } > > + > > + dip_pipe->streaming = 0; > > + mutex_unlock(&dip_pipe->lock); > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "%s:%s:%d: stop hw\n", > > + __func__, dip_pipe->desc->name, > > + dip_pipe->desc->id); > > + > > + return 0; > > +} > > + > > +static enum mdp_ycbcr_profile > > +map_ycbcr_prof_mplane(struct v4l2_pix_format_mplane *pix_mp, > > + u32 mdp_color) > > +{ > > + 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; > > + } > > + /* V4L2_COLORSPACE_SRGB or else */ > > + if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE) > > + return MDP_YCBCR_PROFILE_FULL_BT601; > > + return MDP_YCBCR_PROFILE_BT601; > > +} > > + > > +/* Stride that is accepted by MDP HW */ > > +static u32 dip_mdp_fmt_get_stride(const struct mtk_dip_dev_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 > > +dip_mdp_fmt_get_stride_contig(const struct mtk_dip_dev_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 > > +dip_mdp_fmt_get_plane_size(const struct mtk_dip_dev_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 int is_contig_mp_buffer(struct mtk_dip_dev_buffer *dev_buf) > > +{ > > + if (MDP_COLOR_GET_PLANE_COUNT(dev_buf->dev_fmt->fmt.img.mdp_color) > > + == 1) > > + return 0; > > + else > > + return 1; > > +} > > + > > +static int fill_ipi_img_param_mp(struct mtk_dip_pipe *dip_pipe, > > + struct img_image_buffer *b, > > + struct mtk_dip_dev_buffer *dev_buf, > > + char *buf_name) > > +{ > > + struct v4l2_pix_format_mplane *pix_mp; > > + struct mtk_dip_dev_mdp_format *mdp_fmt; > > + unsigned int i; > > + unsigned int total_plane_size = 0; > > + > > + if (!dev_buf || !dev_buf->dev_fmt) { > > + dev_err(&dip_pipe->dip_dev->pdev->dev, > > + "%s: %s's dev format not set\n", > > + __func__, buf_name); > > + return -EINVAL; > > + } > > + > > + pix_mp = &dev_buf->fmt.fmt.pix_mp; > > + mdp_fmt = &dev_buf->dev_fmt->fmt.img; > > + > > + b->format.colorformat = dev_buf->dev_fmt->fmt.img.mdp_color; > > + b->format.width = dev_buf->fmt.fmt.pix_mp.width; > > + b->format.height = dev_buf->fmt.fmt.pix_mp.height; > > + b->format.ycbcr_prof = > > + map_ycbcr_prof_mplane(pix_mp, > > + dev_buf->dev_fmt->fmt.img.mdp_color); > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "%s: buf(%s), IPI: w(%d),h(%d),c(0x%x)\n", > > + dip_pipe->desc->name, > > + buf_name, > > + b->format.width, > > + b->format.height, > > + b->format.colorformat); > > + > > + for (i = 0; i < pix_mp->num_planes; ++i) { > > + u32 stride = > > + dip_mdp_fmt_get_stride > > + (mdp_fmt, pix_mp->plane_fmt[i].bytesperline, i); > > + > > + b->format.plane_fmt[i].stride = stride; > > + b->format.plane_fmt[i].size = > > + dip_mdp_fmt_get_plane_size(mdp_fmt, > > + stride, > > + pix_mp->height, i); > > + b->iova[i] = dev_buf->isp_daddr; > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "Contiguous-mp-buf:plane(%i),stride(%d),size(%d),iova(%p)", > > + i, > > + b->format.plane_fmt[i].stride, > > + b->format.plane_fmt[i].size, > > + b->iova[i]); > > iova is an u32, so should we use %x instead of %llx here? Yes. I will use %llx here. > > > + total_plane_size = b->format.plane_fmt[i].size; > > + } > > + > > + for (; i < MDP_COLOR_GET_PLANE_COUNT(b->format.colorformat); ++i) { > > + u32 stride = > > + dip_mdp_fmt_get_stride_contig > > + (mdp_fmt, b->format.plane_fmt[0].stride, i); > > + > > + b->format.plane_fmt[i].stride = stride; > > + b->format.plane_fmt[i].size = > > + dip_mdp_fmt_get_plane_size(mdp_fmt, stride, > > + pix_mp->height, i); > > + b->iova[i] = b->iova[i - 1] + b->format.plane_fmt[i - 1].size; > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "Contiguous-mp-buf:plane(%i),stride(%d),size(%d),iova(%p)", > > + i, > > + b->format.plane_fmt[i].stride, > > + b->format.plane_fmt[i].size, > > + b->iova[i]); > > iova is an u32, so should we use %x instead of %llx here? > Yes. I will use %llx here. > > + total_plane_size += b->format.plane_fmt[i].size; > > + } > > + > > + b->usage = dev_buf->buffer_usage; > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "Contiguous-mp-buf(%s),v4l2-sizeimage(%d),total-plane-size(%d)\n", > > + buf_name, > > + pix_mp->plane_fmt[0].sizeimage, > > + total_plane_size); > > + > > + return 0; > > +} > > + > > +static int fill_ipi_img_param(struct mtk_dip_pipe *dip_pipe, > > + struct img_image_buffer *img, > > + struct mtk_dip_dev_buffer *dev_buf, > > + char *buf_name) > > +{ > > + img->format.width = dev_buf->fmt.fmt.pix_mp.width; > > + img->format.height = dev_buf->fmt.fmt.pix_mp.height; > > + > > + if (dev_buf && dev_buf->dev_fmt) { > > + img->format.colorformat = > > + dev_buf->dev_fmt->fmt.img.mdp_color; > > + } else { > > + dev_err(&dip_pipe->dip_dev->pdev->dev, > > + "%s: %s's dev format not set\n", > > + __func__, buf_name); > > + return -EINVAL; > > + } > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "%s: buf(%s) IPI: w(%d),h(%d),c(0x%x)\n", > > + dip_pipe->desc->name, > > + buf_name, > > + img->format.width, > > + img->format.height, > > + img->format.colorformat); > > + > > + img->format.plane_fmt[0].size = > > + dev_buf->fmt.fmt.pix_mp.plane_fmt[0].sizeimage; > > + img->format.plane_fmt[0].stride = > > + dev_buf->fmt.fmt.pix_mp.plane_fmt[0].bytesperline; > > + img->iova[0] = dev_buf->isp_daddr; > > + img->usage = dev_buf->buffer_usage; > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "size(%d), stride(%d),ycbcr(%d),iova(%p),u(%d)\n", > > + img->format.plane_fmt[0].size, > > + img->format.plane_fmt[0].stride, > > + img->format.ycbcr_prof, > > + img->iova[0], > > + img->usage); > > + > > + return 0; > > +} > > + > > +static int fill_input_ipi_param(struct mtk_dip_pipe *dip_pipe, > > + struct img_input *iin, > > + struct mtk_dip_dev_buffer *dev_buf, > > + char *buf_name) > > +{ > > + struct img_image_buffer *img = &iin->buffer; > > + > > + /* Will map the vale with V4L2 color space in the future */ > > typo? vale => value > I will fix it. > > + img->format.ycbcr_prof = 1; > > + if (is_contig_mp_buffer(dev_buf)) > > + return fill_ipi_img_param_mp(dip_pipe, img, dev_buf, > > + buf_name); > > + else > > + return fill_ipi_img_param(dip_pipe, img, dev_buf, > > + buf_name); > > +} > > + > > +static int fill_output_ipi_param(struct mtk_dip_pipe *dip_pipe, > > + struct img_output *iout, > > + struct mtk_dip_dev_buffer *dev_buf_out, > > + struct mtk_dip_dev_buffer *dev_buf_in, > > + char *buf_name) > > +{ > > + int ret; > > + struct img_image_buffer *img = &iout->buffer; > > + > > + img->format.ycbcr_prof = 0; > > + > > + if (is_contig_mp_buffer(dev_buf_out)) > > + ret = fill_ipi_img_param_mp(dip_pipe, img, dev_buf_out, > > + buf_name); > > + else > > + ret = fill_ipi_img_param(dip_pipe, img, dev_buf_out, > > + buf_name); > > + > > + iout->crop.left = 0; > > + iout->crop.top = 0; > > + iout->crop.width = dev_buf_in->fmt.fmt.pix_mp.width; > > + iout->crop.height = dev_buf_in->fmt.fmt.pix_mp.height; > > + iout->crop.left_subpix = 0; > > + iout->crop.top_subpix = 0; > > + iout->crop.width_subpix = 0; > > + iout->crop.height_subpix = 0; > > + iout->rotation = dev_buf_out->rotation; > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "%s: buf(%s) IPI-ext:c_l(%d),c_t(%d),c_w(%d),c_h(%d)\n", > > + dip_pipe->desc->name, > > + buf_name, > > + iout->crop.left, > > + iout->crop.top, > > + iout->crop.width, > > + iout->crop.height); > > + > > + dev_dbg(&dip_pipe->dip_dev->pdev->dev, > > + "c_ls(%d),c_ts(%d),c_ws(%d),c_hs(%d),rot(%d)\n", > > + iout->crop.left_subpix, > > + iout->crop.top_subpix, > > + iout->crop.width_subpix, > > + iout->crop.height_subpix, > > + iout->rotation); > > + > > + return ret; > > +} > > + > > +int mtk_dip_pipe_job_start(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_pipe_job_info *pipe_job_info) > > +{ > > + struct platform_device *pdev = dip_pipe->dip_dev->pdev; > > + int ret; > > + int out_img_buf_idx; > > + struct img_ipi_frameparam dip_param; > > + struct mtk_dip_dev_buffer *dev_buf_in; > > + struct mtk_dip_dev_buffer *dev_buf_out; > > + struct mtk_dip_dev_buffer *dev_buf_tuning; > > + > > + if (!pipe_job_info) { > > + dev_err(&pdev->dev, > > + "pipe_job_info(%p) in start can't be NULL\n", > > + pipe_job_info); > > + return -EINVAL; > > + } > > + > > + /* We need RAW and at least MDP0 or MDP 1 buffer */ > > + if (!pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_RAW_OUT] || > > + (!pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_MDP0_CAPTURE] && > > + !pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_MDP1_CAPTURE])){ > > + struct mtk_dip_dev_buffer **map = pipe_job_info->buf_map; > > + > > + dev_dbg(&pdev->dev, > > + "can't trigger job: raw(%p), mdp0(%p), mdp1(%p)\n", > > + map[MTK_DIP_VIDEO_NODE_ID_RAW_OUT], > > + map[MTK_DIP_VIDEO_NODE_ID_MDP0_CAPTURE], > > + map[MTK_DIP_VIDEO_NODE_ID_MDP1_CAPTURE]); > > + return -EINVAL; > > + } > > + > > + dev_dbg(&pdev->dev, > > + "%s:%s: pipe-job id(%d)\n", > > + __func__, dip_pipe->desc->name, > > + pipe_job_info->id); > > + > > + /* Fill ipi params for DIP driver */ > > + memset(&dip_param, 0, sizeof(struct img_ipi_frameparam)); > > + > > + dip_param.index = pipe_job_info->id; > > + dip_param.num_outputs = pipe_job_info->num_img_capture_bufs; > > + dip_param.num_inputs = pipe_job_info->num_img_output_bufs; > > + dip_param.type = STREAM_ISP_IC; > > + > > + /* Tuning buffer */ > > + dev_buf_tuning = > > + pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_TUNING_OUT]; > > + if (dev_buf_tuning) { > > + dev_dbg(&pdev->dev, > > + "Tuning buf queued: pa(%p),va(%p),iova(%p)\n", > > + dev_buf_tuning->scp_daddr, > > + dev_buf_tuning->vaddr, > > + dev_buf_tuning->isp_daddr); > > + dip_param.tuning_data.pa = (uint32_t)dev_buf_tuning->scp_daddr; > > + dip_param.tuning_data.va = (uint64_t)dev_buf_tuning->vaddr; > > + dip_param.tuning_data.iova = > > + (uint32_t)dev_buf_tuning->isp_daddr; > > + } else { > > + dev_dbg(&pdev->dev, > > + "Doesn't enqueued tuning buffer, by-pass\n"); > > + dip_param.tuning_data.pa = 0; > > + dip_param.tuning_data.va = 0; > > + dip_param.tuning_data.iova = 0; > > + } > > + > > + /* Raw-in buffer */ > > + dev_buf_in = > > + pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_RAW_OUT]; > > + if (dev_buf_in) { > > + struct img_input *iin = &dip_param.inputs[0]; > > + > > + fill_input_ipi_param(dip_pipe, iin, dev_buf_in, "RAW"); > > + } > > + > > + out_img_buf_idx = 0; > > + > > + /* MDP 0 buffer */ > > + dev_buf_out = > > + pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_MDP0_CAPTURE]; > > + if (dev_buf_out) { > > + struct img_output *iout = &dip_param.outputs[out_img_buf_idx]; > > + > > + fill_output_ipi_param(dip_pipe, iout, dev_buf_out, > > + dev_buf_in, "MDP0"); > > + out_img_buf_idx++; > > + } > > + > > + /* MDP 1 buffer */ > > + dev_buf_out = > > + pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_MDP1_CAPTURE]; > > + if (dev_buf_out) { > > + struct img_output *iout = &dip_param.outputs[out_img_buf_idx]; > > + > > + fill_output_ipi_param(dip_pipe, iout, dev_buf_out, > > + dev_buf_in, "MDP1"); > > + out_img_buf_idx++; > > + } > > + > > + ret = mtk_dip_hw_enqueue(&dip_pipe->dip_dev->dip_hw, &dip_param); > > + > > + if (ret) { > > + dev_dbg(&pdev->dev, > > + "%s:%s: enqueue to HW failed(%d)\n", > > + __func__, dip_pipe->desc->name, ret); > > + return -EBUSY; > > + } > > + > > + return ret; > > +} > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h > > new file mode 100644 > > index 000000000000..f51f7a44379a > > --- /dev/null > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h > > @@ -0,0 +1,321 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (c) 2018 MediaTek Inc. > > + * Author: Frederic Chen <frederic.chen@mediatek.com> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#ifndef _MTK_DIP_DEV_H_ > > +#define _MTK_DIP_DEV_H_ > > + > > +#include <linux/types.h> > > +#include <linux/device.h> > > +#include <linux/platform_device.h> > > +#include <media/v4l2-ctrls.h> > > +#include <media/v4l2-subdev.h> > > +#include <media/v4l2-device.h> > > +#include <linux/videodev2.h> > > +#include <media/videobuf2-core.h> > > +#include <media/videobuf2-v4l2.h> > > + > > +#include "mtk_dip-hw.h" > > +#include "mtk_dip-smem.h" > > + > > +#define MTK_DIP_PIPE_ID_PREVIEW 0 > > +#define MTK_DIP_PIPE_ID_CAPTURE 1 > > +#define MTK_DIP_PIPE_ID_REPROCESS 2 > > +#define MTK_DIP_PIPE_ID_TOTAL_NUM 3 > > + > > +#define MTK_DIP_VIDEO_NODE_ID_RAW_OUT 0 > > +#define MTK_DIP_VIDEO_NODE_ID_TUNING_OUT 1 > > +#define MTK_DIP_VIDEO_NODE_ID_OUT_TOTAL_NUM 2 > > +#define MTK_DIP_VIDEO_NODE_ID_MDP0_CAPTURE 2 > > +#define MTK_DIP_VIDEO_NODE_ID_MDP1_CAPTURE 3 > > +#define MTK_DIP_VIDEO_NODE_ID_CAPTURE_TOTAL_NUM 2 > > +#define MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM \ > > + (MTK_DIP_VIDEO_NODE_ID_OUT_TOTAL_NUM + \ > > + MTK_DIP_VIDEO_NODE_ID_CAPTURE_TOTAL_NUM) > > + > > +#define MTK_DIP_VIDEO_NODE_ID_NO_MASTER -1 > > + > > +#define MTK_DIP_OUTPUT_MIN_WIDTH 2U > > +#define MTK_DIP_OUTPUT_MIN_HEIGHT 2U > > +#define MTK_DIP_OUTPUT_MAX_WIDTH 5376U > > +#define MTK_DIP_OUTPUT_MAX_HEIGHT 4032U > > +#define MTK_DIP_CAPTURE_MIN_WIDTH 2U > > +#define MTK_DIP_CAPTURE_MIN_HEIGHT 2U > > +#define MTK_DIP_CAPTURE_MAX_WIDTH 5376U > > +#define MTK_DIP_CAPTURE_MAX_HEIGHT 4032U > > + > > +#define MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME "MTK-ISP-DIP-V4L2" > > +#define MTK_DIP_DEV_DIP_PREVIEW_NAME \ > > + MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME > > +#define MTK_DIP_DEV_DIP_CAPTURE_NAME "MTK-ISP-DIP-CAP-V4L2" > > +#define MTK_DIP_DEV_DIP_REPROCESS_NAME "MTK-ISP-DIP-REP-V4L2" > > + > > +#define MTK_DIP_DEV_META_BUF_DEFAULT_SIZE (1110 * 1024) > > + > > +#define V4L2_CID_PRIVATE_UT_NUM (V4L2_CID_USER_BASE | 0x1001) > > +#define V4L2_CID_PRIVATE_SET_BUFFER_USAGE (V4L2_CID_PRIVATE_UT_NUM + 2) > > +#define V4L2_CID_MTK_DIP_MAX 100 > > + > > +enum mtk_dip_v4l2_buffer_usage { > > + MTK_DIP_V4l2_BUF_USAGE_DEFAULT = 0, > > + MTK_DIP_V4l2_BUF_USAGE_FD, > > + MTK_DIP_V4l2_BUF_USAGE_POSTPROC, > > + MTK_DIP_V4l2_BUF_USAGE_NONE, > > +}; > > The constants in enums should be capitalized. Could we use V4L2 instead of V4l2 > here? > Yes. I will use V4L2 instead. > > + > > +/* > > + * Supported format and the information used for > > + * size calculation > > + */ > > +struct mtk_dip_dev_meta_format { > > + u32 dataformat; > > + u32 max_buffer_size; > > + u8 flags; > > +}; > > + > > +/* MDP part private format definitation */ > > +struct mtk_dip_dev_mdp_format { > > + u32 pixelformat; > > + u32 mdp_color; > > + u32 colorspace; > > + u8 depth[VIDEO_MAX_PLANES]; > > + u8 row_depth[VIDEO_MAX_PLANES]; > > + u8 num_planes; > > + u8 walign; > > + u8 halign; > > + u8 salign; > > + u32 flags; > > +}; > > + > > +struct mtk_dip_dev_format { > > + union { > > + struct mtk_dip_dev_meta_format meta; > > + struct mtk_dip_dev_mdp_format img; > > + } fmt; > > +}; > > + > > +struct mtk_dip_pipe_job_info { > > + struct media_request *req; > > + int id; > > + struct mtk_dip_dev_buffer* > > + buf_map[MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM]; > > + int num_img_capture_bufs; > > + int num_img_output_bufs; > > + int num_meta_capture_bufs; > > + int num_meta_output_bufs; > > + struct list_head list; > > +}; > > + > > +struct mtk_dip_dev_buffer { > > + struct vb2_v4l2_buffer vbb; > > + struct v4l2_format fmt; > > + struct mtk_dip_dev_format *dev_fmt; > > + int pipe_job_id; > > + void *vaddr; > > + dma_addr_t isp_daddr; > > + dma_addr_t scp_daddr; > > + unsigned int buffer_usage; > > + int rotation; > > + struct list_head list; > > +}; > > + > > +struct mtk_dip_pipe_desc { > > + char *name; > > + int master; > > + int id; > > + struct mtk_dip_video_device_desc *output_queue_descs; > > + int total_output_queues; > > + struct mtk_dip_video_device_desc *capture_queue_descs; > > + int total_capture_queues; > > +}; > > + > > +struct mtk_dip_video_device_desc { > > + int id; > > + char *name; > > + u32 buf_type; > > + u32 cap; > > + int smem_alloc; > > + int dynamic; > > + int default_enable; > > + struct mtk_dip_dev_format *fmts; > > + int num_fmts; > > + char *description; > > + int default_width; > > + int default_height; > > + const struct v4l2_ioctl_ops *ops; > > + int default_fmt_idx; > > +}; > > + > > +struct mtk_dip_dev_queue { > > + struct vb2_queue vbq; > > + /* Serializes vb2 queue and video device operations */ > > + struct mutex lock; > > + struct mtk_dip_dev_format *dev_fmt; > > + /* Firmware uses buffer_usage to select suitable DMA ports */ > > + unsigned int buffer_usage; > > + int rotation; > > +}; > > + > > +struct mtk_dip_video_device { > > + struct video_device vdev; > > + struct mtk_dip_dev_queue dev_q; > > + struct v4l2_format vdev_fmt; > > + struct media_pad vdev_pad; > > + struct v4l2_mbus_framefmt pad_fmt; > > + struct v4l2_ctrl_handler ctrl_handler; > > + int immutable; > > + int enabled; > > + struct mtk_dip_video_device_desc *desc; > > + atomic_t sequence; > > +}; > > + > > +struct mtk_dip_pipe { > > + struct mtk_dip_dev *dip_dev; > > + struct mtk_dip_video_device nodes[MTK_DIP_VIDEO_NODE_ID_TOTAL_NUM]; > > + int num_nodes; > > + int streaming; > > + struct media_pad *subdev_pads; > > + struct media_pipeline pipeline; > > + struct v4l2_subdev subdev; > > + struct v4l2_subdev_fh *fh; > > + struct v4l2_ctrl_handler ctrl_handler; > > + struct mtk_dip_smem_dev *smem_alloc_dev; > > + atomic_t pipe_job_sequence; > > + struct mtk_dip_pipe_job_info pipe_job_infos[VB2_MAX_FRAME]; > > + int num_pipe_job_infos; > > + struct list_head pipe_job_running_list; > > + struct list_head pipe_job_free_list; > > + /* Serializes pipe's stream on/off and buffers enqueue operations */ > > + struct mutex lock; > > + spinlock_t job_lock; /* protect the pipe job list */ > > + struct mtk_dip_pipe_desc *desc; > > +}; > > + > > +struct mtk_dip_dev { > > + struct platform_device *pdev; > > + struct media_device mdev; > > + struct v4l2_device v4l2_dev; > > + struct mtk_dip_pipe dip_pipe[MTK_DIP_PIPE_ID_TOTAL_NUM]; > > + struct mtk_dip_smem_dev smem_alloc_dev; > > + struct mtk_dip_hw dip_hw; > > +}; > > + > > +int mtk_dip_dev_media_register(struct device *dev, > > + struct media_device *media_dev, > > + const char *model); > > + > > +int mtk_dip_dev_v4l2_init(struct mtk_dip_dev *dip_dev); > > + > > +void mtk_dip_dev_v4l2_release(struct mtk_dip_dev *dip_dev); > > + > > +int mtk_dip_dev_v4l2_register(struct device *dev, > > + struct media_device *media_dev, > > + struct v4l2_device *v4l2_dev); > > + > > +int mtk_dip_pipe_v4l2_register(struct mtk_dip_pipe *dip_pipe, > > + struct media_device *media_dev, > > + struct v4l2_device *v4l2_dev); > > + > > +int mtk_dip_pipe_v4l2_unregister(struct mtk_dip_pipe *dip_pipe); > > + > > +void mtk_dip_v4l2_buffer_done(struct vb2_buffer *vb, > > + enum vb2_buffer_state state); > > + > > +int mtk_dip_pipe_queue_buffers(struct media_request *req, int initial); > > + > > +int mtk_dip_pipe_init(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_dev *dip_dev, > > + struct mtk_dip_pipe_desc *setting, > > + struct media_device *media_dev, > > + struct v4l2_device *v4l2_dev, > > + struct mtk_dip_smem_dev *smem_alloc_dev); > > + > > +int mtk_dip_pipe_release(struct mtk_dip_pipe *dip_pipe); > > + > > +int mtk_dip_pipe_job_finish(struct mtk_dip_pipe *dip_pipe, > > + unsigned int pipe_job_info_id, > > + enum vb2_buffer_state state); > > + > > +int mtk_dip_pipe_job_start(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_pipe_job_info *pipe_job_info); > > + > > +int mtk_dip_pipe_init_job_infos(struct mtk_dip_pipe *dip_pipe); > > + > > +struct mtk_dip_dev_format * > > +mtk_dip_pipe_find_fmt(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_video_device *node, > > + u32 format); > > + > > +int mtk_dip_pipe_set_img_fmt(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_video_device *node, > > + struct v4l2_pix_format_mplane *user_fmt, > > + struct v4l2_pix_format_mplane *node_fmt); > > + > > +int mtk_dip_pipe_set_meta_fmt(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_video_device *node, > > + struct v4l2_meta_format *user_fmt, > > + struct v4l2_meta_format *node_fmt); > > Where do we use this function? > mtk_dip_pipe_set_meta_fmt() will not be used anymore. I will remove it. > > + > > +void mtk_dip_pipe_load_default_fmt(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_video_device *node, > > + struct v4l2_format *fmt_to_fill); > > + > > +int mtk_dip_pipe_streamon(struct mtk_dip_pipe *dip_pipe); > > + > > +int mtk_dip_pipe_streamoff(struct mtk_dip_pipe *dip_pipe); > > + > > +int mtk_dip_ctrl_init(struct mtk_dip_pipe *dip_pipe); > > + > > +static inline struct mtk_dip_video_device * > > +mtk_dip_file_to_node(struct file *file) > > +{ > > + return container_of(video_devdata(file), > > + struct mtk_dip_video_device, vdev); > > +} > > + > > +static inline struct mtk_dip_pipe * > > +mtk_dip_subdev_to_pipe(struct v4l2_subdev *sd) > > +{ > > + return container_of(sd, struct mtk_dip_pipe, subdev); > > +} > > + > > +static inline struct mtk_dip_video_device * > > +mtk_dip_vbq_to_node(struct vb2_queue *vq) > > +{ > > + return container_of(vq, struct mtk_dip_video_device, dev_q.vbq); > > +} > > + > > +static inline struct mtk_dip_dev_buffer * > > +mtk_dip_vb2_buf_to_dev_buf(struct vb2_buffer *vb) > > +{ > > + return container_of(vb, struct mtk_dip_dev_buffer, vbb.vb2_buf); > > +} > > + > > +static inline struct mtk_dip_dev *mtk_dip_hw_to_dev(struct mtk_dip_hw *dip_hw) > > +{ > > + return container_of(dip_hw, struct mtk_dip_dev, dip_hw); > > +} > > + > > +static inline int mtk_dip_buf_is_meta(u32 type) > > +{ > > + return type == V4L2_BUF_TYPE_META_CAPTURE || > > + type == V4L2_BUF_TYPE_META_OUTPUT; > > +} > > + > > +static inline int mtk_dip_pipe_get_pipe_from_job_id(int pipe_job_id) > > +{ > > + return (pipe_job_id >> 16) & 0x0000FFFF; > > +} > > + > > +#endif /* _MTK_DIP_DEV_H_ */ > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-hw.h b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-hw.h > > new file mode 100644 > > index 000000000000..d813d8b92e8b > > --- /dev/null > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-hw.h > > @@ -0,0 +1,167 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (C) 2018 MediaTek Inc. > > + * Author: Holmes Chiou <holmes.chiou@mediatek.com> > > + * > > + * This program is free software: you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#ifndef _MTK_DIP_HW_H_ > > +#define _MTK_DIP_HW_H_ > > + > > +#include <linux/clk.h> > > +#include "mtk-img-ipi.h" > > + > > +enum STREAM_TYPE_ENUM { > > + STREAM_UNKNOWN, > > + STREAM_BITBLT, > > + STREAM_GPU_BITBLT, > > + STREAM_DUAL_BITBLT, > > + STREAM_2ND_BITBLT, > > + STREAM_ISP_IC, > > + STREAM_ISP_VR, > > + STREAM_ISP_ZSD, > > + STREAM_ISP_IP, > > + STREAM_ISP_VSS, > > + STREAM_ISP_ZSD_SLOW, > > + STREAM_WPE, > > + STREAM_WPE2, > > +}; > > + > > +enum mtk_dip_hw_user_state { > > + DIP_STATE_INIT = 0, > > + DIP_STATE_STREAMON, > > + DIP_STATE_STREAMOFF > > +}; > > + > > +struct mtk_dip_hw_frame_job { > > + struct img_frameparam fparam; > > + int sequence; > > +}; > > + > > +struct mtk_dip_hw_user_id { > > + struct list_head list_entry; > > + u16 id; > > + u32 num; > > + u16 state; > > +}; > > + > > +struct mtk_dip_hw_subframe { > > + struct img_addr buffer; > > + struct sg_table table; > > + struct img_sw_addr config_data; > > + struct img_addr tuning_buf; > > + struct img_sw_addr frameparam; > > + struct list_head list_entry; > > +}; > > + > > +struct mtk_dip_hw_queue { > > + struct list_head queue; > > + struct mutex queuelock; /* protect queue and queue_cnt */ > > + u32 queue_cnt; > > +}; > > + > > +struct mtk_dip_hw_joblist { > > + struct list_head queue; > > + spinlock_t queuelock; /* protect job list */ > > + u32 queue_cnt; > > +}; > > + > > +struct mtk_dip_hw_thread { > > + struct task_struct *thread; > > + wait_queue_head_t wq; > > +}; > > + > > +struct mtk_dip_hw_work { > > + struct list_head list_entry; > > + struct img_ipi_frameparam frameparams; > > + struct mtk_dip_hw_user_id *user_id; > > +}; > > + > > +struct mtk_dip_hw_submit_work { > > + struct work_struct frame_work; > > + struct mtk_dip_hw *dip_hw; > > +}; > > + > > +struct mtk_dip_hw_mdpcb_work { > > + struct work_struct frame_work; > > + struct img_ipi_frameparam *frameparams; > > +}; > > + > > +struct mtk_dip_hw_clk { > > + struct clk *img_larb5; > > + struct clk *img_dip; > > +}; > > + > > +enum frame_state { > > + FRAME_STATE_INIT = 0, > > + FRAME_STATE_COMPOSING, > > + FRAME_STATE_RUNNING, > > + FRAME_STATE_DONE, > > + FRAME_STATE_STREAMOFF, > > + FRAME_STATE_ERROR, > > + FRAME_STATE_HW_TIMEOUT > > +}; > > + > > +struct mtk_dip_hw { > > + struct mtk_dip_hw_clk dip_clk; > > + struct device *larb_dev; > > + struct mtk_dip_hw_joblist dip_gcejoblist; > > + struct mtk_dip_hw_queue dip_freebufferlist; > > + struct mtk_dip_hw_queue dip_usedbufferlist; > > + struct mtk_dip_hw_thread dip_runner_thread; > > + struct mtk_dip_hw_queue dip_useridlist; > > + struct mtk_dip_hw_queue dip_worklist; > > + struct workqueue_struct *composer_wq; > > + struct mtk_dip_hw_submit_work submit_work; > > + wait_queue_head_t composing_wq; > > + wait_queue_head_t flushing_wq; > > + atomic_t num_composing; /* increase after ipi */ > > + /* increase after calling MDP driver */ > > + atomic_t num_running; > > + /*MDP/GCE callback workqueue */ > > Missing space after "/*". I will correct it. > > > + struct workqueue_struct *mdpcb_workqueue; > > + /* for MDP driver */ > > + struct platform_device *mdp_pdev; > > + /* for VPU driver */ > > + struct platform_device *vpu_pdev; > > + struct rproc *rproc_handle; > > + dma_addr_t scp_workingbuf_addr; > > + /* increase after enqueue */ > > + atomic_t dip_enque_cnt; > > + /* increase after Stream ON, decrease when Stream OFF */ > > + atomic_t dip_stream_cnt; > > + /* increase after open, decrease when close */ > > + atomic_t dip_user_cnt; > > +}; > > + > > +int mtk_dip_hw_enqueue(struct mtk_dip_hw *dip_hw, > > + struct img_ipi_frameparam *frameparams); > > +int mtk_dip_hw_connect(struct mtk_dip_hw *dip_hw); > > +int mtk_dip_hw_disconnect(struct mtk_dip_hw *dip_hw); > > +int mtk_dip_hw_streamon(struct mtk_dip_hw *dip_hw, u16 id); > > +int mtk_dip_hw_streamoff(struct mtk_dip_hw *dip_hw, u16 id); > > + > > +static inline struct mtk_dip_hw_frame_job > > +*mtk_dip_fparam_to_job(struct img_frameparam *fparam) > > +{ > > + return container_of(fparam, struct mtk_dip_hw_frame_job, fparam); > > +} > > + > > +static inline struct mtk_dip_hw_frame_job * > > +mtk_dip_ipi_fparam_to_job(struct img_ipi_frameparam *ipi_fparam) > > +{ > > + return container_of(ipi_fparam, > > + struct mtk_dip_hw_frame_job, > > + fparam.frameparam); > > +} > > + > > +#endif /* _MTK_DIP_HW_H_ */ > > + > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.c > > new file mode 100644 > > index 000000000000..5456c0b54ad4 > > --- /dev/null > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.c > > @@ -0,0 +1,322 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2018 MediaTek Inc. > > + * Author: Frederic Chen <frederic.chen@mediatek.com> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/device.h> > > +#include <linux/platform_device.h> > > +#include <linux/of.h> > > +#include <linux/of_fdt.h> > > +#include <linux/of_reserved_mem.h> > > +#include <linux/dma-contiguous.h> > > +#include <linux/dma-mapping.h> > > +#include <linux/slab.h> > > +#include <linux/err.h> > > +#include <linux/iommu.h> > > +#include <asm/cacheflush.h> > > +#include "mtk_dip-smem.h" > > + > > +#define MTK_DIP_SMEM_DEV_NAME "MTK-DIP-SMEM" > > + > > +static struct reserved_mem *dip_reserved_smem; > > +static struct dma_map_ops smem_dma_ops; > > + > > +struct dma_coherent_mem { > > + void *virt_base; > > + dma_addr_t device_base; > > + unsigned long pfn_base; > > + int size; > > + int flags; > > + unsigned long *bitmap; > > + spinlock_t spinlock; /* protect dma_coherent_mem member */ > > + bool use_dev_dma_pfn_offset; > > +}; > > + > > +static struct dma_coherent_mem *dev_get_coherent_memory(struct device *dev) > > +{ > > + if (dev && dev->dma_mem) > > + return dev->dma_mem; > > + return NULL; > > +} > > + > > +phys_addr_t mtk_dip_smem_iova_to_phys(struct mtk_dip_smem_dev *smem_dev, > > + dma_addr_t iova) > > +{ > > + struct iommu_domain *smem_dom; > > + phys_addr_t addr; > > + phys_addr_t limit; > > + > > + if (!smem_dev) > > + return 0; > > + > > + smem_dom = iommu_get_domain_for_dev(smem_dev->dev.parent); > > + > > + if (!smem_dom) > > + return 0; > > + > > + addr = iommu_iova_to_phys(smem_dom, iova); > > + > > + limit = smem_dev->smem_base + smem_dev->smem_size; > > + > > + if (addr < smem_dev->smem_base || addr >= limit) { > > + dev_err(&smem_dev->dev, > > + "Unexpected scp_daddr %pa (must >= %pa and <%pa)\n", > > + &addr, &smem_dev->smem_base, &limit); > > + return 0; > > + } > > + dev_dbg(&smem_dev->dev, "Pa verifcation pass: %pa(>=%pa, <%pa)\n", > > + &addr, &smem_dev->smem_base, &limit); > > + return addr; > > +} > > + > > +/******************************************** > > + * MTK DIP SMEM DMA ops * > > + ********************************************/ > > +static int mtk_dip_smem_get_sgtable(struct device *dev, > > + struct sg_table *sgt, > > + void *cpu_addr, > > + dma_addr_t dma_addr, > > + size_t size, unsigned long attrs) > > +{ > > + struct mtk_dip_smem_dev *smem_dev = dev_get_drvdata(dev); > > + int n_pages_align; > > + int size_align; > > + int page_start; > > + unsigned long long offset_p; > > + > > + phys_addr_t paddr = mtk_dip_smem_iova_to_phys(smem_dev, dma_addr); > > + > > + offset_p = (unsigned long long)paddr - > > + (unsigned long long)smem_dev->smem_base; > > + > > + dev_dbg(dev, "%s: dma_addr(%p), cpu_addr(%p), pa(%p), size(%d)\n", > > + __func__, dma_addr, cpu_addr, paddr, size); > > Please use %zd for size as it is a size_t. I got it. > > > + > > + size_align = round_up(size, PAGE_SIZE); > > + n_pages_align = size_align >> PAGE_SHIFT; > > + page_start = offset_p >> PAGE_SHIFT; > > + > > + dev_dbg(dev, "%s: page_start(%d), page pa(%p), pa(%p), aligned size(%d)\n", > > + __func__, > > + page_start, > > + page_to_phys(*(smem_dev->smem_pages + page_start)), > > + paddr, > > + size_align > > + ); > > + > > + if (!smem_dev) { > > + dev_err(dev, "can't get sgtable from smem_dev\n"); > > + return -EINVAL; > > + } > > We already dereference smem_dev above. Should we move this check or simply > remove it? > Yes, I will remove it. > > + > > + dev_dbg(dev, "%s: get sgt of the smem: %d pages\n", __func__, > > + n_pages_align); > > + > > + return sg_alloc_table_from_pages(sgt, > > + smem_dev->smem_pages + page_start, > > + n_pages_align, > > + 0, size_align, GFP_KERNEL); > > +} > > + > > <snip> > > > + > > -- > > 2.18.0 > > > > Sincerely, > Shik Sincerely, Frederic Chen
On Thu, May 23, 2019 at 10:46 PM Frederic Chen <frederic.chen@mediatek.com> wrote: > > Dear Tomasz, > > Thank you for your comments. > > > On Wed, 2019-05-22 at 19:25 +0900, Tomasz Figa wrote: > > Hi Frederic, > > > > On Wed, May 22, 2019 at 03:14:15AM +0800, Frederic Chen wrote: > > > Dear Tomasz, > > > > > > I appreciate your comment. It is very helpful for us. > > > > > > > You're welcome. Thanks for replying to all the comments. I'll skip those > > resolved in my reply to keep the message shorter. > > > > > > > > On Thu, 2019-05-09 at 18:48 +0900, Tomasz Figa wrote: > > > > Hi Frederic, > > > > > > > > On Wed, Apr 17, 2019 at 7:45 PM Frederic Chen <frederic.chen@mediatek.com> wrote: [snip] > > > > Also a general note - a work can be queued only once. This means that > > > > current code races when two dip_works are attempted to be queued very > > > > quickly one after another (or even at the same time from different threads). > > > > > > > > I can think of two potential options for fixing this: > > > > > > > > 1) Loop in the work function until there is nothing to queue to the hardware > > > > anymore - but this needs tricky synchronization, because there is still > > > > short time at the end of the work function when a new dip_work could be > > > > added. > > > > > > > > 2) Change this to a kthread that just keeps running in a loop waiting for > > > > some available dip_work to show up and then sending it to the firmware. > > > > This should be simpler, as the kthread shouldn't have a chance to miss > > > > any dip_work queued. > > > > > > > > I'm personally in favor of option 2, as it should simplify the > > > > synchronization. > > > > > > > > > > I would like to re-design this part with a kthread in the next patch. > > > > Actually I missed another option. We could have 1 work_struct for 1 > > request and then we could keep using a workqueue. Perhaps that could be > > simpler than a kthread. > > > > Actually, similar approach could be used for the dip_runner_func. > > Instead of having a kthread looping, we could just have another > > workqueue and 1 dip_runner_work per 1 request. Then we wouldn't need to > > do the waiting loop ourselves anymore. > > > > Does it make sense? > > Yes, it make sense. Let me summarize the modification about the flow. > > First, we will have two work_struct in mtk_dip_request. > > struct mtk_dip_request { > struct media_request request; > //... > /* Prepare DIP part hardware configurtion */ > struct mtk_dip_hw_submit_work submit_work; > /* Replace dip_running thread jobs*/ > struct mtk_dip_hw_composing_work composing_work; > /* Only for composing error handling */ > struct mtk_dip_hw_mdpcb_timeout_work timeout_work; > }; > > Second, the overall flow of handling each request is : > > 1. mtk_dip_hw_enqueue calls queue_work() to put submit_work into its > workqueue > 2. submit_work sends IMG_IPI_FRAME command to SCP to prepare DIP > hardware configuration > 3. dip_scp_handler receives the IMG_IPI_FRAME result from SCP > 4. dip_scp_handler calls queue_work() to put composing_work (instead > of original dip_running thread jobs) into its workqueue > 5. composing_work calls dip_mdp_cmdq_send() to finish the mdp part tasks > 6. dip_mdp_cb_func() trigged by MDP driver calls vb2_buffer_done to > return the buffer (no workqueue required here) > Sounds good to me, but actually then simply making the workqueues freezable doesn't solve the suspend/resume problem, because the work functions wouldn't wait for the firmware/hardware completion anymore. That's also okay, but in this case we need to add some code to suspend to wait for any pending operations to complete. Best regards, Tomasz
Dear Tomasz, I'd like to elaborate more about the tuning_data.va. Would you like to give us some advice about our improvement proposal inline? Thank you very much. On Wed, 2019-05-22 at 03:14 +0800, Frederic Chen wrote: > Dear Tomasz, > > I appreciate your comment. It is very helpful for us. > > > > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c > > > new file mode 100644 > > > index 000000000000..54d2b5f5b802 > > > --- /dev/null > > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c > > > @@ -0,0 +1,1384 @@ [snip] > > > +static void dip_submit_worker(struct work_struct *work) > > > +{ > > > + struct mtk_dip_hw_submit_work *dip_submit_work = > > > + container_of(work, struct mtk_dip_hw_submit_work, frame_work); > > > + struct mtk_dip_hw *dip_hw = dip_submit_work->dip_hw; > > > + struct mtk_dip_dev *dip_dev = mtk_dip_hw_to_dev(dip_hw); > > > + struct mtk_dip_hw_work *dip_work; > > > + struct mtk_dip_hw_subframe *buf; > > > + u32 len, num; > > > + int ret; > > > + > > > + num = atomic_read(&dip_hw->num_composing); > > > + > > > + mutex_lock(&dip_hw->dip_worklist.queuelock); > > > + dip_work = list_first_entry(&dip_hw->dip_worklist.queue, [snip] > > > + > > > + if (dip_work->frameparams.tuning_data.pa == 0) { > > > + dev_dbg(&dip_dev->pdev->dev, > > > + "%s: frame_no(%d) has no tuning_data\n", > > > + __func__, dip_work->frameparams.frame_no); > > > + > > > + memcpy(&dip_work->frameparams.tuning_data, > > > + &buf->tuning_buf, sizeof(buf->tuning_buf)); > > > > Ditto. > > > > I got it. > > > > + memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ); > > > > Ditto. > > I got it. > > > > > > + /* > > > + * When user enqueued without tuning buffer, > > > + * it would use driver internal buffer. > > > + * So, tuning_data.va should be 0 > > > + */ > > > + dip_work->frameparams.tuning_data.va = 0; > > > > I don't understand this. We just zeroed the buffer via this kernel VA few > > lines above, so why would it have to be set to 0? > > > > I will remove this unnecessary line. > > > > + } After confirming the firmware part, I found that we use this field (tuning_data.va) to notify firmware if there is no tuning data from user. - frameparams.tuning_data.va is 0: use the default tuning data in SCP, but we still need to pass frameparams.tuning_data.pa because the buffer contains some working buffer required. - frameparams.tuning_data.va is not 0: the tuning data was passed from the user Since we should not pass cpu addres to SCP, could I rename tuning_data.va as tuning_data.cookie, and write a constant value to indicate if SCP should use its internal default setting or not here? For example, /* SCP uses tuning data passed from userspace*/ dip_work->frameparams.tuning_data.cookie = MTK_DIP_USER_TUNING_DATA; /* SCP uses internal tuning data */ dip_work->frameparams.tuning_data.cookie = MTK_DIP_DEFAULT_TUNING_DATA; > > > + > > > + dip_work->frameparams.drv_data = (u64)dip_hw; > > > > Passing kernel pointers to firmware? > > I will remove this line. > > > > > > + dip_work->frameparams.state = FRAME_STATE_COMPOSING; > > > + > > > + memcpy((void *)buf->frameparam.va, &dip_work->frameparams, > > > + sizeof(dip_work->frameparams)); > > > > There shouldn't be a need to type cast the pointer. > > > > I will fix it. > > > > + > > > + dip_send(dip_hw->vpu_pdev, SCP_IPI_DIP_FRAME, > > > + (void *)&dip_work->frameparams, > > [snip] Sincerely, Frederic Chen
On Tue, Jun 11, 2019 at 5:48 PM Frederic Chen <frederic.chen@mediatek.com> wrote: > > Dear Tomasz, > > I'd like to elaborate more about the tuning_data.va. > Would you like to give us some advice about our improvement proposal inline? > > Thank you very much. > > > On Wed, 2019-05-22 at 03:14 +0800, Frederic Chen wrote: > > Dear Tomasz, > > > > I appreciate your comment. It is very helpful for us. > > > > > > > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c > > > > new file mode 100644 > > > > index 000000000000..54d2b5f5b802 > > > > --- /dev/null > > > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c > > > > @@ -0,0 +1,1384 @@ > > [snip] > > > > > +static void dip_submit_worker(struct work_struct *work) > > > > +{ > > > > + struct mtk_dip_hw_submit_work *dip_submit_work = > > > > + container_of(work, struct mtk_dip_hw_submit_work, frame_work); > > > > + struct mtk_dip_hw *dip_hw = dip_submit_work->dip_hw; > > > > + struct mtk_dip_dev *dip_dev = mtk_dip_hw_to_dev(dip_hw); > > > > + struct mtk_dip_hw_work *dip_work; > > > > + struct mtk_dip_hw_subframe *buf; > > > > + u32 len, num; > > > > + int ret; > > > > + > > > > + num = atomic_read(&dip_hw->num_composing); > > > > + > > > > + mutex_lock(&dip_hw->dip_worklist.queuelock); > > > > + dip_work = list_first_entry(&dip_hw->dip_worklist.queue, > > [snip] > > > > > + > > > > + if (dip_work->frameparams.tuning_data.pa == 0) { > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > + "%s: frame_no(%d) has no tuning_data\n", > > > > + __func__, dip_work->frameparams.frame_no); > > > > + > > > > + memcpy(&dip_work->frameparams.tuning_data, > > > > + &buf->tuning_buf, sizeof(buf->tuning_buf)); > > > > > > Ditto. > > > > > > > I got it. > > > > > > + memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ); > > > > > > Ditto. > > > > I got it. > > > > > > > > > + /* > > > > + * When user enqueued without tuning buffer, > > > > + * it would use driver internal buffer. > > > > + * So, tuning_data.va should be 0 > > > > + */ > > > > + dip_work->frameparams.tuning_data.va = 0; > > > > > > I don't understand this. We just zeroed the buffer via this kernel VA few > > > lines above, so why would it have to be set to 0? > > > > > > > I will remove this unnecessary line. > > > > > > + } > > After confirming the firmware part, I found that we use this field > (tuning_data.va) to notify firmware if there is no tuning data from > user. > > - frameparams.tuning_data.va is 0: use the default tuning data in > SCP, but we still need to pass > frameparams.tuning_data.pa because > the buffer contains some working > buffer required. > - frameparams.tuning_data.va is not 0: the tuning data was passed from > the user > > Since we should not pass cpu addres to SCP, could I rename tuning_data.va > as tuning_data.cookie, and write a constant value to indicate if SCP > should use its internal default setting or not here? > > For example, > /* SCP uses tuning data passed from userspace*/ > dip_work->frameparams.tuning_data.cookie = MTK_DIP_USER_TUNING_DATA; > > /* SCP uses internal tuning data */ > dip_work->frameparams.tuning_data.cookie = MTK_DIP_DEFAULT_TUNING_DATA; Perhaps we could just call it "present" and set to true or false? Best regards, Tomasz
Hi Tomasz, On Tue, 2019-06-11 at 17:59 +0900, Tomasz Figa wrote: > On Tue, Jun 11, 2019 at 5:48 PM Frederic Chen > <frederic.chen@mediatek.com> wrote: > > > > Dear Tomasz, > > > > I'd like to elaborate more about the tuning_data.va. > > Would you like to give us some advice about our improvement proposal inline? > > > > Thank you very much. > > > > > > On Wed, 2019-05-22 at 03:14 +0800, Frederic Chen wrote: > > > Dear Tomasz, > > > > > > I appreciate your comment. It is very helpful for us. > > > > > > > > > > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c > > > > > new file mode 100644 > > > > > index 000000000000..54d2b5f5b802 > > > > > --- /dev/null > > > > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c > > > > > @@ -0,0 +1,1384 @@ > > > > [snip] > > > > > > > +static void dip_submit_worker(struct work_struct *work) > > > > > +{ > > > > > + struct mtk_dip_hw_submit_work *dip_submit_work = > > > > > + container_of(work, struct mtk_dip_hw_submit_work, frame_work); > > > > > + struct mtk_dip_hw *dip_hw = dip_submit_work->dip_hw; > > > > > + struct mtk_dip_dev *dip_dev = mtk_dip_hw_to_dev(dip_hw); > > > > > + struct mtk_dip_hw_work *dip_work; > > > > > + struct mtk_dip_hw_subframe *buf; > > > > > + u32 len, num; > > > > > + int ret; > > > > > + > > > > > + num = atomic_read(&dip_hw->num_composing); > > > > > + > > > > > + mutex_lock(&dip_hw->dip_worklist.queuelock); > > > > > + dip_work = list_first_entry(&dip_hw->dip_worklist.queue, > > > > [snip] > > > > > > > + > > > > > + if (dip_work->frameparams.tuning_data.pa == 0) { > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "%s: frame_no(%d) has no tuning_data\n", > > > > > + __func__, dip_work->frameparams.frame_no); > > > > > + > > > > > + memcpy(&dip_work->frameparams.tuning_data, > > > > > + &buf->tuning_buf, sizeof(buf->tuning_buf)); > > > > > > > > Ditto. > > > > > > > > > > I got it. > > > > > > > > + memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ); > > > > > > > > Ditto. > > > > > > I got it. > > > > > > > > > > > > + /* > > > > > + * When user enqueued without tuning buffer, > > > > > + * it would use driver internal buffer. > > > > > + * So, tuning_data.va should be 0 > > > > > + */ > > > > > + dip_work->frameparams.tuning_data.va = 0; > > > > > > > > I don't understand this. We just zeroed the buffer via this kernel VA few > > > > lines above, so why would it have to be set to 0? > > > > > > > > > > I will remove this unnecessary line. > > > > > > > > + } > > > > After confirming the firmware part, I found that we use this field > > (tuning_data.va) to notify firmware if there is no tuning data from > > user. > > > > - frameparams.tuning_data.va is 0: use the default tuning data in > > SCP, but we still need to pass > > frameparams.tuning_data.pa because > > the buffer contains some working > > buffer required. > > - frameparams.tuning_data.va is not 0: the tuning data was passed from > > the user > > > > Since we should not pass cpu addres to SCP, could I rename tuning_data.va > > as tuning_data.cookie, and write a constant value to indicate if SCP > > should use its internal default setting or not here? > > > > For example, > > /* SCP uses tuning data passed from userspace*/ > > dip_work->frameparams.tuning_data.cookie = MTK_DIP_USER_TUNING_DATA; > > > > /* SCP uses internal tuning data */ > > dip_work->frameparams.tuning_data.cookie = MTK_DIP_DEFAULT_TUNING_DATA; > > Perhaps we could just call it "present" and set to true or false? > Yes. I would like to use "present" here. Original: struct img_addr { u64 va; /* Used by Linux OS access */ u32 pa; /* Used by CM4 access */ u32 iova; /* Used by IOMMU HW access */ } __attribute__ ((__packed__)); struct img_ipi_frameparam { u32 index; u32 frame_no; u64 timestamp; u8 type; /* enum mdp_stream_type */ u8 state; u8 num_inputs; u8 num_outputs; u64 drv_data; struct img_input inputs[IMG_MAX_HW_INPUTS]; struct img_output outputs[IMG_MAX_HW_OUTPUTS]; struct img_addr tuning_data; struct img_addr subfrm_data; struct img_sw_addr config_data; struct img_sw_addr self_data; } __attribute__ ((__packed__)); Modified: struct tuning_buf { u8 present; u32 pa; /* Used by CM4 access */ u32 iova; /* Used by IOMMU HW access */ } __attribute__ ((__packed__)); struct img_ipi_frameparam { /* ... */ struct tuning_buf tuning_data; /* ... */ } __attribute__ ((__packed__)); > Best regards, > Tomasz Sincerely, Frederic Chen
On Tue, Jun 11, 2019 at 7:07 PM Frederic Chen <frederic.chen@mediatek.com> wrote: > > Hi Tomasz, > > > On Tue, 2019-06-11 at 17:59 +0900, Tomasz Figa wrote: > > On Tue, Jun 11, 2019 at 5:48 PM Frederic Chen > > <frederic.chen@mediatek.com> wrote: > > > > > > Dear Tomasz, > > > > > > I'd like to elaborate more about the tuning_data.va. > > > Would you like to give us some advice about our improvement proposal inline? > > > > > > Thank you very much. > > > > > > > > > On Wed, 2019-05-22 at 03:14 +0800, Frederic Chen wrote: > > > > Dear Tomasz, > > > > > > > > I appreciate your comment. It is very helpful for us. > > > > > > > > > > > > > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c > > > > > > new file mode 100644 > > > > > > index 000000000000..54d2b5f5b802 > > > > > > --- /dev/null > > > > > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c > > > > > > @@ -0,0 +1,1384 @@ > > > > > > [snip] > > > > > > > > > +static void dip_submit_worker(struct work_struct *work) > > > > > > +{ > > > > > > + struct mtk_dip_hw_submit_work *dip_submit_work = > > > > > > + container_of(work, struct mtk_dip_hw_submit_work, frame_work); > > > > > > + struct mtk_dip_hw *dip_hw = dip_submit_work->dip_hw; > > > > > > + struct mtk_dip_dev *dip_dev = mtk_dip_hw_to_dev(dip_hw); > > > > > > + struct mtk_dip_hw_work *dip_work; > > > > > > + struct mtk_dip_hw_subframe *buf; > > > > > > + u32 len, num; > > > > > > + int ret; > > > > > > + > > > > > > + num = atomic_read(&dip_hw->num_composing); > > > > > > + > > > > > > + mutex_lock(&dip_hw->dip_worklist.queuelock); > > > > > > + dip_work = list_first_entry(&dip_hw->dip_worklist.queue, > > > > > > [snip] > > > > > > > > > + > > > > > > + if (dip_work->frameparams.tuning_data.pa == 0) { > > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > > + "%s: frame_no(%d) has no tuning_data\n", > > > > > > + __func__, dip_work->frameparams.frame_no); > > > > > > + > > > > > > + memcpy(&dip_work->frameparams.tuning_data, > > > > > > + &buf->tuning_buf, sizeof(buf->tuning_buf)); > > > > > > > > > > Ditto. > > > > > > > > > > > > > I got it. > > > > > > > > > > + memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ); > > > > > > > > > > Ditto. > > > > > > > > I got it. > > > > > > > > > > > > > > > + /* > > > > > > + * When user enqueued without tuning buffer, > > > > > > + * it would use driver internal buffer. > > > > > > + * So, tuning_data.va should be 0 > > > > > > + */ > > > > > > + dip_work->frameparams.tuning_data.va = 0; > > > > > > > > > > I don't understand this. We just zeroed the buffer via this kernel VA few > > > > > lines above, so why would it have to be set to 0? > > > > > > > > > > > > > I will remove this unnecessary line. > > > > > > > > > > + } > > > > > > After confirming the firmware part, I found that we use this field > > > (tuning_data.va) to notify firmware if there is no tuning data from > > > user. > > > > > > - frameparams.tuning_data.va is 0: use the default tuning data in > > > SCP, but we still need to pass > > > frameparams.tuning_data.pa because > > > the buffer contains some working > > > buffer required. > > > - frameparams.tuning_data.va is not 0: the tuning data was passed from > > > the user > > > > > > Since we should not pass cpu addres to SCP, could I rename tuning_data.va > > > as tuning_data.cookie, and write a constant value to indicate if SCP > > > should use its internal default setting or not here? > > > > > > For example, > > > /* SCP uses tuning data passed from userspace*/ > > > dip_work->frameparams.tuning_data.cookie = MTK_DIP_USER_TUNING_DATA; > > > > > > /* SCP uses internal tuning data */ > > > dip_work->frameparams.tuning_data.cookie = MTK_DIP_DEFAULT_TUNING_DATA; > > > > Perhaps we could just call it "present" and set to true or false? > > > > Yes. I would like to use "present" here. > > Original: > struct img_addr { > u64 va; /* Used by Linux OS access */ > u32 pa; /* Used by CM4 access */ > u32 iova; /* Used by IOMMU HW access */ > } __attribute__ ((__packed__)); > > struct img_ipi_frameparam { > u32 index; > u32 frame_no; > u64 timestamp; > u8 type; /* enum mdp_stream_type */ > u8 state; > u8 num_inputs; > u8 num_outputs; > u64 drv_data; > struct img_input inputs[IMG_MAX_HW_INPUTS]; > struct img_output outputs[IMG_MAX_HW_OUTPUTS]; > struct img_addr tuning_data; > struct img_addr subfrm_data; > struct img_sw_addr config_data; > struct img_sw_addr self_data; > } __attribute__ ((__packed__)); > > > Modified: > struct tuning_buf { > u8 present; I'd make this u32 to keep the other fields aligned. > u32 pa; /* Used by CM4 access */ > u32 iova; /* Used by IOMMU HW access */ > } __attribute__ ((__packed__)); Best regards, Tomasz
Hi Tomasz, On Tue, 2019-06-11 at 19:13 +0900, Tomasz Figa wrote: > On Tue, Jun 11, 2019 at 7:07 PM Frederic Chen > <frederic.chen@mediatek.com> wrote: > > > > Hi Tomasz, > > > > > > On Tue, 2019-06-11 at 17:59 +0900, Tomasz Figa wrote: > > > On Tue, Jun 11, 2019 at 5:48 PM Frederic Chen > > > <frederic.chen@mediatek.com> wrote: > > > > > > > > Dear Tomasz, > > > > > > > > I'd like to elaborate more about the tuning_data.va. > > > > Would you like to give us some advice about our improvement proposal inline? > > > > > > > > Thank you very much. > > > > > > > > > > > > On Wed, 2019-05-22 at 03:14 +0800, Frederic Chen wrote: > > > > > Dear Tomasz, > > > > > > > > > > I appreciate your comment. It is very helpful for us. > > > > > > > > > > > > > > > > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c > > > > > > > new file mode 100644 > > > > > > > index 000000000000..54d2b5f5b802 > > > > > > > --- /dev/null > > > > > > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c > > > > > > > @@ -0,0 +1,1384 @@ > > > > > > > > [snip] > > > > > > > > > > > +static void dip_submit_worker(struct work_struct *work) > > > > > > > +{ > > > > > > > + struct mtk_dip_hw_submit_work *dip_submit_work = > > > > > > > + container_of(work, struct mtk_dip_hw_submit_work, frame_work); > > > > > > > + struct mtk_dip_hw *dip_hw = dip_submit_work->dip_hw; > > > > > > > + struct mtk_dip_dev *dip_dev = mtk_dip_hw_to_dev(dip_hw); > > > > > > > + struct mtk_dip_hw_work *dip_work; > > > > > > > + struct mtk_dip_hw_subframe *buf; > > > > > > > + u32 len, num; > > > > > > > + int ret; > > > > > > > + > > > > > > > + num = atomic_read(&dip_hw->num_composing); > > > > > > > + > > > > > > > + mutex_lock(&dip_hw->dip_worklist.queuelock); > > > > > > > + dip_work = list_first_entry(&dip_hw->dip_worklist.queue, > > > > > > > > [snip] > > > > > > > > > > > + > > > > > > > + if (dip_work->frameparams.tuning_data.pa == 0) { > > > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > > > + "%s: frame_no(%d) has no tuning_data\n", > > > > > > > + __func__, dip_work->frameparams.frame_no); > > > > > > > + > > > > > > > + memcpy(&dip_work->frameparams.tuning_data, > > > > > > > + &buf->tuning_buf, sizeof(buf->tuning_buf)); > > > > > > > > > > > > Ditto. > > > > > > > > > > > > > > > > I got it. > > > > > > > > > > > > + memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ); > > > > > > > > > > > > Ditto. > > > > > > > > > > I got it. > > > > > > > > > > > > > > > > > > + /* > > > > > > > + * When user enqueued without tuning buffer, > > > > > > > + * it would use driver internal buffer. > > > > > > > + * So, tuning_data.va should be 0 > > > > > > > + */ > > > > > > > + dip_work->frameparams.tuning_data.va = 0; > > > > > > > > > > > > I don't understand this. We just zeroed the buffer via this kernel VA few > > > > > > lines above, so why would it have to be set to 0? > > > > > > > > > > > > > > > > I will remove this unnecessary line. > > > > > > > > > > > > + } > > > > > > > > After confirming the firmware part, I found that we use this field > > > > (tuning_data.va) to notify firmware if there is no tuning data from > > > > user. > > > > > > > > - frameparams.tuning_data.va is 0: use the default tuning data in > > > > SCP, but we still need to pass > > > > frameparams.tuning_data.pa because > > > > the buffer contains some working > > > > buffer required. > > > > - frameparams.tuning_data.va is not 0: the tuning data was passed from > > > > the user > > > > > > > > Since we should not pass cpu addres to SCP, could I rename tuning_data.va > > > > as tuning_data.cookie, and write a constant value to indicate if SCP > > > > should use its internal default setting or not here? > > > > > > > > For example, > > > > /* SCP uses tuning data passed from userspace*/ > > > > dip_work->frameparams.tuning_data.cookie = MTK_DIP_USER_TUNING_DATA; > > > > > > > > /* SCP uses internal tuning data */ > > > > dip_work->frameparams.tuning_data.cookie = MTK_DIP_DEFAULT_TUNING_DATA; > > > > > > Perhaps we could just call it "present" and set to true or false? > > > > > > > Yes. I would like to use "present" here. > > > > Original: > > struct img_addr { > > u64 va; /* Used by Linux OS access */ > > u32 pa; /* Used by CM4 access */ > > u32 iova; /* Used by IOMMU HW access */ > > } __attribute__ ((__packed__)); > > > > struct img_ipi_frameparam { > > u32 index; > > u32 frame_no; > > u64 timestamp; > > u8 type; /* enum mdp_stream_type */ > > u8 state; > > u8 num_inputs; > > u8 num_outputs; > > u64 drv_data; > > struct img_input inputs[IMG_MAX_HW_INPUTS]; > > struct img_output outputs[IMG_MAX_HW_OUTPUTS]; > > struct img_addr tuning_data; > > struct img_addr subfrm_data; > > struct img_sw_addr config_data; > > struct img_sw_addr self_data; > > } __attribute__ ((__packed__)); > > > > > > Modified: > > struct tuning_buf { > > u8 present; > > I'd make this u32 to keep the other fields aligned. > > > u32 pa; /* Used by CM4 access */ > > u32 iova; /* Used by IOMMU HW access */ > > } __attribute__ ((__packed__)); > We will use u32 to keep the fields aligned in next patch. > Best regards, > Tomasz Sincerely, Frederic Chen
Dear Tomasz, Would you comment on the following points in further? Thank you for the review. On Thu, 2019-05-09 at 18:48 +0900, Tomasz Figa wrote: > Hi Frederic, > [snip] > > +int mtk_dip_pipe_job_start(struct mtk_dip_pipe *dip_pipe, > > + struct mtk_dip_pipe_job_info *pipe_job_info) > > +{ > > + struct platform_device *pdev = dip_pipe->dip_dev->pdev; > > + int ret; > > + int out_img_buf_idx; > > + struct img_ipi_frameparam dip_param; > > + struct mtk_dip_dev_buffer *dev_buf_in; > > + struct mtk_dip_dev_buffer *dev_buf_out; > > + struct mtk_dip_dev_buffer *dev_buf_tuning; > > + > > + if (!pipe_job_info) { > > + dev_err(&pdev->dev, > > + "pipe_job_info(%p) in start can't be NULL\n", > > + pipe_job_info); > > + return -EINVAL; > > + } > > This should be impossible to happen. > > > + > > + /* We need RAW and at least MDP0 or MDP 1 buffer */ > > + if (!pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_RAW_OUT] || > > + (!pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_MDP0_CAPTURE] && > > + !pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_MDP1_CAPTURE])){ > > + struct mtk_dip_dev_buffer **map = pipe_job_info->buf_map; > > + > > + dev_dbg(&pdev->dev, > > + "can't trigger job: raw(%p), mdp0(%p), mdp1(%p)\n", > > + map[MTK_DIP_VIDEO_NODE_ID_RAW_OUT], > > + map[MTK_DIP_VIDEO_NODE_ID_MDP0_CAPTURE], > > + map[MTK_DIP_VIDEO_NODE_ID_MDP1_CAPTURE]); > > + return -EINVAL; > > This must be validated at the time of request_validate. We can't fail at > this stage anymore. After the modification about checking the required buffers in req_validate(), we got failed in the following testRequests() of V4L2 compliance test. The V4L2 compliance test case doesn't know which buffers of the video devices are required and expects that the MEDIA_REQUEST_IOC_QUEUE succeed when the request has any valid buffer. For example, when the request has MDP 0 buffer only, the DIP's req_validate() should return an error since it also need a buffer from RAW video device, but it make compliance test get failed. May I still check the required buffers in req_validate() in the next patch? I will add some note to explain that the compliance test failed item is related to the limitation? ======================================================= int testRequests(struct node *node, bool test_streaming) // ...... if (i) fail_on_test(!buf.qbuf(node)); buf.s_flags(buf.g_flags() | V4L2_BUF_FLAG_REQUEST_FD); buf.s_request_fd(buf_req_fds[i]); buf.s_field(V4L2_FIELD_ANY); fail_on_test(buf.qbuf(node)); if (v4l_type_is_video(buf.g_type()) && v4l_type_is_output(buf.g_type())) fail_on_test(buf.g_field() == V4L2_FIELD_ANY); fail_on_test(buf.querybuf(node, i)); // ...... // LINE 1807 in v4l2-test-buffers.cpp, we will got the failed here. // Since we need one RAW and one MDP0 buffer at least. // v4l2-test-buffers.cpp(1807): doioctl_fd(buf_req_fds[i], // MEDIA_REQUEST_IOC_QUEUE, 0) // test Requests: FAIL fail_on_test(doioctl_fd(buf_req_fds[i], MEDIA_REQUEST_IOC_QUEUE, 0)); ======================================================= > > + > > +static int mtk_dip_vb2_queue_setup(struct vb2_queue *vq, > > + unsigned int *num_buffers, > > + unsigned int *num_planes, > > + unsigned int sizes[], > > + struct device *alloc_devs[]) > > +{ > > + struct mtk_dip_pipe *dip_pipe = vb2_get_drv_priv(vq); > > + struct mtk_dip_video_device *node = > > + mtk_dip_vbq_to_node(vq); > > + struct device *dev = &dip_pipe->dip_dev->pdev->dev; > > + struct device *buf_alloc_ctx; > > + [snip] > > + > > + if (vq->type == V4L2_BUF_TYPE_META_CAPTURE || > > + vq->type == V4L2_BUF_TYPE_META_OUTPUT) > > + size = fmt->fmt.meta.buffersize; > > + else > > + size = fmt->fmt.pix_mp.plane_fmt[0].sizeimage; > > + > > + if (*num_planes) { > > + if (sizes[0] < size) { > > + dev_dbg(dev, "%s:%s:%s: size error(user:%d, max:%d)\n", > > + __func__, dip_pipe->desc->name, > > + node->desc->name, sizes[0], size); > > + return -EINVAL; > > + } > > I don't think this is an error. This is for handling VIDIOC_CREATE_BUFS, > which can allocate for any arbitrary format. > > Whether the size of the buffer is big enough for current format should be > checked in .buf_prepare callback. When executing V4L2 compliance test, we need to check this image size to pass the following q.create_bufs() test (LINE:709, v4l2-test-buffers.cpp). ======================================================== node->g_fmt(fmt, q.g_type()); //.... fmt.s_height(fmt.g_height() / 2); for (unsigned p = 0; p < fmt.g_num_planes(); p++) fmt.s_sizeimage(fmt.g_sizeimage(p) / 2, p); // LINE 709 in v4l2-test-buffers.cpp // It seems that the driver needs to return EINVAL when the buffer //size is smaller than the sizeimage required fail_on_test(q.create_bufs(node, 1, &fmt) != EINVAL); ======================================================== The kernel document has some similar description on VIDIOC_CREATE_BUFS. https://www.kernel.org/doc/html/latest/media/uapi/v4l/vidioc-create-bufs.html ======================================================= Usually if the format.pix.sizeimage field is less than the minimum required for the given format, then an error will be returned since drivers will typically not allow this. ======================================================= Should we check the image size of each plane here so that we can pass the test? > > > + } else { > > + *num_planes = 1; > > + sizes[0] = size; > > + } > > + > > + dev_dbg(dev, "%s:%s:%s: n_planes(%d), n_bufs(%d), size(%d)\n", > > + __func__, dip_pipe->desc->name, > > + node->desc->name, *num_planes, *num_buffers, sizes[0]); > > + > > + return 0; > > +} > > + Sincerely, Frederic Chen
Hi Frederic, On Tue, Jun 25, 2019 at 9:16 PM Frederic Chen <frederic.chen@mediatek.com> wrote: > > Dear Tomasz, > > Would you comment on the following points in further? Thank you for the > review. > > On Thu, 2019-05-09 at 18:48 +0900, Tomasz Figa wrote: > > Hi Frederic, > > > > [snip] > > > > +int mtk_dip_pipe_job_start(struct mtk_dip_pipe *dip_pipe, > > > + struct mtk_dip_pipe_job_info *pipe_job_info) > > > +{ > > > + struct platform_device *pdev = dip_pipe->dip_dev->pdev; > > > + int ret; > > > + int out_img_buf_idx; > > > + struct img_ipi_frameparam dip_param; > > > + struct mtk_dip_dev_buffer *dev_buf_in; > > > + struct mtk_dip_dev_buffer *dev_buf_out; > > > + struct mtk_dip_dev_buffer *dev_buf_tuning; > > > + > > > + if (!pipe_job_info) { > > > + dev_err(&pdev->dev, > > > + "pipe_job_info(%p) in start can't be NULL\n", > > > + pipe_job_info); > > > + return -EINVAL; > > > + } > > > > This should be impossible to happen. > > > > > + > > > + /* We need RAW and at least MDP0 or MDP 1 buffer */ > > > + if (!pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_RAW_OUT] || > > > + (!pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_MDP0_CAPTURE] && > > > + !pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_MDP1_CAPTURE])){ > > > + struct mtk_dip_dev_buffer **map = pipe_job_info->buf_map; > > > + > > > + dev_dbg(&pdev->dev, > > > + "can't trigger job: raw(%p), mdp0(%p), mdp1(%p)\n", > > > + map[MTK_DIP_VIDEO_NODE_ID_RAW_OUT], > > > + map[MTK_DIP_VIDEO_NODE_ID_MDP0_CAPTURE], > > > + map[MTK_DIP_VIDEO_NODE_ID_MDP1_CAPTURE]); > > > + return -EINVAL; > > > > This must be validated at the time of request_validate. We can't fail at > > this stage anymore. > > After the modification about checking the required buffers in > req_validate(), we got failed in the following testRequests() > of V4L2 compliance test. The V4L2 compliance test case doesn't know > which buffers of the video devices are required and expects that the > MEDIA_REQUEST_IOC_QUEUE succeed when the request has any valid buffer. > > For example, when the request has MDP 0 buffer only, the DIP's > req_validate() should return an error since it also need a buffer > from RAW video device, but it make compliance test get failed. > > May I still check the required buffers in req_validate() in the next > patch? I will add some note to explain that the compliance test failed > item is related to the limitation? > > ======================================================= > int testRequests(struct node *node, bool test_streaming) > // ...... > if (i) > fail_on_test(!buf.qbuf(node)); > buf.s_flags(buf.g_flags() | V4L2_BUF_FLAG_REQUEST_FD); > buf.s_request_fd(buf_req_fds[i]); > buf.s_field(V4L2_FIELD_ANY); > fail_on_test(buf.qbuf(node)); > if (v4l_type_is_video(buf.g_type()) && v4l_type_is_output(buf.g_type())) > fail_on_test(buf.g_field() == V4L2_FIELD_ANY); > fail_on_test(buf.querybuf(node, i)); > > // ...... > > // LINE 1807 in v4l2-test-buffers.cpp, we will got the failed here. > // Since we need one RAW and one MDP0 buffer at least. > // v4l2-test-buffers.cpp(1807): doioctl_fd(buf_req_fds[i], > // MEDIA_REQUEST_IOC_QUEUE, 0) > // test Requests: FAIL > fail_on_test(doioctl_fd(buf_req_fds[i], MEDIA_REQUEST_IOC_QUEUE, 0)); > ======================================================= > Sounds like a limitation of the compliance test. Request API testing there is still new and possibly just made for simple mem-to-mem devices. Hans, the driver always requires some buffers to be given, like the raw frame input, while other, e.g. downscaled output, are optional. Any ideas? > > > + > > > +static int mtk_dip_vb2_queue_setup(struct vb2_queue *vq, > > > + unsigned int *num_buffers, > > > + unsigned int *num_planes, > > > + unsigned int sizes[], > > > + struct device *alloc_devs[]) > > > +{ > > > + struct mtk_dip_pipe *dip_pipe = vb2_get_drv_priv(vq); > > > + struct mtk_dip_video_device *node = > > > + mtk_dip_vbq_to_node(vq); > > > + struct device *dev = &dip_pipe->dip_dev->pdev->dev; > > > + struct device *buf_alloc_ctx; > > > + > > [snip] > > > > + > > > + if (vq->type == V4L2_BUF_TYPE_META_CAPTURE || > > > + vq->type == V4L2_BUF_TYPE_META_OUTPUT) > > > + size = fmt->fmt.meta.buffersize; > > > + else > > > + size = fmt->fmt.pix_mp.plane_fmt[0].sizeimage; > > > + > > > + if (*num_planes) { > > > + if (sizes[0] < size) { > > > + dev_dbg(dev, "%s:%s:%s: size error(user:%d, max:%d)\n", > > > + __func__, dip_pipe->desc->name, > > > + node->desc->name, sizes[0], size); > > > + return -EINVAL; > > > + } > > > > I don't think this is an error. This is for handling VIDIOC_CREATE_BUFS, > > which can allocate for any arbitrary format. > > > > Whether the size of the buffer is big enough for current format should be > > checked in .buf_prepare callback. > > When executing V4L2 compliance test, we need to check this image size to > pass the following q.create_bufs() test (LINE:709, > v4l2-test-buffers.cpp). > > ======================================================== > node->g_fmt(fmt, q.g_type()); > //.... > fmt.s_height(fmt.g_height() / 2); > for (unsigned p = 0; p < fmt.g_num_planes(); p++) > fmt.s_sizeimage(fmt.g_sizeimage(p) / 2, p); > > // LINE 709 in v4l2-test-buffers.cpp > // It seems that the driver needs to return EINVAL when the buffer > //size is smaller than the sizeimage required > fail_on_test(q.create_bufs(node, 1, &fmt) != EINVAL); > ======================================================== > > The kernel document has some similar description on VIDIOC_CREATE_BUFS. > > https://www.kernel.org/doc/html/latest/media/uapi/v4l/vidioc-create-bufs.html > > ======================================================= > Usually if the format.pix.sizeimage field is less than the minimum > required for the given format, then an error will be returned since > drivers will typically not allow this. > ======================================================= > > Should we check the image size of each plane here so that we can pass > the test? Note that "given format" there means the format field of structv4l2_create_buffers, _not_ the currently active format. That's really strange because we don't get that inside queue_setup. Hans, how should we handle this in the driver? Right now we just call vb2_create_bufs(), which doesn't care about anything else than sizeimage. Do we need to implement our own .vidioc_create_bufs() handler that validates the sizeimage wrt the other parts of the given format before calling vb2_create_bufs()? Another thing is that the spec isn't very precise on this, especially given the "usually" and "typically" in the description quoted above. Best regards, Tomasz