linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/6] rcar-vin: Add support for V4L2_FIELD_ALTERNATE
@ 2019-08-08  1:18 Niklas Söderlund
  2019-08-08  1:18 ` [PATCH v2 1/6] rcar-vin: Fix incorrect return statement in rvin_try_format() Niklas Söderlund
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Niklas Söderlund @ 2019-08-08  1:18 UTC (permalink / raw)
  To: Laurent Pinchart, Kieran Bingham, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

Hi,

This series adds support for V4L2_FIELD_ALTERNATE to rcar-vin. This
removes a limitation in the driver where a video sources delivering
video using the alternating field format was forced to use the rcar-vin
interlacer and V4L2_FIELD_INTERLACED was delivers to the user. After
this series the use of the interlacer is still default but the user have
the option to explicit ask for alternate.

It is based on latest media-tree and tested on R-Car Gen2 and Gen3
hardware.

Patch 1/6, 2/6 and 3/6 prepares for the work by fixing a broken return 
statement, use available macros and renaming a poorly variable. Patch 
4/6 fixes a problem with scaling (Gen2 only) which was found when 
testing alternating between V4L2_FIELD_ALTERNATE and 
V4L2_FIELD_INTERLACED. Patch 5/6 is the real change adding support for 
the new field format. Last 6/6 takes advantage of that the hardware 
interlacer is no longer a requirement and removes a bit of ugly code as 
a result.

Niklas Söderlund (6):
  rcar-vin: Fix incorrect return statement in rvin_try_format()
  rcar-vin: Make use of V4L2_FIELD_IS_INTERLACED() macro
  rcar-vin: Rename rectangle holding the video source information
  rcar-vin: Do not reset the crop and compose rectangles in s_fmt
  rcar-vin: Add support for V4L2_FIELD_ALTERNATE
  rcar-vin: Clean up how format is set on subdevice

 drivers/media/platform/rcar-vin/rcar-dma.c  |  54 ++++++-----
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 100 +++++++++-----------
 drivers/media/platform/rcar-vin/rcar-vin.h  |   4 +-
 3 files changed, 78 insertions(+), 80 deletions(-)

-- 
2.22.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 1/6] rcar-vin: Fix incorrect return statement in rvin_try_format()
  2019-08-08  1:18 [PATCH v2 0/6] rcar-vin: Add support for V4L2_FIELD_ALTERNATE Niklas Söderlund
@ 2019-08-08  1:18 ` Niklas Söderlund
  2019-08-08  8:25   ` Laurent Pinchart
  2019-08-08  1:18 ` [PATCH v2 2/6] rcar-vin: Make use of V4L2_FIELD_IS_INTERLACED() macro Niklas Söderlund
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Niklas Söderlund @ 2019-08-08  1:18 UTC (permalink / raw)
  To: Laurent Pinchart, Kieran Bingham, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

While refactoring code the return statement became corrupted, fix it by
returning the correct return code.

Reported-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Fixes: 897e371389e77514 ("media: rcar-vin: simplify how formats are set and reset"
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index cfed0a2604133849..402b40fcf7184fde 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -239,7 +239,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
 done:
 	v4l2_subdev_free_pad_config(pad_cfg);
 
-	return 0;
+	return ret;
 }
 
 static int rvin_querycap(struct file *file, void *priv,
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 2/6] rcar-vin: Make use of V4L2_FIELD_IS_INTERLACED() macro
  2019-08-08  1:18 [PATCH v2 0/6] rcar-vin: Add support for V4L2_FIELD_ALTERNATE Niklas Söderlund
  2019-08-08  1:18 ` [PATCH v2 1/6] rcar-vin: Fix incorrect return statement in rvin_try_format() Niklas Söderlund
@ 2019-08-08  1:18 ` Niklas Söderlund
  2019-08-08  8:27   ` Laurent Pinchart
  2019-08-08  1:18 ` [PATCH v2 3/6] rcar-vin: Rename rectangle holding the video source information Niklas Söderlund
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Niklas Söderlund @ 2019-08-08  1:18 UTC (permalink / raw)
  To: Laurent Pinchart, Kieran Bingham, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

The V4L2_FIELD_IS_INTERLACED() can be used to make the code more
readable, use it.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index f16f2966f9628b72..6be1f33d44e2170c 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -557,16 +557,11 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
 	rvin_write(vin, 0, VNSPPOC_REG);
 	rvin_write(vin, 0, VNSLPOC_REG);
 	rvin_write(vin, vin->format.width - 1, VNEPPOC_REG);
-	switch (vin->format.field) {
-	case V4L2_FIELD_INTERLACED:
-	case V4L2_FIELD_INTERLACED_TB:
-	case V4L2_FIELD_INTERLACED_BT:
+
+	if (V4L2_FIELD_IS_INTERLACED(vin->format.field))
 		rvin_write(vin, vin->format.height / 2 - 1, VNELPOC_REG);
-		break;
-	default:
+	else
 		rvin_write(vin, vin->format.height - 1, VNELPOC_REG);
-		break;
-	}
 
 	vin_dbg(vin,
 		"Pre-Clip: %ux%u@%u:%u YS: %d XS: %d Post-Clip: %ux%u@%u:%u\n",
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 3/6] rcar-vin: Rename rectangle holding the video source information
  2019-08-08  1:18 [PATCH v2 0/6] rcar-vin: Add support for V4L2_FIELD_ALTERNATE Niklas Söderlund
  2019-08-08  1:18 ` [PATCH v2 1/6] rcar-vin: Fix incorrect return statement in rvin_try_format() Niklas Söderlund
  2019-08-08  1:18 ` [PATCH v2 2/6] rcar-vin: Make use of V4L2_FIELD_IS_INTERLACED() macro Niklas Söderlund
