linux-kernel - Re: [PATCH 3/8] media: vidc: decoder: 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]

Message-ID: <c161a005-0c28-0502-6edf-a74fa15b3849@linaro.org>
Date:   Mon, 29 Aug 2016 17:22:09 +0300
From:   Stanimir Varbanov <stanimir.varbanov@...aro.org>
To:     Hans Verkuil <hansverk@...co.com>,
        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 3/8] media: vidc: decoder: add video decoder files

Hi Hans,

<cut>

>>>> +static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>>>> +{
>>>> +	struct vidc_inst *inst = vb2_get_drv_priv(q);
>>>> +	struct hfi_core *hfi = &inst->core->hfi;
>>>> +	struct device *dev = inst->core->dev;
>>>> +	struct hfi_buffer_requirements bufreq;
>>>> +	struct hfi_buffer_count_actual buf_count;
>>>> +	struct vb2_queue *queue;
>>>> +	u32 ptype;
>>>> +	int ret;
>>>> +
>>>> +	switch (q->type) {
>>>> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>>>> +		queue = &inst->bufq_cap;
>>>> +		break;
>>>> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>>>> +		queue = &inst->bufq_out;
>>>> +		break;
>>>> +	default:
>>>
>>> If start_streaming fails, then all pending buffers have to be returned by the driver
>>> by calling vb2_buffer_done(VB2_BUF_STATE_QUEUED). This will give ownership back to
>>> userspace.
>>
>> Infact this error path shouldn't be possible, because videobu2-core
>> should check q->type before jumping here?
> 
> Sorry, I wasn't clear. It is not just this place (and you are right, the error
> path here isn't possible), but any place where an error is returned in this
> function.
> 
> Those should be replaced by a goto fail; and in the fail label all pending buffers
> have to be returned. Same code as in stop_streaming, but you call vb2_buffer_done
> with state QUEUED instead of state ERROR.

OK, I need to call vb2_buffer_done(ERROR) for every queued buffer
(before invocation of STREAM_ON) if start_streaming failed.

I think that in present code I have calls to vb2_buffer_done but with
status DONE.

<cut>

>>>> +static int vdec_op_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
>>>> +{
>>>> +	struct vidc_inst *inst = ctrl_to_inst(ctrl);
>>>> +	struct vdec_controls *ctr = &inst->controls.dec;
>>>> +	struct hfi_core *hfi = &inst->core->hfi;
>>>> +	union hfi_get_property hprop;
>>>> +	u32 ptype = HFI_PROPERTY_PARAM_PROFILE_LEVEL_CURRENT;
>>>> +	int ret;
>>>> +
>>>> +	switch (ctrl->id) {
>>>> +	case V4L2_CID_MPEG_VIDEO_H264_PROFILE:
>>>> +	case V4L2_CID_MPEG_VIDEO_MPEG4_PROFILE:
>>>> +	case V4L2_CID_MPEG_VIDEO_VPX_PROFILE:
>>>> +		ret = vidc_hfi_session_get_property(hfi, inst->hfi_inst, ptype,
>>>> +						    &hprop);
>>>> +		if (!ret)
>>>> +			ctr->profile = hprop.profile_level.profile;
>>>> +		ctrl->val = ctr->profile;
>>>> +		break;
>>>> +	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
>>>> +	case V4L2_CID_MPEG_VIDEO_MPEG4_LEVEL:
>>>> +		ret = vidc_hfi_session_get_property(hfi, inst->hfi_inst, ptype,
>>>> +						    &hprop);
>>>> +		if (!ret)
>>>> +			ctr->level = hprop.profile_level.level;
>>>> +		ctrl->val = ctr->level;
>>>> +		break;
>>>> +	case V4L2_CID_MPEG_VIDEO_DECODER_MPEG4_DEBLOCK_FILTER:
>>>> +		ctrl->val = ctr->post_loop_deb_mode;
>>>> +		break;
>>>
>>> Why are these volatile?
>>
>> Because the firmware acording to stream headers that profile and levels
>> are different.
> 
> But when these change, isn't the driver told about it? And can these
> change midstream? I would expect this to be set once when you start
> decoding and not change afterwards.

Actually the decoder firmware will detect the profile/level pair itself
based on elementary stream headers. So I'd expect that getting those
parameters by session_get_property API will just return what the decoder
thinks about profile/level.

-- 
regards,
Stan