Re: [PATCH 2/2] rcar-vin: Create compose rectangle where it is used

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Nklas,

Thank you for the patch.

On Wed, Oct 09, 2019 at 01:22:01AM +0200, Niklas Söderlund wrote:
> The rectangle used to correct the compose settings when changing the
> format was created inside a helper function and not where it was used.
> This is confusing and makes the code harder to read, fix this.
> 
> This cleanup is made possible due to refactoring elsewhere and there is
> no functional change.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 25 +++++++++------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index f809350c514c337c..9a9b89c0dc0b3be4 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -181,8 +181,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 *src_rect,
> -			   struct v4l2_rect *compose)
> +			   struct v4l2_rect *src_rect)
>  {
>  	struct v4l2_subdev *sd = vin_to_source(vin);
>  	struct v4l2_subdev_pad_config *pad_cfg;
> @@ -229,13 +228,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);
>  
> @@ -259,28 +251,33 @@ 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 src_rect, compose;
> +	struct v4l2_rect fmt_rect, src_rect;

I would rename fmt_rect to something more descriptive. Maybe full_rect,
compose_bounds, ... ? Apart from that,

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

>  	int ret;
>  
>  	if (vb2_is_busy(&vin->queue))
>  		return -EBUSY;
>  
>  	ret = rvin_try_format(vin, V4L2_SUBDEV_FORMAT_ACTIVE, &f->fmt.pix,
> -			      &src_rect, &compose);
> +			      &src_rect);
>  	if (ret)
>  		return ret;
>  
>  	vin->format = f->fmt.pix;
> +
> +	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, &compose);
> +	v4l2_rect_map_inside(&vin->compose, &fmt_rect);
>  	vin->src_rect = src_rect;
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart