Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751368AbdCYWrO (ORCPT ); Sat, 25 Mar 2017 18:47:14 -0400 Received: from ns.mm-sol.com ([37.157.136.199]:46356 "EHLO extserv.mm-sol.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751258AbdCYWrM (ORCPT ); Sat, 25 Mar 2017 18:47:12 -0400 From: Stanimir Varbanov Subject: Re: [PATCH v7 5/9] media: venus: vdec: add video decoder files To: Hans Verkuil , Mauro Carvalho Chehab References: <1489423058-12492-1-git-send-email-stanimir.varbanov@linaro.org> <1489423058-12492-6-git-send-email-stanimir.varbanov@linaro.org> <52b39f43-6f70-0cf6-abaf-4bb5bd2b3d86@xs4all.nl> Cc: Andy Gross , Bjorn Andersson , Stephen Boyd , Srinivas Kandagatla , linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org Message-ID: Date: Sun, 26 Mar 2017 00:30:35 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <52b39f43-6f70-0cf6-abaf-4bb5bd2b3d86@xs4all.nl> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5803 Lines: 185 Thanks for the comments! On 03/24/2017 04:41 PM, Hans Verkuil wrote: > Some comments and questions below: > > On 03/13/17 17:37, Stanimir Varbanov wrote: >> This consists of video decoder implementation plus decoder >> controls. >> >> Signed-off-by: Stanimir Varbanov >> --- >> drivers/media/platform/qcom/venus/vdec.c | 1091 ++++++++++++++++++++++++ >> drivers/media/platform/qcom/venus/vdec.h | 23 + >> drivers/media/platform/qcom/venus/vdec_ctrls.c | 149 ++++ >> 3 files changed, 1263 insertions(+) >> create mode 100644 drivers/media/platform/qcom/venus/vdec.c >> create mode 100644 drivers/media/platform/qcom/venus/vdec.h >> create mode 100644 drivers/media/platform/qcom/venus/vdec_ctrls.c >> >> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c >> new file mode 100644 >> index 000000000000..ec5203f2ba81 >> --- /dev/null >> +++ b/drivers/media/platform/qcom/venus/vdec.c >> @@ -0,0 +1,1091 @@ >> +/* >> + * Copyright (c) 2012-2016, The Linux Foundation. All rights reserved. >> + * Copyright (C) 2017 Linaro Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "hfi_venus_io.h" >> +#include "core.h" >> +#include "helpers.h" >> +#include "vdec.h" >> + >> +static u32 get_framesize_uncompressed(unsigned int plane, u32 width, u32 height) >> +{ >> + u32 y_stride, uv_stride, y_plane; >> + u32 y_sclines, uv_sclines, uv_plane; >> + u32 size; >> + >> + y_stride = ALIGN(width, 128); >> + uv_stride = ALIGN(width, 128); >> + y_sclines = ALIGN(height, 32); >> + uv_sclines = ALIGN(((height + 1) >> 1), 16); >> + >> + y_plane = y_stride * y_sclines; >> + uv_plane = uv_stride * uv_sclines + SZ_4K; >> + size = y_plane + uv_plane + SZ_8K; >> + >> + return ALIGN(size, SZ_4K); >> +} >> + >> +static u32 get_framesize_compressed(unsigned int width, unsigned int height) >> +{ >> + return ((width * height * 3 / 2) / 2) + 128; >> +} >> + >> +static const struct venus_format vdec_formats[] = { >> + { >> + .pixfmt = V4L2_PIX_FMT_NV12, >> + .num_planes = 1, >> + .type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, > > Just curious: is NV12 the only uncompressed format supported by the hardware? > Or just the only one that is implemented here? > >> + }, { >> + .pixfmt = V4L2_PIX_FMT_MPEG4, >> + .num_planes = 1, >> + .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, >> + }, { >> + .pixfmt = V4L2_PIX_FMT_MPEG2, >> + .num_planes = 1, >> + .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, >> + }, { >> + .pixfmt = V4L2_PIX_FMT_H263, >> + .num_planes = 1, >> + .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, >> + }, { >> + .pixfmt = V4L2_PIX_FMT_VC1_ANNEX_G, >> + .num_planes = 1, >> + .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, >> + }, { >> + .pixfmt = V4L2_PIX_FMT_VC1_ANNEX_L, >> + .num_planes = 1, >> + .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, >> + }, { >> + .pixfmt = V4L2_PIX_FMT_H264, >> + .num_planes = 1, >> + .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, >> + }, { >> + .pixfmt = V4L2_PIX_FMT_VP8, >> + .num_planes = 1, >> + .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, >> + }, { >> + .pixfmt = V4L2_PIX_FMT_XVID, >> + .num_planes = 1, >> + .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, >> + }, > > num_planes is always 1, do you need it at all? And if it is always one, > why use _MPLANE at all? Is this for future additions? > >> +}; >> + three reasons: - _MPLAIN allows one plane only - downstream qualcomm driver use _MPLAIN (the second plain is used for extaradata, I ignored the extaradata support for now until v4l2 metadata api is merged) - I still believe that qualcomm firmware guys will add support the second or even third plain at some point. >> + >> +static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, >> + u32 tag, u32 bytesused, u32 data_offset, u32 flags, >> + u64 timestamp_us) >> +{ >> + struct vb2_v4l2_buffer *vbuf; >> + struct vb2_buffer *vb; >> + unsigned int type; >> + >> + if (buf_type == HFI_BUFFER_INPUT) >> + type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE; >> + else >> + type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; >> + >> + vbuf = helper_find_buf(inst, type, tag); >> + if (!vbuf) >> + return; >> + >> + vbuf->flags = flags; >> + >> + if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { >> + vb = &vbuf->vb2_buf; >> + vb->planes[0].bytesused = >> + max_t(unsigned int, inst->output_buf_size, bytesused); >> + vb->planes[0].data_offset = data_offset; >> + vb->timestamp = timestamp_us * NSEC_PER_USEC; >> + vbuf->sequence = inst->sequence++; > > timestamp and sequence are only set for CAPTURE, not OUTPUT. Is that correct? Correct. I can add sequence for the OUTPUT queue too, but I have no idea how that sequence is used by userspace. > >> + >> + if (vbuf->flags & V4L2_BUF_FLAG_LAST) { >> + const struct v4l2_event ev = { .type = V4L2_EVENT_EOS }; >> + >> + v4l2_event_queue_fh(&inst->fh, &ev); >> + } >> + } >> + >> + v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_DONE); >> +} -- regards, Stan