Re: [PATCH v10 09/30] rcar-vin: read subdevice format for crop only when needed

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Monday, 29 January 2018 18:34:14 EET Niklas Söderlund wrote:
> Instead of caching the subdevice format each time the video device
> format is set read it directly when it's needed. As it turns out the
> format is only needed when figuring out the max rectangle for cropping.
> 
> This simplifies 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 | 112 +---
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  12 ---
>  2 files changed, 61 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> c2265324c7c96308..4d5be2d0c79c9c9a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -90,35 +90,54 @@ static u32 rvin_format_sizeimage(struct v4l2_pix_format
> *pix) * V4L2
>   */
> 
> -static void rvin_reset_crop_compose(struct rvin_dev *vin)
> +static int rvin_get_source_format(struct rvin_dev *vin,
> +   struct v4l2_mbus_framefmt *mbus_fmt)
>  {
> + struct v4l2_subdev_format fmt = {
> + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> + .pad = vin->digital->source_pad,
> + };
> + int ret;
> +
> + ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, );
> + if (ret)
> + return ret;
> +
> + memcpy(mbus_fmt, , sizeof(*mbus_fmt));

You can use

*mbus_fmt = fmt.format;

That way the compiler will catch more mistakes, for instance incompatible 
types between the two arguments.

> +
> + return 0;
> +}
> +
> +static int rvin_reset_crop_compose(struct rvin_dev *vin)
> +{
> + struct v4l2_mbus_framefmt source_fmt;
> + int ret;
> +
> + ret = rvin_get_source_format(vin, _fmt);
> + if (ret)
> + return ret;
> +
>   vin->crop.top = vin->crop.left = 0;
> - vin->crop.width = vin->source.width;
> - vin->crop.height = vin->source.height;
> + vin->crop.width = source_fmt.width;
> + vin->crop.height = source_fmt.height;
> 
>   vin->compose.top = vin->compose.left = 0;
>   vin->compose.width = vin->format.width;
>   vin->compose.height = vin->format.height;
> +
> + return 0;
>  }
> 
>  static int rvin_reset_format(struct rvin_dev *vin)
>  {
> - struct v4l2_subdev_format fmt = {
> - .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> - };
> - struct v4l2_mbus_framefmt *mf = 
> + struct v4l2_mbus_framefmt source_fmt;
>   int ret;
> 
> - fmt.pad = vin->digital->source_pad;
> -
> - ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, );
> + ret = rvin_get_source_format(vin, _fmt);
>   if (ret)
>   return ret;

You retrieve the source format once here...

> - vin->format.width   = mf->width;
> - vin->format.height  = mf->height;
> - vin->format.colorspace  = mf->colorspace;
> - vin->format.field   = mf->field;
> + v4l2_fill_pix_format(>format, _fmt);
> 
>   /*
>* If the subdevice uses ALTERNATE field mode and G_STD is
> @@ -147,7 +166,9 @@ static int rvin_reset_format(struct rvin_dev *vin)
>   break;
>   }
> 
> - rvin_reset_crop_compose(vin);
> + ret = rvin_reset_crop_compose(vin);

... and this function then retrieves it a second time. Can't you pass it to 
rvin_reset_crop_compose() ? If the source changes its format autonomously 
between the two calls you'll end up with an inconsistent result otherwise.

> + if (ret)
> + return ret;
> 
>   vin->format.bytesperline = rvin_format_bytesperline(>format);
>   vin->format.sizeimage = rvin_format_sizeimage(>format);
> @@ -156,9 +177,7 @@ static 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;
> @@ -190,25 +209,16 @@ static int __rvin_try_format_source(struct rvin_dev
> *vin,
> 
>   v4l2_fill_pix_format(pix, );
> 
> - source->width = pix->width;
> - source->height = pix->height;
> -
>   pix->field = field;
>   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,
> -

[PATCH v10 09/30] 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 it's needed. As it turns out the
format is only needed when figuring out the max rectangle for cropping.

This simplifies 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 | 112 +++-
 drivers/media/platform/rcar-vin/rcar-vin.h  |  12 ---
 2 files changed, 61 insertions(+), 63 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c 
b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index c2265324c7c96308..4d5be2d0c79c9c9a 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -90,35 +90,54 @@ static u32 rvin_format_sizeimage(struct v4l2_pix_format 
*pix)
  * V4L2
  */
 
-static void rvin_reset_crop_compose(struct rvin_dev *vin)
+static int rvin_get_source_format(struct rvin_dev *vin,
+ struct v4l2_mbus_framefmt *mbus_fmt)
 {
+   struct v4l2_subdev_format fmt = {
+   .which = V4L2_SUBDEV_FORMAT_ACTIVE,
+   .pad = vin->digital->source_pad,
+   };
+   int ret;
+
+   ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, );
+   if (ret)
+   return ret;
+
+   memcpy(mbus_fmt, , sizeof(*mbus_fmt));
+
+   return 0;
+}
+
+static int rvin_reset_crop_compose(struct rvin_dev *vin)
+{
+   struct v4l2_mbus_framefmt source_fmt;
+   int ret;
+
+   ret = rvin_get_source_format(vin, _fmt);
+   if (ret)
+   return ret;
+
vin->crop.top = vin->crop.left = 0;
-   vin->crop.width = vin->source.width;
-   vin->crop.height = vin->source.height;
+   vin->crop.width = source_fmt.width;
+   vin->crop.height = source_fmt.height;
 
vin->compose.top = vin->compose.left = 0;
vin->compose.width = vin->format.width;
vin->compose.height = vin->format.height;
+
+   return 0;
 }
 
 static int rvin_reset_format(struct rvin_dev *vin)
 {
-   struct v4l2_subdev_format fmt = {
-   .which = V4L2_SUBDEV_FORMAT_ACTIVE,
-   };
-   struct v4l2_mbus_framefmt *mf = 
+   struct v4l2_mbus_framefmt source_fmt;
int ret;
 
-   fmt.pad = vin->digital->source_pad;
-
-   ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, );
+   ret = rvin_get_source_format(vin, _fmt);
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(>format, _fmt);
 
/*
 * If the subdevice uses ALTERNATE field mode and G_STD is
@@ -147,7 +166,9 @@ static int rvin_reset_format(struct rvin_dev *vin)
break;
}
 
-   rvin_reset_crop_compose(vin);
+   ret = rvin_reset_crop_compose(vin);
+   if (ret)
+   return ret;
 
vin->format.bytesperline = rvin_format_bytesperline(>format);
vin->format.sizeimage = rvin_format_sizeimage(>format);
@@ -156,9 +177,7 @@ static 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;
@@ -190,25 +209,16 @@ static int __rvin_try_format_source(struct rvin_dev *vin,
 
v4l2_fill_pix_format(pix, );
 
-   source->width = pix->width;
-   source->height = pix->height;
-
pix->field = field;
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;
@@ -229,7 +239,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;
 
@@ -238,7 +248,6 @@ static int __rvin_try_format(struct rvin_dev *vin,
case V4L2_FIELD_BOTTOM:
case V4L2_FIELD_ALTERNATE:
pix->height /= 2;
-