[PATCH 0/3] smiapp PM improvements, omap3isp system crash fix

[PATCH 0/3] smiapp PM improvements, omap3isp system crash fix

[Sakari Ailus]

Hi folks,

Here's a few patches remaining from my previous smiapp and omap3isp sets.

Sakari Ailus (3):
  omap3isp: Don't restart CCDC if we're about to stop
  smiapp: Avoid maintaining power state information
  smiapp: Put the device again if starting streaming fails

 drivers/media/i2c/smiapp/smiapp-core.c    | 198 +++++++++++++---------
 drivers/media/i2c/smiapp/smiapp-regs.c    |   3 -
 drivers/media/i2c/smiapp/smiapp.h         |   1 -
 drivers/media/platform/omap3isp/ispccdc.c |  12 +-
 4 files changed, 122 insertions(+), 92 deletions(-)

-- 
2.20.1

[PATCH 1/3] omap3isp: Don't restart CCDC if we're about to stop

[Sakari Ailus]

The omap3isp driver set the new buffer and enabled the CCDC in a situation
a new buffer was available but streaming was about to be stopped on the
CCDC. This lead to frequent system crashes in case there were buffers
queued when streming was being stopped.

Fix this by first checking whether there's an intent to stop streaming and
if there isn't, then set the new buffer and re-enable CCDC.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/ispccdc.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index e2f336c715a4..471ae7cdb813 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1607,6 +1607,11 @@ static int ccdc_isr_buffer(struct isp_ccdc_device *ccdc)
 		return 0;
 	}
 
+	/* Don't restart CCDC if we're just about to stop streaming. */
+	if (ccdc->state == ISP_PIPELINE_STREAM_CONTINUOUS &&
+	    ccdc->stopping & CCDC_STOP_REQUEST)
+		return 0;
+
 	if (!ccdc_has_all_fields(ccdc))
 		return 1;
 
@@ -1661,16 +1666,15 @@ static void ccdc_vd0_isr(struct isp_ccdc_device *ccdc)
 		spin_unlock_irqrestore(&ccdc->lock, flags);
 	}
 
-	if (ccdc->output & CCDC_OUTPUT_MEMORY)
-		restart = ccdc_isr_buffer(ccdc);
-
 	spin_lock_irqsave(&ccdc->lock, flags);
-
 	if (ccdc_handle_stopping(ccdc, CCDC_EVENT_VD0)) {
 		spin_unlock_irqrestore(&ccdc->lock, flags);
 		return;
 	}
 
+	if (ccdc->output & CCDC_OUTPUT_MEMORY)
+		restart = ccdc_isr_buffer(ccdc);
+
 	if (!ccdc->shadow_update)
 		ccdc_apply_controls(ccdc);
 	spin_unlock_irqrestore(&ccdc->lock, flags);
-- 
2.20.1

[PATCH 2/3] smiapp: Avoid maintaining power state information

[Sakari Ailus]

Instead of keeping track of the power state ourselves, let runtime PM
handle it.

This also splits handling controls between side effect management and
writing the new configuration to the sensor's registers.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c | 199 ++++++++++++++-----------
 drivers/media/i2c/smiapp/smiapp-regs.c |   3 -
 drivers/media/i2c/smiapp/smiapp.h      |   1 -
 3 files changed, 113 insertions(+), 90 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 2af796abb6b8..bcc703b83309 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -413,21 +413,14 @@ static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
 	struct smiapp_sensor *sensor =
 		container_of(ctrl->handler, struct smiapp_subdev, ctrl_handler)
 			->sensor;
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
+	int pm_status;
 	u32 orient = 0;
