From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.8 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING, SPF_PASS,UNPARSEABLE_RELAY,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 37295C43381 for ; Wed, 13 Mar 2019 10:37:02 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 024452171F for ; Wed, 13 Mar 2019 10:37:02 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="E0j4Xxs3"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="dldBmEY2" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 024452171F Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=mediatek.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Date:To:From:Subject:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=FyzwEj1mbLkA+UAo62CFCqcDbmT3J9fvylWsxYMXxY4=; b=E0j4Xxs3qveGsl QeL94EpeDTIAvNdSLSoNGqQPYp5fE/k/Fb7TH/4sRyTJytelcuvFZqkH81Fu6DhpPF4Buf8finkZz lGDq1v78vB3yWJuKuoL9xiH+mgwb78QR02Xci+3GasKfHtTUKOb19LMh6CbVrmM4dLUugNagjdRFQ SoA8v2HvlrP4k8d9UFevR7M8JqMj7eaR3f4HnFxynTQAjcLKaTlGpAWN5niybqj6TpAjFFmhT7iaf 0SMqE9VX9E4Sv7HIIPpzXITUJC3hb1+5T8bGjVegetPkYwrkHMLfNJI7t4La3YYh6GEg3Dne6Vcmr LVDPjfIFgZWQiMXDxvdw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1h41Fo-0000Xa-Q0; Wed, 13 Mar 2019 10:36:56 +0000 Received: from merlin.infradead.org ([2001:8b0:10b:1231::1]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1h41Av-0001eR-In; Wed, 13 Mar 2019 10:31:54 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=merlin.20170209; h=MIME-Version:Content-Transfer-Encoding: Content-Type:References:In-Reply-To:Date:CC:To:From:Subject:Message-ID:Sender :Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help: List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=rmwAeLkD2j6r44NUww21CDNkgN7OQ4VLNVfr7lP28lE=; b=dldBmEY2ZK6mMOGhMChql1mZOG JvRbTSrepkJ9X1KO96TwHH83s56ABOUQXz9Itbjh2WfiQqgzHP5kYh3+pWhrokFKVfoy15CKc5NUe 1IurtJkVjHecPuxbDFuvCeqexvVSkzVIzIUpPs5fBQXjr0CvqMPrdERA3JqbK0Ow3BeyyXJcStqza qwTweBQ/81kXv2fJuvht540RhuPeNErwl0bdFdaC7uBztK3jYq0LliaJnExvKV95mo2SwyvPvxF+z ta56hAkX31qtfnSRTu/6l7mO2nxkc++RPYGcnk0dIUhBfMueOfgWWjF37i9VirrmAOViXMuR2ut9u Uvjvn+VQ==; Received: from mailgw02.mediatek.com ([216.200.240.185]) by merlin.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1h3xql-000649-Bh; Wed, 13 Mar 2019 06:58:53 +0000 X-UUID: 510380aae475449fab42501b44e77628-20190312 X-UUID: 510380aae475449fab42501b44e77628-20190312 Received: from mtkcas67.mediatek.inc [(172.29.193.45)] by mailgw02.mediatek.com (envelope-from ) (musrelay.mediatek.com ESMTP with TLS) with ESMTP id 4111386; Tue, 12 Mar 2019 22:54:14 -0800 Received: from mtkmbs03n2.mediatek.inc (172.21.101.182) by MTKMBS62N2.mediatek.inc (172.29.193.42) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Tue, 12 Mar 2019 23:54:12 -0700 Received: from mtkcas08.mediatek.inc (172.21.101.126) by mtkmbs03n2.mediatek.inc (172.21.101.182) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Wed, 13 Mar 2019 14:54:05 +0800 Received: from [172.21.84.99] (172.21.84.99) by mtkcas08.mediatek.inc (172.21.101.73) with Microsoft SMTP Server id 15.0.1395.4 via Frontend Transport; Wed, 13 Mar 2019 14:54:04 +0800 Message-ID: <1552460044.13953.114.camel@mtksdccf07> Subject: Re: [RFC PATCH V0 7/7] [media] platform: mtk-isp: Add Mediatek ISP Pass 1 driver From: Jungo Lin To: Tomasz Figa Date: Wed, 13 Mar 2019 14:54:04 +0800 In-Reply-To: References: <1549348966-14451-1-git-send-email-frederic.chen@mediatek.com> <1549348966-14451-8-git-send-email-frederic.chen@mediatek.com> X-Mailer: Evolution 3.2.3-0ubuntu6 MIME-Version: 1.0 X-TM-SNTS-SMTP: FAB3ACBE3DFA27249E7C98C8239F4C4CC54D8E00FC2453C7541DAA119D9771582000:8 X-MTK: N X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190313_025851_637498_38E5AD0D X-CRM114-Status: GOOD ( 45.20 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Sean Cheng =?UTF-8?Q?=28=E9=84=AD=E6=98=87=E5=BC=98=29?= , Laurent Pinchart , Rynn Wu =?UTF-8?Q?=28=E5=90=B3=E8=82=B2=E6=81=A9=29?= , srv_heupstream@mediatek.com, Mauro Carvalho Chehab , Holmes Chiou =?UTF-8?Q?=28=E9=82=B1=E6=8C=BA=29?= , Jerry-ch Chen , Sj Huang , yuzhao@chromium.org, Christie Yu =?UTF-8?Q?=28=E6=B8=B8=E9=9B=85=E6=83=A0=29?= , zwisler@chromium.org, Matthias Brugger , linux-mediatek@lists.infradead.org, Frederic Chen , Hans Verkuil , "list@263.net:IOMMU DRIVERS , Joerg Roedel , " , Linux Media Mailing List Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, 2019-03-12 at 19:04 +0900, Tomasz Figa wrote: > Hi Frederic, Jungo, > > Please see more comments inline. > Hi Tomasz: Thanks for your part 3 comments. Below is our feedback. > > +static int mtk_cam_vb2_queue_setup(struct vb2_queue *vq, > > + unsigned int *num_buffers, > > + unsigned int *num_planes, > > + unsigned int sizes[], > > + struct device *alloc_devs[]) > > +{ > > + struct mtk_cam_mem2mem2_device *m2m2 = vb2_get_drv_priv(vq); > > + struct mtk_cam_dev_video_device *node = > > + mtk_cam_vbq_to_isp_node(vq); > > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > > + struct device *dev = &isp_dev->pdev->dev; > > + void *buf_alloc_ctx = NULL; > > Don't initialize by default, if not strictly necessary. > Ok, fixed in next patch. > > + int queue_id = mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > > + > > + /* Get V4L2 format with the following method */ > > + const struct v4l2_format *fmt = &node->vdev_fmt; > > + > > + *num_planes = 1; > > This doesn't handle VIDIOC_CREATE_BUFS, which triggers a > .queue_setup() call with *num_planes > 0 and sizes[] already > initialized. The driver needs to validate that the sizes and number of > planes are valid in that case and return -EINVAL otherwise. Perhaps > you should try running v4l2-compliance > (https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance) on > this driver, which should catch issues like this. > Ok, we will add code check logic for this user case. The function will be similar to below: https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/vivid/vivid-vid-cap.c#L87 https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/ipu3/ipu3-v4l2.c#L388 > > + *num_buffers = clamp_val(*num_buffers, 1, VB2_MAX_FRAME); > > + > > + if (isp_dev->ctx.queue[queue_id].desc.smem_alloc) { > > + buf_alloc_ctx = isp_dev->ctx.smem_vb2_alloc_ctx; > > + dev_dbg(dev, "Select smem_vb2_alloc_ctx(%llx)\n", > > + (unsigned long long)buf_alloc_ctx); > > Use %p for printing pointers. > For security reason, we will change to use "%pK" for printing kernel pointers. > > + } else { > > + buf_alloc_ctx = isp_dev->ctx.default_vb2_alloc_ctx; > > + dev_dbg(dev, "Select default_vb2_alloc_ctx(%llx)\n", > > + (unsigned long long)buf_alloc_ctx); > > + } > > + > > + vq->dma_attrs |= DMA_ATTR_NON_CONSISTENT; > > + dev_dbg(dev, "queue(%d): cached mmap enabled\n", queue_id); > > This isn't supported in upstream. (By the way, neither it is in Chrome > OS 4.19 kernel. If we really need cached mmap for some reason, we > should propose a proper solution upstream. I'd still first investigate > why write-combine mmap is slow for operations that should be simple > one-time writes or reads.) > Ok, we will remove this in upstream version and follow your suggestion to find out better solution. > > + > > + if (vq->type == V4L2_BUF_TYPE_META_CAPTURE || > > + vq->type == V4L2_BUF_TYPE_META_OUTPUT) > > + sizes[0] = fmt->fmt.meta.buffersize; > > + else > > + sizes[0] = fmt->fmt.pix_mp.plane_fmt[0].sizeimage; > > + > > + alloc_devs[0] = (struct device *)buf_alloc_ctx; > > Please don't add random type casts. If the compiler gives a type > error, that normally means that there is something wrong elsewhere in > the code (i.e. the type of buf_alloc_ctx variable is wrong here - it > should be struct device *) and casting just masks the original > problem. > Ok, we will fix this coding defect. > > + > > + dev_dbg(dev, "queue(%d):type(%d),size(%d),alloc_ctx(%llx)\n", > > + queue_id, vq->type, sizes[0], > > + (unsigned long long)buf_alloc_ctx); > > + > > + /* Initialize buffer queue */ > > + INIT_LIST_HEAD(&node->buffers); > > This is incorrect. .queue_setup() can be also called on > VIDIOC_CREATE_BUFS, which must preserve the other buffers in the > queue. > In our new version, we have removed this for request-API new design. > > + > > + return 0; > > +} > [snip] > > +static int mtk_cam_vb2_start_streaming(struct vb2_queue *vq, unsigned int count) > > +{ > > + struct mtk_cam_mem2mem2_device *m2m2 = vb2_get_drv_priv(vq); > > + struct mtk_cam_dev_video_device *node = > > + mtk_cam_vbq_to_isp_node(vq); > > + int r; > > nit: "ret" is more common and already used by some other functions in > this patch. > We will rename this variable in next patch and try to unify the variable naming in our driver. > > + > > + if (m2m2->streaming) { > > + r = -EBUSY; > > + goto fail_return_bufs; > > + } > > We shouldn't need to check this ourselves. It's not possible to have > this call on an already streaming vb2 queue. Since we start streaming > the m2m2 subdev only when all video nodes start streaming, this should > never happen. > Ok, we will remove this redundant checking in next patch. > > + > > + if (!node->enabled) { > > + pr_err("Node (%ld) is not enable\n", node - m2m2->nodes); > > + r = -EINVAL; > > + goto fail_return_bufs; > > + } > > + > > + r = media_pipeline_start(&node->vdev.entity, &m2m2->pipeline); > > + if (r < 0) { > > + pr_err("Node (%ld) media_pipeline_start failed\n", > > + node - m2m2->nodes); > > + goto fail_return_bufs; > > + } > > + > > + if (!mtk_cam_all_nodes_streaming(m2m2, node)) > > + return 0; > > + > > + /* Start streaming of the whole pipeline now */ > > + > > + r = v4l2_subdev_call(&m2m2->subdev, video, s_stream, 1); > > + if (r < 0) { > > + pr_err("Node (%ld) v4l2_subdev_call s_stream failed\n", > > + node - m2m2->nodes); > > + goto fail_stop_pipeline; > > + } > > + return 0; > > + > > +fail_stop_pipeline: > > + media_pipeline_stop(&node->vdev.entity); > > +fail_return_bufs: > > + mtk_cam_return_all_buffers(m2m2, node, VB2_BUF_STATE_QUEUED); > > + > > + return r; > > +} > > + > > +static void mtk_cam_vb2_stop_streaming(struct vb2_queue *vq) > > +{ > > + struct mtk_cam_mem2mem2_device *m2m2 = vb2_get_drv_priv(vq); > > + struct mtk_cam_dev_video_device *node = > > + mtk_cam_vbq_to_isp_node(vq); > > + int r; > > + > > + WARN_ON(!node->enabled); > > + > > + /* Was this the first node with streaming disabled? */ > > + if (mtk_cam_all_nodes_streaming(m2m2, node)) { > > + /* Yes, really stop streaming now */ > > + r = v4l2_subdev_call(&m2m2->subdev, video, s_stream, 0); > > + if (r) > > + dev_err(m2m2->dev, "failed to stop streaming\n"); > > + } > > + > > + mtk_cam_return_all_buffers(m2m2, node, VB2_BUF_STATE_ERROR); > > + media_pipeline_stop(&node->vdev.entity); > > +} > > + > > +static int mtk_cam_videoc_querycap(struct file *file, void *fh, > > + struct v4l2_capability *cap) > > +{ > > + struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file); > > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > > + int queue_id = > > + mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > > + struct mtk_cam_ctx_queue *node_ctx = &isp_dev->ctx.queue[queue_id]; > > It feels like this could be just stored as node->ctx. > After refactoring this function, the node_ctx is removed. Moreover, we will follow your suggestion to store mtk_cam_ctx_queue pointer in mtk_cam_dev_video_device for better access. > > + > > + strlcpy(cap->driver, m2m2->name, sizeof(cap->driver)); > > + strlcpy(cap->card, m2m2->model, sizeof(cap->card)); > > + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", > > + node->name); > > + > > + cap->device_caps = > > + mtk_cam_node_get_v4l2_cap(node_ctx) | V4L2_CAP_STREAMING; > > + cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS; > > No need to set these 2 fields manually. They are filled in > automatically from struct video_device::device_caps. > We will remove this redundant code. > > + > > + return 0; > > +} > > + > > +/* Propagate forward always the format from the mtk_cam_dev subdev */ > > It doesn't seem to match what the function is doing, i.e. returning > the format structure of the node itself. I'd just drop this comment. > The code should be written in a self-explaining way anyway. > Ok, we will remove this comment to avoid misunderstanding. > > +static int mtk_cam_videoc_g_fmt(struct file *file, void *fh, > > + struct v4l2_format *f) > > +{ > > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > > + > > + f->fmt = node->vdev_fmt.fmt; > > + > > + return 0; > > +} > > + > > +static int mtk_cam_videoc_try_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); > > + int ret = 0; > > + > > + ret = mtk_cam_ctx_fmt_set_img(dev_ctx, queue_id, > > + &f->fmt.pix_mp, > > + &node->vdev_fmt.fmt.pix_mp); > > Doesn't this actually change the current node format? VIDIOC_TRY_FMT > must not have any side effects on current driver state. > This is a bug in our implementation. We will fix it in next patch. > > + > > + /* Simply set the format to the node context in the initial version */ > > + if (ret) { > > + pr_warn("Fmt(%d) not support for queue(%d), will load default fmt\n", > > + f->fmt.pix_mp.pixelformat, queue_id); > > No need for this warning. > Ok, we will remove this in next patch. > > + > > + ret = mtk_cam_ctx_format_load_default_fmt > > + (&dev_ctx->queue[queue_id], f); > > Wouldn't this also change the current node state? > > Also, something wrong with the number of spaces after "ret =". > Ditto. > > + } > > + > > + if (!ret) { > > + node->vdev_fmt.fmt.pix_mp = f->fmt.pix_mp; > > + dev_ctx->queue[queue_id].fmt.pix_mp = node->vdev_fmt.fmt.pix_mp; > > Ditto. > > > + } > > + > > + return ret; > > VIDIOC_TRY_FMT must not return an error unless for the cases described > in https://www.kernel.org/doc/html/latest/media/uapi/v4l/vidioc-g-fmt.html#return-value > . > Ok, we will try to follow the VIDIOC_TYPE_FMT API description of V4L2 manual. > > +} > > + > > +static 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 *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); > > + int ret = 0; > > + > > + ret = mtk_cam_ctx_fmt_set_img(dev_ctx, queue_id, > > + &f->fmt.pix_mp, > > + &node->vdev_fmt.fmt.pix_mp); > > + > > + /* Simply set the format to the node context in the initial version */ > > + if (!ret) > > + dev_ctx->queue[queue_id].fmt.pix_mp = node->vdev_fmt.fmt.pix_mp; > > + else > > + dev_warn(&isp_dev->pdev->dev, > > + "s_fmt, format not support\n"); > > No need for error messages for userspace errors. > Ok, we will remove this check. > > + > > + return ret; > > 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; } > > +} > > + > > +static int mtk_cam_enum_framesizes(struct file *filp, void *priv, > > + struct v4l2_frmsizeenum *sizes) > > +{ > > + struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(filp); > > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > > + struct mtk_cam_dev_video_device *node = > > + file_to_mtk_cam_node(filp); > > + int queue_id = mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > > + > > + if (sizes->index != 0) > > + return -EINVAL; > > + > > + if (queue_id == MTK_CAM_CTX_P1_MAIN_STREAM_OUT) { > > + sizes->type = V4L2_FRMSIZE_TYPE_CONTINUOUS; > > + sizes->stepwise.max_width = CAM_B_MAX_WIDTH; > > + sizes->stepwise.min_width = CAM_MIN_WIDTH; > > + sizes->stepwise.max_height = CAM_B_MAX_HEIGHT; > > + sizes->stepwise.min_height = CAM_MIN_HEIGHT; > > + sizes->stepwise.step_height = 1; > > + sizes->stepwise.step_width = 1; > > + } else if (queue_id == MTK_CAM_CTX_P1_PACKED_BIN_OUT) { > > + sizes->type = V4L2_FRMSIZE_TYPE_CONTINUOUS; > > + sizes->stepwise.max_width = RRZ_MAX_WIDTH; > > + sizes->stepwise.min_width = RRZ_MIN_WIDTH; > > + sizes->stepwise.max_height = RRZ_MAX_HEIGHT; > > + sizes->stepwise.min_height = RRZ_MIN_HEIGHT; > > + sizes->stepwise.step_height = 1; > > + sizes->stepwise.step_width = 1; > > + } > > + > > + return 0; > > +} > > + > > +static int mtk_cam_meta_enum_format(struct file *file, > > + void *fh, struct v4l2_fmtdesc *f) > > +{ > > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > > + > > + if (f->index > 0 || f->type != node->vbq.type) > > + return -EINVAL; > > f->type is already checked by the V4L2 core. See v4l_enum_fmt(). > We will remove the redundant checking in next patch. > > + > > + f->pixelformat = node->vdev_fmt.fmt.meta.dataformat; > > + > > + return 0; > > +} > > + > > +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; } const struct v4l2_ioctl_ops mtk_cam_v4l2_meta_cap_ioctl_ops = { .vidioc_querycap = mtk_cam_videoc_querycap, .vidioc_enum_fmt_meta_cap = mtk_cam_meta_enum_format, .vidioc_g_fmt_meta_cap = mtk_cam_videoc_g_meta_fmt, .vidioc_s_fmt_meta_cap = mtk_cam_videoc_g_meta_fmt, .vidioc_try_fmt_meta_cap = mtk_cam_videoc_g_meta_fmt, ... > > + > > +static int mtk_cam_videoc_g_meta_fmt(struct file *file, > > + void *fh, struct v4l2_format *f) > > +{ > > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > > + > > + if (f->type != node->vbq.type) > > + return -EINVAL; > > Not needed. > Ok, remove it in next patch. > > + > > + f->fmt = node->vdev_fmt.fmt; > > + > > + return 0; > > +} > > + > > +int mtk_cam_videoc_qbuf(struct file *file, void *priv, struct v4l2_buffer *p) > > +{ > > + struct video_device *vdev = video_devdata(file); > > + struct vb2_buffer *vb; > > + struct mtk_cam_dev_buffer *db; > > + int r = 0; > > + > > + /* check if vb2 queue is busy */ > > + if (vdev->queue->owner && > > + vdev->queue->owner != file->private_data) > > + return -EBUSY; > > This should be already handled by the core. > Ok, remove it in next patch. > > + > > + /* Keep the value of sequence in v4l2_buffer */ > > + /* in ctx buf since vb2_qbuf will set it to 0 */ > > + vb = vdev->queue->bufs[p->index]; > > Why do you need a sequence number for buffers on queue time? The field > is not specified to be set by the userspace and should be ignored by > the driver. The driver should rely on the Request API to match any > buffers together anyway. > It is our old design for frame-sync mechanism. However, we change to use "Request API" design and this function is removed in new version. > > + > > + if (vb) { > > + db = mtk_cam_vb2_buf_to_dev_buf(vb); > > + pr_debug("qbuf: p:%llx,vb:%llx, db:%llx\n", > > + (unsigned long long)p, > > + (unsigned long long)vb, > > + (unsigned long long)db); > > + db->ctx_buf.user_sequence = p->sequence; > > + } > > + > > Generally this driver shouldn't need to implement this callback > itself. Instead it can just use the vb2_ioctl_qbuf() helper. > Same as above. This function is removed in new version. > > + r = vb2_qbuf(vdev->queue, vdev->v4l2_dev->mdev, p); > > + > > + if (r) > > + pr_err("vb2_qbuf failed(err=%d): buf idx(%d)\n", > > + r, p->index); > > + > > + return r; > > +} > > +EXPORT_SYMBOL_GPL(mtk_cam_videoc_qbuf); > > + > > +/******************** function pointers ********************/ > > + > > +/* subdev internal operations */ > > +static const struct v4l2_subdev_internal_ops mtk_cam_subdev_internal_ops = { > > + .open = mtk_cam_subdev_open, > > + .close = mtk_cam_subdev_close, > > +}; > > + > > +static const struct v4l2_subdev_core_ops mtk_cam_subdev_core_ops = { > > + .subscribe_event = mtk_cam_subdev_subscribe_event, > > + .unsubscribe_event = mtk_cam_subdev_unsubscribe_event, > > +}; > > + > > +static const struct v4l2_subdev_video_ops mtk_cam_subdev_video_ops = { > > + .s_stream = mtk_cam_subdev_s_stream, > > +}; > > + > > +static const struct v4l2_subdev_ops mtk_cam_subdev_ops = { > > + .core = &mtk_cam_subdev_core_ops, > > + .video = &mtk_cam_subdev_video_ops, > > +}; > > + > > +static const struct media_entity_operations mtk_cam_media_ops = { > > + .link_setup = mtk_cam_link_setup, > > + .link_validate = v4l2_subdev_link_validate, > > +}; > > + > > +#ifdef CONFIG_MEDIATEK_MEDIA_REQUEST > > +static void mtk_cam_vb2_buf_request_complete(struct vb2_buffer *vb) > > +{ > > + struct mtk_cam_mem2mem2_device *m2m2 = vb2_get_drv_priv(vb->vb2_queue); > > + > > + v4l2_ctrl_request_complete(vb->req_obj.req, > > + m2m2->v4l2_dev->ctrl_handler); > > +} > > +#endif > > + > > +static const struct vb2_ops mtk_cam_vb2_ops = { > > + .buf_queue = mtk_cam_vb2_buf_queue, > > + .queue_setup = mtk_cam_vb2_queue_setup, > > + .start_streaming = mtk_cam_vb2_start_streaming, > > + .stop_streaming = mtk_cam_vb2_stop_streaming, > > + .wait_prepare = vb2_ops_wait_prepare, > > + .wait_finish = vb2_ops_wait_finish, > > +#ifdef CONFIG_MEDIATEK_MEDIA_REQUEST > > + .buf_request_complete = mtk_cam_vb2_buf_request_complete, > > +#endif > > +}; > > + > > +static const struct v4l2_file_operations mtk_cam_v4l2_fops = { > > + .unlocked_ioctl = video_ioctl2, > > + .open = v4l2_fh_open, > > + .release = vb2_fop_release, > > + .poll = vb2_fop_poll, > > + .mmap = vb2_fop_mmap, > > +#ifdef CONFIG_COMPAT > > + .compat_ioctl32 = v4l2_compat_ioctl32, > > +#endif > > +}; > > + > > +static const struct v4l2_ioctl_ops mtk_cam_v4l2_ioctl_ops = { > > + .vidioc_querycap = mtk_cam_videoc_querycap, > > + .vidioc_enum_framesizes = mtk_cam_enum_framesizes, > > + > > + .vidioc_g_fmt_vid_cap_mplane = mtk_cam_videoc_g_fmt, > > + .vidioc_s_fmt_vid_cap_mplane = mtk_cam_videoc_s_fmt, > > + .vidioc_try_fmt_vid_cap_mplane = mtk_cam_videoc_try_fmt, > > + > > + .vidioc_g_fmt_vid_out_mplane = mtk_cam_videoc_g_fmt, > > + .vidioc_s_fmt_vid_out_mplane = mtk_cam_videoc_s_fmt, > > + .vidioc_try_fmt_vid_out_mplane = mtk_cam_videoc_try_fmt, > > + > > + /* buffer queue management */ > > + .vidioc_reqbufs = vb2_ioctl_reqbufs, > > + .vidioc_create_bufs = vb2_ioctl_create_bufs, > > + .vidioc_prepare_buf = vb2_ioctl_prepare_buf, > > + .vidioc_querybuf = vb2_ioctl_querybuf, > > + .vidioc_qbuf = mtk_cam_videoc_qbuf, > > + .vidioc_dqbuf = vb2_ioctl_dqbuf, > > + .vidioc_streamon = vb2_ioctl_streamon, > > + .vidioc_streamoff = vb2_ioctl_streamoff, > > + .vidioc_expbuf = vb2_ioctl_expbuf, > > +}; > > + > > +static const struct v4l2_ioctl_ops mtk_cam_v4l2_meta_ioctl_ops = { > > + .vidioc_querycap = mtk_cam_videoc_querycap, > > + > > + .vidioc_enum_fmt_meta_cap = mtk_cam_meta_enum_format, > > + .vidioc_g_fmt_meta_cap = mtk_cam_videoc_g_meta_fmt, > > + .vidioc_s_fmt_meta_cap = mtk_cam_videoc_s_meta_fmt, > > + .vidioc_try_fmt_meta_cap = mtk_cam_videoc_g_meta_fmt, > > + > > + .vidioc_enum_fmt_meta_out = mtk_cam_meta_enum_format, > > + .vidioc_g_fmt_meta_out = mtk_cam_videoc_g_meta_fmt, > > + .vidioc_s_fmt_meta_out = mtk_cam_videoc_s_meta_fmt, > > + .vidioc_try_fmt_meta_out = mtk_cam_videoc_g_meta_fmt, > > + > > + .vidioc_reqbufs = vb2_ioctl_reqbufs, > > + .vidioc_create_bufs = vb2_ioctl_create_bufs, > > + .vidioc_prepare_buf = vb2_ioctl_prepare_buf, > > + .vidioc_querybuf = vb2_ioctl_querybuf, > > + .vidioc_qbuf = mtk_cam_videoc_qbuf, > > + .vidioc_dqbuf = vb2_ioctl_dqbuf, > > + .vidioc_streamon = vb2_ioctl_streamon, > > + .vidioc_streamoff = vb2_ioctl_streamoff, > > + .vidioc_expbuf = vb2_ioctl_expbuf, > > +}; > > The ops should be split for each node type, {VIDEO, META} x {OUTPUT, > CAPTURE}. Then the core would validate the type given to all the > ioctls automatically. > Ok, below is new implementation. static const struct v4l2_ioctl_ops mtk_cam_v4l2_vcap_ioctl_ops static const struct v4l2_ioctl_ops mtk_cam_v4l2_vout_ioctl_ops static const struct v4l2_ioctl_ops mtk_cam_v4l2_meta_cap_ioctl_ops static const struct v4l2_ioctl_ops mtk_cam_v4l2_meta_out_ioctl_ops > > + > > +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; }; > > + > > +/* Config node's video properties */ > > +/* according to the device context requirement */ > > +static void mtk_cam_node_to_v4l2(struct mtk_cam_dev *isp_dev, u32 node, > > + struct video_device *vdev, > > + struct v4l2_format *f) > > +{ > > + u32 cap; > > + struct mtk_cam_ctx *device_ctx = &isp_dev->ctx; > > + struct mtk_cam_ctx_queue *node_ctx = &device_ctx->queue[node]; > > + > > + WARN_ON(node >= mtk_cam_dev_get_total_node(isp_dev)); > > + WARN_ON(!node_ctx); > > How is this possible? > Ok, we will remove this in next version. > > + > > + /* set cap of the node */ > > + cap = mtk_cam_node_get_v4l2_cap(node_ctx); > > + f->type = mtk_cam_node_get_format_type(node_ctx); > > + vdev->ioctl_ops = mtk_cam_node_get_ioctl_ops(node_ctx); > > + > > + if (mtk_cam_ctx_format_load_default_fmt(&device_ctx->queue[node], f)) { > > + dev_err(&isp_dev->pdev->dev, > > + "Can't load default for node (%d): (%s)", > > + node, device_ctx->queue[node].desc.name); > > mtk_cam_ctx_format_load_default_fmt() doesn't fail (and that's right). > It should be actually made void. > Ok, we will revise this in next version. > > + } else { > > + if (device_ctx->queue[node].desc.image) { > > + dev_dbg(&isp_dev->pdev->dev, > > + "Node (%d): (%s), dfmt (f:0x%x w:%d: h:%d s:%d)\n", > > + node, device_ctx->queue[node].desc.name, > > + f->fmt.pix_mp.pixelformat, > > + f->fmt.pix_mp.width, > > + f->fmt.pix_mp.height, > > + f->fmt.pix_mp.plane_fmt[0].sizeimage > > + ); > > + node_ctx->fmt.pix_mp = f->fmt.pix_mp; > > + } else { > > + dev_dbg(&isp_dev->pdev->dev, > > + "Node (%d): (%s), dfmt (f:0x%x s:%u)\n", > > + node, device_ctx->queue[node].desc.name, > > + f->fmt.meta.dataformat, > > + f->fmt.meta.buffersize > > + ); > > + node_ctx->fmt.meta = f->fmt.meta; > > + } > > Drop the debugging messages and just replace the whole if/else block with: > > node_ctx->fmt = f->fmt; > Same as above. > 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. Best regards, Jungo > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel