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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
