diff mbox series

[1/3] venus: venc: Init the session only once in queue_setup

Message ID 20201120001037.10032-2-stanimir.varbanov@linaro.org
State New
Headers show
Series Venus encoder improvements | expand

Commit Message

Stanimir Varbanov Nov. 20, 2020, 12:10 a.m. UTC 
Init the hfi session only once in queue_setup and also cover that
with inst->lock.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

---
 drivers/media/platform/qcom/venus/venc.c | 98 ++++++++++++++++++------
 1 file changed, 73 insertions(+), 25 deletions(-)

-- 
2.17.1

Comments

Fritz Koenig Nov. 21, 2020, 1:33 a.m. UTC | #1 
On Thu, Nov 19, 2020 at 4:12 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>

> Init the hfi session only once in queue_setup and also cover that

> with inst->lock.

>

> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

> ---

>  drivers/media/platform/qcom/venus/venc.c | 98 ++++++++++++++++++------

>  1 file changed, 73 insertions(+), 25 deletions(-)

>

> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c

> index 4ecf78e30b59..3a2e449663d8 100644

> --- a/drivers/media/platform/qcom/venus/venc.c

> +++ b/drivers/media/platform/qcom/venus/venc.c

> @@ -725,8 +725,10 @@ static int venc_init_session(struct venus_inst *inst)

>         int ret;

>

>         ret = hfi_session_init(inst, inst->fmt_cap->pixfmt);

> -       if (ret)

> -               return ret;

> +       if (ret == -EINVAL)

> +               return 0;

> +       else if (ret)

> +               goto deinit;

>

>         ret = venus_helper_set_input_resolution(inst, inst->width,

>                                                 inst->height);

> @@ -762,17 +764,13 @@ static int venc_out_num_buffers(struct venus_inst *inst, unsigned int *num)

>         struct hfi_buffer_requirements bufreq;

>         int ret;

>

> -       ret = venc_init_session(inst);

> +       ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq);

>         if (ret)

>                 return ret;

>

> -       ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq);

> -

>         *num = bufreq.count_actual;

>

> -       hfi_session_deinit(inst);

> -

> -       return ret;

> +       return 0;

>  }

>

