On 09/16/2013 02:55 PM, Paul Berry wrote:
> On 13 September 2013 23:10, Kenneth Graunke <kenn...@whitecape.org
> <mailto:kenn...@whitecape.org>> wrote:
>
> This was an embarassingly large amount of copy and pasted code,
> and it wasn't particularly simple code either. By factoring it out
> into a helper function, we consolidate the complexity.
>
> Signed-off-by: Kenneth Graunke <kenn...@whitecape.org
> <mailto:kenn...@whitecape.org>>
>
>
> I really like what you're doing here. A few minor comments:
Thanks!
> ---
> src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 144
> +++++++++-------------
> 1 file changed, 58 insertions(+), 86 deletions(-)
>
> 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 37e3174..8413308 100644
> --- a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> @@ -224,6 +224,37 @@ gen7_check_surface_setup(uint32_t *surf, bool
> is_render_target)
> }
> }
>
> +static void
> +gen7_emit_buffer_surface_state(struct brw_context *brw,
> + uint32_t *out_offset,
> + drm_intel_bo *bo,
> + unsigned buffer_offset,
> + unsigned surface_format,
> + unsigned buffer_size,
> + unsigned pitch,
> + unsigned mocs)
> +{
> + uint32_t *surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
> + 8 * 4, 32, out_offset);
> + memset(surf, 0, 8 * 4);
> +
> + surf[0] = BRW_SURFACE_BUFFER << BRW_SURFACE_TYPE_SHIFT |
> + surface_format << BRW_SURFACE_FORMAT_SHIFT |
> + BRW_SURFACE_RC_READ_WRITE;
> + surf[1] = bo->offset + buffer_offset; /* reloc */
> + surf[2] = SET_FIELD(buffer_size & 0x7f, GEN7_SURFACE_WIDTH) |
> + SET_FIELD((buffer_size >> 7) & 0x3fff,
> GEN7_SURFACE_HEIGHT);
> + surf[3] = SET_FIELD((buffer_size >> 21) & 0x3f, BRW_SURFACE_DEPTH) |
>
>
> These three instances of buffer_size should be (buffer_size - 1). I
> think that there was a pre-existing bug in
> gen7_update_buffer_texture_surface() where it wasn't subtracting 1 where
> it should. Probably we should fix the bug as a pre-patch.
Yeah, I think there's a pre-existing bug here. I'll fix that in a patch
at the start of the series.
>
> + (pitch - 1);
> + surf[4] = 0;
>
>
> Technically this line is unnecessary given the memset() above. I'm
> quibbling, though--it's hard to imagine this making a significant
> performance difference :)
True enough :) Removed.
[snip]
> @@ -371,38 +386,15 @@ gen7_create_constant_surface(struct
> brw_context *brw,
> {
> uint32_t stride = dword_pitch ? 4 : 16;
> uint32_t elements = ALIGN(size, stride) / stride;
> - const GLint w = elements - 1;
>
> - uint32_t *surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
> - 8 * 4, 32, out_offset);
> - memset(surf, 0, 8 * 4);
> -
> - surf[0] = BRW_SURFACE_BUFFER << BRW_SURFACE_TYPE_SHIFT |
> - BRW_SURFACEFORMAT_R32G32B32A32_FLOAT <<
> BRW_SURFACE_FORMAT_SHIFT |
> - BRW_SURFACE_RC_READ_WRITE;
> -
> - assert(bo);
> - surf[1] = bo->offset + offset; /* reloc */
> -
> - /* note that these differ from GEN6 */
> - surf[2] = SET_FIELD(w & 0x7f, GEN7_SURFACE_WIDTH) |
> - SET_FIELD((w >> 7) & 0x3fff, GEN7_SURFACE_HEIGHT);
> - surf[3] = SET_FIELD((w >> 21) & 0x3f, BRW_SURFACE_DEPTH) |
> - (stride - 1);
> -
> - if (brw->is_haswell) {
> - surf[7] = SET_FIELD(HSW_SCS_RED, GEN7_SURFACE_SCS_R) |
> - SET_FIELD(HSW_SCS_GREEN, GEN7_SURFACE_SCS_G) |
> - SET_FIELD(HSW_SCS_BLUE, GEN7_SURFACE_SCS_B) |
> - SET_FIELD(HSW_SCS_ALPHA, GEN7_SURFACE_SCS_A);
> - }
>
>
> I don't see this Haswell-specific code in
> gen7_emit_buffer_surface_state(). Is that a problem?
I don't think those matter for buffer surfaces - I was just overzealous
when I added them originally. We already dropped it for some buffer
surfaces.
Still, it's definitely worth testing.
>
> -
> - drm_intel_bo_emit_reloc(brw->batch.bo <http://batch.bo>,
> - *out_offset + 4,
> - bo, offset,
> - I915_GEM_DOMAIN_SAMPLER, 0);
> -
> - gen7_check_surface_setup(surf, false /* is_render_target */);
> + gen7_emit_buffer_surface_state(brw,
> + out_offset,
> + bo,
> + offset,
> + BRW_SURFACEFORMAT_R32G32B32A32_FLOAT,
> + elements - 1,
> + stride,
> + 0 /* mocs */);
> }
>
> /**
> @@ -411,34 +403,14 @@ gen7_create_constant_surface(struct
> brw_context *brw,
> void
> gen7_create_shader_time_surface(struct brw_context *brw, uint32_t
> *out_offset)
> {
> - const int w = brw->shader_time.bo->size - 1;
> -
> - uint32_t *surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
> - 8 * 4, 32, out_offset);
> - memset(surf, 0, 8 * 4);
> -
> - surf[0] = BRW_SURFACE_BUFFER << BRW_SURFACE_TYPE_SHIFT |
> - BRW_SURFACEFORMAT_RAW << BRW_SURFACE_FORMAT_SHIFT |
> - BRW_SURFACE_RC_READ_WRITE;
> -
> - surf[1] = brw->shader_time.bo->offset; /* reloc */
> -
> - /* note that these differ from GEN6 */
> - surf[2] = SET_FIELD(w & 0x7f, GEN7_SURFACE_WIDTH) |
> - SET_FIELD((w >> 7) & 0x3fff, GEN7_SURFACE_HEIGHT);
> - surf[3] = SET_FIELD((w >> 21) & 0x3f, BRW_SURFACE_DEPTH);
> -
> - /* Unlike texture or renderbuffer surfaces, we only do untyped
> operations
> - * on the shader_time surface, so there's no need to set HSW channel
> - * overrides.
> - */
> -
> - drm_intel_bo_emit_reloc(brw->batch.bo <http://batch.bo>,
> - *out_offset + 4,
> - brw->shader_time.bo
> <http://shader_time.bo>, 0,
> - I915_GEM_DOMAIN_SAMPLER, 0);
> -
> - gen7_check_surface_setup(surf, false /* is_render_target */);
> + gen7_emit_buffer_surface_state(brw,
> + out_offset,
> + brw->shader_time.bo
> <http://shader_time.bo>,
> + 0,
> + BRW_SURFACEFORMAT_RAW,
> + brw->shader_time.bo->size - 1,
> + 1,
> + 0 /* mocs */);
> }
>
> static void
> --
> 1.8.3.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org <mailto:mesa-dev@lists.freedesktop.org>
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