@ 2019-08-08  1:18 ` Niklas Söderlund
  2019-08-08  8:30   ` Laurent Pinchart
  2019-08-08  1:18 ` [PATCH v2 4/6] rcar-vin: Do not reset the crop and compose rectangles in s_fmt Niklas Söderlund
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Niklas Söderlund @ 2019-08-08  1:18 UTC (permalink / raw)
  To: Laurent Pinchart, Kieran Bingham, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

The variable to hold the video source information dimensions was poorly
named 'source'. This is confusing as a lot of other members of structs
share the same name with different purposes, rename it src_rect in
preparation of refactoring code.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 33 +++++++++++----------
 drivers/media/platform/rcar-vin/rcar-vin.h  |  4 +--
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 402b40fcf7184fde..8b30267f1636aaf1 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -163,13 +163,13 @@ static int rvin_reset_format(struct rvin_dev *vin)
 
 	rvin_format_align(vin, &vin->format);
 
-	vin->source.top = 0;
-	vin->source.left = 0;
-	vin->source.width = vin->format.width;
-	vin->source.height = vin->format.height;
+	vin->src_rect.top = 0;
+	vin->src_rect.left = 0;
+	vin->src_rect.width = vin->format.width;
+	vin->src_rect.height = vin->format.height;
 
-	vin->crop = vin->source;
-	vin->compose = vin->source;
+	vin->crop = vin->src_rect;
+	vin->compose = vin->src_rect;
 
 	return 0;
 }
@@ -281,7 +281,7 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
 	vin->format = f->fmt.pix;
 	vin->crop = crop;
 	vin->compose = compose;
-	vin->source = crop;
+	vin->src_rect = crop;
 
 	return 0;
 }
@@ -319,8 +319,8 @@ static int rvin_g_selection(struct file *file, void *fh,
 	case V4L2_SEL_TGT_CROP_BOUNDS:
 	case V4L2_SEL_TGT_CROP_DEFAULT:
 		s->r.left = s->r.top = 0;
-		s->r.width = vin->source.width;
-		s->r.height = vin->source.height;
+		s->r.width = vin->src_rect.width;
+		s->r.height = vin->src_rect.height;
 		break;
 	case V4L2_SEL_TGT_CROP:
 		s->r = vin->crop;
@@ -362,21 +362,22 @@ static int rvin_s_selection(struct file *file, void *fh,
 	case V4L2_SEL_TGT_CROP:
 		/* Can't crop outside of source input */
 		max_rect.top = max_rect.left = 0;
-		max_rect.width = vin->source.width;
-		max_rect.height = vin->source.height;
+		max_rect.width = vin->src_rect.width;
+		max_rect.height = vin->src_rect.height;
 		v4l2_rect_map_inside(&r, &max_rect);
 
-		v4l_bound_align_image(&r.width, 6, vin->source.width, 0,
-				      &r.height, 2, vin->source.height, 0, 0);
+		v4l_bound_align_image(&r.width, 6, vin->src_rect.width, 0,
+				      &r.height, 2, vin->src_rect.height, 0, 0);
 
-		r.top  = clamp_t(s32, r.top, 0, vin->source.height - r.height);
-		r.left = clamp_t(s32, r.left, 0, vin->source.width - r.width);
+		r.top  = clamp_t(s32, r.top, 0,
+				 vin->src_rect.height - r.height);
+		r.left = clamp_t(s32, r.left, 0, vin->src_rect.width - r.width);
 
 		vin->crop = s->r = r;
 
 		vin_dbg(vin, "Cropped %dx%d@%d:%d of %dx%d\n",
 			r.width, r.height, r.left, r.top,
-			vin->source.width, vin->source.height);
+			vin->src_rect.width, vin->src_rect.height);
 		break;
 	case V4L2_SEL_TGT_COMPOSE:
 		/* Make sure compose rect fits inside output format */
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index e562c2ff21ec7e7b..86e9bad44484092c 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -176,7 +176,7 @@ struct rvin_info {
  *
  * @crop:		active cropping
  * @compose:		active composing
- * @source:		active size of the video source
+ * @src_rect:		active size of the video source
  * @std:		active video standard of the video source
  *
  * @alpha:		Alpha component to fill in for supported pixel formats
@@ -215,7 +215,7 @@ struct rvin_dev {
 
 	struct v4l2_rect crop;
 	struct v4l2_rect compose;
-	struct v4l2_rect source;
+	struct v4l2_rect src_rect;
 	v4l2_std_id std;
 
 	unsigned int alpha;
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 4/6] rcar-vin: Do not reset the crop and compose rectangles in s_fmt
  2019-08-08  1:18 [PATCH v2 0/6] rcar-vin: Add support for V4L2_FIELD_ALTERNATE Niklas Söderlund
                   ` (2 preceding siblings ...)
  2019-08-08  1:18 ` [PATCH v2 3/6] rcar-vin: Rename rectangle holding the video source information Niklas Söderlund
@ 2019-08-08  1:18 ` Niklas Söderlund
  2019-08-08  8:37   ` Laurent Pinchart
  2019-08-08  1:18 ` [PATCH v2 5/6] rcar-vin: Add support for V4L2_FIELD_ALTERNATE Niklas Söderlund
  2019-08-08  1:18 ` [PATCH v2 6/6] rcar-vin: Clean up how format is set on subdevice Niklas Söderlund
  5 siblings, 1 reply; 14+ messages in thread
From: Niklas Söderlund @ 2019-08-08  1:18 UTC (permalink / raw)
  To: Laurent Pinchart, Kieran Bingham, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

The crop and compose rectangles are reset when s_fmt is called
resulting in potentially valid rectangles being lost when updating the
format. Fix this by mapping the rectangles inside the new format.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 8b30267f1636aaf1..5dcd787a9cf96ac9 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -279,8 +279,8 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
 		return ret;
 
 	vin->format = f->fmt.pix;
-	vin->crop = crop;
-	vin->compose = compose;
+	v4l2_rect_map_inside(&vin->crop, &crop);
+	v4l2_rect_map_inside(&vin->compose, &compose);
 	vin->src_rect = crop;
 
 	return 0;
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 5/6] rcar-vin: Add support for V4L2_FIELD_ALTERNATE
  2019-08-08  1:18 [PATCH v2 0/6] rcar-vin: Add support for V4L2_FIELD_ALTERNATE Niklas Söderlund
                   ` (3 preceding siblings ...)
  2019-08-08  1:18 ` [PATCH v2 4/6] rcar-vin: Do not reset the crop and compose rectangles in s_fmt Niklas Söderlund
@ 2019-08-08  1:18 ` Niklas Söderlund
  2019-08-08  1:18 ` [PATCH v2 6/6] rcar-vin: Clean up how format is set on subdevice Niklas Söderlund
  5 siblings, 0 replies; 14+ messages in thread
From: Niklas Söderlund @ 2019-08-08  1:18 UTC (permalink / raw)
  To: Laurent Pinchart, Kieran Bingham, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

The hardware is capable to passing V4L2_FIELD_ALTERNATE to user-space.
Allow users to request this field format but still default to using the
hardware interlacer if alternating is not explicitly requested.

Before this change a sensor providing data using alternate would be
always combined to an interlaced frame. After this change the user can
request to receive frames as alternate if the sensor provides it.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-dma.c  | 43 +++++++++++++--------
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 31 +++++++--------
 2 files changed, 39 insertions(+), 35 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 6be1f33d44e2170c..56db239017d4a44b 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -529,12 +529,17 @@ static void rvin_set_coeff(struct rvin_dev *vin, unsigned short xs)
 
 static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
 {
+	unsigned int crop_height;
 	u32 xs, ys;
 
 	/* Set scaling coefficient */
+	crop_height = vin->crop.height;
+	if (V4L2_FIELD_IS_INTERLACED(vin->format.field))
+		crop_height *= 2;
+
 	ys = 0;
-	if (vin->crop.height != vin->compose.height)
-		ys = (4096 * vin->crop.height) / vin->compose.height;
+	if (crop_height != vin->compose.height)
+		ys = (4096 * crop_height) / vin->compose.height;
 	rvin_write(vin, ys, VNYS_REG);
 
 	xs = 0;
@@ -575,21 +580,9 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
 	/* Set Start/End Pixel/Line Pre-Clip */
 	rvin_write(vin, vin->crop.left, VNSPPRC_REG);
 	rvin_write(vin, vin->crop.left + vin->crop.width - 1, VNEPPRC_REG);
+	rvin_write(vin, vin->crop.top, VNSLPRC_REG);
+	rvin_write(vin, vin->crop.top + vin->crop.height - 1, VNELPRC_REG);
 
-	switch (vin->format.field) {
-	case V4L2_FIELD_INTERLACED:
-	case V4L2_FIELD_INTERLACED_TB:
-	case V4L2_FIELD_INTERLACED_BT:
-		rvin_write(vin, vin->crop.top / 2, VNSLPRC_REG);
-		rvin_write(vin, (vin->crop.top + vin->crop.height) / 2 - 1,
-			   VNELPRC_REG);
-		break;
-	default:
-		rvin_write(vin, vin->crop.top, VNSLPRC_REG);
-		rvin_write(vin, vin->crop.top + vin->crop.height - 1,
-			   VNELPRC_REG);
-		break;
-	}
 
 	/* TODO: Add support for the UDS scaler. */
 	if (vin->info->model != RCAR_GEN3)
@@ -634,6 +627,9 @@ static int rvin_setup(struct rvin_dev *vin)
 		vnmc = VNMC_IM_ODD_EVEN;
 		progressive = true;
 		break;
+	case V4L2_FIELD_ALTERNATE:
+		vnmc = VNMC_IM_ODD_EVEN;
+		break;
 	default:
 		vnmc = VNMC_IM_ODD;
 		break;
@@ -792,6 +788,18 @@ static bool rvin_capture_active(struct rvin_dev *vin)
 	return rvin_read(vin, VNMS_REG) & VNMS_CA;
 }
 
+static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
+{
+	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
+		/* If FS is set it is an Even field. */
+		if (vnms & VNMS_FS)
+			return V4L2_FIELD_BOTTOM;
+		return V4L2_FIELD_TOP;
+	}
+
+	return vin->format.field;
+}
+
 static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
 {
 	const struct rvin_video_format *fmt;
@@ -941,7 +949,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
 
 	/* Capture frame */
 	if (vin->queue_buf[slot]) {
-		vin->queue_buf[slot]->field = vin->format.field;
+		vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
 		vin->queue_buf[slot]->sequence = vin->sequence;
 		vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
 		vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf,
@@ -1068,6 +1076,7 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd,
 		case V4L2_FIELD_TOP:
 		case V4L2_FIELD_BOTTOM:
 		case V4L2_FIELD_NONE:
+		case V4L2_FIELD_ALTERNATE:
 			break;
 		case V4L2_FIELD_INTERLACED_TB:
 		case V4L2_FIELD_INTERLACED_BT:
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 5dcd787a9cf96ac9..d9ee899144eba547 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -114,15 +114,7 @@ static void rvin_format_align(struct rvin_dev *vin, struct v4l2_pix_format *pix)
 	case V4L2_FIELD_INTERLACED_TB:
 	case V4L2_FIELD_INTERLACED_BT:
 	case V4L2_FIELD_INTERLACED:
-		break;
 	case V4L2_FIELD_ALTERNATE:
-		/*
-		 * Driver does not (yet) support outputting ALTERNATE to a
-		 * userspace. It does support outputting INTERLACED so use
-		 * the VIN hardware to combine the two fields.
-		 */
-		pix->field = V4L2_FIELD_INTERLACED;
-		pix->height *= 2;
 		break;
 	default:
 		pix->field = RVIN_DEFAULT_FIELD;
@@ -161,15 +153,25 @@ static int rvin_reset_format(struct rvin_dev *vin)
 
 	v4l2_fill_pix_format(&vin->format, &fmt.format);
 
-	rvin_format_align(vin, &vin->format);
-
 	vin->src_rect.top = 0;
 	vin->src_rect.left = 0;
 	vin->src_rect.width = vin->format.width;
 	vin->src_rect.height = vin->format.height;
 
+	/*  Make use of the hardware interlacer by default. */
+	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
+		vin->format.field = V4L2_FIELD_INTERLACED;
+		vin->format.height *= 2;
+	}
+
+	rvin_format_align(vin, &vin->format);
+
 	vin->crop = vin->src_rect;
-	vin->compose = vin->src_rect;
+
+	vin->compose.top = 0;
+	vin->compose.left = 0;
+	vin->compose.width = vin->format.width;
+	vin->compose.height = vin->format.height;
 
 	return 0;
 }
@@ -213,13 +215,6 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
 		crop->left = 0;
 		crop->width = pix->width;
 		crop->height = pix->height;
-
-		/*
-		 * If source is ALTERNATE the driver will use the VIN hardware
-		 * to INTERLACE it. The crop height then needs to be doubled.
-		 */
-		if (pix->field == V4L2_FIELD_ALTERNATE)
-			crop->height *= 2;
 	}
 
 	if (field != V4L2_FIELD_ANY)
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 6/6] rcar-vin: Clean up how format is set on subdevice
  2019-08-08  1:18 [PATCH v2 0/6] rcar-vin: Add support for V4L2_FIELD_ALTERNATE Niklas Söderlund
                   ` (4 preceding siblings ...)
  2019-08-08  1:18 ` [PATCH v2 5/6] rcar-vin: Add support for V4L2_FIELD_ALTERNATE Niklas Söderlund
@ 2019-08-08  1:18 ` Niklas Söderlund
  5 siblings, 0 replies; 14+ messages in thread
