by Alexandre Courbot

[permalink] [raw]

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

On Fri, Nov 27, 2020 at 7:42 AM Stanimir Varbanov
<[email protected]> wrote:
>
>
>
> On 11/26/20 8:28 AM, Alexandre Courbot wrote:
> > On Wed, Nov 25, 2020 at 10:01 PM Stanimir Varbanov
> > <[email protected]> wrote:
> >>
> >>
> >>
> >> On 11/25/20 5:46 AM, Alexandre Courbot wrote:
> >>> On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov
> >>> <[email protected]> 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 <[email protected]>
> >>>> ---
> >>>> 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.

2020-12-01 12:26:28

by Dikshita Agarwal

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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,