Re: [RFC PATCH V0 7/7] [media] platform: mtk-isp: Add Mediatek DIP driver

[Brian Norris <briannorris@chromium.org>]

Hi,

On Fri, Feb 01, 2019 at 07:21:31PM +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 +
>  drivers/media/platform/mtk-isp/isp_50/Makefile     |   17 +
>  drivers/media/platform/mtk-isp/isp_50/dip/Makefile |   35 +
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-core.h     |  188 +++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c     |  173 +++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h     |   43 +
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h      |  319 ++++
>  .../mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c      | 1643 ++++++++++++++++++++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.c      |  374 +++++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.h      |  191 +++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-smem-drv.c |  452 ++++++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-smem.h     |   25 +
>  .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c         | 1000 ++++++++++++
>  .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h         |   38 +
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c     |  292 ++++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h     |   60 +
>  .../media/platform/mtk-isp/isp_50/dip/mtk_dip.c    | 1385 +++++++++++++++++
>  .../media/platform/mtk-isp/isp_50/dip/mtk_dip.h    |   93 ++
>  18 files changed, 6346 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-core.h
>  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-ctrl.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.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-smem-drv.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-v4l2-util.c
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.h
> 

...

> 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 0000000..9d29507
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c
> @@ -0,0 +1,173 @@
> +// 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"
> +#include "mtk_dip-ctrl.h"
> +
> +#define CONFIG_MTK_DIP_COMMON_UT

Please don't do this. You're pretending to have configurability that you
don't actually have.

> +
> +static void handle_buf_usage_config(struct v4l2_ctrl *ctrl);
> +static void handle_buf_rotate_config(struct v4l2_ctrl *ctrl);
> +static int mtk_dip_ctx_s_ctrl(struct v4l2_ctrl *ctrl);
> +
> +static void handle_buf_usage_config(struct v4l2_ctrl *ctrl)
> +{
> +	struct mtk_dip_ctx_queue *queue =
> +		container_of(ctrl->handler,
> +			     struct mtk_dip_ctx_queue, 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;
> +	}
> +	queue->buffer_usage = ctrl->val;
> +}
> +
> +static void handle_buf_rotate_config(struct v4l2_ctrl *ctrl)
> +{
> +	struct mtk_dip_ctx_queue *queue =
> +		container_of(ctrl->handler,
> +			     struct mtk_dip_ctx_queue, ctrl_handler);
> +
> +	if (ctrl->val != 0 || ctrl->val != 90 ||
> +	    ctrl->val != 180 || ctrl->val != 270) {
> +		pr_err("Invalid buffer rotation %d", ctrl->val);
> +		return;
> +	}
> +	queue->rotation = ctrl->val;
> +}
> +
> +static const struct v4l2_ctrl_ops mtk_dip_ctx_ctrl_ops = {
> +	.s_ctrl = mtk_dip_ctx_s_ctrl,
> +};
> +
> +#ifdef CONFIG_MTK_DIP_COMMON_UT

Kill the #ifdef if you're not actually going to make this a Kconfig.
Same elsewhere.

> +
> +static void handle_ctrl_common_util_ut_set_debug_mode
> +	(struct v4l2_ctrl *ctrl)
> +{
> +	struct mtk_dip_ctx *dev_ctx =
> +		container_of(ctrl->handler, struct mtk_dip_ctx, ctrl_handler);
> +	dev_ctx->mode = ctrl->val;
> +	dev_dbg(&dev_ctx->pdev->dev, "Set ctx(id = %d) mode to %d\n",
> +		dev_ctx->ctx_id, dev_ctx->mode);
> +}
> +
> +static const struct v4l2_ctrl_config mtk_dip_mode_config = {
> +	.ops	= &mtk_dip_ctx_ctrl_ops,
> +	.id	= V4L2_CID_PRIVATE_SET_CTX_MODE_NUM,
> +	.name	= "MTK ISP UNIT TEST CASE",
> +	.type	= V4L2_CTRL_TYPE_INTEGER,
> +	.min	= 0,
> +	.max	= 65535,
> +	.step	= 1,
> +	.def	= 0,
> +	.flags	= V4L2_CTRL_FLAG_SLIDER | V4L2_CTRL_FLAG_EXECUTE_ON_WRITE,
> +};
> +#endif /* CONFIG_MTK_DIP_COMMON_UT */
> +
> +static int mtk_dip_ctx_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	switch (ctrl->id) {
> +	#ifdef CONFIG_MTK_DIP_COMMON_UT
> +	case V4L2_CID_PRIVATE_SET_CTX_MODE_NUM:
> +		handle_ctrl_common_util_ut_set_debug_mode(ctrl);
> +		break;
> +	#endif /* CONFIG_MTK_DIP_COMMON_UT */
> +	default:
> +			break;
> +	}
> +	return 0;
> +}
> +

...

> diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c
> new file mode 100644
> index 0000000..c735919
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c
> @@ -0,0 +1,1643 @@
...

> +#ifdef MTK_DIP_CTX_DIP_V4L2_UT

What is this #ifdef'ery? I don't see MTK_DIP_CTX_DIP_V4L2_UT anywhere.

