[KS workshop follow-up] multiple sensor contexts

[KS workshop follow-up] multiple sensor contexts

[Guennadi Liakhovetski]

Hi all

At the V4L/DVB workshop in Prague a couple of weeks ago possible merits of 
supporting multiple camera sensor contexts have been discussed. Such 
contexts are often promoted by camera manufacturers as a hardware 
optimization to support fast switching to the snapshot mode. Such a switch 
is often accompanied by a change of the frame format. Typically, a smaller 
frame is used for the preview mode and a larger frame is used for photo 
shooting. Those sensors provide 2 (or more) sets of frame size and data 
format registers and a single command to switch between them. The 
decision, whether or not to support these multiple camera contexts has 
been postponed until some measurements become available, how much time 
such a "fast switching" implementation would save us.

I took the mt9m111 driver, that supports mt9m111, mt9m131, and mt9m112 
camera sensors from Aptina. They do indeed implement two contexts, 
however, the driver first had to be somewhat reorganised to make use of 
them. I pushed my (highly!) experimental tree to

git://linuxtv.org/gliakhovetski/v4l-dvb.git staging-3.3

with the addition of the below debugging diff, that pre-programs a fixed 
format into the second context registers and switches to it, once a 
matching S_FMT is called. On the i.MX31 based pcm037 board, that I've got, 
this sensor is attached to the I2C bus #2, running at 20kHz. The explicit 
programming of the new format parameters measures to take around 27ms, 
which is also about what we win, when using the second context.

As for interpretation: firstly 20kHz is not much, I expect many other set 
ups to run much faster. But even if we accept, that on some hardware > 
20kHz doesn't work and we really lose 27ms when not using multiple 
register contexts, is it a lot? Thinking about my personal photographing 
experiences with cameras and camera-phones, I don't think, I'd notice a 
27ms latency;-) I don't think anything below 200ms really makes a 
difference and, I think, the major contributor to the snapshot latency is 
the need to synchronise on a frame, and, possibly skip or shoot several 
frames, instead of just one.

So, my conclusion would be: when working with "sane" camera sensors, i.e., 
those, where you don't have to reprogram 100s of registers from some magic 
tables to configure a different frame format (;-)), supporting several 
register contexts doesn't bring a huge advantage in terms of snapshot 
latency. OTOH, it can well happen, that at some point we anyway will have 
to support those multiple register contexts for some other reason.

Opinions?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index 45739a1..23ffcd9 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -13,6 +13,7 @@
 #include <linux/log2.h>
 #include <linux/gpio.h>
 #include <linux/delay.h>
+#include <linux/time.h>
 #include <linux/v4l2-mediabus.h>
 
 #include <media/soc_camera.h>
@@ -309,11 +310,10 @@ static int mt9m111_reg_mask(struct i2c_client *client, const u16 reg,
 	return ret;
 }
 
-static int mt9m111_set_context(struct mt9m111 *mt9m111,
-			       struct mt9m111_context *ctx)
+static int mt9m111_set_context(struct mt9m111 *mt9m111)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
-	return reg_write(CONTEXT_CONTROL, ctx->control);
+	return reg_write(CONTEXT_CONTROL, mt9m111->ctx->control);
 }
 
 static int mt9m111_setup_rect_ctx(struct mt9m111 *mt9m111,
@@ -349,10 +349,7 @@ static int mt9m111_setup_geometry(struct mt9m111 *mt9m111, struct v4l2_rect *rec
 	if (code != V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE) {
 		/* IFP in use, down-scaling possible */
 		if (!ret)
-			ret = mt9m111_setup_rect_ctx(mt9m111, &context_b,
-						     rect, width, height);
-		if (!ret)
-			ret = mt9m111_setup_rect_ctx(mt9m111, &context_a,
+			ret = mt9m111_setup_rect_ctx(mt9m111, mt9m111->ctx,
 						     rect, width, height);
 	}
 
@@ -523,11 +520,8 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
 		return -EINVAL;
 	}
 
-	ret = mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
+	ret = mt9m111_reg_mask(client, mt9m111->ctx->output_fmt_ctrl2,
 			       data_outfmt2, mask_outfmt2);
-	if (!ret)
-		ret = mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
-				       data_outfmt2, mask_outfmt2);
 
 	return ret;
 }
