diff mbox series

drm/imx: ipuv3-plane: Fix overlay plane width

Message ID 20221108141420.176696-1-p.zabel@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series drm/imx: ipuv3-plane: Fix overlay plane width | expand

Commit Message

Philipp Zabel Nov. 8, 2022, 2:14 p.m. UTC 
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(-)


base-commit: 9abf2313adc1ca1b6180c508c25f22f9395cc780

Comments

Sebastian Reichel Nov. 8, 2022, 2:49 p.m. UTC | #1 
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
>
Lucas Stach Dec. 2, 2022, 3:43 p.m. UTC | #2 
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
Philipp Zabel Dec. 16, 2022, 6:08 p.m. UTC | #3 
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
Ian Ray Dec. 20, 2022, 8:33 a.m. UTC | #4 
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
> >
diff mbox series

Patch

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);