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