@@ -576,27 +570,53 @@ static int mt9m111_try_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+#define SNAPSHOT_WIDTH 640
+#define SNAPSHOT_HEIGHT 480
+#define SNAPSHOT_CODE V4L2_MBUS_FMT_SBGGR8_1X8
+
 static int mt9m111_s_fmt(struct v4l2_subdev *sd,
 			 struct v4l2_mbus_framefmt *mf)
 {
 	const struct mt9m111_datafmt *fmt;
 	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
 	struct v4l2_rect *rect = &mt9m111->rect;
+	struct timespec ts_1, ts_2;
 	int ret;
 
+	/* FIXME: testing only: an arbitrary policy: != VGA use context A */
+	if (mf->width != SNAPSHOT_WIDTH || mf->height != SNAPSHOT_HEIGHT ||
+	    mf->code != SNAPSHOT_CODE)
+		mt9m111->ctx = &context_a;
+	else
+		mt9m111->ctx = &context_b;
+
 	mt9m111_try_fmt(sd, mf);
 	fmt = mt9m111_find_datafmt(mt9m111, mf->code);
 	/* try_fmt() guarantees fmt != NULL && fmt->code == mf->code */
 
-	ret = mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
-	if (!ret)
-		ret = mt9m111_set_pixfmt(mt9m111, mf->code);
+	ret = mt9m111_set_context(mt9m111);
+	ktime_get_ts(&ts_1);
+	if (!mt9m111->power_count || mt9m111->ctx == &context_a) {
+		if (!ret)
+			ret = mt9m111_setup_geometry(mt9m111, rect,
+					mf->width, mf->height, mf->code);
+		if (!ret)
+			ret = mt9m111_set_pixfmt(mt9m111, mf->code);
+	}
+	ktime_get_ts(&ts_2);
 	if (!ret) {
 		mt9m111->width	= mf->width;
 		mt9m111->height	= mf->height;
 		mt9m111->fmt	= fmt;
 	}
 
+	if (ts_2.tv_nsec < ts_1.tv_nsec) {
+		ts_2.tv_nsec += 1000000000;
+		ts_2.tv_sec -= 1;
+	}
+
+	pr_info("Context switch took %lu.%09lus\n", ts_2.tv_sec - ts_1.tv_sec, ts_2.tv_nsec - ts_1.tv_nsec);
+
 	return ret;
 }
 
@@ -762,7 +782,7 @@ static int mt9m111_suspend(struct mt9m111 *mt9m111)
 
 static void mt9m111_restore_state(struct mt9m111 *mt9m111)
 {
-	mt9m111_set_context(mt9m111, mt9m111->ctx);
+	mt9m111_set_context(mt9m111);
 	mt9m111_set_pixfmt(mt9m111, mt9m111->fmt->code);
 	mt9m111_setup_geometry(mt9m111, &mt9m111->rect,
 			mt9m111->width, mt9m111->height, mt9m111->fmt->code);
@@ -780,23 +800,6 @@ static int mt9m111_resume(struct mt9m111 *mt9m111)
 	return ret;
 }
 
