Hi Tomasz,

I Appreciate your review comments, here's the reply.

On Mon, 2020-05-25 at 14:24 +0200, Tomasz Figa wrote:

r

On Fri, May 22, 2020 at 4:11 PM Jerry-ch Chen
[off-list ref] wrote:

quoted

Hi Tomasz,

On Thu, 2020-05-21 at 18:28 +0000, Tomasz Figa wrote:

quoted

Hi Jerry,

On Wed, Dec 04, 2019 at 08:47:32PM +0800, Jerry-ch Chen wrote:

[snip]

quoted

quoted

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.

Ah, okay, I thought there is only one selection possible.

quoted

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;
};

Note that bit fields are not recommended in UAPI because of not being
well specified by the language. Instead bits should be defined using
macros with explicit masks or sometimes enums.

Ok, I'll define them in macro with masks.

quoted

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
 *
 */

Makes sense, thanks.

quoted

quoted

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

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;
};

Indeed this should be defined, but as per my comment above, please
avoid using the bitfield construct and define shifts and masks
instead.

Ok, I'll fix it.

quoted

quoted

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

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?

Sorry, what private IOCTLs are you referring to? If you mean private
control IDs, then yes, they should go to that header.

yes, the IDs, sorry for the wrong expression.

[snip]

quoted

quoted

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.

Loops generally make the code cleaner and there might be some desire
to add support for more formats in the future, e.g. in case a next
generation of the hardware shows up.

Ok, got it.

quoted

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;

nit: The typical convention is to check for problems and return the
error code earlier, with the success handled at the end of the block.

Got it.

quoted

                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;

Wouldn't a loop eliminate the need to if/else if through the various
supported cases and duplicate the size checks?

quoted

                else
                        return -EINVAL;

        }
        return 0;
}

How about the following?

mtk_fd_vb2_queue_setup(...)
{
        struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);

        if (vq->type == V4L2_BUF_TYPE_META_CAPTURE) {
                if (*num_planes == 0) {
                        *num_planes = 1;
                        sizes[0] = ctx->dst_fmt.buffersize;
                        return 0;
                }

                if (*num_planes != 1 || sizes[0] < ctx->dst_fmt.buffersize)
                                return -EINVAL;

                return 0;
        }

        /* V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE */
        if (*num_planes == 0) {
                        *num_planes = ctx->src_fmt.num_planes;
                        for (i = 0; i < ctx->src_fmt.num_planes; ++i)
                                sizes[i] = ctx->src_fmt.plane_fmt[i].sizeimage;
                        return 0;
        }

        if (*num_planes < ctx->src_fmt.num_planes)
                return -EINVAL;

        for (i = 0; i < ctx->src_fmt.num_planes; ++i)
                if (sizes[i] < ctx->src_fmt.plane_fmt[i].sizeimage)
                        return -EINVAL;

        return 0;
}

Note that it fully separates the code dealing with each queue and thus
improves the readability.

In this case, it could actually be beneficial to split the vb2_ops
implementation into one that deals only with video_output_mplane and
one only with meta_capture. This would allow eliminating the special
casing based on vq->type and thus further simplify the code. Not sure
if it applies to the other vb2 callbacks, though.

Got it, thanks for the comments.

[snip]

quoted

quoted

quoted

+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;

num_planes should be already filled in by the helper. That actually
raises a question on whether there is any need to have sfmt passed to
this function at all. Perhaps all we need is the value of pixelformat?

Yes, I'll replace sfmt with u32 pixfmt.

quoted

        dfmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
        dfmt->quantization = V4L2_QUANTIZATION_DEFAULT;
        dfmt->xfer_func =
                V4L2_MAP_XFER_FUNC_DEFAULT(dfmt->colorspace);
}

Isn't still a need to clamp() width and height to min/max, though?

Yes, I'll add them back.

This function will be refined as :

static void mtk_fd_fill_pixfmt_mp(struct v4l2_pix_format_mplane *dfmt,
                                  u32 pixfmt)
{
        v4l2_fill_pixfmt_mp(dfmt, pixfmt, dfmt->width, dfmt->height);

        dfmt->field = V4L2_FIELD_NONE;
        dfmt->colorspace = V4L2_COLORSPACE_BT2020;
        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);
}

[snip]

quoted

quoted

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);

How about making this a loop in terms of ctx->src_fmt.num_planes?

yes, it will be refined as following, I use the src_vb2_buf to reduce
the length for fitting 80 columns

src_vb2_buf = &src_buf->vb2_buf;
dst_vb2_buf = &dst_buf->vb2_buf;

for (i = 0; i < ctx->src_fmt.num_planes; i++)
	fd_param.src_img[i].dma_addr =
		vb2_dma_contig_plane_dma_addr(src_vb2_buf,i);

fd_param.user_result.dma_addr =
	vb2_dma_contig_plane_dma_addr(dst_vb2_buf, 0);

quoted

        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);
}

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