[PATCH] drm/imx: ipuv3-plane: Fix overlay plane width

[PATCH] drm/imx: ipuv3-plane: Fix overlay plane width

[Philipp Zabel]

ipu_src_rect_width() was introduced to support odd screen resolutions
such as 1366x768 by internally rounding up primary plane width to a
multiple of 8 and compensating with reduced horizontal blanking.
This also caused overlay plane width to be rounded up, which was not
intended. Fix overlay plane width by limiting the rounding up to the
primary plane.

drm_rect_width(&new_state->src) >> 16 is the same value as
drm_rect_width(dst) because there is no plane scaling support.

Fixes: 94dfec48fca7 ("drm/imx: Add 8 pixel alignment fix")
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index dba4f7d81d69..80142d9a4a55 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -614,6 +614,11 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 		break;
 	}
 
+	if (ipu_plane->dp_flow == IPU_DP_FLOW_SYNC_BG)
+		width = ipu_src_rect_width(new_state);
+	else
+		width = drm_rect_width(&new_state->src) >> 16;
+
 	eba = drm_plane_state_to_eba(new_state, 0);
 
 	/*
@@ -622,8 +627,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 	 */
 	if (ipu_state->use_pre) {
 		axi_id = ipu_chan_assign_axi_id(ipu_plane->dma);
-		ipu_prg_channel_configure(ipu_plane->ipu_ch, axi_id,
-					  ipu_src_rect_width(new_state),
+		ipu_prg_channel_configure(ipu_plane->ipu_ch, axi_id, width,
 					  drm_rect_height(&new_state->src) >> 16,
 					  fb->pitches[0], fb->format->format,
 					  fb->modifier, &eba);
@@ -678,9 +682,8 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 		break;
 	}
 
-	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, ALIGN(drm_rect_width(dst), 8));
+	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, width);
 
