[PATCH 0/5] v4l: ctrls: Unlocked variants of (some) functions for driver's internal use

[PATCH 0/5] v4l: ctrls: Unlocked variants of (some) functions for driver's internal use

[Sakari Ailus]

Hi,

This patchset adds unlocked variants of control framework functions to
set controls and modify their range. As in many cases the driver internal
data structures are protected using the same lock the control handler uses,
thus either forcing to poke the control framework data structures directly
or releasing the lock which leads to serialisation issues.

Also use the new unlocked variants in the smiapp driver.

-- 
Kind regards,
Sakari

[PATCH 1/5] v4l: ctrls: Move control lock/unlock above the control access functions

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

The v4l2_ctrl_{,un}lock will be needed elsewhere. Define them before the
functions that perform operations on controls.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/media/v4l2-ctrls.h |   36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 16f7f26..2d17819 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -307,6 +307,24 @@ int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler *hdl,
   */
 void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl);
 
+/** v4l2_ctrl_lock() - Helper function to lock the handler
+  * associated with the control.
+  * @ctrl:	The control to lock.
+  */
+static inline void v4l2_ctrl_lock(struct v4l2_ctrl *ctrl)
+{
+	mutex_lock(ctrl->handler->lock);
+}
+
+/** v4l2_ctrl_unlock() - Helper function to unlock the handler
+  * associated with the control.
+  * @ctrl:	The control to unlock.
+  */
+static inline void v4l2_ctrl_unlock(struct v4l2_ctrl *ctrl)
+{
+	mutex_unlock(ctrl->handler->lock);
+}
+
 /** v4l2_ctrl_handler_setup() - Call the s_ctrl op for all controls belonging
   * to the handler to initialize the hardware to the current control values.
   * @hdl:	The control handler.
@@ -562,24 +580,6 @@ void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
 int v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
 			s32 min, s32 max, u32 step, s32 def);
 
-/** v4l2_ctrl_lock() - Helper function to lock the handler
-  * associated with the control.
-  * @ctrl:	The control to lock.
-  */
-static inline void v4l2_ctrl_lock(struct v4l2_ctrl *ctrl)
-{
-	mutex_lock(ctrl->handler->lock);
-}
-
-/** v4l2_ctrl_unlock() - Helper function to unlock the handler
-  * associated with the control.
-  * @ctrl:	The control to unlock.
-  */
-static inline void v4l2_ctrl_unlock(struct v4l2_ctrl *ctrl)
-{
-	mutex_unlock(ctrl->handler->lock);
-}
-
 /** v4l2_ctrl_notify() - Function to set a notify callback for a control.
   * @ctrl:	The control.
   * @notify:	The callback function.
-- 
1.7.10.4

[PATCH 2/5] v4l: ctrls: Provide an unlocked variant of v4l2_ctrl_modify_range()

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

Drivers may use the v4l2_ctrl_modify_range() internally as part of other
operations that need to be both serialised using a driver's lock which can
also be used to serialise access to the control handler. Provide an unlocked
version of the function, __v4l2_ctrl_modify_range() which then may be used
by drivers for the purpose.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c |   10 +++++-----
 include/media/v4l2-ctrls.h           |   18 ++++++++++++++++--
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 55c6832..7324ef0 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2886,12 +2886,14 @@ void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl, v4l2_ctrl_notify_fnc notify, void
 }
 EXPORT_SYMBOL(v4l2_ctrl_notify);
 
-int v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
-			s32 min, s32 max, u32 step, s32 def)
+int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
+			     s32 min, s32 max, u32 step, s32 def)
 {
 	int ret = check_range(ctrl->type, min, max, step, def);
 	struct v4l2_ext_control c;
 
+	lockdep_assert_held(ctrl->handler->lock);
+
 	switch (ctrl->type) {
 	case V4L2_CTRL_TYPE_INTEGER:
 	case V4L2_CTRL_TYPE_BOOLEAN:
@@ -2904,7 +2906,6 @@ int v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
 	default:
 		return -EINVAL;
 	}
-	v4l2_ctrl_lock(ctrl);
 	ctrl->minimum = min;
 	ctrl->maximum = max;
 	ctrl->step = step;
@@ -2916,10 +2917,9 @@ int v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
 		ret = set_ctrl(NULL, ctrl, &c, V4L2_EVENT_CTRL_CH_RANGE);
 	else
 		send_event(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE);
-	v4l2_ctrl_unlock(ctrl);
 	return ret;
 }
-EXPORT_SYMBOL(v4l2_ctrl_modify_range);
+EXPORT_SYMBOL(__v4l2_ctrl_modify_range);
 
 static int v4l2_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
 {
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 2d17819..371c4f1 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -560,6 +560,11 @@ void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active);
   */
 void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
 