From: Niklas Söderlund @ 2019-08-08  1:18 UTC (permalink / raw)
  To: Laurent Pinchart, Kieran Bingham, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

With support for V4L2_FIELD_ALTERNATE added it's possible to clean up
how formats are set on the subdevice. This makes the code easier to read
as variable names now more clearly express their intent.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 ++++++++++-----------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index d9ee899144eba547..08a5ad7b4af368c2 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -178,7 +178,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
 
 static int rvin_try_format(struct rvin_dev *vin, u32 which,
 			   struct v4l2_pix_format *pix,
-			   struct v4l2_rect *crop, struct v4l2_rect *compose)
+			   struct v4l2_rect *src_rect)
 {
 	struct v4l2_subdev *sd = vin_to_source(vin);
 	struct v4l2_subdev_pad_config *pad_cfg;
@@ -197,24 +197,24 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
 	if (!rvin_format_from_pixel(vin, pix->pixelformat))
 		pix->pixelformat = RVIN_DEFAULT_FORMAT;
 
-	v4l2_fill_mbus_format(&format.format, pix, vin->mbus_code);
-
 	/* Allow the video device to override field and to scale */
 	field = pix->field;
 	width = pix->width;
 	height = pix->height;
 
+	v4l2_fill_mbus_format(&format.format, pix, vin->mbus_code);
+
 	ret = v4l2_subdev_call(sd, pad, set_fmt, pad_cfg, &format);
 	if (ret < 0 && ret != -ENOIOCTLCMD)
 		goto done;
 
 	v4l2_fill_pix_format(pix, &format.format);
 
-	if (crop) {
-		crop->top = 0;
-		crop->left = 0;
-		crop->width = pix->width;
-		crop->height = pix->height;
+	if (src_rect) {
+		src_rect->top = 0;
+		src_rect->left = 0;
+		src_rect->width = pix->width;
+		src_rect->height = pix->height;
 	}
 
 	if (field != V4L2_FIELD_ANY)
@@ -224,13 +224,6 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
 	pix->height = height;
 
 	rvin_format_align(vin, pix);
-
-	if (compose) {
-		compose->top = 0;
-		compose->left = 0;
-		compose->width = pix->width;
-		compose->height = pix->height;
-	}
 done:
 	v4l2_subdev_free_pad_config(pad_cfg);
 
@@ -254,29 +247,34 @@ static int rvin_try_fmt_vid_cap(struct file *file, void *priv,
 {
 	struct rvin_dev *vin = video_drvdata(file);
 
-	return rvin_try_format(vin, V4L2_SUBDEV_FORMAT_TRY, &f->fmt.pix, NULL,
-			       NULL);
+	return rvin_try_format(vin, V4L2_SUBDEV_FORMAT_TRY, &f->fmt.pix, NULL);
 }
 
 static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
 			      struct v4l2_format *f)
 {
 	struct rvin_dev *vin = video_drvdata(file);
-	struct v4l2_rect crop, compose;
+	struct v4l2_rect fmt_rect, src_rect;
 	int ret;
 
 	if (vb2_is_busy(&vin->queue))
 		return -EBUSY;
 
 	ret = rvin_try_format(vin, V4L2_SUBDEV_FORMAT_ACTIVE, &f->fmt.pix,
-			      &crop, &compose);
+			      &src_rect);
 	if (ret)
 		return ret;
 
 	vin->format = f->fmt.pix;
-	v4l2_rect_map_inside(&vin->crop, &crop);
-	v4l2_rect_map_inside(&vin->compose, &compose);
-	vin->src_rect = crop;
+
+	fmt_rect.top = 0;
+	fmt_rect.left = 0;
+	fmt_rect.width = vin->format.width;
+	fmt_rect.height = vin->format.height;
+
+	v4l2_rect_map_inside(&vin->crop, &src_rect);
+	v4l2_rect_map_inside(&vin->compose, &fmt_rect);
+	vin->src_rect = src_rect;
 
 	return 0;
 }
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/6] rcar-vin: Fix incorrect return statement in rvin_try_format()
  2019-08-08  1:18 ` [PATCH v2 1/6] rcar-vin: Fix incorrect return statement in rvin_try_format() Niklas Söderlund
@ 2019-08-08  8:25   ` Laurent Pinchart
  0 siblings, 0 replies; 14+ messages in thread
