<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Mon, Apr 18, 2016 at 5:37 PM, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 04/18/2016 05:10 PM, Jason Ekstrand wrote: > --- > src/intel/vulkan/genX_cmd_buffer.c | 76 +++++++++++++++++++++----------------- > 1 file changed, 42 insertions(+), 34 deletions(-) > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c b/src/intel/vulkan/genX_cmd_buffer.c > index 932ba65..713de82 100644 > --- a/src/intel/vulkan/genX_cmd_buffer.c > +++ b/src/intel/vulkan/genX_cmd_buffer.c > @@ -924,28 +924,34 @@ cmd_buffer_emit_depth_stencil(struct anv_cmd_buffer *cmd_buffer) > > /* Emit 3DSTATE_DEPTH_BUFFER */ > if (has_depth) { > - anv_batch_emit(&cmd_buffer->batch, GENX(3DSTATE_DEPTH_BUFFER), > - .SurfaceType = SURFTYPE_2D, > - .DepthWriteEnable = true, > - .StencilWriteEnable = has_stencil, > - .HierarchicalDepthBufferEnable = false, > - .SurfaceFormat = isl_surf_get_depth_format(&device->isl_dev, > - &image->depth_surface.isl), > - .SurfacePitch = image->depth_surface.isl.row_pitch - 1, > - .SurfaceBaseAddress = { > + anv_batch_emit_blk(&cmd_buffer->batch, GENX(3DSTATE_DEPTH_BUFFER), db) { > + db.SurfaceType = SURFTYPE_2D; > + db.DepthWriteEnable = true; > + db.StencilWriteEnable = has_stencil; > + db.HierarchicalDepthBufferEnable = false; > + > + db.SurfaceFormat = isl_surf_get_depth_format(&device->isl_dev, > + &image->depth_surface.isl); > + > + db.SurfaceBaseAddress = (struct anv_address) { > .bo = image->bo, > .offset = image->offset + image->depth_surface.offset, > - }, > - .Height = fb->height - 1, > - .Width = fb->width - 1, > - .LOD = 0, > - .Depth = 1 - 1, > - .MinimumArrayElement = 0, > - .DepthBufferObjectControlState = GENX(MOCS), > + }; > + db.DepthBufferObjectControlState = GENX(MOCS), > + > + db.SurfacePitch = image->depth_surface.isl.row_pitch - 1; > + db.Height = fb->height - 1; > + db.Width = fb->width - 1; > + db.LOD = 0; > + db.Depth = 1 - 1; > + db.MinimumArrayElement = 0; > + > #if GEN_GEN >= 8 > - .SurfaceQPitch = isl_surf_get_array_pitch_el_rows(&image->depth_surface.isl) >> 2, > + db.SurfaceQPitch = > + isl_surf_get_array_pitch_el_rows(&image->depth_surface.isl) >> 2, > #endif > - .RenderTargetViewExtent = 1 - 1); > + db.RenderTargetViewExtent = 1 - 1; > + } > } else { > /* Even when no depth buffer is present, the hardware requires that > * 3DSTATE_DEPTH_BUFFER be programmed correctly. The Broadwell PRM says: > @@ -965,45 +971,47 @@ cmd_buffer_emit_depth_stencil(struct anv_cmd_buffer *cmd_buffer) > * nor stencil buffer is present. Also, D16_UNORM is not allowed to > * be combined with a stencil buffer so we use D32_FLOAT instead. > */ > - anv_batch_emit(&cmd_buffer->batch, GENX(3DSTATE_DEPTH_BUFFER), > - .SurfaceType = SURFTYPE_2D, > - .SurfaceFormat = D32_FLOAT, > - .Width = fb->width - 1, > - .Height = fb->height - 1, > - .StencilWriteEnable = has_stencil); > + anv_batch_emit_blk(&cmd_buffer->batch, GENX(3DSTATE_DEPTH_BUFFER), db) { > + db.SurfaceType = SURFTYPE_2D; > + db.SurfaceFormat = D32_FLOAT; > + db.Width = fb->width - 1; > + db.Height = fb->height - 1; > + db.StencilWriteEnable = has_stencil; > + } > } > > /* Emit 3DSTATE_STENCIL_BUFFER */ > if (has_stencil) { > - anv_batch_emit(&cmd_buffer->batch, GENX(3DSTATE_STENCIL_BUFFER), > + anv_batch_emit_blk(&cmd_buffer->batch, GENX(3DSTATE_STENCIL_BUFFER), sb) { </div></div>I think all the code that follows violates the style guide. This is part of the reason I think we may regret this style choice. To be clear... I'll go with the crowd, but I just want to be sure the crowd fully has 2n eyes open. </blockquote><div> </div><div>Right. This is a classic example of where the hard "vertical align" rule breaks down. The code below is so broken up by comments and #if's that each of the 4 assignments is in it's own visual section of the code and aligning them doesn't make as much sense. </div><div>At the end of the day, a hard "align things" rule won't really work. It needs to be more of a "sometimes we align things to make them look nicer" which, as has been pointed out isn't something you can make indent, vim, or emacs do for you. Honestly, I'm not *that* much of a fan of aligning things and, if it were only up to me, I probably wouldn't bother. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="HOEnZb"><div class="h5"> > #if GEN_GEN >= 8 || GEN_IS_HASWELL > - .StencilBufferEnable = true, > + sb.StencilBufferEnable = true, > #endif > - .StencilBufferObjectControlState = GENX(MOCS), > + sb.StencilBufferObjectControlState = GENX(MOCS), > > /* Stencil buffers have strange pitch. The PRM says: > * > * The pitch must be set to 2x the value computed based on width, > * as the stencil buffer is stored with two rows interleaved. > */ > - .SurfacePitch = 2 * image->stencil_surface.isl.row_pitch - 1, > + sb.SurfacePitch = 2 * image->stencil_surface.isl.row_pitch - 1, > > #if GEN_GEN >= 8 > - .SurfaceQPitch = isl_surf_get_array_pitch_el_rows(&image->stencil_surface.isl) >> 2, > + sb.SurfaceQPitch = isl_surf_get_array_pitch_el_rows(&image->stencil_surface.isl) >> 2, > #endif > - .SurfaceBaseAddress = { > + sb.SurfaceBaseAddress = (struct anv_address) { > .bo = image->bo, > .offset = image->offset + image->stencil_surface.offset, > - }); > + }; > + } > } else { > - anv_batch_emit(&cmd_buffer->batch, GENX(3DSTATE_STENCIL_BUFFER)); > + anv_batch_emit_blk(&cmd_buffer->batch, GENX(3DSTATE_STENCIL_BUFFER), sb); > } > > /* Disable hierarchial depth buffers. */ > - anv_batch_emit(&cmd_buffer->batch, GENX(3DSTATE_HIER_DEPTH_BUFFER)); > + anv_batch_emit_blk(&cmd_buffer->batch, GENX(3DSTATE_HIER_DEPTH_BUFFER), hz); > > /* Clear the clear params. */ > - anv_batch_emit(&cmd_buffer->batch, GENX(3DSTATE_CLEAR_PARAMS)); > + anv_batch_emit_blk(&cmd_buffer->batch, GENX(3DSTATE_CLEAR_PARAMS), cp); > } > > /** > </div></div></blockquote></div> </div></div>