[PATCH] mt9t031: preserve output format on VIDIOC_S_CROP

[PATCH] mt9t031: preserve output format on VIDIOC_S_CROP

[Guennadi Liakhovetski]

Interpretation of the V4L2 API specification, according to which the 
VIDIOC_S_CROP ioctl for capture devices shall set the input window and 
preserve the scales, thus possibly changing the output window, seems to be 
incorrect. Switch to using a more intuitive definition, i.e., to 
preserving the output format while changing scales.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

Val, I do not have any mt9t031 hardware any more, could you, please, test 
this patch and verify, that it does indeed do, what's described above?

diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
index a9061bf..a604fa0 100644
--- a/drivers/media/video/mt9t031.c
+++ b/drivers/media/video/mt9t031.c
@@ -63,6 +63,8 @@
 struct mt9t031 {
 	struct v4l2_subdev subdev;
 	struct v4l2_rect rect;	/* Sensor window */
+	unsigned int out_width;
+	unsigned int out_height;
 	int model;	/* V4L2_IDENT_MT9T031* codes from v4l2-chip-ident.h */
 	u16 xskip;
 	u16 yskip;
@@ -284,6 +286,26 @@ static u16 mt9t031_skip(s32 *source, s32 target, s32 max)
 	return skip;
 }
 
+static u16 mt9t031_skip_out(s32 *source, s32 *target)
+{
+	unsigned int skip;
+
+	if (*source < *target + *target / 2) {
+		*target = *source;
+		return 1;
+	}
+
+	skip = (*source + *target / 2) / *target;
+	if (skip > 8) {
+		skip = 8;
+		*target = (*source + 4) / 8;
+	}
+	/* We try to preserve input, but with division we have to adjust it too */
+	*source = *target * skip;
+
+	return skip;
+}
+
 /* rect is the sensor rectangle, the caller guarantees parameter validity */
 static int mt9t031_set_params(struct i2c_client *client,
 			      struct v4l2_rect *rect, u16 xskip, u16 yskip)
@@ -393,6 +415,7 @@ static int mt9t031_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 	struct v4l2_rect rect = a->c;
 	struct i2c_client *client = sd->priv;
 	struct mt9t031 *mt9t031 = to_mt9t031(client);
+	u16 xskip, yskip;
 
 	rect.width = ALIGN(rect.width, 2);
 	rect.height = ALIGN(rect.height, 2);
@@ -403,7 +426,10 @@ static int mt9t031_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 	soc_camera_limit_side(&rect.top, &rect.height,
 		     MT9T031_ROW_SKIP, MT9T031_MIN_HEIGHT, MT9T031_MAX_HEIGHT);
 
-	return mt9t031_set_params(client, &rect, mt9t031->xskip, mt9t031->yskip);
+	xskip = mt9t031_skip_out(&rect.width, &mt9t031->out_width);
+	yskip = mt9t031_skip_out(&rect.height, &mt9t031->out_height);
+
+	return mt9t031_set_params(client, &rect, xskip, yskip);
 }
 
 static int mt9t031_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
