Re: [PATCH v6 10/25] rcar-vin: read subdevice format for crop only when needed

[Hans Verkuil]

On 23/08/17 01:26, Niklas Söderlund wrote:
> Instead of caching the subdevice format each time the video device
> format is set read it directly when its needed. As it turns out the

its -> it's

> format is only needed when figuring out the max rectangle for cropping.
> 
> This simplify the code and makes it clearer what the source format is

simplify -> simplifies

> used for.
> 
> Signed-off-by: Niklas Söderlund 

Reviewed-by: Hans Verkuil 

Regards,

Hans

> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 88 
> ++---
>  drivers/media/platform/rcar-vin/rcar-vin.h  | 12 
>  2 files changed, 42 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c 
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 305a74d033b2d9c5..c8c764188b85a926 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -90,24 +90,30 @@ static u32 rvin_format_sizeimage(struct v4l2_pix_format 
> *pix)
>   * V4L2
>   */
>  
> -int rvin_reset_format(struct rvin_dev *vin)
> +static int rvin_get_sd_format(struct rvin_dev *vin, struct v4l2_pix_format 
> *pix)
>  {
>   struct v4l2_subdev_format fmt = {
>   .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> + .pad = vin->digital.source_pad,
>   };
> - struct v4l2_mbus_framefmt *mf = 
>   int ret;
>  
> - fmt.pad = vin->digital.source_pad;
> -
>   ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, );
>   if (ret)
>   return ret;
>  
> - vin->format.width   = mf->width;
> - vin->format.height  = mf->height;
> - vin->format.colorspace  = mf->colorspace;
> - vin->format.field   = mf->field;
> + v4l2_fill_pix_format(pix, );
> +
> + return 0;
> +}
> +
> +int rvin_reset_format(struct rvin_dev *vin)
> +{
> + int ret;
> +
> + ret = rvin_get_sd_format(vin, >format);
> + if (ret)
> + return ret;
>  
>   /*
>* If the subdevice uses ALTERNATE field mode and G_STD is
> @@ -137,12 +143,12 @@ int rvin_reset_format(struct rvin_dev *vin)
>   }
>  
>   vin->crop.top = vin->crop.left = 0;
> - vin->crop.width = mf->width;
> - vin->crop.height = mf->height;
> + vin->crop.width = vin->format.width;
> + vin->crop.height = vin->format.height;
>  
>   vin->compose.top = vin->compose.left = 0;
> - vin->compose.width = mf->width;
> - vin->compose.height = mf->height;
> + vin->compose.width = vin->format.width;
> + vin->compose.height = vin->format.height;
>  
>   vin->format.bytesperline = rvin_format_bytesperline(>format);
>   vin->format.sizeimage = rvin_format_sizeimage(>format);
> @@ -151,9 +157,7 @@ int rvin_reset_format(struct rvin_dev *vin)
>  }
>  
>  static int __rvin_try_format_source(struct rvin_dev *vin,
> - u32 which,
> - struct v4l2_pix_format *pix,
> - struct rvin_source_fmt *source)
> + u32 which, struct v4l2_pix_format *pix)
>  {
>   struct v4l2_subdev *sd;
>   struct v4l2_subdev_pad_config *pad_cfg;
> @@ -186,25 +190,15 @@ static int __rvin_try_format_source(struct rvin_dev 
> *vin,
>   v4l2_fill_pix_format(pix, );
>  
>   pix->field = field;
> -
> - source->width = pix->width;
> - source->height = pix->height;
> -
>   pix->width = width;
>   pix->height = height;
> -
> - vin_dbg(vin, "Source resolution: %ux%u\n", source->width,
> - source->height);
> -
>  done:
>   v4l2_subdev_free_pad_config(pad_cfg);
>   return ret;
>  }
>  
>  static int __rvin_try_format(struct rvin_dev *vin,
> -  u32 which,
> -  struct v4l2_pix_format *pix,
> -  struct rvin_source_fmt *source)
> +  u32 which, struct v4l2_pix_format *pix)
>  {
>   u32 walign;
>   int ret;
> @@ -225,7 +219,7 @@ static int __rvin_try_format(struct rvin_dev *vin,
>   pix->sizeimage = 0;
>  
>   /* Limit to source capabilities */
> - ret = __rvin_try_format_source(vin, which, pix, source);
> + ret = __rvin_try_format_source(vin, which, pix);
>   if (ret)
>   return ret;
>  
> @@ -234,7 +228,6 @@ static int __rvin_try_format(struct rvin_dev *vin,
>   case V4L2_FIELD_BOTTOM:
>   case V4L2_FIELD_ALTERNATE:
>   pix->height /= 2;
> - source->height /= 2;
>   break;
>   case V4L2_FIELD_NONE:
>   case V4L2_FIELD_INTERLACED_TB:
> @@ -286,30 +279,23 @@ static int rvin_try_fmt_vid_cap(struct file *file, void 
> *priv,
>   struct v4l2_format *f)
>  {
>   struct rvin_dev *vin = video_drvdata(file);
> - struct rvin_source_fmt source;
>  
> - return __rvin_try_format(vin, 

[PATCH v6 10/25] rcar-vin: read subdevice format for crop only when needed

[Niklas Söderlund]

Instead of caching the subdevice format each time the video device
format is set read it directly when its needed. As it turns out the
format is only needed when figuring out the max rectangle for cropping.

This simplify the code and makes it clearer what the source format is
used for.

Signed-off-by: Niklas Söderlund 
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 88 ++---
 drivers/media/platform/rcar-vin/rcar-vin.h  | 12 
 2 files changed, 42 insertions(+), 58 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c 
b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 305a74d033b2d9c5..c8c764188b85a926 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -90,24 +90,30 @@ static u32 rvin_format_sizeimage(struct v4l2_pix_format 
*pix)
  * V4L2
  */
 
-int rvin_reset_format(struct rvin_dev *vin)
+static int rvin_get_sd_format(struct rvin_dev *vin, struct v4l2_pix_format 
*pix)
 {
struct v4l2_subdev_format fmt = {
.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+   .pad = vin->digital.source_pad,
};
-   struct v4l2_mbus_framefmt *mf = 
int ret;
 
-   fmt.pad = vin->digital.source_pad;
-
ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, );
if (ret)
return ret;
 
-   vin->format.width   = mf->width;
-   vin->format.height  = mf->height;
-   vin->format.colorspace  = mf->colorspace;
-   vin->format.field   = mf->field;
+   v4l2_fill_pix_format(pix, );
+
+   return 0;
+}
+
+int rvin_reset_format(struct rvin_dev *vin)
+{
+   int ret;
+
+   ret = rvin_get_sd_format(vin, >format);
+   if (ret)
+   return ret;
 
/*
 * If the subdevice uses ALTERNATE field mode and G_STD is
@@ -137,12 +143,12 @@ int rvin_reset_format(struct rvin_dev *vin)
}
 
vin->crop.top = vin->crop.left = 0;
-   vin->crop.width = mf->width;
-   vin->crop.height = mf->height;
+   vin->crop.width = vin->format.width;
+   vin->crop.height = vin->format.height;
 
vin->compose.top = vin->compose.left = 0;
-   vin->compose.width = mf->width;
-   vin->compose.height = mf->height;
+   vin->compose.width = vin->format.width;
+   vin->compose.height = vin->format.height;
 
vin->format.bytesperline = rvin_format_bytesperline(>format);
vin->format.sizeimage = rvin_format_sizeimage(>format);
@@ -151,9 +157,7 @@ int rvin_reset_format(struct rvin_dev *vin)
 }
 
 static int __rvin_try_format_source(struct rvin_dev *vin,
-   u32 which,
-   struct v4l2_pix_format *pix,
-   struct rvin_source_fmt *source)
+   u32 which, struct v4l2_pix_format *pix)
 {
struct v4l2_subdev *sd;
struct v4l2_subdev_pad_config *pad_cfg;
@@ -186,25 +190,15 @@ static int __rvin_try_format_source(struct rvin_dev *vin,
v4l2_fill_pix_format(pix, );
 
pix->field = field;
-
-   source->width = pix->width;
-   source->height = pix->height;
-
pix->width = width;
pix->height = height;
-
-   vin_dbg(vin, "Source resolution: %ux%u\n", source->width,
-   source->height);
-
 done:
v4l2_subdev_free_pad_config(pad_cfg);
return ret;
 }
 
 static int __rvin_try_format(struct rvin_dev *vin,
-u32 which,
-struct v4l2_pix_format *pix,
-struct rvin_source_fmt *source)
+u32 which, struct v4l2_pix_format *pix)
 {
u32 walign;
int ret;
@@ -225,7 +219,7 @@ static int __rvin_try_format(struct rvin_dev *vin,
pix->sizeimage = 0;
 
/* Limit to source capabilities */
-   ret = __rvin_try_format_source(vin, which, pix, source);
+   ret = __rvin_try_format_source(vin, which, pix);
if (ret)
return ret;
 
@@ -234,7 +228,6 @@ static int __rvin_try_format(struct rvin_dev *vin,
case V4L2_FIELD_BOTTOM:
case V4L2_FIELD_ALTERNATE:
pix->height /= 2;
-   source->height /= 2;
break;
case V4L2_FIELD_NONE:
case V4L2_FIELD_INTERLACED_TB:
@@ -286,30 +279,23 @@ static int rvin_try_fmt_vid_cap(struct file *file, void 
*priv,
struct v4l2_format *f)
 {
struct rvin_dev *vin = video_drvdata(file);
-   struct rvin_source_fmt source;
 
-   return __rvin_try_format(vin, V4L2_SUBDEV_FORMAT_TRY, >fmt.pix,
-);
+   return __rvin_try_format(vin, V4L2_SUBDEV_FORMAT_TRY, >fmt.pix);
 }
 
 static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
  struct v4l2_format *f)
 {
struct rvin_dev *vin =