-static int mt9m111_init(struct mt9m111 *mt9m111)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
-	int ret;
-
-	/* Default HIGHPOWER context */
-	mt9m111->ctx = &context_b;
-	ret = mt9m111_enable(mt9m111);
-	if (!ret)
-		ret = mt9m111_reset(mt9m111);
-	if (!ret)
-		ret = mt9m111_set_context(mt9m111, mt9m111->ctx);
-	if (ret)
-		dev_err(&client->dev, "mt9m111 init failed: %d\n", ret);
-	return ret;
-}
-
 /*
  * Interface active, can use i2c. If it fails, it can indeed mean, that
  * this wasn't our capture interface, so, we wait for the right one
@@ -805,7 +808,6 @@ static int mt9m111_video_probe(struct i2c_client *client)
 {
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
 	s32 data;
-	int ret;
 
 	data = reg_read(CHIP_VERSION);
 
@@ -826,9 +828,8 @@ static int mt9m111_video_probe(struct i2c_client *client)
 		return -ENODEV;
 	}
 
-	ret = mt9m111_init(mt9m111);
-	if (ret)
-		return ret;
+	mt9m111->ctx = &context_b;
+
 	return v4l2_ctrl_handler_setup(&mt9m111->hdl);
 }
 
@@ -836,33 +837,45 @@ static int mt9m111_s_power(struct v4l2_subdev *sd, int on)
 {
 	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	int ret = 0;
+	int ret;
 
 	mutex_lock(&mt9m111->power_lock);
 
-	/*
-	 * If the power count is modified from 0 to != 0 or from != 0 to 0,
-	 * update the power state.
-	 */
-	if (mt9m111->power_count == !on) {
-		if (on) {
+	if (on) {
+		if (!mt9m111->power_count) {
+			struct v4l2_mbus_framefmt mf_snapshot = {
+				.width	= SNAPSHOT_WIDTH,
+				.height	= SNAPSHOT_HEIGHT,
+				.code	= SNAPSHOT_CODE,
+			};
+
+			/* Pre-program a fixed snapshot format */
+			mt9m111_s_fmt(&mt9m111->subdev, &mf_snapshot);
+
+			/* Default preview context */
+			mt9m111->ctx = &context_a;
+
 			ret = mt9m111_resume(mt9m111);
-			if (ret) {
+			if (ret < 0)
 				dev_err(&client->dev,
 					"Failed to resume the sensor: %d\n", ret);
-				goto out;
-			}
 		} else {
-			mt9m111_suspend(mt9m111);
+			ret = 0;
 		}
+		if (!ret)
+			mt9m111->power_count++;
+	} else {
+		if (WARN_ON(!mt9m111->power_count)) {
+			ret = -EINVAL;
+			goto out;
+		}
+		ret = mt9m111_suspend(mt9m111);
+		mt9m111->power_count--;
 	}
 
-	/* Update the power count. */
-	mt9m111->power_count += on ? 1 : -1;
-	WARN_ON(mt9m111->power_count < 0);
-
 out:
 	mutex_unlock(&mt9m111->power_lock);
+
 	return ret;
 }
 

Re: [KS workshop follow-up] multiple sensor contexts

[Sylwester Nawrocki]

Hi Guennadi,

On 11/07/2011 05:17 PM, Guennadi Liakhovetski wrote:
> Hi all
> 
> At the V4L/DVB workshop in Prague a couple of weeks ago possible merits of
> supporting multiple camera sensor contexts have been discussed. Such
> contexts are often promoted by camera manufacturers as a hardware
> optimization to support fast switching to the snapshot mode. Such a switch
> is often accompanied by a change of the frame format. Typically, a smaller
> frame is used for the preview mode and a larger frame is used for photo
> shooting. Those sensors provide 2 (or more) sets of frame size and data
> format registers and a single command to switch between them. The
> decision, whether or not to support these multiple camera contexts has
> been postponed until some measurements become available, how much time
> such a "fast switching" implementation would save us.
> 
> I took the mt9m111 driver, that supports mt9m111, mt9m131, and mt9m112
> camera sensors from Aptina. They do indeed implement two contexts,
> however, the driver first had to be somewhat reorganised to make use of
> them. I pushed my (highly!) experimental tree to
> 
> git://linuxtv.org/gliakhovetski/v4l-dvb.git staging-3.3
> 
> with the addition of the below debugging diff, that pre-programs a fixed
> format into the second context registers and switches to it, once a
> matching S_FMT is called. On the i.MX31 based pcm037 board, that I've got,
> this sensor is attached to the I2C bus #2, running at 20kHz. The explicit
> programming of the new format parameters measures to take around 27ms,
> which is also about what we win, when using the second context.

I was expecting the re-programming time being not significant like this.
I'll try to reserve some time next week to measure how long the sensor
re-programming takes in case of m5mols device. It has quite a few registers
to write but its I2C bus clock frequency is 400 kHz so perhaps the results 
will be similar to yours. 

> 
> As for interpretation: firstly 20kHz is not much, I expect many other set
> ups to run much faster. But even if we accept, that on some hardware>
> 20kHz doesn't work and we really lose 27ms when not using multiple
> register contexts, is it a lot? Thinking about my personal photographing
> experiences with cameras and camera-phones, I don't think, I'd notice a
> 27ms latency;-) I don't think anything below 200ms really makes a
> difference and, I think, the major contributor to the snapshot latency is
> the need to synchronise on a frame, and, possibly skip or shoot several
> frames, instead of just one.
> 
> So, my conclusion would be: when working with "sane" camera sensors, i.e.,
> those, where you don't have to reprogram 100s of registers from some magic
> tables to configure a different frame format (;-)), supporting several
> register contexts doesn't bring a huge advantage in terms of snapshot
> latency. OTOH, it can well happen, that at some point we anyway will have
> to support those multiple register contexts for some other reason.

