Re: [PATCH 3/8] media: vidc: decoder: add video decoder files

[Hans Verkuil]

On 08/23/16 14:45, Stanimir Varbanov wrote:
> Hi Hans,
> 
> Thanks for the valuable comments!
> 
> <cut>
> 
>>> +static int vdec_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>>> +{
>>> +	struct vidc_inst *inst = to_inst(file);
>>> +	struct v4l2_captureparm *cap = &a->parm.capture;
>>> +	struct v4l2_fract *timeperframe = &cap->timeperframe;
>>> +	u64 us_per_frame, fps;
>>> +
>>> +	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
>>> +	    a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>>> +		return -EINVAL;
>>> +
>>> +	memset(cap->reserved, 0, sizeof(cap->reserved));
>>> +	if (!timeperframe->denominator)
>>> +		timeperframe->denominator = inst->timeperframe.denominator;
>>> +	if (!timeperframe->numerator)
>>> +		timeperframe->numerator = inst->timeperframe.numerator;
>>> +	cap->readbuffers = 0;
>>
>> Just set readbuffers to the minimum number of required buffers. Hmm, shouldn't
>> v4l2-compliance complain about a 0 value for readbuffers? Odd.
> 
> I guess I misunderstood the meaning of those fields and just zeroing
> them to make v4l2-compliance happy.

These fields are really left-overs from old times. One of these days I'll
probably remove them. I think that there are still one or two old drivers
that actually use these.

>>> +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.

> 
>>
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (!vb2_is_streaming(queue))
>>> +		return 0;
>>
>> Can never happen, no need to test for this.
> 
> This can happen cause start_streaming is called for OUTPUT and for
> CAPTURE queues, see the above switch (q->type). If start_streaming is
> called for the CAPTURE thus vb2_is_streaming() checks does
> start_streaming is already called for OUTPUT. So this guarantee that the
> firmware will be started when the streaming is started for both queues.

Ah, now I see it. Please rename 'queue' to 'other_queue' or something like
that. I thought it was the queue that start_streaming was called for, but
that is called 'q'. A bit confusing :-)

> 
>>
>>> +
>>> +	inst->in_reconfig = false;
>>> +	inst->sequence = 0;
>>> +
>>> +	ret = pm_runtime_get_sync(dev);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	ret = vdec_init_session(inst);
>>> +	if (ret)
>>> +		goto put_sync;
>>> +
>>> +	ret = vdec_set_properties(inst);
>>> +	if (ret)
>>> +		goto deinit_sess;
>>> +
>>> +	ret = vdec_check_configuration(inst);
>>> +	if (ret)
>>> +		goto deinit_sess;
>>> +
>>> +	ptype = HFI_PROPERTY_PARAM_BUFFER_COUNT_ACTUAL;
>>> +	buf_count.type = HFI_BUFFER_INPUT;
>>> +	buf_count.count_actual = inst->num_input_bufs;
>>> +
>>> +	ret = vidc_hfi_session_set_property(hfi, inst->hfi_inst,
>>> +					    ptype, &buf_count);
>>> +	if (ret)
>>> +		goto deinit_sess;
>>> +
>>> +	ret = vidc_buf_descs(inst, HFI_BUFFER_OUTPUT, &bufreq);
>>> +	if (ret)
>>> +		goto deinit_sess;
>>> +
>>> +	ptype = HFI_PROPERTY_PARAM_BUFFER_COUNT_ACTUAL;
>>> +	buf_count.type = HFI_BUFFER_OUTPUT;
>>> +	buf_count.count_actual = inst->num_output_bufs;
>>> +
>>> +	ret = vidc_hfi_session_set_property(hfi, inst->hfi_inst,
>>> +					    ptype, &buf_count);
>>> +	if (ret)
>>> +		goto deinit_sess;
>>> +
>>> +	if (inst->num_output_bufs != bufreq.count_actual) {
>>> +		struct hfi_buffer_display_hold_count_actual display;
>>> +
>>> +		ptype = HFI_PROPERTY_PARAM_BUFFER_DISPLAY_HOLD_COUNT_ACTUAL;
>>> +		display.type = HFI_BUFFER_OUTPUT;
>>> +		display.hold_count = inst->num_output_bufs -
>>> +				     bufreq.count_actual;
>>> +
>>> +		ret = vidc_hfi_session_set_property(hfi, inst->hfi_inst,
>>> +						    ptype, &display);
>>> +		if (ret)
>>> +			goto deinit_sess;
>>> +	}
>>> +
>>> +	ret = vidc_vb2_start_streaming(inst);
>>> +	if (ret)
>>> +		goto deinit_sess;
>>> +
>>> +	return 0;
>>> +
>>> +deinit_sess:
>>> +	vidc_hfi_session_deinit(hfi, inst->hfi_inst);
>>
>> Note that vidc_vb2_start_streaming already calls vidc_hfi_session_deinit on error.
> 
> Probably you wanted to say vdec_init_session has deinit on error path?
> If so I cannot see the problem, vidc_hfi_session_deinit is called only once?

Never mind. I thought vidc_hfi_session_deinit() was called from vidc_vb2_start_streaming,
but it is called from vidc_vb2_stop_streaming. My mistake.

>>> +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.

Regards,

	Hans