Messages in this thread Patch in this message | | | Date | Tue, 28 Sep 2021 09:55:38 +0530 | From | mansur@codeauro ... | Subject | Re: [V3] venus: vdec: decoded picture buffer handling during reconfig sequence |
| |
On 2021-09-21 19:17, Stanimir Varbanov wrote: > Hi Mansur, > > On 8/25/21 2:08 PM, Mansur Alisha Shaik wrote: >> In existing implementation, driver is freeing and un-mapping all the >> decoded picture buffers(DPB) as part of dynamic resolution change(DRC) >> handling. As a result, when firmware try to access the DPB buffer, >> from >> previous sequence, SMMU context fault is seen due to the buffer being >> already unmapped. >> >> With this change, driver defines ownership of each DPB buffer. If a >> buffer >> is owned by firmware, driver would skip from un-mapping the same. >> >> Signed-off-by: Mansur Alisha Shaik <mansur@codeaurora.org> >> >> Changes in V3: >> - Migrated id allocation using kernel API ida_alloc_min() >> >> --- >> drivers/media/platform/qcom/venus/helpers.c | 50 >> ++++++++++++++++++++- >> drivers/media/platform/qcom/venus/helpers.h | 2 + >> drivers/media/platform/qcom/venus/vdec.c | 7 ++- >> 3 files changed, 57 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/helpers.c >> b/drivers/media/platform/qcom/venus/helpers.c >> index 8012f5c7bf34..f36361d346b2 100644 >> --- a/drivers/media/platform/qcom/venus/helpers.c >> +++ b/drivers/media/platform/qcom/venus/helpers.c >> @@ -3,6 +3,7 @@ >> * Copyright (c) 2012-2016, The Linux Foundation. All rights >> reserved. >> * Copyright (C) 2017 Linaro Ltd. >> */ >> +#include <linux/idr.h> >> #include <linux/list.h> >> #include <linux/mutex.h> >> #include <linux/slab.h> >> @@ -21,6 +22,13 @@ >> #define NUM_MBS_720P (((1280 + 15) >> 4) * ((720 + 15) >> 4)) >> #define NUM_MBS_4K (((4096 + 15) >> 4) * ((2304 + 15) >> 4)) >> >> +static DEFINE_IDA(dpb_out_tag_ida); > > No global static variables please. Make it part of venus_inst > structure. As per my understanding it is not just static global variable. We are defining the ida structure and assign to name when pass as param as follows struct ida { struct idr idr; struct ida_bitmap *free_bitmap; }; #define IDA_INIT(name) { .idr = IDR_INIT((name).idr), .free_bitmap = NULL, } #define DEFINE_IDA(name) struct ida name = IDA_INIT(name)
Any ida related API's expect pointer to this structure. If we move the variable then it might be bit difficult use ida_xxx() API'same
> >> + >> +enum dpb_buf_owner { >> + DRIVER, >> + FIRMWARE, >> +}; >> + >> struct intbuf { >> struct list_head list; >> u32 type; >> @@ -28,6 +36,8 @@ struct intbuf { >> void *va; >> dma_addr_t da; >> unsigned long attrs; >> + enum dpb_buf_owner owned_by; >> + u32 dpb_out_tag; >> }; >> >> bool venus_helper_check_codec(struct venus_inst *inst, u32 >> v4l2_pixfmt) >> @@ -95,9 +105,16 @@ int venus_helper_queue_dpb_bufs(struct venus_inst >> *inst) >> fdata.device_addr = buf->da; >> fdata.buffer_type = buf->type; >> >> + if (buf->owned_by == FIRMWARE) >> + continue; >> + >> + fdata.clnt_data = buf->dpb_out_tag; >> + >> ret = hfi_session_process_buf(inst, &fdata); >> if (ret) >> goto fail; >> + >> + buf->owned_by = FIRMWARE; >> } >> >> fail: >> @@ -110,13 +127,19 @@ int venus_helper_free_dpb_bufs(struct venus_inst >> *inst) >> struct intbuf *buf, *n; >> >> list_for_each_entry_safe(buf, n, &inst->dpbbufs, list) { >> + if (buf->owned_by == FIRMWARE) >> + continue; >> + >> + ida_free(&dpb_out_tag_ida, buf->dpb_out_tag); >> + >> list_del_init(&buf->list); >> dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da, >> buf->attrs); >> kfree(buf); >> } >> >> - INIT_LIST_HEAD(&inst->dpbbufs); >> + if (list_empty(&inst->dpbbufs)) >> + INIT_LIST_HEAD(&inst->dpbbufs); >> >> return 0; >> } >> @@ -134,6 +157,7 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst >> *inst) >> unsigned int i; >> u32 count; >> int ret; >> + int id; >> >> /* no need to allocate dpb buffers */ >> if (!inst->dpb_fmt) >> @@ -171,6 +195,15 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst >> *inst) >> ret = -ENOMEM; >> goto fail; >> } >> + buf->owned_by = DRIVER; >> + >> + id = ida_alloc_min(&dpb_out_tag_ida, VB2_MAX_FRAME, GFP_KERNEL); >> + if (id < 0) { >> + ret = id; >> + goto fail; >> + } >> + >> + buf->dpb_out_tag = id; >> >> list_add_tail(&buf->list, &inst->dpbbufs); >> } >> @@ -1365,6 +1398,21 @@ venus_helper_find_buf(struct venus_inst *inst, >> unsigned int type, u32 idx) >> } >> EXPORT_SYMBOL_GPL(venus_helper_find_buf); >> >> +void venus_helper_find_dpb_buf(struct venus_inst *inst, struct >> vb2_v4l2_buffer *vbuf, >> + unsigned int type, unsigned int buf_type, u32 tag) > > If this helper will return void then it should be renamed to something > like venus_helper_dpb_buf_change_owner(). > Ok. I will change while posting next version. >> +{ >> + struct intbuf *dpb_buf; >> + >> + if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE && >> + buf_type == HFI_BUFFER_OUTPUT) >> + list_for_each_entry(dpb_buf, &inst->dpbbufs, list) >> + if (dpb_buf->dpb_out_tag == tag) { >> + dpb_buf->owned_by = DRIVER; >> + break; >> + } >> +} >> +EXPORT_SYMBOL_GPL(venus_helper_find_dpb_buf); >> + >> int venus_helper_vb2_buf_init(struct vb2_buffer *vb) >> { >> struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); >> diff --git a/drivers/media/platform/qcom/venus/helpers.h >> b/drivers/media/platform/qcom/venus/helpers.h >> index e6269b4be3af..17c5aadaec82 100644 >> --- a/drivers/media/platform/qcom/venus/helpers.h >> +++ b/drivers/media/platform/qcom/venus/helpers.h >> @@ -14,6 +14,8 @@ struct venus_core; >> bool venus_helper_check_codec(struct venus_inst *inst, u32 >> v4l2_pixfmt); >> struct vb2_v4l2_buffer *venus_helper_find_buf(struct venus_inst >> *inst, >> unsigned int type, u32 idx); >> +void venus_helper_find_dpb_buf(struct venus_inst *inst, struct >> vb2_v4l2_buffer *vbuf, >> + unsigned int type, unsigned int buf_type, u32 idx); >> void venus_helper_buffers_done(struct venus_inst *inst, unsigned int >> type, >> enum vb2_buffer_state state); >> int venus_helper_vb2_buf_init(struct vb2_buffer *vb); >> diff --git a/drivers/media/platform/qcom/venus/vdec.c >> b/drivers/media/platform/qcom/venus/vdec.c >> index 198e47eb63f4..cafdc3d8e473 100644 >> --- a/drivers/media/platform/qcom/venus/vdec.c >> +++ b/drivers/media/platform/qcom/venus/vdec.c >> @@ -1297,6 +1297,7 @@ static void vdec_buf_done(struct venus_inst >> *inst, unsigned int buf_type, >> struct vb2_v4l2_buffer *vbuf; >> struct vb2_buffer *vb; >> unsigned int type; >> + struct intbuf *dpb_buf; >> >> vdec_pm_touch(inst); >> >> @@ -1306,8 +1307,10 @@ static void vdec_buf_done(struct venus_inst >> *inst, unsigned int buf_type, >> type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; >> >> vbuf = venus_helper_find_buf(inst, type, tag); >> - if (!vbuf) >> + if (!vbuf) { >> + venus_helper_find_dpb_buf(inst, vbuf, type, buf_type, tag); >> return; >> + } >> >> vbuf->flags = flags; >> vbuf->field = V4L2_FIELD_NONE; >> @@ -1622,6 +1625,8 @@ static int vdec_close(struct file *file) >> >> vdec_pm_get(inst); >> >> + venus_helper_free_dpb_bufs(inst); >> + INIT_LIST_HEAD(&inst->dpbbufs); > > This belongs to venus_helper_free_dpb_bufs not here. > Ok, I will remove it from here, since in free_dpb() api INIT_LIST_HEAD() when list is free.
>> v4l2_m2m_ctx_release(inst->m2m_ctx); >> v4l2_m2m_release(inst->m2m_dev); >> vdec_ctrl_deinit(inst); >> > > -- > regards, > Stan Thanks, Mansur
| |