Re: [RFC PATCH V4 4/4] platform: mtk-isp: Add Mediatek FD driver
From: Jerry-ch Chen <hidden>
Date: 2020-05-22 14:11:23
Also in:
linux-devicetree, linux-media, linux-mediatek
Hi Tomasz, On Thu, 2020-05-21 at 18:28 +0000, Tomasz Figa wrote:
Hi Jerry, On Wed, Dec 04, 2019 at 08:47:32PM +0800, Jerry-ch Chen wrote:quoted
From: Jerry-ch Chen <redacted> 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 <redacted> --- drivers/media/platform/Kconfig | 2 + drivers/media/platform/Makefile | 2 + drivers/media/platform/mtk-isp/fd/Kconfig | 19 + drivers/media/platform/mtk-isp/fd/Makefile | 5 + drivers/media/platform/mtk-isp/fd/mtk_fd.h | 149 ++ drivers/media/platform/mtk-isp/fd/mtk_fd_40.c | 1279 +++++++++++++++++ include/uapi/linux/v4l2-controls.h | 4 + include/uapi/linux/videodev2.h | 3 + 8 files changed, 1463 insertions(+) create mode 100644 drivers/media/platform/mtk-isp/fd/Kconfig 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.cThank you for the patch. Please see my comments inline. Really sorry for the much delayed review.
I appreciate your comments, here's the reply
[snip]quoted
+config VIDEO_MEDIATEK_FD + tristate "Mediatek face detection processing function" + depends on VIDEO_V4L2 + depends on ARCH_MEDIATEK + select VIDEOBUF2_DMA_CONTIG + select VIDEOBUF2_CORE + select VIDEOBUF2_V4L2 + select VIDEOBUF2_MEMOPS + select MEDIA_CONTROLLER + select MTK_SCP + + default n + help + Support the Face Detection (FD) feature in the Mediatek + mt8183 Soc.MT8183?
Fixed.
quoted
+ + FD driver is a V4L2 memory-to-memory device driver which + provides hardware accelerated face detection function, + it can detect different sizes of faces in a raw image.A YUV image I guess?
Fixed
[snip]quoted
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 000000000000..d85bdcb70d6d --- /dev/null +++ b/drivers/media/platform/mtk-isp/fd/mtk_fd.h@@ -0,0 +1,149 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +// +// Copyright (c) 2018 MediaTek Inc. + +#ifndef __MTK_FD_HW_H__ +#define __MTK_FD_HW_H__ + +#include <linux/completion.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 + +#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_YVU_2P 6 +#define MTK_FD_HW_FMT_YUV_2P 7 +#define MTK_FD_HW_FMT_UNKNOWN 8What is an unknown format?
It's used in translate v4l2 pixel format to the value that FD HW recognized. But I think the case of UNKNWOWN shouldn't happened. I will remove it
quoted
+ +#define MTK_FD_IPI_CMD_INIT 0 +#define MTK_FD_IPI_CMD_INIT_ACK 1 +#define MTK_FD_IPI_CMD_ENQUEUE 2 +#define MTK_FD_IPI_CMD_ENQ_ACK 3 +#define MTK_FD_IPI_CMD_EXIT 4 +#define MTK_FD_IPI_CMD_EXIT_ACK 5 +#define MTK_FD_IPI_CMD_RESET 6 +#define MTK_FD_IPI_CMD_RESET_ACK 7 + +#define MTK_FD_REG_OFFSET_HW_ENABLE 0x4 +#define MTK_FD_REG_OFFSET_INT_EN 0x15c +#define MTK_FD_REG_OFFSET_INT_VAL 0x168 +#define MTK_FD_REG_OFFSET_RESULT 0x178 + +#define MTK_FD_SET_HW_ENABLE 0x111 +#define MTK_FD_RS_BUF_SIZE 2289664 +#define MTK_FD_HW_WORK_BUF_SIZE 0x100000How about using the SZ_1M?
yes, fixed.
quoted
+#define MTK_FD_MAX_SPEEDUP 7 +#define MTK_FD_MAX_RESULT_NUM 1026Perhaps NUM_RESULTS?
fixed as MTK_FD_MAX_NUM_RESULT.
quoted
+ +/* Max scale size counts */ +#define MTK_FD_SCALE_ARR_NUM 15Perhaps NUM_SCALE_SIZES?
fixed as MTK_FD_NUM_SCALE_SIZE
quoted
+ +#define MTK_FD_HW_TIMEOUT 1000What's the unit?
it's in milliseconds, I would like to rename it to MTK_FD_HW_TIMEOUT_MSEC
quoted
+ +enum face_angle { + MTK_FD_FACE_FRONT, + MTK_FD_FACE_RIGHT_50, + MTK_FD_FACE_LEFT_50, + MTK_FD_FACE_RIGHT_90, + MTK_FD_FACE_LEFT_90, + MTK_FD_FACE_ANGLE_NUM, +};This enum seems to define values for the V4L2_CID_MTK_FD_DETECT_POSE control. Considering that this is an enumeration and the values are actually integers (-90, -50, 0, 50, 90), perhaps this should be an INTEGER_MENU control instead?
this ioctl let user select multiple face positions(combination of angles
and directions) to be detected. so I thought I am not able to use the
INTEGER_MENU for this purpose.
A bit-field as following should be used by user.
I consider adding it to uapi.
struct face_direction_def {
__u16 MTK_FD_FACE_DIR_0 : 1,
MTK_FD_FACE_DIR_30 : 1,
MTK_FD_FACE_DIR_60 : 1,
MTK_FD_FACE_DIR_90 : 1,
MTK_FD_FACE_DIR_120 : 1,
MTK_FD_FACE_DIR_150 : 1,
MTK_FD_FACE_DIR_180 : 1,
MTK_FD_FACE_DIR_210 : 1,
MTK_FD_FACE_DIR_240 : 1,
MTK_FD_FACE_DIR_270 : 1,
MTK_FD_FACE_DIR_300 : 1,
MTK_FD_FACE_DIR_330 : 1,
: 4;
};
User can also select some face directions of each face angle in one
ioctl, for example:
/*
* u16 face_directions[MTK_FD_FACE_ANGLE_NUM] = {0};
*
* face_directions[MTK_FD_FACE_FRONT] = 0x7; //angle:0, dir:0,30,60
* face_directions[MTK_FACE_RIGHT_50] = 0x2; //angle:50, dir:30
*
*/
quoted
+ +struct fd_buffer { + __u32 scp_addr; /* used by SCP */ + __u32 dma_addr; /* used by DMA HW */ +} __packed;
fd buffer is used for scp ipi
quoted
+ +struct fd_face_result { + char data[16]; +};
fd_face_result is used for user, so it should be moved to
include/uapi/linux.
In fact, it has bit-field definition for user, so I would like to define
it in include/uapi/linux as following:
struct fd_face_result {
__u64 face_idx : 12,
type : 1,
x0 : 10,
y0 : 10,
x1 : 10,
y1 : 10,
fcv1 : 11;
__u64 fcv2 : 7,
rip_dir : 4,
rop_dir : 3,
det_size : 5;
};
quoted
+ +struct fd_user_output { + struct fd_face_result results[MTK_FD_MAX_RESULT_NUM]; + __u16 number;Is this perhaps the number of results? If so, would num_results be a better name?
yes, fixed.
quoted
+};Since this struct is the meta buffer format, it is a part of the userspace interface and should be defined in a header under include/uapi/linux/.
Ok, I will create include/uapi/linux/mtk_fd_40.h which suppose to include structures that userspace will use. should the private IOCTLs be placed in it together?
quoted
+ +struct user_param { + u8 fd_speedup; + u8 fd_extra_model; + u8 scale_img_num; + u8 src_img_fmt; + __u16 scale_img_width[MTK_FD_SCALE_ARR_NUM]; + __u16 scale_img_height[MTK_FD_SCALE_ARR_NUM]; + __u16 face_directions[MTK_FD_FACE_ANGLE_NUM];Is this a user-facing definition or an interface between the kernel driver and firmware? If the latter, the __ types shouldn't be used.
It's the later, I'll fix it.
quoted
+} __packed; + +struct fd_init_param { + struct fd_buffer fd_manager; + __u32 rs_dma_addr;Ditto.
done.
quoted
+} __packed; + +struct fd_enq_param { + __u64 output_vaddr;Ditto.
done.
quoted
+ struct fd_buffer src_img[2]; + struct fd_buffer user_result; + struct user_param user_param; +} __packed; + +struct fd_ack_param { + __u32 ret_code; + __u32 ret_msg;Ditto.
done.
quoted
+} __packed;[snip]quoted
+/* */Was there supposed to be a comment here? :)
Ooops, no, it should be removed.
quoted
+static int mtk_fd_hw_alloc_rs_dma_addr(struct mtk_fd_dev *fd) +{ + struct device *dev = fd->dev; + void *va; + dma_addr_t dma_handle; + + va = dma_alloc_coherent(dev, MTK_FD_RS_BUF_SIZE, &dma_handle, + GFP_KERNEL); + if (!va) { + dev_err(dev, "dma_alloc null va\n");No need to print errors for memory allocation failures, because one will be printed automatically.
Ok, log removed.
[snip]quoted
+static int mtk_fd_hw_connect(struct mtk_fd_dev *fd) +{ + int ret; + + ret = rproc_boot(fd->rproc_handle); +nit: Unnecessary blank line.
fixed.
[snip]quoted
+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); + unsigned int size[2]; + unsigned int plane; + + switch (vq->type) { + case V4L2_BUF_TYPE_META_CAPTURE: + size[0] = ctx->dst_fmt.buffersize; + break; + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: + size[0] = ctx->src_fmt.plane_fmt[0].sizeimage; + if (*num_planes == 2) + size[1] = ctx->src_fmt.plane_fmt[1].sizeimage; + break; + }Is this code above needed? The code below sets sizes[] and it uses a for loop, without opencoded assignment for the second plane.
Looks like not really useful here,
it should check sizes and num_planes if num_plane not zero,
and for V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, it will at most have 2
planes, maybe no need for loop as well.
I will refine this function as following:
mtk_fd_vb2_queue_setup(...)
{
struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
if (*num_planes == 0) {
if (vq->type == V4L2_BUF_TYPE_META_CAPTURE) {
sizes[0] = ctx->dst_fmt.buffersize;
*num_planes = 1;
return 0;
} else if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
*num_planes = ctx->src_fmt.num_planes;
sizes[0] = ctx->src_fmt.plane_fmt[0].sizeimage;
if (*num_planes == 2)
sizes[1] = ctx->src_fmt.plane_fmt[1].sizeimage;
return 0;
}
return -EINVAL;
}
/* If num_plane not zero, check the num_plane and sizes*/
if (vq->type == V4L2_BUF_TYPE_META_CAPTURE) {
if ((*num_planes == 1) &&
(sizes[0] <= ctx->dst_fmt.buffersize))
return 0;
else
return -EINVAL;
}
if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
if ((*num_planes == 1) &&
(sizes[0] <= ctx->src_fmt.plane_fmt[0].sizeimage))
return 0;
else if ((*num_planes == 2) &&
(sizes[0] <= ctx->src_fmt.plane_fmt[0].sizeimage) &&
(sizes[1] <= ctx->src_fmt.plane_fmt[1].sizeimage))
return 0;
else
return -EINVAL;
}
return 0;
}
quoted
+ + if (*num_planes > 2) + return -EINVAL; + if (*num_planes == 0) { + if (vq->type == V4L2_BUF_TYPE_META_CAPTURE) { + sizes[0] = ctx->dst_fmt.buffersize; + *num_planes = 1; + return 0; + } + + *num_planes = ctx->src_fmt.num_planes; + for (plane = 0; plane < *num_planes; plane++) + sizes[plane] = ctx->src_fmt.plane_fmt[plane].sizeimage; + return 0; + } + + for (plane = 0; plane < *num_planes; plane++) { + if (sizes[plane] < size[plane]) + return -EINVAL; + } + return 0; +}[snip]quoted
+static int mtk_fd_enum_fmt_out_mp(struct file *file, void *fh, + struct v4l2_fmtdesc *f) +{ + if (f->index >= NUM_FORMATS)Please use ARRAY_SIZE().
fixed.
quoted
+ return -EINVAL; + + f->pixelformat = mtk_fd_img_fmts[f->index].pixelformat; + return 0; +} + +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;Could some of the code above be replaced with v4l2_fill_pixfmt_mp()?
I would like to refine as following
mtk_fd_fill_pixfmt_mp(...){
v4l2_fill_pixfmt_mp(dfmt, sfmt->pixelformat, dfmt->width,
dfmt->height);
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);
}
quoted
+} + +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++) {Please use ARRAY_SIZE rather than a custom macro.
fixed.
quoted
+ dev_fmt = &mtk_fd_img_fmts[i]; + if (dev_fmt->pixelformat == format) + return dev_fmt; + } + + return NULL; +} + +static int mtk_fd_try_fmt_out_mp(struct file *file, + void *fh, + struct v4l2_format *f) +{ + 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) + fmt = &mtk_fd_img_fmts[0]; /* Get default img fmt */nit: Please move the comment to a separate line and add braces.
fixed.
quoted
+ + mtk_fd_fill_pixfmt_mp(pix_mp, fmt); + return 0; +}[snip]quoted
+static unsigned int get_fd_img_fmt(unsigned int fourcc) +{ + switch (fourcc) { + case V4L2_PIX_FMT_VYUY: + return MTK_FD_HW_FMT_VYUY; + case V4L2_PIX_FMT_YUYV: + return MTK_FD_HW_FMT_YUYV; + case V4L2_PIX_FMT_YVYU: + return MTK_FD_HW_FMT_YVYU; + case V4L2_PIX_FMT_UYVY: + return MTK_FD_HW_FMT_UYVY; + case V4L2_PIX_FMT_NV16M: + return MTK_FD_HW_FMT_YUV_2P; + case V4L2_PIX_FMT_NV61M: + return MTK_FD_HW_FMT_YVU_2P; + default: + return MTK_FD_HW_FMT_UNKNOWN;If we want to be paranoid, we can just add a WARN here and return 0. Shouldn't be a need to define an unknown format.
Ok, I will refine as following: default: dev_warn(dev, "%s: Unsupported V4L2_PIX_FMT\n", __func__); return 0;
[snip]quoted
+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_DOWN_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_DOWN_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_SCALE_IMG_NUM); + if (ctrl) + user_param->scale_img_num = ctrl->val; +nit: Either separate the code dealing with all controls from each other, or none.
Fixed.
quoted
+ ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_DETECT_POSE); + if (ctrl) + for (i = 0; i < ctrl->elems; i++) + user_param->face_directions[i] = ctrl->p_new.p_u16[i]; + ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_DETECT_SPEED); + if (ctrl) + user_param->fd_speedup = ctrl->val; + ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_DETECTION_MODEL); + if (ctrl) + user_param->fd_extra_model = ctrl->val; +} + +static void mtk_fd_device_run(void *priv) +{ + struct mtk_fd_ctx *ctx = priv; + struct mtk_fd_dev *fd = ctx->fd_dev; + struct vb2_v4l2_buffer *src_buf, *dst_buf; + struct fd_enq_param fd_param; + void *plane_vaddr; + + src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx); + dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); + + fd_param.src_img[0].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); + plane_vaddr = vb2_plane_vaddr(&dst_buf->vb2_buf, 0); + fd_param.output_vaddr = (u64)(unsigned long)plane_vaddr;Why is the vaddr needed here? Specfically, it could pose a security problem if CPU virtual addresses are exchanged with firmware.
This should be removed... SCP side as well.
quoted
+ fd_param.user_param.src_img_fmt = + get_fd_img_fmt(ctx->src_fmt.pixelformat); + if (ctx->src_fmt.num_planes == 2) + fd_param.src_img[1].dma_addr = + vb2_dma_contig_plane_dma_addr(&src_buf->vb2_buf, 1);nit: Could this be moved above, to be just below src_img[0] initialization, for readability reasons?
Ok, this function will be refined as
static void mtk_fd_device_run(void *priv)
{
struct mtk_fd_ctx *ctx = priv;
struct mtk_fd_dev *fd = ctx->fd_dev;
struct vb2_v4l2_buffer *src_buf, *dst_buf;
struct fd_enq_param fd_param;
src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
fd_param.src_img[0].dma_addr =
vb2_dma_contig_plane_dma_addr(&src_buf->vb2_buf, 0);
if (ctx->src_fmt.num_planes == 2)
fd_param.src_img[1].dma_addr =
vb2_dma_contig_plane_dma_addr(&src_buf->vb2_buf, 1);
fd_param.user_result.dma_addr =
vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
fd_param.user_param.src_img_fmt =
get_fd_img_fmt(fd->dev, ctx->src_fmt.pixelformat);
mtk_fd_fill_user_param(&fd_param.user_param, &ctx->hdl);
/* Complete request controls if any */
v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req, &ctx->hdl);
fd->output = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
mtk_fd_hw_job_exec(fd, &fd_param);
}quoted
+ mtk_fd_fill_user_param(&fd_param.user_param, &ctx->hdl); + + /* Complete request controls if any */ + v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req, &ctx->hdl); + + fd->output = plane_vaddr; + mtk_fd_hw_job_exec(fd, &fd_param); +} + +static struct v4l2_m2m_ops fd_m2m_ops = { + .device_run = mtk_fd_device_run, +}; + +static const struct media_device_ops fd_m2m_media_ops = { + .req_validate = vb2_request_validate, + .req_queue = v4l2_m2m_request_queue, +}; + +static int mtk_fd_video_device_register(struct mtk_fd_dev *fd) +{ + struct video_device *vfd = &fd->vfd; + struct v4l2_m2m_dev *m2m_dev = fd->m2m_dev; + struct device *dev = fd->dev; + int ret; + + vfd->fops = &fd_video_fops; + vfd->release = video_device_release; + vfd->lock = &fd->vfd_lock; + vfd->v4l2_dev = &fd->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, dev_driver_string(dev), sizeof(vfd->name)); + + video_set_drvdata(vfd, fd); + + ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0); + if (ret) { + dev_err(dev, "Failed to register video device\n"); + goto err_free_dev; + } + + ret = v4l2_m2m_register_media_controller(m2m_dev, vfd, + MEDIA_ENT_F_PROC_VIDEO_STATISTICS); + 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) +{ + struct media_device *mdev = &fd->mdev; + struct device *dev = fd->dev; + int ret; + + ret = v4l2_device_register(dev, &fd->v4l2_dev); + if (ret) { + dev_err(dev, "Failed to register v4l2 device\n"); + return ret; + } + + fd->m2m_dev = v4l2_m2m_init(&fd_m2m_ops); + if (IS_ERR(fd->m2m_dev)) { + dev_err(dev, "Failed to init mem2mem device\n"); + ret = PTR_ERR(fd->m2m_dev); + goto err_unreg_v4l2_dev; + } + + mdev->dev = dev; + strscpy(mdev->model, dev_driver_string(dev), sizeof(mdev->model)); + snprintf(mdev->bus_info, sizeof(mdev->bus_info), + "platform:%s", dev_name(dev)); + media_device_init(mdev); + mdev->ops = &fd_m2m_media_ops; + fd->v4l2_dev.mdev = mdev; + + ret = mtk_fd_video_device_register(fd); + if (ret) { + dev_err(dev, "Failed to register video device\n"); + goto err_cleanup_mdev; + } + + ret = media_device_register(mdev); + if (ret) { + dev_err(dev, "Failed to register mem2mem media device\n"); + goto err_unreg_vdev; + } + + return 0; + +err_unreg_vdev: + v4l2_m2m_unregister_media_controller(fd->m2m_dev); + video_unregister_device(&fd->vfd); + video_device_release(&fd->vfd);This is inconsistent with the registration, which registers all of these in a function. Perhaps it would be worth to move this cleanup to a mtk_fd_video_device_unregister() function?
Fixed.
quoted
+err_cleanup_mdev: + media_device_cleanup(mdev); + v4l2_m2m_release(fd->m2m_dev); +err_unreg_v4l2_dev: + v4l2_device_unregister(&fd->v4l2_dev); + return ret; +} + +static void mtk_fd_dev_v4l2_release(struct mtk_fd_dev *fd) +{ + v4l2_m2m_unregister_media_controller(fd->m2m_dev); + video_unregister_device(&fd->vfd); + video_device_release(&fd->vfd); + media_device_cleanup(&fd->mdev); + v4l2_m2m_release(fd->m2m_dev); + v4l2_device_unregister(&fd->v4l2_dev); +} + +static irqreturn_t mtk_fd_irq(int irq, void *data) +{ + struct mtk_fd_dev *fd = (struct mtk_fd_dev *)data; + + /* must read this register otherwise HW will keep sending irq */ + readl(fd->fd_base + MTK_FD_REG_OFFSET_INT_VAL); + fd->output->number = readl(fd->fd_base + MTK_FD_REG_OFFSET_RESULT); + dev_dbg(fd->dev, "mtk_fd_face_num:%d\n", fd->output->number); + + mtk_fd_hw_done(fd, VB2_BUF_STATE_DONE); + return IRQ_HANDLED; +} + +static int mtk_fd_hw_get_scp_mem(struct mtk_fd_dev *fd) +{ + struct device *dev = fd->dev; + dma_addr_t addr; + void *ptr; + u32 ret; + + /* + * Allocate coherent reserved memory for SCP firmware usage. + * The size of SCP composer's memory is fixed to 0x100000 + * for the requirement of firmware. + */ + ptr = dma_alloc_coherent(&fd->scp_pdev->dev, + MTK_FD_HW_WORK_BUF_SIZE, &addr, GFP_KERNEL); + if (!ptr) + return -ENOMEM; + + fd->scp_mem.scp_addr = addr; + fd->scp_mem_virt_addr = ptr; + dev_info(dev, "scp addr:%pad va:%pK\n", &addr, ptr);These addresses are nothing a user should be concerned about, so please don't use the _info level here. If you think this is an important debugging information, please make it dev_dbg(). Otherwise, please just remove.
Removed.
quoted
+ + /* + * This reserved memory is also be used by FD HW. + * Need to get iova address for FD DMA. + */ + addr = dma_map_resource(dev, addr, MTK_FD_HW_WORK_BUF_SIZE, + DMA_TO_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);Note that the second argument to dma_map_resource() is phys_addr_t, but the code above passes a DMA address. This works only by luck, because both physical and SCP DMA address space have the same addresses. To be fully correct, dma_map_single() needs to be used, with the kernel virtual address passed to it, but currently it doesn't handle the memory from a reserved pool. Thus, we can only keep the hack as is, but please add a comment explaining it, e.g. /* * FIXME: Assume SCP DMA and physical addresses are the same until * dma_map_single() is fixed to handle reserved memory allocations. */
Ok, I've insert it above the comments there. as following: /* * FIXME: Assume SCP DMA and physical addresses are the same until * dma_map_single() is fixed to handle reserved memory allocations. * * This reserved memory will also be used by FD HW. * Need to get iova address for FD DMA. */
quoted
+ if (dma_mapping_error(dev, addr)) { + dev_err(dev, "Failed to map scp iova\n"); + ret = -ENOMEM; + goto fail_free_mem; + } + fd->scp_mem.dma_addr = addr; + dev_info(dev, "scp iova addr:%pad\n", &addr);Ditto.
fixed.
Best regards, Tomasz
Thanks and best regards, Jerry _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel