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

Groups > linux.kernel > #1536063

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

Show all headers | View raw

Hi Hans,

Thanks for the comments!

On 12/05/2016 01:32 PM, Hans Verkuil wrote:
> 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.

OK I will give a try.

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

yes, this return is wrong, will correct it in next version.

-- 
regards,
Stan

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