[Mesa-dev] [v2 02/19] i965: Separate miptree creation from auxiliary buffer setup

[Ben Widawsky]

On Thu, Feb 11, 2016 at 08:33:55PM +0200, Topi Pohjolainen wrote:
> Currently the logic allocating and setting up miptrees is closely
> combined with decision making when to re-allocate buffers in
> X-tiled layout and when to associate colors with auxiliary buffers.
> 
> These auxiliary buffers are in turn also represented as miptrees
> and are created by the same miptree creation logic calling itself
> recursively. This means considering in vain if the auxiliary buffers
> should be represented in X-tiled layout or if they should be
> associated with auxiliary buffers again.
> While this is somewhat unnecessary, this doesn't impose any problems
> currently. Miptrees for auxiliary buffers are created as simgle-sampled
> fusing the consideration for multi-sampled compression auxiliary
> buffers. The format in turn is such that is not applicable for
> single-sampled fast clears (that would require accompaning auxiliary
> buffer).
> But once the driver starts to support lossless compression of color
> buffers the auxiliary buffer will have a format that would itself
> be applicable for lossless compression. This would be rather
> difficult and ugly to detect in the current miptree creation logic,
> and therefore this patch seeks to separate the association logic
> from the general allocation and setup steps.
> 
> v2 (Ben):
>    - Do not reconsider for X-tiling in intel_miptree_create()
>      as it was just forced to Y-tiling in miptree_create().
>    - Do not drop checks for allocation failures.
> 
> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> ---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 56 +++++++++++++++++++--------
>  1 file changed, 39 insertions(+), 17 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 97dd3d9..e8b3116 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -609,17 +609,17 @@ intel_get_yf_ys_bo_size(struct intel_mipmap_tree *mt, unsigned *alignment,
>     return size;
>  }
>  
> -struct intel_mipmap_tree *
> -intel_miptree_create(struct brw_context *brw,
> -                     GLenum target,
> -                     mesa_format format,
> -                     GLuint first_level,
> -                     GLuint last_level,
> -                     GLuint width0,
> -                     GLuint height0,
> -                     GLuint depth0,
> -                     GLuint num_samples,
> -                     uint32_t layout_flags)
> +static struct intel_mipmap_tree *
> +miptree_create(struct brw_context *brw,
> +               GLenum target,
> +               mesa_format format,
> +               GLuint first_level,
> +               GLuint last_level,
> +               GLuint width0,
> +               GLuint height0,
> +               GLuint depth0,
> +               GLuint num_samples,
> +               uint32_t layout_flags)
>  {
>     struct intel_mipmap_tree *mt;
>     mesa_format tex_format = format;
> @@ -644,12 +644,8 @@ intel_miptree_create(struct brw_context *brw,
>        return NULL;
>     }
>  
> -   bool y_or_x = false;
> -
> -   if (mt->tiling == (I915_TILING_Y | I915_TILING_X)) {
> -      y_or_x = true;
> +   if (mt->tiling == (I915_TILING_Y | I915_TILING_X))
>        mt->tiling = I915_TILING_Y;
> -   }
>  
>     if (layout_flags & MIPTREE_LAYOUT_ACCELERATED_UPLOAD)
>        alloc_flags |= BO_ALLOC_FOR_RENDER;
> @@ -682,11 +678,37 @@ intel_miptree_create(struct brw_context *brw,
>  
>     mt->pitch = pitch;
>  
> +   return mt;
> +}
> +
> +struct intel_mipmap_tree *
> +intel_miptree_create(struct brw_context *brw,
> +                     GLenum target,
> +                     mesa_format format,
> +                     GLuint first_level,
> +                     GLuint last_level,
> +                     GLuint width0,
> +                     GLuint height0,
> +                     GLuint depth0,
> +                     GLuint num_samples,
> +                     uint32_t layout_flags)
> +{
> +   struct intel_mipmap_tree *mt = miptree_create(
> +                                     brw, target, format,
> +                                     first_level, last_level,
> +                                     width0, height0, depth0, num_samples,
> +                                     layout_flags);
> +
>     /* If the BO is too large to fit in the aperture, we need to use the
>      * BLT engine to support it.  Prior to Sandybridge, the BLT paths can't
>      * handle Y-tiling, so we need to fall back to X.
>      */
> -   if (brw->gen < 6 && y_or_x && mt->bo->size >= brw->max_gtt_map_object_size) {
> +   if (brw->gen < 6 && mt->bo->size >= brw->max_gtt_map_object_size &&
> +       mt->tiling == I915_TILING_Y) {
> +      unsigned long pitch = mt->pitch;
> +      const uint32_t alloc_flags =
> +         (layout_flags & MIPTREE_LAYOUT_ACCELERATED_UPLOAD) ?
> +         BO_ALLOC_FOR_RENDER : 0;
>        perf_debug("%dx%d miptree larger than aperture; falling back to X-tiled\n",
>                   mt->total_width, mt->total_height);

I still dislike this code, but it's a big improvement. I primarily dislike the
disjoint alloc_flags settings. I spent a bit trying to rework this to come out
better than you have, and I was not successful.

I also sort of question whether the perf_debug message is worth keeping. There
is just nothing that can be done in this case - but I am not super familiar with
the rules for what qualifies for perf_debug (in my head, it's stuff we, or an
app developer, could realistically fix). I guess the app could use smaller
buffers...

Reviewed-by: Ben Widawsky <benjamin.widawsky at intel.com>