[Mesa-dev] [PATCH 1/2] anv/cmd_buffer: Set the correct surface type for depth/stencil

[Ilia Mirkin]

On Tue, Nov 29, 2016 at 3:44 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>
>
> On Tue, Nov 29, 2016 at 12:24 PM, Nanley Chery <nanleychery at gmail.com>
> wrote:
>>
>> On Mon, Nov 28, 2016 at 03:45:41PM -0800, Jason Ekstrand wrote:
>> > ---
>> >  src/intel/vulkan/genX_cmd_buffer.c | 51
>> > ++++++++++++++++++++++++++++++++++++--
>> >  1 file changed, 49 insertions(+), 2 deletions(-)
>> >
>>
>> This patch does not match the one you've merged
>> (d4ef87c1bb4290293148cbd6cb782764df38f8f4). The committed code
>> introduces the following buggy hunk:
>>
>> @@ -2106,7 +2152,12 @@ cmd_buffer_emit_depth_stencil(struct anv_cmd_buffer
>> *cmd_buffer)
>>         * be combined with a stencil buffer so we use D32_FLOAT instead.
>>         */
>>        anv_batch_emit(&cmd_buffer->batch, GENX(3DSTATE_DEPTH_BUFFER), db)
>> {
>> -         db.SurfaceType          = SURFTYPE_2D;
>> +         if (has_stencil) {
>> +            db.SurfaceType       = SURFTYPE_2D;
>> +
>> depth_stencil_surface_type(image->stencil_surface.isl.dim);

this line is a no-op.

>> +         } else {
>> +            db.SurfaceType       = SURFTYPE_2D;
>> +         }
>>           db.SurfaceFormat        = D32_FLOAT;
>>           db.Width                = fb->width - 1;
>>           db.Height               = fb->height - 1;
>
>
> Yes, it introduces that hunk.  What's buggy about it?  The old code made
> Jenkins explode.  Maybe I should have sent a v2.
>
>>
>>
>> -Nanley
>>
>> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
>> > b/src/intel/vulkan/genX_cmd_buffer.c
>> > index a965cd6..c953b93 100644
>> > --- a/src/intel/vulkan/genX_cmd_buffer.c
>> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
>> > @@ -2033,6 +2033,51 @@ genX(cmd_buffer_emit_gen7_depth_flush)(struct
>> > anv_cmd_buffer *cmd_buffer)
>> >     }
>> >  }
>> >
>> > +static uint32_t
>> > +depth_stencil_surface_type(enum isl_surf_dim dim)
>> > +{
>> > +   switch (dim) {
>> > +   case ISL_SURF_DIM_1D:
>> > +      if (GEN_GEN >= 9) {
>> > +         /* From the Sky Lake PRM, 3DSTATAE_DEPTH_BUFFER::SurfaceType
>> > +          *
>> > +          *    Programming Notes:
>> > +          *    The Surface Type of the depth buffer must be the same as
>> > the
>> > +          *    Surface Type of the render target(s) (defined in
>> > +          *    SURFACE_STATE), unless either the depth buffer or render
>> > +          *    targets are SURFTYPE_NULL (see exception below for SKL).
>> > 1D
>> > +          *    surface type not allowed for depth surface and stencil
>> > surface.
>> > +          *
>> > +          *    Workaround:
>> > +          *    If depth/stencil is enabled with 1D render target,
>> > +          *    depth/stencil surface type needs to be set to 2D surface
>> > type
>> > +          *    and height set to 1. Depth will use (legacy) TileY and
>> > stencil
>> > +          *    will use TileW. For this case only, the Surface Type of
>> > the
>> > +          *    depth buffer can be 2D while the Surface Type of the
>> > render
>> > +          *    target(s) are 1D, representing an exception to a
>> > programming
>> > +          *    note above.
>> > +          */
>> > +         return SURFTYPE_2D;
>> > +      } else {
>> > +         return SURFTYPE_1D;
>> > +      }
>> > +   case ISL_SURF_DIM_2D:
>> > +      return SURFTYPE_2D;
>> > +   case ISL_SURF_DIM_3D:
>> > +      if (GEN_GEN >= 9) {
>> > +         /* The Sky Lake docs list the value for 3D as "Reserved".
>> > However,
>> > +          * they have the exact same layout as 2D arrays on gen9+, so
>> > we can
>> > +          * just use 2D here.
>> > +          */
>> > +         return SURFTYPE_2D;
>> > +      } else {
>> > +         return SURFTYPE_3D;
>> > +      }
>> > +   default:
>> > +      unreachable("Invalid surface dimension");
>> > +   }
>> > +}
>> > +
>> >  static void
>> >  cmd_buffer_emit_depth_stencil(struct anv_cmd_buffer *cmd_buffer)
>> >  {
>> > @@ -2054,7 +2099,8 @@ 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),
>> > db) {
>> > -         db.SurfaceType                   = SURFTYPE_2D;
>> > +         db.SurfaceType                   =
>> > +            depth_stencil_surface_type(image->depth_surface.isl.dim);
>> >           db.DepthWriteEnable              = true;
>> >           db.StencilWriteEnable            = has_stencil;
>> >
>> > @@ -2106,7 +2152,8 @@ cmd_buffer_emit_depth_stencil(struct
>> > anv_cmd_buffer *cmd_buffer)
>> >         * be combined with a stencil buffer so we use D32_FLOAT instead.
>> >         */
>> >        anv_batch_emit(&cmd_buffer->batch, GENX(3DSTATE_DEPTH_BUFFER),
>> > db) {
>> > -         db.SurfaceType          = SURFTYPE_2D;
>> > +         db.SurfaceType          =
>> > +            depth_stencil_surface_type(image->depth_surface.isl.dim);
>> >           db.SurfaceFormat        = D32_FLOAT;
>> >           db.Width                = fb->width - 1;
>> >           db.Height               = fb->height - 1;
>> > --
>> > 2.5.0.400.gff86faf
>> >
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>