Hmm, I'm wondering what should the drivers do in case of devices that require 
explicit setting of some control registers for still capture. Guessing on pixel
format basis is rather a poor men's solution. This is what M-5MOLS mainline driver
doeas - if JPEG format is set it switches to snaphot mode. This way YUV format
cannot be used in snaphot mode.
There are also distinct resolutions supported for snaphot mode, and it can't be
currently properly enumerated.

Hence my question is, should such (whatsoever rare) devices stick with _private_ 
controls ?

If we have used VIDIOC_STREAMON for triggering capture, something like boolean 
SNAPSHOT control is needed, for instance, to tell the sensor controller it should
fire the flash. 

--
Regards,
Sylwester

> 
> Opinions?
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/

Re: [KS workshop follow-up] multiple sensor contexts

[Guennadi Liakhovetski]

On Wed, 9 Nov 2011, Sylwester Nawrocki wrote:

> Hi Guennadi,
> 
> On 11/07/2011 05:17 PM, Guennadi Liakhovetski wrote:
> > Hi all
> > 
> > At the V4L/DVB workshop in Prague a couple of weeks ago possible merits of
> > supporting multiple camera sensor contexts have been discussed. Such
> > contexts are often promoted by camera manufacturers as a hardware
> > optimization to support fast switching to the snapshot mode. Such a switch
> > is often accompanied by a change of the frame format. Typically, a smaller
> > frame is used for the preview mode and a larger frame is used for photo
> > shooting. Those sensors provide 2 (or more) sets of frame size and data
> > format registers and a single command to switch between them. The
> > decision, whether or not to support these multiple camera contexts has
> > been postponed until some measurements become available, how much time
> > such a "fast switching" implementation would save us.
> > 
> > I took the mt9m111 driver, that supports mt9m111, mt9m131, and mt9m112
> > camera sensors from Aptina. They do indeed implement two contexts,
> > however, the driver first had to be somewhat reorganised to make use of
> > them. I pushed my (highly!) experimental tree to
> > 
> > git://linuxtv.org/gliakhovetski/v4l-dvb.git staging-3.3
> > 
> > with the addition of the below debugging diff, that pre-programs a fixed
> > format into the second context registers and switches to it, once a
> > matching S_FMT is called. On the i.MX31 based pcm037 board, that I've got,
> > this sensor is attached to the I2C bus #2, running at 20kHz. The explicit
> > programming of the new format parameters measures to take around 27ms,
> > which is also about what we win, when using the second context.
> 
> I was expecting the re-programming time being not significant like this.
> I'll try to reserve some time next week to measure how long the sensor
> re-programming takes in case of m5mols device. It has quite a few registers
> to write but its I2C bus clock frequency is 400 kHz so perhaps the results 
> will be similar to yours. 
> 
> > 
> > As for interpretation: firstly 20kHz is not much, I expect many other set
> > ups to run much faster. But even if we accept, that on some hardware>
> > 20kHz doesn't work and we really lose 27ms when not using multiple
> > register contexts, is it a lot? Thinking about my personal photographing
> > experiences with cameras and camera-phones, I don't think, I'd notice a
> > 27ms latency;-) I don't think anything below 200ms really makes a
> > difference and, I think, the major contributor to the snapshot latency is
> > the need to synchronise on a frame, and, possibly skip or shoot several
> > frames, instead of just one.
> > 
> > So, my conclusion would be: when working with "sane" camera sensors, i.e.,
> > those, where you don't have to reprogram 100s of registers from some magic
> > tables to configure a different frame format (;-)), supporting several
> > register contexts doesn't bring a huge advantage in terms of snapshot
> > latency. OTOH, it can well happen, that at some point we anyway will have
> > to support those multiple register contexts for some other reason.
> 
> Hmm, I'm wondering what should the drivers do in case of devices that require 
> explicit setting of some control registers for still capture. Guessing on pixel
> format basis is rather a poor men's solution. This is what M-5MOLS mainline driver
> doeas - if JPEG format is set it switches to snaphot mode. This way YUV format
> cannot be used in snaphot mode.
> There are also distinct resolutions supported for snaphot mode, and it can't be
> currently properly enumerated.
> 
> Hence my question is, should such (whatsoever rare) devices stick with _private_ 
> controls ?
> 
> If we have used VIDIOC_STREAMON for triggering capture, something like boolean 
> SNAPSHOT control is needed, for instance, to tell the sensor controller it should
> fire the flash. 