-	width = ipu_src_rect_width(new_state);
 	height = drm_rect_height(&new_state->src) >> 16;
 	info = drm_format_info(fb->format->format);
 	ipu_calculate_bursts(width, info->cpp[0], fb->pitches[0],
@@ -744,8 +747,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 		ipu_cpmem_set_burstsize(ipu_plane->ipu_ch, 16);
 
 		ipu_cpmem_zero(ipu_plane->alpha_ch);
-		ipu_cpmem_set_resolution(ipu_plane->alpha_ch,
-					 ipu_src_rect_width(new_state),
+		ipu_cpmem_set_resolution(ipu_plane->alpha_ch, width,
 					 drm_rect_height(&new_state->src) >> 16);
 		ipu_cpmem_set_format_passthrough(ipu_plane->alpha_ch, 8);
 		ipu_cpmem_set_high_priority(ipu_plane->alpha_ch);

base-commit: 9abf2313adc1ca1b6180c508c25f22f9395cc780
-- 
2.30.2

Re: [PATCH] drm/imx: ipuv3-plane: Fix overlay plane width

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3087 bytes --]

Hi,

On Tue, Nov 08, 2022 at 03:14:20PM +0100, Philipp Zabel wrote:
> ipu_src_rect_width() was introduced to support odd screen resolutions
> such as 1366x768 by internally rounding up primary plane width to a
> multiple of 8 and compensating with reduced horizontal blanking.
> This also caused overlay plane width to be rounded up, which was not
> intended. Fix overlay plane width by limiting the rounding up to the
> primary plane.
> 
> drm_rect_width(&new_state->src) >> 16 is the same value as
> drm_rect_width(dst) because there is no plane scaling support.
> 
> Fixes: 94dfec48fca7 ("drm/imx: Add 8 pixel alignment fix")
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---

Looks sensible, but I no longer have access to the affected
hardware. Maybe Ian or Kitty (both added to Cc) can give it
a test on arch/arm/boot/dts/imx6dl-b155v2.dts

-- Sebastian

>  drivers/gpu/drm/imx/ipuv3-plane.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index dba4f7d81d69..80142d9a4a55 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -614,6 +614,11 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>  		break;
>  	}
>  
> +	if (ipu_plane->dp_flow == IPU_DP_FLOW_SYNC_BG)
> +		width = ipu_src_rect_width(new_state);
> +	else
> +		width = drm_rect_width(&new_state->src) >> 16;
> +
>  	eba = drm_plane_state_to_eba(new_state, 0);
>  
>  	/*
> @@ -622,8 +627,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>  	 */
>  	if (ipu_state->use_pre) {
>  		axi_id = ipu_chan_assign_axi_id(ipu_plane->dma);
> -		ipu_prg_channel_configure(ipu_plane->ipu_ch, axi_id,
> -					  ipu_src_rect_width(new_state),
> +		ipu_prg_channel_configure(ipu_plane->ipu_ch, axi_id, width,
>  					  drm_rect_height(&new_state->src) >> 16,
>  					  fb->pitches[0], fb->format->format,
>  					  fb->modifier, &eba);
> @@ -678,9 +682,8 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>  		break;
>  	}
>  
> -	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, ALIGN(drm_rect_width(dst), 8));
> +	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, width);
>  
> -	width = ipu_src_rect_width(new_state);
>  	height = drm_rect_height(&new_state->src) >> 16;
>  	info = drm_format_info(fb->format->format);
>  	ipu_calculate_bursts(width, info->cpp[0], fb->pitches[0],
> @@ -744,8 +747,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>  		ipu_cpmem_set_burstsize(ipu_plane->ipu_ch, 16);
>  
>  		ipu_cpmem_zero(ipu_plane->alpha_ch);
> -		ipu_cpmem_set_resolution(ipu_plane->alpha_ch,
> -					 ipu_src_rect_width(new_state),
> +		ipu_cpmem_set_resolution(ipu_plane->alpha_ch, width,
>  					 drm_rect_height(&new_state->src) >> 16);
>  		ipu_cpmem_set_format_passthrough(ipu_plane->alpha_ch, 8);
>  		ipu_cpmem_set_high_priority(ipu_plane->alpha_ch);
> 
> base-commit: 9abf2313adc1ca1b6180c508c25f22f9395cc780
> -- 
> 2.30.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm/imx: ipuv3-plane: Fix overlay plane width

[Lucas Stach]

Am Dienstag, dem 08.11.2022 um 15:14 +0100 schrieb Philipp Zabel:
> ipu_src_rect_width() was introduced to support odd screen resolutions
> such as 1366x768 by internally rounding up primary plane width to a
> multiple of 8 and compensating with reduced horizontal blanking.
> This also caused overlay plane width to be rounded up, which was not
> intended. Fix overlay plane width by limiting the rounding up to the
> primary plane.
> 
> drm_rect_width(&new_state->src) >> 16 is the same value as
> drm_rect_width(dst) because there is no plane scaling support.

Heh, I stumbled at exactly this point while reading the code changes
and was about to suggest it be added to the changelog until I realized
that it's already here. :)
> 
> Fixes: 94dfec48fca7 ("drm/imx: Add 8 pixel alignment fix")
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

One note below.

> ---
>  drivers/gpu/drm/imx/ipuv3-plane.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index dba4f7d81d69..80142d9a4a55 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -614,6 +614,11 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>  		break;
>  	}
>  
> +	if (ipu_plane->dp_flow == IPU_DP_FLOW_SYNC_BG)
> +		width = ipu_src_rect_width(new_state);
> +	else
> +		width = drm_rect_width(&new_state->src) >> 16;
> +
>  	eba = drm_plane_state_to_eba(new_state, 0);
>  
>  	/*
> @@ -622,8 +627,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>  	 */
>  	if (ipu_state->use_pre) {
>  		axi_id = ipu_chan_assign_axi_id(ipu_plane->dma);
> -		ipu_prg_channel_configure(ipu_plane->ipu_ch, axi_id,
> -					  ipu_src_rect_width(new_state),
> +		ipu_prg_channel_configure(ipu_plane->ipu_ch, axi_id, width,
>  					  drm_rect_height(&new_state->src) >> 16,
>  					  fb->pitches[0], fb->format->format,
>  					  fb->modifier, &eba);
> @@ -678,9 +682,8 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>  		break;
>  	}
>  
> -	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, ALIGN(drm_rect_width(dst), 8));
> +	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, width);
>  
> -	width = ipu_src_rect_width(new_state);
>  	height = drm_rect_height(&new_state->src) >> 16;

There's a opportunity for a follow-up cleanup here:
"drm_rect_height(&new_state->src) >> 16" is used multiple times in this
function, which could be replaced by using this local height variable
instead.

