Date:   Mon, 14 Nov 2016 11:42:25 +0200
From:   Stanimir Varbanov <stanimir.varbanov@...aro.org>
To:     Hans Verkuil <hverkuil@...all.nl>,
        Mauro Carvalho Chehab <mchehab@...nel.org>
Cc:     Andy Gross <andy.gross@...aro.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v3 3/9] media: venus: adding core part and helper
 functions

Hi Hans,

Thanks for the comments!

On 11/11/2016 01:32 PM, Hans Verkuil wrote:
> Hi Stanimir,
> 
> Some comments:
> 
> On 11/07/2016 06:33 PM, Stanimir Varbanov wrote:
>>  * core.c has implemented the platform dirver methods, file
>> operations and v4l2 registration.
>>
>>  * helpers.c has implemented common helper functions for:
>>    - buffer management
>>
>>    - vb2_ops and functions for format propagation,
>>
>>    - functions for allocating and freeing buffers for
>>    internal usage. The buffer parameters describing internal
>>    buffers depends on current format, resolution and codec.
>>
>>    - functions for calculation of current load of the
>>    hardware. Depending on the count of instances and
>>    resolutions it selects the best clock rate for the video
>>    core.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@...aro.org>
>> ---
>>  drivers/media/platform/qcom/venus/core.c    | 557 +++++++++++++++++++++++++
>>  drivers/media/platform/qcom/venus/core.h    | 261 ++++++++++++
>>  drivers/media/platform/qcom/venus/helpers.c | 612 ++++++++++++++++++++++++++++
>>  drivers/media/platform/qcom/venus/helpers.h |  43 ++
>>  4 files changed, 1473 insertions(+)
>>  create mode 100644 drivers/media/platform/qcom/venus/core.c
>>  create mode 100644 drivers/media/platform/qcom/venus/core.h
>>  create mode 100644 drivers/media/platform/qcom/venus/helpers.c
>>  create mode 100644 drivers/media/platform/qcom/venus/helpers.h
>>
> 
> <snip>
> 
>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
>> new file mode 100644
>> index 000000000000..21ed053aeb17
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/venus/core.h
> 
> <snip>
> 
>> +struct venus_ctrl {
>> +	u32 id;
>> +	enum v4l2_ctrl_type type;
>> +	s32 min;
>> +	s32 max;
>> +	s32 def;
>> +	u32 step;
>> +	u64 menu_skip_mask;
>> +	u32 flags;
>> +	const char * const *qmenu;
>> +};
> 
> Why duplicate struct v4l2_ctrl_config? Just use that struct to define custom controls
> together with v4l2_ctrl_new_custom().

OK, I will rework the controls to avoid struct v4l2_ctrl_config duplication.

