On 18 August 2013 22:15, Kenneth Graunke <kenn...@whitecape.org> wrote:
> SURF_INDEX_DRAW() has been the identity function since the dawn of time,
> and both the shader code and binding table upload code relied on that,
> simply using X rather than SURF_INDEX_DRAW(X).
>
> Even if that continues to be true, using the macro clarifies the code.
>
> The comment about draw buffers needing to be first in order for
> headerless render target writes to work turned out to be wrong; with
> this change, SURF_INDEX_DRAW can be changed to arbitrary indices and
> everything continues working.
>
> The confusion was over the "Render Target Index" field in the FB write
> message header. If it were a binding table index, then RT 0 would have
> to be at index 0 for headerless FB writes to work. However, it's
> actually an index into the blend state table, so there's no problem.
>
> Signed-off-by: Kenneth Graunke <kenn...@whitecape.org>
> Cc: Paul Berry <stereotype...@gmail.com>
>
Reviewed-by: Paul Berry <stereotype...@gmail.com>
> ---
> src/mesa/drivers/dri/i965/brw_context.h | 4 ----
> src/mesa/drivers/dri/i965/brw_fs_emit.cpp | 2 +-
> src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 12 ++++++------
> src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 14 ++++++++------
> 4 files changed, 15 insertions(+), 17 deletions(-)
>
> In order to fix the comment as Paul requested, I had to do some digging.
> The end result was discovering that the comment was entirely wrong - the
> only reason SURF_INDEX_DRAW had to be the identity function was that we
> weren't calling it. With that fixed, it can be arbitrary, so there's
> no need for the comment at all.
>
> I did try totally scrambling the order of the binding table indexes
> after applying the patch, and there were no Piglit regressions on IVB.
> So I'm confident that it works (at least on Gen7+).
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
> index acd8e3f..e0deee0 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -602,10 +602,6 @@ struct brw_vs_prog_data {
> * | : | : |
> * | 63 | SOL Binding 63 |
> * +-----+-------------------------+
> - *
> - * Note that nothing actually uses the SURF_INDEX_DRAW macro, so it has
> to be
> - * the identity function or things will break. We do want to keep draw
> buffers
> - * first so we can use headerless render target writes for RT 0.
> */
> #define SURF_INDEX_DRAW(d) (d)
> #define SURF_INDEX_FRAG_CONST_BUFFER (BRW_MAX_DRAW_BUFFERS + 1)
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
> index 4225922..b90cf0f 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
> @@ -170,7 +170,7 @@ fs_generator::generate_fb_write(fs_inst *inst)
> inst->base_mrf,
> implied_header,
> msg_control,
> - inst->target,
> + SURF_INDEX_DRAW(inst->target),
> inst->mlen,
> 0,
> eot,
> 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 8847f91..16579f9 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> @@ -533,8 +533,8 @@ brw_update_null_renderbuffer_surface(struct
> brw_context *brw, unsigned int unit)
> /* _NEW_BUFFERS */
> const struct gl_framebuffer *fb = ctx->DrawBuffer;
>
> - surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
> - 6 * 4, 32, &brw->wm.surf_offset[unit]);
> + surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE, 6 * 4, 32,
> + &brw->wm.surf_offset[SURF_INDEX_DRAW(unit)]);
>
> if (fb->Visual.samples > 1) {
> /* On Gen6, null render targets seem to cause GPU hangs when
> @@ -587,7 +587,7 @@ brw_update_null_renderbuffer_surface(struct
> brw_context *brw, unsigned int unit)
>
> if (bo) {
> drm_intel_bo_emit_reloc(brw->batch.bo,
> - brw->wm.surf_offset[unit] + 4,
> + brw->wm.surf_offset[SURF_INDEX_DRAW(unit)]
> + 4,
> bo, 0,
> I915_GEM_DOMAIN_RENDER,
> I915_GEM_DOMAIN_RENDER);
> }
> @@ -635,8 +635,8 @@ brw_update_renderbuffer_surface(struct brw_context
> *brw,
>
> region = irb->mt->region;
>
> - surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
> - 6 * 4, 32, &brw->wm.surf_offset[unit]);
> + surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE, 6 * 4, 32,
> + &brw->wm.surf_offset[SURF_INDEX_DRAW(unit)]);
>
> format = brw->render_target_format[rb_format];
> if (unlikely(!brw->format_supported_as_render_target[rb_format])) {
> @@ -692,7 +692,7 @@ brw_update_renderbuffer_surface(struct brw_context
> *brw,
> }
>
> drm_intel_bo_emit_reloc(brw->batch.bo,
> - brw->wm.surf_offset[unit] + 4,
> + brw->wm.surf_offset[SURF_INDEX_DRAW(unit)] + 4,
> region->bo,
> surf[1] - region->bo->offset,
> I915_GEM_DOMAIN_RENDER,
> diff --git a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> index 34cf63b..cdd2242 100644
> --- a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> @@ -475,8 +475,8 @@ gen7_update_null_renderbuffer_surface(struct
> brw_context *brw, unsigned unit)
> /* _NEW_BUFFERS */
> const struct gl_framebuffer *fb = ctx->DrawBuffer;
>
> - uint32_t *surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
> - 8 * 4, 32, &brw->wm.surf_offset[unit]);
> + uint32_t *surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE, 8 * 4,
> 32,
> +
> &brw->wm.surf_offset[SURF_INDEX_DRAW(unit)]);
> memset(surf, 0, 8 * 4);
>
> /* From the Ivybridge PRM, Volume 4, Part 1, page 65,
> @@ -518,8 +518,10 @@ gen7_update_renderbuffer_surface(struct brw_context
> *brw,
> GLenum gl_target = rb->TexImage ?
> rb->TexImage->TexObject->Target : GL_TEXTURE_2D;
>
> - uint32_t *surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
> - 8 * 4, 32,
> &brw->wm.surf_offset[unit]);
> + uint32_t surf_index = SURF_INDEX_DRAW(unit);
> +
> + uint32_t *surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE, 8 * 4,
> 32,
> + &brw->wm.surf_offset[surf_index]);
> memset(surf, 0, 8 * 4);
>
> intel_miptree_used_for_rendering(irb->mt);
> @@ -588,7 +590,7 @@ gen7_update_renderbuffer_surface(struct brw_context
> *brw,
> (depth - 1) << GEN7_SURFACE_RENDER_TARGET_VIEW_EXTENT_SHIFT;
>
> if (irb->mt->mcs_mt) {
> - gen7_set_surface_mcs_info(brw, surf, brw->wm.surf_offset[unit],
> + gen7_set_surface_mcs_info(brw, surf,
> brw->wm.surf_offset[surf_index],
> irb->mt->mcs_mt, true /* is RT */);
> }
>
> @@ -602,7 +604,7 @@ gen7_update_renderbuffer_surface(struct brw_context
> *brw,
> }
>
> drm_intel_bo_emit_reloc(brw->batch.bo,
> - brw->wm.surf_offset[unit] + 4,
> + brw->wm.surf_offset[surf_index] + 4,
> region->bo,
> surf[1] - region->bo->offset,
> I915_GEM_DOMAIN_RENDER,
> --
> 1.8.3.4
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