>  	info = drm_format_info(fb->format->format);
>  	ipu_calculate_bursts(width, info->cpp[0], fb->pitches[0],
> @@ -744,8 +747,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>  		ipu_cpmem_set_burstsize(ipu_plane->ipu_ch, 16);
>  
>  		ipu_cpmem_zero(ipu_plane->alpha_ch);
> -		ipu_cpmem_set_resolution(ipu_plane->alpha_ch,
> -					 ipu_src_rect_width(new_state),
> +		ipu_cpmem_set_resolution(ipu_plane->alpha_ch, width,
>  					 drm_rect_height(&new_state->src) >> 16);
>  		ipu_cpmem_set_format_passthrough(ipu_plane->alpha_ch, 8);
>  		ipu_cpmem_set_high_priority(ipu_plane->alpha_ch);
> 
> base-commit: 9abf2313adc1ca1b6180c508c25f22f9395cc780

Re: [PATCH] drm/imx: ipuv3-plane: Fix overlay plane width

[Philipp Zabel]

On Fr, 2022-12-02 at 16:43 +0100, Lucas Stach wrote:
> Am Dienstag, dem 08.11.2022 um 15:14 +0100 schrieb Philipp Zabel:
> > ipu_src_rect_width() was introduced to support odd screen resolutions
> > such as 1366x768 by internally rounding up primary plane width to a
> > multiple of 8 and compensating with reduced horizontal blanking.
> > This also caused overlay plane width to be rounded up, which was not
> > intended. Fix overlay plane width by limiting the rounding up to the
> > primary plane.
> > 
> > drm_rect_width(&new_state->src) >> 16 is the same value as
> > drm_rect_width(dst) because there is no plane scaling support.
> 
> Heh, I stumbled at exactly this point while reading the code changes
> and was about to suggest it be added to the changelog until I realized
> that it's already here. :)
> > 
> > Fixes: 94dfec48fca7 ("drm/imx: Add 8 pixel alignment fix")
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

Thank you, applied to drm-misc-next.

[...]
> There's a opportunity for a follow-up cleanup here:
> "drm_rect_height(&new_state->src) >> 16" is used multiple times in this
> function, which could be replaced by using this local height variable
> instead.

Will create a follow-up patch.

regards
Philipp

Re: EXT: Re: [PATCH] drm/imx: ipuv3-plane: Fix overlay plane width

[Ian Ray]

On Tue, Nov 08, 2022 at 03:49:55PM +0100, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Nov 08, 2022 at 03:14:20PM +0100, Philipp Zabel wrote:
> > ipu_src_rect_width() was introduced to support odd screen resolutions
> > such as 1366x768 by internally rounding up primary plane width to a
> > multiple of 8 and compensating with reduced horizontal blanking.
> > This also caused overlay plane width to be rounded up, which was not
> > intended. Fix overlay plane width by limiting the rounding up to the
> > primary plane.
> > 
> > drm_rect_width(&new_state->src) >> 16 is the same value as
> > drm_rect_width(dst) because there is no plane scaling support.
> > 
> > Fixes: 94dfec48fca7 ("drm/imx: Add 8 pixel alignment fix")
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Sorry for the delay in testing, and thank you for the patch!


Tested-by: Ian Ray <ian.ray@ge.com>


> > ---
> 
> Looks sensible, but I no longer have access to the affected
> hardware. Maybe Ian or Kitty (both added to Cc) can give it
> a test on arch/arm/boot/dts/imx6dl-b155v2.dts
> 
> -- Sebastian
> 
> >  drivers/gpu/drm/imx/ipuv3-plane.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> > index dba4f7d81d69..80142d9a4a55 100644
> > --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> > @@ -614,6 +614,11 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
> >  		break;
> >  	}
> >  
> > +	if (ipu_plane->dp_flow == IPU_DP_FLOW_SYNC_BG)
> > +		width = ipu_src_rect_width(new_state);
> > +	else
> > +		width = drm_rect_width(&new_state->src) >> 16;
> > +
> >  	eba = drm_plane_state_to_eba(new_state, 0);
> >  
> >  	/*
> > @@ -622,8 +627,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
> >  	 */
> >  	if (ipu_state->use_pre) {
> >  		axi_id = ipu_chan_assign_axi_id(ipu_plane->dma);
> > -		ipu_prg_channel_configure(ipu_plane->ipu_ch, axi_id,
> > -					  ipu_src_rect_width(new_state),
> > +		ipu_prg_channel_configure(ipu_plane->ipu_ch, axi_id, width,
> >  					  drm_rect_height(&new_state->src) >> 16,
> >  					  fb->pitches[0], fb->format->format,
> >  					  fb->modifier, &eba);
> > @@ -678,9 +682,8 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
> >  		break;
> >  	}
> >  
> > -	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, ALIGN(drm_rect_width(dst), 8));
> > +	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, width);
> >  
> > -	width = ipu_src_rect_width(new_state);
> >  	height = drm_rect_height(&new_state->src) >> 16;
> >  	info = drm_format_info(fb->format->format);
> >  	ipu_calculate_bursts(width, info->cpp[0], fb->pitches[0],
> > @@ -744,8 +747,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
> >  		ipu_cpmem_set_burstsize(ipu_plane->ipu_ch, 16);
> >  
> >  		ipu_cpmem_zero(ipu_plane->alpha_ch);
> > -		ipu_cpmem_set_resolution(ipu_plane->alpha_ch,
> > -					 ipu_src_rect_width(new_state),
> > +		ipu_cpmem_set_resolution(ipu_plane->alpha_ch, width,
> >  					 drm_rect_height(&new_state->src) >> 16);
> >  		ipu_cpmem_set_format_passthrough(ipu_plane->alpha_ch, 8);
> >  		ipu_cpmem_set_high_priority(ipu_plane->alpha_ch);
> > 
> > base-commit: 9abf2313adc1ca1b6180c508c25f22f9395cc780
> > -- 
> > 2.30.2
> >