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

Tomasz Figa
2019-03-21T04:00:14Z

On Wed, Mar 13, 2019 at 3:54 PM Jungo Lin <jungo.lin@mediatek.com> wrote:
>
> On Tue, 2019-03-12 at 19:04 +0900, Tomasz Figa wrote:
[snip]
> > Instead of opencoding most of this function, one would normally call
> > mtk_cam_videoc_try_fmt() first to adjust the format struct and then
> > just update the driver state with the adjusted format.
> >
> > Also, similarly to VIDIOC_TRY_FMT, VIDIOC_SET_FMT doesn't fail unless
> > in the very specific cases, as described in
> > https://www.kernel.org/doc/html/latest/media/uapi/v4l/vidioc-g-fmt.html#return-value
> > .
> >
>
> Ok, below is our revised version of this function.
>
> int mtk_cam_videoc_s_fmt(struct file *file, void *fh,
> struct v4l2_format *f)
> {
> struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file);
> struct mtk_cam_dev *cam_dev = mtk_cam_m2m_to_dev(m2m2);
> struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
>
> /* Get the valid format*/
> mtk_cam_videoc_try_fmt(file, fh, f);
> /* Configure to video device */
> mtk_cam_ctx_fmt_set_img(&cam_dev->pdev->dev,
> &node->vdev_fmt.fmt.pix_mp,
> &f->fmt.pix_mp,
> node->queue_id);
>
> return 0;
> }
>

Looks almost good. We still need to signal the -EBUSY error condition
if an attempt to change the format is made while streaming is active.

[snip]
> > > +static int mtk_cam_videoc_s_meta_fmt(struct file *file,
> > > + void *fh, struct v4l2_format *f)
> > > +{
> > > + struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file);
> > > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2);
> > > + struct mtk_cam_ctx *dev_ctx = &isp_dev->ctx;
> > > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file);
> > > + int queue_id = mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node);
> > > +
> >
> > No need for this blank line.
> >
>
> We will fix this coding style in next patch.
>
> > > + int ret = 0;
> >
> > Please don't default-initialize without a good reason.
> >
>
> Ok, fix in next patch.
>
> > > +
> > > + if (f->type != node->vbq.type)
> > > + return -EINVAL;
> >
> > Ditto.
> >
>
> Ok, fix in next patch.
>
> > > +
> > > + ret = mtk_cam_ctx_format_load_default_fmt(&dev_ctx->queue[queue_id], f);
> > > +
> >
> > No need for this blank line.
> >
>
> Ok, fix in next patch.
>
> > > + if (!ret) {
> > > + node->vdev_fmt.fmt.meta = f->fmt.meta;
> > > + dev_ctx->queue[queue_id].fmt.meta = node->vdev_fmt.fmt.meta;
> > > + } else {
> > > + dev_warn(&isp_dev->pdev->dev,
> > > + "s_meta_fm failed, format not support\n");
> >
> > No need for this warning.
> >
>
> Ok, fix in next patch.
>
> > > + }
> > > +
> > > + return ret;
> > > +}
> >
> > Actually, why do we even need to do all the things? Do we support
> > multiple different meta formats on the same video node? If not, we can
> > just have all the TRY_FMT/S_FMT/G_FMT return the hardcoded format.
> >
>
> Ok, it is a good idea. We will return the hardcode format for meta video
> devices.
> Below is the revision version.
>
> int mtk_cam_meta_enum_format(struct file *file,
> void *fh, struct v4l2_fmtdesc *f)
> {
> struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
>
> f->pixelformat = node->vdev_fmt.fmt.meta.dataformat;
>
> return 0;
> }

Need to error out if f->index > 0. Also need to initialize the other
output fields - flags and description.