From: Laurent Pinchart @ 2019-08-08  8:25 UTC (permalink / raw)
  To: Niklas Söderlund; +Cc: Kieran Bingham, linux-media, linux-renesas-soc

Hi Niklas,

Thank you for the patch.

On Thu, Aug 08, 2019 at 03:18:45AM +0200, Niklas Söderlund wrote:
> While refactoring code the return statement became corrupted, fix it by
> returning the correct return code.
> 
> Reported-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Fixes: 897e371389e77514 ("media: rcar-vin: simplify how formats are set and reset"
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index cfed0a2604133849..402b40fcf7184fde 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -239,7 +239,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
>  done:
>  	v4l2_subdev_free_pad_config(pad_cfg);
>  
> -	return 0;
> +	return ret;

If the v4l2_subdev_call() call above returns -ENOIOCTLCMD, which you
don't consider as an error, you will end up returning that error value
here. You should set ret to 0 before the done: label. With this fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  }
>  
>  static int rvin_querycap(struct file *file, void *priv,

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 2/6] rcar-vin: Make use of V4L2_FIELD_IS_INTERLACED() macro
  2019-08-08  1:18 ` [PATCH v2 2/6] rcar-vin: Make use of V4L2_FIELD_IS_INTERLACED() macro Niklas Söderlund
@ 2019-08-08  8:27   ` Laurent Pinchart
  0 siblings, 0 replies; 14+ messages in thread
