[PATCH 2/3] mach64: fix image corruption due to reading accelerator registers

[Mikulas Patocka]

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 at redhat.com>
> > Cc: stable at 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 at 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 at sci.fi
> http://www.sci.fi/~syrjala/
>