Re: [Mesa-dev] [PATCH 03/23] i965: Separate miptree creation from auxiliary buffer setup

[Pohjolainen, Topi]

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 
> 
> 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();
> > +  return NULL;
> > +   }
> > +
> > +   return mt;
> > +}
> > +
> > +struct intel_mipmap_tree *
> > +intel_miptree_create(struct brw_context *brw,
> > + GLenum target,
> > + mesa_format format,
> > +  

Re: [Mesa-dev] [PATCH 03/23] i965: Separate miptree creation from auxiliary buffer setup

[Pohjolainen, Topi]

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 
> 
> 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();
> > +  return NULL;
> > +   }
> > +
> > +   return mt;
> > +}
> > +
> > +struct 

Re: [Mesa-dev] [PATCH 03/23] i965: Separate miptree creation from auxiliary buffer setup

[Ben Widawsky]

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 

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();
> +  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)
> +{

[Mesa-dev] [PATCH 03/23] i965: Separate miptree creation from auxiliary buffer setup

[Topi Pohjolainen]

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 
---
 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();
+  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