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

Groups > linux.kernel > #1521459 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 3/9] media: venus: adding core part and helper  functions Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-11-14 10:50 +0100
    Re: [PATCH v3 3/9] media: venus: adding core part and helper  functions Hans Verkuil <hverkuil@xs4all.nl> - 2016-11-14 10:50 +0100
      Re: [PATCH v3 3/9] media: venus: adding core part and helper  functions Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-11-14 11:20 +0100
        Re: [PATCH v3 3/9] media: venus: adding core part and helper  functions Hans Verkuil <hverkuil@xs4all.nl> - 2016-11-14 11:30 +0100
          Re: [PATCH v3 3/9] media: venus: adding core part and helper  functions Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2016-11-17 10:20 +0100

#1521459 — 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@linaro.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

[toc] | [next] | [standalone]

#1521462

On 11/14/2016 10:42 AM, Stanimir Varbanov wrote:
> 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@linaro.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.

Are you sure the buffers are released for both queues? I may have missed that when
reviewing.

I would recommend to call hfi_session_stop when the first stop_streaming is called,
not when it is called for both queues. I say this because stopping streaming without
releasing the buffers is likely to cause problems.

Did you turn on CONFIG_VIDEO_ADV_DEBUG? If it is on, and you don't release buffers
then I think you will see warnings in the kernel log.

Regards,

	Hans

[toc] | [prev] | [next] | [standalone]

#1521483

Hi Hans,

<cut>

>>>
>>>> +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.
> 
> Are you sure the buffers are released for both queues? I may have missed that when
> reviewing.

yes, hfi_session_stop will instruct the firmware to stop using provided
buffers and return ownership to the host driver by fill_buf_done and
empty_buf_done callbacks.

> 
> I would recommend to call hfi_session_stop when the first stop_streaming is called,
> not when it is called for both queues. I say this because stopping streaming without
> releasing the buffers is likely to cause problems.

this is what I tried to implement with above
!vb2_is_streaming(other_queue) thing.

> 
> Did you turn on CONFIG_VIDEO_ADV_DEBUG? If it is on, and you don't release buffers
> then I think you will see warnings in the kernel log.

OK I will enable it to be sure that warnings are missing.

-- 
regards,
Stan

[toc] | [prev] | [next] | [standalone]

#1521501

On 11/14/2016 11:11 AM, Stanimir Varbanov wrote:
> Hi Hans,
> 
> <cut>
> 
>>>>
>>>>> +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.
>>
>> Are you sure the buffers are released for both queues? I may have missed that when
>> reviewing.
> 
> yes, hfi_session_stop will instruct the firmware to stop using provided
> buffers and return ownership to the host driver by fill_buf_done and
> empty_buf_done callbacks.
> 
>>
>> I would recommend to call hfi_session_stop when the first stop_streaming is called,
>> not when it is called for both queues. I say this because stopping streaming without
>> releasing the buffers is likely to cause problems.
> 
> this is what I tried to implement with above
> !vb2_is_streaming(other_queue) thing.

That doesn't work: if the application calls STREAMON(CAPTURE) followed by STREAMOFF(CAPTURE)
without ever starting the OUTPUT queue, this will not clean up the capture queue.

Regards,

	Hans

> 
>>
>> Did you turn on CONFIG_VIDEO_ADV_DEBUG? If it is on, and you don't release buffers
>> then I think you will see warnings in the kernel log.
> 
> OK I will enable it to be sure that warnings are missing.
> 

[toc] | [prev] | [next] | [standalone]

#1524175

Hi Hans,

On 11/14/2016 12:25 PM, Hans Verkuil wrote:
> On 11/14/2016 11:11 AM, Stanimir Varbanov wrote:
>> Hi Hans,
>>
>> <cut>
>>
>>>>>
>>>>>> +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.
>>>
>>> Are you sure the buffers are released for both queues? I may have missed that when
>>> reviewing.
>>
>> yes, hfi_session_stop will instruct the firmware to stop using provided
>> buffers and return ownership to the host driver by fill_buf_done and
>> empty_buf_done callbacks.
>>
>>>
>>> I would recommend to call hfi_session_stop when the first stop_streaming is called,
>>> not when it is called for both queues. I say this because stopping streaming without
>>> releasing the buffers is likely to cause problems.
>>
>> this is what I tried to implement with above
>> !vb2_is_streaming(other_queue) thing.
> 
> That doesn't work: if the application calls STREAMON(CAPTURE) followed by STREAMOFF(CAPTURE)
> without ever starting the OUTPUT queue, this will not clean up the capture queue.

Yes this is a bug which should be fixed.

-- 
regards,
Stan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web