Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1536016

Re: [PATCH v4 5/9] media: venus: vdec: add video decoder files

Show all headers | View raw

I have two comments (and the same two comments apply to the video encoder patch
as well):

On 12/01/2016 10:03 AM, Stanimir Varbanov wrote:
> This consists of video decoder implementation plus decoder
> controls.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/vdec.c       | 976 +++++++++++++++++++++++++
>  drivers/media/platform/qcom/venus/vdec.h       |  32 +
>  drivers/media/platform/qcom/venus/vdec_ctrls.c | 149 ++++
>  3 files changed, 1157 insertions(+)
>  create mode 100644 drivers/media/platform/qcom/venus/vdec.c
>  create mode 100644 drivers/media/platform/qcom/venus/vdec.h
>  create mode 100644 drivers/media/platform/qcom/venus/vdec_ctrls.c
> 
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> new file mode 100644
> index 000000000000..9f585a1e0ff1
> --- /dev/null
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -0,0 +1,976 @@

<snip>

> +static int
> +vdec_s_selection(struct file *file, void *fh, struct v4l2_selection *s)
> +{
> +	struct venus_inst *inst = to_inst(file);
> +
> +	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
> +	    s->target != V4L2_SEL_TGT_COMPOSE)
> +		return -EINVAL;
> +
> +	switch (s->target) {
> +	case V4L2_SEL_TGT_COMPOSE:
> +		s->r.width = inst->out_width;
> +		s->r.height = inst->out_height;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	s->r.top = 0;
> +	s->r.left = 0;
> +
> +	return 0;
> +}

This doesn't actually set anything, so what's the point of this function?

I've fixed the corresponding test in v4l2-compliance so you can now drop this
op and v4l2-compliance won't complain anymore.

> +static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
> +{
> +	struct venus_inst *inst = vb2_get_drv_priv(q);
> +	struct venus_core *core = inst->core;
> +	struct device *dev = core->dev;
> +	u32 ptype;
> +	int ret;
> +
> +	mutex_lock(&inst->lock);
> +
> +	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> +		inst->streamon_out = 1;
> +	else
> +		inst->streamon_cap = 1;
> +
> +	if (!(inst->streamon_out & inst->streamon_cap)) {
> +		mutex_unlock(&inst->lock);
> +		return 0;
> +	}
> +
> +	inst->reconfig = false;
> +	inst->sequence = 0;
> +	inst->codec_cfg = false;
> +
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0)
> +		return ret;

This should be a goto so that 'helper_buffers_done(inst, VB2_BUF_STATE_QUEUED);'
is called on error.

It's wrong anyway since you don't unlock the mutex in this return path either.

> +
> +	ret = vdec_init_session(inst);
> +	if (ret)
> +		goto put_sync;
> +
> +	ret = vdec_set_properties(inst);
> +	if (ret)
> +		goto deinit_sess;
> +
> +	if (core->res->hfi_version == HFI_VERSION_3XX) {
> +		struct hfi_buffer_size_actual buf_sz;
> +
> +		ptype = HFI_PROPERTY_PARAM_BUFFER_SIZE_ACTUAL;
> +		buf_sz.type = HFI_BUFFER_OUTPUT;
> +		buf_sz.size = inst->output_buf_size;
> +
> +		ret = hfi_session_set_property(inst, ptype, &buf_sz);
> +		if (ret)
> +			goto deinit_sess;
> +	}
> +
> +	ret = vdec_verify_conf(inst);
> +	if (ret)
> +		goto deinit_sess;
> +
> +	ret = helper_set_num_bufs(inst, inst->num_input_bufs,
> +				  inst->num_output_bufs);
> +	if (ret)
> +		goto deinit_sess;
> +
> +	ret = helper_vb2_start_streaming(inst);
> +	if (ret)
> +		goto deinit_sess;
> +
> +	mutex_unlock(&inst->lock);
> +
> +	return 0;
> +
> +deinit_sess:
> +	hfi_session_deinit(inst);
> +put_sync:
> +	pm_runtime_put_sync(dev);
> +	helper_buffers_done(inst, VB2_BUF_STATE_QUEUED);
> +	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> +		inst->streamon_out = 0;
> +	else
> +		inst->streamon_cap = 0;
> +	mutex_unlock(&inst->lock);
> +	return ret;
> +}

Regards,

	Hans

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v4 0/9] Qualcomm video decoder/encoder driver Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-12-01 10:10 +0100
  [PATCH v4 5/9] media: venus: vdec: add video decoder files Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-12-01 10:10 +0100
    Re: [PATCH v4 5/9] media: venus: vdec: add video decoder files Hans Verkuil <hverkuil@xs4all.nl> - 2016-12-05 12:40 +0100
      Re: [PATCH v4 5/9] media: venus: vdec: add video decoder files Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-12-05 13:40 +0100
  [PATCH v4 2/9] doc: DT: venus: binding document for Qualcomm video driver Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-12-01 10:20 +0100
    Re: [PATCH v4 2/9] doc: DT: venus: binding document for Qualcomm  video driver Rob Herring <robh@kernel.org> - 2016-12-06 01:30 +0100
  [PATCH v4 1/9] media: v4l2-mem2mem: extend m2m APIs for more accurate buffer management Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-12-01 10:20 +0100
    Re: [PATCH v4 1/9] media: v4l2-mem2mem: extend m2m APIs for more  accurate buffer management kbuild test robot <lkp@intel.com> - 2016-12-03 02:00 +0100
    Re: [PATCH v4 1/9] media: v4l2-mem2mem: extend m2m APIs for more  accurate buffer management Hans Verkuil <hverkuil@xs4all.nl> - 2016-12-05 12:30 +0100
      Re: [PATCH v4 1/9] media: v4l2-mem2mem: extend m2m APIs for more  accurate buffer management Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-12-05 13:30 +0100
  [PATCH v4 3/9] MAINTAINERS: Add Qualcomm Venus video accelerator driver Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-12-01 10:20 +0100
  Re: [PATCH v4 8/9] media: venus: hfi: add Venus HFI files Hans Verkuil <hverkuil@xs4all.nl> - 2016-12-05 13:10 +0100
    Re: [PATCH v4 8/9] media: venus: hfi: add Venus HFI files Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-12-05 13:30 +0100

csiph-web