Re: [PATCH v13 16/33] rcar-vin: simplify how formats are set and reset

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

I have incorporated your suggestions for the next and hopefully last 
version of this patch-set, a few followups on your review bellow.

On 2018-04-04 01:09:29 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Tuesday, 27 March 2018 00:44:39 EEST Niklas Söderlund wrote:
> > With the recent cleanup of the format code to prepare for Gen3 it's
> > possible to simplify the Gen2 format code path as well. Clean up the
> > process by defining two functions to handle the set format and reset of
> > format when the standard is changed.
> > 
> > While at it replace the driver local struct rvin_source_fmt with a
> > struct v4l2_rect as all it's used for is keep track of the source
> > dimensions.
> 
> I wonder whether we should introduce v4l2_size (or  name 
> here>) when all we need is width and height, as v4l2_rect stores the top and 
> left offsets too. This doesn't have to be fixed here though.

Yes that would have been useful for this change. I have added this to my 
ever growing TODO list :-)

[snip]

> 
> > -   vin->format.bytesperline = rvin_format_bytesperline(>format);
> > -   vin->format.sizeimage = rvin_format_sizeimage(>format);
> > +   vin->source.top = vin->crop.top = 0;
> > +   vin->source.left = vin->crop.left = 0;
> > +   vin->source.width = vin->crop.width = vin->format.width;
> > +   vin->source.height = vin->crop.height = vin->format.height;
> 
> I find multiple assignations on the same line hard to read. How about
> 
>   vin->source.top = 0;
>   vin->source.left = 0;
>   vin->source.width = vin->format.width;
>   vin->source.height = vin->format.height;
> 
>   vin->crop = vin->source;
>   vin->compose = vin->source;

This looks much better and I will use it, thanks !

[snip]

> 
> > +   /*
> > +* 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;
> 
> You're duplicating this logic in rvin_format_align() so it makes me feel that 
> there's still room for some simplification, but that can be done in a 
> separate 
> patch (or I could simply be wrong).

I'm sure someone more clever then me can simplify this more. In this 
particular case rvin_format_align() deals with the video device format 
while here we deal with the crop rectangle. I will keep this in mind for 
future work and see if this can be unified in simpler way.

[snip]

> > -   return __rvin_try_format(vin, V4L2_SUBDEV_FORMAT_TRY, 
> > >fmt.pix,
> > -);
> > +   return rvin_try_format(vin, V4L2_SUBDEV_FORMAT_TRY, >fmt.pix, ,
> > +  );
> 
> How about making crop and compose optional in rvin_try_format() to avoid a 
> need for the two local variables here ?

Great suggestion, I have incorporated this for the next version.

> 
> Apart from these small issues, I think this is a nice simplification,
> 
> Reviewed-by: Laurent Pinchart 

Thanks!

-- 
Regards,
Niklas Söderlund

Re: [PATCH v13 16/33] rcar-vin: simplify how formats are set and reset

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday, 27 March 2018 00:44:39 EEST Niklas Söderlund wrote:
> With the recent cleanup of the format code to prepare for Gen3 it's
> possible to simplify the Gen2 format code path as well. Clean up the
> process by defining two functions to handle the set format and reset of
> format when the standard is changed.
> 
> While at it replace the driver local struct rvin_source_fmt with a
> struct v4l2_rect as all it's used for is keep track of the source
> dimensions.

I wonder whether we should introduce v4l2_size (or ) when all we need is width and height, as v4l2_rect stores the top and 
left offsets too. This doesn't have to be fixed here though.

> Signed-off-by: Niklas Söderlund 
> Reviewed-by: Hans Verkuil 
> 
> ---
> 
> * Changes since v11
> - This patch where 'rcar-vin: read subdevice format for crop only when
> needed'
> - Keep caching the source dimensions and drop all changes to
> rvin_g_selection() and rvin_s_selection().
> - Inline rvin_get_vin_format_from_source() into rvin_reset_format()
> which now is the only user left.
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 116 +++--
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  14 +---
>  2 files changed, 52 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> c39891386576afb8..c4be0bcb8b16f941 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -138,67 +138,60 @@ static int rvin_format_align(struct rvin_dev *vin,
> struct v4l2_pix_format *pix) * V4L2
>   */
> 
> -static void rvin_reset_crop_compose(struct rvin_dev *vin)
> -{
> - vin->crop.top = vin->crop.left = 0;
> - vin->crop.width = vin->source.width;
> - vin->crop.height = vin->source.height;
> -
> - vin->compose.top = vin->compose.left = 0;
> - vin->compose.width = vin->format.width;
> - vin->compose.height = vin->format.height;
> -}
> -
>  static int rvin_reset_format(struct rvin_dev *vin)
>  {
>   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(>format, );
> 
> - rvin_reset_crop_compose(vin);
> + ret = rvin_format_align(vin, >format);
> + if (ret)
> + return ret;

rvin_format_align() always returns 0 so you can remove the error check. You 
can actually turn the function into a void function.

> - vin->format.bytesperline = rvin_format_bytesperline(>format);
> - vin->format.sizeimage = rvin_format_sizeimage(>format);
> + vin->source.top = vin->crop.top = 0;
> + vin->source.left = vin->crop.left = 0;
> + vin->source.width = vin->crop.width = vin->format.width;
> + vin->source.height = vin->crop.height = vin->format.height;

I find multiple assignations on the same line hard to read. How about

vin->source.top = 0;
vin->source.left = 0;
vin->source.width = vin->format.width;
vin->source.height = vin->format.height;

vin->crop = vin->source;
vin->compose = vin->source;

> + vin->compose.top = vin->compose.left = 0;
> + vin->compose.width = vin->format.width;
> + vin->compose.height = vin->format.height;
> 
>   return 0;
>  }

