video: driver: introduce lock for set & get volatile controls

Acquire client and inst lock for s_ctrl and g_volatile_ctrl

Change-Id: I97341b69ea5390133c46711c38be51326c80b224
Signed-off-by: Darshana Patil <[email protected]>

driver/vidc/src/msm_vidc_control.c

@@ -19,6 +19,8 @@
 		(a) = 0;             \
 }
 
+extern struct msm_vidc_core *g_core;
+
 static bool is_priv_ctrl(u32 id)
 {
 	bool private = false;
@@ -1036,19 +1038,29 @@ int msm_v4l2_op_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 
 	inst = container_of(ctrl->handler,
 			    struct msm_vidc_inst, ctrl_handler);
+	inst = get_inst_ref(g_core, inst);
 	if (!inst) {
-		d_vpr_e("%s: could not find inst for ctrl %s id %#x\n", __func__, ctrl->name, ctrl->id);
+		d_vpr_e("%s: could not find inst for ctrl %s id %#x\n",
+			__func__, ctrl->name, ctrl->id);
 		return -EINVAL;
 	}
+	client_lock(inst, __func__);
+	inst_lock(inst, __func__);
 
 	rc = msm_vidc_get_control(inst, ctrl);
-	if (rc)
+	if (rc) {
 		i_vpr_e(inst, "%s: failed for ctrl %s id %#x\n",
 			__func__, ctrl->name, ctrl->id);
-	else
+		goto unlock;
+	} else {
 		i_vpr_h(inst, "%s: ctrl %s id %#x, value %d\n",
 			__func__, ctrl->name, ctrl->id, ctrl->val);
+	}
 
+unlock:
+	inst_unlock(inst, __func__);
+	client_unlock(inst, __func__);
+	put_inst(inst);
 	return rc;
 }
 
@@ -1147,25 +1159,30 @@ int msm_v4l2_op_s_ctrl(struct v4l2_ctrl *ctrl)
 
 	inst = container_of(ctrl->handler,
 		struct msm_vidc_inst, ctrl_handler);
-
+	inst = get_inst_ref(g_core, inst);
 	if (!inst || !inst->capabilities) {
 		d_vpr_e("%s: invalid parameters for inst\n", __func__);
 		return -EINVAL;
 	}
 
+	client_lock(inst, __func__);
+	inst_lock(inst, __func__);
 	capability = inst->capabilities;
 
 	i_vpr_h(inst, FMT_STRING_SET_CTRL,
 		__func__, state_name(inst->state), ctrl->name, ctrl->id, ctrl->val);
 
-	if (!msm_vidc_allow_s_ctrl(inst, ctrl->id))
-		return -EINVAL;
+	if (!msm_vidc_allow_s_ctrl(inst, ctrl->id)) {
+		rc = -EINVAL;
+		goto unlock;
+	}
 
 	cap_id = msm_vidc_get_cap_id(inst, ctrl->id);
 	if (!is_valid_cap_id(cap_id)) {
 		i_vpr_e(inst, "%s: could not find cap_id for ctrl %s\n",
 			__func__, ctrl->name);
-		return -EINVAL;
+		rc = -EINVAL;
+		goto unlock;
 	}
 
 	if (ctrl->id == V4L2_CID_MPEG_VIDC_INPUT_METADATA_FD) {
@@ -1173,16 +1190,18 @@ int msm_v4l2_op_s_ctrl(struct v4l2_ctrl *ctrl)
 			i_vpr_e(inst,
 				"%s: client configured invalid input metadata fd %d\n",
 				__func__, ctrl->val);
-			return 0;
+			rc = 0;
+			goto unlock;
 		}
 		if (!capability->cap[INPUT_META_VIA_REQUEST].value) {
 			i_vpr_e(inst,
 				"%s: input metadata not enabled via request\n", __func__);
-			return -EINVAL;
+			rc = -EINVAL;
+			goto unlock;
 		}
 		rc = msm_vidc_create_input_metadata_buffer(inst, ctrl->val);
 		if (rc)
-			return rc;
+			goto unlock;
 	}
 
 	/* mark client set flag */
@@ -1193,18 +1212,22 @@ int msm_v4l2_op_s_ctrl(struct v4l2_ctrl *ctrl)
 		/* static case */
 		rc = msm_vidc_update_static_property(inst, cap_id, ctrl);
 		if (rc)
-			return rc;
+			goto unlock;
 	} else {
 		/* dynamic case */
 		rc = msm_vidc_adjust_dynamic_property(inst, cap_id, ctrl);
 		if (rc)
-			return rc;
+			goto unlock;
 
 		rc = msm_vidc_set_dynamic_property(inst);
 		if (rc)
-			return rc;
+			goto unlock;
 	}
 
+unlock:
+	inst_unlock(inst, __func__);
+	client_unlock(inst, __func__);
+	put_inst(inst);
 	return rc;
 }
 

