| Message ID | 20201120001037.10032-2-stanimir.varbanov@linaro.org |
|---|---|
| State | Accepted |
| Commit | 5f2ca73dcca96c3de96a0e4d9ea24ebb46c55d2e |
| Headers | show |
| Series | Venus encoder improvements | expand |
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>
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 >
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>
Hi Stan, On 2020-11-20 05:40, Stanimir Varbanov wrote: > Init the hfi session only once in queue_setup and also cover that > with inst->lock. > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > drivers/media/platform/qcom/venus/venc.c | 98 ++++++++++++++++++------ > 1 file changed, 73 insertions(+), 25 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/venc.c > b/drivers/media/platform/qcom/venus/venc.c > index 4ecf78e30b59..3a2e449663d8 100644 > --- a/drivers/media/platform/qcom/venus/venc.c > +++ b/drivers/media/platform/qcom/venus/venc.c > @@ -725,8 +725,10 @@ static int venc_init_session(struct venus_inst > *inst) > int ret; > > ret = hfi_session_init(inst, inst->fmt_cap->pixfmt); > - if (ret) > - return ret; > + if (ret == -EINVAL) > + return 0; > + else if (ret) > + goto deinit; > > ret = venus_helper_set_input_resolution(inst, inst->width, > inst->height); > @@ -762,17 +764,13 @@ static int venc_out_num_buffers(struct > venus_inst *inst, unsigned int *num) > struct hfi_buffer_requirements bufreq; > int ret; > > - ret = venc_init_session(inst); > + ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq); > if (ret) > return ret; > > - ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq); > - > *num = bufreq.count_actual; > > - hfi_session_deinit(inst); > - > - return ret; > + return 0; > } > > static int venc_queue_setup(struct vb2_queue *q, > @@ -781,7 +779,7 @@ static int venc_queue_setup(struct vb2_queue *q, > { > struct venus_inst *inst = vb2_get_drv_priv(q); > unsigned int num, min = 4; > - int ret = 0; > + int ret; > > if (*num_planes) { > if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE && > @@ -803,6 +801,17 @@ static int venc_queue_setup(struct vb2_queue *q, > return 0; > } > > + ret = mutex_lock_interruptible(&inst->lock); > + if (ret) > + return ret; > + > + ret = venc_init_session(inst); > + > + mutex_unlock(&inst->lock); > + > + if (ret) > + return ret; > + > switch (q->type) { > case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > *num_planes = inst->fmt_out->num_planes; > @@ -838,6 +847,54 @@ static int venc_queue_setup(struct vb2_queue *q, > return ret; > } > > +static int venc_buf_init(struct vb2_buffer *vb) > +{ > + struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); > + > + inst->buf_count++; > + > + return venus_helper_vb2_buf_init(vb); > +} > + > +static void venc_release_session(struct venus_inst *inst) > +{ > + int ret, abort = 0; > + > + mutex_lock(&inst->lock); > + > + ret = hfi_session_deinit(inst); > + abort = (ret && ret != -EINVAL) ? 1 : 0; > + > + if (inst->session_error) > + abort = 1; > + > + if (abort) > + hfi_session_abort(inst); > + > + mutex_unlock(&inst->lock); > + > + venus_pm_load_scale(inst); > + INIT_LIST_HEAD(&inst->registeredbufs); > + venus_pm_release_core(inst); > +} > + > +static void venc_buf_cleanup(struct vb2_buffer *vb) > +{ > + struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > + struct venus_buffer *buf = to_venus_buffer(vbuf); > + > + mutex_lock(&inst->lock); > + if (vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) > + if (!list_empty(&inst->registeredbufs)) > + list_del_init(&buf->reg_list); > + mutex_unlock(&inst->lock); > + > + inst->buf_count--; > + if (!inst->buf_count) > + venc_release_session(inst); > +} > + > static int venc_verify_conf(struct venus_inst *inst) > { > enum hfi_version ver = inst->core->res->hfi_version; > @@ -888,38 +945,28 @@ static int venc_start_streaming(struct vb2_queue > *q, unsigned int count) > inst->sequence_cap = 0; > inst->sequence_out = 0; > > - ret = venc_init_session(inst); > - if (ret) > - goto bufs_done; > - > ret = venus_pm_acquire_core(inst); > if (ret) > - goto deinit_sess; > - > - ret = venc_set_properties(inst); > - if (ret) > - goto deinit_sess; With this change, if set ctrl for target bitrate is called after queue setup and before streaming, the new bitrate won’t be set to FW. which is not right and can cause quality issues. The same might apply to other encoder parameters as well. Please fix this in the next version. > + goto error; > > ret = venc_verify_conf(inst); > if (ret) > - goto deinit_sess; > + goto error; > > ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs, > inst->num_output_bufs, 0); > if (ret) > - goto deinit_sess; > + goto error; > > ret = venus_helper_vb2_start_streaming(inst); > if (ret) > - goto deinit_sess; > + goto error; > > mutex_unlock(&inst->lock); > > return 0; > > -deinit_sess: > - hfi_session_deinit(inst); > -bufs_done: > +error: > venus_helper_buffers_done(inst, q->type, VB2_BUF_STATE_QUEUED); > if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > inst->streamon_out = 0; > @@ -940,7 +987,8 @@ static void venc_vb2_buf_queue(struct vb2_buffer > *vb) > > static const struct vb2_ops venc_vb2_ops = { > .queue_setup = venc_queue_setup, > - .buf_init = venus_helper_vb2_buf_init, > + .buf_init = venc_buf_init, > + .buf_cleanup = venc_buf_cleanup, > .buf_prepare = venus_helper_vb2_buf_prepare, > .start_streaming = venc_start_streaming, > .stop_streaming = venus_helper_vb2_stop_streaming,
diff --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,
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(-)