From 7b3b1524c45444e32bf8713969fc90e8edb9bd2f Mon Sep 17 00:00:00 2001 From: Govindaraj Rajagopal Date: Wed, 24 Jun 2020 20:31:31 +0530 Subject: [PATCH] msm: vidc: fix deadlock between queue and flush buffer handling qbuf ioctl acquired bufq[port].lock in one thread and flush call acquired registeredbufs.lock in another thread. So thread-1 is waiting for registeredbufs.lock & thread-2 is waiting for bufq[port].lock i.e leading to deadlock. So added change to avoid above mentioned deadlock. Change-Id: Ie21984fdb562ca7a09f801f036f3a78429ceab94 Signed-off-by: Govindaraj Rajagopal --- msm/vidc/msm_vidc.c | 33 +++++++++++++------------- msm/vidc/msm_vidc_common.c | 45 +++++++++++++++++++++++------------- msm/vidc/msm_vidc_internal.h | 2 +- msm/vidc/vidc_hfi_api.h | 7 +++--- 4 files changed, 49 insertions(+), 38 deletions(-) diff --git a/msm/vidc/msm_vidc.c b/msm/vidc/msm_vidc.c index 180b6a39eb97..3f789a4afe46 100644 --- a/msm/vidc/msm_vidc.c +++ b/msm/vidc/msm_vidc.c @@ -380,13 +380,21 @@ int msm_vidc_qbuf(void *instance, struct media_device *mdev, return -EINVAL; } + q = msm_comm_get_vb2q(inst, b->type); + if (!q) { + s_vpr_e(inst->sid, + "Failed to find buffer queue. type %d\n", b->type); + return -EINVAL; + } + + mutex_lock(&q->lock); if ((inst->out_flush && b->type == OUTPUT_MPLANE) || inst->in_flush) { s_vpr_e(inst->sid, "%s: in flush, discarding qbuf, type %u, index %u\n", __func__, b->type, b->index); - return -EINVAL; + rc = -EINVAL; + goto unlock; } - inst->last_qbuf_time_ns = ktime_get_ns(); for (i = 0; i < b->length; i++) { @@ -409,7 +417,8 @@ int msm_vidc_qbuf(void *instance, struct media_device *mdev, 0, inst->sid); if (rc) { s_vpr_e(inst->sid, "Failed to store input tag"); - return -EINVAL; + rc = -EINVAL; + goto unlock; } } @@ -430,7 +439,7 @@ int msm_vidc_qbuf(void *instance, struct media_device *mdev, rc = msm_comm_store_timestamp(inst, timestamp_us); if (rc) - return rc; + goto unlock; inst->clk_data.frame_rate = msm_comm_get_max_framerate(inst); } if (is_encode_session(inst) && b->type == INPUT_MPLANE) { @@ -440,21 +449,14 @@ int msm_vidc_qbuf(void *instance, struct media_device *mdev, rc = msm_venc_store_timestamp(inst, timestamp_us); if (rc) - return rc; + goto unlock; } - q = msm_comm_get_vb2q(inst, b->type); - if (!q) { - s_vpr_e(inst->sid, - "Failed to find buffer queue. type %d\n", b->type); - return -EINVAL; - } - - mutex_lock(&q->lock); rc = vb2_qbuf(&q->vb2_bufq, mdev, b); - mutex_unlock(&q->lock); if (rc) s_vpr_e(inst->sid, "Failed to qbuf, %d\n", rc); +unlock: + mutex_unlock(&q->lock); return rc; } @@ -1485,7 +1487,6 @@ void *msm_vidc_open(int core_id, int session_type) mutex_init(&inst->bufq[OUTPUT_PORT].lock); mutex_init(&inst->bufq[INPUT_PORT].lock); mutex_init(&inst->lock); - mutex_init(&inst->flush_lock); mutex_init(&inst->ubwc_stats_lock); INIT_MSM_VIDC_LIST(&inst->scratchbufs); @@ -1596,7 +1597,6 @@ fail_bufq_capture: mutex_destroy(&inst->bufq[OUTPUT_PORT].lock); mutex_destroy(&inst->bufq[INPUT_PORT].lock); mutex_destroy(&inst->lock); - mutex_destroy(&inst->flush_lock); DEINIT_MSM_VIDC_LIST(&inst->scratchbufs); DEINIT_MSM_VIDC_LIST(&inst->persistbufs); @@ -1742,7 +1742,6 @@ int msm_vidc_destroy(struct msm_vidc_inst *inst) mutex_destroy(&inst->bufq[OUTPUT_PORT].lock); mutex_destroy(&inst->bufq[INPUT_PORT].lock); mutex_destroy(&inst->lock); - mutex_destroy(&inst->flush_lock); msm_vidc_debugfs_deinit_inst(inst); diff --git a/msm/vidc/msm_vidc_common.c b/msm/vidc/msm_vidc_common.c index d22a7c85d0ea..4fd181f268ab 100644 --- a/msm/vidc/msm_vidc_common.c +++ b/msm/vidc/msm_vidc_common.c @@ -2028,7 +2028,10 @@ static void handle_session_flush(enum hal_command_response cmd, void *data) return; } - mutex_lock(&inst->flush_lock); + if (response->data.flush_type & HAL_FLUSH_INPUT) + mutex_lock(&inst->bufq[INPUT_PORT].lock); + if (response->data.flush_type & HAL_FLUSH_OUTPUT) + mutex_lock(&inst->bufq[OUTPUT_PORT].lock); if (msm_comm_get_stream_output_mode(inst) == HAL_VIDEO_DECODER_SECONDARY) { @@ -2078,7 +2081,10 @@ static void handle_session_flush(enum hal_command_response cmd, void *data) v4l2_event_queue_fh(&inst->event_handler, &flush_event); exit: - mutex_unlock(&inst->flush_lock); + if (response->data.flush_type & HAL_FLUSH_OUTPUT) + mutex_unlock(&inst->bufq[OUTPUT_PORT].lock); + if (response->data.flush_type & HAL_FLUSH_INPUT) + mutex_unlock(&inst->bufq[INPUT_PORT].lock); s_vpr_l(inst->sid, "handled: SESSION_FLUSH_DONE\n"); put_inst(inst); } @@ -2290,7 +2296,7 @@ struct vb2_buffer *msm_comm_get_vb_using_vidc_buffer( return NULL; } - mutex_lock(&inst->bufq[port].lock); + WARN_ON(!mutex_is_locked(&inst->bufq[port].lock)); found = false; q = &inst->bufq[port].vb2_bufq; if (!q->streaming) { @@ -2306,7 +2312,6 @@ struct vb2_buffer *msm_comm_get_vb_using_vidc_buffer( } } unlock: - mutex_unlock(&inst->bufq[port].lock); if (!found) { print_vidc_buffer(VIDC_ERR, "vb2 not found for", inst, mbuf); return NULL; @@ -2321,6 +2326,7 @@ int msm_comm_vb2_buffer_done(struct msm_vidc_inst *inst, struct vb2_buffer *vb2; struct vb2_v4l2_buffer *vbuf; u32 i, port; + int rc = 0; if (!inst || !mbuf) { d_vpr_e("%s: invalid params %pK %pK\n", @@ -2335,16 +2341,19 @@ int msm_comm_vb2_buffer_done(struct msm_vidc_inst *inst, else return -EINVAL; - vb2 = msm_comm_get_vb_using_vidc_buffer(inst, mbuf); - if (!vb2) - return -EINVAL; - /* * access vb2 buffer under q->lock and if streaming only to * ensure the buffer was not free'd by vb2 framework while * we are accessing it here. */ mutex_lock(&inst->bufq[port].lock); + vb2 = msm_comm_get_vb_using_vidc_buffer(inst, mbuf); + if (!vb2) { + s_vpr_e(inst->sid, "%s: port %d buffer not found\n", + __func__, port); + rc = -EINVAL; + goto unlock; + } if (inst->bufq[port].vb2_bufq.streaming) { vbuf = to_vb2_v4l2_buffer(vb2); vbuf->flags = mbuf->vvb.flags; @@ -2360,9 +2369,10 @@ int msm_comm_vb2_buffer_done(struct msm_vidc_inst *inst, s_vpr_e(inst->sid, "%s: port %d is not streaming\n", __func__, port); } +unlock: mutex_unlock(&inst->bufq[port].lock); - return 0; + return rc; } static bool is_eos_buffer(struct msm_vidc_inst *inst, u32 device_addr) @@ -5516,7 +5526,6 @@ int msm_comm_flush(struct msm_vidc_inst *inst, u32 flags) ip_flush = !!(flags & V4L2_CMD_FLUSH_OUTPUT); op_flush = !!(flags & V4L2_CMD_FLUSH_CAPTURE); - if (ip_flush && !op_flush) { s_vpr_e(inst->sid, "Input only flush not supported, making it flush all\n"); @@ -5539,7 +5548,10 @@ int msm_comm_flush(struct msm_vidc_inst *inst, u32 flags) goto exit; } - mutex_lock(&inst->flush_lock); + if (ip_flush) + mutex_lock(&inst->bufq[INPUT_PORT].lock); + if (op_flush) + mutex_lock(&inst->bufq[OUTPUT_PORT].lock); /* enable in flush */ inst->in_flush = ip_flush; inst->out_flush = op_flush; @@ -5595,7 +5607,10 @@ int msm_comm_flush(struct msm_vidc_inst *inst, u32 flags) rc = call_hfi_op(hdev, session_flush, inst->session, HAL_FLUSH_OUTPUT); } - mutex_unlock(&inst->flush_lock); + if (op_flush) + mutex_unlock(&inst->bufq[OUTPUT_PORT].lock); + if (ip_flush) + mutex_unlock(&inst->bufq[INPUT_PORT].lock); if (rc) { s_vpr_e(inst->sid, "Sending flush to firmware failed, flush out all buffers\n"); @@ -6661,7 +6676,6 @@ int msm_comm_flush_vidc_buffer(struct msm_vidc_inst *inst, else return -EINVAL; - mutex_lock(&inst->bufq[port].lock); if (inst->bufq[port].vb2_bufq.streaming) { vb->planes[0].bytesused = 0; vb2_buffer_done(vb, VB2_BUF_STATE_DONE); @@ -6669,7 +6683,6 @@ int msm_comm_flush_vidc_buffer(struct msm_vidc_inst *inst, s_vpr_e(inst->sid, "%s: port %d is not streaming\n", __func__, port); } - mutex_unlock(&inst->bufq[port].lock); return 0; } @@ -7015,7 +7028,7 @@ void handle_release_buffer_reference(struct msm_vidc_inst *inst, unsigned int i = 0; u32 planes[VIDEO_MAX_PLANES] = {0}; - mutex_lock(&inst->flush_lock); + mutex_lock(&inst->bufq[OUTPUT_PORT].lock); mutex_lock(&inst->registeredbufs.lock); found = false; /* check if mbuf was not removed by any chance */ @@ -7104,7 +7117,7 @@ unlock: print_vidc_buffer(VIDC_ERR, "rbr qbuf failed", inst, mbuf); } - mutex_unlock(&inst->flush_lock); + mutex_unlock(&inst->bufq[OUTPUT_PORT].lock); } int msm_comm_unmap_vidc_buffer(struct msm_vidc_inst *inst, diff --git a/msm/vidc/msm_vidc_internal.h b/msm/vidc/msm_vidc_internal.h index 1ea315bb785d..a10853e87366 100644 --- a/msm/vidc/msm_vidc_internal.h +++ b/msm/vidc/msm_vidc_internal.h @@ -494,7 +494,7 @@ struct msm_vidc_inst_smem_ops { struct msm_vidc_inst { struct list_head list; - struct mutex sync_lock, lock, flush_lock; + struct mutex sync_lock, lock; struct msm_vidc_core *core; enum session_type session_type; void *session; diff --git a/msm/vidc/vidc_hfi_api.h b/msm/vidc/vidc_hfi_api.h index 7ce6be36c68d..84b277520ac8 100644 --- a/msm/vidc/vidc_hfi_api.h +++ b/msm/vidc/vidc_hfi_api.h @@ -384,10 +384,9 @@ struct hal_fw_info { }; enum hal_flush { - HAL_FLUSH_INPUT, - HAL_FLUSH_OUTPUT, - HAL_FLUSH_ALL, - HAL_UNUSED_FLUSH = 0x10000000, + HAL_FLUSH_INPUT = BIT(0), + HAL_FLUSH_OUTPUT = BIT(1), + HAL_FLUSH_ALL = HAL_FLUSH_INPUT | HAL_FLUSH_OUTPUT, }; enum hal_event_type {