> +static int check_and_refill_dip_ut_start_ipi_param
> +	(struct img_ipi_frameparam *ipi_param,
> +	 struct mtk_dip_ctx_buffer *ctx_buf_in,
> +	 struct mtk_dip_ctx_buffer *ctx_buf_out)
> +{
> +	/* Check the buffer size information from user space */
> +	int ret = 0;
> +	unsigned char *buffer_ptr = NULL;
> +	const unsigned int src_width = 3264;
...

> diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c
> new file mode 100644
> index 0000000..b425031
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c
> @@ -0,0 +1,1000 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 Mediatek Corporation.
> + * Copyright (c) 2017 Intel Corporation.
> + *
> + * 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.
> + *
> + * MTK_DIP-v4l2 is highly based on Intel IPU 3 chrome driver
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/videodev2.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/videobuf2-dma-contig.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-event.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +
> +#include "mtk_dip-dev.h"
> +#include "mtk_dip-v4l2-util.h"
> +
> +static u32 mtk_dip_node_get_v4l2_cap
> +	(struct mtk_dip_ctx_queue *node_ctx);
> +
> +static int mtk_dip_videoc_s_meta_fmt(struct file *file,
> +				     void *fh, struct v4l2_format *f);
> +
> +static int mtk_dip_subdev_open(struct v4l2_subdev *sd,
> +			       struct v4l2_subdev_fh *fh)
> +{
> +	struct mtk_dip_dev *isp_dev = mtk_dip_subdev_to_dev(sd);
> +
> +	isp_dev->ctx.fh = fh;
> +
> +	return mtk_dip_ctx_open(&isp_dev->ctx);
> +}
> +
> +static int mtk_dip_subdev_close(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_fh *fh)
> +{
> +	struct mtk_dip_dev *isp_dev = mtk_dip_subdev_to_dev(sd);
> +
> +	return mtk_dip_ctx_release(&isp_dev->ctx);
> +}
> +
> +static int mtk_dip_subdev_s_stream(struct v4l2_subdev *sd,
> +				   int enable)
> +{
> +	int ret = 0;
> +
> +	struct mtk_dip_dev *isp_dev = mtk_dip_subdev_to_dev(sd);
> +
> +	if (enable) {
> +		ret = mtk_dip_ctx_streamon(&isp_dev->ctx);
> +
> +		if (!ret)
> +			ret = mtk_dip_dev_queue_buffers
> +				(mtk_dip_ctx_to_dev(&isp_dev->ctx), 1);
> +		if (ret)
> +			pr_err("failed to queue initial buffers (%d)", ret);
> +	}	else {
> +		ret = mtk_dip_ctx_streamoff(&isp_dev->ctx);
> +	}
> +
> +	if (!ret)
> +		isp_dev->mem2mem2.streaming = enable;
> +
> +	return ret;
> +}
> +
> +int mtk_dip_subdev_subscribe_event(struct v4l2_subdev *subdev,
> +				   struct v4l2_fh *fh,
> +				   struct v4l2_event_subscription *sub)

Should be static.

> +{
> +	pr_info("sub type(%x)", sub->type);

I feel like you have this problem in other places too: this definitely
shouldn't be at KERN_INFO level. It seems a bit excessive anyway.

> +	if (sub->type != V4L2_EVENT_PRIVATE_START &&
> +	    sub->type != V4L2_EVENT_MTK_DIP_FRAME_DONE)
> +		return -EINVAL;
> +
> +	return v4l2_event_subscribe(fh, sub, 0, NULL);
> +}
> +
> +int mtk_dip_subdev_unsubscribe_event(struct v4l2_subdev *subdev,
> +				     struct v4l2_fh *fh,

Static.

> +	struct v4l2_event_subscription *sub)
> +{
> +	return v4l2_event_unsubscribe(fh, sub);
> +}
> +
> +static int mtk_dip_link_setup(struct media_entity *entity,
> +			      const struct media_pad *local,
> +	const struct media_pad *remote, u32 flags)
> +{
> +	struct mtk_dip_mem2mem2_device *m2m2 =
> +			container_of(entity,
> +				     struct mtk_dip_mem2mem2_device,
> +				     subdev.entity);
> +	struct mtk_dip_dev *isp_dev =
> +		container_of(m2m2, struct mtk_dip_dev, mem2mem2);
> +
> +	u32 pad = local->index;
> +
> +	dev_dbg(&isp_dev->pdev->dev,
> +		"link setup: %d --> %d\n", pad, remote->index);
> +
> +	WARN_ON(entity->obj_type != MEDIA_ENTITY_TYPE_V4L2_SUBDEV);
> +
> +	WARN_ON(pad >= m2m2->num_nodes);
> +
> +	m2m2->nodes[pad].enabled = !!(flags & MEDIA_LNK_FL_ENABLED);
> +
> +	/* queue_enable can be phase out in the future since */
> +	/* we don't have internal queue of each node in */
> +	/* v4l2 common module */
> +	isp_dev->queue_enabled[pad] = m2m2->nodes[pad].enabled;
> +
> +	return 0;
> +}
> +
> +static void mtk_dip_vb2_buf_queue(struct vb2_buffer *vb)
> +{
> +	struct mtk_dip_mem2mem2_device *m2m2 = vb2_get_drv_priv(vb->vb2_queue);
> +
> +	struct mtk_dip_dev *mtk_dip_dev = mtk_dip_m2m_to_dev(m2m2);
> +
> +	struct device *dev = &mtk_dip_dev->pdev->dev;
> +
> +	struct mtk_dip_dev_buffer *buf = NULL;
> +
> +	struct vb2_v4l2_buffer *v4l2_buf = NULL;
> +
> +	struct mtk_dip_dev_video_device *node =
> +		mtk_dip_vbq_to_isp_node(vb->vb2_queue);
> +
> +	int queue = mtk_dip_dev_get_queue_id_of_dev_node(mtk_dip_dev, node);

You've got a lot of extra blank lines in here.

> +
> +	dev_dbg(dev,
> +		"queue vb2_buf: Node(%s) queue id(%d)\n",
> +		node->name,
> +		queue);
> +
> +	if (queue < 0) {
> +		dev_err(m2m2->dev, "Invalid mtk_dip_dev node.\n");
> +		return;
> +	}
> +
> +	if (mtk_dip_dev->ctx.mode == MTK_DIP_CTX_MODE_DEBUG_BYPASS_ALL) {
> +		dev_dbg(m2m2->dev, "By pass mode, just loop back the buffer\n");
> +		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> +		return;
> +	}
> +
> +	if (!vb)
> +		pr_err("VB can't be null\n");
> +
> +	buf = mtk_dip_vb2_buf_to_dev_buf(vb);
> +
> +	if (!buf)
> +		pr_err("buf can't be null\n");
> +
> +	v4l2_buf = to_vb2_v4l2_buffer(vb);
> +
> +	if (!v4l2_buf)
> +		pr_err("v4l2_buf can't be null\n");
> +
> +	mutex_lock(&mtk_dip_dev->lock);
> +
> +	/* the dma address will be filled in later frame buffer handling*/
> +	mtk_dip_ctx_buf_init(&buf->ctx_buf, queue, (dma_addr_t)0);
> +
> +	/* Added the buffer into the tracking list */
> +	list_add_tail(&buf->m2m2_buf.list,
> +		      &m2m2->nodes[node - m2m2->nodes].buffers);
> +	mutex_unlock(&mtk_dip_dev->lock);
> +
> +	/* Enqueue the buffer */
> +	if (mtk_dip_dev->mem2mem2.streaming) {
> +		dev_dbg(dev, "%s: mtk_dip_dev_queue_buffers\n",
> +			node->name);
> +		mtk_dip_dev_queue_buffers(mtk_dip_dev, 0);
> +	}
> +}
...

> diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c
> new file mode 100644
> index 0000000..ffdc45e
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c
> @@ -0,0 +1,292 @@
> +// 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-ctx.h"
> +#include "mtk_dip.h"
> +#include "mtk_dip-v4l2.h"
> +#include "mtk-mdp3-regs.h"
> +
> +#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"
> +
> +#ifdef MTK_DIP_CTX_DIP_V4L2_UT
> +#include "mtk_dip-dev-ctx-dip-test.h"

The above macros was never defined, and this header doesn't exist.
Please remove.

> +#endif
> +

...

> +int mtk_dip_ctx_dip_v4l2_init(struct platform_device *pdev,
> +			      struct mtk_dip_dev *isp_preview_dev,
> +	struct mtk_dip_dev *isp_capture_dev)
> +{
> +	struct media_device *media_dev;
> +	struct v4l2_device *v4l2_dev;
> +	struct v4l2_ctrl_handler *ctrl_handler;
> +	int ret = 0;
> +
> +	/* initialize the v4l2 common part */
> +	dev_info(&pdev->dev, "init v4l2 common part: dev=%llx\n",
> +		 (unsigned long long)&pdev->dev);
> +
> +	media_dev = &isp_preview_dev->media_dev;
> +	v4l2_dev = &isp_preview_dev->v4l2_dev;
> +	ctrl_handler = &isp_preview_dev->ctx.ctrl_handler;
> +
> +	ret = mtk_dip_media_register(&pdev->dev,
> +				     media_dev,
> +		MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME);
> +
> +	ret = mtk_dip_v4l2_register(&pdev->dev,
> +				    media_dev,
> +		v4l2_dev,
> +		ctrl_handler);
> +
> +	dev_info(&pdev->dev, "init v4l2 preview part\n");
> +	ret = mtk_dip_dev_core_init_ext(pdev,
> +					isp_preview_dev,
> +					&mtk_dip_ctx_desc_dip_preview,
> +		media_dev, v4l2_dev);
> +
> +	if (ret)
> +		dev_err(&pdev->dev, "Preview v4l2 part init failed: %d\n", ret);
> +
> +	dev_info(&pdev->dev, "init v4l2 capture part\n");
> +
> +	ret = mtk_dip_dev_core_init_ext(pdev,
> +					isp_capture_dev,
> +					&mtk_dip_ctx_desc_dip_capture,
> +		media_dev, v4l2_dev);
> +
> +	if (ret)
> +		dev_err(&pdev->dev, "Capture v4l2 part init failed: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +/* MTK ISP context initialization */
> +int mtk_dip_ctx_dip_preview_init(struct mtk_dip_ctx *preview_ctx)
> +{
> +	preview_ctx->ctx_id = MTK_DIP_CTX_P2_ID_PREVIEW;
> +
> +	/* Initialize main data structure */
> +	mtk_dip_ctx_core_queue_setup(preview_ctx, &preview_queues_setting);
> +
> +	return mtk_dip_ctx_core_steup(preview_ctx,
> +		&mtk_dip_ctx_dip_preview_setting);
> +}
> +EXPORT_SYMBOL_GPL(mtk_dip_ctx_dip_preview_init);
> +
> +/* MTK ISP context initialization */
> +int mtk_dip_ctx_dip_capture_init(struct mtk_dip_ctx *capture_ctx)
> +{
> +	capture_ctx->ctx_id =  MTK_DIP_CTX_P2_ID_CAPTURE;
> +	/* Initialize main data structure */
> +	mtk_dip_ctx_core_queue_setup(capture_ctx, &capture_queues_setting);
> +
> +	return mtk_dip_ctx_core_steup(capture_ctx,
> +		&mtk_dip_ctx_dip_capture_setting);
> +}
> +EXPORT_SYMBOL_GPL(mtk_dip_ctx_dip_capture_init);

...

> diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c
> new file mode 100644
> index 0000000..3569c7c
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c
> @@ -0,0 +1,1385 @@
> +// SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
> +/*
> + * 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.
> + */
> +
> +#include <linux/of_device.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/kthread.h> /* thread functions */
> +#include <linux/pm_runtime.h>
> +#include <linux/dma-iommu.h>
> +#include <linux/spinlock.h>
> +#include <linux/slab.h> /* kzalloc and kfree */
> +
> +#include "mtk_vpu.h"
> +#include "mtk-mdp3-cmdq.h"
> +
> +#include "mtk_dip-dev.h"
> +#include "mtk_dip.h"
> +#include "mtk_dip-core.h"
> +#include "mtk_dip-v4l2.h"
> +
> +#define DIP_DEV_NAME		"camera-dip"
> +
> +#define DIP_COMPOSER_THREAD_TIMEOUT     (16U)
> +#define DIP_COMPOSING_WQ_TIMEOUT	(16U)
> +#define DIP_COMPOSING_MAX_NUM		(3)
> +#define DIP_FLUSHING_WQ_TIMEOUT		(16U)
> +
> +#define DIP_MAX_ERR_COUNT		(188U)
> +
> +#define DIP_FRM_SZ		(76 * 1024)
> +#define DIP_SUB_FRM_SZ		(16 * 1024)
> +#define DIP_TUNING_SZ		(32 * 1024)
> +#define DIP_COMP_SZ		(24 * 1024)
> +#define DIP_FRAMEPARAM_SZ	(4 * 1024)
> +
> +#define DIP_TUNING_OFFSET	(DIP_SUB_FRM_SZ)
> +#define DIP_COMP_OFFSET		(DIP_TUNING_OFFSET + DIP_TUNING_SZ)
> +#define DIP_FRAMEPARAM_OFFSET	(DIP_COMP_OFFSET + DIP_COMP_SZ)
> +
> +#define DIP_SUB_FRM_DATA_NUM	(32)
> +
> +#define DIP_SCP_WORKINGBUF_OFFSET	(5 * 1024 * 1024)
> +
> +#define DIP_GET_ID(x)			(((x) & 0xffff0000) >> 16)
> +
> +static const struct of_device_id dip_of_ids[] = {
> +	/* Remider: Add this device node manually in .dtsi */
> +	{ .compatible = "mediatek,mt8183-dip", },
> +	{}
> +};

Please add:

MODULE_DEVICE_TABLE(of, dip_of_ids);

> +
> +static void call_mtk_dip_ctx_finish(struct dip_device *dip_dev,
> +				    struct img_ipi_frameparam *iparam);
> +
> +static struct img_frameparam *dip_create_framejob(int sequence)
> +{
> +	struct dip_frame_job *fjob = NULL;
> +
> +	fjob = kzalloc(sizeof(*fjob), GFP_ATOMIC);
> +
> +	if (!fjob)
> +		return NULL;
> +
> +	fjob->sequence = sequence;
> +
> +	return &fjob->fparam;
> +}
> +
> +static void dip_free_framejob(struct img_frameparam *fparam)
> +{
> +	struct dip_frame_job *fjob = NULL;
> +
> +	fjob = mtk_dip_fparam_to_job(fparam);
> +
> +	/* to avoid use after free issue */
> +	fjob->sequence = -1;
> +
> +	kfree(fjob);
> +}
> +
> +static void dip_enable_ccf_clock(struct dip_device *dip_dev)
> +{
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(dip_dev->larb_dev);
> +	if (ret < 0)
> +		dev_err(&dip_dev->pdev->dev, "cannot get smi larb clock\n");
> +
> +	ret = clk_prepare_enable(dip_dev->dip_clk.DIP_IMG_LARB5);
> +	if (ret)
> +		dev_err(&dip_dev->pdev->dev,
> +			"cannot prepare and enable DIP_IMG_LARB5 clock\n");
> +
> +	ret = clk_prepare_enable(dip_dev->dip_clk.DIP_IMG_DIP);
> +	if (ret)
> +		dev_err(&dip_dev->pdev->dev,
> +			"cannot prepare and enable DIP_IMG_DIP clock\n");
> +}
> +
> +static void dip_disable_ccf_clock(struct dip_device *dip_dev)
> +{
> +	clk_disable_unprepare(dip_dev->dip_clk.DIP_IMG_DIP);
> +	clk_disable_unprepare(dip_dev->dip_clk.DIP_IMG_LARB5);
> +	pm_runtime_put_sync(dip_dev->larb_dev);
> +}
> +
> +static int dip_send(struct platform_device *pdev, enum ipi_id id,
> +		    void *buf, unsigned int  len, unsigned int wait)
> +{
> +	vpu_ipi_send_sync_async(pdev, id, buf, len, wait);
> +	return 0;
> +}
> +
> +static void call_mtk_dip_ctx_finish(struct dip_device *dip_dev,
> +				    struct img_ipi_frameparam *iparam)
> +{
> +	struct mtk_dip_ctx_finish_param fparam;
> +	struct mtk_isp_dip_drv_data *drv_data;
> +	struct mtk_dip_ctx *dev_ctx;
> +	int ctx_id = 0;
> +	int r = 0;
> +
> +	if (!dip_dev) {
> +		pr_err("Can't update buffer status, dip_dev can't be NULL\n");
> +		return;
> +	}
> +
> +	if (!iparam) {
> +		dev_err(&dip_dev->pdev->dev,
> +			"%s: iparam can't be NULL\n", __func__);
> +		return;
> +	}
> +
> +	drv_data = dip_dev_to_drv(dip_dev);
> +
> +	frame_param_ipi_to_ctx(iparam, &fparam);
> +	ctx_id = MTK_DIP_GET_CTX_ID_FROM_SEQUENCE(fparam.frame_id);
> +
> +	if (ctx_id == MTK_DIP_CTX_P2_ID_PREVIEW) {
> +		dev_ctx = &drv_data->isp_preview_dev.ctx;
> +	} else if (ctx_id == MTK_DIP_CTX_P2_ID_CAPTURE) {
> +		dev_ctx = &drv_data->isp_capture_dev.ctx;
> +	} else {
> +		dev_err(&dip_dev->pdev->dev,
> +			"unknown ctx id: %d\n", ctx_id);
> +		return;
> +	}
> +
> +	r = mtk_dip_ctx_core_job_finish(dev_ctx, &fparam);
> +
> +	if (r)
> +		dev_err(&dip_dev->pdev->dev, "finish op failed: %d\n",
> +			r);
> +	dev_dbg(&dip_dev->pdev->dev, "Ready to return buffers: CTX(%d), Frame(%d)\n",
> +		ctx_id, fparam.frame_id);
> +}
> +
> +static void mtk_dip_notify(void *data)
> +{
> +	struct dip_device	*dip_dev;
> +	struct mtk_dip_hw_ctx	*dip_ctx;
> +	struct img_frameparam	*framejob;
> +	struct dip_user_id	*user_id;
> +	struct dip_subframe	*buf, *tmpbuf;
> +	struct img_ipi_frameparam	*frameparam;
> +	u32 num;
> +	bool found = false;
> +
> +	frameparam = (struct img_ipi_frameparam *)data;
> +	dip_ctx = (struct mtk_dip_hw_ctx *)frameparam->drv_data;
> +	dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx);
> +	framejob = container_of(frameparam,
> +				struct img_frameparam,
> +				frameparam);
> +
> +	if (frameparam->state == FRAME_STATE_HW_TIMEOUT) {
> +		dip_send(dip_ctx->vpu_pdev, IPI_DIP_FRAME,
> +			 (void *)frameparam, sizeof(*frameparam), 0);
> +		dev_err(&dip_dev->pdev->dev, "frame no(%d) HW timeout\n",
> +			frameparam->frame_no);
> +	}
> +
> +	mutex_lock(&dip_ctx->dip_usedbufferlist.queuelock);
> +	list_for_each_entry_safe(buf, tmpbuf,
> +				 &dip_ctx->dip_usedbufferlist.queue,
> +				 list_entry) {
> +		if (buf->buffer.pa == frameparam->subfrm_data.pa) {
> +			list_del(&buf->list_entry);
> +			dip_ctx->dip_usedbufferlist.queue_cnt--;
> +			found = true;
> +			dev_dbg(&dip_dev->pdev->dev,
> +				"Find used buffer (%x)\n", buf->buffer.pa);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&dip_ctx->dip_usedbufferlist.queuelock);
> +
> +	if (!found) {
> +		dev_err(&dip_dev->pdev->dev,
> +			"frame_no(%d) buffer(%x) used buffer count(%d)\n",
> +			frameparam->frame_no, frameparam->subfrm_data.pa,
> +			dip_ctx->dip_usedbufferlist.queue_cnt);
> +
> +		frameparam->state = FRAME_STATE_ERROR;
> +
> +	} else {
> +		mutex_lock(&dip_ctx->dip_freebufferlist.queuelock);
> +		list_add_tail(&buf->list_entry,
> +			      &dip_ctx->dip_freebufferlist.queue);
> +		dip_ctx->dip_freebufferlist.queue_cnt++;
> +		mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock);
> +
> +		frameparam->state = FRAME_STATE_DONE;
> +	}
> +
> +	call_mtk_dip_ctx_finish(dip_dev, frameparam);
> +
> +	found = false;
> +	mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> +	list_for_each_entry(user_id,
> +			    &dip_ctx->dip_useridlist.queue,
> +			    list_entry) {
> +		if (DIP_GET_ID(frameparam->index) == user_id->id) {
> +			user_id->num--;
> +			dev_dbg(&dip_dev->pdev->dev,
> +				"user_id(%x) is found, and cnt: %d\n",
> +				user_id->id, user_id->num);
> +			found = true;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> +	wake_up(&dip_ctx->flushing_wq);
> +	dev_dbg(&dip_dev->pdev->dev,
> +		"frame_no(%d) is finished\n", framejob->frameparam.frame_no);
> +	dip_free_framejob(framejob);
> +
> +	num = atomic_dec_return(&dip_ctx->num_running);
> +	dev_dbg(&dip_dev->pdev->dev, "Running count: %d\n", num);
> +}
> +
> +static void mdp_cb_worker(struct work_struct *work)
> +{
> +	struct mtk_mdpcb_work *mdpcb_work;
> +
> +	mdpcb_work = container_of(work, struct mtk_mdpcb_work, frame_work);
> +	mtk_dip_notify(mdpcb_work->frameparams);
> +	kfree(mdpcb_work);
> +}
> +
> +static struct img_ipi_frameparam *convert_to_fparam(struct cmdq_cb_data *data)
> +{
> +	struct device *dev = NULL;

Every use of dev_err() in this function is wrong, since you're
guaranteed to be NULL. Either come up with some better way to report
device errors using the pointers you have, or else just switch to
pr_err().

> +	struct dip_device *dip_dev = NULL;
> +	struct dip_frame_job *fjob = NULL;
> +	struct img_ipi_frameparam *ipi_fparam = NULL;
> +
> +	if (!data) {
> +		dev_err(dev, "DIP got NULL in cmdq_cb_data,%s\n",
> +			__func__);
> +		return NULL;
> +	}
> +
> +	if (data->sta != CMDQ_CB_NORMAL) {
> +		dev_warn(dev, "DIP got CMDQ CB (%d) without CMDQ_CB_NORMAL\n",
> +			 data->sta);
> +	}
> +
> +	if (!data->data) {
> +		dev_err(dev, "DIP got NULL data in cmdq_cb_data,%s\n",
> +			__func__);
> +		return NULL;
> +	}
> +
> +	fjob = mtk_dip_ipi_fparam_to_job(data->data);
> +
> +	if (fjob->sequence == -1) {
> +		dev_err(dev, "Invalid cmdq_cb_data(%llx)\n",
> +			(unsigned long long)data);
> +		ipi_fparam = NULL;
> +	} else {
> +		ipi_fparam = &fjob->fparam.frameparam;
> +		dip_dev = dip_hw_ctx_to_dev((void *)ipi_fparam->drv_data);
> +		dev = &dip_dev->pdev->dev;
> +	}
> +
> +	dev_dbg(dev, "framejob(0x%llx,seq:%d):\n",
> +		(unsigned long long)fjob, fjob->sequence);
> +	dev_dbg(dev, "idx(%d),no(%d),s(%d),n_in(%d),n_out(%d),drv(%llx)\n",
> +		fjob->fparam.frameparam.index,
> +		fjob->fparam.frameparam.frame_no,
> +		fjob->fparam.frameparam.state,
> +		fjob->fparam.frameparam.num_inputs,
> +		fjob->fparam.frameparam.num_outputs,
> +		(unsigned long long)fjob->fparam.frameparam.drv_data
> +	);
> +
> +	return ipi_fparam;
> +}
> +
> +/* Maybe in IRQ context of cmdq */
> +void dip_mdp_cb_func(struct cmdq_cb_data data)

Make this static.

> +{
> +	struct img_ipi_frameparam *frameparam;
> +	struct mtk_dip_hw_ctx *dip_ctx;
> +	struct mtk_mdpcb_work *mdpcb_work;
> +
> +	frameparam = convert_to_fparam(&data);
> +
> +	if (!frameparam) {
> +		dev_err(NULL, "%s return due to invalid cmdq_cb_data(%llx)",

Don't directly pass NULL to dev_err(). Just use pr_err() or similar.

> +			__func__, &data);
> +		return;
> +	}
> +
> +	dip_ctx = (struct mtk_dip_hw_ctx *)frameparam->drv_data;
> +
> +	mdpcb_work = kzalloc(sizeof(*mdpcb_work), GFP_ATOMIC);
> +	WARN_ONCE(!mdpcb_work, "frame_no(%d) is lost", frameparam->frame_no);
> +	if (!mdpcb_work)
> +		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_ctx->mdpcb_workqueue, &mdpcb_work->frame_work);
> +}
> +
> +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_ctx *dip_ctx;
> +	struct dip_device *dip_dev;
> +	unsigned long flags;
> +	u32 num;
> +
> +	WARN_ONCE(!data, "%s is failed due to NULL data\n", __func__);
> +	if (!data)

You can combine these lines:

	
	if (WARN_ONCE(!data, "%s is failed due to NULL data\n", __func__))
		return;

> +		return;
> +
> +	frameparam = (struct img_ipi_frameparam *)data;
> +
> +	framejob = dip_create_framejob(frameparam->index);
> +	WARN_ONCE(!framejob, "frame_no(%d) is lost", frameparam->frame_no);
> +	if (!framejob)

Same here.

> +		return;
> +
> +	dip_ctx = (struct mtk_dip_hw_ctx *)frameparam->drv_data;
> +	dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx);
> +
> +	wake_up(&dip_ctx->composing_wq);
> +	memcpy(&framejob->frameparam, data, sizeof(framejob->frameparam));
> +	num = atomic_dec_return(&dip_ctx->num_composing);
> +
> +	spin_lock_irqsave(&dip_ctx->dip_gcejoblist.queuelock, flags);
> +	list_add_tail(&framejob->list_entry, &dip_ctx->dip_gcejoblist.queue);
> +	dip_ctx->dip_gcejoblist.queue_cnt++;
> +	spin_unlock_irqrestore(&dip_ctx->dip_gcejoblist.queuelock, flags);
> +
> +	dev_dbg(&dip_dev->pdev->dev,
> +		"frame_no(%d) is back, composing num: %d\n",
> +		frameparam->frame_no, num);
> +
> +	wake_up(&dip_ctx->dip_runner_thread.wq);
> +}
> +

...
> +static int dip_runner_func(void *data)
> +{

...

> +
> +			mdp_cmdq_sendtask

I don't see this defined anywhere?

> +				(dip_ctx->mdp_pdev,
> +				 (struct img_config *)
> +					framejob->frameparam.config_data.va,
> +				 &framejob->frameparam, NULL, false,
> +				 dip_mdp_cb_func,
> +				 (void *)&framejob->frameparam);
> +
...
> +	return 0;
> +}
> +
> +static void dip_submit_worker(struct work_struct *work)
> +{
> +	struct mtk_dip_submit_work *dip_submit_work =
> +		container_of(work, struct mtk_dip_submit_work, frame_work);
> +
> +	struct mtk_dip_hw_ctx  *dip_ctx = dip_submit_work->dip_ctx;
> +	struct mtk_dip_work *dip_work;
> +	struct dip_device *dip_dev;
> +	struct dip_subframe *buf;
> +	u32 len, num;
> +	int ret;
> +
> +	dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx);
> +	num  = atomic_read(&dip_ctx->num_composing);
> +
> +	mutex_lock(&dip_ctx->dip_worklist.queuelock);
> +	dip_work = list_first_entry(&dip_ctx->dip_worklist.queue,
> +				    struct mtk_dip_work, list_entry);
> +	mutex_unlock(&dip_ctx->dip_worklist.queuelock);

I see you grab the head of the list here, but then you release the lock.
Then you later assume that reference is still valid, throughout this
function.

That's usually true, because you only remove/delete entries from this
list within this same workqueue (at the end of this function). But it's
not true in dip_release_context() (which doesn't even grab the lock,
BTW).

I think there could be several ways to solve this, but judging by how
this list entry is used...couldn't you just remove it from the list
here, while holding the lock? Then you only have to kfree() it when
you're done under the free_work_list label.

> +
> +	mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> +	if (dip_work->user_id->state == DIP_STATE_STREAMOFF) {
> +		mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> +
> +		dip_work->frameparams.state = FRAME_STATE_STREAMOFF;
> +		call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams);
> +
> +		mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> +		dip_work->user_id->num--;
> +		dev_dbg(&dip_dev->pdev->dev,
> +			"user_id(%x) is streamoff and num: %d, frame_no(%d) index: %x\n",
> +			dip_work->user_id->id, dip_work->user_id->num,
> +			dip_work->frameparams.frame_no,
> +			dip_work->frameparams.index);
> +		mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> +
> +		goto free_work_list;
> +	}
> +	mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> +
> +	while (num >= DIP_COMPOSING_MAX_NUM) {
> +		ret = wait_event_interruptible_timeout
> +			(dip_ctx->composing_wq,
> +			 (num < DIP_COMPOSING_MAX_NUM),
> +			 msecs_to_jiffies(DIP_COMPOSING_WQ_TIMEOUT));
> +
> +		if (ret == -ERESTARTSYS)
> +			dev_err(&dip_dev->pdev->dev,
> +				"interrupted by a signal!\n");
> +		else if (ret == 0)
> +			dev_dbg(&dip_dev->pdev->dev,
> +				"timeout frame_no(%d), num: %d\n",
> +				dip_work->frameparams.frame_no, num);
> +		else
> +			dev_dbg(&dip_dev->pdev->dev,
> +				"wakeup frame_no(%d), num: %d\n",
> +				dip_work->frameparams.frame_no, num);
> +
> +		num = atomic_read(&dip_ctx->num_composing);
> +	};
> +
> +	mutex_lock(&dip_ctx->dip_freebufferlist.queuelock);
> +	if (list_empty(&dip_ctx->dip_freebufferlist.queue)) {
> +		mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock);
> +
> +		dev_err(&dip_dev->pdev->dev,
> +			"frame_no(%d) index: %x no free buffer: %d\n",
> +			dip_work->frameparams.frame_no,
> +			dip_work->frameparams.index,
> +			dip_ctx->dip_freebufferlist.queue_cnt);
> +
> +		/* Call callback to notify V4L2 common framework
> +		 * for failure of enqueue
> +		 */
> +		dip_work->frameparams.state = FRAME_STATE_ERROR;
> +		call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams);
> +
> +		mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> +		dip_work->user_id->num--;
> +		mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> +
> +		goto free_work_list;
> +	}
> +
> +	buf = list_first_entry(&dip_ctx->dip_freebufferlist.queue,
> +			       struct dip_subframe,
> +			       list_entry);
> +	list_del(&buf->list_entry);
> +	dip_ctx->dip_freebufferlist.queue_cnt--;
> +	mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock);
> +
> +	mutex_lock(&dip_ctx->dip_usedbufferlist.queuelock);
> +	list_add_tail(&buf->list_entry, &dip_ctx->dip_usedbufferlist.queue);
> +	dip_ctx->dip_usedbufferlist.queue_cnt++;
> +	mutex_unlock(&dip_ctx->dip_usedbufferlist.queuelock);
> +
> +	memcpy(&dip_work->frameparams.subfrm_data,
> +	       &buf->buffer, sizeof(buf->buffer));
> +
> +	memset((char *)buf->buffer.va, 0, DIP_SUB_FRM_SZ);
> +
> +	memcpy(&dip_work->frameparams.config_data,
> +	       &buf->config_data, sizeof(buf->config_data));
> +
> +	memset((char *)buf->config_data.va, 0, DIP_COMP_SZ);
> +
> +	if (dip_work->frameparams.tuning_data.pa == 0) {
> +		dev_dbg(&dip_dev->pdev->dev,
> +			"frame_no(%d) has no tuning_data\n",
> +			dip_work->frameparams.frame_no);
> +
> +		memcpy(&dip_work->frameparams.tuning_data,
> +		       &buf->tuning_buf, sizeof(buf->tuning_buf));
> +
> +		memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ);
> +		/* 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;
> +	}
> +
> +	dip_work->frameparams.drv_data = (u64)dip_ctx;
> +	dip_work->frameparams.state = FRAME_STATE_COMPOSING;
> +
> +	memcpy((void *)buf->frameparam.va, &dip_work->frameparams,
> +	       sizeof(dip_work->frameparams));
> +
> +	dip_send(dip_ctx->vpu_pdev, IPI_DIP_FRAME,
> +		 (void *)&dip_work->frameparams,
> +		 sizeof(dip_work->frameparams), 0);
> +	num = atomic_inc_return(&dip_ctx->num_composing);
> +
> +free_work_list:
> +
> +	mutex_lock(&dip_ctx->dip_worklist.queuelock);
> +	list_del(&dip_work->list_entry);
> +	dip_ctx->dip_worklist.queue_cnt--;
> +	len = dip_ctx->dip_worklist.queue_cnt;
> +	mutex_unlock(&dip_ctx->dip_worklist.queuelock);
> +
> +	dev_dbg(&dip_dev->pdev->dev,
> +		"frame_no(%d) index: %x, worklist count: %d, composing num: %d\n",
> +		dip_work->frameparams.frame_no, dip_work->frameparams.index,
> +		len, num);
> +
> +	kfree(dip_work);
> +}

...

> +int dip_open_context(struct dip_device *dip_dev)

Should be static.

> +{

...

> +}
> +
> +int dip_release_context(struct dip_device *dip_dev)

Should be static.

> +{
> +	u32 i = 0;
> +	struct dip_subframe *buf, *tmpbuf;
> +	struct mtk_dip_work *dip_work, *tmp_work;
> +	struct dip_user_id  *dip_userid, *tmp_id;
> +	struct mtk_dip_hw_ctx *dip_ctx;
> +
> +	dip_ctx = &dip_dev->dip_ctx;
> +	dev_dbg(&dip_dev->pdev->dev, "composer work queue = %d\n",
> +		dip_ctx->dip_worklist.queue_cnt);
> +
> +	list_for_each_entry_safe(dip_work, tmp_work,
> +				 &dip_ctx->dip_worklist.queue,
> +				 list_entry) {

Shouldn't you be holding the mutex for this? Or alternatively, cancel
any outstanding work and move the flush_workqueue()/destroy_workqueue()
up.

Similar questions for the other lists we're going through here.

> +		list_del(&dip_work->list_entry);
> +		dev_dbg(&dip_dev->pdev->dev, "dip work frame no: %d\n",
> +			dip_work->frameparams.frame_no);
> +		kfree(dip_work);
> +		dip_ctx->dip_worklist.queue_cnt--;
> +	}
> +
> +	if (dip_ctx->dip_worklist.queue_cnt != 0)
> +		dev_dbg(&dip_dev->pdev->dev,
> +			"dip_worklist is not empty (%d)\n",
> +			dip_ctx->dip_worklist.queue_cnt);
> +
> +	list_for_each_entry_safe(dip_userid, tmp_id,
> +				 &dip_ctx->dip_useridlist.queue,
> +				 list_entry) {
> +		list_del(&dip_userid->list_entry);
> +		dev_dbg(&dip_dev->pdev->dev, "dip user id: %x\n",
> +			dip_userid->id);
> +		kfree(dip_userid);
> +		dip_ctx->dip_useridlist.queue_cnt--;
> +	}
> +
> +	if (dip_ctx->dip_useridlist.queue_cnt != 0)
> +		dev_dbg(&dip_dev->pdev->dev,
> +			"dip_useridlist is not empty (%d)\n",
> +			dip_ctx->dip_useridlist.queue_cnt);
> +
> +	flush_workqueue(dip_ctx->mdpcb_workqueue);
> +	destroy_workqueue(dip_ctx->mdpcb_workqueue);
> +	dip_ctx->mdpcb_workqueue = NULL;
> +
> +	flush_workqueue(dip_ctx->composer_wq);
> +	destroy_workqueue(dip_ctx->composer_wq);
> +	dip_ctx->composer_wq = NULL;
> +
> +	atomic_set(&dip_ctx->num_composing, 0);
> +	atomic_set(&dip_ctx->num_running, 0);
> +
> +	kthread_stop(dip_ctx->dip_runner_thread.thread);
> +	dip_ctx->dip_runner_thread.thread = NULL;
> +
> +	atomic_set(&dip_ctx->dip_user_cnt, 0);
> +	atomic_set(&dip_ctx->dip_stream_cnt, 0);
> +	atomic_set(&dip_ctx->dip_enque_cnt, 0);
> +
> +	/* All the buffer should be in the freebufferlist when release */
> +	list_for_each_entry_safe(buf, tmpbuf,
> +				 &dip_ctx->dip_freebufferlist.queue,
> +				 list_entry) {
> +		struct sg_table *sgt = &buf->table;
> +
> +		dev_dbg(&dip_dev->pdev->dev,
> +			"buf pa (%d): %x\n", i, buf->buffer.pa);
> +		dip_ctx->dip_freebufferlist.queue_cnt--;
> +		dma_unmap_sg_attrs(&dip_dev->pdev->dev, sgt->sgl,
> +				   sgt->orig_nents,
> +				   DMA_BIDIRECTIONAL, DMA_ATTR_SKIP_CPU_SYNC);
> +		sg_free_table(sgt);
> +		list_del(&buf->list_entry);
> +		kfree(buf);
> +		buf = NULL;
> +		i++;
> +	}
> +
> +	if (dip_ctx->dip_freebufferlist.queue_cnt != 0 &&
> +	    i != DIP_SUB_FRM_DATA_NUM)
> +		dev_err(&dip_dev->pdev->dev,
> +			"dip_freebufferlist is not empty (%d/%d)\n",
> +			dip_ctx->dip_freebufferlist.queue_cnt, i);
> +
> +	mutex_destroy(&dip_ctx->dip_useridlist.queuelock);
> +	mutex_destroy(&dip_ctx->dip_worklist.queuelock);
> +	mutex_destroy(&dip_ctx->dip_usedbufferlist.queuelock);
> +	mutex_destroy(&dip_ctx->dip_freebufferlist.queuelock);
> +
> +	return 0;
> +}
> +

