[PATCH 0/3] Venus encoder improvements

[PATCH 0/3] Venus encoder improvements

[Stanimir Varbanov]

Hello,

Here is a series which aims to improve encoder staility.

 * 1/3 makes encoder init session to be issued only once.
 * 2/3 refuse to open the encoders/decoders session beyond the
   limit
 * 3/3 forces firmware to raise soft interrupt when acknowledge
   a synchronous command 

Comments are welcome!

regards,
Stan

Stanimir Varbanov (2):
  venus: venc: Init the session only once in queue_setup
  venus: Limit HFI sessions to the maximum supported

Vikash Garodia (1):
  media: hfi_venus: Request interrupt for sync cmds

 drivers/media/platform/qcom/venus/core.h      |  1 +
 drivers/media/platform/qcom/venus/hfi.c       | 19 +++-
 .../media/platform/qcom/venus/hfi_parser.c    |  3 +
 drivers/media/platform/qcom/venus/hfi_venus.c | 74 +++++++-------
 drivers/media/platform/qcom/venus/venc.c      | 98 ++++++++++++++-----
 5 files changed, 133 insertions(+), 62 deletions(-)

-- 
2.17.1

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

[Stanimir Varbanov]

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

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

[Fritz Koenig]

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>

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

[Alexandre Courbot]

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
>

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

[Stanimir Varbanov]

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>

-- 
regards,
Stan

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

[dikshita]

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,

[PATCH 2/3] venus: Limit HFI sessions to the maximum supported

[Stanimir Varbanov]

Currently we rely on firmware to return error when we reach the maximum
supported number of sessions. But this errors are happened at reqbuf
time which is a bit later. The more reasonable way looks like is to
return the error on driver open.

