Re: [RFC PATCH V1 6/6] platform: mtk-isp: Add Mediatek FD driver - Jerry-ch Chen

From: Jerry-ch Chen <Jerry-ch.Chen@mediatek.com>
To: Tomasz Figa <tfiga@chromium.org>
Cc: "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"Sean Cheng (鄭昇弘)" <Sean.Cheng@mediatek.com>,
	"laurent.pinchart+renesas@ideasonboard.com"
	<laurent.pinchart+renesas@ideasonboard.com>,
	"Rynn Wu (吳育恩)" <Rynn.Wu@mediatek.com>,
	srv_heupstream <srv_heupstream@mediatek.com>,
	"mchehab@kernel.org" <mchehab@kernel.org>,
	"Po-Yang Huang (黃柏陽)" <po-yang.huang@mediatek.com>,
	"Holmes Chiou (邱挺)" <holmes.chiou@mediatek.com>,
	"suleiman@chromium.org" <suleiman@chromium.org>,
	"shik@chromium.org" <shik@chromium.org>,
	"Jungo Lin (林明俊)" <jungo.lin@mediatek.com>,
	"Sj Huang (黃信璋)" <sj.huang@mediatek.com>,
	"yuzhao@chromium.org" <yuzhao@chromium.org>
Subject: Re: [RFC PATCH V1 6/6] platform: mtk-isp: Add Mediatek FD driver
Date: Tue, 25 Jun 2019 18:24:56 +0800	[thread overview]
Message-ID: <1561458296.15267.263.camel@mtksdccf07> (raw)
In-Reply-To: <CAAFQd5B92Vx96nZD7NQzVHByQEspdprLpTTOsqrUbjvUf2jxug@mail.gmail.com>

Hi Tomasz,

On Tue, 2019-06-25 at 18:09 +0800, Tomasz Figa wrote:
> On Tue, Jun 25, 2019 at 5:55 PM Jerry-ch Chen
> <Jerry-ch.Chen@mediatek.com> wrote:
> >
> > Hi Tomasz,
> >
> > On Tue, 2019-06-25 at 11:39 +0800, Tomasz Figa wrote:
> > > Hi Jerry,
> > >
> > > On Mon, Jun 24, 2019 at 11:22 PM Jerry-ch Chen
> > > <Jerry-ch.Chen@mediatek.com> wrote:
> > > >
> > > > Hi Tomasz,
> > > >
> > > > On Thu, 2019-06-06 at 18:43 +0800, Tomasz Figa wrote:
> > > > > Hi Jerry,
> > > > >
> > > > > On Tue, Apr 23, 2019 at 06:45:05PM +0800, Jerry-ch Chen wrote:
> > > > > > From: Jerry-ch Chen <jerry-ch.chen@mediatek.com>
> > > > > >
> > > > > > 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.
> > > > > >
> > > > >
> > > > > Thanks for the patch.
> > > > >
> > > > > First of all a general comment about the design:
> > > > >
> > > > > My understanding is that this is a relatively straightforward
> > > > > memory-to-memory device that reads a video frame and detects faces on it.
> > > > > Such devices should be implemented as normal V4L2 memory-to-memory devices,
> > > > > with contexts (instances; pipes) represented by v4l2_fh.
> > > > >
> > > > > Also, please replace the META_OUTPUT queue with proper V4L2 controls, as I
> > > > > don't think there is anything that we couldn't model using controls here.
> > > > >
> > > > > The end result should be a V4L2 m2m driver (using the m2m helpers), where
> > > > > you get a new context (instance; pipe) whenever you open the video node,
> > > > > similar to codecs, video processors (like MTK MDP) and so on.
> > > > >
> > > > > Also please see my comments inline.
> > > > >
> > > > I appreciate your comments,
> > > > sorry for sending the previous two unfinished mail...
> > > >
> > > > FD driver will be implemented as a normal V4L2 m2m driver which has an
> > > > IMAGE_OUTPUT queue and a META_CAPTURE queue(face result).
> > > >
> > > > We will use the following properties.
> > > > /* Is a video mem-to-mem device that supports multiplanar formats */
> > > > #define V4L2_CAP_VIDEO_M2M_MPLANE    0x00004000
> > > >
> > > > The original META_OUTPUT queue contains the following structure will be
> > > > replaced by V4L2 controls,
> > > >
> > > > /* FD_SCALE_NUM is 15. */
> > > > struct fd_user_param {
> > > >         uint8_t rip_feature;
> > > >         uint8_t gfd_skip;
> > > >         uint8_t dynamic_change_model;
> > > >         uint8_t scale_num_from_user;
> > > >         uint16_t source_img_width[FD_SCALE_NUM];
> > > >         uint16_t source_img_height[FD_SCALE_NUM];
> > > > } __packed; //share with co-processor
> > > >
> > > > However, we found that testM2MFormats() in the V4L2 compliance test will
> > > > assume the capture queue has the same format as output queue has,
> > > > therefore, FD driver's capture queue wouldn't be able to use META format
> > > > or the v4l2 test will be failed.
> > > >
> > > > reference: v4l2-compliance/v4l2-test-formats.cpp
> > > > // m2m devices are special in that the format is often per-filehandle.
> > > > // But colorspace information should be passed from output to capture,
> > > > // so test that.
> > > >         if (node->is_m2m)
> > > >                 return testM2MFormats(node);
> > > >
> > > > May we ask for your suggestions about this part?
> > > >
> > >
> > > Ah, I didn't mean mem-to-mem device specifically as per
> > > V4L2_CAP_VIDEO_M2M_MPLANE, because that one implies the regular
> > > VIDEO_OUTPUT -> VIDEO_CAPTURE processing indeed. We should expose just
> > > VIDEO_OUTPUT_MPLANE and META_CAPTURE in the capabilities, but all the
> > > rest would still behave like a mem-to-mem device, i.e. v4l2_fh for
> > > contexts/instances, v4l2_m2m helpers and so on.
> > >
> > I Appreciate for your reply,
> >
> > Sorry I didn't mention the question clearly, we have included these two
> > capabilities, but we get the following v4l2 test failure:
> > fail: v4l2-test-formats.cpp(784): fmt_cap.g_colorspace() !=
> > fmt_out.g_colorspace()
> 
> My point is that we shouldn't set V4L2_CAP_VIDEO_M2M(_MPLANE) in the
> capabilities, because FD is mem-to-mem in terms of the mode of
> operation, not in terms of the definition of
> V4L2_CAP_VIDEO_M2M(_MPLANE). That would make testM2MFormats() not
> called at all.
> 
> Best regards,
> Tomasz
> 
Ok, we got your point,