I like the new rvin_reset_format(), it's much simpler.

> -static int __rvin_try_format_source(struct rvin_dev *vin,
> - u32 which,
> - struct v4l2_pix_format *pix,
> - struct rvin_source_fmt *source)
> +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_subdev *sd;
> + struct v4l2_subdev *sd = vin_to_source(vin);
>   struct v4l2_subdev_pad_config *pad_cfg;
>   struct v4l2_subdev_format format = {
>   .which = which,
> + .pad = vin->digital->source_pad,
>   };
>   enum v4l2_field field;
>   u32 width, height;
>   int ret;
> 
> - sd = vin_to_source(vin);
> -
> - v4l2_fill_mbus_format(, pix, vin->digital->code);
> -
>   pad_cfg = v4l2_subdev_alloc_pad_config(sd);
>   if (pad_cfg == NULL)
>   return -ENOMEM;
> 
> - format.pad = vin->digital->source_pad;
> + if 

[PATCH v13 16/33] rcar-vin: simplify how formats are set and reset

[Niklas Söderlund]

With the recent cleanup of the format code to prepare for Gen3 it's
possible to simplify the Gen2 format code path as well. Clean up the
process by defining two functions to handle the set format and reset of
format when the standard is changed.

While at it replace the driver local struct rvin_source_fmt with a
struct v4l2_rect as all it's used for is keep track of the source
dimensions.

Signed-off-by: Niklas Söderlund 
Reviewed-by: Hans Verkuil 

---

* Changes since v11
- This patch where 'rcar-vin: read subdevice format for crop only when
needed'
- Keep caching the source dimensions and drop all changes to
rvin_g_selection() and rvin_s_selection().
- Inline rvin_get_vin_format_from_source() into rvin_reset_format()
which now is the only user left.
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 116 
 drivers/media/platform/rcar-vin/rcar-vin.h  |  14 +---
 2 files changed, 52 insertions(+), 78 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c 
b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index c39891386576afb8..c4be0bcb8b16f941 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -138,67 +138,60 @@ static int rvin_format_align(struct rvin_dev *vin, struct 
v4l2_pix_format *pix)
  * V4L2
  */
 
-static void rvin_reset_crop_compose(struct rvin_dev *vin)
-{
-   vin->crop.top = vin->crop.left = 0;
-   vin->crop.width = vin->source.width;
-   vin->crop.height = vin->source.height;
-
-   vin->compose.top = vin->compose.left = 0;
-   vin->compose.width = vin->format.width;
-   vin->compose.height = vin->format.height;
-}
-
 static int rvin_reset_format(struct rvin_dev *vin)
 {
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(>format, );
 
-   rvin_reset_crop_compose(vin);
+   ret = rvin_format_align(vin, >format);
+   if (ret)
+   return ret;
 
-   vin->format.bytesperline = rvin_format_bytesperline(>format);
-   vin->format.sizeimage = rvin_format_sizeimage(>format);
+   vin->source.top = vin->crop.top = 0;
+   vin->source.left = vin->crop.left = 0;
+   vin->source.width = vin->crop.width = vin->format.width;
+   vin->source.height = vin->crop.height = vin->format.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_try_format_source(struct rvin_dev *vin,
-   u32 which,
-   struct v4l2_pix_format *pix,
-   struct rvin_source_fmt *source)
+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_subdev *sd;
+   struct v4l2_subdev *sd = vin_to_source(vin);
struct v4l2_subdev_pad_config *pad_cfg;
struct v4l2_subdev_format format = {
.which = which,
+   .pad = vin->digital->source_pad,
};
enum v4l2_field field;
u32 width, height;
int ret;
 
-   sd = vin_to_source(vin);
-
-   v4l2_fill_mbus_format(, pix, vin->digital->code);
-
pad_cfg = v4l2_subdev_alloc_pad_config(sd);
if (pad_cfg == NULL)
return -ENOMEM;
 
-   format.pad = vin->digital->source_pad;
+   if (!rvin_format_from_pixel(pix->pixelformat) ||
+   (vin->info->model == RCAR_M1 &&
+pix->pixelformat == V4L2_PIX_FMT_XBGR32))
+   pix->pixelformat = RVIN_DEFAULT_FORMAT;
+
+   v4l2_fill_mbus_format(, pix, vin->digital->code);
 
/* Allow the video device to override field and to scale */
field = pix->field;
@@ -211,8 +204,16 @@ static int __rvin_try_format_source(struct rvin_dev *vin,
 
v4l2_fill_pix_format(pix, );
 
-   source->width = pix->width;
-   source->height = pix->height;
+   crop->top = 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)