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

[Hans Verkuil]

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