From b4a1aefdd1b35e04d8db287347c9caae5b73f618 Mon Sep 17 00:00:00 2001 From: Vedang Nagar Date: Mon, 27 Mar 2023 08:58:06 +0530 Subject: [PATCH] msm: vidc: Fix race condition between s_fmt & query_ctrl - In s_fmt, there might be a codec change and here we are free the ctrl handler, at the same time client may call query ctrl and this will lead to null pointer access. - To resolve null pointer access, do not free ctrl handler and add new codec controls to the same ctrl handler. Change-Id: Iee87d5cb4d65e31d405cb1fc9f82bebab696d027 Signed-off-by: Vedang Nagar --- driver/vidc/inc/msm_vidc_control.h | 4 +- driver/vidc/inc/msm_vidc_core.h | 3 +- driver/vidc/inc/msm_vidc_inst.h | 1 - driver/vidc/inc/msm_vidc_internal.h | 4 ++ driver/vidc/src/msm_vdec.c | 12 ++--- driver/vidc/src/msm_venc.c | 14 +++--- driver/vidc/src/msm_vidc_control.c | 68 ++++++++++++++++++++++------- driver/vidc/src/msm_vidc_driver.c | 20 +++++---- 8 files changed, 86 insertions(+), 40 deletions(-) diff --git a/driver/vidc/inc/msm_vidc_control.h b/driver/vidc/inc/msm_vidc_control.h index e63595d14f..75bd54c0c6 100644 --- a/driver/vidc/inc/msm_vidc_control.h +++ b/driver/vidc/inc/msm_vidc_control.h @@ -10,8 +10,8 @@ #include "msm_vidc_internal.h" #include "msm_vidc_inst.h" -int msm_vidc_ctrl_init(struct msm_vidc_inst *inst); -int msm_vidc_ctrl_deinit(struct msm_vidc_inst *inst); +int msm_vidc_ctrl_handler_init(struct msm_vidc_inst *inst, bool init); +int msm_vidc_ctrl_handler_deinit(struct msm_vidc_inst *inst); int msm_v4l2_op_s_ctrl(struct v4l2_ctrl *ctrl); int msm_v4l2_op_g_volatile_ctrl(struct v4l2_ctrl *ctrl); int msm_vidc_s_ctrl(struct msm_vidc_inst *inst, struct v4l2_ctrl *ctrl); diff --git a/driver/vidc/inc/msm_vidc_core.h b/driver/vidc/inc/msm_vidc_core.h index 2f67bdfc5b..10b2485703 100644 --- a/driver/vidc/inc/msm_vidc_core.h +++ b/driver/vidc/inc/msm_vidc_core.h @@ -86,7 +86,8 @@ struct msm_vidc_core { u32 intr_status; u32 spur_count; u32 reg_count; - u32 codecs_count; + u32 enc_codecs_count; + u32 dec_codecs_count; struct msm_vidc_core_capability capabilities[CORE_CAP_MAX+1]; struct msm_vidc_inst_capability *inst_caps; struct msm_vidc_mem_addr sfr; diff --git a/driver/vidc/inc/msm_vidc_inst.h b/driver/vidc/inc/msm_vidc_inst.h index 271b08ef15..fff133ff94 100644 --- a/driver/vidc/inc/msm_vidc_inst.h +++ b/driver/vidc/inc/msm_vidc_inst.h @@ -87,7 +87,6 @@ struct msm_vidc_inst { struct v4l2_fh event_handler; struct v4l2_m2m_dev *m2m_dev; struct v4l2_m2m_ctx *m2m_ctx; - struct v4l2_ctrl **ctrls; u32 num_ctrls; enum hfi_rate_control hfi_rc_type; enum hfi_layer_encoding_type hfi_layer_type; diff --git a/driver/vidc/inc/msm_vidc_internal.h b/driver/vidc/inc/msm_vidc_internal.h index 31bba18fd4..e7d84ad87b 100644 --- a/driver/vidc/inc/msm_vidc_internal.h +++ b/driver/vidc/inc/msm_vidc_internal.h @@ -1047,4 +1047,8 @@ struct msm_vidc_sfr { u8 rg_data[1]; }; +struct msm_vidc_ctrl_data { + bool skip_s_ctrl; +}; + #endif // _MSM_VIDC_INTERNAL_H_ diff --git a/driver/vidc/src/msm_vdec.c b/driver/vidc/src/msm_vdec.c index 3367056afb..e461146cf9 100644 --- a/driver/vidc/src/msm_vdec.c +++ b/driver/vidc/src/msm_vdec.c @@ -44,6 +44,10 @@ struct msm_vdec_prop_type_handle { static int msm_vdec_codec_change(struct msm_vidc_inst *inst, u32 v4l2_codec) { int rc = 0; + bool session_init = false; + + if (!inst->codec) + session_init = true; if (inst->codec && inst->fmts[INPUT_PORT].fmt.pix_mp.pixelformat == v4l2_codec) return 0; @@ -68,11 +72,7 @@ static int msm_vdec_codec_change(struct msm_vidc_inst *inst, u32 v4l2_codec) if (rc) goto exit; - rc = msm_vidc_ctrl_deinit(inst); - if (rc) - goto exit; - - rc = msm_vidc_ctrl_init(inst); + rc = msm_vidc_ctrl_handler_init(inst, session_init); if(rc) goto exit; @@ -2832,7 +2832,7 @@ int msm_vdec_inst_deinit(struct msm_vidc_inst *inst) } /* cancel pending batch work */ cancel_batch_work(inst); - rc = msm_vidc_ctrl_deinit(inst); + rc = msm_vidc_ctrl_handler_deinit(inst); if (rc) return rc; diff --git a/driver/vidc/src/msm_venc.c b/driver/vidc/src/msm_venc.c index febf85e643..cd1b1eb597 100644 --- a/driver/vidc/src/msm_venc.c +++ b/driver/vidc/src/msm_venc.c @@ -62,6 +62,10 @@ struct msm_venc_prop_type_handle { static int msm_venc_codec_change(struct msm_vidc_inst *inst, u32 v4l2_codec) { int rc = 0; + bool session_init = false; + + if (!inst->codec) + session_init = true; if (inst->codec && inst->fmts[OUTPUT_PORT].fmt.pix_mp.pixelformat == v4l2_codec) return 0; @@ -86,12 +90,8 @@ static int msm_venc_codec_change(struct msm_vidc_inst *inst, u32 v4l2_codec) if (rc) goto exit; - rc = msm_vidc_ctrl_deinit(inst); - if (rc) - goto exit; - - rc = msm_vidc_ctrl_init(inst); - if (rc) + rc = msm_vidc_ctrl_handler_init(inst, session_init); + if(rc) goto exit; rc = msm_vidc_update_buffer_count(inst, INPUT_PORT); @@ -2018,7 +2018,7 @@ int msm_venc_inst_deinit(struct msm_vidc_inst *inst) d_vpr_e("%s: invalid params\n", __func__); return -EINVAL; } - rc = msm_vidc_ctrl_deinit(inst); + rc = msm_vidc_ctrl_handler_deinit(inst); if (rc) return rc; diff --git a/driver/vidc/src/msm_vidc_control.c b/driver/vidc/src/msm_vidc_control.c index e84a9fd20d..08e07432ae 100644 --- a/driver/vidc/src/msm_vidc_control.c +++ b/driver/vidc/src/msm_vidc_control.c @@ -466,7 +466,7 @@ error: return rc; } -int msm_vidc_ctrl_deinit(struct msm_vidc_inst *inst) +int msm_vidc_ctrl_handler_deinit(struct msm_vidc_inst *inst) { if (!inst) { d_vpr_e("%s: invalid parameters\n", __func__); @@ -475,13 +475,11 @@ int msm_vidc_ctrl_deinit(struct msm_vidc_inst *inst) i_vpr_h(inst, "%s(): num ctrls %d\n", __func__, inst->num_ctrls); v4l2_ctrl_handler_free(&inst->ctrl_handler); memset(&inst->ctrl_handler, 0, sizeof(struct v4l2_ctrl_handler)); - msm_vidc_vmem_free((void **)&inst->ctrls); - inst->ctrls = NULL; return 0; } -int msm_vidc_ctrl_init(struct msm_vidc_inst *inst) +int msm_vidc_ctrl_handler_init(struct msm_vidc_inst *inst, bool init) { int rc = 0; struct msm_vidc_inst_cap *cap; @@ -489,6 +487,7 @@ int msm_vidc_ctrl_init(struct msm_vidc_inst *inst) int idx = 0; struct v4l2_ctrl_config ctrl_cfg = {0}; int num_ctrls = 0, ctrl_idx = 0; + u64 codecs_count, step_or_mask; if (!inst || !inst->core) { d_vpr_e("%s: invalid params\n", __func__); @@ -511,16 +510,18 @@ int msm_vidc_ctrl_init(struct msm_vidc_inst *inst) __func__); return -EINVAL; } - rc = msm_vidc_vmem_alloc(num_ctrls * sizeof(struct v4l2_ctrl *), - (void **)&inst->ctrls, __func__); - if (rc) - return rc; - rc = v4l2_ctrl_handler_init(&inst->ctrl_handler, num_ctrls); - if (rc) { - i_vpr_e(inst, "control handler init failed, %d\n", - inst->ctrl_handler.error); - goto error; + if (init) { + codecs_count = is_encode_session(inst) ? + core->enc_codecs_count : + core->dec_codecs_count; + rc = v4l2_ctrl_handler_init(&inst->ctrl_handler, + INST_CAP_MAX * codecs_count); + if (rc) { + i_vpr_e(inst, "control handler init failed, %d\n", + inst->ctrl_handler.error); + goto error; + } } for (idx = 0; idx < INST_CAP_MAX; idx++) { @@ -550,6 +551,32 @@ int msm_vidc_ctrl_init(struct msm_vidc_inst *inst) memset(&ctrl_cfg, 0, sizeof(struct v4l2_ctrl_config)); + /* + * few controls might have been already initialized in instance initialization, + * so modify the range values for them instead of initializing them again + */ + if (!init) { + struct msm_vidc_ctrl_data ctrl_priv_data; + + ctrl = v4l2_ctrl_find(&inst->ctrl_handler, cap[idx].v4l2_id); + if (ctrl) { + step_or_mask = (cap[idx].flags & CAP_FLAG_MENU) ? + ~(cap[idx].step_or_mask) : + cap[idx].step_or_mask; + memset(&ctrl_priv_data, 0, sizeof(struct msm_vidc_ctrl_data)); + ctrl_priv_data.skip_s_ctrl = true; + ctrl->priv = &ctrl_priv_data; + v4l2_ctrl_modify_range(ctrl, + cap[idx].min, + cap[idx].max, + step_or_mask, + cap[idx].value); + /* reset private data to null to ensure s_ctrl not skipped */ + ctrl->priv = NULL; + continue; + } + } + if (is_priv_ctrl(cap[idx].v4l2_id)) { /* add private control */ ctrl_cfg.def = cap[idx].value; @@ -631,7 +658,6 @@ int msm_vidc_ctrl_init(struct msm_vidc_inst *inst) ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; ctrl->flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE; - inst->ctrls[ctrl_idx] = ctrl; ctrl_idx++; } inst->num_ctrls = num_ctrls; @@ -639,7 +665,7 @@ int msm_vidc_ctrl_init(struct msm_vidc_inst *inst) return 0; error: - msm_vidc_ctrl_deinit(inst); + msm_vidc_ctrl_handler_deinit(inst); return rc; } @@ -894,6 +920,7 @@ int msm_vidc_s_ctrl(struct msm_vidc_inst *inst, struct v4l2_ctrl *ctrl) int msm_v4l2_op_s_ctrl(struct v4l2_ctrl *ctrl) { struct msm_vidc_inst *inst; + struct msm_vidc_ctrl_data *priv_ctrl_data; int rc = 0; if (!ctrl) { @@ -901,6 +928,17 @@ int msm_v4l2_op_s_ctrl(struct v4l2_ctrl *ctrl) return -EINVAL; } + /* + * v4l2_ctrl_modify_range may internally call s_ctrl + * which will again try to acquire lock leading to deadlock, + * Add check to avoid such scenario. + */ + priv_ctrl_data = ctrl->priv ? ctrl->priv : NULL; + if (priv_ctrl_data && priv_ctrl_data->skip_s_ctrl) { + d_vpr_l("%s: skip s_ctrl (%s)\n", __func__, ctrl->name); + return 0; + } + inst = container_of(ctrl->handler, struct msm_vidc_inst, ctrl_handler); inst = get_inst_ref(g_core, inst); if (!inst) { diff --git a/driver/vidc/src/msm_vidc_driver.c b/driver/vidc/src/msm_vidc_driver.c index 57664b9a31..2398746fc4 100644 --- a/driver/vidc/src/msm_vidc_driver.c +++ b/driver/vidc/src/msm_vidc_driver.c @@ -3935,6 +3935,7 @@ int msm_vidc_get_inst_capability(struct msm_vidc_inst *inst) { int rc = 0; int i; + u32 codecs_count = 0; struct msm_vidc_core *core; if (!inst || !inst->core) { @@ -3943,7 +3944,9 @@ int msm_vidc_get_inst_capability(struct msm_vidc_inst *inst) } core = inst->core; - for (i = 0; i < core->codecs_count; i++) { + codecs_count = core->enc_codecs_count + core->dec_codecs_count; + + for (i = 0; i < codecs_count; i++) { if (core->inst_caps[i].domain == inst->domain && core->inst_caps[i].codec == inst->codec) { i_vpr_h(inst, @@ -4061,8 +4064,8 @@ int msm_vidc_init_instance_caps(struct msm_vidc_core *core) { int rc = 0; u8 enc_valid_codecs, dec_valid_codecs; - u8 count_bits, enc_codec_count; - u8 codecs_count = 0; + u8 count_bits, codecs_count = 0; + u8 enc_codecs_count = 0, dec_codecs_count = 0; int i, j, check_bit; int num_platform_cap_data, num_platform_cap_dependency_data; struct msm_platform_inst_capability *platform_cap_data = NULL; @@ -4092,14 +4095,15 @@ int msm_vidc_init_instance_caps(struct msm_vidc_core *core) enc_valid_codecs = core->capabilities[ENC_CODECS].value; count_bits = enc_valid_codecs; - COUNT_BITS(count_bits, codecs_count); - enc_codec_count = codecs_count; + COUNT_BITS(count_bits, enc_codecs_count); + core->enc_codecs_count = enc_codecs_count; dec_valid_codecs = core->capabilities[DEC_CODECS].value; count_bits = dec_valid_codecs; - COUNT_BITS(count_bits, codecs_count); + COUNT_BITS(count_bits, dec_codecs_count); + core->dec_codecs_count = dec_codecs_count; - core->codecs_count = codecs_count; + codecs_count = enc_codecs_count + dec_codecs_count; rc = msm_vidc_vmem_alloc(codecs_count * sizeof(struct msm_vidc_inst_capability), (void **)&core->inst_caps, __func__); if (rc) @@ -4107,7 +4111,7 @@ int msm_vidc_init_instance_caps(struct msm_vidc_core *core) check_bit = 0; /* determine codecs for enc domain */ - for (i = 0; i < enc_codec_count; i++) { + for (i = 0; i < enc_codecs_count; i++) { while (check_bit < (sizeof(enc_valid_codecs) * 8)) { if (enc_valid_codecs & BIT(check_bit)) { core->inst_caps[i].domain = MSM_VIDC_ENCODER;