>  static int venc_queue_setup(struct vb2_queue *q,

> @@ -781,7 +779,7 @@ static int venc_queue_setup(struct vb2_queue *q,

>  {

>         struct venus_inst *inst = vb2_get_drv_priv(q);

>         unsigned int num, min = 4;

> -       int ret = 0;

> +       int ret;

>

>         if (*num_planes) {

>                 if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE &&

> @@ -803,6 +801,17 @@ static int venc_queue_setup(struct vb2_queue *q,

>                 return 0;

>         }

>

> +       ret = mutex_lock_interruptible(&inst->lock);

> +       if (ret)

> +               return ret;

> +

> +       ret = venc_init_session(inst);

> +

> +       mutex_unlock(&inst->lock);

> +

> +       if (ret)

> +               return ret;

> +

>         switch (q->type) {

>         case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:

>                 *num_planes = inst->fmt_out->num_planes;

> @@ -838,6 +847,54 @@ static int venc_queue_setup(struct vb2_queue *q,

>         return ret;

>  }

>

> +static int venc_buf_init(struct vb2_buffer *vb)

> +{

> +       struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);

> +

> +       inst->buf_count++;

> +

> +       return venus_helper_vb2_buf_init(vb);

> +}

> +

> +static void venc_release_session(struct venus_inst *inst)

> +{

> +       int ret, abort = 0;

> +

> +       mutex_lock(&inst->lock);

> +

> +       ret = hfi_session_deinit(inst);

> +       abort = (ret && ret != -EINVAL) ? 1 : 0;

> +

> +       if (inst->session_error)

> +               abort = 1;

> +

> +       if (abort)

> +               hfi_session_abort(inst);

> +

> +       mutex_unlock(&inst->lock);

> +

> +       venus_pm_load_scale(inst);

> +       INIT_LIST_HEAD(&inst->registeredbufs);

> +       venus_pm_release_core(inst);

> +}

> +

> +static void venc_buf_cleanup(struct vb2_buffer *vb)

> +{

> +       struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);

> +       struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);

> +       struct venus_buffer *buf = to_venus_buffer(vbuf);

> +

> +       mutex_lock(&inst->lock);

> +       if (vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)

> +               if (!list_empty(&inst->registeredbufs))

> +                       list_del_init(&buf->reg_list);

> +       mutex_unlock(&inst->lock);

> +

> +       inst->buf_count--;

> +       if (!inst->buf_count)

> +               venc_release_session(inst);

> +}

> +

>  static int venc_verify_conf(struct venus_inst *inst)

>  {

>         enum hfi_version ver = inst->core->res->hfi_version;

> @@ -888,38 +945,28 @@ static int venc_start_streaming(struct vb2_queue *q, unsigned int count)

>         inst->sequence_cap = 0;

>         inst->sequence_out = 0;

>

> -       ret = venc_init_session(inst);

> -       if (ret)

> -               goto bufs_done;

> -

>         ret = venus_pm_acquire_core(inst);

>         if (ret)

> -               goto deinit_sess;

> -

> -       ret = venc_set_properties(inst);

> -       if (ret)

> -               goto deinit_sess;

> +               goto error;

>

>         ret = venc_verify_conf(inst);

>         if (ret)

> -               goto deinit_sess;

> +               goto error;

>

>         ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs,

>                                         inst->num_output_bufs, 0);

>         if (ret)

> -               goto deinit_sess;

> +               goto error;

>

>         ret = venus_helper_vb2_start_streaming(inst);

>         if (ret)

> -               goto deinit_sess;

> +               goto error;

>

>         mutex_unlock(&inst->lock);

>

>         return 0;

>

> -deinit_sess:

> -       hfi_session_deinit(inst);

> -bufs_done:

> +error:

>         venus_helper_buffers_done(inst, q->type, VB2_BUF_STATE_QUEUED);

>         if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)

>                 inst->streamon_out = 0;

> @@ -940,7 +987,8 @@ static void venc_vb2_buf_queue(struct vb2_buffer *vb)

>

>  static const struct vb2_ops venc_vb2_ops = {

>         .queue_setup = venc_queue_setup,

> -       .buf_init = venus_helper_vb2_buf_init,

> +       .buf_init = venc_buf_init,

> +       .buf_cleanup = venc_buf_cleanup,

>         .buf_prepare = venus_helper_vb2_buf_prepare,

>         .start_streaming = venc_start_streaming,

>         .stop_streaming = venus_helper_vb2_stop_streaming,

> --

> 2.17.1

>


Reviewed-by: Fritz Koenig <frkoenig@chromium.org>
Alexandre Courbot Nov. 25, 2020, 3:13 a.m. UTC | #2 
Hi Stan,

On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Init the hfi session only once in queue_setup and also cover that
> with inst->lock.
>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/venc.c | 98 ++++++++++++++++++------
>  1 file changed, 73 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index 4ecf78e30b59..3a2e449663d8 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -725,8 +725,10 @@ static int venc_init_session(struct venus_inst *inst)
>         int ret;
>
>         ret = hfi_session_init(inst, inst->fmt_cap->pixfmt);
> -       if (ret)
> -               return ret;
> +       if (ret == -EINVAL)
> +               return 0;

Why is it safe to ignore EINVAL here?

> +       else if (ret)
> +               goto deinit;
>
>         ret = venus_helper_set_input_resolution(inst, inst->width,
>                                                 inst->height);
> @@ -762,17 +764,13 @@ static int venc_out_num_buffers(struct venus_inst *inst, unsigned int *num)
>         struct hfi_buffer_requirements bufreq;
>         int ret;
>
> -       ret = venc_init_session(inst);
> +       ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq);
>         if (ret)
>                 return ret;
>
> -       ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq);
> -
>         *num = bufreq.count_actual;
>
> -       hfi_session_deinit(inst);
> -
> -       return ret;
> +       return 0;
>  }
>
>  static int venc_queue_setup(struct vb2_queue *q,
> @@ -781,7 +779,7 @@ static int venc_queue_setup(struct vb2_queue *q,
>  {
>         struct venus_inst *inst = vb2_get_drv_priv(q);
>         unsigned int num, min = 4;
> -       int ret = 0;
> +       int ret;
>
>         if (*num_planes) {
>                 if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE &&
> @@ -803,6 +801,17 @@ static int venc_queue_setup(struct vb2_queue *q,
>                 return 0;
>         }
>
> +       ret = mutex_lock_interruptible(&inst->lock);
> +       if (ret)
> +               return ret;
> +
> +       ret = venc_init_session(inst);
> +
> +       mutex_unlock(&inst->lock);
> +
> +       if (ret)
> +               return ret;
> +
>         switch (q->type) {
>         case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>                 *num_planes = inst->fmt_out->num_planes;
> @@ -838,6 +847,54 @@ static int venc_queue_setup(struct vb2_queue *q,
>         return ret;
>  }
>
> +static int venc_buf_init(struct vb2_buffer *vb)
> +{
> +       struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
> +
> +       inst->buf_count++;
> +
> +       return venus_helper_vb2_buf_init(vb);
> +}
> +
> +static void venc_release_session(struct venus_inst *inst)
> +{
> +       int ret, abort = 0;
> +
> +       mutex_lock(&inst->lock);
> +
> +       ret = hfi_session_deinit(inst);
> +       abort = (ret && ret != -EINVAL) ? 1 : 0;

Here as well, I think a comment is warranted to explain why we can
ignore EINVAL.

> +
> +       if (inst->session_error)
> +               abort = 1;
> +
> +       if (abort)
> +               hfi_session_abort(inst);
> +
> +       mutex_unlock(&inst->lock);
> +
> +       venus_pm_load_scale(inst);
> +       INIT_LIST_HEAD(&inst->registeredbufs);
> +       venus_pm_release_core(inst);
> +}
> +
> +static void venc_buf_cleanup(struct vb2_buffer *vb)
> +{
> +       struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
> +       struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +       struct venus_buffer *buf = to_venus_buffer(vbuf);
> +
> +       mutex_lock(&inst->lock);
> +       if (vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> +               if (!list_empty(&inst->registeredbufs))
> +                       list_del_init(&buf->reg_list);
> +       mutex_unlock(&inst->lock);
> +
> +       inst->buf_count--;
> +       if (!inst->buf_count)
> +               venc_release_session(inst);

We are calling venc_init_session() during the queue setup but
venc_release_session() when the last buffer is cleaned up. For
symmetry, wouldn't it make sense to call venc_init_session() when the
first buffer is initialized by venc_buf_init()? Otherwise we can
potentially have a scenario where the queue is set up, but no buffer
is ever created, leading to the session never being released.

> +}
> +
>  static int venc_verify_conf(struct venus_inst *inst)
>  {
>         enum hfi_version ver = inst->core->res->hfi_version;
> @@ -888,38 +945,28 @@ static int venc_start_streaming(struct vb2_queue *q, unsigned int count)
>         inst->sequence_cap = 0;
>         inst->sequence_out = 0;
>
> -       ret = venc_init_session(inst);
> -       if (ret)
> -               goto bufs_done;
> -
>         ret = venus_pm_acquire_core(inst);
>         if (ret)
> -               goto deinit_sess;
> -
> -       ret = venc_set_properties(inst);
> -       if (ret)
> -               goto deinit_sess;
> +               goto error;
>
>         ret = venc_verify_conf(inst);
>         if (ret)
> -               goto deinit_sess;
> +               goto error;
>
>         ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs,
>                                         inst->num_output_bufs, 0);
>         if (ret)
> -               goto deinit_sess;
> +               goto error;
>
>         ret = venus_helper_vb2_start_streaming(inst);
>         if (ret)
> -               goto deinit_sess;
> +               goto error;
>
>         mutex_unlock(&inst->lock);
>
>         return 0;
>
> -deinit_sess:
> -       hfi_session_deinit(inst);
> -bufs_done:
> +error:
>         venus_helper_buffers_done(inst, q->type, VB2_BUF_STATE_QUEUED);
>         if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>                 inst->streamon_out = 0;
> @@ -940,7 +987,8 @@ static void venc_vb2_buf_queue(struct vb2_buffer *vb)
>
>  static const struct vb2_ops venc_vb2_ops = {
>         .queue_setup = venc_queue_setup,
> -       .buf_init = venus_helper_vb2_buf_init,
> +       .buf_init = venc_buf_init,
> +       .buf_cleanup = venc_buf_cleanup,
>         .buf_prepare = venus_helper_vb2_buf_prepare,
>         .start_streaming = venc_start_streaming,
>         .stop_streaming = venus_helper_vb2_stop_streaming,
> --
> 2.17.1
>
Stanimir Varbanov Nov. 26, 2020, 11:50 p.m. UTC | #3 
On 11/25/20 5:13 AM, Alexandre Courbot wrote:
> Hi Stan,
> 
> On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>>
>> Init the hfi session only once in queue_setup and also cover that
>> with inst->lock.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/venc.c | 98 ++++++++++++++++++------
>>  1 file changed, 73 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
>> index 4ecf78e30b59..3a2e449663d8 100644
>> --- a/drivers/media/platform/qcom/venus/venc.c
>> +++ b/drivers/media/platform/qcom/venus/venc.c
>> @@ -725,8 +725,10 @@ static int venc_init_session(struct venus_inst *inst)
>>         int ret;
>>
>>         ret = hfi_session_init(inst, inst->fmt_cap->pixfmt);
>> -       if (ret)
>> -               return ret;
>> +       if (ret == -EINVAL)
>> +               return 0;
> 
> Why is it safe to ignore EINVAL here?

The confusion comes from hfi_session_init() return values. Presently
hfi_session_init will return EINVAL when the session is already init.
Maybe EINVAL is not fitting well with the expected behavior of the
function. I thought about EALREADY, EBUSY but it doesn't fit well to me too.

> 
>> +       else if (ret)
>> +               goto deinit;
>>
>>         ret = venus_helper_set_input_resolution(inst, inst->width,
>>                                                 inst->height);
>> @@ -762,17 +764,13 @@ static int venc_out_num_buffers(struct venus_inst *inst, unsigned int *num)
>>         struct hfi_buffer_requirements bufreq;
>>         int ret;
>>
>> -       ret = venc_init_session(inst);
>> +       ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq);
>>         if (ret)
>>                 return ret;
>>
>> -       ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq);
>> -
>>         *num = bufreq.count_actual;
>>
>> -       hfi_session_deinit(inst);
>> -
>> -       return ret;
>> +       return 0;
>>  }
>>
>>  static int venc_queue_setup(struct vb2_queue *q,
>> @@ -781,7 +779,7 @@ static int venc_queue_setup(struct vb2_queue *q,
>>  {
>>         struct venus_inst *inst = vb2_get_drv_priv(q);
>>         unsigned int num, min = 4;
>> -       int ret = 0;
>> +       int ret;
>>
>>         if (*num_planes) {
>>                 if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE &&
>> @@ -803,6 +801,17 @@ static int venc_queue_setup(struct vb2_queue *q,
>>                 return 0;
>>         }
>>
>> +       ret = mutex_lock_interruptible(&inst->lock);

I'll keep original mutex_lock here in next version.

>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = venc_init_session(inst);
>> +
>> +       mutex_unlock(&inst->lock);
>> +
>> +       if (ret)
>> +               return ret;
>> +
>>         switch (q->type) {
>>         case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>>                 *num_planes = inst->fmt_out->num_planes;
>> @@ -838,6 +847,54 @@ static int venc_queue_setup(struct vb2_queue *q,
>>         return ret;
>>  }
>>
>> +static int venc_buf_init(struct vb2_buffer *vb)
>> +{
>> +       struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
>> +
>> +       inst->buf_count++;
>> +
>> +       return venus_helper_vb2_buf_init(vb);
>> +}
>> +
>> +static void venc_release_session(struct venus_inst *inst)
>> +{
>> +       int ret, abort = 0;
>> +
>> +       mutex_lock(&inst->lock);
>> +
>> +       ret = hfi_session_deinit(inst);
>> +       abort = (ret && ret != -EINVAL) ? 1 : 0;
> 
> Here as well, I think a comment is warranted to explain why we can
> ignore EINVAL.

OK, will update that.

> 
>> +
>> +       if (inst->session_error)
>> +               abort = 1;
>> +
>> +       if (abort)
>> +               hfi_session_abort(inst);
>> +
>> +       mutex_unlock(&inst->lock);
>> +
>> +       venus_pm_load_scale(inst);
>> +       INIT_LIST_HEAD(&inst->registeredbufs);
>> +       venus_pm_release_core(inst);
>> +}
>> +
>> +static void venc_buf_cleanup(struct vb2_buffer *vb)
>> +{
>> +       struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
>> +       struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> +       struct venus_buffer *buf = to_venus_buffer(vbuf);
>> +
>> +       mutex_lock(&inst->lock);
>> +       if (vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
>> +               if (!list_empty(&inst->registeredbufs))
>> +                       list_del_init(&buf->reg_list);
>> +       mutex_unlock(&inst->lock);
>> +
>> +       inst->buf_count--;
>> +       if (!inst->buf_count)
>> +               venc_release_session(inst);
> 
> We are calling venc_init_session() during the queue setup but
> venc_release_session() when the last buffer is cleaned up. For
> symmetry, wouldn't it make sense to call venc_init_session() when the
> first buffer is initialized by venc_buf_init()? Otherwise we can

No, the session must be initialized in queue_setup in order to return
the number and sizes of source/destination buffers.

I raised several times the need of symmetrical operation to queue_setup
to cover reqbuf(0) but there is no progress on that. Latest suggestion
was to use .vidioc_reqbufs ioctl op but I fall with some other issues
and at the end I came to this counting buf_init|cleanup solution.

> potentially have a scenario where the queue is set up, but no buffer
> is ever created, leading to the session never being released.

dmabuf import case?

<cut>
Dikshita Agarwal Dec. 1, 2020, 12:22 p.m. UTC | #4 
Hi Stan,

On 2020-11-20 05:40, Stanimir Varbanov wrote:
> Init the hfi session only once in queue_setup and also cover that
> with inst->lock.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/venc.c | 98 ++++++++++++++++++------
>  1 file changed, 73 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/venc.c
> b/drivers/media/platform/qcom/venus/venc.c
> index 4ecf78e30b59..3a2e449663d8 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -725,8 +725,10 @@ static int venc_init_session(struct venus_inst 
> *inst)
>  	int ret;
> 
>  	ret = hfi_session_init(inst, inst->fmt_cap->pixfmt);
> -	if (ret)
> -		return ret;
> +	if (ret == -EINVAL)
> +		return 0;
> +	else if (ret)
> +		goto deinit;
> 
>  	ret = venus_helper_set_input_resolution(inst, inst->width,
>  						inst->height);
> @@ -762,17 +764,13 @@ static int venc_out_num_buffers(struct
> venus_inst *inst, unsigned int *num)
>  	struct hfi_buffer_requirements bufreq;
>  	int ret;
> 
> -	ret = venc_init_session(inst);
> +	ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq);
>  	if (ret)
>  		return ret;
> 
> -	ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq);
> -
>  	*num = bufreq.count_actual;
> 
> -	hfi_session_deinit(inst);
> -
> -	return ret;
> +	return 0;
>  }
> 
>  static int venc_queue_setup(struct vb2_queue *q,
> @@ -781,7 +779,7 @@ static int venc_queue_setup(struct vb2_queue *q,
>  {
>  	struct venus_inst *inst = vb2_get_drv_priv(q);
>  	unsigned int num, min = 4;
> -	int ret = 0;
> +	int ret;
> 
>  	if (*num_planes) {
>  		if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE &&
> @@ -803,6 +801,17 @@ static int venc_queue_setup(struct vb2_queue *q,
>  		return 0;
>  	}
> 
> +	ret = mutex_lock_interruptible(&inst->lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = venc_init_session(inst);
> +
> +	mutex_unlock(&inst->lock);
> +
> +	if (ret)
> +		return ret;
> +
>  	switch (q->type) {
>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>  		*num_planes = inst->fmt_out->num_planes;
> @@ -838,6 +847,54 @@ static int venc_queue_setup(struct vb2_queue *q,
>  	return ret;
>  }
> 
> +static int venc_buf_init(struct vb2_buffer *vb)
> +{
> +	struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
> +
> +	inst->buf_count++;
> +
> +	return venus_helper_vb2_buf_init(vb);
> +}
> +
> +static void venc_release_session(struct venus_inst *inst)
> +{
> +	int ret, abort = 0;
> +
> +	mutex_lock(&inst->lock);
> +
> +	ret = hfi_session_deinit(inst);
> +	abort = (ret && ret != -EINVAL) ? 1 : 0;
> +
> +	if (inst->session_error)
> +		abort = 1;
> +
> +	if (abort)
> +		hfi_session_abort(inst);
> +
> +	mutex_unlock(&inst->lock);
> +
> +	venus_pm_load_scale(inst);
> +	INIT_LIST_HEAD(&inst->registeredbufs);
> +	venus_pm_release_core(inst);
> +}
> +
> +static void venc_buf_cleanup(struct vb2_buffer *vb)
> +{
> +	struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	struct venus_buffer *buf = to_venus_buffer(vbuf);
> +
> +	mutex_lock(&inst->lock);
> +	if (vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> +		if (!list_empty(&inst->registeredbufs))
> +			list_del_init(&buf->reg_list);
> +	mutex_unlock(&inst->lock);
> +
> +	inst->buf_count--;
> +	if (!inst->buf_count)
> +		venc_release_session(inst);
> +}
> +
>  static int venc_verify_conf(struct venus_inst *inst)
>  {
>  	enum hfi_version ver = inst->core->res->hfi_version;
> @@ -888,38 +945,28 @@ static int venc_start_streaming(struct vb2_queue
> *q, unsigned int count)
>  	inst->sequence_cap = 0;
>  	inst->sequence_out = 0;
> 
> -	ret = venc_init_session(inst);
> -	if (ret)
> -		goto bufs_done;
> -
>  	ret = venus_pm_acquire_core(inst);
>  	if (ret)
> -		goto deinit_sess;
> -
> -	ret = venc_set_properties(inst);
> -	if (ret)
> -		goto deinit_sess;

With this change, if set ctrl for target bitrate is called after queue 
setup and before streaming,
the new bitrate won’t be set to FW. which is not right and can cause 
quality issues.
The same might apply to other encoder parameters as well.
Please fix this in the next version.

> +		goto error;
> 
>  	ret = venc_verify_conf(inst);
>  	if (ret)
> -		goto deinit_sess;
> +		goto error;
> 
>  	ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs,
>  					inst->num_output_bufs, 0);
>  	if (ret)
> -		goto deinit_sess;
> +		goto error;
> 
>  	ret = venus_helper_vb2_start_streaming(inst);
>  	if (ret)
> -		goto deinit_sess;
> +		goto error;
> 
>  	mutex_unlock(&inst->lock);
> 
>  	return 0;
> 
> -deinit_sess:
> -	hfi_session_deinit(inst);
> -bufs_done:
> +error:
>  	venus_helper_buffers_done(inst, q->type, VB2_BUF_STATE_QUEUED);
>  	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>  		inst->streamon_out = 0;
> @@ -940,7 +987,8 @@ static void venc_vb2_buf_queue(struct vb2_buffer 
> *vb)
> 
>  static const struct vb2_ops venc_vb2_ops = {
>  	.queue_setup = venc_queue_setup,
> -	.buf_init = venus_helper_vb2_buf_init,
> +	.buf_init = venc_buf_init,
> +	.buf_cleanup = venc_buf_cleanup,
>  	.buf_prepare = venus_helper_vb2_buf_prepare,
>  	.start_streaming = venc_start_streaming,
>  	.stop_streaming = venus_helper_vb2_stop_streaming,
diff mbox series

Patch

diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 4ecf78e30b59..3a2e449663d8 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -725,8 +725,10 @@  static int venc_init_session(struct venus_inst *inst)
 	int ret;
 
 	ret = hfi_session_init(inst, inst->fmt_cap->pixfmt);
-	if (ret)
-		return ret;
+	if (ret == -EINVAL)
+		return 0;
+	else if (ret)
+		goto deinit;
 
 	ret = venus_helper_set_input_resolution(inst, inst->width,
 						inst->height);
@@ -762,17 +764,13 @@  static int venc_out_num_buffers(struct venus_inst *inst, unsigned int *num)
 	struct hfi_buffer_requirements bufreq;
 	int ret;
 
-	ret = venc_init_session(inst);
+	ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq);
 	if (ret)
 		return ret;
 
-	ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq);
-
 	*num = bufreq.count_actual;
 
-	hfi_session_deinit(inst);
-
-	return ret;
+	return 0;
 }
 
 static int venc_queue_setup(struct vb2_queue *q,
@@ -781,7 +779,7 @@  static int venc_queue_setup(struct vb2_queue *q,
 {
 	struct venus_inst *inst = vb2_get_drv_priv(q);
 	unsigned int num, min = 4;
-	int ret = 0;
+	int ret;
 
 	if (*num_planes) {
 		if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE &&
@@ -803,6 +801,17 @@  static int venc_queue_setup(struct vb2_queue *q,
 		return 0;
 	}
 
+	ret = mutex_lock_interruptible(&inst->lock);
+	if (ret)
+		return ret;
+
+	ret = venc_init_session(inst);
+
+	mutex_unlock(&inst->lock);
+
+	if (ret)
+		return ret;
+
 	switch (q->type) {
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
 		*num_planes = inst->fmt_out->num_planes;
@@ -838,6 +847,54 @@  static int venc_queue_setup(struct vb2_queue *q,
 	return ret;
 }
 
+static int venc_buf_init(struct vb2_buffer *vb)
+{
+	struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
+
+	inst->buf_count++;
+
+	return venus_helper_vb2_buf_init(vb);
+}
+
+static void venc_release_session(struct venus_inst *inst)
+{
+	int ret, abort = 0;
+
+	mutex_lock(&inst->lock);
+
+	ret = hfi_session_deinit(inst);
+	abort = (ret && ret != -EINVAL) ? 1 : 0;
+
+	if (inst->session_error)
+		abort = 1;
+
+	if (abort)
+		hfi_session_abort(inst);
+
+	mutex_unlock(&inst->lock);
+
+	venus_pm_load_scale(inst);
+	INIT_LIST_HEAD(&inst->registeredbufs);
+	venus_pm_release_core(inst);
+}
+
+static void venc_buf_cleanup(struct vb2_buffer *vb)
+{
+	struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
+	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+	struct venus_buffer *buf = to_venus_buffer(vbuf);
+
+	mutex_lock(&inst->lock);
+	if (vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+		if (!list_empty(&inst->registeredbufs))
+			list_del_init(&buf->reg_list);
+	mutex_unlock(&inst->lock);
+
+	inst->buf_count--;
+	if (!inst->buf_count)
+		venc_release_session(inst);
+}
+
 static int venc_verify_conf(struct venus_inst *inst)
 {
 	enum hfi_version ver = inst->core->res->hfi_version;
@@ -888,38 +945,28 @@  static int venc_start_streaming(struct vb2_queue *q, unsigned int count)
 	inst->sequence_cap = 0;
 	inst->sequence_out = 0;
 
-	ret = venc_init_session(inst);
-	if (ret)
-		goto bufs_done;
-
 	ret = venus_pm_acquire_core(inst);
 	if (ret)
-		goto deinit_sess;
-
-	ret = venc_set_properties(inst);
-	if (ret)
-		goto deinit_sess;
+		goto error;
 
 	ret = venc_verify_conf(inst);
 	if (ret)
-		goto deinit_sess;
+		goto error;
 
 	ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs,
 					inst->num_output_bufs, 0);
 	if (ret)
-		goto deinit_sess;
+		goto error;
 
 	ret = venus_helper_vb2_start_streaming(inst);
 	if (ret)
-		goto deinit_sess;
+		goto error;
 
 	mutex_unlock(&inst->lock);
 
 	return 0;
 
-deinit_sess:
-	hfi_session_deinit(inst);
-bufs_done:
+error:
 	venus_helper_buffers_done(inst, q->type, VB2_BUF_STATE_QUEUED);
 	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
 		inst->streamon_out = 0;
@@ -940,7 +987,8 @@  static void venc_vb2_buf_queue(struct vb2_buffer *vb)
 
 static const struct vb2_ops venc_vb2_ops = {
 	.queue_setup = venc_queue_setup,
-	.buf_init = venus_helper_vb2_buf_init,
+	.buf_init = venc_buf_init,
+	.buf_cleanup = venc_buf_cleanup,
 	.buf_prepare = venus_helper_vb2_buf_prepare,
 	.start_streaming = venc_start_streaming,
 	.stop_streaming = venus_helper_vb2_stop_streaming,