+	unsigned int i;
 	int exposure;
 	int rval;
 
 	switch (ctrl->id) {
-	case V4L2_CID_ANALOGUE_GAIN:
-		return smiapp_write(
-			sensor,
-			SMIAPP_REG_U16_ANALOGUE_GAIN_CODE_GLOBAL, ctrl->val);
-
-	case V4L2_CID_EXPOSURE:
-		return smiapp_write(
-			sensor,
-			SMIAPP_REG_U16_COARSE_INTEGRATION_TIME, ctrl->val);
-
 	case V4L2_CID_HFLIP:
 	case V4L2_CID_VFLIP:
 		if (sensor->streaming)
@@ -440,15 +433,10 @@ static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
 			orient |= SMIAPP_IMAGE_ORIENTATION_VFLIP;
 
 		orient ^= sensor->hvflip_inv_mask;
-		rval = smiapp_write(sensor, SMIAPP_REG_U8_IMAGE_ORIENTATION,
-				    orient);
-		if (rval < 0)
-			return rval;
 
 		smiapp_update_mbus_formats(sensor);
 
-		return 0;
-
+		break;
 	case V4L2_CID_VBLANK:
 		exposure = sensor->exposure->val;
 
@@ -461,59 +449,105 @@ static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
 				return rval;
 		}
 
-		return smiapp_write(
-			sensor, SMIAPP_REG_U16_FRAME_LENGTH_LINES,
-			sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].height
-			+ ctrl->val);
-
-	case V4L2_CID_HBLANK:
-		return smiapp_write(
-			sensor, SMIAPP_REG_U16_LINE_LENGTH_PCK,
-			sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].width
-			+ ctrl->val);
-
+		break;
 	case V4L2_CID_LINK_FREQ:
 		if (sensor->streaming)
 			return -EBUSY;
 
-		return smiapp_pll_update(sensor);
-
-	case V4L2_CID_TEST_PATTERN: {
-		unsigned int i;
+		rval = smiapp_pll_update(sensor);
+		if (rval)
+			return rval;
 
+		return 0;
+	case V4L2_CID_TEST_PATTERN:
 		for (i = 0; i < ARRAY_SIZE(sensor->test_data); i++)
 			v4l2_ctrl_activate(
 				sensor->test_data[i],
 				ctrl->val ==
 				V4L2_SMIAPP_TEST_PATTERN_MODE_SOLID_COLOUR);
 
-		return smiapp_write(
-			sensor, SMIAPP_REG_U16_TEST_PATTERN_MODE, ctrl->val);
+		break;
 	}
 
+	pm_runtime_get_noresume(&client->dev);
+	pm_status = pm_runtime_get_if_in_use(&client->dev);
+	pm_runtime_put_noidle(&client->dev);
+	if (!pm_status)
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_ANALOGUE_GAIN:
+		rval = smiapp_write(
+			sensor,
+			SMIAPP_REG_U16_ANALOGUE_GAIN_CODE_GLOBAL, ctrl->val);
+
+		break;
+	case V4L2_CID_EXPOSURE:
+		rval = smiapp_write(
+			sensor,
+			SMIAPP_REG_U16_COARSE_INTEGRATION_TIME, ctrl->val);
+
+		break;
+	case V4L2_CID_HFLIP:
+	case V4L2_CID_VFLIP:
+		rval = smiapp_write(sensor, SMIAPP_REG_U8_IMAGE_ORIENTATION,
+				    orient);
+
+		break;
+	case V4L2_CID_VBLANK:
+		rval = smiapp_write(
+			sensor, SMIAPP_REG_U16_FRAME_LENGTH_LINES,
+			sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].height
+			+ ctrl->val);
+
+		break;
+	case V4L2_CID_HBLANK:
+		rval = smiapp_write(
+			sensor, SMIAPP_REG_U16_LINE_LENGTH_PCK,
+			sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].width
+			+ ctrl->val);
+
+		break;
+	case V4L2_CID_TEST_PATTERN:
+		rval = smiapp_write(
+			sensor, SMIAPP_REG_U16_TEST_PATTERN_MODE, ctrl->val);
+
+		break;
 	case V4L2_CID_TEST_PATTERN_RED:
