Hi Tomasz,
On Fri, 2019-08-02 at 16:28 +0800, Tomasz Figa wrote:
> Hi Jerry,
>
> On Tue, Jul 09, 2019 at 04:41:12PM +0800, Jerry-ch Chen wrote:
> > From: Jerry-ch Chen <jerry-ch.chen@xxxxxxxxxxxx>
> >
> > This patch adds the driver of Face Detection (FD) unit in
> > Mediatek camera system, providing face detection function.
> >
> > 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: Jerry-ch Chen <jerry-ch.chen@xxxxxxxxxxxx>
> > ---
> > drivers/media/platform/Makefile | 2 +
> > drivers/media/platform/mtk-isp/fd/Makefile | 5 +
> > drivers/media/platform/mtk-isp/fd/mtk_fd.h | 157 +++
> > drivers/media/platform/mtk-isp/fd/mtk_fd_40.c | 1259 +++++++++++++++++++++++++
> > include/uapi/linux/v4l2-controls.h | 4 +
> > 5 files changed, 1427 insertions(+)
> > create mode 100644 drivers/media/platform/mtk-isp/fd/Makefile
> > create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd.h
> > create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd_40.c
> >
>
> Thanks for the patch! I finally got a chance to fully review the code. Sorry
> for the delay. Please check my comments inline.
>
I appreciate your comments.
I've fixed most of the comments and verifying them,
Sorry for the delay, here is the reply.
>
> > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> > index e6deb25..8b817cc 100644
> > --- a/drivers/media/platform/Makefile
> > +++ b/drivers/media/platform/Makefile
> > @@ -94,6 +94,8 @@ obj-$(CONFIG_VIDEO_MEDIATEK_MDP) += mtk-mdp/
> >
> > obj-$(CONFIG_VIDEO_MEDIATEK_JPEG) += mtk-jpeg/
> >
> > +obj-$(CONFIG_VIDEO_MEDIATEK_FD) += mtk-isp/fd/
> > +
> > obj-$(CONFIG_VIDEO_QCOM_CAMSS) += qcom/camss/
> >
> > obj-$(CONFIG_VIDEO_QCOM_VENUS) += qcom/venus/
> > diff --git a/drivers/media/platform/mtk-isp/fd/Makefile b/drivers/media/platform/mtk-isp/fd/Makefile
> > new file mode 100644
> > index 0000000..9b1c501
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/fd/Makefile
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +mtk-fd-objs += mtk_fd_40.o
> > +
> > +obj-$(CONFIG_VIDEO_MEDIATEK_FD) += mtk-fd.o
> > \ No newline at end of file
> > diff --git a/drivers/media/platform/mtk-isp/fd/mtk_fd.h b/drivers/media/platform/mtk-isp/fd/mtk_fd.h
> > new file mode 100644
> > index 0000000..289999b
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/fd/mtk_fd.h
> > @@ -0,0 +1,157 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +//
> > +// Copyright (c) 2018 MediaTek Inc.
> > +
> > +#ifndef __MTK_FD_HW_H__
> > +#define __MTK_FD_HW_H__
> > +
> > +#include <linux/io.h>
> > +#include <linux/types.h>
> > +#include <linux/platform_device.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/videobuf2-v4l2.h>
> > +
> > +#define MTK_FD_OUTPUT_MIN_WIDTH 26U
> > +#define MTK_FD_OUTPUT_MIN_HEIGHT 26U
> > +#define MTK_FD_OUTPUT_MAX_WIDTH 640U
> > +#define MTK_FD_OUTPUT_MAX_HEIGHT 480U
> > +
> > +/* Control the user defined image widths and heights
> > + * to be scaled and performed face detection in FD HW.
> > + * MTK FD support up to 14 user defined image sizes to perform face detection.
> > + */
> > +#define V4L2_CID_MTK_FD_SCALE_IMG_WIDTH (V4L2_CID_USER_MTK_FD_BASE + 1)
> > +#define V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT (V4L2_CID_USER_MTK_FD_BASE + 2)
> > +
> > +/* Control the numbers of user defined image sizes.
> > + * The default value is 0 which means user is not going
> > + * to define the specific image sizes.
> > + */
> > +#define V4L2_CID_MTK_FD_SCALE_IMG_NUM (V4L2_CID_USER_MTK_FD_BASE + 3)
> > +
> > +/* Control the Face Pose to be detected.
> > + * Here describe the value as following:
> > + * {0, detect the front face with rotation from 0 to 270 degrees},
> > + * {1, detect the front face with rotation from 0 to 240 and 300 degrees},
> > + * {2, detect the front face with rotation from 0 to 240 and 330 degrees},
> > + * {3, detect the front face with rotation from 0 to 240 and left side face}.
> > + */
> > +#define V4L2_CID_MTK_FD_DETECT_POSE (V4L2_CID_USER_MTK_FD_BASE + 4)
> > +#define V4L2_CID_MTK_FD_DETECT_SPEEDUP (V4L2_CID_USER_MTK_FD_BASE + 5)
> > +#define V4L2_CID_MTK_FD_EXTRA_MODEL (V4L2_CID_USER_MTK_FD_BASE + 6)
> > +
> > +/* We reserve 16 controls for this driver. */
> > +#define V4L2_CID_MTK_FD_MAX 16
> > +
> > +#define ENABLE_FD 0x111
> > +#define FD_HW_ENABLE 0x4
> > +#define FD_INT_EN 0x15c
> > +#define FD_INT 0x168
> > +#define FD_RESULT 0x178
> > +#define FD_IRQ_MASK 0x001
> > +
> > +#define RS_MAX_BUF_SIZE 2288788
> > +#define FD_MAX_SPEEDUP 7
> > +#define FD_MAX_POSE_VAL 0xfffffffffffffff
> > +#define FD_DEF_POSE_VAL 0x3ff
> > +#define MAX_FD_SEL_NUM 1026
> > +
> > +/* The max. number of face sizes could be detected, for feature scaling */
> > +#define FACE_SIZE_NUM_MAX 14
> > +/* FACE_SIZE_NUM_MAX + 1, first scale for input image W/H */
> > +#define FD_SCALE_NUM 15
> > +
> > +enum fd_state {
> > + FD_ENQ,
> > + FD_CBD,
> > +};
> > +
> > +enum fd_img_format {
> > + FMT_VYUY = 2,
> > + FMT_UYVY,
> > + FMT_YVYU,
> > + FMT_YUYV,
> > + FMT_UNKNOWN
> > +};
>
> Please use #define macros for hardware/firmware interface definitions.
>
Ok, I will refine as following
#define MTK_FD_HW_FMT_VYUY 2
#define MTK_FD_HW_FMT_UYVY 3
#define MTK_FD_HW_FMT_YVYU 4
#define MTK_FD_HW_FMT_YUYV 5
#define MTK_FD_HW_FMT_UNKNOWN 6
> > +
> > +struct fd_buffer {
> > + __u32 scp_addr; /* used by SCP */
> > + __u32 dma_addr; /* used by DMA HW */
> > +} __packed;
> > +
> > +enum fd_scp_cmd {
> > + FD_CMD_INIT,
> > + FD_CMD_ENQUEUE,
> > +};
>
> Ditto.
Ok, I will refine as following
#define MTK_FD_IPI_CMD_INIT 0
#define MTK_FD_IPI_CMD_ENQUEUE 1
>
> > +
> > +struct fd_user_output {
> > + __u64 results[MAX_FD_SEL_NUM];
> > + __u16 number;
> > +};
> > +
> > +struct user_param {
> > + u8 fd_pose;
> > + u8 fd_speedup;
> > + u8 fd_extra_model;
> > + u8 scale_img_num;
> > + u8 src_img_fmt;
> > + __u16 scale_img_width[FD_SCALE_NUM];
> > + __u16 scale_img_height[FD_SCALE_NUM];
> > +} __packed;
> > +
> > +struct fd_hw_param {
> > + struct fd_buffer src_img;
> > + struct fd_buffer user_result;
> > + struct user_param user_param;
> > +} __packed;
> > +
> > +struct cmd_init_info {
> > + struct fd_buffer fd_manager;
> > + __u32 rs_dma_addr;
> > +} __packed;
> > +
> > +struct ipi_message {
> > + u8 cmd_id;
> > + union {
> > + struct cmd_init_info init_param;
> > + struct fd_hw_param hw_param;
> > + };
> > +} __packed;
> > +
> > +struct mtk_fd_hw {
> > + struct clk *fd_clk;
> > + struct rproc *rproc_handle;
> > + struct platform_device *scp_pdev;
> > + struct fd_buffer scp_mem;
> > + wait_queue_head_t wq;
> > + void __iomem *fd_base;
> > + atomic_t fd_user_cnt;
> > + enum fd_state state;
> > + u32 fd_irq_result;
> > + /* Ensure only one job in hw */
> > + struct mutex fd_hw_lock;
> > +};
> > +
> > +struct mtk_fd_dev {
> > + struct platform_device *pdev;
> > + struct v4l2_device v4l2_dev;
> > + struct v4l2_m2m_dev *m2m_dev;
> > + struct media_device mdev;
> > + struct video_device vfd;
> > + struct mtk_fd_hw fd_hw;
>
> Could we just put the contents of that struct directly here? That should
> simplify dereference chains in the code.
>
Ok, I will fix it.
> > + /* Lock for V4L2 operations */
> > + struct mutex vfd_lock;
> > +};
> > +
> > +struct mtk_fd_ctx {
> > + struct mtk_fd_dev *fd_dev;
> > + struct device *dev;
> > + struct v4l2_fh fh;
> > + struct v4l2_ctrl_handler hdl;
> > + struct v4l2_pix_format_mplane src_fmt;
> > + struct v4l2_meta_format dst_fmt;
> > + struct user_param user_param;
> > +};
> > +
> > +#endif/*__MTK_FD_HW_H__*/
> > diff --git a/drivers/media/platform/mtk-isp/fd/mtk_fd_40.c b/drivers/media/platform/mtk-isp/fd/mtk_fd_40.c
> > new file mode 100644
> > index 0000000..246d3aa
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/fd/mtk_fd_40.c
> > @@ -0,0 +1,1259 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (c) 2018 MediaTek Inc.
> > +
> > +#include <linux/clk.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_data/mtk_scp.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/remoteproc.h>
> > +#include <media/videobuf2-dma-contig.h>
> > +#include <media/videobuf2-v4l2.h>
> > +#include <linux/wait.h>
> > +#include <media/v4l2-ioctl.h>
> > +#include <media/v4l2-event.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-mem2mem.h>
> > +#include <media/videobuf2-core.h>
> > +
> > +#include "mtk_fd.h"
> > +
> > +static struct v4l2_meta_format fw_param_fmts[] = {
>
> const?
>
> Also, isn't this the buffer with the detected faces rather than params?
This struct will be removed. Yes, it's the buffer with detected faces.
>
> > + {
> > + .dataformat = V4L2_META_FMT_MTISP_PARAMS,
>
> This should use its own format.
Ok, I will define V4L2_META_FMT_MTFD_RESULT in videodev2.h
>
> > + .buffersize = 1024 * 30,
>
> Please define a C struct for this buffer and use sizeof() here.
>
Ok, that will be sizeof(struct fd_user_output);
> > + },
> > +};
>
> Actually, is there a reason to have this array at all if there is only 1
> format supported? I think we could just put the right constants directly in
> the code.
>
I agree, I'll remove this array and use the constants in the code.
> > +
> > +static const struct v4l2_pix_format_mplane in_img_fmts[] = {
> > + {
> > + .width = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .height = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_VYUY,
> > + .colorspace = V4L2_COLORSPACE_BT2020,
> > + .field = V4L2_FIELD_NONE,
> > + .num_planes = 1,
> > + },
> > + {
> > + .width = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .height = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_YUYV,
> > + .colorspace = V4L2_COLORSPACE_BT2020,
> > + .field = V4L2_FIELD_NONE,
> > + .num_planes = 1,
> > + },
> > + {
> > + .width = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .height = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_YVYU,
> > + .colorspace = V4L2_COLORSPACE_BT2020,
> > + .field = V4L2_FIELD_NONE,
> > + .num_planes = 1,
> > + },
> > + {
> > + .width = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .height = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_UYVY,
> > + .colorspace = V4L2_COLORSPACE_BT2020,
> > + .field = V4L2_FIELD_NONE,
> > + .num_planes = 1,
>
> If all the formats have the same values for a field, there is no reason to
> have the field initialized here. Just make initialize them to the constant
> values inside TRY_FMT.
>
Ok, I will do so.
> Hmm, are the interleaved YUV formats the only formats supported by this
> hardware? That would be quite unfortunate given the formats supported by the
> other IP blocks on this SoC use the more typical planar formats.
>
The supported YUV 1 plane formats are listed above.
The rest supported format is YUV 2 plane format, which should be
V4L2_PIX_FMT_NV16M.
For the in_img_fmts[], I will refine as following:
static const struct v4l2_pix_format_mplane mtk_fd_img_fmts[] = {
{
.pixelformat = V4L2_PIX_FMT_VYUY,
.num_planes = 1,
},
{
.pixelformat = V4L2_PIX_FMT_YUYV,
.num_planes = 1,
},
{
.pixelformat = V4L2_PIX_FMT_YVYU,
.num_planes = 1,
},
{
.pixelformat = V4L2_PIX_FMT_UYVY,
.num_planes = 1,
},
{
.pixelformat = V4L2_PIX_FMT_NV16M,
.num_planes = 2,
},
{
.pixelformat = V4L2_PIX_FMT_NV61M,
.num_planes = 2,
},
};
> > + },
> > +};
> > +
> > +#define NUM_FORMATS ARRAY_SIZE(in_img_fmts)
> > +
> > +static inline struct mtk_fd_dev *mtk_fd_hw_to_dev(struct mtk_fd_hw *fd_hw)
> > +{
> > + return container_of(fd_hw, struct mtk_fd_dev, fd_hw);
> > +}
> > +
> > +static inline struct mtk_fd_ctx *fh_to_ctx(struct v4l2_fh *fh)
> > +{
> > + return container_of(fh, struct mtk_fd_ctx, fh);
> > +}
> > +
> > +static int mtk_fd_load_scp(struct mtk_fd_hw *fd_hw)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + struct device *dev = &fd_dev->pdev->dev;
> > + phandle rproc_phandle;
> > + int ret;
> > +
> > + /* init scp */
> > + fd_hw->scp_pdev = scp_get_pdev(fd_dev->pdev);
> > + if (!fd_hw->scp_pdev) {
> > + dev_err(dev, "Failed to get scp device\n");
> > + return -ENODEV;
> > + }
> > +
> > + if (of_property_read_u32(fd_dev->pdev->dev.of_node, "mediatek,scp",
> > + &rproc_phandle)) {
> > + dev_err(dev, "Could not get scp device\n");
> > + return -EINVAL;
> > + }
> > +
> > + fd_hw->rproc_handle = rproc_get_by_phandle(rproc_phandle);
> > + if (!fd_hw->rproc_handle) {
> > + dev_err(dev, "Could not get FD's rproc_handle\n");
> > + return -EINVAL;
> > + }
> > +
> > + ret = rproc_boot(fd_hw->rproc_handle);
> > + if (ret < 0) {
> > + /**
> > + * Return 0 if downloading firmware successfully,
> > + * otherwise it is failed
> > + */
> > + dev_err(dev, "rproc_boot failed\n");
> > + return -ENODEV;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static dma_addr_t mtk_fd_hw_alloc_rs_dma_addr(struct mtk_fd_hw *fd_hw)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + struct device *dev = &fd_dev->pdev->dev;
> > + void *va;
> > + dma_addr_t dma_handle;
> > +
> > + va = dma_alloc_coherent(dev, RS_MAX_BUF_SIZE, &dma_handle, GFP_KERNEL);
>
> I see this allocated here, but I don't see this freed anywhere.
>
I will fix it in next version.
> > + if (!va) {
> > + dev_err(dev, "dma_alloc null va\n");
> > + return -ENOMEM;
> > + }
> > + memset(va, 0, RS_MAX_BUF_SIZE);
> > +
> > + return dma_handle;
> > +}
> > +
> > +static int mtk_fd_send_ipi_init(struct mtk_fd_hw *fd_hw)
> > +{
> > + struct ipi_message fd_init_msg;
> > + dma_addr_t rs_dma_addr;
> > +
> > + fd_init_msg.cmd_id = FD_CMD_INIT;
> > +
> > + fd_init_msg.init_param.fd_manager.scp_addr = fd_hw->scp_mem.scp_addr;
> > + fd_init_msg.init_param.fd_manager.dma_addr = fd_hw->scp_mem.dma_addr;
> > +
> > + rs_dma_addr = mtk_fd_hw_alloc_rs_dma_addr(fd_hw);
> > + if (!rs_dma_addr)
> > + return -ENOMEM;
> > +
> > + fd_init_msg.init_param.rs_dma_addr = rs_dma_addr;
> > +
> > + return scp_ipi_send(fd_hw->scp_pdev, SCP_IPI_FD_CMD, &fd_init_msg,
> > + sizeof(fd_init_msg), 0);
> > +}
> > +
> > +static int mtk_fd_hw_enable(struct mtk_fd_hw *fd_hw)
> > +{
> > + int ret;
> > +
> > + ret = mtk_fd_load_scp(fd_hw);
> > + if (ret)
> > + return ret;
> > +
> > + ret = mtk_fd_send_ipi_init(fd_hw);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_hw_connect(struct mtk_fd_hw *fd_hw)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + s32 usercount;
> > +
> > + mutex_lock(&fd_hw->fd_hw_lock);
>
> First of all, we don't need a mutex here, because all the V4L2 ioctls are
> already running with the video device mutex.
Ok, I will remove the using of the mutex here.
>
> > + usercount = atomic_inc_return(&fd_hw->fd_user_cnt);
>
> If we already use a mutex, there is no point in using atomic types.
>
Ok, I will replace the atomic types with normal types
> > + if (usercount == 1) {
> > + pm_runtime_get_sync(&fd_dev->pdev->dev);
> > + if (mtk_fd_hw_enable(fd_hw)) {
> > + pm_runtime_put_sync(&fd_dev->pdev->dev);
> > + atomic_dec_return(&fd_hw->fd_user_cnt);
> > + mutex_unlock(&fd_hw->fd_hw_lock);
> > + return -EINVAL;
> > + }
> > + }
>
> This is a simple mem-to-mem device, so there is no reason to keep it active
> all the time it's streaming. Please just get the runtime PM counter when
> queuing a job to the hardware and release it when the job finishes.
>
> I guess we might still want to do the costly operations like rproc_boot()
> when we start streaming, though.
>
Do you mean by moving the pm_runtime_get/put stuff to the job execution
and job finish place?
the rproc_boot() operation will be done here.
> > + mutex_unlock(&fd_hw->fd_hw_lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_wait_irq(struct mtk_fd_hw *fd_hw)
> > +{
> > + int timeout;
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + struct device *dev = &fd_dev->pdev->dev;
> > +
> > + timeout = wait_event_interruptible_timeout
> > + (fd_hw->wq, (fd_hw->fd_irq_result & FD_IRQ_MASK),
> > + usecs_to_jiffies(1000000));
> > + if (!timeout) {
> > + dev_err(dev, "%s timeout, %d\n", __func__,
> > + fd_hw->fd_irq_result);
> > + return -EAGAIN;
> > + } else if (!(fd_hw->fd_irq_result & FD_IRQ_MASK)) {
> > + dev_err(dev, "%s No IRQ mask:0x%8x\n",
> > + __func__, fd_hw->fd_irq_result);
> > + return -EINVAL;
> > + }
> > + fd_hw->fd_irq_result = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static void mtk_fd_hw_disconnect(struct mtk_fd_hw *fd_hw)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + s32 usercount;
> > +
> > + mutex_lock(&fd_hw->fd_hw_lock);
> > + atomic_dec_return(&fd_hw->fd_user_cnt);
> > + usercount = atomic_read(&fd_hw->fd_user_cnt);
> > + if (usercount == 0) {
> > + if (fd_hw->state == FD_ENQ)
> > + mtk_fd_wait_irq(fd_hw);
>
> This shouldn't be possible to happen as the queues should be already stopped
> at this point.
>
Ok, I will remove it.
> > +
> > + pm_runtime_put_sync(&fd_dev->pdev->dev);
>
> Any reason to use pm_runtime_put_sync() over pm_runtime_put()?
>
No special reason to do so, the pm_runtime_put_sync here will be moved
to the place each job finished.
> > + rproc_shutdown(fd_hw->rproc_handle);
> > + rproc_put(fd_hw->rproc_handle);
> > + }
> > + mutex_unlock(&fd_hw->fd_hw_lock);
> > +}
> > +
> > +static void mtk_fd_hw_job_finish(struct mtk_fd_hw *fd_hw,
> > + struct fd_hw_param *fd_param,
> > + enum vb2_buffer_state vb_state)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + struct mtk_fd_ctx *ctx;
> > + struct device *dev = &fd_dev->pdev->dev;
> > + struct vb2_buffer *src_vb, *dst_vb;
> > + struct vb2_v4l2_buffer *src_vbuf = NULL, *dst_vbuf = NULL;
> > +
> > + ctx = v4l2_m2m_get_curr_priv(fd_dev->m2m_dev);
> > + if (!ctx) {
> > + dev_err(dev, "Instance released before end of transaction\n");
> > + return;
> > + }
>
> This shouldn't be possible. I suspect you're seeing this because
> .stop_streaming is not implemented correctly. See my comment there.
>
Ok, I will remove this.
> > +
> > + src_vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > + if (WARN_ON(!src_vb))
> > + return;
>
> This shouldn't be possible.
>
Ditto.
> > + src_vbuf = to_vb2_v4l2_buffer(src_vb);
> > +
> > + dst_vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > + if (WARN_ON(!dst_vb))
> > + return;
>
> Ditto.
>
Ditto.
> > + dst_vbuf = to_vb2_v4l2_buffer(dst_vb);
> > +
> > + dst_vbuf->vb2_buf.timestamp = src_vbuf->vb2_buf.timestamp;
> > + dst_vbuf->timecode = src_vbuf->timecode;
> > + dst_vbuf->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> > + dst_vbuf->flags |= src_vbuf->flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
>
> We should be able to just use v4l2_m2m_buf_copy_metadata().
>
Ok, I will refine as:
v4l2_m2m_buf_copy_metadata(src_vbuf, dst_vbuf,
V4L2_BUF_FLAG_TSTAMP_SRC_MASK);
> > +
> > + v4l2_m2m_buf_done(src_vbuf, vb_state);
> > + v4l2_m2m_buf_done(dst_vbuf, vb_state);
> > + v4l2_m2m_job_finish(fd_dev->m2m_dev, ctx->fh.m2m_ctx);
> > +}
> > +
> > +static int mtk_fd_hw_job_exec(struct mtk_fd_hw *fd_hw,
> > + struct fd_hw_param *fd_param,
> > + void *output_vaddr)
> > +{
> > + struct fd_user_output *fd_output;
> > + struct ipi_message fd_ipi_msg;
> > + int ret;
> > + u32 num;
> > +
> > + if (fd_param->user_param.src_img_fmt == FMT_UNKNOWN)
> > + goto param_err;
>
> Is this possible?
>
Only if user set wrong format, I will remove this.
> > +
> > + mutex_lock(&fd_hw->fd_hw_lock);
> > + fd_hw->state = FD_ENQ;
>
> What is this state for?
>
It was for checking status, if a job is processing, the state is
FD_ENQ,
then we should wait for the last job to be done when pm_suspend().
> > + fd_ipi_msg.cmd_id = FD_CMD_ENQUEUE;
> > + memcpy(&fd_ipi_msg.hw_param, fd_param, sizeof(fd_ipi_msg.hw_param));
> > + ret = scp_ipi_send(fd_hw->scp_pdev, SCP_IPI_FD_CMD, &fd_ipi_msg,
> > + sizeof(fd_ipi_msg), 0);
> > + if (ret)
> > + goto buf_err;
> > +
> > + ret = mtk_fd_wait_irq(fd_hw);
> > + if (ret)
> > + goto buf_err;
>
> This function is called from device_run and it shouldn't be synchronous. It
> should only queue the job to the hardware to be handled asynchronously when
> it finishes.
>
Ok, I will fix it.
> > +
> > + num = readl(fd_hw->fd_base + FD_RESULT);
> > + /* Disable FD ISR */
> > + writel(0x0, fd_hw->fd_base + FD_INT_EN);
> > +
> > + fd_output = (struct fd_user_output *)output_vaddr;
> > + fd_output->number = num;
> > + fd_hw->state = FD_CBD;
> > + mutex_unlock(&fd_hw->fd_hw_lock);
> > +
> > + mtk_fd_hw_job_finish(fd_hw, fd_param, VB2_BUF_STATE_DONE);
> > + return 0;
> > +
> > +buf_err:
> > + mutex_unlock(&fd_hw->fd_hw_lock);
> > +param_err:
> > + mtk_fd_hw_job_finish(fd_hw, fd_param, VB2_BUF_STATE_ERROR);
> > + return ret;
> > +}
> > +
> > +static int mtk_fd_vb2_buf_out_validate(struct vb2_buffer *vb)
> > +{
> > + struct vb2_v4l2_buffer *v4l2_buf = to_vb2_v4l2_buffer(vb);
> > +
> > + v4l2_buf->field = V4L2_FIELD_NONE;
>
> We need to fail with -EINVAL if v4l2_buf->field was different than
> V4L2_FIELD_ANY or _NONE.
>
Ok, I will refine as following:
if (v4l2_buf->field == V4L2_FIELD_ANY)
v4l2_buf->field = V4L2_FIELD_NONE;
if (v4l2_buf->field != V4L2_FIELD_NONE)
return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_vb2_buf_prepare(struct vb2_buffer *vb)
> > +{
> > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > + struct vb2_queue *vq = vb->vb2_queue;
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > + struct device *dev = ctx->dev;
> > + struct v4l2_pix_format_mplane *pixfmt;
> > +
> > + switch (vq->type) {
> > + case V4L2_BUF_TYPE_META_CAPTURE:
> > + if (vb2_plane_size(vb, 0) < ctx->dst_fmt.buffersize) {
> > + dev_err(dev, "meta size %d is too small\n");
>
> Please make this dev_dbg(), because userspace misbehavior must not trigger
> kernel error logs.
>
Ok, fixed.
> > + return -EINVAL;
> > + }
> > + break;
> > + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > + pixfmt = &ctx->src_fmt;
> > +
> > + if (vbuf->field == V4L2_FIELD_ANY)
> > + vbuf->field = V4L2_FIELD_NONE;
> > +
> > + if (vb->num_planes != 1 || vbuf->field != V4L2_FIELD_NONE) {
> > + dev_err(dev, "plane or field %d not supported\n",
> > + vb->num_planes, vbuf->field);
>
> Ditto.
>
Done.
> > + return -EINVAL;
> > + }
> > + if (vb2_plane_size(vb, 0) < pixfmt->plane_fmt[0].sizeimage) {
> > + dev_err(dev, "plane %d is too small\n");
>
> Ditto.
>
Done.
> > + return -EINVAL;
> > + }
> > + break;
> > + default:
> > + dev_err(dev, "invalid queue type: %d\n", vq->type);
> > + return -EINVAL;
>
> We shouldn't need to handle this.
>
Ok, I will remove this.
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void mtk_fd_vb2_buf_queue(struct vb2_buffer *vb)
> > +{
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +
> > + v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
> > +}
> > +
> > +static int mtk_fd_vb2_queue_setup(struct vb2_queue *vq,
> > + unsigned int *num_buffers,
> > + unsigned int *num_planes,
> > + unsigned int sizes[],
> > + struct device *alloc_devs[])
> > +{
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > + struct device *dev = ctx->dev;
> > + unsigned int size;
> > +
> > + switch (vq->type) {
> > + case V4L2_BUF_TYPE_META_CAPTURE:
> > + size = ctx->dst_fmt.buffersize;
> > + break;
> > + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > + size = ctx->src_fmt.plane_fmt[0].sizeimage;
> > + break;
> > + default:
> > + dev_err(dev, "invalid queue type: %d\n", vq->type);
>
> We should need to handle this.
>
Do you mean by giving a size instead of return -EINVAL?
> > + return -EINVAL;
> > + }
> > +
> > + if (!*num_planes) {
> > + *num_planes = 1;
> > + sizes[0] = size;
> > + }
>
> We need to handle the case when *num_planes is non-zero. We then need to
> check if it's 1 and *sizes >= size and fail with -EINVAL if not.
>
Ok, I will refine as following:
if (*num_planes) {
if (sizes[0] < size || *num_planes != 1)
return -EINVAL;
} else {
*num_planes = 1;
sizes[0] = size;
}
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_vb2_start_streaming(struct vb2_queue *vq, unsigned int count)
> > +{
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > +
> > + return mtk_fd_hw_connect(&ctx->fd_dev->fd_hw);
>
> This would be called twice for every context, once for the VIDEO_OUTPUT
> queue and once for the META_CAPTURE queue. Perhaps it would make sense to
> just do this for the VIDEO_OUTPUT queue?
>
Ok, I will refine as:
if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
return mtk_fd_hw_connect(&ctx->fd_dev->fd_hw);
else
return 0;
> > +}
> > +
> > +static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
> > +{
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > + struct vb2_buffer *vb;
>
> How do we guarantee here that the hardware isn't still accessing the buffers
> removed below?
>
Maybe we can check the driver state flag and aborting the unfinished
jobs?
(fd_hw->state == FD_ENQ)
> > +
> > + if (V4L2_TYPE_IS_OUTPUT(vq->type))
> > + vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > + else
> > + vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > +
> > + while (vb) {
> > + v4l2_m2m_buf_done(to_vb2_v4l2_buffer(vb), VB2_BUF_STATE_ERROR);
> > + if (V4L2_TYPE_IS_OUTPUT(vq->type))
> > + vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > + else
> > + vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > + }
>
> We can use v4l2_m2m_buf_remove(). Also we can move the call into the loop
> condition:
>
> while ((vb == v4l2_m2m_buf_remove(...)))
> v4l2_m2m_buf_done(...);
>
Ok, I will refine as following:
while ((vb = v4l2_m2m_buf_remove(V4L2_TYPE_IS_OUTPUT(vq->type)?
&m2m_ctx->out_q_ctx :
&m2m_ctx->cap_q_ctx)))
v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);
> > +
> > + mtk_fd_hw_disconnect(&ctx->fd_dev->fd_hw);
>
> This should also probably be done only for 1 queue. Since the VIDEO_OUTPUT
> queue supports requestes, I'd consider it the master one.
>
Ok, only for output queue to trigger disconnect.
if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
mtk_fd_hw_disconnect(&ctx->fd_dev->fd_hw);
> > +}
> > +
> > +static void mtk_fd_vb2_request_complete(struct vb2_buffer *vb)
> > +{
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > +
> > + v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->hdl);
> > +}
> > +
> > +static void mtk_fd_fill_pixfmt_mp(struct v4l2_pix_format_mplane *dfmt,
> > + const struct v4l2_pix_format_mplane *sfmt)
> > +{
> > + dfmt->width = sfmt->width;
> > + dfmt->height = sfmt->height;
> > + dfmt->pixelformat = sfmt->pixelformat;
> > + dfmt->field = sfmt->field;
> > + dfmt->colorspace = sfmt->colorspace;
> > + dfmt->num_planes = sfmt->num_planes;
> > +
> > + /* Use default */
> > + dfmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > + dfmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> > + dfmt->xfer_func =
> > + V4L2_MAP_XFER_FUNC_DEFAULT(dfmt->colorspace);
> > + dfmt->plane_fmt[0].bytesperline = dfmt->width * 2;
> > + dfmt->plane_fmt[0].sizeimage =
> > + dfmt->height * dfmt->plane_fmt[0].bytesperline;
> > + memset(dfmt->reserved, 0, sizeof(dfmt->reserved));
> > +}
>
> Could we unify this function with mtk_fd_m2m_try_fmt_out_mp()? That function
> should be almost directly reusable for the default format initialization +/-
> the arguments passed to it.
>
Ok, I will try to reuse it as following:
static void mtk_fd_fill_pixfmt_mp(struct v4l2_pix_format_mplane *dfmt,
const struct v4l2_pix_format_mplane *sfmt)
{
dfmt->field = V4L2_FIELD_NONE;
dfmt->colorspace = V4L2_COLORSPACE_BT2020;
dfmt->num_planes = sfmt->num_planes;
dfmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
dfmt->quantization = V4L2_QUANTIZATION_DEFAULT;
dfmt->xfer_func =
V4L2_MAP_XFER_FUNC_DEFAULT(dfmt->colorspace);
/* Keep user setting as possible */
dfmt->width = clamp(dfmt->width,
MTK_FD_OUTPUT_MIN_WIDTH,
MTK_FD_OUTPUT_MAX_WIDTH);
dfmt->height = clamp(dfmt->height,
MTK_FD_OUTPUT_MIN_HEIGHT,
MTK_FD_OUTPUT_MAX_HEIGHT);
if (sfmt->num_planes == 2) {
/* NV16M and NV61M has 1 byte per pixel */
dfmt->plane_fmt[0].bytesperline = dfmt->width;
dfmt->plane_fmt[1].bytesperline = dfmt->width;
} else {
/* 2 bytes per pixel */
dfmt->plane_fmt[0].bytesperline = dfmt->width * 2;
}
dfmt->plane_fmt[0].sizeimage =
dfmt->height * dfmt->plane_fmt[0].bytesperline;
}
> > +
> > +static const struct v4l2_pix_format_mplane *mtk_fd_find_fmt(u32 format)
> > +{
> > + unsigned int i;
> > + const struct v4l2_pix_format_mplane *dev_fmt;
> > +
> > + for (i = 0; i < NUM_FORMATS; i++) {
> > + dev_fmt = &in_img_fmts[i];
> > + if (dev_fmt->pixelformat == format)
> > + return dev_fmt;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +static int mtk_fd_m2m_querycap(struct file *file, void *fh,
> > + struct v4l2_capability *cap)
> > +{
> > + struct mtk_fd_dev *fd_dev = video_drvdata(file);
> > + struct device *dev = &fd_dev->pdev->dev;
> > +
> > + strscpy(cap->driver, dev->driver->name, sizeof(cap->driver));
>
> Please use dev_driver_string().
>
Done,
> > + strscpy(cap->card, fd_dev->vfd.name, sizeof(cap->card));
>
> I think we can also make this dev_driver_string().
>
Done.
> > + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> > + dev_name(&fd_dev->pdev->dev));
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_enum_fmt_out_mp(struct file *file, void *fh,
> > + struct v4l2_fmtdesc *f)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < NUM_FORMATS; ++i) {
> > + if (i == f->index) {
> > + f->pixelformat = in_img_fmts[i].pixelformat;
> > + return 0;
> > + }
> > + }
>
> Why don't we just check if f->index is within the [0, ARRAY_SIZE()-1] bounds
> and then just use it to index the array directly?
>
I will refine as :
static int mtk_fd_m2m_enum_fmt_out_mp(struct file *file, void *fh,
struct v4l2_fmtdesc *f)
{
if ((f->index >= 0) && (f->index < NUM_FORMATS)) {
f->pixelformat = in_img_fmts[f->index].pixelformat;
return 0;
}
return -EINVAL;
}
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int mtk_fd_m2m_try_fmt_out_mp(struct file *file,
> > + void *fh,
> > + struct v4l2_format *f)
>
> I think we could just shorten the function prefixes to "mtk_fd_".
>
Do you mean by replace mtk_fd_m2m_* with mtk_fd_ ?
> > +{
> > + struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > + const struct v4l2_pix_format_mplane *fmt;
> > +
> > + fmt = mtk_fd_find_fmt(pix_mp->pixelformat);
> > + if (!fmt) {
> > + /* Get default img fmt */
> > + fmt = &in_img_fmts[0];
> > + f->fmt.pix.pixelformat = fmt->pixelformat;
> > + }
> > +
> > + /* Use default */
> > + pix_mp->field = fmt->field;
> > + pix_mp->colorspace = fmt->colorspace;
> > + pix_mp->num_planes = fmt->num_planes;
> > + pix_mp->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > + pix_mp->quantization = V4L2_QUANTIZATION_DEFAULT;
> > + pix_mp->xfer_func =
> > + V4L2_MAP_XFER_FUNC_DEFAULT(pix_mp->colorspace);
> > +
> > + /* Keep user setting as possible */
> > + pix_mp->width = clamp(pix_mp->width,
> > + MTK_FD_OUTPUT_MIN_WIDTH,
> > + MTK_FD_OUTPUT_MAX_WIDTH);
> > + pix_mp->height = clamp(pix_mp->height,
> > + MTK_FD_OUTPUT_MIN_HEIGHT,
> > + MTK_FD_OUTPUT_MAX_HEIGHT);
> > +
> > + pix_mp->plane_fmt[0].bytesperline = pix_mp->width * 2;
>
> Please add a comment explaining why this is always 2.
>
Done.
/* 2 bytes per pixel */
> > + pix_mp->plane_fmt[0].sizeimage =
> > + pix_mp->plane_fmt[0].bytesperline * pix_mp->height;
> > + memset(pix_mp->plane_fmt[0].reserved, 0,
> > + sizeof(pix_mp->plane_fmt[0].reserved));
>
> No need to zero this here. The core will handle it.
>
Ok, I will remove it.
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_g_fmt_out_mp(struct file *file, void *fh,
> > + struct v4l2_format *f)
> > +{
> > + struct mtk_fd_ctx *ctx = fh_to_ctx(fh);
> > +
> > + f->fmt.pix_mp = ctx->src_fmt;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_s_fmt_out_mp(struct file *file, void *fh,
> > + struct v4l2_format *f)
> > +{
> > + struct mtk_fd_ctx *ctx = fh_to_ctx(fh);
> > + struct vb2_queue *vq;
> > +
> > + /* Change not allowed if queue is streaming. */
> > + vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> > + if (vb2_is_streaming(vq) || vb2_is_busy(vq)) {
>
> vb2_is_busy() is a superset of vb2_is_streaming(), so it's enough to just
> check the former.
>
Ok, I will just check the former.
> > + dev_dbg(ctx->dev, "vb2_is_streaming or vb2_is_busy");
> > + return -EBUSY;
> > + }
> > +
> > + mtk_fd_m2m_try_fmt_out_mp(file, fh, f);
> > + ctx->src_fmt = f->fmt.pix_mp;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_enum_fmt_meta_cap(struct file *file, void *fh,
> > + struct v4l2_fmtdesc *f)
> > +{
> > + if (f->index)
> > + return -EINVAL;
> > +
> > + strscpy(f->description, "Face detection result",
> > + sizeof(f->description));
> > + f->pixelformat = fw_param_fmts[0].dataformat;
> > + f->flags = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_g_fmt_meta_cap(struct file *file, void *fh,
> > + struct v4l2_format *f)
> > +{
> > + f->fmt.meta.dataformat = fw_param_fmts[0].dataformat;
> > + f->fmt.meta.buffersize = fw_param_fmts[0].buffersize;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct vb2_ops mtk_fd_vb2_ops = {
> > + .queue_setup = mtk_fd_vb2_queue_setup,
> > + .buf_out_validate = mtk_fd_vb2_buf_out_validate,
> > + .buf_prepare = mtk_fd_vb2_buf_prepare,
> > + .buf_queue = mtk_fd_vb2_buf_queue,
> > + .start_streaming = mtk_fd_vb2_start_streaming,
> > + .stop_streaming = mtk_fd_vb2_stop_streaming,
> > + .wait_prepare = vb2_ops_wait_prepare,
> > + .wait_finish = vb2_ops_wait_finish,
> > + .buf_request_complete = mtk_fd_vb2_request_complete,
> > +};
> > +
> > +static const struct v4l2_ioctl_ops mtk_fd_v4l2_video_out_ioctl_ops = {
> > + .vidioc_querycap = mtk_fd_m2m_querycap,
> > + .vidioc_enum_fmt_vid_out_mplane = mtk_fd_m2m_enum_fmt_out_mp,
> > + .vidioc_g_fmt_vid_out_mplane = mtk_fd_m2m_g_fmt_out_mp,
> > + .vidioc_s_fmt_vid_out_mplane = mtk_fd_m2m_s_fmt_out_mp,
> > + .vidioc_try_fmt_vid_out_mplane = mtk_fd_m2m_try_fmt_out_mp,
> > + .vidioc_enum_fmt_meta_cap = mtk_fd_m2m_enum_fmt_meta_cap,
> > + .vidioc_g_fmt_meta_cap = mtk_fd_m2m_g_fmt_meta_cap,
> > + .vidioc_s_fmt_meta_cap = mtk_fd_m2m_g_fmt_meta_cap,
> > + .vidioc_try_fmt_meta_cap = mtk_fd_m2m_g_fmt_meta_cap,
> > + .vidioc_reqbufs = v4l2_m2m_ioctl_reqbufs,
> > + .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
> > + .vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
> > + .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
> > + .vidioc_querybuf = v4l2_m2m_ioctl_querybuf,
> > + .vidioc_qbuf = v4l2_m2m_ioctl_qbuf,
> > + .vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf,
> > + .vidioc_streamon = v4l2_m2m_ioctl_streamon,
> > + .vidioc_streamoff = v4l2_m2m_ioctl_streamoff,
> > + .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> > + .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> > +};
> > +
> > +static int
> > +mtk_fd_queue_init(void *priv, struct vb2_queue *src_vq,
> > + struct vb2_queue *dst_vq)
> > +{
> > + struct mtk_fd_ctx *ctx = priv;
> > + int ret;
> > +
> > + src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> > + src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> > + src_vq->supports_requests = true;
> > + src_vq->drv_priv = ctx;
> > + src_vq->ops = &mtk_fd_vb2_ops;
> > + src_vq->mem_ops = &vb2_dma_contig_memops;
> > + src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> > + src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > + src_vq->lock = &ctx->fd_dev->vfd_lock;
> > + src_vq->dev = ctx->fd_dev->v4l2_dev.dev;
> > +
> > + ret = vb2_queue_init(src_vq);
> > + if (ret)
> > + return ret;
> > +
> > + dst_vq->type = V4L2_BUF_TYPE_META_CAPTURE;
> > + dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> > + dst_vq->drv_priv = ctx;
> > + dst_vq->ops = &mtk_fd_vb2_ops;
> > + dst_vq->mem_ops = &vb2_dma_contig_memops;
> > + dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> > + dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > + dst_vq->lock = &ctx->fd_dev->vfd_lock;
> > + dst_vq->dev = ctx->fd_dev->v4l2_dev.dev;
> > +
> > + return vb2_queue_init(dst_vq);
> > +}
> > +
> > +static int mtk_fd_dev_g_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct mtk_fd_ctx *ctx = ctrl->priv;
> > + int i;
> > +
> > + switch (ctrl->id) {
> > + case V4L2_CID_MTK_FD_SCALE_IMG_WIDTH:
> > + for (i = 0; i < ctrl->elems; i++)
> > + ctrl->p_new.p_u16[i] =
> > + ctx->user_param.scale_img_width[i];
> > + break;
> > + case V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT:
> > + for (i = 0; i < ctrl->elems; i++)
> > + ctrl->p_new.p_u16[i] =
> > + ctx->user_param.scale_img_height[i];
> > + break;
> > + case V4L2_CID_MTK_FD_DETECT_POSE:
> > + ctrl->val = ctx->user_param.fd_pose;
> > + break;
> > + case V4L2_CID_MTK_FD_DETECT_SPEEDUP:
> > + ctrl->val = ctx->user_param.fd_speedup;
> > + break;
> > + case V4L2_CID_MTK_FD_SCALE_IMG_NUM:
> > + ctrl->val = ctx->user_param.scale_img_num;
> > + break;
> > + case V4L2_CID_MTK_FD_EXTRA_MODEL:
> > + ctrl->val = ctx->user_param.fd_extra_model;
> > + break;
> > + default:
> > + dev_dbg(ctx->dev, "%s: unexpected control: 0x%x:%d",
> > + __func__, ctrl->id, ctrl->val);
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_dev_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct mtk_fd_ctx *ctx = ctrl->priv;
> > + int i;
> > +
> > + switch (ctrl->id) {
> > + case V4L2_CID_MTK_FD_SCALE_IMG_WIDTH:
> > + for (i = 0; i < ctrl->elems; i++)
> > + ctx->user_param.scale_img_width[i] =
> > + ctrl->p_new.p_u16[i];
> > + break;
> > + case V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT:
> > + for (i = 0; i < ctrl->elems; i++)
> > + ctx->user_param.scale_img_height[i] =
> > + ctrl->p_new.p_u16[i];
> > + break;
> > + case V4L2_CID_MTK_FD_DETECT_POSE:
> > + ctx->user_param.fd_pose = ctrl->val;
> > + break;
> > + case V4L2_CID_MTK_FD_DETECT_SPEEDUP:
> > + ctx->user_param.fd_speedup = ctrl->val;
> > + break;
> > + case V4L2_CID_MTK_FD_SCALE_IMG_NUM:
> > + ctx->user_param.scale_img_num = ctrl->val;
> > + break;
> > + case V4L2_CID_MTK_FD_EXTRA_MODEL:
> > + ctx->user_param.fd_extra_model = ctrl->val;
> > + break;
> > + default:
> > + dev_dbg(ctx->dev, "%s: unexpected control: 0x%x:%d",
> > + __func__, ctrl->id, ctrl->val);
> > + return -EINVAL;
>
> No need to handle this, as the framework will only pass the controls we
> registered earlier to this function.
>
Ok, I will remove this .
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops mtk_fd_dev_ctrl_ops = {
> > + .g_volatile_ctrl = mtk_fd_dev_g_ctrl,
>
> I don't see any volatile controls below. No need to implement this callback.
>
Ok, I will remove this .
> > + .s_ctrl = mtk_fd_dev_s_ctrl,
>
> I think you might not even need to implement this function (or just provide
> a dummy one that returns 0 if its required) if you just use the controls
> directly when preparing the job for the hardware. Check v4l2_ctrl_find().
>
I will remove the mtk_fd_dev_ctrl_ops, and use the following function to
get control values when preparing the job for HW,
static void mtk_fd_fill_user_param(struct user_param *user_param,
struct v4l2_ctrl_handler *hdl)
{
struct v4l2_ctrl *ctrl;
int i;
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_SCALE_IMG_WIDTH);
if (ctrl)
for (i = 0; i < ctrl->elems; i++)
user_param->scale_img_width[i] = ctrl->p_new.p_u16[i];
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT);
if (ctrl)
for (i = 0; i < ctrl->elems; i++)
user_param->scale_img_height[i] = ctrl->p_new.p_u16[i];
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_DETECT_POSE);
if (ctrl)
user_param->fd_pose = ctrl->val;
pr_info("pose:%d\n", user_param->fd_pose);
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_DETECT_SPEEDUP);
if (ctrl)
user_param->fd_speedup = ctrl->val;
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_SCALE_IMG_NUM);
if (ctrl)
user_param->scale_img_num = ctrl->val;
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_EXTRA_MODEL);
if (ctrl)
user_param->fd_extra_model = ctrl->val;
}
> > +};
> > +
> > +struct v4l2_ctrl_config mtk_fd_controls[] = {
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_SCALE_IMG_WIDTH,
> > + .name = "FD scale image widths",
> > + .type = V4L2_CTRL_TYPE_U16,
> > + .min = MTK_FD_OUTPUT_MIN_WIDTH,
> > + .max = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .step = 1,
> > + .def = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .dims = { FD_SCALE_NUM },
>
> Something wrong with indentation here.
>
Done.
> > + },
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT,
> > + .name = "FD scale image heights",
> > + .type = V4L2_CTRL_TYPE_U16,
> > + .min = MTK_FD_OUTPUT_MIN_HEIGHT,
> > + .max = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .step = 1,
> > + .def = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .dims = { FD_SCALE_NUM },
> > + },
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_SCALE_IMG_NUM,
> > + .name = "FD scale size counts",
> > + .type = V4L2_CTRL_TYPE_INTEGER,
> > + .min = 0,
> > + .max = FACE_SIZE_NUM_MAX,
> > + .step = 1,
> > + .def = 0,
> > + },
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_DETECT_POSE,
> > + .name = "FD detect face pose",
> > + .type = V4L2_CTRL_TYPE_INTEGER,
> > + .min = 0,
> > + .max = 3,
> > + .step = 1,
> > + .def = 0,
> > + },
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_DETECT_SPEEDUP,
> > + .name = "FD detection speedup",
> > + .type = V4L2_CTRL_TYPE_INTEGER,
> > + .min = 0,
> > + .max = FD_MAX_SPEEDUP,
> > + .step = 1,
> > + .def = 0,
> > + },
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_EXTRA_MODEL,
> > + .name = "FD use extra model",
> > + .type = V4L2_CTRL_TYPE_BOOLEAN,
> > + .min = 0,
> > + .max = 1,
> > + .step = 1,
> > + .def = 0,
> > + },
> > +};
> > +
> > +static int mtk_fd_ctrls_setup(struct mtk_fd_ctx *ctx)
> > +{
> > + struct v4l2_ctrl_handler *hdl = &ctx->hdl;
> > + struct v4l2_ctrl *ctl;
> > + int i;
> > +
> > + v4l2_ctrl_handler_init(hdl, V4L2_CID_MTK_FD_MAX);
> > + if (hdl->error)
> > + return hdl->error;
> > +
> > + for (i = 0; i < ARRAY_SIZE(mtk_fd_controls); i++) {
> > + ctl = v4l2_ctrl_new_custom(hdl, &mtk_fd_controls[i], ctx);
> > + if (hdl->error) {
> > + v4l2_ctrl_handler_free(hdl);
> > + dev_err(ctx->dev, "Failed to register controls:%d", i);
> > + return hdl->error;
> > + }
> > + }
> > +
> > + ctx->fh.ctrl_handler = &ctx->hdl;
> > + v4l2_ctrl_handler_setup(hdl);
> > +
> > + return 0;
> > +}
> > +
> > +static unsigned int get_fd_img_fmt(unsigned int fourcc)
> > +{
> > + switch (fourcc) {
> > + case V4L2_PIX_FMT_VYUY:
> > + return FMT_VYUY;
> > + case V4L2_PIX_FMT_YUYV:
> > + return FMT_YUYV;
> > + case V4L2_PIX_FMT_YVYU:
> > + return FMT_YVYU;
> > + case V4L2_PIX_FMT_UYVY:
> > + return FMT_UYVY;
> > + default:
> > + return FMT_UNKNOWN;
> > + }
> > +}
> > +
> > +static void init_ctx_fmt(struct mtk_fd_ctx *ctx)
> > +{
> > + const struct v4l2_pix_format_mplane *fmt;
> > +
> > + /* Initialize M2M source fmt */
> > + fmt = &in_img_fmts[0];
> > + mtk_fd_fill_pixfmt_mp(&ctx->src_fmt, fmt);
> > +
> > + /* Initialize M2M destination fmt */
> > + ctx->dst_fmt.buffersize = fw_param_fmts[0].buffersize;
> > + ctx->dst_fmt.dataformat = fw_param_fmts[0].dataformat;
>
> Why not just make dst_fmt a pointer and assign &fw_params_fmt[0] there?
>
fw_params_fmt[] will be removed and assign the const value here.
ctx->dst_fmt.buffersize = sizeof(struct fd_user_output);
ctx->dst_fmt.dataformat = V4L2_META_FMT_MTFD_RESULT;
> > +}
> > +
> > +/*
> > + * V4L2 file operations.
> > + */
> > +static int mtk_vfd_open(struct file *filp)
> > +{
> > + struct mtk_fd_dev *fd_dev = video_drvdata(filp);
> > + struct video_device *vdev = video_devdata(filp);
> > + struct mtk_fd_ctx *ctx;
> > + int ret;
> > +
> > + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> > + if (!ctx)
> > + return -ENOMEM;
> > +
> > + ctx->fd_dev = fd_dev;
> > + ctx->dev = &fd_dev->pdev->dev;
> > +
> > + v4l2_fh_init(&ctx->fh, vdev);
> > + filp->private_data = &ctx->fh;
> > +
> > + init_ctx_fmt(ctx);
> > +
> > + ret = mtk_fd_ctrls_setup(ctx);
> > + if (ret) {
> > + dev_err(ctx->dev, "Failed to set up controls:%d\n", ret);
> > + goto err_fh_ctrl;
> > + }
> > +
> > + ctx->fh.m2m_ctx = v4l2_m2m_ctx_init(fd_dev->m2m_dev, ctx,
> > + &mtk_fd_queue_init);
> > + if (IS_ERR(ctx->fh.m2m_ctx)) {
> > + ret = PTR_ERR(ctx->fh.m2m_ctx);
> > + goto err_init_ctx;
> > + }
> > +
> > + v4l2_fh_add(&ctx->fh);
> > +
> > + return 0;
> > +
> > +err_init_ctx:
> > + v4l2_ctrl_handler_free(&ctx->hdl);
> > +err_fh_ctrl:
> > + v4l2_fh_exit(&ctx->fh);
> > + kfree(ctx);
> > +
> > + return ret;
> > +}
> > +
> > +static int mtk_vfd_release(struct file *filp)
> > +{
> > + struct mtk_fd_ctx *ctx = container_of(filp->private_data,
> > + struct mtk_fd_ctx, fh);
> > +
> > + v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
> > +
> > + v4l2_ctrl_handler_free(&ctx->hdl);
> > + v4l2_fh_del(&ctx->fh);
> > + v4l2_fh_exit(&ctx->fh);
> > +
> > + kfree(ctx);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct v4l2_file_operations fd_video_fops = {
> > + .owner = THIS_MODULE,
> > + .open = mtk_vfd_open,
> > + .release = mtk_vfd_release,
> > + .poll = v4l2_m2m_fop_poll,
> > + .unlocked_ioctl = video_ioctl2,
> > + .mmap = v4l2_m2m_fop_mmap,
>
> We also need the compat_ioctl here for 32-bit userspace on 64-bit kernels.
>
Done.
> > +};
> > +
> > +static void mtk_fd_device_run(void *priv)
> > +{
> > + struct mtk_fd_ctx *ctx = priv;
> > + struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > + struct fd_hw_param fd_param;
> > + void *fd_result_vaddr;
> > +
> > + src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > + dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > +
> > + memset(&fd_param, 0, sizeof(fd_param));
> > +
> > + fd_param.src_img.dma_addr =
> > + vb2_dma_contig_plane_dma_addr(&src_buf->vb2_buf, 0);
> > + fd_param.user_result.dma_addr =
> > + vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
> > + fd_result_vaddr = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
> > +
> > + ctx->user_param.src_img_fmt = get_fd_img_fmt(ctx->src_fmt.pixelformat);
> > + memcpy(&fd_param.user_param, &ctx->user_param, sizeof(ctx->user_param));
> > +
> > + /* Complete request controls if any */
> > + v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req, &ctx->hdl);
> > +
> > + mtk_fd_hw_job_exec(&ctx->fd_dev->fd_hw, &fd_param, fd_result_vaddr);
> > +}
> > +
> > +static struct v4l2_m2m_ops fd_m2m_ops = {
> > + .device_run = mtk_fd_device_run,
> > +};
> > +
> > +static int mtk_fd_request_validate(struct media_request *req)
> > +{
> > + unsigned int count;
> > +
> > + count = vb2_request_buffer_cnt(req);
> > + if (!count)
> > + return -ENOENT;
>
> Why -ENOENT?
>
Reference the return code in vb2_request_validate()
I consider refining as following:
static int mtk_fd_request_validate(struct media_request *req)
{
if (vb2_request_buffer_cnt(req) > 1)
return -EINVAL;
return vb2_request_validate(req);
}
or maybe I don't need to worry the request count greater than 1,
just remove this function and set vb2_request_validate as .req_validate
directly?
> > + else if (count > 1)
> > + return -EINVAL;
> > +
> > + return vb2_request_validate(req);
> > +}
> > +
> > +static const struct media_device_ops fd_m2m_media_ops = {
> > + .req_validate = mtk_fd_request_validate,
> > + .req_queue = v4l2_m2m_request_queue,
> > +};
> > +
> > +static int mtk_fd_video_device_register(struct mtk_fd_dev *fd_dev)
> > +{
> > + struct video_device *vfd = &fd_dev->vfd;
> > + struct v4l2_m2m_dev *m2m_dev = fd_dev->m2m_dev;
> > + struct device *dev = &fd_dev->pdev->dev;
> > + int function, ret;
> > +
> > + vfd->fops = &fd_video_fops;
> > + vfd->release = video_device_release;
> > + vfd->lock = &fd_dev->vfd_lock;
> > + vfd->v4l2_dev = &fd_dev->v4l2_dev;
> > + vfd->vfl_dir = VFL_DIR_M2M;
> > + vfd->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_OUTPUT_MPLANE |
> > + V4L2_CAP_META_CAPTURE;
> > + vfd->ioctl_ops = &mtk_fd_v4l2_video_out_ioctl_ops;
> > +
> > + strscpy(vfd->name, "MTK-FD-V4L2", sizeof(vfd->name));
>
> Please use dev_driver_string().
>
Done.
> > + video_set_drvdata(vfd, fd_dev);
> > +
> > + ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
> > + if (ret) {
> > + dev_err(dev, "Failed to register video device\n");
> > + goto err_free_dev;
> > + }
> > +
> > + function = MEDIA_ENT_F_PROC_VIDEO_DECODER;
>
> MEDIA_ENT_F_PROC_VIDEO_STATISTICS fits here much more.
>
> Also nit: Any reason to have this assigned to this intermediate variable
> rather than just directly passed to the function?
>
Too long for 80 columns, I will remove the intermediate variable and
refine as following:
ret = v4l2_m2m_register_media_controller(m2m_dev, vfd,
MEDIA_ENT_F_PROC_VIDEO_STATISTICS);
> > + ret = v4l2_m2m_register_media_controller(m2m_dev, vfd, function);
> > + if (ret) {
> > + dev_err(dev, "Failed to init mem2mem media controller\n");
> > + goto err_unreg_video;
> > + }
> > + return 0;
> > +
> > +err_unreg_video:
> > + video_unregister_device(vfd);
> > +err_free_dev:
> > + video_device_release(vfd);
> > + return ret;
> > +}
> > +
> > +static int mtk_fd_dev_v4l2_init(struct mtk_fd_dev *fd_dev)
> > +{
> > + struct media_device *mdev = &fd_dev->mdev;
> > + struct device *dev = &fd_dev->pdev->dev;
> > + int ret;
> > +
> > + ret = v4l2_device_register(dev, &fd_dev->v4l2_dev);
> > + if (ret) {
> > + dev_err(dev, "Failed to register v4l2 device\n");
> > + return ret;
> > + }
> > +
> > + fd_dev->m2m_dev = v4l2_m2m_init(&fd_m2m_ops);
> > + if (IS_ERR(fd_dev->m2m_dev)) {
> > + dev_err(dev, "Failed to init mem2mem device\n");
> > + ret = PTR_ERR(fd_dev->m2m_dev);
> > + goto fail_m2m_dev;
>
> Please name the labels after the next clean-up step to be done, not the
> failure.
>
Ok, I will refine as following:
err_unreg_vdev:
v4l2_m2m_unregister_media_controller(fd_dev->m2m_dev);
video_unregister_device(&fd_dev->vfd);
video_device_release(&fd_dev->vfd);
err_cleanup_mdev:
media_device_cleanup(mdev);
v4l2_m2m_release(fd_dev->m2m_dev);
err_unreg_v4l2_dev:
v4l2_device_unregister(&fd_dev->v4l2_dev);
> > + }
> > +
> > + mdev->dev = dev;
> > + strscpy(mdev->model, "MTK-FD-V4L2", sizeof(mdev->model));
>
> Could we just use dev_driver_string() here instead?
>
Done.
> > + snprintf(mdev->bus_info, sizeof(mdev->bus_info),
> > + "platform:%s", dev_name(dev));
> > + media_device_init(mdev);
> > + mdev->ops = &fd_m2m_media_ops;
> > + fd_dev->v4l2_dev.mdev = mdev;
> > +
> > + ret = mtk_fd_video_device_register(fd_dev);
> > + if (ret) {
> > + dev_err(dev, "Failed to register video device\n");
> > + goto err_vdev;
> > + }
> > +
> > + ret = media_device_register(mdev);
> > + if (ret) {
> > + dev_err(dev, "Failed to register mem2mem media device\n");
> > + goto fail_mdev;
> > + }
> > +
> > + return 0;
> > +
> > +fail_mdev:
> > + v4l2_m2m_unregister_media_controller(fd_dev->m2m_dev);
> > + video_unregister_device(&fd_dev->vfd);
> > + video_device_release(&fd_dev->vfd);
> > +err_vdev:
> > + media_device_cleanup(mdev);
> > + v4l2_m2m_release(fd_dev->m2m_dev);
> > +fail_m2m_dev:
> > + v4l2_device_unregister(&fd_dev->v4l2_dev);
> > + return ret;
> > +}
> > +
> > +static void mtk_fd_dev_v4l2_release(struct mtk_fd_dev *fd_dev)
> > +{
> > + v4l2_m2m_unregister_media_controller(fd_dev->m2m_dev);
> > + video_unregister_device(&fd_dev->vfd);
> > + video_device_release(&fd_dev->vfd);
> > + media_device_cleanup(&fd_dev->mdev);
> > + v4l2_m2m_release(fd_dev->m2m_dev);
> > + v4l2_device_unregister(&fd_dev->v4l2_dev);
> > +}
> > +
> > +static irqreturn_t mtk_fd_irq(int irq, void *data)
> > +{
> > + struct mtk_fd_hw *fd_hw = (struct mtk_fd_hw *)data;
> > +
> > + fd_hw->fd_irq_result = readl(fd_hw->fd_base + FD_INT);
>
> We should be able to just handle the job completion directly from here.
>
Ok, I will try to handle the job completion from here.
> > + wake_up_interruptible(&fd_hw->wq);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int mtk_fd_hw_get_scp_mem(struct mtk_fd_hw *fd_hw,
> > + struct fd_buffer *scp_mem)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + struct device *dev = &fd_dev->pdev->dev;
> > + dma_addr_t addr;
> > + u32 size;
> > +
> > + scp_mem->scp_addr = scp_get_reserve_mem_phys(SCP_FD_MEM_ID);
> > + size = scp_get_reserve_mem_size(SCP_FD_MEM_ID);
> > + if (!scp_mem->scp_addr || !size)
> > + return -EPROBE_DEFER;
>
> Why -EPROBE_DEFER? Should we have some way to distinguish between the SCP
> driver not being initialized yet and some other error?
>
Ok, I will fix it to -ENOMEM.
Not being initialized or some other error can be found during device
probe.
(will also load scp stuff there)
> > +
> > + /* get dma addr address */
> > + addr = dma_map_page_attrs(dev, phys_to_page(scp_mem->scp_addr), 0,
> > + size, DMA_BIDIRECTIONAL,
>
> Should this be really bidirectional?
>
I will fix it to DMA_TO_DEVICE
> > + DMA_ATTR_SKIP_CPU_SYNC);
> > + if (dma_mapping_error(dev, addr)) {
> > + scp_mem->scp_addr = 0;
> > + dev_err(dev, "Failed to map scp addr\n");
> > + return -ENOMEM;
> > + }
> > + scp_mem->dma_addr = addr;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_probe(struct platform_device *pdev)
> > +{
> > + struct mtk_fd_dev *fd_dev;
>
> nit: Perhaps just fd, to have shorter code?
>
Ok, I will refine as
struct mtk_fd_dev *fd;
> > + struct device *dev = &pdev->dev;
> > + struct mtk_fd_hw *fd_hw;
> > + struct resource *res;
> > + int irq;
> > + int ret;
> > +
> > + fd_dev = devm_kzalloc(&pdev->dev, sizeof(*fd_dev), GFP_KERNEL);
> > + if (!fd_dev)
> > + return -ENOMEM;
> > +
> > + dev_set_drvdata(dev, fd_dev);
> > + fd_hw = &fd_dev->fd_hw;
> > + fd_dev->pdev = pdev;
>
> Is pdev useful for anything? Perhaps you want to store &pdev->dev instead?
>
Yes, I will store that instead.
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0) {
> > + dev_err(dev, "no IRQ:%d resource info\n", irq);
> > + return irq;
> > + }
> > + ret = devm_request_irq(dev, irq, mtk_fd_irq, IRQF_SHARED,
> > + dev_driver_string(dev),
> > + fd_hw);
>
> Why not just fd_dev?
>
Ok, I will use fd_dev here.
> > + if (ret) {
> > + dev_err(dev, "req_irq fail:%d\n", irq);
> > + return ret;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + fd_hw->fd_base = devm_ioremap_resource(dev, res);
> > + if (!fd_hw->fd_base) {
> > + dev_err(dev, "unable to get fd reg base\n");
> > + return PTR_ERR(fd_hw->fd_base);
> > + }
> > +
> > + fd_hw->fd_clk = devm_clk_get(dev, "fd");
> > + if (IS_ERR(fd_hw->fd_clk)) {
> > + dev_err(dev, "cannot get fd_clk_img_fd clock\n");
> > + return PTR_ERR(fd_hw->fd_clk);
> > + }
> > +
> > + ret = mtk_fd_hw_get_scp_mem(fd_hw, &fd_hw->scp_mem);
> > + if (ret) {
> > + dev_err(dev, "scp memory init failed: %d\n", ret);
>
> nit: Could we make the error messages a bit more consistent? For example
> "Failed to get IRQ resource (%d)", "Failed to request IRQ (%d)", "Failed to
> ... (%d)", etc.
>
Ok, I will fix it.
> > + return ret;
> > + }
> > +
> > + atomic_set(&fd_hw->fd_user_cnt, 0);
> > + init_waitqueue_head(&fd_hw->wq);
> > + mutex_init(&fd_dev->vfd_lock);
> > + mutex_init(&fd_hw->fd_hw_lock);
> > + pm_runtime_enable(dev);
> > +
> > + ret = mtk_fd_dev_v4l2_init(fd_dev);
> > + if (ret) {
> > + mutex_destroy(&fd_dev->fd_hw.fd_hw_lock);
> > + mutex_destroy(&fd_dev->vfd_lock);
> > + pm_runtime_disable(&pdev->dev);
>
> Please move the clean-up to an error path on the bottom of the function.
>
Done.
> > + dev_err(dev, "v4l2 init failed: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_remove(struct platform_device *pdev)
> > +{
> > + struct mtk_fd_dev *fd_dev = dev_get_drvdata(&pdev->dev);
> > +
> > + mtk_fd_dev_v4l2_release(fd_dev);
> > + mutex_destroy(&fd_dev->fd_hw.fd_hw_lock);
> > + mutex_destroy(&fd_dev->vfd_lock);
> > + pm_runtime_disable(&pdev->dev);
>
> The order of calls in remove should normally be the opposite to probe.
>
Ok, I will refine as following
mtk_fd_dev_v4l2_release(fd_dev);
pm_runtime_disable(&pdev->dev);
mutex_destroy(&fd_dev->fd_hw.fd_hw_lock);
mutex_destroy(&fd_dev->vfd_lock);
rproc_put(fd_dev->fd_hw.rproc_handle);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_suspend(struct device *dev)
> > +{
> > + struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
> > +
> > + if (pm_runtime_suspended(dev))
> > + return 0;
> > +
> > + /* suspend FD HW */
> > + writel(0x0, fd_dev->fd_hw.fd_base + FD_HW_ENABLE);
> > + writel(0x0, fd_dev->fd_hw.fd_base + FD_INT_EN);
> > + clk_disable_unprepare(fd_dev->fd_hw.fd_clk);
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_resume(struct device *dev)
> > +{
> > + struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + if (pm_runtime_suspended(dev))
> > + return 0;
> > +
> > + ret = clk_prepare_enable(fd_dev->fd_hw.fd_clk);
> > + if (ret < 0) {
> > + dev_dbg(dev, "open fd clk failed\n");
> > + clk_disable_unprepare(fd_dev->fd_hw.fd_clk);
> > + }
> > +
> > + /* resume FD HW */
> > + writel(ENABLE_FD, fd_dev->fd_hw.fd_base + FD_HW_ENABLE);
> > + if (fd_dev->fd_hw.state == FD_ENQ)
> > + writel(0x1, fd_dev->fd_hw.fd_base + FD_INT_EN);
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mtk_fd_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(mtk_fd_suspend, mtk_fd_resume)
> > + SET_RUNTIME_PM_OPS(mtk_fd_suspend, mtk_fd_resume, NULL)
>
> We need separate runtime and system PM ops. Their behavior is expected to be
> different.
>
> For runtime PM ops, you the functions should just unconditionally power on
> or power off the device.
>
> For system PM ops, suspend needs to make sure that no further job is queued
> to the hardware and that any job being already processed by the hardware is
> completed. Resume needs to resume the processing if there are any further
> jobs to be queued to the hardware.
>
Ok, for runtime suspend/resume I will add the following functions:
static int mtk_fd_runtime_suspend(struct device *dev)
{
struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
dev_dbg(dev, "%s:disable clock\n", __func__);
clk_disable_unprepare(fd_dev->fd_hw.fd_clk);
return 0;
}
static int mtk_fd_runtime_resume(struct device *dev)
{
struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
int ret;
dev_dbg(dev, "%s:enable clock\n", __func__);
ret = clk_prepare_enable(fd_dev->fd_hw.fd_clk);
if (ret < 0) {
dev_err(dev, "Failed to open fd clk:%d\n", ret);
return ret;
}
return 0;
}
Best regards,
Jerry
> > +};
> > +
> > +static const struct of_device_id mtk_fd_of_ids[] = {
> > + { .compatible = "mediatek,mt8183-fd", },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_fd_of_ids);
> > +
> > +static struct platform_driver mtk_fd_driver = {
> > + .probe = mtk_fd_probe,
> > + .remove = mtk_fd_remove,
> > + .driver = {
> > + .name = "mtk-fd-4.0",
> > + .of_match_table = of_match_ptr(mtk_fd_of_ids),
> > + .pm = &mtk_fd_pm_ops,
> > + }
> > +};
> > +module_platform_driver(mtk_fd_driver);
> > +
> > +MODULE_DESCRIPTION("Mediatek FD driver");
> > +MODULE_LICENSE("GPL");
>
> GPL v2
>
> Best regards,
> Tomasz
>
