[PATCH 0/4] rcar-vin: Add support for V4L2_FIELD_ALTERNATE

[PATCH 0/4] rcar-vin: Add support for V4L2_FIELD_ALTERNATE

[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/4 prepares for the work by renaming a poorly variable, 2/4 fixes 
a problem with scaling (Gen2 only) which was found when testing 
alternating between V4L2_FIELD_ALTERNATE and V4L2_FIELD_INTERLACED.  
Patch 3/4 is the real change adding support for the new field format.  
And last 4/4 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 (4):
  rcar-vin: Rename rectangle holding 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.21.0

[PATCH 1/4] rcar-vin: Rename rectangle holding holding the video source information

[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 struts
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>
---
 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 04d798d8070f912c..2d94e700a473572c 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -155,13 +155,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;
 }
@@ -273,7 +273,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;
 }
@@ -311,8 +311,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;
@@ -354,21 +354,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 ac1a6fe90015ed69..f91cf1f26afe7a52 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
  */
 struct rvin_dev {
@@ -213,7 +213,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;
 };
 
-- 
2.21.0

Re: [PATCH 1/4] rcar-vin: Rename rectangle holding holding the video source information

[Geert Uytterhoeven]

Hi Niklas,

Please drop one "holding" from the subject.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/4] rcar-vin: Rename rectangle holding holding the video source information

[Kieran Bingham]

Hi Niklas,

On 05/07/2019 05:55, 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 struts

s/struts/structs/

> share the same name with different purposes, rename it src_rect in
> preparation of refactoring code.
> 

Sounds reasonable to me, and matches the other _rect instances in the
code base.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  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 04d798d8070f912c..2d94e700a473572c 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -155,13 +155,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;
>  }
> @@ -273,7 +273,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;
>  }
> @@ -311,8 +311,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;
> @@ -354,21 +354,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 ac1a6fe90015ed69..f91cf1f26afe7a52 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
>   */
>  struct rvin_dev {
> @@ -213,7 +213,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;
>  };
>  
> 

[PATCH 2/4] rcar-vin: Do not reset the crop and compose rectangles in s_fmt

[Niklas Söderlund]

The crop and compose rectangles where reset when s_fmt was called
resulting in potentially valid rectangles where lost when updating the
format. Fix this by instead trying to map the rectangles inside the new
format.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 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 2d94e700a473572c..d5e860ba6d9a2409 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -271,8 +271,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.21.0

Re: [PATCH 2/4] rcar-vin: Do not reset the crop and compose rectangles in s_fmt

[Sergei Shtylyov]

Hello!

On 05.07.2019 7:55, Niklas Söderlund wrote:

> The crop and compose rectangles where reset when s_fmt was called

    s/where/were/?

> resulting in potentially valid rectangles where lost when updating the

    And here too...

> format. Fix this by instead trying to map the rectangles inside the new
> format.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei

Re: [PATCH 2/4] rcar-vin: Do not reset the crop and compose rectangles in s_fmt

[Kieran Bingham]

Hi Niklas

On 05/07/2019 05:55, Niklas Söderlund wrote:
> The crop and compose rectangles where reset when s_fmt was called

s/where reset/are reset/
s/was called/is called/

> resulting in potentially valid rectangles where lost when updating the

s/where lost/being lost/

> format. Fix this by instead trying to map the rectangles inside the new

I don't think there's any 'trying' here - just doing.

  "Fix this by mapping the rectangles inside ..."

> format.
> 

It could be worth expanding on the consequences of this patch here:

"This may result in the crop and compose windows being modified from the
original setting to ensure that they are valid within the dimensions of
the new format."

But perhaps that's just too much repetition. It's just the point about
making it clear that the existing cropping and compose rectangles may be
different after this call that might be worth expressing somehow.


> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Does this deserve a fixes tag? Or is this more of a solitary change.

With the wordings fixed, and expanded if you desire:

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 2d94e700a473572c..d5e860ba6d9a2409 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -271,8 +271,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;
> 

[PATCH 3/4] rcar-vin: Add support for V4L2_FIELD_ALTERNATE

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

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

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 372d6b106b9970d2..7ac1733455221fe0 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -526,12 +526,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;
@@ -554,16 +559,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",
@@ -577,21 +577,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)
@@ -636,6 +624,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;
@@ -788,6 +779,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;
@@ -937,7 +940,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,
@@ -1064,6 +1067,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 d5e860ba6d9a2409..bc96ed563e365448 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -106,15 +106,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;
@@ -153,15 +145,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;
 }
@@ -205,13 +207,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.21.0

Re: [PATCH 3/4] rcar-vin: Add support for V4L2_FIELD_ALTERNATE

