On Tue, Jun 13, 2017 at 4:54 PM, Jason Ekstrand <[email protected]> wrote:
> Looking through your code, I believe it is correct. I'm a bit concerned, > however, because we're now copying back to the main surface on every draw > call. If an app ever happens to hit this path, it will be correct, but > it's performance will tank. Unfortunately, a better implementation would > be a pain because we would have to track which slice we pulled out and do > something clever when someone goes to texture from it. The resolve > framework could handle this but it would be painful. Ken? Thoughts? > > Reviewed-by: Jason Ekstrand <[email protected]> > That applies to all 3 :-) > On Fri, Jun 9, 2017 at 7:04 AM, Topi Pohjolainen < > [email protected]> wrote: > >> On gen < 6 one doesn't have level or layer specifiers available >> for render and depth targets. In order to support rendering to >> specific level/layer, driver needs to manually offset the surface >> to the desired slice. >> There are, however, alignment restrictions to respect as well and >> in come cases the only option is to use temporary single slice >> surface which driver copies after rendering to the full miptree. >> >> Current alignment workaround introduces new texture images which >> are added to the parent texture object. Texture validation later >> on copies the additional levels back to the surface that contains >> the full mipmap. >> This only works for non-arrayed surfaces and driver currently >> creates new arrayed images in vain - individual layers within the >> newly created are still unaligned the same as before. >> >> This patch drops this mechanism and instead attaches single >> temporary slice into the render buffer. This gets immediately >> copied back to the mipmapped and/or arrayed surface just after >> the render is done. >> >> Sitting on top of earlier series cleaning up the depth buffer >> state, this patch additionally fixes the following piglit tests: >> >> ext_texture_array.copyteximage 2d_array.g45m64 >> ext_texture_array.copyteximage 1d_array.g45m64 >> arb_framebuffer_object.fbo-blit-stretch.g33m64 >> ext_framebuffer_object.fbo-cubemap.g965m64 >> arb_framebuffer_object.fbo-generatemipmap-cubemap.g965m64 >> arb_texture_cube_map.copyteximage cube.g965m64 >> ext_texture_array.copyteximage 1d_array.g965m64 >> ext_texture_array.copyteximage 2d_array.g965m64 >> ext_texture_array.fbo-array.g965m64 >> ext_texture_array.gen-mipmap.g965m64 >> ext_texture_array.fbo-generatemipmap-array.g965m64 >> arb_pixel_buffer_object.texsubimage array pbo.g965m64 >> ext_texture_array.copyteximage 2d_array.ilkm64 >> ext_texture_array.copyteximage 1d_array.ilkm64 >> arb_texture_cube_map.copyteximage cube.ilkm64 >> >> CC: Kenneth Graunke <[email protected]> >> CC: Jason Ekstrand <[email protected]> >> CC: Ian Romanick <[email protected]> >> Signed-off-by: Topi Pohjolainen <[email protected]> >> --- >> src/mesa/drivers/dri/i965/brw_draw.c | 51 >> ++++++++++++++++++++++++ >> src/mesa/drivers/dri/i965/brw_misc_state.c | 4 +- >> src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 3 +- >> src/mesa/drivers/dri/i965/intel_fbo.c | 19 +++++---- >> src/mesa/drivers/dri/i965/intel_fbo.h | 24 +++++++++++ >> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 2 +- >> src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 7 ++++ >> 7 files changed, 99 insertions(+), 11 deletions(-) >> >> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c >> b/src/mesa/drivers/dri/i965/brw_draw.c >> index 611cb86..cb441c3 100644 >> --- a/src/mesa/drivers/dri/i965/brw_draw.c >> +++ b/src/mesa/drivers/dri/i965/brw_draw.c >> @@ -396,6 +396,56 @@ brw_postdraw_set_buffers_need_resolve(struct >> brw_context *brw) >> } >> >> static void >> +intel_renderbuffer_move_temp_back(struct brw_context *brw, >> + struct intel_renderbuffer *irb) >> +{ >> + if (irb->align_wa_mt == NULL) >> + return; >> + >> + brw_render_cache_set_check_flush(brw, irb->align_wa_mt->bo); >> + >> + intel_miptree_copy_slice(brw, irb->align_wa_mt, 0, 0, >> + irb->mt, >> + irb->Base.Base.TexImage->Level, >> irb->mt_layer); >> + >> + intel_miptree_reference(&irb->align_wa_mt, NULL); >> + >> + /* Finally restore the x,y to correspond to full miptree. */ >> + intel_renderbuffer_set_draw_offset(irb); >> + >> + /* Make sure render surface state gets re-emitted with updated >> miptree. */ >> + brw->NewGLState |= _NEW_BUFFERS; >> +} >> + >> +static void >> +brw_postdraw_reconcile_align_wa_slices(struct brw_context *brw) >> +{ >> + struct gl_context *ctx = &brw->ctx; >> + struct gl_framebuffer *fb = ctx->DrawBuffer; >> + >> + struct intel_renderbuffer *depth_irb = >> + intel_get_renderbuffer(fb, BUFFER_DEPTH); >> + struct intel_renderbuffer *stencil_irb = >> + intel_get_renderbuffer(fb, BUFFER_STENCIL); >> + >> + if (depth_irb && depth_irb->align_wa_mt) >> + intel_renderbuffer_move_temp_back(brw, depth_irb); >> + >> + if (stencil_irb && stencil_irb->align_wa_mt) >> + intel_renderbuffer_move_temp_back(brw, stencil_irb); >> + >> + for (unsigned i = 0; i < fb->_NumColorDrawBuffers; i++) { >> + struct intel_renderbuffer *irb = >> + intel_renderbuffer(fb->_ColorDrawBuffers[i]); >> + >> + if (!irb || irb->align_wa_mt == NULL) >> + continue; >> + >> + intel_renderbuffer_move_temp_back(brw, irb); >> + } >> +} >> + >> +static void >> brw_predraw_set_aux_buffers(struct brw_context *brw) >> { >> if (brw->gen < 9) >> @@ -626,6 +676,7 @@ retry: >> intel_batchbuffer_flush(brw); >> >> brw_program_cache_check_size(brw); >> + brw_postdraw_reconcile_align_wa_slices(brw); >> brw_postdraw_set_buffers_need_resolve(brw); >> >> return; >> diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c >> b/src/mesa/drivers/dri/i965/brw_misc_state.c >> index fe021b0..0c25261 100644 >> --- a/src/mesa/drivers/dri/i965/brw_misc_state.c >> +++ b/src/mesa/drivers/dri/i965/brw_misc_state.c >> @@ -132,7 +132,7 @@ get_stencil_miptree(struct intel_renderbuffer *irb) >> return NULL; >> if (irb->mt->stencil_mt) >> return irb->mt->stencil_mt; >> - return irb->mt; >> + return intel_renderbuffer_get_mt(irb); >> } >> >> static bool >> @@ -268,7 +268,7 @@ brw_emit_depthbuffer(struct brw_context *brw) >> /* _NEW_BUFFERS */ >> struct intel_renderbuffer *depth_irb = intel_get_renderbuffer(fb, >> BUFFER_DEPTH); >> struct intel_renderbuffer *stencil_irb = intel_get_renderbuffer(fb, >> BUFFER_STENCIL); >> - struct intel_mipmap_tree *depth_mt = depth_irb ? depth_irb->mt : NULL; >> + struct intel_mipmap_tree *depth_mt = intel_renderbuffer_get_mt(dept >> h_irb); >> struct intel_mipmap_tree *stencil_mt = get_stencil_miptree(stencil_ir >> b); >> uint32_t tile_x = brw->depthstencil.tile_x; >> uint32_t tile_y = brw->depthstencil.tile_y; >> diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c >> b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c >> index 3278ef3..8f818ab 100644 >> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c >> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c >> @@ -1008,7 +1008,8 @@ gen4_update_renderbuffer_surface(struct >> brw_context *brw, >> * miptree and render into that. >> */ >> intel_renderbuffer_move_to_temp(brw, irb, false); >> - mt = irb->mt; >> + assert(irb->align_wa_mt); >> + mt = irb->align_wa_mt; >> } >> } >> >> diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c >> b/src/mesa/drivers/dri/i965/intel_fbo.c >> index a24ddd0..efbaa82 100644 >> --- a/src/mesa/drivers/dri/i965/intel_fbo.c >> +++ b/src/mesa/drivers/dri/i965/intel_fbo.c >> @@ -993,11 +993,11 @@ intel_renderbuffer_move_to_temp(struct brw_context >> *brw, >> >> intel_get_image_dims(rb->TexImage, &width, &height, &depth); >> >> - new_mt = intel_miptree_create(brw, rb->TexImage->TexObject->Target, >> + assert(irb->align_wa_mt == NULL); >> + new_mt = intel_miptree_create(brw, GL_TEXTURE_2D, >> intel_image->base.Base.TexFormat, >> - intel_image->base.Base.Level, >> - intel_image->base.Base.Level, >> - width, height, depth, >> + 0, 0, >> + width, height, 1, >> irb->mt->num_samples, >> layout_flags); >> >> @@ -1005,11 +1005,16 @@ intel_renderbuffer_move_to_temp(struct >> brw_context *brw, >> intel_miptree_alloc_hiz(brw, new_mt); >> } >> >> - intel_miptree_copy_teximage(brw, intel_image, new_mt, invalidate); >> + if (!invalidate) >> + intel_miptree_copy_slice(brw, intel_image->mt, >> + intel_image->base.Base.Level, >> irb->mt_layer, >> + new_mt, 0, 0); >> >> - intel_miptree_reference(&irb->mt, intel_image->mt); >> - intel_renderbuffer_set_draw_offset(irb); >> + intel_miptree_reference(&irb->align_wa_mt, new_mt); >> intel_miptree_release(&new_mt); >> + >> + irb->draw_x = 0; >> + irb->draw_y = 0; >> } >> >> void >> diff --git a/src/mesa/drivers/dri/i965/intel_fbo.h >> b/src/mesa/drivers/dri/i965/intel_fbo.h >> index 2d2ef1e..3c2be82 100644 >> --- a/src/mesa/drivers/dri/i965/intel_fbo.h >> +++ b/src/mesa/drivers/dri/i965/intel_fbo.h >> @@ -67,6 +67,16 @@ struct intel_renderbuffer >> */ >> struct intel_mipmap_tree *singlesample_mt; >> >> + /* Gen < 6 doesn't have layer specifier for render targets or depth. >> Driver >> + * needs to manually offset surfaces to correct level/layer. There >> are, >> + * however, alignment restrictions to respect as well and in come >> cases >> + * the only option is to use temporary single slice surface which >> driver >> + * copies after rendering to the full miptree. >> + * >> + * See intel_renderbuffer_move_to_temp(). >> + */ >> + struct intel_mipmap_tree *align_wa_mt; >> + >> /** >> * \name Miptree view >> * \{ >> @@ -136,6 +146,14 @@ intel_renderbuffer(struct gl_renderbuffer *rb) >> return NULL; >> } >> >> +static inline struct intel_mipmap_tree * >> +intel_renderbuffer_get_mt(struct intel_renderbuffer *irb) >> +{ >> + if (!irb) >> + return NULL; >> + >> + return (irb->align_wa_mt) ? irb->align_wa_mt : irb->mt; >> +} >> >> /** >> * \brief Return the framebuffer attachment specified by attIndex. >> @@ -188,6 +206,12 @@ intel_renderbuffer_get_tile_offsets(struct >> intel_renderbuffer *irb, >> uint32_t *tile_x, >> uint32_t *tile_y) >> { >> + if (irb->align_wa_mt) { >> + *tile_x = 0; >> + *tile_y = 0; >> + return 0; >> + } >> + >> return intel_miptree_get_tile_offsets(irb->mt, irb->mt_level, >> irb->mt_layer, >> tile_x, tile_y); >> } >> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c >> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c >> index a4b2aeb..01b68cd 100644 >> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c >> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c >> @@ -1266,7 +1266,7 @@ intel_miptree_copy_slice_sw(struct brw_context >> *brw, >> } >> } >> >> -static void >> +void >> intel_miptree_copy_slice(struct brw_context *brw, >> struct intel_mipmap_tree *src_mt, >> unsigned src_level, unsigned src_layer, >> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h >> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h >> index 41d3d25..7979f3b 100644 >> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h >> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h >> @@ -853,6 +853,13 @@ void intel_miptree_set_image_offset(struct >> intel_mipmap_tree *mt, >> GLuint img, GLuint x, GLuint y); >> >> void >> +intel_miptree_copy_slice(struct brw_context *brw, >> + struct intel_mipmap_tree *src_mt, >> + unsigned src_level, unsigned src_layer, >> + struct intel_mipmap_tree *dst_mt, >> + unsigned dst_level, unsigned dst_layer); >> + >> +void >> intel_miptree_copy_teximage(struct brw_context *brw, >> struct intel_texture_image *intelImage, >> struct intel_mipmap_tree *dst_mt, bool >> invalidate); >> -- >> 2.9.3 >> >> >
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
