linux-kernel - Re: [PATCH v4 5/9] media: venus: vdec: add video decoder files

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 5 Dec 2016 14:25:18 +0200
From:   Stanimir Varbanov <stanimir.varbanov@...aro.org>
To:     Hans Verkuil <hverkuil@...all.nl>,
        Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>
Cc:     Andy Gross <andy.gross@...aro.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v4 5/9] media: venus: vdec: add video decoder files

Hi Hans,

Thanks for the comments!

On 12/05/2016 01:32 PM, Hans Verkuil wrote:
> I have two comments (and the same two comments apply to the video encoder patch
> as well):
> 
> On 12/01/2016 10:03 AM, Stanimir Varbanov wrote:
>> This consists of video decoder implementation plus decoder
>> controls.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@...aro.org>
>> ---
>>  drivers/media/platform/qcom/venus/vdec.c       | 976 +++++++++++++++++++++++++
>>  drivers/media/platform/qcom/venus/vdec.h       |  32 +
>>  drivers/media/platform/qcom/venus/vdec_ctrls.c | 149 ++++
>>  3 files changed, 1157 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..9f585a1e0ff1
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>> @@ -0,0 +1,976 @@
> 
> <snip>
> 
>> +static int
>> +vdec_s_selection(struct file *file, void *fh, struct v4l2_selection *s)
>> +{
>> +	struct venus_inst *inst = to_inst(file);
>> +
>> +	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
>> +	    s->target != V4L2_SEL_TGT_COMPOSE)
>> +		return -EINVAL;
>> +
>> +	switch (s->target) {
>> +	case V4L2_SEL_TGT_COMPOSE:
>> +		s->r.width = inst->out_width;
>> +		s->r.height = inst->out_height;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	s->r.top = 0;
>> +	s->r.left = 0;
>> +
>> +	return 0;
>> +}
> 
> This doesn't actually set anything, so what's the point of this function?
> 
> I've fixed the corresponding test in v4l2-compliance so you can now drop this
> op and v4l2-compliance won't complain anymore.

OK I will give a try.

> 
>> +static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>> +{
>> +	struct venus_inst *inst = vb2_get_drv_priv(q);
>> +	struct venus_core *core = inst->core;
>> +	struct device *dev = core->dev;
>> +	u32 ptype;
>> +	int ret;
>> +
>> +	mutex_lock(&inst->lock);
>> +
>> +	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>> +		inst->streamon_out = 1;
>> +	else
>> +		inst->streamon_cap = 1;
>> +
>> +	if (!(inst->streamon_out & inst->streamon_cap)) {
>> +		mutex_unlock(&inst->lock);
>> +		return 0;
>> +	}
>> +
>> +	inst->reconfig = false;
>> +	inst->sequence = 0;
>> +	inst->codec_cfg = false;
>> +
>> +	ret = pm_runtime_get_sync(dev);
>> +	if (ret < 0)
>> +		return ret;
> 
> This should be a goto so that 'helper_buffers_done(inst, VB2_BUF_STATE_QUEUED);'
> is called on error.
> 
> It's wrong anyway since you don't unlock the mutex in this return path either.

yes, this return is wrong, will correct it in next version.

-- 
regards,
Stan