[PATCH RESEND] drm/vc4: Make sure to emit a tile coordinates between two MSAA loads.

[PATCH RESEND] drm/vc4: Make sure to emit a tile coordinates between two MSAA loads.

[Eric Anholt]

The HW only executes a load once the tile coordinates packet happens,
and only tracks one at a time, so by emitting our two MSAA loads back
to back we would end up with an undefined color or Z buffer.

Fixes dEQP-EGL.functional.render.multi_context.gles2.rgb888_window

Signed-off-by: Eric Anholt <eric@anholt.net>
Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_render_cl.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_render_cl.c b/drivers/gpu/drm/vc4/vc4_render_cl.c
index 273984f71ae2..3c918eeaf56e 100644
--- a/drivers/gpu/drm/vc4/vc4_render_cl.c
+++ b/drivers/gpu/drm/vc4/vc4_render_cl.c
@@ -148,6 +148,12 @@ static void emit_tile(struct vc4_exec_info *exec,
 	}
 
 	if (setup->zs_read) {
+		if (setup->color_read) {
+			/* Exec previous load. */
+			vc4_tile_coordinates(setup, x, y);
+			vc4_store_before_load(setup);
+		}
+
 		if (args->zs_read.flags &
 		    VC4_SUBMIT_RCL_SURFACE_READ_IS_FULL_RES) {
 			rcl_u8(setup, VC4_PACKET_LOAD_FULL_RES_TILE_BUFFER);
@@ -156,12 +162,6 @@ static void emit_tile(struct vc4_exec_info *exec,
 						    &args->zs_read, x, y) |
 				VC4_LOADSTORE_FULL_RES_DISABLE_COLOR);
 		} else {
-			if (setup->color_read) {
-				/* Exec previous load. */
-				vc4_tile_coordinates(setup, x, y);
-				vc4_store_before_load(setup);
-			}
-
 			rcl_u8(setup, VC4_PACKET_LOAD_TILE_BUFFER_GENERAL);
 			rcl_u16(setup, args->zs_read.bits);
 			rcl_u32(setup, setup->zs_read->paddr +
@@ -291,16 +291,15 @@ static int vc4_create_rcl_bo(struct drm_device *dev, struct vc4_exec_info *exec,
 		}
 	}
 	if (setup->zs_read) {
+		if (setup->color_read) {
+			loop_body_size += VC4_PACKET_TILE_COORDINATES_SIZE;
+			loop_body_size += VC4_PACKET_STORE_TILE_BUFFER_GENERAL_SIZE;
+		}
+
 		if (args->zs_read.flags &
 		    VC4_SUBMIT_RCL_SURFACE_READ_IS_FULL_RES) {
 			loop_body_size += VC4_PACKET_LOAD_FULL_RES_TILE_BUFFER_SIZE;
 		} else {
-			if (setup->color_read &&
-			    !(args->color_read.flags &
-			      VC4_SUBMIT_RCL_SURFACE_READ_IS_FULL_RES)) {
-				loop_body_size += VC4_PACKET_TILE_COORDINATES_SIZE;
-				loop_body_size += VC4_PACKET_STORE_TILE_BUFFER_GENERAL_SIZE;
-			}
 			loop_body_size += VC4_PACKET_LOAD_TILE_BUFFER_GENERAL_SIZE;
 		}
 	}
-- 
2.20.0.rc1

Re: [PATCH RESEND] drm/vc4: Make sure to emit a tile coordinates between two MSAA loads.

[Paul Kocialkowski]

Hi,

Le mercredi 06 février 2019 à 15:25 -0800, Eric Anholt a écrit :
> The HW only executes a load once the tile coordinates packet happens,
> and only tracks one at a time, so by emitting our two MSAA loads back
> to back we would end up with an undefined color or Z buffer.

This change deals with things that I'm not very familiar with, but here
is my take on what is happening here:

- When we have to do more than a single load in the same render command
list, we need to send out tile coordinates and a dummy store in between
the two, because of internal architecture requirements of the GPU;
- We're dealing with the color buffer first and then the z-stencil
buffer;
- As a result, we need to issue that dummy store in the block handling
the z-stencil, if there was a previous color load in the rcl;
- We previously only did that for non-MSAA z-stencil buffers (without
the FULL_RES flag);
- The same thing actually needs to be done for the MSAA case too, as
the reason why we need that dummy store also applies to MSAA loads.

If my understanding is correct, then consider this:
Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Otherwise, I'd be happy to know what I misunderstood in the process!

Cheers,

Paul

> Fixes dEQP-EGL.functional.render.multi_context.gles2.rgb888_window
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_render_cl.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_render_cl.c b/drivers/gpu/drm/vc4/vc4_render_cl.c
> index 273984f71ae2..3c918eeaf56e 100644
> --- a/drivers/gpu/drm/vc4/vc4_render_cl.c
> +++ b/drivers/gpu/drm/vc4/vc4_render_cl.c
> @@ -148,6 +148,12 @@ static void emit_tile(struct vc4_exec_info *exec,
>  	}
>  
>  	if (setup->zs_read) {
> +		if (setup->color_read) {
> +			/* Exec previous load. */
> +			vc4_tile_coordinates(setup, x, y);
> +			vc4_store_before_load(setup);
> +		}
> +
>  		if (args->zs_read.flags &
>  		    VC4_SUBMIT_RCL_SURFACE_READ_IS_FULL_RES) {
>  			rcl_u8(setup, VC4_PACKET_LOAD_FULL_RES_TILE_BUFFER);
> @@ -156,12 +162,6 @@ static void emit_tile(struct vc4_exec_info *exec,
>  						    &args->zs_read, x, y) |
>  				VC4_LOADSTORE_FULL_RES_DISABLE_COLOR);
>  		} else {
> -			if (setup->color_read) {
> -				/* Exec previous load. */
> -				vc4_tile_coordinates(setup, x, y);
> -				vc4_store_before_load(setup);
> -			}
> -
>  			rcl_u8(setup, VC4_PACKET_LOAD_TILE_BUFFER_GENERAL);
>  			rcl_u16(setup, args->zs_read.bits);
>  			rcl_u32(setup, setup->zs_read->paddr +
> @@ -291,16 +291,15 @@ static int vc4_create_rcl_bo(struct drm_device *dev, struct vc4_exec_info *exec,
>  		}
>  	}
>  	if (setup->zs_read) {
> +		if (setup->color_read) {
> +			loop_body_size += VC4_PACKET_TILE_COORDINATES_SIZE;
> +			loop_body_size += VC4_PACKET_STORE_TILE_BUFFER_GENERAL_SIZE;
> +		}
> +
>  		if (args->zs_read.flags &
>  		    VC4_SUBMIT_RCL_SURFACE_READ_IS_FULL_RES) {
>  			loop_body_size += VC4_PACKET_LOAD_FULL_RES_TILE_BUFFER_SIZE;
>  		} else {
> -			if (setup->color_read &&
> -			    !(args->color_read.flags &
> -			      VC4_SUBMIT_RCL_SURFACE_READ_IS_FULL_RES)) {
> -				loop_body_size += VC4_PACKET_TILE_COORDINATES_SIZE;
> -				loop_body_size += VC4_PACKET_STORE_TILE_BUFFER_GENERAL_SIZE;
> -			}
>  			loop_body_size += VC4_PACKET_LOAD_TILE_BUFFER_GENERAL_SIZE;
>  		}
>  	}
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH RESEND] drm/vc4: Make sure to emit a tile coordinates between two MSAA loads.

[Eric Anholt]

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

Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:

> Hi,
>
> Le mercredi 06 février 2019 à 15:25 -0800, Eric Anholt a écrit :
>> The HW only executes a load once the tile coordinates packet happens,
>> and only tracks one at a time, so by emitting our two MSAA loads back
>> to back we would end up with an undefined color or Z buffer.
>
> This change deals with things that I'm not very familiar with, but here
> is my take on what is happening here:
>
> - When we have to do more than a single load in the same render command
> list, we need to send out tile coordinates and a dummy store in between
> the two, because of internal architecture requirements of the GPU;
> - We're dealing with the color buffer first and then the z-stencil
> buffer;
> - As a result, we need to issue that dummy store in the block handling
> the z-stencil, if there was a previous color load in the rcl;
> - We previously only did that for non-MSAA z-stencil buffers (without
> the FULL_RES flag);
> - The same thing actually needs to be done for the MSAA case too, as
> the reason why we need that dummy store also applies to MSAA loads.
>
> If my understanding is correct, then consider this:
> Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

This is all correct.  Thanks!

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