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-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html