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.pohjolai...@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.widaw...@intel.com>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
