* [PATCH v5] videodev2: Set vb2_rect's width and height as unsigned
@ 2013-11-06 18:42 Ricardo Ribalda Delgado
2013-11-07 17:25 ` Prabhakar Lad
2013-11-08 2:42 ` Laurent Pinchart
0 siblings, 2 replies; 7+ messages in thread
From: Ricardo Ribalda Delgado @ 2013-11-06 18:42 UTC (permalink / raw)
To: Laurent Pinchart, Mauro Carvalho Chehab, Sakari Ailus,
Guennadi Liakhovetski, Hans Verkuil, Lad, Prabhakar,
Frank Schäfer, Ondrej Zary, open list:MT9M032 APTINA SE...
Cc: Ricardo Ribalda Delgado
As discussed on the media summit 2013, there is no reason for the width
and height to be signed.
Therefore this patch is an attempt to convert those fields from __s32 to
__u32.
Acked-by: Sakari Ailus <sakari.ailus@iki.fi> (documentation and smiapp)
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
v5: Comments by Sakari Ailus
-Fix typos in summary
v4: Wrong patch format
v3: Comments by Sakari Ailus
-Update also doc
v2: Comments by Sakari Ailus and Laurent Pinchart
-Fix alignment on all drivers
-Replace min with min_t where possible and remove unneeded checks
Documentation/DocBook/media/v4l/compat.xml | 12 ++++++++
Documentation/DocBook/media/v4l/dev-overlay.xml | 8 ++---
Documentation/DocBook/media/v4l/vidioc-cropcap.xml | 8 ++---
drivers/media/i2c/mt9m032.c | 16 +++++-----
drivers/media/i2c/mt9p031.c | 28 ++++++++++--------
drivers/media/i2c/mt9t001.c | 26 ++++++++++-------
drivers/media/i2c/mt9v032.c | 34 ++++++++++++----------
drivers/media/i2c/smiapp/smiapp-core.c | 8 ++---
drivers/media/i2c/soc_camera/mt9m111.c | 4 +--
drivers/media/i2c/tvp5150.c | 14 ++++-----
drivers/media/pci/bt8xx/bttv-driver.c | 6 ++--
drivers/media/pci/saa7134/saa7134-video.c | 4 ---
drivers/media/platform/soc_camera/soc_scale_crop.c | 4 +--
include/uapi/linux/videodev2.h | 4 +--
14 files changed, 97 insertions(+), 79 deletions(-)
diff --git a/Documentation/DocBook/media/v4l/compat.xml b/Documentation/DocBook/media/v4l/compat.xml
index 0c7195e..5dbe68b 100644
--- a/Documentation/DocBook/media/v4l/compat.xml
+++ b/Documentation/DocBook/media/v4l/compat.xml
@@ -2523,6 +2523,18 @@ that used it. It was originally scheduled for removal in 2.6.35.
</orderedlist>
</section>
+ <section>
+ <title>V4L2 in Linux 3.12</title>
+ <orderedlist>
+ <listitem>
+ <para> In struct <structname>v4l2_rect</structname>, the type
+of <structfield>width</structfield> and <structfield>height</structfield>
+fields changed from _s32 to _u32.
+ </para>
+ </listitem>
+ </orderedlist>
+ </section>
+
<section id="other">
<title>Relation of V4L2 to other Linux multimedia APIs</title>
diff --git a/Documentation/DocBook/media/v4l/dev-overlay.xml b/Documentation/DocBook/media/v4l/dev-overlay.xml
index 40d1d76..a44ac66 100644
--- a/Documentation/DocBook/media/v4l/dev-overlay.xml
+++ b/Documentation/DocBook/media/v4l/dev-overlay.xml
@@ -346,16 +346,14 @@ rectangle, in pixels.</entry>
rectangle, in pixels. Offsets increase to the right and down.</entry>
</row>
<row>
- <entry>__s32</entry>
+ <entry>__u32</entry>
<entry><structfield>width</structfield></entry>
<entry>Width of the rectangle, in pixels.</entry>
</row>
<row>
- <entry>__s32</entry>
+ <entry>__u32</entry>
<entry><structfield>height</structfield></entry>
- <entry>Height of the rectangle, in pixels. Width and
-height cannot be negative, the fields are signed for hysterical
-reasons. <!-- video4linux-list@redhat.com on 22 Oct 2002 subject
+ <entry>Height of the rectangle, in pixels.<!-- video4linux-list@redhat.com on 22 Oct 2002 subject
"Re:[V4L][patches!] Re:v4l2/kernel-2.5" --></entry>
</row>
</tbody>
diff --git a/Documentation/DocBook/media/v4l/vidioc-cropcap.xml b/Documentation/DocBook/media/v4l/vidioc-cropcap.xml
index bf7cc97..26b8f8f 100644
--- a/Documentation/DocBook/media/v4l/vidioc-cropcap.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-cropcap.xml
@@ -133,16 +133,14 @@ rectangle, in pixels.</entry>
rectangle, in pixels.</entry>
</row>
<row>
- <entry>__s32</entry>
+ <entry>__u32</entry>
<entry><structfield>width</structfield></entry>
<entry>Width of the rectangle, in pixels.</entry>
</row>
<row>
- <entry>__s32</entry>
+ <entry>__u32</entry>
<entry><structfield>height</structfield></entry>
- <entry>Height of the rectangle, in pixels. Width
-and height cannot be negative, the fields are signed for
-hysterical reasons. <!-- video4linux-list@redhat.com
+ <entry>Height of the rectangle, in pixels.<!-- video4linux-list@redhat.com
on 22 Oct 2002 subject "Re:[V4L][patches!] Re:v4l2/kernel-2.5" -->
</entry>
</row>
diff --git a/drivers/media/i2c/mt9m032.c b/drivers/media/i2c/mt9m032.c
index 846b15f..85ec3ba 100644
--- a/drivers/media/i2c/mt9m032.c
+++ b/drivers/media/i2c/mt9m032.c
@@ -459,13 +459,15 @@ static int mt9m032_set_pad_crop(struct v4l2_subdev *subdev,
MT9M032_COLUMN_START_MAX);
rect.top = clamp(ALIGN(crop->rect.top, 2), MT9M032_ROW_START_MIN,
MT9M032_ROW_START_MAX);
- rect.width = clamp(ALIGN(crop->rect.width, 2), MT9M032_COLUMN_SIZE_MIN,
- MT9M032_COLUMN_SIZE_MAX);
- rect.height = clamp(ALIGN(crop->rect.height, 2), MT9M032_ROW_SIZE_MIN,
- MT9M032_ROW_SIZE_MAX);
-
- rect.width = min(rect.width, MT9M032_PIXEL_ARRAY_WIDTH - rect.left);
- rect.height = min(rect.height, MT9M032_PIXEL_ARRAY_HEIGHT - rect.top);
+ rect.width = clamp_t(unsigned int, ALIGN(crop->rect.width, 2),
+ MT9M032_COLUMN_SIZE_MIN, MT9M032_COLUMN_SIZE_MAX);
+ rect.height = clamp_t(unsigned int, ALIGN(crop->rect.height, 2),
+ MT9M032_ROW_SIZE_MIN, MT9M032_ROW_SIZE_MAX);
+
+ rect.width = min_t(unsigned int, rect.width,
+ MT9M032_PIXEL_ARRAY_WIDTH - rect.left);
+ rect.height = min_t(unsigned int, rect.height,
+ MT9M032_PIXEL_ARRAY_HEIGHT - rect.top);
__crop = __mt9m032_get_pad_crop(sensor, fh, crop->which);
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 4734836..722f78a 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -518,11 +518,13 @@ static int mt9p031_set_format(struct v4l2_subdev *subdev,
/* Clamp the width and height to avoid dividing by zero. */
width = clamp_t(unsigned int, ALIGN(format->format.width, 2),
- max(__crop->width / 7, MT9P031_WINDOW_WIDTH_MIN),
+ max_t(unsigned int, __crop->width / 7,
+ MT9P031_WINDOW_WIDTH_MIN),
__crop->width);
height = clamp_t(unsigned int, ALIGN(format->format.height, 2),
- max(__crop->height / 8, MT9P031_WINDOW_HEIGHT_MIN),
- __crop->height);
+ max_t(unsigned int, __crop->height / 8,
+ MT9P031_WINDOW_HEIGHT_MIN),
+ __crop->height);
hratio = DIV_ROUND_CLOSEST(__crop->width, width);
vratio = DIV_ROUND_CLOSEST(__crop->height, height);
@@ -564,15 +566,17 @@ static int mt9p031_set_crop(struct v4l2_subdev *subdev,
MT9P031_COLUMN_START_MAX);
rect.top = clamp(ALIGN(crop->rect.top, 2), MT9P031_ROW_START_MIN,
MT9P031_ROW_START_MAX);
- rect.width = clamp(ALIGN(crop->rect.width, 2),
- MT9P031_WINDOW_WIDTH_MIN,
- MT9P031_WINDOW_WIDTH_MAX);
- rect.height = clamp(ALIGN(crop->rect.height, 2),
- MT9P031_WINDOW_HEIGHT_MIN,
- MT9P031_WINDOW_HEIGHT_MAX);
-
- rect.width = min(rect.width, MT9P031_PIXEL_ARRAY_WIDTH - rect.left);
- rect.height = min(rect.height, MT9P031_PIXEL_ARRAY_HEIGHT - rect.top);
+ rect.width = clamp_t(unsigned int, ALIGN(crop->rect.width, 2),
+ MT9P031_WINDOW_WIDTH_MIN,
+ MT9P031_WINDOW_WIDTH_MAX);
+ rect.height = clamp_t(unsigned int, ALIGN(crop->rect.height, 2),
+ MT9P031_WINDOW_HEIGHT_MIN,
+ MT9P031_WINDOW_HEIGHT_MAX);
+
+ rect.width = min_t(unsigned int, rect.width,
+ MT9P031_PIXEL_ARRAY_WIDTH - rect.left);
+ rect.height = min_t(unsigned int, rect.height,
+ MT9P031_PIXEL_ARRAY_HEIGHT - rect.top);
__crop = __mt9p031_get_pad_crop(mt9p031, fh, crop->pad, crop->which);
diff --git a/drivers/media/i2c/mt9t001.c b/drivers/media/i2c/mt9t001.c
index 7964634..d41c70e 100644
--- a/drivers/media/i2c/mt9t001.c
+++ b/drivers/media/i2c/mt9t001.c
@@ -291,10 +291,12 @@ static int mt9t001_set_format(struct v4l2_subdev *subdev,
/* Clamp the width and height to avoid dividing by zero. */
width = clamp_t(unsigned int, ALIGN(format->format.width, 2),
- max(__crop->width / 8, MT9T001_WINDOW_HEIGHT_MIN + 1),
+ max_t(unsigned int, __crop->width / 8,
+ MT9T001_WINDOW_HEIGHT_MIN + 1),
__crop->width);
height = clamp_t(unsigned int, ALIGN(format->format.height, 2),
- max(__crop->height / 8, MT9T001_WINDOW_HEIGHT_MIN + 1),
+ max_t(unsigned int, __crop->height / 8,
+ MT9T001_WINDOW_HEIGHT_MIN + 1),
__crop->height);
hratio = DIV_ROUND_CLOSEST(__crop->width, width);
@@ -339,15 +341,17 @@ static int mt9t001_set_crop(struct v4l2_subdev *subdev,
rect.top = clamp(ALIGN(crop->rect.top, 2),
MT9T001_ROW_START_MIN,
MT9T001_ROW_START_MAX);
- rect.width = clamp(ALIGN(crop->rect.width, 2),
- MT9T001_WINDOW_WIDTH_MIN + 1,
- MT9T001_WINDOW_WIDTH_MAX + 1);
- rect.height = clamp(ALIGN(crop->rect.height, 2),
- MT9T001_WINDOW_HEIGHT_MIN + 1,
- MT9T001_WINDOW_HEIGHT_MAX + 1);
-
- rect.width = min(rect.width, MT9T001_PIXEL_ARRAY_WIDTH - rect.left);
- rect.height = min(rect.height, MT9T001_PIXEL_ARRAY_HEIGHT - rect.top);
+ rect.width = clamp_t(unsigned int, ALIGN(crop->rect.width, 2),
+ MT9T001_WINDOW_WIDTH_MIN + 1,
+ MT9T001_WINDOW_WIDTH_MAX + 1);
+ rect.height = clamp_t(unsigned int, ALIGN(crop->rect.height, 2),
+ MT9T001_WINDOW_HEIGHT_MIN + 1,
+ MT9T001_WINDOW_HEIGHT_MAX + 1);
+
+ rect.width = min_t(unsigned int, rect.width,
+ MT9T001_PIXEL_ARRAY_WIDTH - rect.left);
+ rect.height = min_t(unsigned int, rect.height,
+ MT9T001_PIXEL_ARRAY_HEIGHT - rect.top);
__crop = __mt9t001_get_pad_crop(mt9t001, fh, crop->pad, crop->which);
diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
index 2c50eff..ba8da9b 100644
--- a/drivers/media/i2c/mt9v032.c
+++ b/drivers/media/i2c/mt9v032.c
@@ -420,12 +420,14 @@ static int mt9v032_set_format(struct v4l2_subdev *subdev,
format->which);
/* Clamp the width and height to avoid dividing by zero. */
- width = clamp_t(unsigned int, ALIGN(format->format.width, 2),
- max(__crop->width / 8, MT9V032_WINDOW_WIDTH_MIN),
- __crop->width);
- height = clamp_t(unsigned int, ALIGN(format->format.height, 2),
- max(__crop->height / 8, MT9V032_WINDOW_HEIGHT_MIN),
- __crop->height);
+ width = clamp(ALIGN(format->format.width, 2),
+ max_t(unsigned int, __crop->width / 8,
+ MT9V032_WINDOW_WIDTH_MIN),
+ __crop->width);
+ height = clamp(ALIGN(format->format.height, 2),
+ max_t(unsigned int, __crop->height / 8,
+ MT9V032_WINDOW_HEIGHT_MIN),
+ __crop->height);
hratio = DIV_ROUND_CLOSEST(__crop->width, width);
vratio = DIV_ROUND_CLOSEST(__crop->height, height);
@@ -471,15 +473,17 @@ static int mt9v032_set_crop(struct v4l2_subdev *subdev,
rect.top = clamp(ALIGN(crop->rect.top + 1, 2) - 1,
MT9V032_ROW_START_MIN,
MT9V032_ROW_START_MAX);
- rect.width = clamp(ALIGN(crop->rect.width, 2),
- MT9V032_WINDOW_WIDTH_MIN,
- MT9V032_WINDOW_WIDTH_MAX);
- rect.height = clamp(ALIGN(crop->rect.height, 2),
- MT9V032_WINDOW_HEIGHT_MIN,
- MT9V032_WINDOW_HEIGHT_MAX);
-
- rect.width = min(rect.width, MT9V032_PIXEL_ARRAY_WIDTH - rect.left);
- rect.height = min(rect.height, MT9V032_PIXEL_ARRAY_HEIGHT - rect.top);
+ rect.width = clamp_t(unsigned int, ALIGN(crop->rect.width, 2),
+ MT9V032_WINDOW_WIDTH_MIN,
+ MT9V032_WINDOW_WIDTH_MAX);
+ rect.height = clamp_t(unsigned int, ALIGN(crop->rect.height, 2),
+ MT9V032_WINDOW_HEIGHT_MIN,
+ MT9V032_WINDOW_HEIGHT_MAX);
+
+ rect.width = min_t(unsigned int,
+ rect.width, MT9V032_PIXEL_ARRAY_WIDTH - rect.left);
+ rect.height = min_t(unsigned int,
+ rect.height, MT9V032_PIXEL_ARRAY_HEIGHT - rect.top);
__crop = __mt9v032_get_pad_crop(mt9v032, fh, crop->pad, crop->which);
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index ae66d91..3fadb9e 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2028,8 +2028,8 @@ static int smiapp_set_crop(struct v4l2_subdev *subdev,
sel->r.width = min(sel->r.width, src_size->width);
sel->r.height = min(sel->r.height, src_size->height);
- sel->r.left = min(sel->r.left, src_size->width - sel->r.width);
- sel->r.top = min(sel->r.top, src_size->height - sel->r.height);
+ sel->r.left = min_t(int, sel->r.left, src_size->width - sel->r.width);
+ sel->r.top = min_t(int, sel->r.top, src_size->height - sel->r.height);
*crops[sel->pad] = sel->r;
@@ -2121,8 +2121,8 @@ static int smiapp_set_selection(struct v4l2_subdev *subdev,
sel->r.left = max(0, sel->r.left & ~1);
sel->r.top = max(0, sel->r.top & ~1);
- sel->r.width = max(0, SMIAPP_ALIGN_DIM(sel->r.width, sel->flags));
- sel->r.height = max(0, SMIAPP_ALIGN_DIM(sel->r.height, sel->flags));
+ sel->r.width = SMIAPP_ALIGN_DIM(sel->r.width, sel->flags);
+ sel->r.height = SMIAPP_ALIGN_DIM(sel->r.height, sel->flags);
sel->r.width = max_t(unsigned int,
sensor->limits[SMIAPP_LIMIT_MIN_X_OUTPUT_SIZE],
diff --git a/drivers/media/i2c/soc_camera/mt9m111.c b/drivers/media/i2c/soc_camera/mt9m111.c
index 6f40566..ccf5940 100644
--- a/drivers/media/i2c/soc_camera/mt9m111.c
+++ b/drivers/media/i2c/soc_camera/mt9m111.c
@@ -208,8 +208,8 @@ struct mt9m111 {
struct mt9m111_context *ctx;
struct v4l2_rect rect; /* cropping rectangle */
struct v4l2_clk *clk;
- int width; /* output */
- int height; /* sizes */
+ unsigned int width; /* output */
+ unsigned int height; /* sizes */
struct mutex power_lock; /* lock to protect power_count */
int power_count;
const struct mt9m111_datafmt *fmt;
diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 89c0b13..7b8962e 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -867,7 +867,7 @@ static int tvp5150_s_crop(struct v4l2_subdev *sd, const struct v4l2_crop *a)
struct v4l2_rect rect = a->c;
struct tvp5150 *decoder = to_tvp5150(sd);
v4l2_std_id std;
- int hmax;
+ unsigned int hmax;
v4l2_dbg(1, debug, sd, "%s left=%d, top=%d, width=%d, height=%d\n",
__func__, rect.left, rect.top, rect.width, rect.height);
@@ -877,9 +877,9 @@ static int tvp5150_s_crop(struct v4l2_subdev *sd, const struct v4l2_crop *a)
/* tvp5150 has some special limits */
rect.left = clamp(rect.left, 0, TVP5150_MAX_CROP_LEFT);
- rect.width = clamp(rect.width,
- TVP5150_H_MAX - TVP5150_MAX_CROP_LEFT - rect.left,
- TVP5150_H_MAX - rect.left);
+ rect.width = clamp_t(unsigned int, rect.width,
+ TVP5150_H_MAX - TVP5150_MAX_CROP_LEFT - rect.left,
+ TVP5150_H_MAX - rect.left);
rect.top = clamp(rect.top, 0, TVP5150_MAX_CROP_TOP);
/* Calculate height based on current standard */
@@ -893,9 +893,9 @@ static int tvp5150_s_crop(struct v4l2_subdev *sd, const struct v4l2_crop *a)
else
hmax = TVP5150_V_MAX_OTHERS;
- rect.height = clamp(rect.height,
- hmax - TVP5150_MAX_CROP_TOP - rect.top,
- hmax - rect.top);
+ rect.height = clamp_t(unsigned int, rect.height,
+ hmax - TVP5150_MAX_CROP_TOP - rect.top,
+ hmax - rect.top);
tvp5150_write(sd, TVP5150_VERT_BLANKING_START, rect.top);
tvp5150_write(sd, TVP5150_VERT_BLANKING_STOP,
diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
index c6532de..41ec4fa 100644
--- a/drivers/media/pci/bt8xx/bttv-driver.c
+++ b/drivers/media/pci/bt8xx/bttv-driver.c
@@ -1126,9 +1126,9 @@ bttv_crop_calc_limits(struct bttv_crop *c)
c->min_scaled_height = 32;
} else {
c->min_scaled_width =
- (max(48, c->rect.width >> 4) + 3) & ~3;
+ (max_t(unsigned int, 48, c->rect.width >> 4) + 3) & ~3;
c->min_scaled_height =
- max(32, c->rect.height >> 4);
+ max_t(unsigned int, 32, c->rect.height >> 4);
}
c->max_scaled_width = c->rect.width & ~3;
@@ -2024,7 +2024,7 @@ limit_scaled_size_lock (struct bttv_fh * fh,
/* We cannot scale up. When the scaled image is larger
than crop.rect we adjust the crop.rect as required
by the V4L2 spec, hence cropcap.bounds are our limit. */
- max_width = min(b->width, (__s32) MAX_HACTIVE);
+ max_width = min_t(unsigned int, b->width, MAX_HACTIVE);
max_height = b->height;
/* We cannot capture the same line as video and VBI data.
diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
index fb60da8..bdbd805 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -1979,10 +1979,6 @@ static int saa7134_s_crop(struct file *file, void *f, const struct v4l2_crop *cr
if (crop->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
crop->type != V4L2_BUF_TYPE_VIDEO_OVERLAY)
return -EINVAL;
- if (crop->c.height < 0)
- return -EINVAL;
- if (crop->c.width < 0)
- return -EINVAL;
if (res_locked(fh->dev, RESOURCE_OVERLAY))
return -EBUSY;
diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c b/drivers/media/platform/soc_camera/soc_scale_crop.c
index cbd3a34..8e74fb7 100644
--- a/drivers/media/platform/soc_camera/soc_scale_crop.c
+++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
@@ -141,8 +141,8 @@ int soc_camera_client_s_crop(struct v4l2_subdev *sd,
* Popular special case - some cameras can only handle fixed sizes like
* QVGA, VGA,... Take care to avoid infinite loop.
*/
- width = max(cam_rect->width, 2);
- height = max(cam_rect->height, 2);
+ width = max_t(unsigned int, cam_rect->width, 2);
+ height = max_t(unsigned int, cam_rect->height, 2);
/*
* Loop as long as sensor is not covering the requested rectangle and
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 437f1b0..6ae7bbe 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -207,8 +207,8 @@ enum v4l2_priority {
struct v4l2_rect {
__s32 left;
__s32 top;
- __s32 width;
- __s32 height;
+ __u32 width;
+ __u32 height;
};
struct v4l2_fract {
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v5] videodev2: Set vb2_rect's width and height as unsigned
2013-11-06 18:42 [PATCH v5] videodev2: Set vb2_rect's width and height as unsigned Ricardo Ribalda Delgado
@ 2013-11-07 17:25 ` Prabhakar Lad
2013-11-08 2:42 ` Laurent Pinchart
1 sibling, 0 replies; 7+ messages in thread
From: Prabhakar Lad @ 2013-11-07 17:25 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Sakari Ailus,
Guennadi Liakhovetski, Hans Verkuil, Frank Schäfer,
Ondrej Zary, open list:MT9M032 APTINA SE...
Hi Ricardo,
Thanks for the patch.
On Thu, Nov 7, 2013 at 12:12 AM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> As discussed on the media summit 2013, there is no reason for the width
> and height to be signed.
>
> Therefore this patch is an attempt to convert those fields from __s32 to
> __u32.
>
> Acked-by: Sakari Ailus <sakari.ailus@iki.fi> (documentation and smiapp)
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Acked-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
Regards,
--Prabhakar Lad
> ---
> v5: Comments by Sakari Ailus
> -Fix typos in summary
>
> v4: Wrong patch format
>
> v3: Comments by Sakari Ailus
> -Update also doc
>
> v2: Comments by Sakari Ailus and Laurent Pinchart
>
> -Fix alignment on all drivers
> -Replace min with min_t where possible and remove unneeded checks
>
> Documentation/DocBook/media/v4l/compat.xml | 12 ++++++++
> Documentation/DocBook/media/v4l/dev-overlay.xml | 8 ++---
> Documentation/DocBook/media/v4l/vidioc-cropcap.xml | 8 ++---
> drivers/media/i2c/mt9m032.c | 16 +++++-----
> drivers/media/i2c/mt9p031.c | 28 ++++++++++--------
> drivers/media/i2c/mt9t001.c | 26 ++++++++++-------
> drivers/media/i2c/mt9v032.c | 34 ++++++++++++----------
> drivers/media/i2c/smiapp/smiapp-core.c | 8 ++---
> drivers/media/i2c/soc_camera/mt9m111.c | 4 +--
> drivers/media/i2c/tvp5150.c | 14 ++++-----
> drivers/media/pci/bt8xx/bttv-driver.c | 6 ++--
> drivers/media/pci/saa7134/saa7134-video.c | 4 ---
> drivers/media/platform/soc_camera/soc_scale_crop.c | 4 +--
> include/uapi/linux/videodev2.h | 4 +--
> 14 files changed, 97 insertions(+), 79 deletions(-)
>
> diff --git a/Documentation/DocBook/media/v4l/compat.xml b/Documentation/DocBook/media/v4l/compat.xml
> index 0c7195e..5dbe68b 100644
> --- a/Documentation/DocBook/media/v4l/compat.xml
> +++ b/Documentation/DocBook/media/v4l/compat.xml
> @@ -2523,6 +2523,18 @@ that used it. It was originally scheduled for removal in 2.6.35.
> </orderedlist>
> </section>
>
> + <section>
> + <title>V4L2 in Linux 3.12</title>
> + <orderedlist>
> + <listitem>
> + <para> In struct <structname>v4l2_rect</structname>, the type
> +of <structfield>width</structfield> and <structfield>height</structfield>
> +fields changed from _s32 to _u32.
> + </para>
> + </listitem>
> + </orderedlist>
> + </section>
> +
> <section id="other">
> <title>Relation of V4L2 to other Linux multimedia APIs</title>
>
> diff --git a/Documentation/DocBook/media/v4l/dev-overlay.xml b/Documentation/DocBook/media/v4l/dev-overlay.xml
> index 40d1d76..a44ac66 100644
> --- a/Documentation/DocBook/media/v4l/dev-overlay.xml
> +++ b/Documentation/DocBook/media/v4l/dev-overlay.xml
> @@ -346,16 +346,14 @@ rectangle, in pixels.</entry>
> rectangle, in pixels. Offsets increase to the right and down.</entry>
> </row>
> <row>
> - <entry>__s32</entry>
> + <entry>__u32</entry>
> <entry><structfield>width</structfield></entry>
> <entry>Width of the rectangle, in pixels.</entry>
> </row>
> <row>
> - <entry>__s32</entry>
> + <entry>__u32</entry>
> <entry><structfield>height</structfield></entry>
> - <entry>Height of the rectangle, in pixels. Width and
> -height cannot be negative, the fields are signed for hysterical
> -reasons. <!-- video4linux-list@redhat.com on 22 Oct 2002 subject
> + <entry>Height of the rectangle, in pixels.<!-- video4linux-list@redhat.com on 22 Oct 2002 subject
> "Re:[V4L][patches!] Re:v4l2/kernel-2.5" --></entry>
> </row>
> </tbody>
> diff --git a/Documentation/DocBook/media/v4l/vidioc-cropcap.xml b/Documentation/DocBook/media/v4l/vidioc-cropcap.xml
> index bf7cc97..26b8f8f 100644
> --- a/Documentation/DocBook/media/v4l/vidioc-cropcap.xml
> +++ b/Documentation/DocBook/media/v4l/vidioc-cropcap.xml
> @@ -133,16 +133,14 @@ rectangle, in pixels.</entry>
> rectangle, in pixels.</entry>
> </row>
> <row>
> - <entry>__s32</entry>
> + <entry>__u32</entry>
> <entry><structfield>width</structfield></entry>
> <entry>Width of the rectangle, in pixels.</entry>
> </row>
> <row>
> - <entry>__s32</entry>
> + <entry>__u32</entry>
> <entry><structfield>height</structfield></entry>
> - <entry>Height of the rectangle, in pixels. Width
> -and height cannot be negative, the fields are signed for
> -hysterical reasons. <!-- video4linux-list@redhat.com
> + <entry>Height of the rectangle, in pixels.<!-- video4linux-list@redhat.com
> on 22 Oct 2002 subject "Re:[V4L][patches!] Re:v4l2/kernel-2.5" -->
> </entry>
> </row>
> diff --git a/drivers/media/i2c/mt9m032.c b/drivers/media/i2c/mt9m032.c
> index 846b15f..85ec3ba 100644
> --- a/drivers/media/i2c/mt9m032.c
> +++ b/drivers/media/i2c/mt9m032.c
> @@ -459,13 +459,15 @@ static int mt9m032_set_pad_crop(struct v4l2_subdev *subdev,
> MT9M032_COLUMN_START_MAX);
> rect.top = clamp(ALIGN(crop->rect.top, 2), MT9M032_ROW_START_MIN,
> MT9M032_ROW_START_MAX);
> - rect.width = clamp(ALIGN(crop->rect.width, 2), MT9M032_COLUMN_SIZE_MIN,
> - MT9M032_COLUMN_SIZE_MAX);
> - rect.height = clamp(ALIGN(crop->rect.height, 2), MT9M032_ROW_SIZE_MIN,
> - MT9M032_ROW_SIZE_MAX);
> -
> - rect.width = min(rect.width, MT9M032_PIXEL_ARRAY_WIDTH - rect.left);
> - rect.height = min(rect.height, MT9M032_PIXEL_ARRAY_HEIGHT - rect.top);
> + rect.width = clamp_t(unsigned int, ALIGN(crop->rect.width, 2),
> + MT9M032_COLUMN_SIZE_MIN, MT9M032_COLUMN_SIZE_MAX);
> + rect.height = clamp_t(unsigned int, ALIGN(crop->rect.height, 2),
> + MT9M032_ROW_SIZE_MIN, MT9M032_ROW_SIZE_MAX);
> +
> + rect.width = min_t(unsigned int, rect.width,
> + MT9M032_PIXEL_ARRAY_WIDTH - rect.left);
> + rect.height = min_t(unsigned int, rect.height,
> + MT9M032_PIXEL_ARRAY_HEIGHT - rect.top);
>
> __crop = __mt9m032_get_pad_crop(sensor, fh, crop->which);
>
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index 4734836..722f78a 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -518,11 +518,13 @@ static int mt9p031_set_format(struct v4l2_subdev *subdev,
>
> /* Clamp the width and height to avoid dividing by zero. */
> width = clamp_t(unsigned int, ALIGN(format->format.width, 2),
> - max(__crop->width / 7, MT9P031_WINDOW_WIDTH_MIN),
> + max_t(unsigned int, __crop->width / 7,
> + MT9P031_WINDOW_WIDTH_MIN),
> __crop->width);
> height = clamp_t(unsigned int, ALIGN(format->format.height, 2),
> - max(__crop->height / 8, MT9P031_WINDOW_HEIGHT_MIN),
> - __crop->height);
> + max_t(unsigned int, __crop->height / 8,
> + MT9P031_WINDOW_HEIGHT_MIN),
> + __crop->height);
>
> hratio = DIV_ROUND_CLOSEST(__crop->width, width);
> vratio = DIV_ROUND_CLOSEST(__crop->height, height);
> @@ -564,15 +566,17 @@ static int mt9p031_set_crop(struct v4l2_subdev *subdev,
> MT9P031_COLUMN_START_MAX);
> rect.top = clamp(ALIGN(crop->rect.top, 2), MT9P031_ROW_START_MIN,
> MT9P031_ROW_START_MAX);
> - rect.width = clamp(ALIGN(crop->rect.width, 2),
> - MT9P031_WINDOW_WIDTH_MIN,
> - MT9P031_WINDOW_WIDTH_MAX);
> - rect.height = clamp(ALIGN(crop->rect.height, 2),
> - MT9P031_WINDOW_HEIGHT_MIN,
> - MT9P031_WINDOW_HEIGHT_MAX);
> -
> - rect.width = min(rect.width, MT9P031_PIXEL_ARRAY_WIDTH - rect.left);
> - rect.height = min(rect.height, MT9P031_PIXEL_ARRAY_HEIGHT - rect.top);
> + rect.width = clamp_t(unsigned int, ALIGN(crop->rect.width, 2),
> + MT9P031_WINDOW_WIDTH_MIN,
> + MT9P031_WINDOW_WIDTH_MAX);
> + rect.height = clamp_t(unsigned int, ALIGN(crop->rect.height, 2),
> + MT9P031_WINDOW_HEIGHT_MIN,
> + MT9P031_WINDOW_HEIGHT_MAX);
> +
> + rect.width = min_t(unsigned int, rect.width,
> + MT9P031_PIXEL_ARRAY_WIDTH - rect.left);
> + rect.height = min_t(unsigned int, rect.height,
> + MT9P031_PIXEL_ARRAY_HEIGHT - rect.top);
>
> __crop = __mt9p031_get_pad_crop(mt9p031, fh, crop->pad, crop->which);
>
> diff --git a/drivers/media/i2c/mt9t001.c b/drivers/media/i2c/mt9t001.c
> index 7964634..d41c70e 100644
> --- a/drivers/media/i2c/mt9t001.c
> +++ b/drivers/media/i2c/mt9t001.c
> @@ -291,10 +291,12 @@ static int mt9t001_set_format(struct v4l2_subdev *subdev,
>
> /* Clamp the width and height to avoid dividing by zero. */
> width = clamp_t(unsigned int, ALIGN(format->format.width, 2),
> - max(__crop->width / 8, MT9T001_WINDOW_HEIGHT_MIN + 1),
> + max_t(unsigned int, __crop->width / 8,
> + MT9T001_WINDOW_HEIGHT_MIN + 1),
> __crop->width);
> height = clamp_t(unsigned int, ALIGN(format->format.height, 2),
> - max(__crop->height / 8, MT9T001_WINDOW_HEIGHT_MIN + 1),
> + max_t(unsigned int, __crop->height / 8,
> + MT9T001_WINDOW_HEIGHT_MIN + 1),
> __crop->height);
>
> hratio = DIV_ROUND_CLOSEST(__crop->width, width);
> @@ -339,15 +341,17 @@ static int mt9t001_set_crop(struct v4l2_subdev *subdev,
> rect.top = clamp(ALIGN(crop->rect.top, 2),
> MT9T001_ROW_START_MIN,
> MT9T001_ROW_START_MAX);
> - rect.width = clamp(ALIGN(crop->rect.width, 2),
> - MT9T001_WINDOW_WIDTH_MIN + 1,
> - MT9T001_WINDOW_WIDTH_MAX + 1);
> - rect.height = clamp(ALIGN(crop->rect.height, 2),
> - MT9T001_WINDOW_HEIGHT_MIN + 1,
> - MT9T001_WINDOW_HEIGHT_MAX + 1);
> -
> - rect.width = min(rect.width, MT9T001_PIXEL_ARRAY_WIDTH - rect.left);
> - rect.height = min(rect.height, MT9T001_PIXEL_ARRAY_HEIGHT - rect.top);
> + rect.width = clamp_t(unsigned int, ALIGN(crop->rect.width, 2),
> + MT9T001_WINDOW_WIDTH_MIN + 1,
> + MT9T001_WINDOW_WIDTH_MAX + 1);
> + rect.height = clamp_t(unsigned int, ALIGN(crop->rect.height, 2),
> + MT9T001_WINDOW_HEIGHT_MIN + 1,
> + MT9T001_WINDOW_HEIGHT_MAX + 1);
> +
> + rect.width = min_t(unsigned int, rect.width,
> + MT9T001_PIXEL_ARRAY_WIDTH - rect.left);
> + rect.height = min_t(unsigned int, rect.height,
> + MT9T001_PIXEL_ARRAY_HEIGHT - rect.top);
>
> __crop = __mt9t001_get_pad_crop(mt9t001, fh, crop->pad, crop->which);
>
> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> index 2c50eff..ba8da9b 100644
> --- a/drivers/media/i2c/mt9v032.c
> +++ b/drivers/media/i2c/mt9v032.c
> @@ -420,12 +420,14 @@ static int mt9v032_set_format(struct v4l2_subdev *subdev,
> format->which);
>
> /* Clamp the width and height to avoid dividing by zero. */
> - width = clamp_t(unsigned int, ALIGN(format->format.width, 2),
> - max(__crop->width / 8, MT9V032_WINDOW_WIDTH_MIN),
> - __crop->width);
> - height = clamp_t(unsigned int, ALIGN(format->format.height, 2),
> - max(__crop->height / 8, MT9V032_WINDOW_HEIGHT_MIN),
> - __crop->height);
> + width = clamp(ALIGN(format->format.width, 2),
> + max_t(unsigned int, __crop->width / 8,
> + MT9V032_WINDOW_WIDTH_MIN),
> + __crop->width);
> + height = clamp(ALIGN(format->format.height, 2),
> + max_t(unsigned int, __crop->height / 8,
> + MT9V032_WINDOW_HEIGHT_MIN),
> + __crop->height);
>
> hratio = DIV_ROUND_CLOSEST(__crop->width, width);
> vratio = DIV_ROUND_CLOSEST(__crop->height, height);
> @@ -471,15 +473,17 @@ static int mt9v032_set_crop(struct v4l2_subdev *subdev,
> rect.top = clamp(ALIGN(crop->rect.top + 1, 2) - 1,
> MT9V032_ROW_START_MIN,
> MT9V032_ROW_START_MAX);
> - rect.width = clamp(ALIGN(crop->rect.width, 2),
> - MT9V032_WINDOW_WIDTH_MIN,
> - MT9V032_WINDOW_WIDTH_MAX);
> - rect.height = clamp(ALIGN(crop->rect.height, 2),
> - MT9V032_WINDOW_HEIGHT_MIN,
> - MT9V032_WINDOW_HEIGHT_MAX);
> -
> - rect.width = min(rect.width, MT9V032_PIXEL_ARRAY_WIDTH - rect.left);
> - rect.height = min(rect.height, MT9V032_PIXEL_ARRAY_HEIGHT - rect.top);
> + rect.width = clamp_t(unsigned int, ALIGN(crop->rect.width, 2),
> + MT9V032_WINDOW_WIDTH_MIN,
> + MT9V032_WINDOW_WIDTH_MAX);
> + rect.height = clamp_t(unsigned int, ALIGN(crop->rect.height, 2),
> + MT9V032_WINDOW_HEIGHT_MIN,
> + MT9V032_WINDOW_HEIGHT_MAX);
> +
> + rect.width = min_t(unsigned int,
> + rect.width, MT9V032_PIXEL_ARRAY_WIDTH - rect.left);
> + rect.height = min_t(unsigned int,
> + rect.height, MT9V032_PIXEL_ARRAY_HEIGHT - rect.top);
>
> __crop = __mt9v032_get_pad_crop(mt9v032, fh, crop->pad, crop->which);
>
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
> index ae66d91..3fadb9e 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -2028,8 +2028,8 @@ static int smiapp_set_crop(struct v4l2_subdev *subdev,
> sel->r.width = min(sel->r.width, src_size->width);
> sel->r.height = min(sel->r.height, src_size->height);
>
> - sel->r.left = min(sel->r.left, src_size->width - sel->r.width);
> - sel->r.top = min(sel->r.top, src_size->height - sel->r.height);
> + sel->r.left = min_t(int, sel->r.left, src_size->width - sel->r.width);
> + sel->r.top = min_t(int, sel->r.top, src_size->height - sel->r.height);
>
> *crops[sel->pad] = sel->r;
>
> @@ -2121,8 +2121,8 @@ static int smiapp_set_selection(struct v4l2_subdev *subdev,
>
> sel->r.left = max(0, sel->r.left & ~1);
> sel->r.top = max(0, sel->r.top & ~1);
> - sel->r.width = max(0, SMIAPP_ALIGN_DIM(sel->r.width, sel->flags));
> - sel->r.height = max(0, SMIAPP_ALIGN_DIM(sel->r.height, sel->flags));
> + sel->r.width = SMIAPP_ALIGN_DIM(sel->r.width, sel->flags);
> + sel->r.height = SMIAPP_ALIGN_DIM(sel->r.height, sel->flags);
>
> sel->r.width = max_t(unsigned int,
> sensor->limits[SMIAPP_LIMIT_MIN_X_OUTPUT_SIZE],
> diff --git a/drivers/media/i2c/soc_camera/mt9m111.c b/drivers/media/i2c/soc_camera/mt9m111.c
> index 6f40566..ccf5940 100644
> --- a/drivers/media/i2c/soc_camera/mt9m111.c
> +++ b/drivers/media/i2c/soc_camera/mt9m111.c
> @@ -208,8 +208,8 @@ struct mt9m111 {
> struct mt9m111_context *ctx;
> struct v4l2_rect rect; /* cropping rectangle */
> struct v4l2_clk *clk;
> - int width; /* output */
> - int height; /* sizes */
> + unsigned int width; /* output */
> + unsigned int height; /* sizes */
> struct mutex power_lock; /* lock to protect power_count */
> int power_count;
> const struct mt9m111_datafmt *fmt;
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index 89c0b13..7b8962e 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -867,7 +867,7 @@ static int tvp5150_s_crop(struct v4l2_subdev *sd, const struct v4l2_crop *a)
> struct v4l2_rect rect = a->c;
> struct tvp5150 *decoder = to_tvp5150(sd);
> v4l2_std_id std;
> - int hmax;
> + unsigned int hmax;
>
> v4l2_dbg(1, debug, sd, "%s left=%d, top=%d, width=%d, height=%d\n",
> __func__, rect.left, rect.top, rect.width, rect.height);
> @@ -877,9 +877,9 @@ static int tvp5150_s_crop(struct v4l2_subdev *sd, const struct v4l2_crop *a)
>
> /* tvp5150 has some special limits */
> rect.left = clamp(rect.left, 0, TVP5150_MAX_CROP_LEFT);
> - rect.width = clamp(rect.width,
> - TVP5150_H_MAX - TVP5150_MAX_CROP_LEFT - rect.left,
> - TVP5150_H_MAX - rect.left);
> + rect.width = clamp_t(unsigned int, rect.width,
> + TVP5150_H_MAX - TVP5150_MAX_CROP_LEFT - rect.left,
> + TVP5150_H_MAX - rect.left);
> rect.top = clamp(rect.top, 0, TVP5150_MAX_CROP_TOP);
>
> /* Calculate height based on current standard */
> @@ -893,9 +893,9 @@ static int tvp5150_s_crop(struct v4l2_subdev *sd, const struct v4l2_crop *a)
> else
> hmax = TVP5150_V_MAX_OTHERS;
>
> - rect.height = clamp(rect.height,
> - hmax - TVP5150_MAX_CROP_TOP - rect.top,
> - hmax - rect.top);
> + rect.height = clamp_t(unsigned int, rect.height,
> + hmax - TVP5150_MAX_CROP_TOP - rect.top,
> + hmax - rect.top);
>
> tvp5150_write(sd, TVP5150_VERT_BLANKING_START, rect.top);
> tvp5150_write(sd, TVP5150_VERT_BLANKING_STOP,
> diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
> index c6532de..41ec4fa 100644
> --- a/drivers/media/pci/bt8xx/bttv-driver.c
> +++ b/drivers/media/pci/bt8xx/bttv-driver.c
> @@ -1126,9 +1126,9 @@ bttv_crop_calc_limits(struct bttv_crop *c)
> c->min_scaled_height = 32;
> } else {
> c->min_scaled_width =
> - (max(48, c->rect.width >> 4) + 3) & ~3;
> + (max_t(unsigned int, 48, c->rect.width >> 4) + 3) & ~3;
> c->min_scaled_height =
> - max(32, c->rect.height >> 4);
> + max_t(unsigned int, 32, c->rect.height >> 4);
> }
>
> c->max_scaled_width = c->rect.width & ~3;
> @@ -2024,7 +2024,7 @@ limit_scaled_size_lock (struct bttv_fh * fh,
> /* We cannot scale up. When the scaled image is larger
> than crop.rect we adjust the crop.rect as required
> by the V4L2 spec, hence cropcap.bounds are our limit. */
> - max_width = min(b->width, (__s32) MAX_HACTIVE);
> + max_width = min_t(unsigned int, b->width, MAX_HACTIVE);
> max_height = b->height;
>
> /* We cannot capture the same line as video and VBI data.
> diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
> index fb60da8..bdbd805 100644
> --- a/drivers/media/pci/saa7134/saa7134-video.c
> +++ b/drivers/media/pci/saa7134/saa7134-video.c
> @@ -1979,10 +1979,6 @@ static int saa7134_s_crop(struct file *file, void *f, const struct v4l2_crop *cr
> if (crop->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> crop->type != V4L2_BUF_TYPE_VIDEO_OVERLAY)
> return -EINVAL;
> - if (crop->c.height < 0)
> - return -EINVAL;
> - if (crop->c.width < 0)
> - return -EINVAL;
>
> if (res_locked(fh->dev, RESOURCE_OVERLAY))
> return -EBUSY;
> diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c b/drivers/media/platform/soc_camera/soc_scale_crop.c
> index cbd3a34..8e74fb7 100644
> --- a/drivers/media/platform/soc_camera/soc_scale_crop.c
> +++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
> @@ -141,8 +141,8 @@ int soc_camera_client_s_crop(struct v4l2_subdev *sd,
> * Popular special case - some cameras can only handle fixed sizes like
> * QVGA, VGA,... Take care to avoid infinite loop.
> */
> - width = max(cam_rect->width, 2);
> - height = max(cam_rect->height, 2);
> + width = max_t(unsigned int, cam_rect->width, 2);
> + height = max_t(unsigned int, cam_rect->height, 2);
>
> /*
> * Loop as long as sensor is not covering the requested rectangle and
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 437f1b0..6ae7bbe 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -207,8 +207,8 @@ enum v4l2_priority {
> struct v4l2_rect {
> __s32 left;
> __s32 top;
> - __s32 width;
> - __s32 height;
> + __u32 width;
> + __u32 height;
> };
>
> struct v4l2_fract {
> --
> 1.8.4.rc3
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5] videodev2: Set vb2_rect's width and height as unsigned
2013-11-06 18:42 [PATCH v5] videodev2: Set vb2_rect's width and height as unsigned Ricardo Ribalda Delgado
2013-11-07 17:25 ` Prabhakar Lad
@ 2013-11-08 2:42 ` Laurent Pinchart
2013-11-08 10:12 ` Ricardo Ribalda Delgado
1 sibling, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2013-11-08 2:42 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Mauro Carvalho Chehab, Sakari Ailus, Guennadi Liakhovetski,
Hans Verkuil, Lad, Prabhakar, Frank Schäfer, Ondrej Zary,
open list:MT9M032 APTINA SE...
Hi Ricardo,
Thank you for the patch.
On Wednesday 06 November 2013 19:42:16 Ricardo Ribalda Delgado wrote:
> As discussed on the media summit 2013, there is no reason for the width
> and height to be signed.
>
> Therefore this patch is an attempt to convert those fields from __s32 to
> __u32.
>
> Acked-by: Sakari Ailus <sakari.ailus@iki.fi> (documentation and smiapp)
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> v5: Comments by Sakari Ailus
> -Fix typos in summary
>
> v4: Wrong patch format
>
> v3: Comments by Sakari Ailus
> -Update also doc
>
> v2: Comments by Sakari Ailus and Laurent Pinchart
>
> -Fix alignment on all drivers
> -Replace min with min_t where possible and remove unneeded checks
>
> Documentation/DocBook/media/v4l/compat.xml | 12 ++++++++
> Documentation/DocBook/media/v4l/dev-overlay.xml | 8 ++---
> Documentation/DocBook/media/v4l/vidioc-cropcap.xml | 8 ++---
> drivers/media/i2c/mt9m032.c | 16 +++++-----
> drivers/media/i2c/mt9p031.c | 28 ++++++++++--------
> drivers/media/i2c/mt9t001.c | 26 ++++++++++-------
> drivers/media/i2c/mt9v032.c | 34 ++++++++++--------
> drivers/media/i2c/smiapp/smiapp-core.c | 8 ++---
> drivers/media/i2c/soc_camera/mt9m111.c | 4 +--
> drivers/media/i2c/tvp5150.c | 14 ++++-----
> drivers/media/pci/bt8xx/bttv-driver.c | 6 ++--
> drivers/media/pci/saa7134/saa7134-video.c | 4 ---
> drivers/media/platform/soc_camera/soc_scale_crop.c | 4 +--
> include/uapi/linux/videodev2.h | 4 +--
> 14 files changed, 97 insertions(+), 79 deletions(-)
>
> diff --git a/Documentation/DocBook/media/v4l/compat.xml
> b/Documentation/DocBook/media/v4l/compat.xml index 0c7195e..5dbe68b 100644
> --- a/Documentation/DocBook/media/v4l/compat.xml
> +++ b/Documentation/DocBook/media/v4l/compat.xml
> @@ -2523,6 +2523,18 @@ that used it. It was originally scheduled for removal
> in 2.6.35. </orderedlist>
> </section>
>
> + <section>
> + <title>V4L2 in Linux 3.12</title>
> + <orderedlist>
> + <listitem>
> + <para> In struct <structname>v4l2_rect</structname>, the type
> +of <structfield>width</structfield> and <structfield>height</structfield>
> +fields changed from _s32 to _u32.
> + </para>
> + </listitem>
> + </orderedlist>
> + </section>
> +
> <section id="other">
> <title>Relation of V4L2 to other Linux multimedia APIs</title>
>
> diff --git a/Documentation/DocBook/media/v4l/dev-overlay.xml
> b/Documentation/DocBook/media/v4l/dev-overlay.xml index 40d1d76..a44ac66
> 100644
> --- a/Documentation/DocBook/media/v4l/dev-overlay.xml
> +++ b/Documentation/DocBook/media/v4l/dev-overlay.xml
> @@ -346,16 +346,14 @@ rectangle, in pixels.</entry>
> rectangle, in pixels. Offsets increase to the right and down.</entry>
> </row>
> <row>
> - <entry>__s32</entry>
> + <entry>__u32</entry>
> <entry><structfield>width</structfield></entry>
> <entry>Width of the rectangle, in pixels.</entry>
> </row>
> <row>
> - <entry>__s32</entry>
> + <entry>__u32</entry>
> <entry><structfield>height</structfield></entry>
> - <entry>Height of the rectangle, in pixels. Width and
> -height cannot be negative, the fields are signed for hysterical
> -reasons. <!-- video4linux-list@redhat.com on 22 Oct 2002 subject
I like the concept of hysterical reasons :-)
> + <entry>Height of the rectangle, in pixels.<!--
> video4linux-list@redhat.com on 22 Oct 2002 subject "Re:[V4L][patches!]
> Re:v4l2/kernel-2.5" --></entry>
> </row>
> </tbody>
> diff --git a/Documentation/DocBook/media/v4l/vidioc-cropcap.xml
> b/Documentation/DocBook/media/v4l/vidioc-cropcap.xml index bf7cc97..26b8f8f
> 100644
> --- a/Documentation/DocBook/media/v4l/vidioc-cropcap.xml
> +++ b/Documentation/DocBook/media/v4l/vidioc-cropcap.xml
> @@ -133,16 +133,14 @@ rectangle, in pixels.</entry>
> rectangle, in pixels.</entry>
> </row>
> <row>
> - <entry>__s32</entry>
> + <entry>__u32</entry>
> <entry><structfield>width</structfield></entry>
> <entry>Width of the rectangle, in pixels.</entry>
> </row>
> <row>
> - <entry>__s32</entry>
> + <entry>__u32</entry>
> <entry><structfield>height</structfield></entry>
> - <entry>Height of the rectangle, in pixels. Width
> -and height cannot be negative, the fields are signed for
> -hysterical reasons. <!-- video4linux-list@redhat.com
> + <entry>Height of the rectangle, in pixels.<!--
> video4linux-list@redhat.com on 22 Oct 2002 subject "Re:[V4L][patches!]
> Re:v4l2/kernel-2.5" --> </entry>
> </row>
> diff --git a/drivers/media/i2c/mt9m032.c b/drivers/media/i2c/mt9m032.c
> index 846b15f..85ec3ba 100644
> --- a/drivers/media/i2c/mt9m032.c
> +++ b/drivers/media/i2c/mt9m032.c
> @@ -459,13 +459,15 @@ static int mt9m032_set_pad_crop(struct v4l2_subdev
> *subdev, MT9M032_COLUMN_START_MAX);
> rect.top = clamp(ALIGN(crop->rect.top, 2), MT9M032_ROW_START_MIN,
> MT9M032_ROW_START_MAX);
> - rect.width = clamp(ALIGN(crop->rect.width, 2), MT9M032_COLUMN_SIZE_MIN,
> - MT9M032_COLUMN_SIZE_MAX);
> - rect.height = clamp(ALIGN(crop->rect.height, 2), MT9M032_ROW_SIZE_MIN,
> - MT9M032_ROW_SIZE_MAX);
> -
> - rect.width = min(rect.width, MT9M032_PIXEL_ARRAY_WIDTH - rect.left);
> - rect.height = min(rect.height, MT9M032_PIXEL_ARRAY_HEIGHT - rect.top);
> + rect.width = clamp_t(unsigned int, ALIGN(crop->rect.width, 2),
> + MT9M032_COLUMN_SIZE_MIN, MT9M032_COLUMN_SIZE_MAX);
> + rect.height = clamp_t(unsigned int, ALIGN(crop->rect.height, 2),
> + MT9M032_ROW_SIZE_MIN, MT9M032_ROW_SIZE_MAX);
Would it make sense to define the size-related macros as unsigned integers
instead of using clamp_t ? For instance MT9M032_PIXEL_ARRAY_WIDTH could be
defined as 1600U instead of 1600. Same for the other Aptina sensor drivers.
This might introduce other issues, so I don't know whether that would be a
good solution.
> + rect.width = min_t(unsigned int, rect.width,
> + MT9M032_PIXEL_ARRAY_WIDTH - rect.left);
> + rect.height = min_t(unsigned int, rect.height,
> + MT9M032_PIXEL_ARRAY_HEIGHT - rect.top);
>
> __crop = __mt9m032_get_pad_crop(sensor, fh, crop->which);
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5] videodev2: Set vb2_rect's width and height as unsigned
2013-11-08 2:42 ` Laurent Pinchart
@ 2013-11-08 10:12 ` Ricardo Ribalda Delgado
2013-11-08 10:39 ` Sakari Ailus
0 siblings, 1 reply; 7+ messages in thread
From: Ricardo Ribalda Delgado @ 2013-11-08 10:12 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Sakari Ailus, Guennadi Liakhovetski,
Hans Verkuil, Lad, Prabhakar, Frank Schäfer, Ondrej Zary,
open list:MT9M032 APTINA SE...
Hello Laurent
Thanks for your comments
On Fri, Nov 8, 2013 at 3:42 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Wednesday 06 November 2013 19:42:16 Ricardo Ribalda Delgado wrote:
>> As discussed on the media summit 2013, there is no reason for the width
>> and height to be signed.
>>
>> Therefore this patch is an attempt to convert those fields from __s32 to
>> __u32.
>>
>> Acked-by: Sakari Ailus <sakari.ailus@iki.fi> (documentation and smiapp)
>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>> ---
>> v5: Comments by Sakari Ailus
>> -Fix typos in summary
>>
>> v4: Wrong patch format
>>
>> v3: Comments by Sakari Ailus
>> -Update also doc
>>
>> v2: Comments by Sakari Ailus and Laurent Pinchart
>>
>> -Fix alignment on all drivers
>> -Replace min with min_t where possible and remove unneeded checks
>>
>> Documentation/DocBook/media/v4l/compat.xml | 12 ++++++++
>> Documentation/DocBook/media/v4l/dev-overlay.xml | 8 ++---
>> Documentation/DocBook/media/v4l/vidioc-cropcap.xml | 8 ++---
>> drivers/media/i2c/mt9m032.c | 16 +++++-----
>> drivers/media/i2c/mt9p031.c | 28 ++++++++++--------
>> drivers/media/i2c/mt9t001.c | 26 ++++++++++-------
>> drivers/media/i2c/mt9v032.c | 34 ++++++++++--------
>> drivers/media/i2c/smiapp/smiapp-core.c | 8 ++---
>> drivers/media/i2c/soc_camera/mt9m111.c | 4 +--
>> drivers/media/i2c/tvp5150.c | 14 ++++-----
>> drivers/media/pci/bt8xx/bttv-driver.c | 6 ++--
>> drivers/media/pci/saa7134/saa7134-video.c | 4 ---
>> drivers/media/platform/soc_camera/soc_scale_crop.c | 4 +--
>> include/uapi/linux/videodev2.h | 4 +--
>> 14 files changed, 97 insertions(+), 79 deletions(-)
>>
>> diff --git a/Documentation/DocBook/media/v4l/compat.xml
>> b/Documentation/DocBook/media/v4l/compat.xml index 0c7195e..5dbe68b 100644
>> --- a/Documentation/DocBook/media/v4l/compat.xml
>> +++ b/Documentation/DocBook/media/v4l/compat.xml
>> @@ -2523,6 +2523,18 @@ that used it. It was originally scheduled for removal
>> in 2.6.35. </orderedlist>
>> </section>
>>
>> + <section>
>> + <title>V4L2 in Linux 3.12</title>
>> + <orderedlist>
>> + <listitem>
>> + <para> In struct <structname>v4l2_rect</structname>, the type
>> +of <structfield>width</structfield> and <structfield>height</structfield>
>> +fields changed from _s32 to _u32.
>> + </para>
>> + </listitem>
>> + </orderedlist>
>> + </section>
>> +
>> <section id="other">
>> <title>Relation of V4L2 to other Linux multimedia APIs</title>
>>
>> diff --git a/Documentation/DocBook/media/v4l/dev-overlay.xml
>> b/Documentation/DocBook/media/v4l/dev-overlay.xml index 40d1d76..a44ac66
>> 100644
>> --- a/Documentation/DocBook/media/v4l/dev-overlay.xml
>> +++ b/Documentation/DocBook/media/v4l/dev-overlay.xml
>> @@ -346,16 +346,14 @@ rectangle, in pixels.</entry>
>> rectangle, in pixels. Offsets increase to the right and down.</entry>
>> </row>
>> <row>
>> - <entry>__s32</entry>
>> + <entry>__u32</entry>
>> <entry><structfield>width</structfield></entry>
>> <entry>Width of the rectangle, in pixels.</entry>
>> </row>
>> <row>
>> - <entry>__s32</entry>
>> + <entry>__u32</entry>
>> <entry><structfield>height</structfield></entry>
>> - <entry>Height of the rectangle, in pixels. Width and
>> -height cannot be negative, the fields are signed for hysterical
>> -reasons. <!-- video4linux-list@redhat.com on 22 Oct 2002 subject
>
> I like the concept of hysterical reasons :-)
>
>> + <entry>Height of the rectangle, in pixels.<!--
>> video4linux-list@redhat.com on 22 Oct 2002 subject "Re:[V4L][patches!]
>> Re:v4l2/kernel-2.5" --></entry>
>> </row>
>> </tbody>
>> diff --git a/Documentation/DocBook/media/v4l/vidioc-cropcap.xml
>> b/Documentation/DocBook/media/v4l/vidioc-cropcap.xml index bf7cc97..26b8f8f
>> 100644
>> --- a/Documentation/DocBook/media/v4l/vidioc-cropcap.xml
>> +++ b/Documentation/DocBook/media/v4l/vidioc-cropcap.xml
>> @@ -133,16 +133,14 @@ rectangle, in pixels.</entry>
>> rectangle, in pixels.</entry>
>> </row>
>> <row>
>> - <entry>__s32</entry>
>> + <entry>__u32</entry>
>> <entry><structfield>width</structfield></entry>
>> <entry>Width of the rectangle, in pixels.</entry>
>> </row>
>> <row>
>> - <entry>__s32</entry>
>> + <entry>__u32</entry>
>> <entry><structfield>height</structfield></entry>
>> - <entry>Height of the rectangle, in pixels. Width
>> -and height cannot be negative, the fields are signed for
>> -hysterical reasons. <!-- video4linux-list@redhat.com
>> + <entry>Height of the rectangle, in pixels.<!--
>> video4linux-list@redhat.com on 22 Oct 2002 subject "Re:[V4L][patches!]
>> Re:v4l2/kernel-2.5" --> </entry>
>> </row>
>> diff --git a/drivers/media/i2c/mt9m032.c b/drivers/media/i2c/mt9m032.c
>> index 846b15f..85ec3ba 100644
>> --- a/drivers/media/i2c/mt9m032.c
>> +++ b/drivers/media/i2c/mt9m032.c
>> @@ -459,13 +459,15 @@ static int mt9m032_set_pad_crop(struct v4l2_subdev
>> *subdev, MT9M032_COLUMN_START_MAX);
>> rect.top = clamp(ALIGN(crop->rect.top, 2), MT9M032_ROW_START_MIN,
>> MT9M032_ROW_START_MAX);
>> - rect.width = clamp(ALIGN(crop->rect.width, 2), MT9M032_COLUMN_SIZE_MIN,
>> - MT9M032_COLUMN_SIZE_MAX);
>> - rect.height = clamp(ALIGN(crop->rect.height, 2), MT9M032_ROW_SIZE_MIN,
>> - MT9M032_ROW_SIZE_MAX);
>> -
>> - rect.width = min(rect.width, MT9M032_PIXEL_ARRAY_WIDTH - rect.left);
>> - rect.height = min(rect.height, MT9M032_PIXEL_ARRAY_HEIGHT - rect.top);
>> + rect.width = clamp_t(unsigned int, ALIGN(crop->rect.width, 2),
>> + MT9M032_COLUMN_SIZE_MIN, MT9M032_COLUMN_SIZE_MAX);
>> + rect.height = clamp_t(unsigned int, ALIGN(crop->rect.height, 2),
>> + MT9M032_ROW_SIZE_MIN, MT9M032_ROW_SIZE_MAX);
>
> Would it make sense to define the size-related macros as unsigned integers
> instead of using clamp_t ? For instance MT9M032_PIXEL_ARRAY_WIDTH could be
> defined as 1600U instead of 1600. Same for the other Aptina sensor drivers.
> This might introduce other issues, so I don't know whether that would be a
> good solution.
After a quick look, I see that the sized-related macros are combined
(a lot) with border macros, like top/left. I think setting those
macros to unsiged will trigger many other warnings/lines fixes, and
add castings in many places.
Also I am not aware of a reason why clamp_t is better than clamp (I am
probably wrong here....). If there is a good reason for not using
clamp_t I have no problem in reviewing again the patch and use
unsigned constants.
Thanks!!!
>
>> + rect.width = min_t(unsigned int, rect.width,
>> + MT9M032_PIXEL_ARRAY_WIDTH - rect.left);
>> + rect.height = min_t(unsigned int, rect.height,
>> + MT9M032_PIXEL_ARRAY_HEIGHT - rect.top);
>>
>> __crop = __mt9m032_get_pad_crop(sensor, fh, crop->which);
>>
>
> --
> Regards,
>
> Laurent Pinchart
>
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5] videodev2: Set vb2_rect's width and height as unsigned
2013-11-08 10:12 ` Ricardo Ribalda Delgado
@ 2013-11-08 10:39 ` Sakari Ailus
2013-11-08 13:41 ` Ricardo Ribalda Delgado
0 siblings, 1 reply; 7+ messages in thread
From: Sakari Ailus @ 2013-11-08 10:39 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Guennadi Liakhovetski,
Hans Verkuil, Lad, Prabhakar, Frank Schäfer, Ondrej Zary,
open list:MT9M032 APTINA SE...
On Fri, Nov 08, 2013 at 11:12:54AM +0100, Ricardo Ribalda Delgado wrote:
...
> Also I am not aware of a reason why clamp_t is better than clamp (I am
> probably wrong here....). If there is a good reason for not using
> clamp_t I have no problem in reviewing again the patch and use
> unsigned constants.
clamp_t() should only be used if you need to force a type for the clamping
operation. It's always better if you don't have to, and all the arguments
are of the same type: type casting can have an effect on the end result and
bugs related to that can be difficult to find.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5] videodev2: Set vb2_rect's width and height as unsigned
2013-11-08 10:39 ` Sakari Ailus
@ 2013-11-08 13:41 ` Ricardo Ribalda Delgado
2013-11-25 9:12 ` Ricardo Ribalda Delgado
0 siblings, 1 reply; 7+ messages in thread
From: Ricardo Ribalda Delgado @ 2013-11-08 13:41 UTC (permalink / raw)
To: Sakari Ailus
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Guennadi Liakhovetski,
Hans Verkuil, Lad, Prabhakar, Frank Schäfer, Ondrej Zary,
open list:MT9M032 APTINA SE...
Hello Sakari
On Fri, Nov 8, 2013 at 11:39 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> On Fri, Nov 08, 2013 at 11:12:54AM +0100, Ricardo Ribalda Delgado wrote:
> ...
>> Also I am not aware of a reason why clamp_t is better than clamp (I am
>> probably wrong here....). If there is a good reason for not using
>> clamp_t I have no problem in reviewing again the patch and use
>> unsigned constants.
>
> clamp_t() should only be used if you need to force a type for the clamping
> operation. It's always better if you don't have to, and all the arguments
> are of the same type: type casting can have an effect on the end result and
> bugs related to that can be difficult to find.
>
But IMHO in these case, we will cause much more castings in other
places. I find more descriptive a casting via clamp_t, than via ().
Regards!
> --
> Kind regards,
>
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5] videodev2: Set vb2_rect's width and height as unsigned
2013-11-08 13:41 ` Ricardo Ribalda Delgado
@ 2013-11-25 9:12 ` Ricardo Ribalda Delgado
0 siblings, 0 replies; 7+ messages in thread
From: Ricardo Ribalda Delgado @ 2013-11-25 9:12 UTC (permalink / raw)
To: Sakari Ailus
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Guennadi Liakhovetski,
Hans Verkuil, Lad, Prabhakar, Frank Schäfer, Ondrej Zary,
open list:MT9M032 APTINA SE...
Hello
Is there anything that needs to be addressed on this patch?
Thanks!
On Fri, Nov 8, 2013 at 2:41 PM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> Hello Sakari
>
> On Fri, Nov 8, 2013 at 11:39 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>> On Fri, Nov 08, 2013 at 11:12:54AM +0100, Ricardo Ribalda Delgado wrote:
>> ...
>>> Also I am not aware of a reason why clamp_t is better than clamp (I am
>>> probably wrong here....). If there is a good reason for not using
>>> clamp_t I have no problem in reviewing again the patch and use
>>> unsigned constants.
>>
>> clamp_t() should only be used if you need to force a type for the clamping
>> operation. It's always better if you don't have to, and all the arguments
>> are of the same type: type casting can have an effect on the end result and
>> bugs related to that can be difficult to find.
>>
>
> But IMHO in these case, we will cause much more castings in other
> places. I find more descriptive a casting via clamp_t, than via ().
>
> Regards!
>
>> --
>> Kind regards,
>>
>> Sakari Ailus
>> e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
>
>
>
> --
> Ricardo Ribalda
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-11-25 9:12 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-06 18:42 [PATCH v5] videodev2: Set vb2_rect's width and height as unsigned Ricardo Ribalda Delgado
2013-11-07 17:25 ` Prabhakar Lad
2013-11-08 2:42 ` Laurent Pinchart
2013-11-08 10:12 ` Ricardo Ribalda Delgado
2013-11-08 10:39 ` Sakari Ailus
2013-11-08 13:41 ` Ricardo Ribalda Delgado
2013-11-25 9:12 ` Ricardo Ribalda Delgado
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).