...

> +static int mtk_dip_probe(struct platform_device *pdev)
> +{
> +	struct mtk_isp_dip_drv_data *dip_drv;
> +	struct dip_device *dip_dev;
> +	struct mtk_dip_hw_ctx *dip_ctx;
> +	struct device_node *node;
> +	struct platform_device *larb_pdev;
> +
> +	int ret = 0;
> +
> +	dev_info(&pdev->dev, "E. DIP driver probe.\n");
> +
> +	dip_drv = devm_kzalloc(&pdev->dev, sizeof(*dip_drv), GFP_KERNEL);

Need to check for NULL.

> +	dev_set_drvdata(&pdev->dev, dip_drv);
> +	dip_dev = &dip_drv->dip_dev;
> +
> +	if (!dip_dev)
> +		return -ENOMEM;
> +
> +	dev_info(&pdev->dev, "Created dip_dev = 0x%p\n", dip_dev);
> +
> +	dip_dev->pdev = pdev;
> +	dip_ctx = &dip_dev->dip_ctx;
> +
> +	node = of_parse_phandle(pdev->dev.of_node, "mediatek,larb", 0);
> +	if (!node) {
> +		dev_err(&pdev->dev, "no mediatek,larb found");
> +		return -EINVAL;
> +	}
> +	larb_pdev = of_find_device_by_node(node);
> +	if (!larb_pdev) {
> +		dev_err(&pdev->dev, "no mediatek,larb device found");
> +		return -EINVAL;
> +	}
> +	dip_dev->larb_dev = &larb_pdev->dev;
> +
> +	/*CCF: Grab clock pointer (struct clk*) */

Add a space before 'CCF'.

> +	dip_dev->dip_clk.DIP_IMG_LARB5 = devm_clk_get(&pdev->dev,
> +						      "DIP_CG_IMG_LARB5");
> +	dip_dev->dip_clk.DIP_IMG_DIP = devm_clk_get(&pdev->dev,
> +						    "DIP_CG_IMG_DIP");
> +	if (IS_ERR(dip_dev->dip_clk.DIP_IMG_LARB5)) {
> +		dev_err(&pdev->dev, "cannot get DIP_IMG_LARB5 clock\n");
> +		return PTR_ERR(dip_dev->dip_clk.DIP_IMG_LARB5);
> +	}
> +	if (IS_ERR(dip_dev->dip_clk.DIP_IMG_DIP)) {
> +		dev_err(&pdev->dev, "cannot get DIP_IMG_DIP clock\n");
> +		return PTR_ERR(dip_dev->dip_clk.DIP_IMG_DIP);
> +	}
> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	atomic_set(&dip_ctx->dip_user_cnt, 0);
> +	atomic_set(&dip_ctx->dip_stream_cnt, 0);
> +	atomic_set(&dip_ctx->dip_enque_cnt, 0);
> +
> +	atomic_set(&dip_ctx->num_composing, 0);
> +	atomic_set(&dip_ctx->num_running, 0);
> +
> +	dip_ctx->dip_worklist.queue_cnt = 0;
> +
> +	ret = mtk_dip_ctx_dip_v4l2_init(pdev,
> +					&dip_drv->isp_preview_dev,
> +		&dip_drv->isp_capture_dev);
> +
> +	if (ret)
> +		dev_err(&pdev->dev, "v4l2 init failed: %d\n", ret);
> +
> +	dev_info(&pdev->dev, "X. DIP driver probe.\n");
> +
> +	return ret;
> +}
> +
> +static int mtk_dip_remove(struct platform_device *pdev)
> +{
> +	struct mtk_isp_dip_drv_data *drv_data =
> +		dev_get_drvdata(&pdev->dev);
> +
> +	/*  */

What's with the empty comments? Here and below.

> +	if (drv_data) {
> +		mtk_dip_dev_core_release(pdev, &drv_data->isp_preview_dev);
> +		mtk_dip_dev_core_release(pdev, &drv_data->isp_capture_dev);
> +		dev_info(&pdev->dev, "E. %s\n", __func__);

Remove this line.

> +	}
> +
> +	pm_runtime_disable(&pdev->dev);
> +
> +	/*  */
> +	return 0;
> +}
> +

...

> +static struct platform_driver mtk_dip_driver = {
> +	.probe   = mtk_dip_probe,
> +	.remove  = mtk_dip_remove,
> +	.driver  = {
> +		.name  = DIP_DEV_NAME,
> +		.owner = THIS_MODULE,

You don't need the .owner line. module_platform_driver() /
platform_driver_register() takes care of this for you.

Brian

> +		.of_match_table = dip_of_ids,
> +		.pm     = &mtk_dip_pm_ops,
> +	}
> +};
> +
> +module_platform_driver(mtk_dip_driver);
> +
> +MODULE_DESCRIPTION("Camera DIP driver");
> +MODULE_LICENSE("GPL");
...