From: Laurent Pinchart @ 2019-08-08  8:27 UTC (permalink / raw)
  To: Niklas Söderlund; +Cc: Kieran Bingham, linux-media, linux-renesas-soc

Hi Niklas,

Thank you for the patch.

On Thu, Aug 08, 2019 at 03:18:46AM +0200, Niklas Söderlund wrote:
> The V4L2_FIELD_IS_INTERLACED() can be used to make the code more
> readable, use it.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index f16f2966f9628b72..6be1f33d44e2170c 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -557,16 +557,11 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
>  	rvin_write(vin, 0, VNSPPOC_REG);
>  	rvin_write(vin, 0, VNSLPOC_REG);
>  	rvin_write(vin, vin->format.width - 1, VNEPPOC_REG);
> -	switch (vin->format.field) {
> -	case V4L2_FIELD_INTERLACED:
> -	case V4L2_FIELD_INTERLACED_TB:
> -	case V4L2_FIELD_INTERLACED_BT:
> +
> +	if (V4L2_FIELD_IS_INTERLACED(vin->format.field))
>  		rvin_write(vin, vin->format.height / 2 - 1, VNELPOC_REG);
> -		break;
> -	default:
> +	else
>  		rvin_write(vin, vin->format.height - 1, VNELPOC_REG);
> -		break;
> -	}
>  
>  	vin_dbg(vin,
>  		"Pre-Clip: %ux%u@%u:%u YS: %d XS: %d Post-Clip: %ux%u@%u:%u\n",

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 3/6] rcar-vin: Rename rectangle holding the video source information
  2019-08-08  1:18 ` [PATCH v2 3/6] rcar-vin: Rename rectangle holding the video source information Niklas Söderlund
@ 2019-08-08  8:30   ` Laurent Pinchart
  2019-09-04 12:52     ` Niklas Söderlund
  0 siblings, 1 reply; 14+ messages in thread
From: Laurent Pinchart @ 2019-08-08  8:30 UTC (permalink / raw)
  To: Niklas Söderlund; +Cc: Kieran Bingham, linux-media, linux-renesas-soc

Hi Niklas,

Thank you for the patch.

On Thu, Aug 08, 2019 at 03:18:47AM +0200, Niklas Söderlund wrote:
> The variable to hold the video source information dimensions was poorly
> named 'source'. This is confusing as a lot of other members of structs
> share the same name with different purposes, rename it src_rect in
> preparation of refactoring code.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 33 +++++++++++----------
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  4 +--
>  2 files changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 402b40fcf7184fde..8b30267f1636aaf1 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -163,13 +163,13 @@ static int rvin_reset_format(struct rvin_dev *vin)
>  
>  	rvin_format_align(vin, &vin->format);
>  
> -	vin->source.top = 0;
> -	vin->source.left = 0;
> -	vin->source.width = vin->format.width;
> -	vin->source.height = vin->format.height;
> +	vin->src_rect.top = 0;
> +	vin->src_rect.left = 0;
> +	vin->src_rect.width = vin->format.width;
> +	vin->src_rect.height = vin->format.height;
>  
> -	vin->crop = vin->source;
> -	vin->compose = vin->source;
> +	vin->crop = vin->src_rect;
> +	vin->compose = vin->src_rect;
>  
>  	return 0;
>  }
> @@ -281,7 +281,7 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
>  	vin->format = f->fmt.pix;
>  	vin->crop = crop;
>  	vin->compose = compose;
> -	vin->source = crop;
> +	vin->src_rect = crop;
>  
>  	return 0;
>  }
> @@ -319,8 +319,8 @@ static int rvin_g_selection(struct file *file, void *fh,
>  	case V4L2_SEL_TGT_CROP_BOUNDS:
>  	case V4L2_SEL_TGT_CROP_DEFAULT:
>  		s->r.left = s->r.top = 0;
> -		s->r.width = vin->source.width;
> -		s->r.height = vin->source.height;
> +		s->r.width = vin->src_rect.width;
> +		s->r.height = vin->src_rect.height;
>  		break;
>  	case V4L2_SEL_TGT_CROP:
>  		s->r = vin->crop;
> @@ -362,21 +362,22 @@ static int rvin_s_selection(struct file *file, void *fh,
>  	case V4L2_SEL_TGT_CROP:
>  		/* Can't crop outside of source input */
>  		max_rect.top = max_rect.left = 0;
> -		max_rect.width = vin->source.width;
> -		max_rect.height = vin->source.height;
> +		max_rect.width = vin->src_rect.width;
> +		max_rect.height = vin->src_rect.height;
>  		v4l2_rect_map_inside(&r, &max_rect);
>  
> -		v4l_bound_align_image(&r.width, 6, vin->source.width, 0,
> -				      &r.height, 2, vin->source.height, 0, 0);
> +		v4l_bound_align_image(&r.width, 6, vin->src_rect.width, 0,
> +				      &r.height, 2, vin->src_rect.height, 0, 0);
>  
> -		r.top  = clamp_t(s32, r.top, 0, vin->source.height - r.height);
> -		r.left = clamp_t(s32, r.left, 0, vin->source.width - r.width);
> +		r.top  = clamp_t(s32, r.top, 0,
> +				 vin->src_rect.height - r.height);
> +		r.left = clamp_t(s32, r.left, 0, vin->src_rect.width - r.width);
>  
>  		vin->crop = s->r = r;
>  
>  		vin_dbg(vin, "Cropped %dx%d@%d:%d of %dx%d\n",
>  			r.width, r.height, r.left, r.top,
> -			vin->source.width, vin->source.height);
> +			vin->src_rect.width, vin->src_rect.height);
>  		break;
>  	case V4L2_SEL_TGT_COMPOSE:
>  		/* Make sure compose rect fits inside output format */
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index e562c2ff21ec7e7b..86e9bad44484092c 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -176,7 +176,7 @@ struct rvin_info {
>   *
>   * @crop:		active cropping
>   * @compose:		active composing
> - * @source:		active size of the video source
> + * @src_rect:		active size of the video source

As this only holds a size you don't need a full rectangle, src_width and
src_height would save a bit of space. Up to you, in any case

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>   * @std:		active video standard of the video source
>   *
>   * @alpha:		Alpha component to fill in for supported pixel formats
> @@ -215,7 +215,7 @@ struct rvin_dev {
>  
>  	struct v4l2_rect crop;
>  	struct v4l2_rect compose;
> -	struct v4l2_rect source;
> +	struct v4l2_rect src_rect;
>  	v4l2_std_id std;
>  
>  	unsigned int alpha;

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 4/6] rcar-vin: Do not reset the crop and compose rectangles in s_fmt
  2019-08-08  1:18 ` [PATCH v2 4/6] rcar-vin: Do not reset the crop and compose rectangles in s_fmt Niklas Söderlund
