On Mon, Feb 08, 2016 at 06:51:23PM +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. > > Signed-off-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
Shameless plug. I think we reached the same conclusion about the existing code :-). I forgot what I actually did, but I'm pretty sure I at least did this stuff, and maybe some more. I'd love it if you could see if anything is useful there. https://patchwork.freedesktop.org/patch/56792/ > --- > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 75 > +++++++++++++++++---------- > 1 file changed, 49 insertions(+), 26 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > index 033f4c6..d655de8 100644 > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > @@ -611,17 +611,18 @@ 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, > + enum intel_msaa_layout msaa_layout, > + uint32_t layout_flags) > { > struct intel_mipmap_tree *mt; > mesa_format tex_format = format; > @@ -638,9 +639,7 @@ 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); > + msaa_layout, layout_flags); > /* > * pitch == 0 || height == 0 indicates the null texture > */ > @@ -658,12 +657,8 @@ intel_miptree_create(struct brw_context *brw, > total_height = ALIGN(total_height, 64); > } > > - 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; > @@ -686,12 +681,46 @@ intel_miptree_create(struct brw_context *brw, > } > > mt->pitch = pitch; > + mt->offset = 0; > + > + if (!mt->bo) { > + intel_miptree_release(&mt); > + return NULL; > + } > + > + 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, > + compute_msaa_layout(brw, format, > + num_samples, false), > + 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. > */ > + const bool y_or_x = mt->tiling == (I915_TILING_Y | I915_TILING_X); > if (brw->gen < 6 && y_or_x && mt->bo->size >= > brw->max_gtt_map_object_size) { > + unsigned long pitch = mt->pitch; > + const uint32_t alloc_flags = > + (layout_flags & MIPTREE_LAYOUT_ACCELERATED_UPLOAD) ? > + BO_ALLOC_FOR_RENDER : 0; mt may be NULL if miptree_create fails. SO I think you need an if (!mt) return NULL; > perf_debug("%dx%d miptree larger than aperture; falling back to > X-tiled\n", > mt->total_width, mt->total_height); > > @@ -700,14 +729,8 @@ intel_miptree_create(struct brw_context *brw, > mt->bo = drm_intel_bo_alloc_tiled(brw->bufmgr, "miptree", > mt->total_width, mt->total_height, mt->cpp, > &mt->tiling, &pitch, alloc_flags); > - mt->pitch = pitch; > - } > - > - mt->offset = 0; > > - if (!mt->bo) { > - intel_miptree_release(&mt); > - return NULL; > + mt->pitch = pitch; You could get rid of pitch local variable and just pass &mt->pitch, if you want. > } > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
