2016-04-22 07:38:15

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v7 5/8] [media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver

Attachments:

(No filename) (2.52 kB)
.config.gz (52.52 kB)
Download all attachments

2016-04-22 07:51:15

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v7 6/8] [media] vcodec: mediatek: Add Mediatek VP8 Video Encoder Driver

Hi,

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.6-rc4 next-20160421]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url: https://github.com/0day-ci/linux/commits/Tiffany-Lin/Add-MT8173-Video-Encoder-Driver-and-VPU-Driver/20160422-123111
base: git://linuxtv.org/media_tree.git master
config: i386-allyesconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386

All warnings (new ones prefixed by >>):

drivers/media/platform/mtk-vcodec/venc/venc_vp8_if.c: In function 'vp8_enc_alloc_work_buf':
>> drivers/media/platform/mtk-vcodec/venc/venc_vp8_if.c:214:35: warning: format '%lx' expects argument of type 'long unsigned int', but argument 7 has type 'size_t {aka unsigned int}' [-Wformat=]
drivers/media/platform/mtk-vcodec/venc/venc_vp8_if.c: In function 'vp8_enc_compose_one_frame':
>> drivers/media/platform/mtk-vcodec/venc/venc_vp8_if.c:279:10: warning: format '%ld' expects argument of type 'long int', but argument 4 has type 'size_t {aka unsigned int}' [-Wformat=]
mtk_vcodec_err(inst, "bitstream buf size is too small(%ld)",
^

vim +214 drivers/media/platform/mtk-vcodec/venc/venc_vp8_if.c

208 tmp_va = vpu_mapping_dm_addr(inst->vpu_inst.dev,
209 wb[i].vpua);
210 memcpy(inst->work_bufs[i].va, tmp_va, wb[i].size);
211 }
212 wb[i].iova = inst->work_bufs[i].dma_addr;
213
> 214 mtk_vcodec_debug(inst,
215 "work_bufs[%d] va=0x%p,iova=0x%p,size=0x%lx",
216 i, inst->work_bufs[i].va,
217 (void *)inst->work_bufs[i].dma_addr,
218 inst->work_bufs[i].size);
219 }
220
221 mtk_vcodec_debug_leave(inst);
222
223 return ret;
224
225 err_alloc:
226 vp8_enc_free_work_buf(inst);
227
228 return ret;
229 }
230
231 static unsigned int vp8_enc_wait_venc_done(struct venc_vp8_inst *inst)
232 {
233 unsigned int irq_status = 0;
234 struct mtk_vcodec_ctx *ctx = (struct mtk_vcodec_ctx *)inst->ctx;
235
236 if (!mtk_vcodec_wait_for_done_ctx(ctx, MTK_INST_IRQ_RECEIVED,
237 WAIT_INTR_TIMEOUT_MS)) {
238 irq_status = ctx->irq_status;
239 mtk_vcodec_debug(inst, "isr return %x", irq_status);
240 }
241 return irq_status;
242 }
243
244 /*
245 * Compose ac_tag, bitstream header and bitstream payload into
246 * one bitstream buffer.
247 */
248 static int vp8_enc_compose_one_frame(struct venc_vp8_inst *inst,
249 struct mtk_vcodec_mem *bs_buf,
250 unsigned int *bs_size)
251 {
252 unsigned int not_key;
253 u32 bs_frm_size;
254 u32 bs_hdr_len;
255 unsigned int ac_tag_size;
256 u8 ac_tag[MAX_AC_TAG_SIZE];
257
258 bs_frm_size = vp8_enc_read_reg(inst, VENC_BITSTREAM_FRAME_SIZE);
259 bs_hdr_len = vp8_enc_read_reg(inst, VENC_BITSTREAM_HEADER_LEN);
260
261 /* if a frame is key frame, not_key is 0 */
262 not_key = !inst->vpu_inst.is_key_frm;
263 *(u32 *)ac_tag = __cpu_to_le32((bs_hdr_len << 5) | 0x10 | not_key);
264 /* key frame */
265 if (not_key == 0) {
266 ac_tag_size = MAX_AC_TAG_SIZE;
267 ac_tag[3] = 0x9d;
268 ac_tag[4] = 0x01;
269 ac_tag[5] = 0x2a;
270 ac_tag[6] = inst->vsi->config.pic_w;
271 ac_tag[7] = inst->vsi->config.pic_w >> 8;
272 ac_tag[8] = inst->vsi->config.pic_h;
273 ac_tag[9] = inst->vsi->config.pic_h >> 8;
274 } else {
275 ac_tag_size = 3;
276 }
277
278 if (bs_buf->size < bs_hdr_len + bs_frm_size + ac_tag_size) {
> 279 mtk_vcodec_err(inst, "bitstream buf size is too small(%ld)",
280 bs_buf->size);
281 return -EINVAL;
282 }

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (3.86 kB)
.config.gz (52.52 kB)
Download all attachments

2016-04-22 08:07:01

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v7 7/8] [media] vcodec: mediatek: Add Mediatek H264 Video Encoder Driver

Attachments:

(No filename) (1.94 kB)
.config.gz (52.52 kB)
Download all attachments

2016-04-22 09:38:46

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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Tiffany Lin <[email protected]>
> >
> > ---
> > 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);
>