@ 2019-08-08  8:37   ` Laurent Pinchart
  2019-08-08 13:35     ` Niklas Söderlund
  0 siblings, 1 reply; 14+ messages in thread
From: Laurent Pinchart @ 2019-08-08  8:37 UTC (permalink / raw)
  To: Niklas Söderlund; +Cc: Kieran Bingham, linux-media, linux-renesas-soc

Hi Niklas,

Thank you for the patch.

On Thu, Aug 08, 2019 at 03:18:48AM +0200, Niklas Söderlund wrote:
> The crop and compose rectangles are reset when s_fmt is called
> resulting in potentially valid rectangles being lost when updating the
> format.

Isn't this the expected behaviour ?

> Fix this by mapping the rectangles inside the new format.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 8b30267f1636aaf1..5dcd787a9cf96ac9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -279,8 +279,8 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
>  		return ret;
>  
>  	vin->format = f->fmt.pix;
> -	vin->crop = crop;
> -	vin->compose = compose;
> +	v4l2_rect_map_inside(&vin->crop, &crop);
> +	v4l2_rect_map_inside(&vin->compose, &compose);
>  	vin->src_rect = crop;
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 4/6] rcar-vin: Do not reset the crop and compose rectangles in s_fmt
  2019-08-08  8:37   ` Laurent Pinchart
@ 2019-08-08 13:35     ` Niklas Söderlund
  2019-08-28 10:25       ` Hans Verkuil
  0 siblings, 1 reply; 14+ messages in thread
From: Niklas Söderlund @ 2019-08-08 13:35 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Kieran Bingham, linux-media, linux-renesas-soc

Hi Laurent,

Thanks for your feedback.

On 2019-08-08 11:37:51 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thu, Aug 08, 2019 at 03:18:48AM +0200, Niklas Söderlund wrote:
> > The crop and compose rectangles are reset when s_fmt is called
> > resulting in potentially valid rectangles being lost when updating the
> > format.
> 
> Isn't this the expected behaviour ?

I though so to at first but I had a nagging feeling as I was not sure.  
So I went and looked at vivid and it does do map the old crop/compose 
rectangles inside the new limits vivid_s_fmt_vid_cap().

Please note that the variable names in this patch are horrible and are 
fixed later in this series. In essence the current crop rectangle is 
mapped inside the size of the video coming from the sensor and the 
compose rectangle is mapped inside the new format set on the VIN video 
device.

I'm open to dropping this patch, I just want this driver to get this 
part right so I mimic vivid.

> 
> > Fix this by mapping the rectangles inside the new format.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index 8b30267f1636aaf1..5dcd787a9cf96ac9 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -279,8 +279,8 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
> >  		return ret;
> >  
> >  	vin->format = f->fmt.pix;
> > -	vin->crop = crop;
> > -	vin->compose = compose;
> > +	v4l2_rect_map_inside(&vin->crop, &crop);
> > +	v4l2_rect_map_inside(&vin->compose, &compose);
> >  	vin->src_rect = crop;
> >  
> >  	return 0;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 4/6] rcar-vin: Do not reset the crop and compose rectangles in s_fmt
  2019-08-08 13:35     ` Niklas Söderlund
