| Message ID | 20190206232550.12012-1-eric@anholt.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] drm/vc4: Make sure to emit a tile coordinates between two MSAA loads. | expand |
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 <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!
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; } }
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(-)