@@ -437,8 +463,8 @@ static int mt9t031_g_fmt(struct v4l2_subdev *sd,
 	struct i2c_client *client = sd->priv;
 	struct mt9t031 *mt9t031 = to_mt9t031(client);
 
-	mf->width	= mt9t031->rect.width / mt9t031->xskip;
-	mf->height	= mt9t031->rect.height / mt9t031->yskip;
+	mf->width	= mt9t031->out_width;
+	mf->height	= mt9t031->out_height;
 	mf->code	= V4L2_MBUS_FMT_SBGGR10_1X10;
 	mf->colorspace	= V4L2_COLORSPACE_SRGB;
 	mf->field	= V4L2_FIELD_NONE;
@@ -455,8 +481,9 @@ static int mt9t031_s_fmt(struct v4l2_subdev *sd,
 	struct v4l2_rect rect = mt9t031->rect;
 
 	/*
-	 * try_fmt has put width and height within limits.
-	 * S_FMT: use binning and skipping for scaling
+	 * try_fmt has put width and height within limits. Note: when converting
+	 * to a generic v4l2-subdev driver, try_fmt will have to be called
+	 * explicitly. S_FMT: use binning and skipping for scaling.
 	 */
 	xskip = mt9t031_skip(&rect.width, mf->width, MT9T031_MAX_WIDTH);
 	yskip = mt9t031_skip(&rect.height, mf->height, MT9T031_MAX_HEIGHT);
@@ -464,6 +491,9 @@ static int mt9t031_s_fmt(struct v4l2_subdev *sd,
 	mf->code	= V4L2_MBUS_FMT_SBGGR10_1X10;
 	mf->colorspace	= V4L2_COLORSPACE_SRGB;
 
+	mt9t031->out_width	= mf->width;
+	mt9t031->out_height	= mf->height;
+
 	/* mt9t031_set_params() doesn't change width and height */
 	return mt9t031_set_params(client, &rect, xskip, yskip);
 }
@@ -807,6 +837,9 @@ static int mt9t031_probe(struct i2c_client *client,
 	mt9t031->rect.width	= MT9T031_MAX_WIDTH;
 	mt9t031->rect.height	= MT9T031_MAX_HEIGHT;
 
+	mt9t031->out_width	= MT9T031_MAX_WIDTH;
+	mt9t031->out_height	= MT9T031_MAX_HEIGHT;
+
 	/*
 	 * Simulated autoexposure. If enabled, we calculate shutter width
 	 * ourselves in the driver based on vertical blanking and frame width

Re: [PATCH] mt9t031: preserve output format on VIDIOC_S_CROP

[Guennadi Liakhovetski]

On Wed, 14 Apr 2010, Guennadi Liakhovetski wrote:

> Interpretation of the V4L2 API specification, according to which the 
> VIDIOC_S_CROP ioctl for capture devices shall set the input window and 
> preserve the scales, thus possibly changing the output window, seems to be 
> incorrect. Switch to using a more intuitive definition, i.e., to 
> preserving the output format while changing scales.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> Val, I do not have any mt9t031 hardware any more, could you, please, test 
> this patch and verify, that it does indeed do, what's described above?

There hasn't been any replies to this, so, I presume, this patch cannot be 
tested at present. Therefore I'm going to leave it out of my pull requests 
until it gets tested somehow.

Thanks
Guennadi

> 
> diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
> index a9061bf..a604fa0 100644
> --- a/drivers/media/video/mt9t031.c
> +++ b/drivers/media/video/mt9t031.c
> @@ -63,6 +63,8 @@
>  struct mt9t031 {
>  	struct v4l2_subdev subdev;
>  	struct v4l2_rect rect;	/* Sensor window */
> +	unsigned int out_width;
> +	unsigned int out_height;
>  	int model;	/* V4L2_IDENT_MT9T031* codes from v4l2-chip-ident.h */
>  	u16 xskip;
>  	u16 yskip;
> @@ -284,6 +286,26 @@ static u16 mt9t031_skip(s32 *source, s32 target, s32 max)
>  	return skip;
>  }
>  
> +static u16 mt9t031_skip_out(s32 *source, s32 *target)
> +{
> +	unsigned int skip;
> +
> +	if (*source < *target + *target / 2) {
> +		*target = *source;
> +		return 1;
> +	}
> +
> +	skip = (*source + *target / 2) / *target;
> +	if (skip > 8) {
> +		skip = 8;
> +		*target = (*source + 4) / 8;
> +	}
> +	/* We try to preserve input, but with division we have to adjust it too */
> +	*source = *target * skip;
> +
> +	return skip;
> +}
> +
>  /* rect is the sensor rectangle, the caller guarantees parameter validity */
>  static int mt9t031_set_params(struct i2c_client *client,
>  			      struct v4l2_rect *rect, u16 xskip, u16 yskip)
> @@ -393,6 +415,7 @@ static int mt9t031_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
>  	struct v4l2_rect rect = a->c;
>  	struct i2c_client *client = sd->priv;
>  	struct mt9t031 *mt9t031 = to_mt9t031(client);
> +	u16 xskip, yskip;
>  
>  	rect.width = ALIGN(rect.width, 2);
>  	rect.height = ALIGN(rect.height, 2);
> @@ -403,7 +426,10 @@ static int mt9t031_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
>  	soc_camera_limit_side(&rect.top, &rect.height,
>  		     MT9T031_ROW_SKIP, MT9T031_MIN_HEIGHT, MT9T031_MAX_HEIGHT);
>  
> -	return mt9t031_set_params(client, &rect, mt9t031->xskip, mt9t031->yskip);
> +	xskip = mt9t031_skip_out(&rect.width, &mt9t031->out_width);
> +	yskip = mt9t031_skip_out(&rect.height, &mt9t031->out_height);
> +
> +	return mt9t031_set_params(client, &rect, xskip, yskip);
>  }
>  
>  static int mt9t031_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
> @@ -437,8 +463,8 @@ static int mt9t031_g_fmt(struct v4l2_subdev *sd,
>  	struct i2c_client *client = sd->priv;
>  	struct mt9t031 *mt9t031 = to_mt9t031(client);
>  
> -	mf->width	= mt9t031->rect.width / mt9t031->xskip;
> -	mf->height	= mt9t031->rect.height / mt9t031->yskip;
> +	mf->width	= mt9t031->out_width;
> +	mf->height	= mt9t031->out_height;
>  	mf->code	= V4L2_MBUS_FMT_SBGGR10_1X10;
>  	mf->colorspace	= V4L2_COLORSPACE_SRGB;
>  	mf->field	= V4L2_FIELD_NONE;
> @@ -455,8 +481,9 @@ static int mt9t031_s_fmt(struct v4l2_subdev *sd,
>  	struct v4l2_rect rect = mt9t031->rect;
>  
>  	/*
> -	 * try_fmt has put width and height within limits.
> -	 * S_FMT: use binning and skipping for scaling
> +	 * try_fmt has put width and height within limits. Note: when converting
> +	 * to a generic v4l2-subdev driver, try_fmt will have to be called
> +	 * explicitly. S_FMT: use binning and skipping for scaling.
>  	 */
>  	xskip = mt9t031_skip(&rect.width, mf->width, MT9T031_MAX_WIDTH);
>  	yskip = mt9t031_skip(&rect.height, mf->height, MT9T031_MAX_HEIGHT);
> @@ -464,6 +491,9 @@ static int mt9t031_s_fmt(struct v4l2_subdev *sd,
>  	mf->code	= V4L2_MBUS_FMT_SBGGR10_1X10;
>  	mf->colorspace	= V4L2_COLORSPACE_SRGB;
>  
> +	mt9t031->out_width	= mf->width;
> +	mt9t031->out_height	= mf->height;
> +
>  	/* mt9t031_set_params() doesn't change width and height */
>  	return mt9t031_set_params(client, &rect, xskip, yskip);
>  }
> @@ -807,6 +837,9 @@ static int mt9t031_probe(struct i2c_client *client,
>  	mt9t031->rect.width	= MT9T031_MAX_WIDTH;
>  	mt9t031->rect.height	= MT9T031_MAX_HEIGHT;
>  
> +	mt9t031->out_width	= MT9T031_MAX_WIDTH;
> +	mt9t031->out_height	= MT9T031_MAX_HEIGHT;
> +
>  	/*
>  	 * Simulated autoexposure. If enabled, we calculate shutter width
>  	 * ourselves in the driver based on vertical blanking and frame width
> 

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

Re: [PATCH] mt9t031: preserve output format on VIDIOC_S_CROP

[Valentin Longchamp]

Guennadi Liakhovetski wrote:
> On Wed, 14 Apr 2010, Guennadi Liakhovetski wrote:
> 
>> Interpretation of the V4L2 API specification, according to which the 
>> VIDIOC_S_CROP ioctl for capture devices shall set the input window and 
>> preserve the scales, thus possibly changing the output window, seems to be 
>> incorrect. Switch to using a more intuitive definition, i.e., to 
>> preserving the output format while changing scales.
>>
>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> ---
>>
>> Val, I do not have any mt9t031 hardware any more, could you, please, test 
>> this patch and verify, that it does indeed do, what's described above?
> 
> There hasn't been any replies to this, so, I presume, this patch cannot be 
> tested at present. Therefore I'm going to leave it out of my pull requests 
> until it gets tested somehow.

Sorry Guennadi, the testing is on my todo-list, but I am getting nearer 
to the end of my thesis and I really am very busy at the moment. I hope 
I can give it a spin on a mt9t031 in the coming weeks.

Val

> 
> Thanks
> Guennadi
> 
>> diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
>> index a9061bf..a604fa0 100644
>> --- a/drivers/media/video/mt9t031.c
>> +++ b/drivers/media/video/mt9t031.c
>> @@ -63,6 +63,8 @@
>>  struct mt9t031 {
>>  	struct v4l2_subdev subdev;
>>  	struct v4l2_rect rect;	/* Sensor window */
>> +	unsigned int out_width;
>> +	unsigned int out_height;
>>  	int model;	/* V4L2_IDENT_MT9T031* codes from v4l2-chip-ident.h */
>>  	u16 xskip;
>>  	u16 yskip;
>> @@ -284,6 +286,26 @@ static u16 mt9t031_skip(s32 *source, s32 target, s32 max)
>>  	return skip;
>>  }
>>  
>> +static u16 mt9t031_skip_out(s32 *source, s32 *target)
>> +{
>> +	unsigned int skip;
>> +
>> +	if (*source < *target + *target / 2) {
>> +		*target = *source;
>> +		return 1;
>> +	}
>> +
>> +	skip = (*source + *target / 2) / *target;
>> +	if (skip > 8) {
>> +		skip = 8;
>> +		*target = (*source + 4) / 8;
>> +	}
>> +	/* We try to preserve input, but with division we have to adjust it too */
>> +	*source = *target * skip;
>> +
>> +	return skip;
>> +}
>> +
>>  /* rect is the sensor rectangle, the caller guarantees parameter validity */
>>  static int mt9t031_set_params(struct i2c_client *client,
>>  			      struct v4l2_rect *rect, u16 xskip, u16 yskip)
>> @@ -393,6 +415,7 @@ static int mt9t031_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
>>  	struct v4l2_rect rect = a->c;
>>  	struct i2c_client *client = sd->priv;
>>  	struct mt9t031 *mt9t031 = to_mt9t031(client);
>> +	u16 xskip, yskip;
>>  
>>  	rect.width = ALIGN(rect.width, 2);
>>  	rect.height = ALIGN(rect.height, 2);
>> @@ -403,7 +426,10 @@ static int mt9t031_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
>>  	soc_camera_limit_side(&rect.top, &rect.height,
>>  		     MT9T031_ROW_SKIP, MT9T031_MIN_HEIGHT, MT9T031_MAX_HEIGHT);
>>  
>> -	return mt9t031_set_params(client, &rect, mt9t031->xskip, mt9t031->yskip);
>> +	xskip = mt9t031_skip_out(&rect.width, &mt9t031->out_width);
>> +	yskip = mt9t031_skip_out(&rect.height, &mt9t031->out_height);
>> +
>> +	return mt9t031_set_params(client, &rect, xskip, yskip);
>>  }
>>  
>>  static int mt9t031_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
>> @@ -437,8 +463,8 @@ static int mt9t031_g_fmt(struct v4l2_subdev *sd,
>>  	struct i2c_client *client = sd->priv;
>>  	struct mt9t031 *mt9t031 = to_mt9t031(client);
>>  
>> -	mf->width	= mt9t031->rect.width / mt9t031->xskip;
>> -	mf->height	= mt9t031->rect.height / mt9t031->yskip;
>> +	mf->width	= mt9t031->out_width;
>> +	mf->height	= mt9t031->out_height;
>>  	mf->code	= V4L2_MBUS_FMT_SBGGR10_1X10;
>>  	mf->colorspace	= V4L2_COLORSPACE_SRGB;
>>  	mf->field	= V4L2_FIELD_NONE;
>> @@ -455,8 +481,9 @@ static int mt9t031_s_fmt(struct v4l2_subdev *sd,
>>  	struct v4l2_rect rect = mt9t031->rect;
>>  
>>  	/*
>> -	 * try_fmt has put width and height within limits.
>> -	 * S_FMT: use binning and skipping for scaling
>> +	 * try_fmt has put width and height within limits. Note: when converting
>> +	 * to a generic v4l2-subdev driver, try_fmt will have to be called
>> +	 * explicitly. S_FMT: use binning and skipping for scaling.
>>  	 */
>>  	xskip = mt9t031_skip(&rect.width, mf->width, MT9T031_MAX_WIDTH);
>>  	yskip = mt9t031_skip(&rect.height, mf->height, MT9T031_MAX_HEIGHT);
>> @@ -464,6 +491,9 @@ static int mt9t031_s_fmt(struct v4l2_subdev *sd,
>>  	mf->code	= V4L2_MBUS_FMT_SBGGR10_1X10;
>>  	mf->colorspace	= V4L2_COLORSPACE_SRGB;
>>  
>> +	mt9t031->out_width	= mf->width;
>> +	mt9t031->out_height	= mf->height;
>> +
>>  	/* mt9t031_set_params() doesn't change width and height */
>>  	return mt9t031_set_params(client, &rect, xskip, yskip);
>>  }
>> @@ -807,6 +837,9 @@ static int mt9t031_probe(struct i2c_client *client,
>>  	mt9t031->rect.width	= MT9T031_MAX_WIDTH;
>>  	mt9t031->rect.height	= MT9T031_MAX_HEIGHT;
>>  
>> +	mt9t031->out_width	= MT9T031_MAX_WIDTH;
>> +	mt9t031->out_height	= MT9T031_MAX_HEIGHT;
>> +
>>  	/*
>>  	 * Simulated autoexposure. If enabled, we calculate shutter width
>>  	 * ourselves in the driver based on vertical blanking and frame width
>>
> 
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/


-- 
Valentin Longchamp, PhD Student, EPFL-STI-LSRO1
valentin.longchamp@epfl.ch, Phone: +41216937827
http://people.epfl.ch/valentin.longchamp
MEB3494, Station 9, CH-1015 Lausanne

Re: [PATCH] mt9t031: preserve output format on VIDIOC_S_CROP

[Guennadi Liakhovetski]

On Thu, 22 Apr 2010, Valentin Longchamp wrote:

> Guennadi Liakhovetski wrote:
> > On Wed, 14 Apr 2010, Guennadi Liakhovetski wrote:
> > 
> > > Interpretation of the V4L2 API specification, according to which the
> > > VIDIOC_S_CROP ioctl for capture devices shall set the input window and
> > > preserve the scales, thus possibly changing the output window, seems to be
> > > incorrect. Switch to using a more intuitive definition, i.e., to
> > > preserving the output format while changing scales.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > ---
> > > 
> > > Val, I do not have any mt9t031 hardware any more, could you, please, test
> > > this patch and verify, that it does indeed do, what's described above?
> > 
> > There hasn't been any replies to this, so, I presume, this patch cannot be
> > tested at present. Therefore I'm going to leave it out of my pull requests
> > until it gets tested somehow.
> 
> Sorry Guennadi, the testing is on my todo-list, but I am getting nearer to the
> end of my thesis and I really am very busy at the moment. I hope I can give it
> a spin on a mt9t031 in the coming weeks.

Np, we can push it out with the next development cycle, noone is 
complaining, so, noone actually has a problem with the present version 
either, I just would prefer to get it fixed, but it's not urgent.

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