[Mesa-dev] [PATCH 01/23] i965: Let caller of intel_miptree_create_layout() decide msaa layout

[Ben Widawsky]

On Mon, Feb 08, 2016 at 06:51:21PM +0200, Topi Pohjolainen wrote:
> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> ---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 108dd87..0edd59f 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -64,8 +64,11 @@ intel_miptree_alloc_mcs(struct brw_context *brw,
>   */
>  static enum intel_msaa_layout
>  compute_msaa_layout(struct brw_context *brw, mesa_format format,
> -                    bool disable_aux_buffers)
> +                    unsigned num_samples, bool disable_aux_buffers)
>  {
> +   if (num_samples <= 1)
> +      return INTEL_MSAA_LAYOUT_NONE;
> +
>     /* Prior to Gen7, all MSAA surfaces used IMS layout. */
>     if (brw->gen < 7)
>        return INTEL_MSAA_LAYOUT_IMS;
> @@ -299,6 +302,7 @@ intel_miptree_create_layout(struct brw_context *brw,
>                              GLuint height0,
>                              GLuint depth0,
>                              GLuint num_samples,
> +                            enum intel_msaa_layout msaa_layout,

Is there a reason why you decided not to roll this into layout flags? It seems
to fit into that pretty well IMO.

>                              uint32_t layout_flags)
>  {
>     struct intel_mipmap_tree *mt = calloc(sizeof(*mt), 1);
> @@ -343,13 +347,11 @@ intel_miptree_create_layout(struct brw_context *brw,
>     mt->cpp = _mesa_get_format_bytes(format);
>     mt->num_samples = num_samples;
>     mt->compressed = _mesa_is_format_compressed(format);
> -   mt->msaa_layout = INTEL_MSAA_LAYOUT_NONE;
> +   mt->msaa_layout = msaa_layout;
>     mt->refcount = 1;
>  
>     if (num_samples > 1) {
>        /* Adjust width/height/depth for MSAA */
> -      mt->msaa_layout = compute_msaa_layout(brw, format,
> -                                            mt->disable_aux_buffers);
>        if (mt->msaa_layout == INTEL_MSAA_LAYOUT_IMS) {
>           /* From the Ivybridge PRM, Volume 1, Part 1, page 108:
>            * "If the surface is multisampled and it is a depth or stencil
> @@ -636,6 +638,8 @@ intel_miptree_create(struct brw_context *brw,
>     mt = intel_miptree_create_layout(brw, target, format,
>                                      first_level, last_level, width0,
>                                      height0, depth0, num_samples,
> +                                    compute_msaa_layout(brw, format,
> +                                                        num_samples, false),
>                                      layout_flags);
>     /*
>      * pitch == 0 || height == 0  indicates the null texture
> @@ -743,6 +747,7 @@ intel_miptree_create_for_bo(struct brw_context *brw,
>     struct intel_mipmap_tree *mt;
>     uint32_t tiling, swizzle;
>     GLenum target;
> +   const bool disable_aux_buffers = layout_flags & MIPTREE_LAYOUT_DISABLE_AUX;
>  
>     drm_intel_bo_get_tiling(bo, &tiling, &swizzle);
>  
> @@ -769,6 +774,8 @@ intel_miptree_create_for_bo(struct brw_context *brw,
>     mt = intel_miptree_create_layout(brw, target, format,
>                                      0, 0,
>                                      width, height, depth, 0,
> +                                    compute_msaa_layout(brw, format, 0,
> +                                                        disable_aux_buffers),

If you roll in to layout flags, you just pass layout flags to
compute_msaa_layout()

>                                      layout_flags);
>     if (!mt)
>        return NULL;

Patch makes sense to me though, and I don't spot anything wrong with the
mechanics. I'd prefer you use layout flags, but either way it's:
Reviewed-by: Ben Widawsky <benjamin.widawsky at intel.com>