Re: [PATCH v7 2/8] [media] VPU: mediatek: support Mediatek VPU

From	andrew-ct chen <andrew-ct.chen@mediatek.com>
Newsgroups	linux.kernel
Subject	Re: [PATCH v7 2/8] [media] VPU: mediatek: support Mediatek VPU
Date	2016-04-25 11:30 +0200
Message-ID	<rrL06-3oi-19@gated-at.bofh.it> (permalink)
References	<rqAT7-4e7-1@gated-at.bofh.it> <rqAT7-4e7-5@gated-at.bofh.it> <rqAT7-4e7-7@gated-at.bofh.it> <rrJrk-1Vb-11@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

On Mon, 2016-04-25 at 09:40 +0200, Hans Verkuil wrote:
> On 04/22/2016 06:25 AM, Tiffany Lin wrote:
> > From: Andrew-CT Chen <andrew-ct.chen@mediatek.com>
> > 
> > The VPU driver for hw video codec embedded in Mediatek's MT8173 SOCs.
> > It is able to handle video decoding/encoding of in a range of formats.
> > The driver provides with VPU firmware download, memory management and
> > the communication interface between CPU and VPU.
> > For VPU initialization, it will create virtual memory for CPU access and
> > IOMMU address for vcodec hw device access. When a decode/encode instance
> > opens a device node, vpu driver will download vpu firmware to the device.
> > A decode/encode instant will decode/encode a frame using VPU
> > interface to interrupt vpu to handle decoding/encoding jobs.
> > 
> > Signed-off-by: Andrew-CT Chen <andrew-ct.chen@mediatek.com>
> > Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
> > 
> > ---
> >  drivers/media/platform/Kconfig           |   13 +
> >  drivers/media/platform/Makefile          |    2 +
> >  drivers/media/platform/mtk-vpu/Makefile  |    3 +
> >  drivers/media/platform/mtk-vpu/mtk_vpu.c |  950 ++++++++++++++++++++++++++++++
> >  drivers/media/platform/mtk-vpu/mtk_vpu.h |  162 +++++
> >  5 files changed, 1130 insertions(+)
> >  create mode 100644 drivers/media/platform/mtk-vpu/Makefile
> >  create mode 100755 drivers/media/platform/mtk-vpu/mtk_vpu.c
> >  create mode 100644 drivers/media/platform/mtk-vpu/mtk_vpu.h
> > 
> 
> 
> > +int vpu_load_firmware(struct platform_device *pdev)
> > +{
> > +	struct mtk_vpu *vpu = platform_get_drvdata(pdev);
> > +	struct device *dev = &pdev->dev;
> > +	struct vpu_run *run = &vpu->run;
> > +	const struct firmware *vpu_fw;
> > +	int ret;
> > +
> > +	if (!pdev) {
> > +		dev_err(dev, "VPU platform device is invalid\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = vpu_clock_enable(vpu);
> > +	if (ret) {
> > +		dev_err(dev, "enable clock failed %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	mutex_lock(&vpu->vpu_mutex);
> > +
> > +	if (vpu_running(vpu)) {
> > +		mutex_unlock(&vpu->vpu_mutex);
> > +		vpu_clock_disable(vpu);
> > +		dev_warn(dev, "vpu is running already\n");
> 
> This warning should be dropped. Currently vpu_load_firmware is called
> every time the video device is opened and no one else has the video device
> open. So calling this multiple times is perfectly normal and the log shouldn't
> be spammed with warnings.
> 
> I would recommend adding a fw_loaded bool to struct mtk_vpu and just
> check that at the beginning of this function and just return 0 if it is true.
> 
> Then you don't need to enable the vpu clock either.
> 
> I hope I understand the hw correctly, though.
> 
> Assuming you can do this, then this code from the v4l driver needs an
> additional comment:

We will change this in next version.

> 
> >>> +	if (v4l2_fh_is_singular(&ctx->fh)) {
> 
> Add a comment here that says that vpu_load_firmware checks if it was
> loaded already and does nothing in that case.

We will change this in next version. Thanks

> 
> >>> +		ret = vpu_load_firmware(dev->vpu_plat_dev);
> >>> +		if (ret < 0) {
> >>> +			/*
> >>> +			  * Return 0 if downloading firmware successfully,
> >>> +			  * otherwise it is failed
> >>> +			  */
> >>> +			mtk_v4l2_err("vpu_load_firmware failed!");
> >>> +			goto err_load_fw;
> >>> +		}
> 
> That makes it clear to the reader (i.e. me :-) ) that you can safely call
> vpu_load_firmware multiple times.
> 
> Regards,
> 
> 	Hans
> 
> > +		return 0;
> > +	}
> > +
> > +	run->signaled = false;
> > +	dev_dbg(vpu->dev, "firmware request\n");
> > +	/* Downloading program firmware to device*/
> > +	ret = load_requested_vpu(vpu, vpu_fw, P_FW);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to request %s, %d\n", VPU_P_FW, ret);
> > +		goto OUT_LOAD_FW;
> > +	}
> > +
> > +	/* Downloading data firmware to device */
> > +	ret = load_requested_vpu(vpu, vpu_fw, D_FW);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to request %s, %d\n", VPU_D_FW, ret);
> > +		goto OUT_LOAD_FW;
> > +	}
> > +
> > +	/* boot up vpu */
> > +	vpu_cfg_writel(vpu, 0x1, VPU_RESET);
> > +
> > +	ret = wait_event_interruptible_timeout(run->wq,
> > +					       run->signaled,
> > +					       msecs_to_jiffies(INIT_TIMEOUT_MS)
> > +					       );
> > +	if (ret == 0) {
> > +		ret = -ETIME;
> > +		dev_err(dev, "wait vpu initialization timout!\n");
> > +		goto OUT_LOAD_FW;
> > +	} else if (-ERESTARTSYS == ret) {
> > +		dev_err(dev, "wait vpu interrupted by a signal!\n");
> > +		goto OUT_LOAD_FW;
> > +	}
> > +
> > +	ret = 0;
> > +	dev_info(dev, "vpu is ready. Fw version %s\n", run->fw_ver);
> > +
> > +OUT_LOAD_FW:
> > +	mutex_unlock(&vpu->vpu_mutex);
> > +	vpu_clock_disable(vpu);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(vpu_load_firmware);
>

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[PATCH v7 0/8] Add MT8173 Video Encoder Driver and VPU Driver Tiffany Lin <tiffany.lin@mediatek.com> - 2016-04-22 06:30 +0200
  [PATCH v7 8/8] arm64: dts: mediatek: Add Video Encoder for MT8173 Tiffany Lin <tiffany.lin@mediatek.com> - 2016-04-22 06:30 +0200
  [PATCH v7 3/8] arm64: dts: mediatek: Add node for Mediatek Video Processor Unit Tiffany Lin <tiffany.lin@mediatek.com> - 2016-04-22 06:30 +0200
    [PATCH v7 4/8] dt-bindings: Add a binding for Mediatek Video Encoder Tiffany Lin <tiffany.lin@mediatek.com> - 2016-04-22 06:30 +0200
      [PATCH v7 7/8] [media] vcodec: mediatek: Add Mediatek H264 Video Encoder Driver Tiffany Lin <tiffany.lin@mediatek.com> - 2016-04-22 06:30 +0200
      [PATCH v7 6/8] [media] vcodec: mediatek: Add Mediatek VP8 Video Encoder Driver Tiffany Lin <tiffany.lin@mediatek.com> - 2016-04-22 06:30 +0200
      Re: [PATCH v7 5/8] [media] vcodec: mediatek: Add Mediatek V4L2 Video  Encoder Driver Hans Verkuil <hverkuil@xs4all.nl> - 2016-04-22 15:50 +0200
        Re: [PATCH v7 5/8] [media] vcodec: mediatek: Add Mediatek V4L2  Video Encoder Driver tiffany lin <tiffany.lin@mediatek.com> - 2016-04-25 07:20 +0200
          Re: [PATCH v7 5/8] [media] vcodec: mediatek: Add Mediatek V4L2 Video  Encoder Driver Wu-Cheng Li (李務誠)   <wuchengli@chromium.org> - 2016-04-25 07:50 +0200
            Re: [PATCH v7 5/8] [media] vcodec: mediatek: Add Mediatek V4L2  Video Encoder Driver tiffany lin <tiffany.lin@mediatek.com> - 2016-04-25 09:10 +0200
  [PATCH v7 1/8] dt-bindings: Add a binding for Mediatek Video Processor Tiffany Lin <tiffany.lin@mediatek.com> - 2016-04-22 06:30 +0200
    [PATCH v7 2/8] [media] VPU: mediatek: support Mediatek VPU Tiffany Lin <tiffany.lin@mediatek.com> - 2016-04-22 06:30 +0200
      Re: [PATCH v7 2/8] [media] VPU: mediatek: support Mediatek VPU Hans Verkuil <hverkuil@xs4all.nl> - 2016-04-25 09:50 +0200
        Re: [PATCH v7 2/8] [media] VPU: mediatek: support Mediatek VPU andrew-ct chen <andrew-ct.chen@mediatek.com> - 2016-04-25 11:30 +0200

csiph-web