@ 2019-08-28 10:25       ` Hans Verkuil
  0 siblings, 0 replies; 14+ messages in thread
From: Hans Verkuil @ 2019-08-28 10:25 UTC (permalink / raw)
  To: Niklas Söderlund, Laurent Pinchart
  Cc: Kieran Bingham, linux-media, linux-renesas-soc

On 8/8/19 3:35 PM, Niklas Söderlund wrote:
> Hi Laurent,
> 
> Thanks for your feedback.
> 
> On 2019-08-08 11:37:51 +0300, Laurent Pinchart wrote:
>> Hi Niklas,
>>
>> Thank you for the patch.
>>
>> On Thu, Aug 08, 2019 at 03:18:48AM +0200, Niklas Söderlund wrote:
>>> The crop and compose rectangles are reset when s_fmt is called
>>> resulting in potentially valid rectangles being lost when updating the
>>> format.
>>
>> Isn't this the expected behaviour ?
> 
> I though so to at first but I had a nagging feeling as I was not sure.  
> So I went and looked at vivid and it does do map the old crop/compose 
> rectangles inside the new limits vivid_s_fmt_vid_cap().
> 
> Please note that the variable names in this patch are horrible and are 
> fixed later in this series. In essence the current crop rectangle is 
> mapped inside the size of the video coming from the sensor and the 
> compose rectangle is mapped inside the new format set on the VIN video 
> device.

The only ioctls that completely reset everything are S_STD and S_DV_TIMINGS.

Ioctls such as S_FMT and S_SELECTION will try to keep as much of the
pre-existing configuration as possible.

So this patch is correct.

Regards,

	Hans

> 
> I'm open to dropping this patch, I just want this driver to get this 
> part right so I mimic vivid.
> 
>>
>>> Fix this by mapping the rectangles inside the new format.
>>>
>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>> ---
>>>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>> index 8b30267f1636aaf1..5dcd787a9cf96ac9 100644
>>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>> @@ -279,8 +279,8 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
>>>  		return ret;
>>>  
>>>  	vin->format = f->fmt.pix;
>>> -	vin->crop = crop;
>>> -	vin->compose = compose;
>>> +	v4l2_rect_map_inside(&vin->crop, &crop);
>>> +	v4l2_rect_map_inside(&vin->compose, &compose);
>>>  	vin->src_rect = crop;
>>>  
>>>  	return 0;
>>
>> -- 
>> Regards,
>>
>> Laurent Pinchart
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 3/6] rcar-vin: Rename rectangle holding the video source information
  2019-08-08  8:30   ` Laurent Pinchart