driver/vidc/src/msm_vidc_driver.c

@@ -2102,18 +2102,6 @@ int msm_vidc_get_fence_fd(struct msm_vidc_inst *inst, int *fence_fd)
 		return -EINVAL;
 	}
 
-	/*
-	 * Acquire inst->lock for fence fd creation
-	 * to avoid sync_file_create() and sync_file_poll()
-	 * race conditions in e2e playback usecase.
-	 */
-	inst = get_inst_ref(g_core, inst);
-	if (!inst) {
-		d_vpr_e("%s: invalid params\n", __func__);
-		return -EINVAL;
-	}
-	inst_lock(inst, __func__);
-
 	list_for_each_entry_safe(fence, dummy_fence, &inst->fence_list, list) {
 		if (fence->dma_fence.seqno ==
 			(u64)inst->capabilities->cap[FENCE_ID].value) {
@@ -2137,8 +2125,6 @@ int msm_vidc_get_fence_fd(struct msm_vidc_inst *inst, int *fence_fd)
 	*fence_fd = fence->fd;
 
 exit:
-	inst_unlock(inst, __func__);
-	put_inst(inst);
 	return rc;
 }
 
@@ -3914,7 +3900,6 @@ static int msm_vidc_vb2_buffer_done(struct msm_vidc_inst *inst,
 	vbuf->flags = buf->flags;
 	vb2->timestamp = buf->timestamp;
 	vb2->planes[0].bytesused = buf->data_size + vb2->planes[0].data_offset;
-	v4l2_ctrl_request_complete(vb2->req_obj.req, &inst->ctrl_handler);
 	vb2_buffer_done(vb2, state);
 
 	return 0;

driver/vidc/src/msm_vidc_vb2.c

@@ -447,8 +447,33 @@ void msm_vidc_buf_queue(struct vb2_buffer *vb2)
 		return;
 	}
 
+	/*
+	 * As part of every qbuf initalise request to true.
+	 * If there are any dynamic controls associated with qbuf,
+	 * they will set as part s_ctrl() from v4l2_ctrl_request_setup().
+	 * Once v4l2_ctrl_request_setup() is done, reset request variable.
+	 * If the buffer does not have any requests with it, then
+	 * v4l2_ctrl_request_setup() will return 0.
+	 */
+	inst->request = true;
+	rc = v4l2_ctrl_request_setup(vb2->req_obj.req,
+			&inst->ctrl_handler);
+	inst->request = false;
+	v4l2_ctrl_request_complete(vb2->req_obj.req, &inst->ctrl_handler);
+	/*
+	 * call request_setup and request_complete without acquiring lock
+	 * to avoid deadlock issues because request_setup or request_complete
+	 * would call .s_ctrl and .g_volatile_ctrl respectively which acquire
+	 * lock too.
+	 */
 	client_lock(inst, __func__);
 	inst_lock(inst, __func__);
+	if (rc) {
+		i_vpr_e(inst, "%s: request setup failed, error %d\n",
+			__func__, rc);
+		goto unlock;
+	}
+
 	if (is_session_error(inst)) {
 		i_vpr_e(inst, "%s: inst in error state\n", __func__);
 		rc = -EINVAL;
@@ -487,25 +512,6 @@ void msm_vidc_buf_queue(struct vb2_buffer *vb2)
 			goto unlock;
 	}
 
-	/*
-	 * As part of every qbuf initalise request to true.
-	 * If there are any dynamic controls associated with qbuf,
-	 * they will set as part s_ctrl() from v4l2_ctrl_request_setup().
-	 * Once v4l2_ctrl_request_setup() is done, reset request variable.
-	 * If the buffer does not have any requests with it, then
-	 * v4l2_ctrl_request_setup() will return 0.
-	 */
-	inst->request = true;
-	rc = v4l2_ctrl_request_setup(vb2->req_obj.req,
-			&inst->ctrl_handler);
-	if (rc) {
-		inst->request = false;
-		i_vpr_e(inst, "%s: request setup failed, error %d\n",
-			__func__, rc);
-		goto unlock;
-	}
-	inst->request = false;
-
 	if (inst->capabilities->cap[INPUT_META_VIA_REQUEST].value) {
 		rc = msm_vidc_update_input_meta_buffer_index(inst, vb2);
 		if (rc)
@@ -526,7 +532,6 @@ void msm_vidc_buf_queue(struct vb2_buffer *vb2)
 unlock:
 	if (rc) {
 		msm_vidc_change_inst_state(inst, MSM_VIDC_ERROR, __func__);
-		v4l2_ctrl_request_complete(vb2->req_obj.req, &inst->ctrl_handler);
 		vb2_buffer_done(vb2, VB2_BUF_STATE_ERROR);
 	}
 	inst_unlock(inst, __func__);