> 
>> +
>> +/*
>> + * Offset base for buffers on the destination queue - used to distinguish
>> + * between source and destination buffers when mmapping - they receive the same
>> + * offsets but for different queues
>> + */
>> +#define DST_QUEUE_OFF_BASE	(1 << 30)
>> +
>> +static inline struct venus_inst *to_inst(struct file *filp)
>> +{
>> +	return container_of(filp->private_data, struct venus_inst, fh);
>> +}
>> +
>> +static inline void *to_hfi_priv(struct venus_core *core)
>> +{
>> +	return core->priv;
>> +}
>> +
>> +static inline struct vb2_queue *
>> +to_vb2q(struct file *file, enum v4l2_buf_type type)
>> +{
>> +	struct venus_inst *inst = to_inst(file);
>> +
>> +	if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
>> +		return &inst->bufq_cap;
>> +	else if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>> +		return &inst->bufq_out;
>> +
>> +	return NULL;
>> +}
>> +
>> +#endif
>> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
>> new file mode 100644
>> index 000000000000..c2d1446ad254
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/venus/helpers.c
> 
> <snip>
> 
>> +void vidc_vb2_stop_streaming(struct vb2_queue *q)
>> +{
>> +	struct venus_inst *inst = vb2_get_drv_priv(q);
>> +	struct venus_core *core = inst->core;
>> +	struct device *dev = core->dev;
>> +	struct vb2_queue *other_queue;
>> +	struct vidc_buffer *buf, *n;
>> +	enum vb2_buffer_state state;
>> +	int ret;
>> +
>> +	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>> +		other_queue = &inst->bufq_cap;
>> +	else
>> +		other_queue = &inst->bufq_out;
>> +
>> +	if (!vb2_is_streaming(other_queue))
>> +		return;
> 
> This seems wrong to me: this return means that the buffers of queue q are never
> released. Either drop this 'if' or release both queues when the last queue
> stops streaming. I think dropping the 'if' is best.

I have done this way because hfi_session_stop must be called only once,
and buffers will be released on first streamoff for both queues.

> 
>> +
>> +	ret = hfi_session_stop(inst);
>> +	if (ret) {
>> +		dev_err(dev, "session: stop failed (%d)\n", ret);
>> +		goto abort;
>> +	}
>> +
>> +	ret = hfi_session_unload_res(inst);
>> +	if (ret) {
>> +		dev_err(dev, "session: release resources failed (%d)\n", ret);
>> +		goto abort;
>> +	}
>> +
>> +	ret = session_unregister_bufs(inst);
>> +	if (ret) {
>> +		dev_err(dev, "failed to release capture buffers: %d\n", ret);
>> +		goto abort;
>> +	}
>> +
>> +	ret = intbufs_free(inst);
>> +
>> +	if (inst->state == INST_INVALID || core->state == CORE_INVALID)
>> +		ret = -EINVAL;
>> +
>> +abort:
>> +	if (ret)
>> +		hfi_session_abort(inst);
>> +
>> +	load_scale_clocks(core);
>> +
>> +	ret = hfi_session_deinit(inst);
>> +
>> +	pm_runtime_put_sync(dev);
>> +
>> +	mutex_lock(&inst->bufqueue_lock);
>> +
>> +	if (list_empty(&inst->bufqueue)) {
>> +		mutex_unlock(&inst->bufqueue_lock);
>> +		return;
>> +	}
>> +
>> +	if (ret)
>> +		state = VB2_BUF_STATE_ERROR;
>> +	else
>> +		state = VB2_BUF_STATE_DONE;
> 
> Are you sure that the state depends on 'ret'? Usually when stop_streaming is
> called none of the pending buffers are filled with valid frame data, so the
> state is set to ERROR.
> 
> STATE_DONE implies that the contents of the buffers has valid frame data.

OK I will return STATE_ERROR for all released buffers.

> 
>> +
>> +	list_for_each_entry_safe(buf, n, &inst->bufqueue, list) {
>> +		vb2_buffer_done(&buf->vb.vb2_buf, state);
>> +		list_del(&buf->list);
>> +	}
>> +
>> +	mutex_unlock(&inst->bufqueue_lock);
>> +}
>> +
>> +int vidc_vb2_start_streaming(struct venus_inst *inst)
>> +{
>> +	struct venus_core *core = inst->core;
>> +	struct vidc_buffer *buf, *n;
>> +	int ret;
>> +
>> +	ret = intbufs_alloc(inst);
>> +	if (ret)
>> +		return ret;
> 
> This should still release all buffers instead of returning an error.

Yes, thanks for catching this.

> 
>> +
>> +	ret = session_register_bufs(inst);
>> +	if (ret)
>> +		goto err_bufs_free;
>> +
>> +	load_scale_clocks(core);
>> +
>> +	ret = hfi_session_load_res(inst);
>> +	if (ret)
>> +		goto err_unreg_bufs;
>> +
>> +	ret = hfi_session_start(inst);
>> +	if (ret)
>> +		goto err_unload_res;
>> +
>> +	mutex_lock(&inst->bufqueue_lock);
>> +	list_for_each_entry_safe(buf, n, &inst->bufqueue, list) {
>> +		ret = session_set_buf(&buf->vb.vb2_buf);
>> +		if (ret)
>> +			break;
>> +	}
>> +	mutex_unlock(&inst->bufqueue_lock);
>> +
>> +	if (ret)
>> +		goto err_session_stop;
>> +
>> +	return 0;
>> +
>> +err_session_stop:
>> +	hfi_session_stop(inst);
>> +err_unload_res:
>> +	hfi_session_unload_res(inst);
>> +err_unreg_bufs:
>> +	session_unregister_bufs(inst);
>> +err_bufs_free:
>> +	intbufs_free(inst);
>> +
>> +	mutex_lock(&inst->bufqueue_lock);
>> +
>> +	if (list_empty(&inst->bufqueue))
>> +		goto err_done;
>> +
>> +	list_for_each_entry_safe(buf, n, &inst->bufqueue, list) {
>> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED);
>> +		list_del(&buf->list);
>> +	}
> 
> I think this is done in the wrong place. The vdec has its own high level start_streaming
> that calls this function, but that high level function doesn't release the buffers at
> all if there is an error. Same for venc. I propose that you make a new function that
> just releases the buffers and call that from the vdec/venc start_streaming function
> whenever an error occurs.

yes, that is a good suggestion.


-- 
regards,
Stan

Powered by blists - more mailing lists