Re: [PATCH v3 3/9] media: venus: adding core part and helper functions

[Hans Verkuil]

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@xxxxxxxxxx>
> ---
>  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().

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

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

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

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

> +
> +err_done:
> +	mutex_unlock(&inst->bufqueue_lock);
> +
> +	return ret;
> +}

Regards,

	Hans