[Kieran Bingham]

Hi Niklas,

On 05/07/2019 05:55, Niklas Söderlund wrote:
> 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.

I'm afraid I have found this patch quite difficult to review accurately ...

I think I can infer that we are removing an existing workaround where
V4L2_FIELD_ALTERNATE was converted to V4L2_FIELD_INTERLACED_xx formats,
and also now where we used to store 'frame' heights, we store 'field'
heights...

Is that somewhere close as an approximation? (Perhaps it might be good
to detail some of that in the commit message, at least any bits that are
accurate of course)


I might have to look at this again later, or let some other eyeballs
look as I'm afraid I don't feel that I've got a good overview of it yet.

I wonder if it could be split in anyway to be clearer, but it's hard to
tell :-)

Perhaps it's just me being unable to see all the changes at once.


< Some of my discussion comments below might seem out of order, as I've
made multiple passes through this :-D >

> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c  | 54 +++++++++++----------
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 31 +++++-------
>  2 files changed, 42 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 372d6b106b9970d2..7ac1733455221fe0 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -526,12 +526,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;
> @@ -554,16 +559,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))

Ok, so I had to go check - V4L2_FIELD_IS_INTERLACED() does not include
'_ALTERNATE' - so this hunk is an improvement, but a somewhat unrelated
change.

Perhaps this could be split out to before this patch, anything to
simplify this patch would be good.

>  		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",
> @@ -577,21 +577,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);

Should those be s/vin->crop.height/crop_height/ ? <edit - no>

How come there's no comparable if (V4L2_FIELD_IS_INTERLACED... in this
function?

Oh - because actually rvin_crop_scale_comp_gen2() is called from within
this function. They are not parallel functions for two implementations.


>  
> -	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;

So - I think if i understand correctly - we used to store the
frame-height in vin->crop, and now we store the field height.

Is that right ?
 (where field-height == frame-height on progressive frames)



> -	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)
> @@ -636,6 +624,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;
> @@ -788,6 +779,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;
> @@ -937,7 +940,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,
> @@ -1064,6 +1067,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 d5e860ba6d9a2409..bc96ed563e365448 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -106,15 +106,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;

Ok - now I get it, this used to double the format height to work around
the lack of _ALTERNATE implementation on the sink pad/device...

So this part is removal of the existing workaround.


>  		break;
>  	default:
>  		pix->field = RVIN_DEFAULT_FIELD;
> @@ -153,15 +145,25 @@ static int rvin_reset_format(struct rvin_dev *vin)
>  
>  	v4l2_fill_pix_format(&vin->format, &fmt.format);

This call v4l2_fill_pix_format() does the following:
 "vin->format.field = fmt.format.field;"

Ok - so that's obtaining the *source pad 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;
> +	}

And here we are resetting the vin->format which looks like it represents
the VIN sink device right?

I guess we are changing alternate-fields to interlaced frames to prevent
the driver from 'passing through' alternate formats to maintain the
user-space experience here?


> +
> +	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;
>  }
> @@ -205,13 +207,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;

And this part is just removing of the previous workaround right?


>  	}
>  
>  	if (field != V4L2_FIELD_ANY)
> 

Re: [PATCH 3/4] rcar-vin: Add support for V4L2_FIELD_ALTERNATE

[Niklas Söderlund]

HI Kieran,

Thanks for your review!

On 2019-07-19 12:13:30 +0100, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 05/07/2019 05:55, Niklas Söderlund wrote:
> > 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.
> 
> I'm afraid I have found this patch quite difficult to review accurately ...

Yes this have been a confusing workaround to have in the code and it's 
blocking many other things I wish to do with the driver.

> 
> I think I can infer that we are removing an existing workaround where
> V4L2_FIELD_ALTERNATE was converted to V4L2_FIELD_INTERLACED_xx formats,
> and also now where we used to store 'frame' heights, we store 'field'
> heights...

Yes. But it's still the default behavior to combine V4L2_FIELD_ALTERNATE 
into V4L2_FIELD_INTERLACED if the sensor reports V4L2_FIELD_ALTERNATE as 
I do not wish to change the user-space behavior. This only happens if 
V4L2_FIELD_ANY is requested by the user. This patch allows the user to 
request V4L2_FIELD_ALTERNATE and get it.

> 
> Is that somewhere close as an approximation? (Perhaps it might be good
> to detail some of that in the commit message, at least any bits that are
> accurate of course)

I will try to expand the commit message thanks for pointing it out.