Yes, I think, that was one of the conclusions of the workshop on this 
topic: we need a new control (a common one, not a private one) to switch 
to snapshot mode. I think, it has been agreed, that someone, (who first 
hits a valid practical use-case?;-)) submits an RFC / patch for such a 
control with a sufficiently detailed description of its semantics...

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [KS workshop follow-up] multiple sensor contexts

[Sakari Ailus]

Hi Guennadi,

On Mon, Nov 07, 2011 at 05:17:23PM +0100, Guennadi Liakhovetski wrote:
> Hi all
> 
> At the V4L/DVB workshop in Prague a couple of weeks ago possible merits of 
> supporting multiple camera sensor contexts have been discussed. Such 
> contexts are often promoted by camera manufacturers as a hardware 
> optimization to support fast switching to the snapshot mode. Such a switch 
> is often accompanied by a change of the frame format. Typically, a smaller 
> frame is used for the preview mode and a larger frame is used for photo 
> shooting. Those sensors provide 2 (or more) sets of frame size and data 
> format registers and a single command to switch between them. The 
> decision, whether or not to support these multiple camera contexts has 
> been postponed until some measurements become available, how much time 
> such a "fast switching" implementation would save us.
> 
> I took the mt9m111 driver, that supports mt9m111, mt9m131, and mt9m112 
> camera sensors from Aptina. They do indeed implement two contexts, 
> however, the driver first had to be somewhat reorganised to make use of 
> them. I pushed my (highly!) experimental tree to
> 
> git://linuxtv.org/gliakhovetski/v4l-dvb.git staging-3.3
> 
> with the addition of the below debugging diff, that pre-programs a fixed 
> format into the second context registers and switches to it, once a 
> matching S_FMT is called. On the i.MX31 based pcm037 board, that I've got, 
> this sensor is attached to the I2C bus #2, running at 20kHz. The explicit 
> programming of the new format parameters measures to take around 27ms, 
> which is also about what we win, when using the second context.

27 ms isn't a lot. May I ask what's the reason for such an unusual I2C
speed? Even the relatively low speed 400 kHz spec has been available for
almost 20 years by now.

> As for interpretation: firstly 20kHz is not much, I expect many other set 
> ups to run much faster. But even if we accept, that on some hardware > 
> 20kHz doesn't work and we really lose 27ms when not using multiple 
> register contexts, is it a lot? Thinking about my personal photographing 
> experiences with cameras and camera-phones, I don't think, I'd notice a 
> 27ms latency;-) I don't think anything below 200ms really makes a 
> difference and, I think, the major contributor to the snapshot latency is 
> the need to synchronise on a frame, and, possibly skip or shoot several 
> frames, instead of just one.
> 
> So, my conclusion would be: when working with "sane" camera sensors, i.e., 
> those, where you don't have to reprogram 100s of registers from some magic 
> tables to configure a different frame format (;-)), supporting several 
> register contexts doesn't bring a huge advantage in terms of snapshot 
> latency. OTOH, it can well happen, that at some point we anyway will have 
> to support those multiple register contexts for some other reason.

Sensor have seldom anything  which would require passing a huge amount of
data to configure the sensor. I personally don't see need for supporting
different contects in the foreseeable future --- but of course I could be
wrong. Also knowing much of a future desired configuration is a difficult
guess. The hardware people will hopefully rather provide a faster bus to
transfer the settings to the sensor to make the configuration delays
smaller.

Kind regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk