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 <[email protected]>

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);

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;

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;

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_

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;
 

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;
 

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) {

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;