-		return smiapp_write(
+		rval = smiapp_write(
 			sensor, SMIAPP_REG_U16_TEST_DATA_RED, ctrl->val);
 
+		break;
 	case V4L2_CID_TEST_PATTERN_GREENR:
-		return smiapp_write(
+		rval = smiapp_write(
 			sensor, SMIAPP_REG_U16_TEST_DATA_GREENR, ctrl->val);
 
+		break;
 	case V4L2_CID_TEST_PATTERN_BLUE:
-		return smiapp_write(
+		rval = smiapp_write(
 			sensor, SMIAPP_REG_U16_TEST_DATA_BLUE, ctrl->val);
 
+		break;
 	case V4L2_CID_TEST_PATTERN_GREENB:
-		return smiapp_write(
+		rval = smiapp_write(
 			sensor, SMIAPP_REG_U16_TEST_DATA_GREENB, ctrl->val);
 
+		break;
 	case V4L2_CID_PIXEL_RATE:
 		/* For v4l2_ctrl_s_ctrl_int64() used internally. */
-		return 0;
+		rval = 0;
 
+		break;
 	default:
-		return -EINVAL;
+		rval = -EINVAL;
 	}
+
+	if (pm_status > 0) {
+		pm_runtime_mark_last_busy(&client->dev);
+		pm_runtime_put_autosuspend(&client->dev);
+	}
+
+	return rval;
 }
 
 static const struct v4l2_ctrl_ops smiapp_ctrl_ops = {
@@ -1184,10 +1218,6 @@ static int smiapp_power_on(struct device *dev)
 	sleep = SMIAPP_RESET_DELAY(sensor->hwcfg->ext_clk);
 	usleep_range(sleep, sleep);
 
-	mutex_lock(&sensor->mutex);
-
-	sensor->active = true;
-
 	/*
 	 * Failures to respond to the address change command have been noticed.
 	 * Those failures seem to be caused by the sensor requiring a longer
@@ -1270,24 +1300,9 @@ static int smiapp_power_on(struct device *dev)
 		goto out_cci_addr_fail;
 	}
 
-	/* Are we still initialising...? If not, proceed with control setup. */
-	if (sensor->pixel_array) {
-		rval = __v4l2_ctrl_handler_setup(
-			&sensor->pixel_array->ctrl_handler);
-		if (rval)
-			goto out_cci_addr_fail;
-
-		rval = __v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
-		if (rval)
-			goto out_cci_addr_fail;
-	}
-
-	mutex_unlock(&sensor->mutex);
-
 	return 0;
 
 out_cci_addr_fail:
-	mutex_unlock(&sensor->mutex);
 	gpiod_set_value(sensor->xshutdown, 0);
 	clk_disable_unprepare(sensor->ext_clk);
 
@@ -1305,8 +1320,6 @@ static int smiapp_power_off(struct device *dev)
 	struct smiapp_sensor *sensor =
 		container_of(ssd, struct smiapp_sensor, ssds[0]);
 
-	mutex_lock(&sensor->mutex);
-
 	/*
 	 * Currently power/clock to lens are enable/disabled separately
 	 * but they are essentially the same signals. So if the sensor is
@@ -1319,10 +1332,6 @@ static int smiapp_power_off(struct device *dev)
 			     SMIAPP_REG_U8_SOFTWARE_RESET,
 			     SMIAPP_SOFTWARE_RESET);
 
-	sensor->active = false;
-
-	mutex_unlock(&sensor->mutex);
-
 	gpiod_set_value(sensor->xshutdown, 0);
 	clk_disable_unprepare(sensor->ext_clk);
 	usleep_range(5000, 5000);
@@ -1507,6 +1516,30 @@ static int smiapp_stop_streaming(struct smiapp_sensor *sensor)
  * V4L2 subdev video operations
  */
 
+static int smiapp_pm_get_init(struct smiapp_sensor *sensor)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
+	int rval;
+
+	rval = pm_runtime_get_sync(&client->dev);
+	if (rval < 0) {
+		if (rval != -EBUSY && rval != -EAGAIN)
+			pm_runtime_set_active(&client->dev);
+		pm_runtime_put_noidle(&client->dev);
+
+		return rval;
+	} else if (!rval) {
+		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
+					       ctrl_handler);
+		if (rval)
+			return rval;
+
+		return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
+	}
+
+	return 0;
+}
+
 static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
 {
 	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
@@ -1516,27 +1549,25 @@ static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
 	if (sensor->streaming == enable)
 		return 0;
 
-	if (enable) {
-		rval = pm_runtime_get_sync(&client->dev);
-		if (rval < 0) {
-			if (rval != -EBUSY && rval != -EAGAIN)
-				pm_runtime_set_active(&client->dev);
-			pm_runtime_put(&client->dev);
-			return rval;
-		}
-
-		sensor->streaming = true;
-
-		rval = smiapp_start_streaming(sensor);
-		if (rval < 0)
-			sensor->streaming = false;
-	} else {
-		rval = smiapp_stop_streaming(sensor);
+	if (!enable) {
+		smiapp_stop_streaming(sensor);
 		sensor->streaming = false;
 		pm_runtime_mark_last_busy(&client->dev);
 		pm_runtime_put_autosuspend(&client->dev);
+
+		return 0;
 	}
 
+	rval = smiapp_pm_get_init(sensor);
+	if (rval)
+		return rval;
+
+	sensor->streaming = true;
+
+	rval = smiapp_start_streaming(sensor);
+	if (rval < 0)
+		sensor->streaming = false;
+
 	return rval;
 }
 
@@ -2291,13 +2322,9 @@ smiapp_sysfs_nvm_read(struct device *dev, struct device_attribute *attr,
 	if (!sensor->dev_init_done)
 		return -EBUSY;
 
-	rval = pm_runtime_get_sync(&client->dev);
-	if (rval < 0) {
-		if (rval != -EBUSY && rval != -EAGAIN)
-			pm_runtime_set_active(&client->dev);
-		pm_runtime_put_noidle(&client->dev);
+	rval = smiapp_pm_get_init(sensor);
+	if (rval < 0)
 		return -ENODEV;
-	}
 
 	rval = smiapp_read_nvm(sensor, buf, PAGE_SIZE);
 	if (rval < 0) {
diff --git a/drivers/media/i2c/smiapp/smiapp-regs.c b/drivers/media/i2c/smiapp/smiapp-regs.c
index 0470e47c2f7a..ce8c1d47fbf0 100644
--- a/drivers/media/i2c/smiapp/smiapp-regs.c
+++ b/drivers/media/i2c/smiapp/smiapp-regs.c
@@ -223,9 +223,6 @@ int smiapp_write_no_quirk(struct smiapp_sensor *sensor, u32 reg, u32 val)
 	     len != SMIAPP_REG_32BIT) || flags)
 		return -EINVAL;
 
-	if (!sensor->active)
-		return 0;
-
 	msg.addr = client->addr;
 	msg.flags = 0; /* Write */
 	msg.len = 2 + len;
diff --git a/drivers/media/i2c/smiapp/smiapp.h b/drivers/media/i2c/smiapp/smiapp.h
index 3ab874a5deba..4837d80dc453 100644
--- a/drivers/media/i2c/smiapp/smiapp.h
+++ b/drivers/media/i2c/smiapp/smiapp.h
@@ -198,7 +198,6 @@ struct smiapp_sensor {
 
 	u8 hvflip_inv_mask; /* H/VFLIP inversion due to sensor orientation */
 	u8 frame_skip;
-	bool active; /* is the sensor powered on? */
 	u16 embedded_start; /* embedded data start line */
 	u16 embedded_end;
 	u16 image_start; /* image data start line */
-- 
2.20.1

[PATCH 3/3] smiapp: Put the device again if starting streaming fails

[Sakari Ailus]

If there was an error in starting streaming, put the runtime usage count
of the device.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index bcc703b83309..6bbad1c3b8fc 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -1565,8 +1565,11 @@ static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
 	sensor->streaming = true;
 
 	rval = smiapp_start_streaming(sensor);
-	if (rval < 0)
+	if (rval < 0) {
 		sensor->streaming = false;
+		pm_runtime_mark_last_busy(&client->dev);
+		pm_runtime_put_autosuspend(&client->dev);
+	}
 
 	return rval;
 }
-- 
2.20.1