Thanks and Best Regards,
Jerry
> >
> > Which is caused by the following code testing the m2m buffers'
> > capabilities, FD driver have fmt_cap with V4L2_BUF_TYPE_META_CAPTURE and
> > fmt_out with V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, therefore, our fmt_cap
> > won't have colorspace information.
> >
> > Reference:
> > https://github.com/gjasny/v4l-utils/blob/master/utils/v4l2-compliance/v4l2-test-formats.cpp#L774
> > fail_on_test(fmt_cap.g_colorspace() != fmt_out.g_colorspace());
> > fail_on_test(fmt_cap.g_ycbcr_enc() != fmt_out.g_ycbcr_enc());
> > fail_on_test(fmt_cap.g_quantization() != fmt_out.g_quantization());
> > fail_on_test(fmt_cap.g_xfer_func() != fmt_out.g_xfer_func());
> >
> > Not sure if the maintainer of v4l2 test would consider modifying here to
> > allow the use case of FD driver?
> >
> > > [snip]
> > >
> > > > > > +
> > > > > > +   return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int mtk_fd_suspend(struct device *dev)
> > > > > > +{
> > > > > > +   struct mtk_fd_dev *fd_dev;
> > > > > > +   int ret;
> > > > > > +
> > > > > > +   if (pm_runtime_suspended(dev))
> > > > > > +           return 0;
> > > > > > +
> > > > > > +   fd_dev = dev_get_drvdata(dev);
> > > > > > +
> > > > > > +   if (atomic_read(&fd_dev->fd_hw.fd_user_cnt) > 0) {
> > > > > > +           ret = pm_runtime_put_sync(fd_dev->fd_hw.larb_dev);
> > > > > > +           clk_disable_unprepare(fd_dev->fd_hw.fd_clk);
> > > > > > +           return ret;
> > > > > > +   }
> > > > >
> > > > > This isn't going to work, because the hardware may be still processing a
> > > > > frame at this point. You need a way to ensure that the hardware goes idle
> > > > > here first and then in resume, you need to make the hardware continue when
> > > > > it left before suspend.
> > > > >
> > > > For this part, I would like to do as following:
> > > > when suspend, it should set the driver power state as idle or suspended
> > > > to stop further enqueue jobs, should be judged in mtk_fd_hw_job_exec()
> > > > or somewhere, then wait for the unfinished job return or timeout, and
> > > > finally close the clock.
> > > > When resume, we set the driver power state as resumed and let the new
> > > > jobs to be enqueued.
> > > >
> > > > Or another way is to create a wait queue or work queue to store the jobs
> > > > from user. When suspend, we change the driver status to restrict the new
> > > > jobs joining to work queue and close the clock. When resume, driver
> > > > continue execute the jobs from the work queue.
> > > >
> > >
> > > I wouldn't introduce a workqueue only for handling suspend/resume. If
> > > we end up in a need to use a workqueue for some other purposes too,
> > > then a freezable workqueue could work for blocking further requests
> > > during suspend indeed. If we don't need a workqueue for anything else,
> > > then a simple boolean flag set and wait for last job to finish in
> > > suspend and flag reset and call to schedule a next job in resume
> > > should be good enough.
> > >
> > > Best regards,
> > > Tomasz
> >
> > Ok, we got it.
> >
> > Sincerely,
> > Jerry
> >