[snip]
> > > +static u32 mtk_cam_node_get_v4l2_cap(struct mtk_cam_ctx_queue *node_ctx)
> > > +{
> > > + u32 cap = 0;
> > > +
> > > + if (node_ctx->desc.capture)
> > > + if (node_ctx->desc.image)
> > > + cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE;
> > > + else
> > > + cap = V4L2_CAP_META_CAPTURE;
> > > + else
> > > + if (node_ctx->desc.image)
> > > + cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE;
> > > + else
> > > + cap = V4L2_CAP_META_OUTPUT;
> > > +
> > > + return cap;
> > > +}
> >
> > Why not just have this defined statically as node_ctx->desc.cap?
> >
>
> Ok, it is refactoring done.
>
> > > +
> > > +static u32 mtk_cam_node_get_format_type(struct mtk_cam_ctx_queue *node_ctx)
> > > +{
> > > + u32 type;
> > > +
> > > + if (node_ctx->desc.capture)
> > > + if (node_ctx->desc.image)
> > > + type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> > > + else
> > > + type = V4L2_BUF_TYPE_META_CAPTURE;
> > > + else
> > > + if (node_ctx->desc.image)
> > > + type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> > > + else
> > > + type = V4L2_BUF_TYPE_META_OUTPUT;
> > > +
> > > + return type;
> > > +}
> >
> > Why not just have this defined statically as node_ctx->desc.buf_type?
> >
>
> Same as above.
>
> > > +
> > > +static const struct v4l2_ioctl_ops *mtk_cam_node_get_ioctl_ops
> > > + (struct mtk_cam_ctx_queue *node_ctx)
> > > +{
> > > + const struct v4l2_ioctl_ops *ops = NULL;
> > > +
> > > + if (node_ctx->desc.image)
> > > + ops = &mtk_cam_v4l2_ioctl_ops;
> > > + else
> > > + ops = &mtk_cam_v4l2_meta_ioctl_ops;
> > > + return ops;
> > > +}
> >
> > It's also preferable to just put this inside some structure rather
> > than have getter functions. (node_ctx->desc.ioctl_ops?)
> >
>
> Same as above.
> Below is the new version for struct mtk_cam_ctx_queue_desc
>
> /*
> * struct mtk_cam_ctx_queue_desc - queue attributes
> * setup by device context owner
> * @id: id of the context queue
> * @name: media entity name
> * @cap: mapped to V4L2 capabilities
> * @buf_type: mapped to V4L2 buffer type
> * @capture: true for capture queue (device to user)
> * false for output queue (from user to device)
> * @image: true for image, false for meta data
> * @smem_alloc: Using the cam_smem_drv as alloc ctx or not
> * @dma_port: the dma port associated to the buffer
> * @fmts: supported format
> * @num_fmts: the number of supported formats
> * @default_fmt_idx: default format of this queue
> * @max_buf_count: maximum V4L2 buffer count
> * @max_buf_count: mapped to v4l2_ioctl_ops
> */
> struct mtk_cam_ctx_queue_desc {
> u8 id;
> char *name;
> u32 cap;
> u32 buf_type;
> u32 dma_port;
> u32 smem_alloc:1;
> u8 capture:1;
> u8 image:1;
> u8 num_fmts;
> u8 default_fmt_idx;
> u8 max_buf_count;
> const struct v4l2_ioctl_ops *ioctl_ops;
> struct v4l2_format *fmts;
> };

SGTM +/- the missing kerneldoc for the new fields.

[snip]
> > Sorry, ran out of time for today. Fourth part will come. :)
> >
> > Best regards,
> > Tomasz
> >
>
> Appreciate your support and hard working on this review.
> We will look forward your part 4 review.

Thanks for replying to all the comments, it's very helpful.

As I mentioned in another reply, I'm going to be busy for the next few
days, so I'd suggest addressing the existing comments, fixing any
v4l2-compliance issues and also checking if any changes could be
applied to the other drivers (DIP, FD, Seninf, MDP) too and then
sending RFC V1.

Best regards,
Tomasz