To achieve that modify hfi_session_create to return error when we reach
maximum count of sessions and thus refuse open.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/core.h      |  1 +
 drivers/media/platform/qcom/venus/hfi.c       | 19 +++++++++++++++----
 .../media/platform/qcom/venus/hfi_parser.c    |  3 +++
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index db0e6738281e..3a477fcdd3a8 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -96,6 +96,7 @@ struct venus_format {
 #define MAX_CAP_ENTRIES		32
 #define MAX_ALLOC_MODE_ENTRIES	16
 #define MAX_CODEC_NUM		32
+#define MAX_SESSIONS		16
 
 struct raw_formats {
 	u32 buftype;
diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c
index 638ed5cfe05e..8420be6d3991 100644
--- a/drivers/media/platform/qcom/venus/hfi.c
+++ b/drivers/media/platform/qcom/venus/hfi.c
@@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst)
 int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
 {
 	struct venus_core *core = inst->core;
+	int ret;
 
 	if (!ops)
 		return -EINVAL;
@@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
 	init_completion(&inst->done);
 	inst->ops = ops;
 
-	mutex_lock(&core->lock);
-	list_add_tail(&inst->list, &core->instances);
-	atomic_inc(&core->insts_count);
+	ret = mutex_lock_interruptible(&core->lock);
+	if (ret)
+		return ret;
+
+	ret = atomic_read(&core->insts_count);
+	if (ret + 1 > core->max_sessions_supported) {
+		ret = -EAGAIN;
+	} else {
+		atomic_inc(&core->insts_count);
+		list_add_tail(&inst->list, &core->instances);
+		ret = 0;
+	}
+
 	mutex_unlock(&core->lock);
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(hfi_session_create);
 
diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
index 363ee2a65453..52898633a8e6 100644
--- a/drivers/media/platform/qcom/venus/hfi_parser.c
+++ b/drivers/media/platform/qcom/venus/hfi_parser.c
@@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
 		words_count--;
 	}
 
+	if (!core->max_sessions_supported)
+		core->max_sessions_supported = MAX_SESSIONS;
+
 	parser_fini(inst, codecs, domain);
 
 	return HFI_ERR_NONE;
-- 
2.17.1

Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported

[Fritz Koenig]

On Thu, Nov 19, 2020 at 4:12 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Currently we rely on firmware to return error when we reach the maximum
> supported number of sessions. But this errors are happened at reqbuf
> time which is a bit later. The more reasonable way looks like is to
> return the error on driver open.
>
> To achieve that modify hfi_session_create to return error when we reach
> maximum count of sessions and thus refuse open.
>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.h      |  1 +
>  drivers/media/platform/qcom/venus/hfi.c       | 19 +++++++++++++++----
>  .../media/platform/qcom/venus/hfi_parser.c    |  3 +++
>  3 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index db0e6738281e..3a477fcdd3a8 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -96,6 +96,7 @@ struct venus_format {
>  #define MAX_CAP_ENTRIES                32
>  #define MAX_ALLOC_MODE_ENTRIES 16
>  #define MAX_CODEC_NUM          32
> +#define MAX_SESSIONS           16
>
>  struct raw_formats {
>         u32 buftype;
> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c
> index 638ed5cfe05e..8420be6d3991 100644
> --- a/drivers/media/platform/qcom/venus/hfi.c
> +++ b/drivers/media/platform/qcom/venus/hfi.c
> @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst)
>  int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
>  {
>         struct venus_core *core = inst->core;
> +       int ret;
>
>         if (!ops)
>                 return -EINVAL;
> @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
>         init_completion(&inst->done);
>         inst->ops = ops;
>
> -       mutex_lock(&core->lock);
> -       list_add_tail(&inst->list, &core->instances);
> -       atomic_inc(&core->insts_count);
> +       ret = mutex_lock_interruptible(&core->lock);
> +       if (ret)
> +               return ret;
> +
> +       ret = atomic_read(&core->insts_count);
> +       if (ret + 1 > core->max_sessions_supported) {
> +               ret = -EAGAIN;
> +       } else {
> +               atomic_inc(&core->insts_count);
> +               list_add_tail(&inst->list, &core->instances);
> +               ret = 0;
> +       }
> +
>         mutex_unlock(&core->lock);
>
> -       return 0;
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(hfi_session_create);
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
> index 363ee2a65453..52898633a8e6 100644
> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>                 words_count--;
>         }
>

My understanding of the hardware is that there is a max number of
macroblocks that can be worked on at a time.  That works out to
nominally 16 clips.  But large clips can take more resources.  Does
|max_sessions_supported| get updated with the amount that system can
use?  Or is it always a constant?

If it changes depending on system load, then couldn't
|core->max_sessions_supported| be 0 if all of the resources have been
used up?  If that is the case then the below check would appear to be
incorrect.

> +       if (!core->max_sessions_supported)
> +               core->max_sessions_supported = MAX_SESSIONS;
> +
>         parser_fini(inst, codecs, domain);
>
>         return HFI_ERR_NONE;
> --
> 2.17.1
>

Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported

[Stanimir Varbanov]

On 11/21/20 3:14 AM, Fritz Koenig wrote:
> On Thu, Nov 19, 2020 at 4:12 PM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>>
>> Currently we rely on firmware to return error when we reach the maximum
>> supported number of sessions. But this errors are happened at reqbuf
>> time which is a bit later. The more reasonable way looks like is to
>> return the error on driver open.
>>
>> To achieve that modify hfi_session_create to return error when we reach
>> maximum count of sessions and thus refuse open.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/core.h      |  1 +
>>  drivers/media/platform/qcom/venus/hfi.c       | 19 +++++++++++++++----
>>  .../media/platform/qcom/venus/hfi_parser.c    |  3 +++
>>  3 files changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
>> index db0e6738281e..3a477fcdd3a8 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -96,6 +96,7 @@ struct venus_format {
>>  #define MAX_CAP_ENTRIES                32
>>  #define MAX_ALLOC_MODE_ENTRIES 16
>>  #define MAX_CODEC_NUM          32
>> +#define MAX_SESSIONS           16
>>
>>  struct raw_formats {
>>         u32 buftype;
>> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c
>> index 638ed5cfe05e..8420be6d3991 100644
>> --- a/drivers/media/platform/qcom/venus/hfi.c
>> +++ b/drivers/media/platform/qcom/venus/hfi.c
>> @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst)
>>  int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
>>  {
>>         struct venus_core *core = inst->core;
>> +       int ret;
>>
>>         if (!ops)
>>                 return -EINVAL;
>> @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
>>         init_completion(&inst->done);
>>         inst->ops = ops;
>>
>> -       mutex_lock(&core->lock);
>> -       list_add_tail(&inst->list, &core->instances);
>> -       atomic_inc(&core->insts_count);
>> +       ret = mutex_lock_interruptible(&core->lock);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = atomic_read(&core->insts_count);
>> +       if (ret + 1 > core->max_sessions_supported) {
>> +               ret = -EAGAIN;
>> +       } else {
>> +               atomic_inc(&core->insts_count);
>> +               list_add_tail(&inst->list, &core->instances);
>> +               ret = 0;
>> +       }
>> +
>>         mutex_unlock(&core->lock);
>>
>> -       return 0;
>> +       return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(hfi_session_create);
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
>> index 363ee2a65453..52898633a8e6 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
>> @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>>                 words_count--;
>>         }
>>
> 
> My understanding of the hardware is that there is a max number of
> macroblocks that can be worked on at a time.  That works out to
> nominally 16 clips.  But large clips can take more resources.  Does
> |max_sessions_supported| get updated with the amount that system can
> use?  Or is it always a constant?

The number of max sessions supported is constant.

> 
> If it changes depending on system load, then couldn't
> |core->max_sessions_supported| be 0 if all of the resources have been
> used up?  If that is the case then the below check would appear to be
> incorrect.

No, this is not the case. Changing dynamically the number of max
sessions depending on session load is possible but it would be complex
to implement. For example, think of decoder dynamic resolution change
where we don't know in advance the new resolution (session load).

> 
>> +       if (!core->max_sessions_supported)
>> +               core->max_sessions_supported = MAX_SESSIONS;
>> +
>>         parser_fini(inst, codecs, domain);
>>
>>         return HFI_ERR_NONE;
>> --
>> 2.17.1
>>

-- 
regards,
Stan

Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported

[Fritz Koenig]

On Sun, Nov 22, 2020 at 6:48 AM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
>
>
> On 11/21/20 3:14 AM, Fritz Koenig wrote:
> > On Thu, Nov 19, 2020 at 4:12 PM Stanimir Varbanov
> > <stanimir.varbanov@linaro.org> wrote:
> >>
> >> Currently we rely on firmware to return error when we reach the maximum
> >> supported number of sessions. But this errors are happened at reqbuf
> >> time which is a bit later. The more reasonable way looks like is to
> >> return the error on driver open.
> >>
> >> To achieve that modify hfi_session_create to return error when we reach
> >> maximum count of sessions and thus refuse open.
> >>
> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> >> ---
> >>  drivers/media/platform/qcom/venus/core.h      |  1 +
> >>  drivers/media/platform/qcom/venus/hfi.c       | 19 +++++++++++++++----
> >>  .../media/platform/qcom/venus/hfi_parser.c    |  3 +++
> >>  3 files changed, 19 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> >> index db0e6738281e..3a477fcdd3a8 100644
> >> --- a/drivers/media/platform/qcom/venus/core.h
> >> +++ b/drivers/media/platform/qcom/venus/core.h
> >> @@ -96,6 +96,7 @@ struct venus_format {
> >>  #define MAX_CAP_ENTRIES                32
> >>  #define MAX_ALLOC_MODE_ENTRIES 16
> >>  #define MAX_CODEC_NUM          32
> >> +#define MAX_SESSIONS           16
> >>
> >>  struct raw_formats {
> >>         u32 buftype;
> >> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c
> >> index 638ed5cfe05e..8420be6d3991 100644
> >> --- a/drivers/media/platform/qcom/venus/hfi.c
> >> +++ b/drivers/media/platform/qcom/venus/hfi.c
> >> @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst)
> >>  int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
> >>  {
> >>         struct venus_core *core = inst->core;
> >> +       int ret;
> >>
> >>         if (!ops)
> >>                 return -EINVAL;
> >> @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
> >>         init_completion(&inst->done);
> >>         inst->ops = ops;
> >>
> >> -       mutex_lock(&core->lock);
> >> -       list_add_tail(&inst->list, &core->instances);
> >> -       atomic_inc(&core->insts_count);
> >> +       ret = mutex_lock_interruptible(&core->lock);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       ret = atomic_read(&core->insts_count);
> >> +       if (ret + 1 > core->max_sessions_supported) {
> >> +               ret = -EAGAIN;
> >> +       } else {
> >> +               atomic_inc(&core->insts_count);
> >> +               list_add_tail(&inst->list, &core->instances);
> >> +               ret = 0;
> >> +       }
> >> +
> >>         mutex_unlock(&core->lock);
> >>
> >> -       return 0;
> >> +       return ret;
> >>  }
> >>  EXPORT_SYMBOL_GPL(hfi_session_create);
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
> >> index 363ee2a65453..52898633a8e6 100644
> >> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> >> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> >> @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
> >>                 words_count--;
> >>         }
> >>
> >
> > My understanding of the hardware is that there is a max number of
> > macroblocks that can be worked on at a time.  That works out to
> > nominally 16 clips.  But large clips can take more resources.  Does
> > |max_sessions_supported| get updated with the amount that system can
> > use?  Or is it always a constant?
>
> The number of max sessions supported is constant.
>
> >
> > If it changes depending on system load, then couldn't
> > be 0 if all of the resources have been
> > used up?  If that is the case then the below check would appear to be
> > incorrect.
>
> No, this is not the case. Changing dynamically the number of max
> sessions depending on session load is possible but it would be complex
> to implement. For example, think of decoder dynamic resolution change
> where we don't know in advance the new resolution (session load).
>

Sorry, I should have been more specific.  The complexity of
dynamically changing the max sessions did not seem to be captured in
the patch, so I wanted to make sure that
|core->max_sessions_supported| was constant.  As it is constant, then
this patch looks to capture everything.

Reviewed-by: Fritz Koenig <frkoenig@chromium.org>
> >
> >> +       if (!core->max_sessions_supported)
> >> +               core->max_sessions_supported = MAX_SESSIONS;
> >> +
> >>         parser_fini(inst, codecs, domain);
> >>
> >>         return HFI_ERR_NONE;
> >> --
> >> 2.17.1
> >>
>
> --
> regards,
> Stan

Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported

[Alexandre Courbot]

On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Currently we rely on firmware to return error when we reach the maximum
> supported number of sessions. But this errors are happened at reqbuf
> time which is a bit later. The more reasonable way looks like is to
> return the error on driver open.
>
> To achieve that modify hfi_session_create to return error when we reach
> maximum count of sessions and thus refuse open.
>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.h      |  1 +
>  drivers/media/platform/qcom/venus/hfi.c       | 19 +++++++++++++++----
>  .../media/platform/qcom/venus/hfi_parser.c    |  3 +++
>  3 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index db0e6738281e..3a477fcdd3a8 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -96,6 +96,7 @@ struct venus_format {
>  #define MAX_CAP_ENTRIES                32
>  #define MAX_ALLOC_MODE_ENTRIES 16
>  #define MAX_CODEC_NUM          32
> +#define MAX_SESSIONS           16
>
>  struct raw_formats {
>         u32 buftype;
> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c
> index 638ed5cfe05e..8420be6d3991 100644
> --- a/drivers/media/platform/qcom/venus/hfi.c
> +++ b/drivers/media/platform/qcom/venus/hfi.c
> @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst)
>  int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
>  {
>         struct venus_core *core = inst->core;
> +       int ret;
>
>         if (!ops)
>                 return -EINVAL;
> @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
>         init_completion(&inst->done);
>         inst->ops = ops;
>
> -       mutex_lock(&core->lock);
> -       list_add_tail(&inst->list, &core->instances);
> -       atomic_inc(&core->insts_count);
> +       ret = mutex_lock_interruptible(&core->lock);
> +       if (ret)
> +               return ret;

Why do we change to mutex_lock_interruptible() here? This makes this
function return an error even though we could obtain the lock just by
trying a bit harder.

> +
> +       ret = atomic_read(&core->insts_count);
> +       if (ret + 1 > core->max_sessions_supported) {
> +               ret = -EAGAIN;
> +       } else {
> +               atomic_inc(&core->insts_count);
> +               list_add_tail(&inst->list, &core->instances);
> +               ret = 0;
> +       }
> +
>         mutex_unlock(&core->lock);
>
> -       return 0;
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(hfi_session_create);
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
> index 363ee2a65453..52898633a8e6 100644
> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>                 words_count--;
>         }
>
> +       if (!core->max_sessions_supported)
> +               core->max_sessions_supported = MAX_SESSIONS;
> +
>         parser_fini(inst, codecs, domain);
>
>         return HFI_ERR_NONE;
> --
> 2.17.1
>

Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported

[Stanimir Varbanov]

On 11/25/20 5:46 AM, Alexandre Courbot wrote:
> On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>>
>> Currently we rely on firmware to return error when we reach the maximum
>> supported number of sessions. But this errors are happened at reqbuf
>> time which is a bit later. The more reasonable way looks like is to
>> return the error on driver open.
>>
>> To achieve that modify hfi_session_create to return error when we reach
>> maximum count of sessions and thus refuse open.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/core.h      |  1 +
>>  drivers/media/platform/qcom/venus/hfi.c       | 19 +++++++++++++++----
>>  .../media/platform/qcom/venus/hfi_parser.c    |  3 +++
>>  3 files changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
>> index db0e6738281e..3a477fcdd3a8 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -96,6 +96,7 @@ struct venus_format {
>>  #define MAX_CAP_ENTRIES                32
>>  #define MAX_ALLOC_MODE_ENTRIES 16
>>  #define MAX_CODEC_NUM          32
>> +#define MAX_SESSIONS           16
>>
>>  struct raw_formats {
>>         u32 buftype;
>> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c
>> index 638ed5cfe05e..8420be6d3991 100644
>> --- a/drivers/media/platform/qcom/venus/hfi.c
>> +++ b/drivers/media/platform/qcom/venus/hfi.c
>> @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst)
>>  int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
>>  {
>>         struct venus_core *core = inst->core;
>> +       int ret;
>>
>>         if (!ops)
>>                 return -EINVAL;
>> @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
>>         init_completion(&inst->done);
>>         inst->ops = ops;
>>
>> -       mutex_lock(&core->lock);
>> -       list_add_tail(&inst->list, &core->instances);
>> -       atomic_inc(&core->insts_count);
>> +       ret = mutex_lock_interruptible(&core->lock);
>> +       if (ret)
>> +               return ret;
> 
> Why do we change to mutex_lock_interruptible() here? This makes this

Because mutex_lock_interruptible is preferable in kernel docs, but I
agree that changing mutex_lock with mutex_lock_interruptible should be
subject of another lock related patches. I will drop this in next patch
version.

> function return an error even though we could obtain the lock just by
> trying a bit harder.

I didn't get that. The behavior of mutex_lock_interruptible is that same
as mutex_lock, i.e. the it will sleep to acquire the lock. The
difference is that the sleep could be interrupted by a signal. You might
think about mutex_trylock?

> 
>> +
>> +       ret = atomic_read(&core->insts_count);
>> +       if (ret + 1 > core->max_sessions_supported) {
>> +               ret = -EAGAIN;
>> +       } else {
>> +               atomic_inc(&core->insts_count);
>> +               list_add_tail(&inst->list, &core->instances);
>> +               ret = 0;
>> +       }
>> +
>>         mutex_unlock(&core->lock);
>>
>> -       return 0;
>> +       return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(hfi_session_create);
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
>> index 363ee2a65453..52898633a8e6 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
>> @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>>                 words_count--;
>>         }
>>
>> +       if (!core->max_sessions_supported)
>> +               core->max_sessions_supported = MAX_SESSIONS;
>> +
>>         parser_fini(inst, codecs, domain);
>>
>>         return HFI_ERR_NONE;
>> --
>> 2.17.1
>>

-- 
regards,
Stan

Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported

[Alexandre Courbot]

On Wed, Nov 25, 2020 at 10:01 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
>
>
> On 11/25/20 5:46 AM, Alexandre Courbot wrote:
> > On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov
> > <stanimir.varbanov@linaro.org> wrote:
> >>
> >> Currently we rely on firmware to return error when we reach the maximum
> >> supported number of sessions. But this errors are happened at reqbuf
> >> time which is a bit later. The more reasonable way looks like is to
> >> return the error on driver open.
> >>
> >> To achieve that modify hfi_session_create to return error when we reach
> >> maximum count of sessions and thus refuse open.
> >>
> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> >> ---
> >>  drivers/media/platform/qcom/venus/core.h      |  1 +
> >>  drivers/media/platform/qcom/venus/hfi.c       | 19 +++++++++++++++----
> >>  .../media/platform/qcom/venus/hfi_parser.c    |  3 +++
> >>  3 files changed, 19 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> >> index db0e6738281e..3a477fcdd3a8 100644
> >> --- a/drivers/media/platform/qcom/venus/core.h
> >> +++ b/drivers/media/platform/qcom/venus/core.h
> >> @@ -96,6 +96,7 @@ struct venus_format {
> >>  #define MAX_CAP_ENTRIES                32
> >>  #define MAX_ALLOC_MODE_ENTRIES 16
> >>  #define MAX_CODEC_NUM          32
> >> +#define MAX_SESSIONS           16
> >>
> >>  struct raw_formats {
> >>         u32 buftype;
> >> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c
> >> index 638ed5cfe05e..8420be6d3991 100644
> >> --- a/drivers/media/platform/qcom/venus/hfi.c
> >> +++ b/drivers/media/platform/qcom/venus/hfi.c
> >> @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst)
> >>  int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
> >>  {
> >>         struct venus_core *core = inst->core;
> >> +       int ret;
> >>
> >>         if (!ops)
> >>                 return -EINVAL;
> >> @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
> >>         init_completion(&inst->done);
> >>         inst->ops = ops;
> >>
> >> -       mutex_lock(&core->lock);
> >> -       list_add_tail(&inst->list, &core->instances);
> >> -       atomic_inc(&core->insts_count);
> >> +       ret = mutex_lock_interruptible(&core->lock);
> >> +       if (ret)
> >> +               return ret;
> >
> > Why do we change to mutex_lock_interruptible() here? This makes this
>
> Because mutex_lock_interruptible is preferable in kernel docs, but I
> agree that changing mutex_lock with mutex_lock_interruptible should be
> subject of another lock related patches. I will drop this in next patch
> version.
>
> > function return an error even though we could obtain the lock just by
> > trying a bit harder.
>
> I didn't get that. The behavior of mutex_lock_interruptible is that same
> as mutex_lock, i.e. the it will sleep to acquire the lock. The
> difference is that the sleep could be interrupted by a signal. You might
> think about mutex_trylock?

Unless that mutex can be held by someone else for a rather long time
(i.e. to the point where we may want to give priority to signals when
userspace opens the device, since that's where hfi_session_create() is
called), I am not convinced this change is necessary? It may confuse
userspace into thinking there was a serious error while there is none.
Granted, userspace should manage this case, and from what I can see
this code is correct, but I'm not sure we would gain anything by
adding this extra complexity.

>
> >
> >> +
> >> +       ret = atomic_read(&core->insts_count);
> >> +       if (ret + 1 > core->max_sessions_supported) {
> >> +               ret = -EAGAIN;
> >> +       } else {
> >> +               atomic_inc(&core->insts_count);
> >> +               list_add_tail(&inst->list, &core->instances);
> >> +               ret = 0;
> >> +       }
> >> +
> >>         mutex_unlock(&core->lock);
> >>
> >> -       return 0;
> >> +       return ret;
> >>  }
> >>  EXPORT_SYMBOL_GPL(hfi_session_create);
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
> >> index 363ee2a65453..52898633a8e6 100644
> >> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> >> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> >> @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
> >>                 words_count--;
> >>         }
> >>
> >> +       if (!core->max_sessions_supported)
> >> +               core->max_sessions_supported = MAX_SESSIONS;
> >> +
> >>         parser_fini(inst, codecs, domain);
> >>
> >>         return HFI_ERR_NONE;
> >> --
> >> 2.17.1
> >>
>
> --
> regards,
> Stan

Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported

[Stanimir Varbanov]

On 11/26/20 8:28 AM, Alexandre Courbot wrote:
> On Wed, Nov 25, 2020 at 10:01 PM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>>
>>
>>
>> On 11/25/20 5:46 AM, Alexandre Courbot wrote:
>>> On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov
>>> <stanimir.varbanov@linaro.org> wrote:
>>>>
>>>> Currently we rely on firmware to return error when we reach the maximum
>>>> supported number of sessions. But this errors are happened at reqbuf
>>>> time which is a bit later. The more reasonable way looks like is to
>>>> return the error on driver open.
>>>>
>>>> To achieve that modify hfi_session_create to return error when we reach
>>>> maximum count of sessions and thus refuse open.
>>>>
>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>>> ---
>>>>  drivers/media/platform/qcom/venus/core.h      |  1 +
>>>>  drivers/media/platform/qcom/venus/hfi.c       | 19 +++++++++++++++----
>>>>  .../media/platform/qcom/venus/hfi_parser.c    |  3 +++
>>>>  3 files changed, 19 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
>>>> index db0e6738281e..3a477fcdd3a8 100644
>>>> --- a/drivers/media/platform/qcom/venus/core.h
>>>> +++ b/drivers/media/platform/qcom/venus/core.h
>>>> @@ -96,6 +96,7 @@ struct venus_format {
>>>>  #define MAX_CAP_ENTRIES                32
>>>>  #define MAX_ALLOC_MODE_ENTRIES 16
>>>>  #define MAX_CODEC_NUM          32
>>>> +#define MAX_SESSIONS           16
>>>>
>>>>  struct raw_formats {
>>>>         u32 buftype;
>>>> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c
>>>> index 638ed5cfe05e..8420be6d3991 100644
>>>> --- a/drivers/media/platform/qcom/venus/hfi.c
>>>> +++ b/drivers/media/platform/qcom/venus/hfi.c
>>>> @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst)
>>>>  int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
>>>>  {
>>>>         struct venus_core *core = inst->core;
>>>> +       int ret;
>>>>
>>>>         if (!ops)
>>>>                 return -EINVAL;
>>>> @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
>>>>         init_completion(&inst->done);
>>>>         inst->ops = ops;
>>>>
>>>> -       mutex_lock(&core->lock);
>>>> -       list_add_tail(&inst->list, &core->instances);
>>>> -       atomic_inc(&core->insts_count);
>>>> +       ret = mutex_lock_interruptible(&core->lock);
>>>> +       if (ret)
>>>> +               return ret;
>>>
>>> Why do we change to mutex_lock_interruptible() here? This makes this
>>
>> Because mutex_lock_interruptible is preferable in kernel docs, but I
>> agree that changing mutex_lock with mutex_lock_interruptible should be
>> subject of another lock related patches. I will drop this in next patch
>> version.
>>
>>> function return an error even though we could obtain the lock just by
>>> trying a bit harder.
>>
>> I didn't get that. The behavior of mutex_lock_interruptible is that same
>> as mutex_lock, i.e. the it will sleep to acquire the lock. The
>> difference is that the sleep could be interrupted by a signal. You might
>> think about mutex_trylock?
> 
> Unless that mutex can be held by someone else for a rather long time
> (i.e. to the point where we may want to give priority to signals when
> userspace opens the device, since that's where hfi_session_create() is
> called), I am not convinced this change is necessary? It may confuse

Exactly, if there is a case where the core->lock is taken (firmware
recovery) and it is not unlocked for very long time (deadlock?) then
client process cannot be interrupted with a signal.

> userspace into thinking there was a serious error while there is none.

The client should be able to handle EINTR, right?

> Granted, userspace should manage this case, and from what I can see
> this code is correct, but I'm not sure we would gain anything by
> adding this extra complexity.

The benefit is that if something wrong is happening in the driver the
client process will be killable.

> 
>>
>>>
>>>> +
>>>> +       ret = atomic_read(&core->insts_count);
>>>> +       if (ret + 1 > core->max_sessions_supported) {
>>>> +               ret = -EAGAIN;
>>>> +       } else {
>>>> +               atomic_inc(&core->insts_count);
>>>> +               list_add_tail(&inst->list, &core->instances);
>>>> +               ret = 0;
>>>> +       }
>>>> +
>>>>         mutex_unlock(&core->lock);
>>>>
>>>> -       return 0;
>>>> +       return ret;
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(hfi_session_create);
>>>>
>>>> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
>>>> index 363ee2a65453..52898633a8e6 100644
>>>> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
>>>> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
>>>> @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>>>>                 words_count--;
>>>>         }
>>>>
>>>> +       if (!core->max_sessions_supported)
>>>> +               core->max_sessions_supported = MAX_SESSIONS;
>>>> +
>>>>         parser_fini(inst, codecs, domain);
>>>>
>>>>         return HFI_ERR_NONE;
>>>> --
>>>> 2.17.1
>>>>
>>
>> --
>> regards,
>> Stan

-- 
regards,
Stan

Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported

[Alexandre Courbot]

On Fri, Nov 27, 2020 at 7:42 AM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
>
>
> On 11/26/20 8:28 AM, Alexandre Courbot wrote:
> > On Wed, Nov 25, 2020 at 10:01 PM Stanimir Varbanov
> > <stanimir.varbanov@linaro.org> wrote:
> >>
> >>
> >>
> >> On 11/25/20 5:46 AM, Alexandre Courbot wrote:
> >>> On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov
> >>> <stanimir.varbanov@linaro.org> wrote:
> >>>>
> >>>> Currently we rely on firmware to return error when we reach the maximum
> >>>> supported number of sessions. But this errors are happened at reqbuf
> >>>> time which is a bit later. The more reasonable way looks like is to
> >>>> return the error on driver open.
> >>>>
> >>>> To achieve that modify hfi_session_create to return error when we reach
> >>>> maximum count of sessions and thus refuse open.
> >>>>
> >>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> >>>> ---
> >>>>  drivers/media/platform/qcom/venus/core.h      |  1 +
> >>>>  drivers/media/platform/qcom/venus/hfi.c       | 19 +++++++++++++++----
> >>>>  .../media/platform/qcom/venus/hfi_parser.c    |  3 +++
> >>>>  3 files changed, 19 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> >>>> index db0e6738281e..3a477fcdd3a8 100644
> >>>> --- a/drivers/media/platform/qcom/venus/core.h
> >>>> +++ b/drivers/media/platform/qcom/venus/core.h
> >>>> @@ -96,6 +96,7 @@ struct venus_format {
> >>>>  #define MAX_CAP_ENTRIES                32
> >>>>  #define MAX_ALLOC_MODE_ENTRIES 16
> >>>>  #define MAX_CODEC_NUM          32
> >>>> +#define MAX_SESSIONS           16
> >>>>
> >>>>  struct raw_formats {
> >>>>         u32 buftype;
> >>>> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c
> >>>> index 638ed5cfe05e..8420be6d3991 100644
> >>>> --- a/drivers/media/platform/qcom/venus/hfi.c
> >>>> +++ b/drivers/media/platform/qcom/venus/hfi.c
> >>>> @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst)
> >>>>  int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
> >>>>  {
> >>>>         struct venus_core *core = inst->core;
> >>>> +       int ret;
> >>>>
> >>>>         if (!ops)
> >>>>                 return -EINVAL;
> >>>> @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
> >>>>         init_completion(&inst->done);
> >>>>         inst->ops = ops;
> >>>>
> >>>> -       mutex_lock(&core->lock);
> >>>> -       list_add_tail(&inst->list, &core->instances);
> >>>> -       atomic_inc(&core->insts_count);
> >>>> +       ret = mutex_lock_interruptible(&core->lock);
> >>>> +       if (ret)
> >>>> +               return ret;
> >>>
> >>> Why do we change to mutex_lock_interruptible() here? This makes this
> >>
> >> Because mutex_lock_interruptible is preferable in kernel docs, but I
> >> agree that changing mutex_lock with mutex_lock_interruptible should be
> >> subject of another lock related patches. I will drop this in next patch
> >> version.
> >>
> >>> function return an error even though we could obtain the lock just by
> >>> trying a bit harder.
> >>
> >> I didn't get that. The behavior of mutex_lock_interruptible is that same
> >> as mutex_lock, i.e. the it will sleep to acquire the lock. The
> >> difference is that the sleep could be interrupted by a signal. You might
> >> think about mutex_trylock?
> >
> > Unless that mutex can be held by someone else for a rather long time
> > (i.e. to the point where we may want to give priority to signals when
> > userspace opens the device, since that's where hfi_session_create() is
> > called), I am not convinced this change is necessary? It may confuse
>
> Exactly, if there is a case where the core->lock is taken (firmware
> recovery) and it is not unlocked for very long time (deadlock?) then
> client process cannot be interrupted with a signal.
>
> > userspace into thinking there was a serious error while there is none.
>
> The client should be able to handle EINTR, right?
>
> > Granted, userspace should manage this case, and from what I can see
> > this code is correct, but I'm not sure we would gain anything by
> > adding this extra complexity.
>
> The benefit is that if something wrong is happening in the driver the
> client process will be killable.

Ack, that definitely makes sense in that context, even though it
should probably be done separately from this patch series. :)

Cheers,
Alex.

[PATCH 3/3] media: hfi_venus: Request interrupt for sync cmds

[Stanimir Varbanov]

From: Vikash Garodia <vgarodia@codeaurora.org>

For synchronous commands, update the message queue variable.
This would inform video firmware to raise interrupt on host
CPU whenever there is a response for such commands.

Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 74 ++++++++++---------
 1 file changed, 41 insertions(+), 33 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index 4be4a75ddcb6..b8fdb464ba9c 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -372,7 +372,7 @@ static void venus_soft_int(struct venus_hfi_device *hdev)
 }
 
 static int venus_iface_cmdq_write_nolock(struct venus_hfi_device *hdev,
-					 void *pkt)
+					 void *pkt, bool sync)
 {
 	struct device *dev = hdev->core->dev;
 	struct hfi_pkt_hdr *cmd_packet;
@@ -397,15 +397,23 @@ static int venus_iface_cmdq_write_nolock(struct venus_hfi_device *hdev,
 	if (rx_req)
 		venus_soft_int(hdev);
 
+	/* Inform video firmware to raise interrupt for synchronous commands */
+	queue = &hdev->queues[IFACEQ_MSG_IDX];
+	if (sync) {
+		queue->qhdr->rx_req = 1;
+		/* ensure rx_req is updated in memory */
+		wmb();
+	}
+
 	return 0;
 }
 
-static int venus_iface_cmdq_write(struct venus_hfi_device *hdev, void *pkt)
+static int venus_iface_cmdq_write(struct venus_hfi_device *hdev, void *pkt, bool sync)
 {
 	int ret;
 
 	mutex_lock(&hdev->lock);
-	ret = venus_iface_cmdq_write_nolock(hdev, pkt);
+	ret = venus_iface_cmdq_write_nolock(hdev, pkt, sync);
 	mutex_unlock(&hdev->lock);
 
 	return ret;
@@ -428,7 +436,7 @@ static int venus_hfi_core_set_resource(struct venus_core *core, u32 id,
 	if (ret)
 		return ret;
 
-	ret = venus_iface_cmdq_write(hdev, pkt);
+	ret = venus_iface_cmdq_write(hdev, pkt, false);
 	if (ret)
 		return ret;
 
@@ -778,7 +786,7 @@ static int venus_sys_set_debug(struct venus_hfi_device *hdev, u32 debug)
 
 	pkt_sys_debug_config(pkt, HFI_DEBUG_MODE_QUEUE, debug);
 
-	ret = venus_iface_cmdq_write(hdev, pkt);
+	ret = venus_iface_cmdq_write(hdev, pkt, false);
 	if (ret)
 		return ret;
 
@@ -795,7 +803,7 @@ static int venus_sys_set_coverage(struct venus_hfi_device *hdev, u32 mode)
 
 	pkt_sys_coverage_config(pkt, mode);
 
-	ret = venus_iface_cmdq_write(hdev, pkt);
+	ret = venus_iface_cmdq_write(hdev, pkt, false);
 	if (ret)
 		return ret;
 
@@ -816,7 +824,7 @@ static int venus_sys_set_idle_message(struct venus_hfi_device *hdev,
 
 	pkt_sys_idle_indicator(pkt, enable);
 
-	ret = venus_iface_cmdq_write(hdev, pkt);
+	ret = venus_iface_cmdq_write(hdev, pkt, false);
 	if (ret)
 		return ret;
 
@@ -834,7 +842,7 @@ static int venus_sys_set_power_control(struct venus_hfi_device *hdev,
 
 	pkt_sys_power_control(pkt, enable);
 
-	ret = venus_iface_cmdq_write(hdev, pkt);
+	ret = venus_iface_cmdq_write(hdev, pkt, false);
 	if (ret)
 		return ret;
 
@@ -885,14 +893,14 @@ static int venus_sys_set_default_properties(struct venus_hfi_device *hdev)
 	return ret;
 }
 
-static int venus_session_cmd(struct venus_inst *inst, u32 pkt_type)
+static int venus_session_cmd(struct venus_inst *inst, u32 pkt_type, bool sync)
 {
 	struct venus_hfi_device *hdev = to_hfi_priv(inst->core);
 	struct hfi_session_pkt pkt;
 
 	pkt_session_cmd(&pkt, pkt_type, inst);
 
-	return venus_iface_cmdq_write(hdev, &pkt);
+	return venus_iface_cmdq_write(hdev, &pkt, sync);
 }
 
 static void venus_flush_debug_queue(struct venus_hfi_device *hdev)
@@ -922,7 +930,7 @@ static int venus_prepare_power_collapse(struct venus_hfi_device *hdev,
 
 	pkt_sys_pc_prep(&pkt);
 
-	ret = venus_iface_cmdq_write(hdev, &pkt);
+	ret = venus_iface_cmdq_write(hdev, &pkt, false);
 	if (ret)
 		return ret;
 
@@ -1064,13 +1072,13 @@ static int venus_core_init(struct venus_core *core)
 
 	venus_set_state(hdev, VENUS_STATE_INIT);
 
-	ret = venus_iface_cmdq_write(hdev, &pkt);
+	ret = venus_iface_cmdq_write(hdev, &pkt, false);
 	if (ret)
 		return ret;
 
 	pkt_sys_image_version(&version_pkt);
 
-	ret = venus_iface_cmdq_write(hdev, &version_pkt);
+	ret = venus_iface_cmdq_write(hdev, &version_pkt, false);
 	if (ret)
 		dev_warn(dev, "failed to send image version pkt to fw\n");
 
@@ -1099,7 +1107,7 @@ static int venus_core_ping(struct venus_core *core, u32 cookie)
 
 	pkt_sys_ping(&pkt, cookie);
 
-	return venus_iface_cmdq_write(hdev, &pkt);
+	return venus_iface_cmdq_write(hdev, &pkt, false);
 }
 
 static int venus_core_trigger_ssr(struct venus_core *core, u32 trigger_type)
@@ -1112,7 +1120,7 @@ static int venus_core_trigger_ssr(struct venus_core *core, u32 trigger_type)
 	if (ret)
 		return ret;
 
-	return venus_iface_cmdq_write(hdev, &pkt);
+	return venus_iface_cmdq_write(hdev, &pkt, false);
 }
 
 static int venus_session_init(struct venus_inst *inst, u32 session_type,
@@ -1130,7 +1138,7 @@ static int venus_session_init(struct venus_inst *inst, u32 session_type,
 	if (ret)
 		goto err;
 
-	ret = venus_iface_cmdq_write(hdev, &pkt);
+	ret = venus_iface_cmdq_write(hdev, &pkt, true);
 	if (ret)
 		goto err;
 
@@ -1151,7 +1159,7 @@ static int venus_session_end(struct venus_inst *inst)
 			dev_warn(dev, "fw coverage msg ON failed\n");
 	}
 
-	return venus_session_cmd(inst, HFI_CMD_SYS_SESSION_END);
+	return venus_session_cmd(inst, HFI_CMD_SYS_SESSION_END, true);
 }
 
 static int venus_session_abort(struct venus_inst *inst)
@@ -1160,7 +1168,7 @@ static int venus_session_abort(struct venus_inst *inst)
 
 	venus_flush_debug_queue(hdev);
 
-	return venus_session_cmd(inst, HFI_CMD_SYS_SESSION_ABORT);
+	return venus_session_cmd(inst, HFI_CMD_SYS_SESSION_ABORT, true);
 }
 
 static int venus_session_flush(struct venus_inst *inst, u32 flush_mode)
@@ -1173,22 +1181,22 @@ static int venus_session_flush(struct venus_inst *inst, u32 flush_mode)
 	if (ret)
 		return ret;
 
-	return venus_iface_cmdq_write(hdev, &pkt);
+	return venus_iface_cmdq_write(hdev, &pkt, true);
 }
 
 static int venus_session_start(struct venus_inst *inst)
 {
-	return venus_session_cmd(inst, HFI_CMD_SESSION_START);
+	return venus_session_cmd(inst, HFI_CMD_SESSION_START, true);
 }
 
 static int venus_session_stop(struct venus_inst *inst)
 {
-	return venus_session_cmd(inst, HFI_CMD_SESSION_STOP);
+	return venus_session_cmd(inst, HFI_CMD_SESSION_STOP, true);
 }
 
 static int venus_session_continue(struct venus_inst *inst)
 {
-	return venus_session_cmd(inst, HFI_CMD_SESSION_CONTINUE);
+	return venus_session_cmd(inst, HFI_CMD_SESSION_CONTINUE, false);
 }
 
 static int venus_session_etb(struct venus_inst *inst,
@@ -1205,7 +1213,7 @@ static int venus_session_etb(struct venus_inst *inst,
 		if (ret)
 			return ret;
 
-		ret = venus_iface_cmdq_write(hdev, &pkt);
+		ret = venus_iface_cmdq_write(hdev, &pkt, false);
 	} else if (session_type == VIDC_SESSION_TYPE_ENC) {
 		struct hfi_session_empty_buffer_uncompressed_plane0_pkt pkt;
 
@@ -1213,7 +1221,7 @@ static int venus_session_etb(struct venus_inst *inst,
 		if (ret)
 			return ret;
 
-		ret = venus_iface_cmdq_write(hdev, &pkt);
+		ret = venus_iface_cmdq_write(hdev, &pkt, false);
 	} else {
 		ret = -EINVAL;
 	}
@@ -1232,7 +1240,7 @@ static int venus_session_ftb(struct venus_inst *inst,
 	if (ret)
 		return ret;
 
-	return venus_iface_cmdq_write(hdev, &pkt);
+	return venus_iface_cmdq_write(hdev, &pkt, false);
 }
 
 static int venus_session_set_buffers(struct venus_inst *inst,
@@ -1252,7 +1260,7 @@ static int venus_session_set_buffers(struct venus_inst *inst,
 	if (ret)
 		return ret;
 
-	return venus_iface_cmdq_write(hdev, pkt);
+	return venus_iface_cmdq_write(hdev, pkt, false);
 }
 
 static int venus_session_unset_buffers(struct venus_inst *inst,
@@ -1272,17 +1280,17 @@ static int venus_session_unset_buffers(struct venus_inst *inst,
 	if (ret)
 		return ret;
 
-	return venus_iface_cmdq_write(hdev, pkt);
+	return venus_iface_cmdq_write(hdev, pkt, true);
 }
 
 static int venus_session_load_res(struct venus_inst *inst)
 {
-	return venus_session_cmd(inst, HFI_CMD_SESSION_LOAD_RESOURCES);
+	return venus_session_cmd(inst, HFI_CMD_SESSION_LOAD_RESOURCES, true);
 }
 
 static int venus_session_release_res(struct venus_inst *inst)
 {
-	return venus_session_cmd(inst, HFI_CMD_SESSION_RELEASE_RESOURCES);
+	return venus_session_cmd(inst, HFI_CMD_SESSION_RELEASE_RESOURCES, true);
 }
 
 static int venus_session_parse_seq_hdr(struct venus_inst *inst, u32 seq_hdr,
@@ -1299,7 +1307,7 @@ static int venus_session_parse_seq_hdr(struct venus_inst *inst, u32 seq_hdr,
 	if (ret)
 		return ret;
 
-	ret = venus_iface_cmdq_write(hdev, pkt);
+	ret = venus_iface_cmdq_write(hdev, pkt, false);
 	if (ret)
 		return ret;
 
@@ -1320,7 +1328,7 @@ static int venus_session_get_seq_hdr(struct venus_inst *inst, u32 seq_hdr,
 	if (ret)
 		return ret;
 
-	return venus_iface_cmdq_write(hdev, pkt);
+	return venus_iface_cmdq_write(hdev, pkt, false);
 }
 
 static int venus_session_set_property(struct venus_inst *inst, u32 ptype,
@@ -1339,7 +1347,7 @@ static int venus_session_set_property(struct venus_inst *inst, u32 ptype,
 	if (ret)
 		return ret;
 
-	return venus_iface_cmdq_write(hdev, pkt);
+	return venus_iface_cmdq_write(hdev, pkt, false);
 }
 
 static int venus_session_get_property(struct venus_inst *inst, u32 ptype)
@@ -1352,7 +1360,7 @@ static int venus_session_get_property(struct venus_inst *inst, u32 ptype)
 	if (ret)
 		return ret;
 
-	return venus_iface_cmdq_write(hdev, &pkt);
+	return venus_iface_cmdq_write(hdev, &pkt, true);
 }
 
 static int venus_resume(struct venus_core *core)
-- 
2.17.1

Re: [PATCH 3/3] media: hfi_venus: Request interrupt for sync cmds

[Fritz Koenig]

On Thu, Nov 19, 2020 at 4:12 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> From: Vikash Garodia <vgarodia@codeaurora.org>
>
> For synchronous commands, update the message queue variable.
> This would inform video firmware to raise interrupt on host
> CPU whenever there is a response for such commands.
>
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/hfi_venus.c | 74 ++++++++++---------
>  1 file changed, 41 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index 4be4a75ddcb6..b8fdb464ba9c 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -372,7 +372,7 @@ static void venus_soft_int(struct venus_hfi_device *hdev)
>  }
>
>  static int venus_iface_cmdq_write_nolock(struct venus_hfi_device *hdev,
> -                                        void *pkt)
> +                                        void *pkt, bool sync)
>  {
>         struct device *dev = hdev->core->dev;
>         struct hfi_pkt_hdr *cmd_packet;
> @@ -397,15 +397,23 @@ static int venus_iface_cmdq_write_nolock(struct venus_hfi_device *hdev,
>         if (rx_req)
>                 venus_soft_int(hdev);
>
> +       /* Inform video firmware to raise interrupt for synchronous commands */
> +       queue = &hdev->queues[IFACEQ_MSG_IDX];

I don't think there is any reason to scope queue outside of  the sync
block below.

>
> +       if (sync) {
> +               queue->qhdr->rx_req = 1;
> +               /* ensure rx_req is updated in memory */
> +               wmb();
> +       }
> +
>         return 0;
>  }
>
> -static int venus_iface_cmdq_write(struct venus_hfi_device *hdev, void *pkt)
> +static int venus_iface_cmdq_write(struct venus_hfi_device *hdev, void *pkt, bool sync)
>  {
>         int ret;
>
>         mutex_lock(&hdev->lock);
> -       ret = venus_iface_cmdq_write_nolock(hdev, pkt);
> +       ret = venus_iface_cmdq_write_nolock(hdev, pkt, sync);
>         mutex_unlock(&hdev->lock);
>
>         return ret;
> @@ -428,7 +436,7 @@ static int venus_hfi_core_set_resource(struct venus_core *core, u32 id,
>         if (ret)
>                 return ret;
>
> -       ret = venus_iface_cmdq_write(hdev, pkt);
> +       ret = venus_iface_cmdq_write(hdev, pkt, false);
>         if (ret)
>                 return ret;
>
> @@ -778,7 +786,7 @@ static int venus_sys_set_debug(struct venus_hfi_device *hdev, u32 debug)
>
>         pkt_sys_debug_config(pkt, HFI_DEBUG_MODE_QUEUE, debug);
>
> -       ret = venus_iface_cmdq_write(hdev, pkt);
> +       ret = venus_iface_cmdq_write(hdev, pkt, false);
>         if (ret)
>                 return ret;
>
> @@ -795,7 +803,7 @@ static int venus_sys_set_coverage(struct venus_hfi_device *hdev, u32 mode)
>
>         pkt_sys_coverage_config(pkt, mode);
>
> -       ret = venus_iface_cmdq_write(hdev, pkt);
> +       ret = venus_iface_cmdq_write(hdev, pkt, false);
>         if (ret)
>                 return ret;
>
> @@ -816,7 +824,7 @@ static int venus_sys_set_idle_message(struct venus_hfi_device *hdev,
>
>         pkt_sys_idle_indicator(pkt, enable);
>
> -       ret = venus_iface_cmdq_write(hdev, pkt);
> +       ret = venus_iface_cmdq_write(hdev, pkt, false);
>         if (ret)
>                 return ret;
>
> @@ -834,7 +842,7 @@ static int venus_sys_set_power_control(struct venus_hfi_device *hdev,
>
>         pkt_sys_power_control(pkt, enable);
>
> -       ret = venus_iface_cmdq_write(hdev, pkt);
> +       ret = venus_iface_cmdq_write(hdev, pkt, false);
>         if (ret)
>                 return ret;
>
> @@ -885,14 +893,14 @@ static int venus_sys_set_default_properties(struct venus_hfi_device *hdev)
>         return ret;
>  }
>
> -static int venus_session_cmd(struct venus_inst *inst, u32 pkt_type)
> +static int venus_session_cmd(struct venus_inst *inst, u32 pkt_type, bool sync)
>  {
>         struct venus_hfi_device *hdev = to_hfi_priv(inst->core);
>         struct hfi_session_pkt pkt;
>
>         pkt_session_cmd(&pkt, pkt_type, inst);
>
> -       return venus_iface_cmdq_write(hdev, &pkt);
> +       return venus_iface_cmdq_write(hdev, &pkt, sync);
>  }
>
>  static void venus_flush_debug_queue(struct venus_hfi_device *hdev)
> @@ -922,7 +930,7 @@ static int venus_prepare_power_collapse(struct venus_hfi_device *hdev,
>
>         pkt_sys_pc_prep(&pkt);
>
> -       ret = venus_iface_cmdq_write(hdev, &pkt);
> +       ret = venus_iface_cmdq_write(hdev, &pkt, false);
>         if (ret)
>                 return ret;
>
> @@ -1064,13 +1072,13 @@ static int venus_core_init(struct venus_core *core)
>
>         venus_set_state(hdev, VENUS_STATE_INIT);
>
> -       ret = venus_iface_cmdq_write(hdev, &pkt);
> +       ret = venus_iface_cmdq_write(hdev, &pkt, false);
>         if (ret)
>                 return ret;
>
>         pkt_sys_image_version(&version_pkt);
>
> -       ret = venus_iface_cmdq_write(hdev, &version_pkt);
> +       ret = venus_iface_cmdq_write(hdev, &version_pkt, false);
>         if (ret)
>                 dev_warn(dev, "failed to send image version pkt to fw\n");
>
> @@ -1099,7 +1107,7 @@ static int venus_core_ping(struct venus_core *core, u32 cookie)
>
>         pkt_sys_ping(&pkt, cookie);
>
> -       return venus_iface_cmdq_write(hdev, &pkt);
> +       return venus_iface_cmdq_write(hdev, &pkt, false);
>  }
>
>  static int venus_core_trigger_ssr(struct venus_core *core, u32 trigger_type)
> @@ -1112,7 +1120,7 @@ static int venus_core_trigger_ssr(struct venus_core *core, u32 trigger_type)
>         if (ret)
>                 return ret;
>
> -       return venus_iface_cmdq_write(hdev, &pkt);
> +       return venus_iface_cmdq_write(hdev, &pkt, false);
>  }
>
>  static int venus_session_init(struct venus_inst *inst, u32 session_type,
> @@ -1130,7 +1138,7 @@ static int venus_session_init(struct venus_inst *inst, u32 session_type,
>         if (ret)
>                 goto err;
>
> -       ret = venus_iface_cmdq_write(hdev, &pkt);
> +       ret = venus_iface_cmdq_write(hdev, &pkt, true);
>         if (ret)
>                 goto err;
>
> @@ -1151,7 +1159,7 @@ static int venus_session_end(struct venus_inst *inst)
>                         dev_warn(dev, "fw coverage msg ON failed\n");
>         }
>
> -       return venus_session_cmd(inst, HFI_CMD_SYS_SESSION_END);
> +       return venus_session_cmd(inst, HFI_CMD_SYS_SESSION_END, true);
>  }
>
>  static int venus_session_abort(struct venus_inst *inst)
> @@ -1160,7 +1168,7 @@ static int venus_session_abort(struct venus_inst *inst)
>
>         venus_flush_debug_queue(hdev);
>
> -       return venus_session_cmd(inst, HFI_CMD_SYS_SESSION_ABORT);
> +       return venus_session_cmd(inst, HFI_CMD_SYS_SESSION_ABORT, true);
>  }
>
>  static int venus_session_flush(struct venus_inst *inst, u32 flush_mode)
> @@ -1173,22 +1181,22 @@ static int venus_session_flush(struct venus_inst *inst, u32 flush_mode)
>         if (ret)
>                 return ret;
>
> -       return venus_iface_cmdq_write(hdev, &pkt);
> +       return venus_iface_cmdq_write(hdev, &pkt, true);
>  }
>
>  static int venus_session_start(struct venus_inst *inst)
>  {
> -       return venus_session_cmd(inst, HFI_CMD_SESSION_START);
> +       return venus_session_cmd(inst, HFI_CMD_SESSION_START, true);
>  }
>
>  static int venus_session_stop(struct venus_inst *inst)
>  {
> -       return venus_session_cmd(inst, HFI_CMD_SESSION_STOP);
> +       return venus_session_cmd(inst, HFI_CMD_SESSION_STOP, true);
>  }
>
>  static int venus_session_continue(struct venus_inst *inst)
>  {
> -       return venus_session_cmd(inst, HFI_CMD_SESSION_CONTINUE);
> +       return venus_session_cmd(inst, HFI_CMD_SESSION_CONTINUE, false);
>  }
>
>  static int venus_session_etb(struct venus_inst *inst,
> @@ -1205,7 +1213,7 @@ static int venus_session_etb(struct venus_inst *inst,
>                 if (ret)
>                         return ret;
>
> -               ret = venus_iface_cmdq_write(hdev, &pkt);
> +               ret = venus_iface_cmdq_write(hdev, &pkt, false);
>         } else if (session_type == VIDC_SESSION_TYPE_ENC) {
>                 struct hfi_session_empty_buffer_uncompressed_plane0_pkt pkt;
>
> @@ -1213,7 +1221,7 @@ static int venus_session_etb(struct venus_inst *inst,
>                 if (ret)
>                         return ret;
>
> -               ret = venus_iface_cmdq_write(hdev, &pkt);
> +               ret = venus_iface_cmdq_write(hdev, &pkt, false);
>         } else {
>                 ret = -EINVAL;
>         }
> @@ -1232,7 +1240,7 @@ static int venus_session_ftb(struct venus_inst *inst,
>         if (ret)
>                 return ret;
>
> -       return venus_iface_cmdq_write(hdev, &pkt);
> +       return venus_iface_cmdq_write(hdev, &pkt, false);
>  }
>
>  static int venus_session_set_buffers(struct venus_inst *inst,
> @@ -1252,7 +1260,7 @@ static int venus_session_set_buffers(struct venus_inst *inst,
>         if (ret)
>                 return ret;
>
> -       return venus_iface_cmdq_write(hdev, pkt);
> +       return venus_iface_cmdq_write(hdev, pkt, false);
>  }
>
>  static int venus_session_unset_buffers(struct venus_inst *inst,
> @@ -1272,17 +1280,17 @@ static int venus_session_unset_buffers(struct venus_inst *inst,
>         if (ret)
>                 return ret;
>
> -       return venus_iface_cmdq_write(hdev, pkt);
> +       return venus_iface_cmdq_write(hdev, pkt, true);
>  }
>
>  static int venus_session_load_res(struct venus_inst *inst)
>  {
> -       return venus_session_cmd(inst, HFI_CMD_SESSION_LOAD_RESOURCES);
> +       return venus_session_cmd(inst, HFI_CMD_SESSION_LOAD_RESOURCES, true);
>  }
>
>  static int venus_session_release_res(struct venus_inst *inst)
>  {
> -       return venus_session_cmd(inst, HFI_CMD_SESSION_RELEASE_RESOURCES);
> +       return venus_session_cmd(inst, HFI_CMD_SESSION_RELEASE_RESOURCES, true);
>  }
>
>  static int venus_session_parse_seq_hdr(struct venus_inst *inst, u32 seq_hdr,
> @@ -1299,7 +1307,7 @@ static int venus_session_parse_seq_hdr(struct venus_inst *inst, u32 seq_hdr,
>         if (ret)
>                 return ret;
>
> -       ret = venus_iface_cmdq_write(hdev, pkt);
> +       ret = venus_iface_cmdq_write(hdev, pkt, false);
>         if (ret)
>                 return ret;
>
> @@ -1320,7 +1328,7 @@ static int venus_session_get_seq_hdr(struct venus_inst *inst, u32 seq_hdr,
>         if (ret)
>                 return ret;
>
> -       return venus_iface_cmdq_write(hdev, pkt);
> +       return venus_iface_cmdq_write(hdev, pkt, false);
>  }
>
>  static int venus_session_set_property(struct venus_inst *inst, u32 ptype,
> @@ -1339,7 +1347,7 @@ static int venus_session_set_property(struct venus_inst *inst, u32 ptype,
>         if (ret)
>                 return ret;
>
> -       return venus_iface_cmdq_write(hdev, pkt);
> +       return venus_iface_cmdq_write(hdev, pkt, false);
>  }
>
>  static int venus_session_get_property(struct venus_inst *inst, u32 ptype)
> @@ -1352,7 +1360,7 @@ static int venus_session_get_property(struct venus_inst *inst, u32 ptype)
>         if (ret)
>                 return ret;
>
> -       return venus_iface_cmdq_write(hdev, &pkt);
> +       return venus_iface_cmdq_write(hdev, &pkt, true);
>  }
>
>  static int venus_resume(struct venus_core *core)
> --
> 2.17.1
>

Re: [PATCH 3/3] media: hfi_venus: Request interrupt for sync cmds

[Stanimir Varbanov]

On 11/21/20 3:02 AM, Fritz Koenig wrote:
> On Thu, Nov 19, 2020 at 4:12 PM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>>
>> From: Vikash Garodia <vgarodia@codeaurora.org>
>>
>> For synchronous commands, update the message queue variable.
>> This would inform video firmware to raise interrupt on host
>> CPU whenever there is a response for such commands.
>>
>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/hfi_venus.c | 74 ++++++++++---------
>>  1 file changed, 41 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index 4be4a75ddcb6..b8fdb464ba9c 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -372,7 +372,7 @@ static void venus_soft_int(struct venus_hfi_device *hdev)
>>  }
>>
>>  static int venus_iface_cmdq_write_nolock(struct venus_hfi_device *hdev,
>> -                                        void *pkt)
>> +                                        void *pkt, bool sync)
>>  {
>>         struct device *dev = hdev->core->dev;
>>         struct hfi_pkt_hdr *cmd_packet;
>> @@ -397,15 +397,23 @@ static int venus_iface_cmdq_write_nolock(struct venus_hfi_device *hdev,
>>         if (rx_req)
>>                 venus_soft_int(hdev);
>>
>> +       /* Inform video firmware to raise interrupt for synchronous commands */
>> +       queue = &hdev->queues[IFACEQ_MSG_IDX];
> 
> I don't think there is any reason to scope queue outside of  the sync
> block below.

OK. I'll move into the 'if' statment.

> 
>>
>> +       if (sync) {
>> +               queue->qhdr->rx_req = 1;
>> +               /* ensure rx_req is updated in memory */
>> +               wmb();
>> +       }
>> +
>>         return 0;
>>  }
>>
<cut>

-- 
-- 
regards,
Stan

Re: [PATCH 3/3] media: hfi_venus: Request interrupt for sync cmds

[Alexandre Courbot]

On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> From: Vikash Garodia <vgarodia@codeaurora.org>
>
> For synchronous commands, update the message queue variable.
> This would inform video firmware to raise interrupt on host
> CPU whenever there is a response for such commands.
>
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/hfi_venus.c | 74 ++++++++++---------
>  1 file changed, 41 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index 4be4a75ddcb6..b8fdb464ba9c 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -372,7 +372,7 @@ static void venus_soft_int(struct venus_hfi_device *hdev)
>  }
>
>  static int venus_iface_cmdq_write_nolock(struct venus_hfi_device *hdev,
> -                                        void *pkt)
> +                                        void *pkt, bool sync)
>  {
>         struct device *dev = hdev->core->dev;
>         struct hfi_pkt_hdr *cmd_packet;
> @@ -397,15 +397,23 @@ static int venus_iface_cmdq_write_nolock(struct venus_hfi_device *hdev,
>         if (rx_req)
>                 venus_soft_int(hdev);
>
> +       /* Inform video firmware to raise interrupt for synchronous commands */
> +       queue = &hdev->queues[IFACEQ_MSG_IDX];
> +       if (sync) {
> +               queue->qhdr->rx_req = 1;
> +               /* ensure rx_req is updated in memory */
> +               wmb();
> +       }

Wouldn't it be safer to do this before calling venus_soft_int()? I
don't know what the firmware is supposed to do with rx_req but
intuitively it looks like it should be set before we signal it.

> +
>         return 0;
>  }
>
> -static int venus_iface_cmdq_write(struct venus_hfi_device *hdev, void *pkt)
> +static int venus_iface_cmdq_write(struct venus_hfi_device *hdev, void *pkt, bool sync)
>  {
>         int ret;
>
>         mutex_lock(&hdev->lock);
> -       ret = venus_iface_cmdq_write_nolock(hdev, pkt);
> +       ret = venus_iface_cmdq_write_nolock(hdev, pkt, sync);
>         mutex_unlock(&hdev->lock);
>
>         return ret;
> @@ -428,7 +436,7 @@ static int venus_hfi_core_set_resource(struct venus_core *core, u32 id,
>         if (ret)
>                 return ret;
>
> -       ret = venus_iface_cmdq_write(hdev, pkt);
> +       ret = venus_iface_cmdq_write(hdev, pkt, false);
>         if (ret)
>                 return ret;
>
> @@ -778,7 +786,7 @@ static int venus_sys_set_debug(struct venus_hfi_device *hdev, u32 debug)
>
>         pkt_sys_debug_config(pkt, HFI_DEBUG_MODE_QUEUE, debug);
>
> -       ret = venus_iface_cmdq_write(hdev, pkt);
> +       ret = venus_iface_cmdq_write(hdev, pkt, false);
>         if (ret)
>                 return ret;
>
> @@ -795,7 +803,7 @@ static int venus_sys_set_coverage(struct venus_hfi_device *hdev, u32 mode)
>
>         pkt_sys_coverage_config(pkt, mode);
>
> -       ret = venus_iface_cmdq_write(hdev, pkt);
> +       ret = venus_iface_cmdq_write(hdev, pkt, false);
>         if (ret)
>                 return ret;
>
> @@ -816,7 +824,7 @@ static int venus_sys_set_idle_message(struct venus_hfi_device *hdev,
>
>         pkt_sys_idle_indicator(pkt, enable);
>
> -       ret = venus_iface_cmdq_write(hdev, pkt);
> +       ret = venus_iface_cmdq_write(hdev, pkt, false);
>         if (ret)
>                 return ret;
>
> @@ -834,7 +842,7 @@ static int venus_sys_set_power_control(struct venus_hfi_device *hdev,
>
>         pkt_sys_power_control(pkt, enable);
>
> -       ret = venus_iface_cmdq_write(hdev, pkt);
> +       ret = venus_iface_cmdq_write(hdev, pkt, false);
>         if (ret)
>                 return ret;
>
> @@ -885,14 +893,14 @@ static int venus_sys_set_default_properties(struct venus_hfi_device *hdev)
>         return ret;
>  }
>
> -static int venus_session_cmd(struct venus_inst *inst, u32 pkt_type)
> +static int venus_session_cmd(struct venus_inst *inst, u32 pkt_type, bool sync)
>  {
>         struct venus_hfi_device *hdev = to_hfi_priv(inst->core);
>         struct hfi_session_pkt pkt;
>
>         pkt_session_cmd(&pkt, pkt_type, inst);
>
> -       return venus_iface_cmdq_write(hdev, &pkt);
> +       return venus_iface_cmdq_write(hdev, &pkt, sync);
>  }
>
>  static void venus_flush_debug_queue(struct venus_hfi_device *hdev)
> @@ -922,7 +930,7 @@ static int venus_prepare_power_collapse(struct venus_hfi_device *hdev,
>
>         pkt_sys_pc_prep(&pkt);
>
> -       ret = venus_iface_cmdq_write(hdev, &pkt);
> +       ret = venus_iface_cmdq_write(hdev, &pkt, false);
>         if (ret)
>                 return ret;
>
> @@ -1064,13 +1072,13 @@ static int venus_core_init(struct venus_core *core)
>
>         venus_set_state(hdev, VENUS_STATE_INIT);
>
> -       ret = venus_iface_cmdq_write(hdev, &pkt);
> +       ret = venus_iface_cmdq_write(hdev, &pkt, false);
>         if (ret)
>                 return ret;
>
>         pkt_sys_image_version(&version_pkt);
>
> -       ret = venus_iface_cmdq_write(hdev, &version_pkt);
> +       ret = venus_iface_cmdq_write(hdev, &version_pkt, false);
>         if (ret)
>                 dev_warn(dev, "failed to send image version pkt to fw\n");
>
> @@ -1099,7 +1107,7 @@ static int venus_core_ping(struct venus_core *core, u32 cookie)
>
>         pkt_sys_ping(&pkt, cookie);
>
> -       return venus_iface_cmdq_write(hdev, &pkt);
> +       return venus_iface_cmdq_write(hdev, &pkt, false);
>  }
>
>  static int venus_core_trigger_ssr(struct venus_core *core, u32 trigger_type)
> @@ -1112,7 +1120,7 @@ static int venus_core_trigger_ssr(struct venus_core *core, u32 trigger_type)
>         if (ret)
>                 return ret;
>
> -       return venus_iface_cmdq_write(hdev, &pkt);
> +       return venus_iface_cmdq_write(hdev, &pkt, false);
>  }
>
>  static int venus_session_init(struct venus_inst *inst, u32 session_type,
> @@ -1130,7 +1138,7 @@ static int venus_session_init(struct venus_inst *inst, u32 session_type,
>         if (ret)
>                 goto err;
>
> -       ret = venus_iface_cmdq_write(hdev, &pkt);
> +       ret = venus_iface_cmdq_write(hdev, &pkt, true);
>         if (ret)
>                 goto err;
>
> @@ -1151,7 +1159,7 @@ static int venus_session_end(struct venus_inst *inst)
>                         dev_warn(dev, "fw coverage msg ON failed\n");
>         }
>
> -       return venus_session_cmd(inst, HFI_CMD_SYS_SESSION_END);
> +       return venus_session_cmd(inst, HFI_CMD_SYS_SESSION_END, true);
>  }
>
>  static int venus_session_abort(struct venus_inst *inst)
> @@ -1160,7 +1168,7 @@ static int venus_session_abort(struct venus_inst *inst)
>
>         venus_flush_debug_queue(hdev);
>
> -       return venus_session_cmd(inst, HFI_CMD_SYS_SESSION_ABORT);
> +       return venus_session_cmd(inst, HFI_CMD_SYS_SESSION_ABORT, true);
>  }
>
>  static int venus_session_flush(struct venus_inst *inst, u32 flush_mode)
> @@ -1173,22 +1181,22 @@ static int venus_session_flush(struct venus_inst *inst, u32 flush_mode)
>         if (ret)
>                 return ret;
>
> -       return venus_iface_cmdq_write(hdev, &pkt);
> +       return venus_iface_cmdq_write(hdev, &pkt, true);
>  }
>
>  static int venus_session_start(struct venus_inst *inst)
>  {
> -       return venus_session_cmd(inst, HFI_CMD_SESSION_START);
> +       return venus_session_cmd(inst, HFI_CMD_SESSION_START, true);
>  }
>
>  static int venus_session_stop(struct venus_inst *inst)
>  {
> -       return venus_session_cmd(inst, HFI_CMD_SESSION_STOP);
> +       return venus_session_cmd(inst, HFI_CMD_SESSION_STOP, true);
>  }
>
>  static int venus_session_continue(struct venus_inst *inst)
>  {
> -       return venus_session_cmd(inst, HFI_CMD_SESSION_CONTINUE);
> +       return venus_session_cmd(inst, HFI_CMD_SESSION_CONTINUE, false);
>  }
>
>  static int venus_session_etb(struct venus_inst *inst,
> @@ -1205,7 +1213,7 @@ static int venus_session_etb(struct venus_inst *inst,
>                 if (ret)
>                         return ret;
>
> -               ret = venus_iface_cmdq_write(hdev, &pkt);
> +               ret = venus_iface_cmdq_write(hdev, &pkt, false);
>         } else if (session_type == VIDC_SESSION_TYPE_ENC) {
>                 struct hfi_session_empty_buffer_uncompressed_plane0_pkt pkt;
>
> @@ -1213,7 +1221,7 @@ static int venus_session_etb(struct venus_inst *inst,
>                 if (ret)
>                         return ret;
>
> -               ret = venus_iface_cmdq_write(hdev, &pkt);
> +               ret = venus_iface_cmdq_write(hdev, &pkt, false);
>         } else {
>                 ret = -EINVAL;
>         }
> @@ -1232,7 +1240,7 @@ static int venus_session_ftb(struct venus_inst *inst,
>         if (ret)
>                 return ret;
>
> -       return venus_iface_cmdq_write(hdev, &pkt);
> +       return venus_iface_cmdq_write(hdev, &pkt, false);
>  }
>
>  static int venus_session_set_buffers(struct venus_inst *inst,
> @@ -1252,7 +1260,7 @@ static int venus_session_set_buffers(struct venus_inst *inst,
>         if (ret)
>                 return ret;
>
> -       return venus_iface_cmdq_write(hdev, pkt);
> +       return venus_iface_cmdq_write(hdev, pkt, false);
>  }
>
>  static int venus_session_unset_buffers(struct venus_inst *inst,
> @@ -1272,17 +1280,17 @@ static int venus_session_unset_buffers(struct venus_inst *inst,
>         if (ret)
>                 return ret;
>
> -       return venus_iface_cmdq_write(hdev, pkt);
> +       return venus_iface_cmdq_write(hdev, pkt, true);
>  }
>
>  static int venus_session_load_res(struct venus_inst *inst)
>  {
> -       return venus_session_cmd(inst, HFI_CMD_SESSION_LOAD_RESOURCES);
> +       return venus_session_cmd(inst, HFI_CMD_SESSION_LOAD_RESOURCES, true);
>  }
>
>  static int venus_session_release_res(struct venus_inst *inst)
>  {
> -       return venus_session_cmd(inst, HFI_CMD_SESSION_RELEASE_RESOURCES);
> +       return venus_session_cmd(inst, HFI_CMD_SESSION_RELEASE_RESOURCES, true);
>  }
>
>  static int venus_session_parse_seq_hdr(struct venus_inst *inst, u32 seq_hdr,
> @@ -1299,7 +1307,7 @@ static int venus_session_parse_seq_hdr(struct venus_inst *inst, u32 seq_hdr,
>         if (ret)
>                 return ret;
>
> -       ret = venus_iface_cmdq_write(hdev, pkt);
> +       ret = venus_iface_cmdq_write(hdev, pkt, false);
>         if (ret)
>                 return ret;
>
> @@ -1320,7 +1328,7 @@ static int venus_session_get_seq_hdr(struct venus_inst *inst, u32 seq_hdr,
>         if (ret)
>                 return ret;
>
> -       return venus_iface_cmdq_write(hdev, pkt);
> +       return venus_iface_cmdq_write(hdev, pkt, false);
>  }
>
>  static int venus_session_set_property(struct venus_inst *inst, u32 ptype,
> @@ -1339,7 +1347,7 @@ static int venus_session_set_property(struct venus_inst *inst, u32 ptype,
>         if (ret)
>                 return ret;
>
> -       return venus_iface_cmdq_write(hdev, pkt);
> +       return venus_iface_cmdq_write(hdev, pkt, false);
>  }
>
>  static int venus_session_get_property(struct venus_inst *inst, u32 ptype)
> @@ -1352,7 +1360,7 @@ static int venus_session_get_property(struct venus_inst *inst, u32 ptype)
>         if (ret)
>                 return ret;
>
> -       return venus_iface_cmdq_write(hdev, &pkt);
> +       return venus_iface_cmdq_write(hdev, &pkt, true);
>  }
>
>  static int venus_resume(struct venus_core *core)
> --
> 2.17.1
>

Re: [PATCH 3/3] media: hfi_venus: Request interrupt for sync cmds

[Stanimir Varbanov]

On 11/25/20 10:08 AM, Alexandre Courbot wrote:
> On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>>
>> From: Vikash Garodia <vgarodia@codeaurora.org>
>>
>> For synchronous commands, update the message queue variable.
>> This would inform video firmware to raise interrupt on host
>> CPU whenever there is a response for such commands.
>>
>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/hfi_venus.c | 74 ++++++++++---------
>>  1 file changed, 41 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index 4be4a75ddcb6..b8fdb464ba9c 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -372,7 +372,7 @@ static void venus_soft_int(struct venus_hfi_device *hdev)
>>  }
>>
>>  static int venus_iface_cmdq_write_nolock(struct venus_hfi_device *hdev,
>> -                                        void *pkt)
>> +                                        void *pkt, bool sync)
>>  {
>>         struct device *dev = hdev->core->dev;
>>         struct hfi_pkt_hdr *cmd_packet;
>> @@ -397,15 +397,23 @@ static int venus_iface_cmdq_write_nolock(struct venus_hfi_device *hdev,
>>         if (rx_req)
>>                 venus_soft_int(hdev);
>>
>> +       /* Inform video firmware to raise interrupt for synchronous commands */
>> +       queue = &hdev->queues[IFACEQ_MSG_IDX];
>> +       if (sync) {
>> +               queue->qhdr->rx_req = 1;
>> +               /* ensure rx_req is updated in memory */
>> +               wmb();
>> +       }
> 
> Wouldn't it be safer to do this before calling venus_soft_int()? I
> don't know what the firmware is supposed to do with rx_req but
> intuitively it looks like it should be set before we signal it.
> 

I'll leave Vikash to comment. IMO this is a good suggestion.

<cut>

-- 
regards,
Stan