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@xxxxxxxxxxxxxx> > > 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. > + > +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(). > +{ > + 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. > v4l2_m2m_ctx_release(inst->m2m_ctx); > v4l2_m2m_release(inst->m2m_dev); > vdec_ctrl_deinit(inst); > -- regards, Stan -- regards, Stan