+
+/** __v4l2_ctrl_modify_range() - Unlocked variant of v4l2_ctrl_modify_range() */
+int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
+			     s32 min, s32 max, u32 step, s32 def);
+
 /** v4l2_ctrl_modify_range() - Update the range of a control.
   * @ctrl:	The control to update.
   * @min:	The control's minimum value.
@@ -577,8 +582,17 @@ void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
   * This function assumes that the control handler is not locked and will
   * take the lock itself.
   */
-int v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
-			s32 min, s32 max, u32 step, s32 def);
+static inline int v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
+					 s32 min, s32 max, u32 step, s32 def)
+{
+	int rval;
+
+	v4l2_ctrl_lock(ctrl);
+	rval = __v4l2_ctrl_modify_range(ctrl, min, max, step, def);
+	v4l2_ctrl_unlock(ctrl);
+
+	return rval;
+}
 
 /** v4l2_ctrl_notify() - Function to set a notify callback for a control.
   * @ctrl:	The control.
-- 
1.7.10.4

[PATCH 3/5] smiapp: Use unlocked __v4l2_ctrl_modify_range()

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

Instead of modifying the control ranges directly by manipulating struct
v4l2_ctrl, use __v4l2_ctrl_modify_range() for the purpose.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |   47 ++++++++++----------------------
 1 file changed, 15 insertions(+), 32 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 446c82c..c669525 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -319,13 +319,7 @@ static void __smiapp_update_exposure_limits(struct smiapp_sensor *sensor)
 		+ sensor->vblank->val
 		- sensor->limits[SMIAPP_LIMIT_COARSE_INTEGRATION_TIME_MAX_MARGIN];
 
-	ctrl->maximum = max;
-	if (ctrl->default_value > max)
-		ctrl->default_value = max;
-	if (ctrl->val > max)
-		ctrl->val = max;
-	if (ctrl->cur.val > max)
-		ctrl->cur.val = max;
+	__v4l2_ctrl_modify_range(ctrl, ctrl->minimum, max, ctrl->step, max);
 }
 
 /*
@@ -782,36 +776,25 @@ static void smiapp_update_blanking(struct smiapp_sensor *sensor)
 {
 	struct v4l2_ctrl *vblank = sensor->vblank;
 	struct v4l2_ctrl *hblank = sensor->hblank;
+	int min, max;
 
-	vblank->minimum =
-		max_t(int,
-		      sensor->limits[SMIAPP_LIMIT_MIN_FRAME_BLANKING_LINES],
-		      sensor->limits[SMIAPP_LIMIT_MIN_FRAME_LENGTH_LINES_BIN] -
-		      sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].height);
-	vblank->maximum =
-		sensor->limits[SMIAPP_LIMIT_MAX_FRAME_LENGTH_LINES_BIN] -
+	min = max_t(int,
+		    sensor->limits[SMIAPP_LIMIT_MIN_FRAME_BLANKING_LINES],
+		    sensor->limits[SMIAPP_LIMIT_MIN_FRAME_LENGTH_LINES_BIN] -
+		    sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].height);
+	max = sensor->limits[SMIAPP_LIMIT_MAX_FRAME_LENGTH_LINES_BIN] -
 		sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].height;
 
-	vblank->val = clamp_t(int, vblank->val,
-			      vblank->minimum, vblank->maximum);
-	vblank->default_value = vblank->minimum;
-	vblank->val = vblank->val;
-	vblank->cur.val = vblank->val;
-
-	hblank->minimum =
-		max_t(int,
-		      sensor->limits[SMIAPP_LIMIT_MIN_LINE_LENGTH_PCK_BIN] -
-		      sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].width,
-		      sensor->limits[SMIAPP_LIMIT_MIN_LINE_BLANKING_PCK_BIN]);
-	hblank->maximum =
-		sensor->limits[SMIAPP_LIMIT_MAX_LINE_LENGTH_PCK_BIN] -
+	__v4l2_ctrl_modify_range(vblank, min, max, vblank->step, min);
+
+	min = max_t(int,
+		    sensor->limits[SMIAPP_LIMIT_MIN_LINE_LENGTH_PCK_BIN] -
+		    sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].width,
+		    sensor->limits[SMIAPP_LIMIT_MIN_LINE_BLANKING_PCK_BIN]);
+	max = sensor->limits[SMIAPP_LIMIT_MAX_LINE_LENGTH_PCK_BIN] -
 		sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].width;
 
-	hblank->val = clamp_t(int, hblank->val,
-			      hblank->minimum, hblank->maximum);
-	hblank->default_value = hblank->minimum;
-	hblank->val = hblank->val;
-	hblank->cur.val = hblank->val;
+	__v4l2_ctrl_modify_range(hblank, min, max, hblank->step, min);
 
 	__smiapp_update_exposure_limits(sensor);
 }
-- 
1.7.10.4

[PATCH 4/5] v4l: ctrls: Unlocked variants of v4l2_ctrl_s_ctrl{,_int64}()

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

Implement unlocked variants of v4l2_ctrl_s_ctrl() and
v4l2_ctrl_s_ctrl_int64(). As drivers need to set controls as they access
driver internal state elsewhere than in the control framework unlocked
variants of these functions become handy.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c |   26 ++++++++++++++++++++------
 include/media/v4l2-ctrls.h           |   27 +++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 7324ef0..36228b4 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2848,27 +2848,41 @@ int v4l2_subdev_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *control)
 }
 EXPORT_SYMBOL(v4l2_subdev_s_ctrl);
 
-int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
+int __v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
 {
 	struct v4l2_ext_control c;
+	int rval;
+
+	lockdep_assert_held(ctrl->handler->lock);
 
 	/* It's a driver bug if this happens. */
 	WARN_ON(!type_is_int(ctrl));
 	c.value = val;