> 
> 
> I might have to look at this again later, or let some other eyeballs
> look as I'm afraid I don't feel that I've got a good overview of it yet.
> 
> I wonder if it could be split in anyway to be clearer, but it's hard to
> tell :-)
> 
> Perhaps it's just me being unable to see all the changes at once.
> 
> 
> < Some of my discussion comments below might seem out of order, as I've
> made multiple passes through this :-D >
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c  | 54 +++++++++++----------
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 31 +++++-------
> >  2 files changed, 42 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index 372d6b106b9970d2..7ac1733455221fe0 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -526,12 +526,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;
> > @@ -554,16 +559,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))
> 
> Ok, so I had to go check - V4L2_FIELD_IS_INTERLACED() does not include
> '_ALTERNATE' - so this hunk is an improvement, but a somewhat unrelated
> change.
> 
> Perhaps this could be split out to before this patch, anything to
> simplify this patch would be good.

Good point, will do so for next version.

> 
> >  		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",
> > @@ -577,21 +577,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);
> 
> Should those be s/vin->crop.height/crop_height/ ? <edit - no>
> 
> How come there's no comparable if (V4L2_FIELD_IS_INTERLACED... in this
> function?
> 
> Oh - because actually rvin_crop_scale_comp_gen2() is called from within
> this function. They are not parallel functions for two implementations.

It's awesome when someone answer there own questions ;-)

> 
> 
> >  
> > -	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;
> 
> So - I think if i understand correctly - we used to store the
> frame-height in vin->crop, and now we store the field height.

Yes.

> 
> Is that right ?
>  (where field-height == frame-height on progressive frames)

Yes.

> 
> 
> 
> > -	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)
> > @@ -636,6 +624,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;
> > @@ -788,6 +779,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;
> > @@ -937,7 +940,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,
> > @@ -1064,6 +1067,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 d5e860ba6d9a2409..bc96ed563e365448 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -106,15 +106,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;
> 
> Ok - now I get it, this used to double the format height to work around
> the lack of _ALTERNATE implementation on the sink pad/device...
> 
> So this part is removal of the existing workaround.
> 
> 
> >  		break;
> >  	default:
> >  		pix->field = RVIN_DEFAULT_FIELD;
> > @@ -153,15 +145,25 @@ static int rvin_reset_format(struct rvin_dev *vin)
> >  
> >  	v4l2_fill_pix_format(&vin->format, &fmt.format);
> 
> This call v4l2_fill_pix_format() does the following:
>  "vin->format.field = fmt.format.field;"
> 
> Ok - so that's obtaining the *source pad 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;
> > +	}
> 
> And here we are resetting the vin->format which looks like it represents
> the VIN sink device right?

Correct.

> 
> I guess we are changing alternate-fields to interlaced frames to prevent
> the driver from 'passing through' alternate formats to maintain the
> user-space experience here?

Yes, the current user-space behavior is to always combine alternate to 
interlaced. With this change the user can request V4L2_FIELD_ALTERNATE 
and get it but if V4L2_FIELD_ANY is asked for it still gets 
V4L2_FIELD_INTERLACED (if the sensor delivers V4L2_FIELD_ALTERNATE).

> 
> 
> > +
> > +	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;
> >  }
> > @@ -205,13 +207,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;
> 
> And this part is just removing of the previous workaround right?

Yes.

I don't think you are so confused about this change you got it all 
correctly ;-) Thanks for your effort in reviewing this!

> 
> 
> >  	}
> >  
> >  	if (field != V4L2_FIELD_ANY)
> > 
> 

-- 
Regards,
Niklas Söderlund

[PATCH 4/4] rcar-vin: Clean up how format is set on subdevice

[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 | 44 ++++++++++-----------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index bc96ed563e365448..fa6cc1b76f02133e 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -170,7 +170,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;
@@ -189,24 +189,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)
@@ -216,17 +216,10 @@ 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);
 
-	return 0;
+	return ret;
 }
 
 static int rvin_querycap(struct file *file, void *priv,
@@ -246,29 +239,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.21.0

Re: [PATCH 4/4] rcar-vin: Clean up how format is set on subdevice

[Kieran Bingham]

Hi Niklas,

On 05/07/2019 05:55, Niklas Söderlund wrote:
> 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 | 44 ++++++++++-----------
>  1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index bc96ed563e365448..fa6cc1b76f02133e 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -170,7 +170,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;
> @@ -189,24 +189,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)
> @@ -216,17 +216,10 @@ 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);
>  
> -	return 0;
> +	return ret;

This return value change looks suspiciously like a small bug-fix.
Does it deserve it's own patch and fixes tag for the stable trees?



>  }
>  
>  static int rvin_querycap(struct file *file, void *priv,
> @@ -246,29 +239,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;
>  }
>