All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 2/3] mach64: fix image corruption due to reading accelerator registers
@ 2018-08-25 19:51 ` Mikulas Patocka
  0 siblings, 0 replies; 7+ messages in thread
From: Mikulas Patocka @ 2018-08-25 19:51 UTC (permalink / raw)
  To: Ville Syrjälä, Bartlomiej Zolnierkiewicz; +Cc: linux-fbdev, dri-devel

Reading the registers without waiting for engine idle returns
unpredictable values. These unpredictable values result in display
corruption - if atyfb_imageblit reads the content of DP_PIX_WIDTH with the
bit DP_HOST_TRIPLE_EN set (from previous invocation), the driver would
never ever clear the bit, resulting in display corruption.

We don't want to wait for idle because it would degrade performance, so
this patch modifies the driver so that it never reads accelerator
registers.

HOST_CNTL doesn't have to be read, we can just write it with
HOST_BYTE_ALIGN because no other part of the driver cares if
HOST_BYTE_ALIGN is set.

DP_PIX_WIDTH is written in the functions atyfb_copyarea and atyfb_fillrect
with the default value and in atyfb_imageblit with the value set according
to the source image data.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org

---
 drivers/video/fbdev/aty/mach64_accel.c |   22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

Index: linux-stable/drivers/video/fbdev/aty/mach64_accel.c
=================================--- linux-stable.orig/drivers/video/fbdev/aty/mach64_accel.c	2018-08-24 19:51:34.000000000 +0200
+++ linux-stable/drivers/video/fbdev/aty/mach64_accel.c	2018-08-24 20:28:55.000000000 +0200
@@ -127,7 +127,7 @@ void aty_init_engine(struct atyfb_par *p
 
 	/* set host attributes */
 	wait_for_fifo(13, par);
-	aty_st_le32(HOST_CNTL, 0, par);
+	aty_st_le32(HOST_CNTL, HOST_BYTE_ALIGN, par);
 
 	/* set pattern attributes */
 	aty_st_le32(PAT_REG0, 0, par);
@@ -233,7 +233,8 @@ void atyfb_copyarea(struct fb_info *info
 		rotation = rotation24bpp(dx, direction);
 	}
 
-	wait_for_fifo(4, par);
+	wait_for_fifo(5, par);
+	aty_st_le32(DP_PIX_WIDTH, par->crtc.dp_pix_width, par);
 	aty_st_le32(DP_SRC, FRGD_SRC_BLIT, par);
 	aty_st_le32(SRC_Y_X, (sx << 16) | sy, par);
 	aty_st_le32(SRC_HEIGHT1_WIDTH1, (width << 16) | area->height, par);
@@ -269,7 +270,8 @@ void atyfb_fillrect(struct fb_info *info
 		rotation = rotation24bpp(dx, DST_X_LEFT_TO_RIGHT);
 	}
 
-	wait_for_fifo(3, par);
+	wait_for_fifo(4, par);
+	aty_st_le32(DP_PIX_WIDTH, par->crtc.dp_pix_width, par);
 	aty_st_le32(DP_FRGD_CLR, color, par);
 	aty_st_le32(DP_SRC,
 		    BKGD_SRC_BKGD_CLR | FRGD_SRC_FRGD_CLR | MONO_SRC_ONE,
@@ -284,7 +286,7 @@ void atyfb_imageblit(struct fb_info *inf
 {
 	struct atyfb_par *par = (struct atyfb_par *) info->par;
 	u32 src_bytes, dx = image->dx, dy = image->dy, width = image->width;
-	u32 pix_width_save, pix_width, host_cntl, rotation = 0, src, mix;
+	u32 pix_width, rotation = 0, src, mix;
 
 	if (par->asleep)
 		return;
@@ -296,8 +298,7 @@ void atyfb_imageblit(struct fb_info *inf
 		return;
 	}
 
-	pix_width = pix_width_save = aty_ld_le32(DP_PIX_WIDTH, par);
-	host_cntl = aty_ld_le32(HOST_CNTL, par) | HOST_BYTE_ALIGN;
+	pix_width = par->crtc.dp_pix_width;
 
 	switch (image->depth) {
 	case 1:
@@ -370,12 +371,11 @@ void atyfb_imageblit(struct fb_info *inf
 		mix = FRGD_MIX_D_XOR_S | BKGD_MIX_D;
 	}
 
-	wait_for_fifo(6, par);
-	aty_st_le32(DP_WRITE_MASK, 0xFFFFFFFF, par);
+	wait_for_fifo(5, par);
 	aty_st_le32(DP_PIX_WIDTH, pix_width, par);
 	aty_st_le32(DP_MIX, mix, par);
 	aty_st_le32(DP_SRC, src, par);
-	aty_st_le32(HOST_CNTL, host_cntl, par);
+	aty_st_le32(HOST_CNTL, HOST_BYTE_ALIGN, par);
 	aty_st_le32(DST_CNTL, DST_Y_TOP_TO_BOTTOM | DST_X_LEFT_TO_RIGHT | rotation, par);
 
 	draw_rect(dx, dy, width, image->height, par);
@@ -424,8 +424,4 @@ void atyfb_imageblit(struct fb_info *inf
 			aty_st_le32(HOST_DATA0, get_unaligned_le32(pbitmap), par);
 		}
 	}
-
-	/* restore pix_width */
-	wait_for_fifo(1, par);
-	aty_st_le32(DP_PIX_WIDTH, pix_width_save, par);
 }

^ permalink raw reply	[flat|nested] 7+ messages in thread
* [PATCH 2/3] mach64: fix image corruption due to reading accelerator registers
@ 2018-08-25 19:51 ` Mikulas Patocka
  0 siblings, 0 replies; 7+ messages in thread