@ 2019-09-04 12:52     ` Niklas Söderlund
  0 siblings, 0 replies; 14+ messages in thread
From: Niklas Söderlund @ 2019-09-04 12:52 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Kieran Bingham, linux-media, linux-renesas-soc

Hi Laurent,

Thanks for your feedback.

On 2019-08-08 11:30:45 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thu, Aug 08, 2019 at 03:18:47AM +0200, Niklas Söderlund wrote:
> > The variable to hold the video source information dimensions was poorly
> > named 'source'. This is confusing as a lot of other members of structs
> > share the same name with different purposes, rename it src_rect in
> > preparation of refactoring code.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 33 +++++++++++----------
> >  drivers/media/platform/rcar-vin/rcar-vin.h  |  4 +--
> >  2 files changed, 19 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index 402b40fcf7184fde..8b30267f1636aaf1 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -163,13 +163,13 @@ static int rvin_reset_format(struct rvin_dev *vin)
> >  
> >  	rvin_format_align(vin, &vin->format);
> >  
> > -	vin->source.top = 0;
> > -	vin->source.left = 0;
> > -	vin->source.width = vin->format.width;
> > -	vin->source.height = vin->format.height;
> > +	vin->src_rect.top = 0;
> > +	vin->src_rect.left = 0;
> > +	vin->src_rect.width = vin->format.width;
> > +	vin->src_rect.height = vin->format.height;
> >  
> > -	vin->crop = vin->source;
> > -	vin->compose = vin->source;
> > +	vin->crop = vin->src_rect;
> > +	vin->compose = vin->src_rect;
> >  
> >  	return 0;
> >  }
> > @@ -281,7 +281,7 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
> >  	vin->format = f->fmt.pix;
> >  	vin->crop = crop;
> >  	vin->compose = compose;
> > -	vin->source = crop;
> > +	vin->src_rect = crop;
> >  
> >  	return 0;
> >  }
> > @@ -319,8 +319,8 @@ static int rvin_g_selection(struct file *file, void *fh,
> >  	case V4L2_SEL_TGT_CROP_BOUNDS:
> >  	case V4L2_SEL_TGT_CROP_DEFAULT:
> >  		s->r.left = s->r.top = 0;
> > -		s->r.width = vin->source.width;
> > -		s->r.height = vin->source.height;
> > +		s->r.width = vin->src_rect.width;
> > +		s->r.height = vin->src_rect.height;
> >  		break;
> >  	case V4L2_SEL_TGT_CROP:
> >  		s->r = vin->crop;
> > @@ -362,21 +362,22 @@ static int rvin_s_selection(struct file *file, void *fh,
> >  	case V4L2_SEL_TGT_CROP:
> >  		/* Can't crop outside of source input */
> >  		max_rect.top = max_rect.left = 0;
> > -		max_rect.width = vin->source.width;
> > -		max_rect.height = vin->source.height;
> > +		max_rect.width = vin->src_rect.width;
> > +		max_rect.height = vin->src_rect.height;
> >  		v4l2_rect_map_inside(&r, &max_rect);
> >  
> > -		v4l_bound_align_image(&r.width, 6, vin->source.width, 0,
> > -				      &r.height, 2, vin->source.height, 0, 0);
> > +		v4l_bound_align_image(&r.width, 6, vin->src_rect.width, 0,
> > +				      &r.height, 2, vin->src_rect.height, 0, 0);
> >  
> > -		r.top  = clamp_t(s32, r.top, 0, vin->source.height - r.height);
> > -		r.left = clamp_t(s32, r.left, 0, vin->source.width - r.width);
> > +		r.top  = clamp_t(s32, r.top, 0,
> > +				 vin->src_rect.height - r.height);
> > +		r.left = clamp_t(s32, r.left, 0, vin->src_rect.width - r.width);
> >  
> >  		vin->crop = s->r = r;
> >  
> >  		vin_dbg(vin, "Cropped %dx%d@%d:%d of %dx%d\n",
> >  			r.width, r.height, r.left, r.top,
> > -			vin->source.width, vin->source.height);
> > +			vin->src_rect.width, vin->src_rect.height);
> >  		break;
> >  	case V4L2_SEL_TGT_COMPOSE:
> >  		/* Make sure compose rect fits inside output format */
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > index e562c2ff21ec7e7b..86e9bad44484092c 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -176,7 +176,7 @@ struct rvin_info {
> >   *
> >   * @crop:		active cropping
> >   * @compose:		active composing
> > - * @source:		active size of the video source
> > + * @src_rect:		active size of the video source
> 
> As this only holds a size you don't need a full rectangle, src_width and
> src_height would save a bit of space. Up to you, in any case

I hope to get around to adding a struct v4l2_size to address this in the 
future. For now I prefer to keep it as a v4l2_rect as there are other 
members of struct rvin_dev which would benefit from a size structure as 
well and having them all the same makes it easier to spot IMHO.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks.

> 
> >   * @std:		active video standard of the video source
> >   *
> >   * @alpha:		Alpha component to fill in for supported pixel formats
> > @@ -215,7 +215,7 @@ struct rvin_dev {
> >  
> >  	struct v4l2_rect crop;
> >  	struct v4l2_rect compose;
> > -	struct v4l2_rect source;
> > +	struct v4l2_rect src_rect;
> >  	v4l2_std_id std;
> >  
> >  	unsigned int alpha;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-09-04 12:52 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-08  1:18 [PATCH v2 0/6] rcar-vin: Add support for V4L2_FIELD_ALTERNATE Niklas Söderlund
2019-08-08  1:18 ` [PATCH v2 1/6] rcar-vin: Fix incorrect return statement in rvin_try_format() Niklas Söderlund
2019-08-08  8:25   ` Laurent Pinchart
2019-08-08  1:18 ` [PATCH v2 2/6] rcar-vin: Make use of V4L2_FIELD_IS_INTERLACED() macro Niklas Söderlund
2019-08-08  8:27   ` Laurent Pinchart
2019-08-08  1:18 ` [PATCH v2 3/6] rcar-vin: Rename rectangle holding the video source information Niklas Söderlund
2019-08-08  8:30   ` Laurent Pinchart
2019-09-04 12:52     ` Niklas Söderlund
2019-08-08  1:18 ` [PATCH v2 4/6] rcar-vin: Do not reset the crop and compose rectangles in s_fmt Niklas Söderlund
2019-08-08  8:37   ` Laurent Pinchart
2019-08-08 13:35     ` Niklas Söderlund
2019-08-28 10:25       ` Hans Verkuil
2019-08-08  1:18 ` [PATCH v2 5/6] rcar-vin: Add support for V4L2_FIELD_ALTERNATE Niklas Söderlund
2019-08-08  1:18 ` [PATCH v2 6/6] rcar-vin: Clean up how format is set on subdevice Niklas Söderlund

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).