Re: [PATCH v4 2/7] media: vsp1: wpf: Fix partition configuration for display pipelines

From: Kieran Bingham <kieran.bingham@ideasonboard.com>
To: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
	linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v4 2/7] media: vsp1: wpf: Fix partition configuration for display pipelines
Date: Sun, 17 Feb 2019 20:16:27 +0000	[thread overview]
Message-ID: <5f5c255c-1071-78e9-e64f-8dcdfa20ea80@ideasonboard.com> (raw)
In-Reply-To: <20190217024852.23328-3-laurent.pinchart+renesas@ideasonboard.com>

Hi Laurent,

On 17/02/2019 02:48, Laurent Pinchart wrote:
> The WPF accesses partition configuration from pipe->partition in the
> partition configuration that is not used for display pipelines.

That sentence is hard to read...

> Writeback support will require full configuration of the WPF while not
> providing a valid pipe->partition. Rework the configuration code to fall
> back to the full image width in that case, as is already done for the
> part of the configuration currently relevant for display pipelines.
> 

Ah yes - this is probably a better route than allocating a table for a
single partition (which is what I had locally).

> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

I like that this change also simplifies the flip/rotate handling code to
make that easier to read.

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

> ---
>  drivers/media/platform/vsp1/vsp1_wpf.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
> index 32bb207b2007..a07c5944b598 100644
> --- a/drivers/media/platform/vsp1/vsp1_wpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_wpf.c
> @@ -362,6 +362,7 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
>  	const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
>  	unsigned int width;
>  	unsigned int height;
> +	unsigned int left;
>  	unsigned int offset;
>  	unsigned int flip;
>  	unsigned int i;
> @@ -371,13 +372,16 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
>  						 RWPF_PAD_SINK);
>  	width = sink_format->width;
>  	height = sink_format->height;
> +	left = 0;
>  
>  	/*
>  	 * Cropping. The partition algorithm can split the image into
>  	 * multiple slices.
>  	 */
> -	if (pipe->partitions > 1)
> +	if (pipe->partitions > 1) {
>  		width = pipe->partition->wpf.width;
> +		left = pipe->partition->wpf.left;
> +	}
>  
>  	vsp1_wpf_write(wpf, dlb, VI6_WPF_HSZCLIP, VI6_WPF_SZCLIP_EN |
>  		       (0 << VI6_WPF_SZCLIP_OFST_SHIFT) |
> @@ -408,13 +412,11 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
>  	flip = wpf->flip.active;
>  
>  	if (flip & BIT(WPF_CTRL_HFLIP) && !wpf->flip.rotate)
> -		offset = format->width - pipe->partition->wpf.left
> -			- pipe->partition->wpf.width;
> +		offset = format->width - left - width;
>  	else if (flip & BIT(WPF_CTRL_VFLIP) && wpf->flip.rotate)
> -		offset = format->height - pipe->partition->wpf.left
> -			- pipe->partition->wpf.width;
> +		offset = format->height - left - width;
>  	else
> -		offset = pipe->partition->wpf.left;
> +		offset = left;

This hunk looks a lot simpler :)

>  
>  	for (i = 0; i < format->num_planes; ++i) {
>  		unsigned int hsub = i > 0 ? fmtinfo->hsub : 1;
> @@ -436,7 +438,7 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
>  		 * image height.
>  		 */
>  		if (wpf->flip.rotate)
> -			height = pipe->partition->wpf.width;
> +			height = width;
>  		else
>  			height = format->height;
>  
> 

-- 
Regards
--
Kieran