[Mesa-dev] [PATCH 1/4] i965: Refactor Gen7+ SURFACE_STATE setup for buffer surfaces.

[Kenneth Graunke]

On 09/16/2013 09:38 AM, Eric Anholt wrote:
> Kenneth Graunke <kenneth at whitecape.org> writes:
> 
>> 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 <kenneth at whitecape.org>
>> ---
>>  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) |
>> +             (pitch - 1);
>> +   surf[4] = 0;
>> +   surf[5] = SET_FIELD(mocs, GEN7_SURFACE_MOCS);
>> +
>> +   /* Emit relocation to surface contents */
>> +   drm_intel_bo_emit_reloc(brw->batch.bo, *out_offset + 4,
>> +                           bo, buffer_offset, I915_GEM_DOMAIN_SAMPLER, 0);
> 
> I think this will segfault if BO is NULL

Clearly.  I don't know how I missed that...thanks for catching it.

[snip]

> But here you might pass in a NULL bo.  Same for patch 4.  I'm confused
> that this didn't produce a piglit regression.

Fixed in both patches.  I'm not sure why it didn't cause regressions.

>>  }
> 
> I think the savings is well worth it in patches 1, 3, and 4, but I'm not
> a fan of patch 2.  I've got a lot of branches outstanding where I'm
> trying to finally finish off mt->first_level and the tile offset usage,
> and sharing code between renderbuffers and textures is just going to
> make that harder to do.  (I'm already really tempted to split gen4/5
> From gen6 RB setup, since I think I could land gen4/5 without
> regressing, while gen6 is the unstable hw)
> 

Hmm.  I'd really like to try and share some code, but I don't want to
get in the way of the work that you're doing to kill off tile offsets.
I think I'll drop patch 2 for now, and maybe try again once the code
isn't in as much flux.

--Ken