-	return set_ctrl_lock(NULL, ctrl, &c);
+	rval = set_ctrl(NULL, ctrl, &c, 0);
+	if (!rval)
+		cur_to_user(&c, ctrl);
+
+	return rval;
 }
-EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
+EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl);
 
-int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl, s64 val)
+int __v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl, s64 val)
 {
 	struct v4l2_ext_control c;
+	int rval;
+
+	lockdep_assert_held(ctrl->handler->lock);
 
 	/* It's a driver bug if this happens. */
 	WARN_ON(ctrl->type != V4L2_CTRL_TYPE_INTEGER64);
 	c.value64 = val;
-	return set_ctrl_lock(NULL, ctrl, &c);
+	rval = set_ctrl(NULL, ctrl, &c, 0);
+	if (!rval)
+		cur_to_user(&c, ctrl);
+
+	return rval;
 }
-EXPORT_SYMBOL(v4l2_ctrl_s_ctrl_int64);
+EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_int64);
 
 void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl, v4l2_ctrl_notify_fnc notify, void *priv)
 {
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 371c4f1..00c1778 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -619,6 +619,8 @@ void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl, v4l2_ctrl_notify_fnc notify, void
   */
 s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
 
+/** __v4l2_ctrl_s_ctrl() - Unlocked variant of v4l2_ctrl_s_ctrl(). */
+int __v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
 /** v4l2_ctrl_s_ctrl() - Helper function to set the control's value from within a driver.
   * @ctrl:	The control.
   * @val:	The new value.
@@ -629,7 +631,16 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
   *
   * This function is for integer type controls only.
   */
-int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
+static inline int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
+{
+	int rval;
+
+	v4l2_ctrl_lock(ctrl);
+	rval = __v4l2_ctrl_s_ctrl(ctrl, val);
+	v4l2_ctrl_unlock(ctrl);
+
+	return rval;
+}
 
 /** v4l2_ctrl_g_ctrl_int64() - Helper function to get a 64-bit control's value from within a driver.
   * @ctrl:	The control.
@@ -642,6 +653,9 @@ int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
   */
 s64 v4l2_ctrl_g_ctrl_int64(struct v4l2_ctrl *ctrl);
 
+/** __v4l2_ctrl_s_ctrl_int64() - Unlocked variant of v4l2_ctrl_s_ctrl_int64(). */
+int __v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl, s64 val);
+
 /** v4l2_ctrl_s_ctrl_int64() - Helper function to set a 64-bit control's value from within a driver.
   * @ctrl:	The control.
   * @val:	The new value.
@@ -652,7 +666,16 @@ s64 v4l2_ctrl_g_ctrl_int64(struct v4l2_ctrl *ctrl);
   *
   * This function is for 64-bit integer type controls only.
   */
-int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl, s64 val);
+static inline int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl, s64 val)
+{
+	int rval;
+
+	v4l2_ctrl_lock(ctrl);
+	rval = __v4l2_ctrl_s_ctrl_int64(ctrl, val);
+	v4l2_ctrl_unlock(ctrl);
+
+	return rval;
+}
 
 /* Internal helper functions that deal with control events. */
 extern const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops;
-- 
1.7.10.4

[PATCH 5/5] smiapp: Set 64-bit integer control using v4l2_ctrl_s_ctrl_int64()

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

Don't manipulate struct v4l2_ctrl directly. Instead, use
v4l2_ctrl_s_ctrl_int64() to change the values.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index c669525..95d3d0e 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -297,8 +297,9 @@ static int smiapp_pll_update(struct smiapp_sensor *sensor)
 	if (rval < 0)
 		return rval;
 
-	sensor->pixel_rate_parray->cur.val64 = pll->vt_pix_clk_freq_hz;
-	sensor->pixel_rate_csi->cur.val64 = pll->pixel_rate_csi;
+	__v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate_parray,
+				 pll->vt_pix_clk_freq_hz);
+	__v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate_csi, pll->pixel_rate_csi);
 
 	return 0;
 }
@@ -471,6 +472,10 @@ static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
 
 		return smiapp_pll_update(sensor);
 
+	case V4L2_CID_PIXEL_RATE:
+		/* For v4l2_ctrl_s_ctrl_int64() used internally. */
+		return 0;
+
 	default:
 		return -EINVAL;
 	}
-- 
1.7.10.4