From: Mikulas Patocka @ 2018-08-25 19:51 UTC (permalink / raw)
  To: Ville Syrjälä, Bartlomiej Zolnierkiewicz; +Cc: linux-fbdev, dri-devel

Reading the registers without waiting for engine idle returns
unpredictable values. These unpredictable values result in display
corruption - if atyfb_imageblit reads the content of DP_PIX_WIDTH with the
bit DP_HOST_TRIPLE_EN set (from previous invocation), the driver would
never ever clear the bit, resulting in display corruption.

We don't want to wait for idle because it would degrade performance, so
this patch modifies the driver so that it never reads accelerator
registers.

HOST_CNTL doesn't have to be read, we can just write it with
HOST_BYTE_ALIGN because no other part of the driver cares if
HOST_BYTE_ALIGN is set.

DP_PIX_WIDTH is written in the functions atyfb_copyarea and atyfb_fillrect
with the default value and in atyfb_imageblit with the value set according
to the source image data.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org

---
 drivers/video/fbdev/aty/mach64_accel.c |   22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

Index: linux-stable/drivers/video/fbdev/aty/mach64_accel.c
===================================================================
--- linux-stable.orig/drivers/video/fbdev/aty/mach64_accel.c	2018-08-24 19:51:34.000000000 +0200
+++ linux-stable/drivers/video/fbdev/aty/mach64_accel.c	2018-08-24 20:28:55.000000000 +0200
@@ -127,7 +127,7 @@ void aty_init_engine(struct atyfb_par *p
 
 	/* set host attributes */
 	wait_for_fifo(13, par);
-	aty_st_le32(HOST_CNTL, 0, par);
+	aty_st_le32(HOST_CNTL, HOST_BYTE_ALIGN, par);
 
 	/* set pattern attributes */
 	aty_st_le32(PAT_REG0, 0, par);
@@ -233,7 +233,8 @@ void atyfb_copyarea(struct fb_info *info
 		rotation = rotation24bpp(dx, direction);
 	}
 
-	wait_for_fifo(4, par);
+	wait_for_fifo(5, par);
+	aty_st_le32(DP_PIX_WIDTH, par->crtc.dp_pix_width, par);
 	aty_st_le32(DP_SRC, FRGD_SRC_BLIT, par);
 	aty_st_le32(SRC_Y_X, (sx << 16) | sy, par);
 	aty_st_le32(SRC_HEIGHT1_WIDTH1, (width << 16) | area->height, par);
@@ -269,7 +270,8 @@ void atyfb_fillrect(struct fb_info *info
 		rotation = rotation24bpp(dx, DST_X_LEFT_TO_RIGHT);
 	}
 
-	wait_for_fifo(3, par);
+	wait_for_fifo(4, par);
+	aty_st_le32(DP_PIX_WIDTH, par->crtc.dp_pix_width, par);
 	aty_st_le32(DP_FRGD_CLR, color, par);
 	aty_st_le32(DP_SRC,
 		    BKGD_SRC_BKGD_CLR | FRGD_SRC_FRGD_CLR | MONO_SRC_ONE,
@@ -284,7 +286,7 @@ void atyfb_imageblit(struct fb_info *inf
 {
 	struct atyfb_par *par = (struct atyfb_par *) info->par;
 	u32 src_bytes, dx = image->dx, dy = image->dy, width = image->width;
-	u32 pix_width_save, pix_width, host_cntl, rotation = 0, src, mix;
+	u32 pix_width, rotation = 0, src, mix;
 
 	if (par->asleep)
 		return;
@@ -296,8 +298,7 @@ void atyfb_imageblit(struct fb_info *inf
 		return;
 	}
 
-	pix_width = pix_width_save = aty_ld_le32(DP_PIX_WIDTH, par);
-	host_cntl = aty_ld_le32(HOST_CNTL, par) | HOST_BYTE_ALIGN;
+	pix_width = par->crtc.dp_pix_width;
 
 	switch (image->depth) {
 	case 1:
@@ -370,12 +371,11 @@ void atyfb_imageblit(struct fb_info *inf
 		mix = FRGD_MIX_D_XOR_S | BKGD_MIX_D;
 	}
 
-	wait_for_fifo(6, par);
-	aty_st_le32(DP_WRITE_MASK, 0xFFFFFFFF, par);
+	wait_for_fifo(5, par);
 	aty_st_le32(DP_PIX_WIDTH, pix_width, par);
 	aty_st_le32(DP_MIX, mix, par);
 	aty_st_le32(DP_SRC, src, par);
-	aty_st_le32(HOST_CNTL, host_cntl, par);
+	aty_st_le32(HOST_CNTL, HOST_BYTE_ALIGN, par);
 	aty_st_le32(DST_CNTL, DST_Y_TOP_TO_BOTTOM | DST_X_LEFT_TO_RIGHT | rotation, par);
 
 	draw_rect(dx, dy, width, image->height, par);
@@ -424,8 +424,4 @@ void atyfb_imageblit(struct fb_info *inf
 			aty_st_le32(HOST_DATA0, get_unaligned_le32(pbitmap), par);
 		}
 	}
-
-	/* restore pix_width */
-	wait_for_fifo(1, par);
-	aty_st_le32(DP_PIX_WIDTH, pix_width_save, par);
 }
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] mach64: fix image corruption due to reading accelerator registers
  2018-08-25 19:51 ` Mikulas Patocka
@ 2018-08-27 12:55   ` Ville Syrjälä
  -1 siblings, 0 replies; 7+ messages in thread
From: Ville Syrjälä @ 2018-08-27 12:55 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: linux-fbdev, dri-devel, Bartlomiej Zolnierkiewicz

On Sat, Aug 25, 2018 at 03:51:52PM -0400, Mikulas Patocka wrote:
> Reading the registers without waiting for engine idle returns
> unpredictable values. These unpredictable values result in display
> corruption - if atyfb_imageblit reads the content of DP_PIX_WIDTH with the
> bit DP_HOST_TRIPLE_EN set (from previous invocation), the driver would
> never ever clear the bit, resulting in display corruption.
> 
> We don't want to wait for idle because it would degrade performance, so
> this patch modifies the driver so that it never reads accelerator
> registers.
> 
> HOST_CNTL doesn't have to be read, we can just write it with
> HOST_BYTE_ALIGN because no other part of the driver cares if
> HOST_BYTE_ALIGN is set.
> 
> DP_PIX_WIDTH is written in the functions atyfb_copyarea and atyfb_fillrect
> with the default value and in atyfb_imageblit with the value set according
> to the source image data.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@vger.kernel.org
> 
> ---
>  drivers/video/fbdev/aty/mach64_accel.c |   22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> Index: linux-stable/drivers/video/fbdev/aty/mach64_accel.c
> =================================> --- linux-stable.orig/drivers/video/fbdev/aty/mach64_accel.c	2018-08-24 19:51:34.000000000 +0200
> +++ linux-stable/drivers/video/fbdev/aty/mach64_accel.c	2018-08-24 20:28:55.000000000 +0200
> @@ -127,7 +127,7 @@ void aty_init_engine(struct atyfb_par *p
>  
>  	/* set host attributes */
>  	wait_for_fifo(13, par);
> -	aty_st_le32(HOST_CNTL, 0, par);
> +	aty_st_le32(HOST_CNTL, HOST_BYTE_ALIGN, par);
>  
>  	/* set pattern attributes */
>  	aty_st_le32(PAT_REG0, 0, par);
> @@ -233,7 +233,8 @@ void atyfb_copyarea(struct fb_info *info
>  		rotation = rotation24bpp(dx, direction);
>  	}
>  
> -	wait_for_fifo(4, par);
> +	wait_for_fifo(5, par);
> +	aty_st_le32(DP_PIX_WIDTH, par->crtc.dp_pix_width, par);
>  	aty_st_le32(DP_SRC, FRGD_SRC_BLIT, par);
>  	aty_st_le32(SRC_Y_X, (sx << 16) | sy, par);
>  	aty_st_le32(SRC_HEIGHT1_WIDTH1, (width << 16) | area->height, par);
> @@ -269,7 +270,8 @@ void atyfb_fillrect(struct fb_info *info
>  		rotation = rotation24bpp(dx, DST_X_LEFT_TO_RIGHT);
>  	}
>  
> -	wait_for_fifo(3, par);
> +	wait_for_fifo(4, par);
> +	aty_st_le32(DP_PIX_WIDTH, par->crtc.dp_pix_width, par);
>  	aty_st_le32(DP_FRGD_CLR, color, par);
>  	aty_st_le32(DP_SRC,
>  		    BKGD_SRC_BKGD_CLR | FRGD_SRC_FRGD_CLR | MONO_SRC_ONE,
> @@ -284,7 +286,7 @@ void atyfb_imageblit(struct fb_info *inf
>  {
>  	struct atyfb_par *par = (struct atyfb_par *) info->par;
>  	u32 src_bytes, dx = image->dx, dy = image->dy, width = image->width;
> -	u32 pix_width_save, pix_width, host_cntl, rotation = 0, src, mix;
> +	u32 pix_width, rotation = 0, src, mix;
>  
>  	if (par->asleep)
>  		return;
> @@ -296,8 +298,7 @@ void atyfb_imageblit(struct fb_info *inf
>  		return;
>  	}
>  
> -	pix_width = pix_width_save = aty_ld_le32(DP_PIX_WIDTH, par);
> -	host_cntl = aty_ld_le32(HOST_CNTL, par) | HOST_BYTE_ALIGN;
> +	pix_width = par->crtc.dp_pix_width;
>  
>  	switch (image->depth) {
>  	case 1:
> @@ -370,12 +371,11 @@ void atyfb_imageblit(struct fb_info *inf
>  		mix = FRGD_MIX_D_XOR_S | BKGD_MIX_D;
>  	}
>  
> -	wait_for_fifo(6, par);
> -	aty_st_le32(DP_WRITE_MASK, 0xFFFFFFFF, par);

Looks like init_engine() sets this one for us. So dropping should be ok.

> +	wait_for_fifo(5, par);
>  	aty_st_le32(DP_PIX_WIDTH, pix_width, par);
>  	aty_st_le32(DP_MIX, mix, par);
>  	aty_st_le32(DP_SRC, src, par);
> -	aty_st_le32(HOST_CNTL, host_cntl, par);
> +	aty_st_le32(HOST_CNTL, HOST_BYTE_ALIGN, par);

Presumably we could drop this as well since it never changes.

Either way
Reviewed-by: Ville Syrjälä <syrjala@sci.fi>

>  	aty_st_le32(DST_CNTL, DST_Y_TOP_TO_BOTTOM | DST_X_LEFT_TO_RIGHT | rotation, par);
>  
>  	draw_rect(dx, dy, width, image->height, par);
> @@ -424,8 +424,4 @@ void atyfb_imageblit(struct fb_info *inf
>  			aty_st_le32(HOST_DATA0, get_unaligned_le32(pbitmap), par);
>  		}
>  	}
> -
> -	/* restore pix_width */
> -	wait_for_fifo(1, par);
> -	aty_st_le32(DP_PIX_WIDTH, pix_width_save, par);
>  }

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] mach64: fix image corruption due to reading accelerator registers
@ 2018-08-27 12:55   ` Ville Syrjälä
  0 siblings, 0 replies; 7+ messages in thread
From: Ville Syrjälä @ 2018-08-27 12:55 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: linux-fbdev, dri-devel, Bartlomiej Zolnierkiewicz

On Sat, Aug 25, 2018 at 03:51:52PM -0400, Mikulas Patocka wrote:
> Reading the registers without waiting for engine idle returns
> unpredictable values. These unpredictable values result in display
> corruption - if atyfb_imageblit reads the content of DP_PIX_WIDTH with the
> bit DP_HOST_TRIPLE_EN set (from previous invocation), the driver would
> never ever clear the bit, resulting in display corruption.
> 
> We don't want to wait for idle because it would degrade performance, so
> this patch modifies the driver so that it never reads accelerator
> registers.
> 
> HOST_CNTL doesn't have to be read, we can just write it with
> HOST_BYTE_ALIGN because no other part of the driver cares if
> HOST_BYTE_ALIGN is set.
> 
> DP_PIX_WIDTH is written in the functions atyfb_copyarea and atyfb_fillrect
> with the default value and in atyfb_imageblit with the value set according
> to the source image data.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@vger.kernel.org
> 
> ---
>  drivers/video/fbdev/aty/mach64_accel.c |   22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> Index: linux-stable/drivers/video/fbdev/aty/mach64_accel.c
> ===================================================================
> --- linux-stable.orig/drivers/video/fbdev/aty/mach64_accel.c	2018-08-24 19:51:34.000000000 +0200
> +++ linux-stable/drivers/video/fbdev/aty/mach64_accel.c	2018-08-24 20:28:55.000000000 +0200
> @@ -127,7 +127,7 @@ void aty_init_engine(struct atyfb_par *p
>  
>  	/* set host attributes */
>  	wait_for_fifo(13, par);
> -	aty_st_le32(HOST_CNTL, 0, par);
> +	aty_st_le32(HOST_CNTL, HOST_BYTE_ALIGN, par);
>  
>  	/* set pattern attributes */
>  	aty_st_le32(PAT_REG0, 0, par);
> @@ -233,7 +233,8 @@ void atyfb_copyarea(struct fb_info *info
>  		rotation = rotation24bpp(dx, direction);
>  	}
>  
> -	wait_for_fifo(4, par);
> +	wait_for_fifo(5, par);
> +	aty_st_le32(DP_PIX_WIDTH, par->crtc.dp_pix_width, par);
>  	aty_st_le32(DP_SRC, FRGD_SRC_BLIT, par);
>  	aty_st_le32(SRC_Y_X, (sx << 16) | sy, par);
>  	aty_st_le32(SRC_HEIGHT1_WIDTH1, (width << 16) | area->height, par);
> @@ -269,7 +270,8 @@ void atyfb_fillrect(struct fb_info *info
>  		rotation = rotation24bpp(dx, DST_X_LEFT_TO_RIGHT);
>  	}
>  
> -	wait_for_fifo(3, par);
> +	wait_for_fifo(4, par);
> +	aty_st_le32(DP_PIX_WIDTH, par->crtc.dp_pix_width, par);
>  	aty_st_le32(DP_FRGD_CLR, color, par);
>  	aty_st_le32(DP_SRC,
>  		    BKGD_SRC_BKGD_CLR | FRGD_SRC_FRGD_CLR | MONO_SRC_ONE,
> @@ -284,7 +286,7 @@ void atyfb_imageblit(struct fb_info *inf
>  {
>  	struct atyfb_par *par = (struct atyfb_par *) info->par;
>  	u32 src_bytes, dx = image->dx, dy = image->dy, width = image->width;
> -	u32 pix_width_save, pix_width, host_cntl, rotation = 0, src, mix;
> +	u32 pix_width, rotation = 0, src, mix;
>  
>  	if (par->asleep)
>  		return;
> @@ -296,8 +298,7 @@ void atyfb_imageblit(struct fb_info *inf
>  		return;
>  	}
>  
> -	pix_width = pix_width_save = aty_ld_le32(DP_PIX_WIDTH, par);
> -	host_cntl = aty_ld_le32(HOST_CNTL, par) | HOST_BYTE_ALIGN;
> +	pix_width = par->crtc.dp_pix_width;
>  
>  	switch (image->depth) {
>  	case 1:
> @@ -370,12 +371,11 @@ void atyfb_imageblit(struct fb_info *inf
>  		mix = FRGD_MIX_D_XOR_S | BKGD_MIX_D;
>  	}
>  
> -	wait_for_fifo(6, par);
> -	aty_st_le32(DP_WRITE_MASK, 0xFFFFFFFF, par);

Looks like init_engine() sets this one for us. So dropping should be ok.

> +	wait_for_fifo(5, par);
>  	aty_st_le32(DP_PIX_WIDTH, pix_width, par);
>  	aty_st_le32(DP_MIX, mix, par);
>  	aty_st_le32(DP_SRC, src, par);
> -	aty_st_le32(HOST_CNTL, host_cntl, par);
> +	aty_st_le32(HOST_CNTL, HOST_BYTE_ALIGN, par);

Presumably we could drop this as well since it never changes.

Either way
Reviewed-by: Ville Syrjälä <syrjala@sci.fi>

>  	aty_st_le32(DST_CNTL, DST_Y_TOP_TO_BOTTOM | DST_X_LEFT_TO_RIGHT | rotation, par);
>  
>  	draw_rect(dx, dy, width, image->height, par);
> @@ -424,8 +424,4 @@ void atyfb_imageblit(struct fb_info *inf
>  			aty_st_le32(HOST_DATA0, get_unaligned_le32(pbitmap), par);
>  		}
>  	}
> -
> -	/* restore pix_width */
> -	wait_for_fifo(1, par);
> -	aty_st_le32(DP_PIX_WIDTH, pix_width_save, par);
>  }

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] mach64: fix image corruption due to reading accelerator registers
  2018-08-27 12:55   ` Ville Syrjälä
@ 2018-08-28 15:34     ` Mikulas Patocka
  -1 siblings, 0 replies; 7+ messages in thread
From: Mikulas Patocka @ 2018-08-28 15:34 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: linux-fbdev, dri-devel, Bartlomiej Zolnierkiewicz

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4792 bytes --]



On Mon, 27 Aug 2018, Ville Syrjälä wrote:

> On Sat, Aug 25, 2018 at 03:51:52PM -0400, Mikulas Patocka wrote:
> > Reading the registers without waiting for engine idle returns
> > unpredictable values. These unpredictable values result in display
> > corruption - if atyfb_imageblit reads the content of DP_PIX_WIDTH with the
> > bit DP_HOST_TRIPLE_EN set (from previous invocation), the driver would
> > never ever clear the bit, resulting in display corruption.
> > 
> > We don't want to wait for idle because it would degrade performance, so
> > this patch modifies the driver so that it never reads accelerator
> > registers.
> > 
> > HOST_CNTL doesn't have to be read, we can just write it with
> > HOST_BYTE_ALIGN because no other part of the driver cares if
> > HOST_BYTE_ALIGN is set.
> > 
> > DP_PIX_WIDTH is written in the functions atyfb_copyarea and atyfb_fillrect
> > with the default value and in atyfb_imageblit with the value set according
> > to the source image data.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Cc: stable@vger.kernel.org
> > 
> > ---
> >  drivers/video/fbdev/aty/mach64_accel.c |   22 +++++++++-------------
> >  1 file changed, 9 insertions(+), 13 deletions(-)
> > 
> > Index: linux-stable/drivers/video/fbdev/aty/mach64_accel.c
> > ===================================================================
> > --- linux-stable.orig/drivers/video/fbdev/aty/mach64_accel.c	2018-08-24 19:51:34.000000000 +0200
> > +++ linux-stable/drivers/video/fbdev/aty/mach64_accel.c	2018-08-24 20:28:55.000000000 +0200
> > @@ -127,7 +127,7 @@ void aty_init_engine(struct atyfb_par *p
> >  
> >  	/* set host attributes */
> >  	wait_for_fifo(13, par);
> > -	aty_st_le32(HOST_CNTL, 0, par);
> > +	aty_st_le32(HOST_CNTL, HOST_BYTE_ALIGN, par);
> >  
> >  	/* set pattern attributes */
> >  	aty_st_le32(PAT_REG0, 0, par);
> > @@ -233,7 +233,8 @@ void atyfb_copyarea(struct fb_info *info
> >  		rotation = rotation24bpp(dx, direction);
> >  	}
> >  
> > -	wait_for_fifo(4, par);
> > +	wait_for_fifo(5, par);
> > +	aty_st_le32(DP_PIX_WIDTH, par->crtc.dp_pix_width, par);
> >  	aty_st_le32(DP_SRC, FRGD_SRC_BLIT, par);
> >  	aty_st_le32(SRC_Y_X, (sx << 16) | sy, par);
> >  	aty_st_le32(SRC_HEIGHT1_WIDTH1, (width << 16) | area->height, par);
> > @@ -269,7 +270,8 @@ void atyfb_fillrect(struct fb_info *info
> >  		rotation = rotation24bpp(dx, DST_X_LEFT_TO_RIGHT);
> >  	}
> >  
> > -	wait_for_fifo(3, par);
> > +	wait_for_fifo(4, par);
> > +	aty_st_le32(DP_PIX_WIDTH, par->crtc.dp_pix_width, par);
> >  	aty_st_le32(DP_FRGD_CLR, color, par);
> >  	aty_st_le32(DP_SRC,
> >  		    BKGD_SRC_BKGD_CLR | FRGD_SRC_FRGD_CLR | MONO_SRC_ONE,
> > @@ -284,7 +286,7 @@ void atyfb_imageblit(struct fb_info *inf
> >  {
> >  	struct atyfb_par *par = (struct atyfb_par *) info->par;
> >  	u32 src_bytes, dx = image->dx, dy = image->dy, width = image->width;
> > -	u32 pix_width_save, pix_width, host_cntl, rotation = 0, src, mix;
> > +	u32 pix_width, rotation = 0, src, mix;
> >  
> >  	if (par->asleep)
> >  		return;
> > @@ -296,8 +298,7 @@ void atyfb_imageblit(struct fb_info *inf
> >  		return;
> >  	}
> >  
> > -	pix_width = pix_width_save = aty_ld_le32(DP_PIX_WIDTH, par);
> > -	host_cntl = aty_ld_le32(HOST_CNTL, par) | HOST_BYTE_ALIGN;
> > +	pix_width = par->crtc.dp_pix_width;
> >  
> >  	switch (image->depth) {
> >  	case 1:
> > @@ -370,12 +371,11 @@ void atyfb_imageblit(struct fb_info *inf
> >  		mix = FRGD_MIX_D_XOR_S | BKGD_MIX_D;
> >  	}
> >  
> > -	wait_for_fifo(6, par);
> > -	aty_st_le32(DP_WRITE_MASK, 0xFFFFFFFF, par);
> 
> Looks like init_engine() sets this one for us. So dropping should be ok.
> 
> > +	wait_for_fifo(5, par);
> >  	aty_st_le32(DP_PIX_WIDTH, pix_width, par);
> >  	aty_st_le32(DP_MIX, mix, par);
> >  	aty_st_le32(DP_SRC, src, par);
> > -	aty_st_le32(HOST_CNTL, host_cntl, par);
> > +	aty_st_le32(HOST_CNTL, HOST_BYTE_ALIGN, par);
> 
> Presumably we could drop this as well since it never changes.

I tried to drop this HOST_CNTL write and it didn't work. It seems that the 
register is written implicitly by some of the other commands.

Mikulas

> Either way
> Reviewed-by: Ville Syrjälä <syrjala@sci.fi>
> 
> >  	aty_st_le32(DST_CNTL, DST_Y_TOP_TO_BOTTOM | DST_X_LEFT_TO_RIGHT | rotation, par);
> >  
> >  	draw_rect(dx, dy, width, image->height, par);
> > @@ -424,8 +424,4 @@ void atyfb_imageblit(struct fb_info *inf
> >  			aty_st_le32(HOST_DATA0, get_unaligned_le32(pbitmap), par);
> >  		}
> >  	}
> > -
> > -	/* restore pix_width */
> > -	wait_for_fifo(1, par);
> > -	aty_st_le32(DP_PIX_WIDTH, pix_width_save, par);
> >  }
> 
> -- 
> Ville Syrjälä
> syrjala@sci.fi
> http://www.sci.fi/~syrjala/
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] mach64: fix image corruption due to reading accelerator registers
@ 2018-08-28 15:34     ` Mikulas Patocka
  0 siblings, 0 replies; 7+ messages in thread
From: Mikulas Patocka @ 2018-08-28 15:34 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: linux-fbdev, dri-devel, Bartlomiej Zolnierkiewicz

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4667 bytes --]



On Mon, 27 Aug 2018, Ville Syrjälä wrote:

> On Sat, Aug 25, 2018 at 03:51:52PM -0400, Mikulas Patocka wrote:
> > Reading the registers without waiting for engine idle returns
> > unpredictable values. These unpredictable values result in display
> > corruption - if atyfb_imageblit reads the content of DP_PIX_WIDTH with the
> > bit DP_HOST_TRIPLE_EN set (from previous invocation), the driver would
> > never ever clear the bit, resulting in display corruption.
> > 
> > We don't want to wait for idle because it would degrade performance, so
> > this patch modifies the driver so that it never reads accelerator
> > registers.
> > 
> > HOST_CNTL doesn't have to be read, we can just write it with
> > HOST_BYTE_ALIGN because no other part of the driver cares if
> > HOST_BYTE_ALIGN is set.
> > 
> > DP_PIX_WIDTH is written in the functions atyfb_copyarea and atyfb_fillrect
> > with the default value and in atyfb_imageblit with the value set according
> > to the source image data.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Cc: stable@vger.kernel.org
> > 
> > ---
> >  drivers/video/fbdev/aty/mach64_accel.c |   22 +++++++++-------------
> >  1 file changed, 9 insertions(+), 13 deletions(-)
> > 
> > Index: linux-stable/drivers/video/fbdev/aty/mach64_accel.c
> > ===================================================================
> > --- linux-stable.orig/drivers/video/fbdev/aty/mach64_accel.c	2018-08-24 19:51:34.000000000 +0200
> > +++ linux-stable/drivers/video/fbdev/aty/mach64_accel.c	2018-08-24 20:28:55.000000000 +0200
> > @@ -127,7 +127,7 @@ void aty_init_engine(struct atyfb_par *p
> >  
> >  	/* set host attributes */
> >  	wait_for_fifo(13, par);
> > -	aty_st_le32(HOST_CNTL, 0, par);
> > +	aty_st_le32(HOST_CNTL, HOST_BYTE_ALIGN, par);
> >  
> >  	/* set pattern attributes */
> >  	aty_st_le32(PAT_REG0, 0, par);
> > @@ -233,7 +233,8 @@ void atyfb_copyarea(struct fb_info *info
> >  		rotation = rotation24bpp(dx, direction);
> >  	}
> >  
> > -	wait_for_fifo(4, par);
> > +	wait_for_fifo(5, par);
> > +	aty_st_le32(DP_PIX_WIDTH, par->crtc.dp_pix_width, par);
> >  	aty_st_le32(DP_SRC, FRGD_SRC_BLIT, par);
> >  	aty_st_le32(SRC_Y_X, (sx << 16) | sy, par);
> >  	aty_st_le32(SRC_HEIGHT1_WIDTH1, (width << 16) | area->height, par);
> > @@ -269,7 +270,8 @@ void atyfb_fillrect(struct fb_info *info
> >  		rotation = rotation24bpp(dx, DST_X_LEFT_TO_RIGHT);
> >  	}
> >  
> > -	wait_for_fifo(3, par);
> > +	wait_for_fifo(4, par);
> > +	aty_st_le32(DP_PIX_WIDTH, par->crtc.dp_pix_width, par);
> >  	aty_st_le32(DP_FRGD_CLR, color, par);
> >  	aty_st_le32(DP_SRC,
> >  		    BKGD_SRC_BKGD_CLR | FRGD_SRC_FRGD_CLR | MONO_SRC_ONE,
> > @@ -284,7 +286,7 @@ void atyfb_imageblit(struct fb_info *inf
> >  {
> >  	struct atyfb_par *par = (struct atyfb_par *) info->par;
> >  	u32 src_bytes, dx = image->dx, dy = image->dy, width = image->width;
> > -	u32 pix_width_save, pix_width, host_cntl, rotation = 0, src, mix;
> > +	u32 pix_width, rotation = 0, src, mix;
> >  
> >  	if (par->asleep)
> >  		return;
> > @@ -296,8 +298,7 @@ void atyfb_imageblit(struct fb_info *inf
> >  		return;
> >  	}
> >  
> > -	pix_width = pix_width_save = aty_ld_le32(DP_PIX_WIDTH, par);
> > -	host_cntl = aty_ld_le32(HOST_CNTL, par) | HOST_BYTE_ALIGN;
> > +	pix_width = par->crtc.dp_pix_width;
> >  
> >  	switch (image->depth) {
> >  	case 1:
> > @@ -370,12 +371,11 @@ void atyfb_imageblit(struct fb_info *inf
> >  		mix = FRGD_MIX_D_XOR_S | BKGD_MIX_D;
> >  	}
> >  
> > -	wait_for_fifo(6, par);
> > -	aty_st_le32(DP_WRITE_MASK, 0xFFFFFFFF, par);
> 
> Looks like init_engine() sets this one for us. So dropping should be ok.
> 
> > +	wait_for_fifo(5, par);
> >  	aty_st_le32(DP_PIX_WIDTH, pix_width, par);
> >  	aty_st_le32(DP_MIX, mix, par);
> >  	aty_st_le32(DP_SRC, src, par);
> > -	aty_st_le32(HOST_CNTL, host_cntl, par);
> > +	aty_st_le32(HOST_CNTL, HOST_BYTE_ALIGN, par);
> 
> Presumably we could drop this as well since it never changes.

I tried to drop this HOST_CNTL write and it didn't work. It seems that the 
register is written implicitly by some of the other commands.

Mikulas

> Either way
> Reviewed-by: Ville Syrjälä <syrjala@sci.fi>
> 
> >  	aty_st_le32(DST_CNTL, DST_Y_TOP_TO_BOTTOM | DST_X_LEFT_TO_RIGHT | rotation, par);
> >  
> >  	draw_rect(dx, dy, width, image->height, par);
> > @@ -424,8 +424,4 @@ void atyfb_imageblit(struct fb_info *inf
> >  			aty_st_le32(HOST_DATA0, get_unaligned_le32(pbitmap), par);
> >  		}
> >  	}
> > -
> > -	/* restore pix_width */
> > -	wait_for_fifo(1, par);
> > -	aty_st_le32(DP_PIX_WIDTH, pix_width_save, par);
> >  }
> 
> -- 
> Ville Syrjälä
> syrjala@sci.fi
> http://www.sci.fi/~syrjala/
> 

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] mach64: fix image corruption due to reading accelerator registers
  2018-08-27 12:55   ` Ville Syrjälä
  (?)
  (?)
@ 2018-10-08 10:37   ` Bartlomiej Zolnierkiewicz
  -1 siblings, 0 replies; 7+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2018-10-08 10:37 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: Mikulas Patocka, dri-devel


On 08/27/2018 02:55 PM, Ville Syrjälä wrote:
> On Sat, Aug 25, 2018 at 03:51:52PM -0400, Mikulas Patocka wrote:
>> Reading the registers without waiting for engine idle returns
>> unpredictable values. These unpredictable values result in display
>> corruption - if atyfb_imageblit reads the content of DP_PIX_WIDTH with the
>> bit DP_HOST_TRIPLE_EN set (from previous invocation), the driver would
>> never ever clear the bit, resulting in display corruption.
>>
>> We don't want to wait for idle because it would degrade performance, so
>> this patch modifies the driver so that it never reads accelerator
>> registers.
>>
>> HOST_CNTL doesn't have to be read, we can just write it with
>> HOST_BYTE_ALIGN because no other part of the driver cares if
>> HOST_BYTE_ALIGN is set.
>>
>> DP_PIX_WIDTH is written in the functions atyfb_copyarea and atyfb_fillrect
>> with the default value and in atyfb_imageblit with the value set according
>> to the source image data.
>>
>> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
>> Cc: stable@vger.kernel.org
>>
>> ---
>>  drivers/video/fbdev/aty/mach64_accel.c |   22 +++++++++-------------
>>  1 file changed, 9 insertions(+), 13 deletions(-)
>>
>> Index: linux-stable/drivers/video/fbdev/aty/mach64_accel.c
>> ===================================================================
>> --- linux-stable.orig/drivers/video/fbdev/aty/mach64_accel.c	2018-08-24 19:51:34.000000000 +0200
>> +++ linux-stable/drivers/video/fbdev/aty/mach64_accel.c	2018-08-24 20:28:55.000000000 +0200
>> @@ -127,7 +127,7 @@ void aty_init_engine(struct atyfb_par *p
>>  
>>  	/* set host attributes */
>>  	wait_for_fifo(13, par);
>> -	aty_st_le32(HOST_CNTL, 0, par);
>> +	aty_st_le32(HOST_CNTL, HOST_BYTE_ALIGN, par);
>>  
>>  	/* set pattern attributes */
>>  	aty_st_le32(PAT_REG0, 0, par);
>> @@ -233,7 +233,8 @@ void atyfb_copyarea(struct fb_info *info
>>  		rotation = rotation24bpp(dx, direction);
>>  	}
>>  
>> -	wait_for_fifo(4, par);
>> +	wait_for_fifo(5, par);
>> +	aty_st_le32(DP_PIX_WIDTH, par->crtc.dp_pix_width, par);
>>  	aty_st_le32(DP_SRC, FRGD_SRC_BLIT, par);
>>  	aty_st_le32(SRC_Y_X, (sx << 16) | sy, par);
>>  	aty_st_le32(SRC_HEIGHT1_WIDTH1, (width << 16) | area->height, par);
>> @@ -269,7 +270,8 @@ void atyfb_fillrect(struct fb_info *info
>>  		rotation = rotation24bpp(dx, DST_X_LEFT_TO_RIGHT);
>>  	}
>>  
>> -	wait_for_fifo(3, par);
>> +	wait_for_fifo(4, par);
>> +	aty_st_le32(DP_PIX_WIDTH, par->crtc.dp_pix_width, par);
>>  	aty_st_le32(DP_FRGD_CLR, color, par);
>>  	aty_st_le32(DP_SRC,
>>  		    BKGD_SRC_BKGD_CLR | FRGD_SRC_FRGD_CLR | MONO_SRC_ONE,
>> @@ -284,7 +286,7 @@ void atyfb_imageblit(struct fb_info *inf
>>  {
>>  	struct atyfb_par *par = (struct atyfb_par *) info->par;
>>  	u32 src_bytes, dx = image->dx, dy = image->dy, width = image->width;
>> -	u32 pix_width_save, pix_width, host_cntl, rotation = 0, src, mix;
>> +	u32 pix_width, rotation = 0, src, mix;
>>  
>>  	if (par->asleep)
>>  		return;
>> @@ -296,8 +298,7 @@ void atyfb_imageblit(struct fb_info *inf
>>  		return;
>>  	}
>>  
>> -	pix_width = pix_width_save = aty_ld_le32(DP_PIX_WIDTH, par);
>> -	host_cntl = aty_ld_le32(HOST_CNTL, par) | HOST_BYTE_ALIGN;
>> +	pix_width = par->crtc.dp_pix_width;
>>  
>>  	switch (image->depth) {
>>  	case 1:
>> @@ -370,12 +371,11 @@ void atyfb_imageblit(struct fb_info *inf
>>  		mix = FRGD_MIX_D_XOR_S | BKGD_MIX_D;
>>  	}
>>  
>> -	wait_for_fifo(6, par);
>> -	aty_st_le32(DP_WRITE_MASK, 0xFFFFFFFF, par);
> 
> Looks like init_engine() sets this one for us. So dropping should be ok.
> 
>> +	wait_for_fifo(5, par);
>>  	aty_st_le32(DP_PIX_WIDTH, pix_width, par);
>>  	aty_st_le32(DP_MIX, mix, par);
>>  	aty_st_le32(DP_SRC, src, par);
>> -	aty_st_le32(HOST_CNTL, host_cntl, par);
>> +	aty_st_le32(HOST_CNTL, HOST_BYTE_ALIGN, par);
> 
> Presumably we could drop this as well since it never changes.
> 
> Either way
> Reviewed-by: Ville Syrjälä <syrjala@sci.fi>

Patch queued for 4.20, thanks.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-10-08 10:37 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-08-25 19:51 [PATCH 2/3] mach64: fix image corruption due to reading accelerator registers Mikulas Patocka
2018-08-25 19:51 ` Mikulas Patocka
2018-08-27 12:55 ` Ville Syrjälä
2018-08-27 12:55   ` Ville Syrjälä
2018-08-28 15:34   ` Mikulas Patocka
2018-08-28 15:34     ` Mikulas Patocka
2018-10-08 10:37   ` Bartlomiej Zolnierkiewicz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.