On Tue, Feb 09, 2016 at 03:05:05PM -0800, Ben Widawsky wrote:
> 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/
Right, the logic below reconsiders TILING_X even though the caller overwrote
it to TILING_Y. Your patch addresses this, I will update this accordingly.
>
> > ---
> > 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;
Oops, you are correct.
>
> > 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.
I tried to keep the changes minimal, but makes sense.
Thanks for the careful review!
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
