Re: [Mesa-dev] [PATCH 01/23] i965: Let caller of intel_miptree_create_layout() decide msaa layout

2016-02-10 Thread Ben Widawsky 
On Wed, Feb 10, 2016 at 10:27:46AM +0200, Pohjolainen, Topi wrote:
> On Tue, Feb 09, 2016 at 12:05:52PM -0800, Ben Widawsky wrote:
> > On Mon, Feb 08, 2016 at 06:51:21PM +0200, Topi Pohjolainen wrote:
> > > Signed-off-by: Topi Pohjolainen 
> > > ---
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 15 +++
> > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
> > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > index 108dd87..0edd59f 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > @@ -64,8 +64,11 @@ intel_miptree_alloc_mcs(struct brw_context *brw,
> > >   */
> > >  static enum intel_msaa_layout
> > >  compute_msaa_layout(struct brw_context *brw, mesa_format format,
> > > -bool disable_aux_buffers)
> > > +unsigned num_samples, bool disable_aux_buffers)
> > >  {
> > > +   if (num_samples <= 1)
> > > +  return INTEL_MSAA_LAYOUT_NONE;
> > > +
> > > /* Prior to Gen7, all MSAA surfaces used IMS layout. */
> > > if (brw->gen < 7)
> > >return INTEL_MSAA_LAYOUT_IMS;
> > > @@ -299,6 +302,7 @@ intel_miptree_create_layout(struct brw_context *brw,
> > >  GLuint height0,
> > >  GLuint depth0,
> > >  GLuint num_samples,
> > > +enum intel_msaa_layout msaa_layout,
> > 
> > Is there a reason why you decided not to roll this into layout flags? It 
> > seems
> > to fit into that pretty well IMO.
> 
> If I understood you correctly, you would like to pass
> 
> enum intel_msaa_layout
> {
>INTEL_MSAA_LAYOUT_NONE,
>INTEL_MSAA_LAYOUT_IMS,
>INTEL_MSAA_LAYOUT_UMS,
>INTEL_MSAA_LAYOUT_CMS,
> };
> 
> along with
> 
> enum {
>MIPTREE_LAYOUT_ACCELERATED_UPLOAD   = 1 << 0,
>MIPTREE_LAYOUT_FORCE_ALL_SLICE_AT_LOD   = 1 << 1,
>MIPTREE_LAYOUT_FOR_BO   = 1 << 2,
>MIPTREE_LAYOUT_DISABLE_AUX  = 1 << 3,
>MIPTREE_LAYOUT_FORCE_HALIGN16   = 1 << 4,
> 
>MIPTREE_LAYOUT_TILING_Y = 1 << 5,
>MIPTREE_LAYOUT_TILING_NONE  = 1 << 6,
>MIPTREE_LAYOUT_TILING_ANY   = MIPTREE_LAYOUT_TILING_Y |
>  MIPTREE_LAYOUT_TILING_NONE,
> };
> 
> in the same uint32_t. Values are conflicting and therefore one of the
> enumerations would need to re-introduce the other with non-conflicting
> values. Or did I misunderstand?

Well, I was thinking of introducing the minimum subset of whatever weird layout
you need to enforce. My assumption at the time was you're passing this info down
so that the function doing the actual layout could act up the type of MSAA. My
though is to keep the actual "layout" code as agnostic to the type of miptree,
only on the parameters provided via the flags. In that case, I think it makes
sense to create a new layout flag. For example...

MIPTREE_LAYOUT_WACKY_MSAA_THING  = 1 << 7
mt->msaa_layout = compute_msaa_layout(...)
if (mt->msaa_layout == whatever)
layout_flags |= MIPTREE_LAYOUT_WACKY_MSAA_THING;
intel_miptree_create_layout(...)

After getting through about half of the patches, it seems like maybe you are
just saving the caller from mt->msaa = compute_msaa_layout(). If that's the
case, that's fine. However, I'm sort of wondering if this works correcting is
MIPTREE_LAYOUT_DISABLE_AUX is specified. It kind of seems like we could hit the
assert with this patch alone.

I think you'd need to remove the assertion completely
   if (mt->disable_aux_buffers)
 assert(mt->msaa_layout != INTEL_MSAA_LAYOUT_CMS)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 01/23] i965: Let caller of intel_miptree_create_layout() decide msaa layout

2016-02-10 Thread Pohjolainen, Topi 
On Tue, Feb 09, 2016 at 12:05:52PM -0800, Ben Widawsky wrote:
> On Mon, Feb 08, 2016 at 06:51:21PM +0200, Topi Pohjolainen wrote:
> > Signed-off-by: Topi Pohjolainen 
> > ---
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 15 +++
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
> > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 108dd87..0edd59f 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -64,8 +64,11 @@ intel_miptree_alloc_mcs(struct brw_context *brw,
> >   */
> >  static enum intel_msaa_layout
> >  compute_msaa_layout(struct brw_context *brw, mesa_format format,
> > -bool disable_aux_buffers)
> > +unsigned num_samples, bool disable_aux_buffers)
> >  {
> > +   if (num_samples <= 1)
> > +  return INTEL_MSAA_LAYOUT_NONE;
> > +
> > /* Prior to Gen7, all MSAA surfaces used IMS layout. */
> > if (brw->gen < 7)
> >return INTEL_MSAA_LAYOUT_IMS;
> > @@ -299,6 +302,7 @@ intel_miptree_create_layout(struct brw_context *brw,
> >  GLuint height0,
> >  GLuint depth0,
> >  GLuint num_samples,
> > +enum intel_msaa_layout msaa_layout,
> 
> Is there a reason why you decided not to roll this into layout flags? It seems
> to fit into that pretty well IMO.

If I understood you correctly, you would like to pass

enum intel_msaa_layout
{
   INTEL_MSAA_LAYOUT_NONE,
   INTEL_MSAA_LAYOUT_IMS,
   INTEL_MSAA_LAYOUT_UMS,
   INTEL_MSAA_LAYOUT_CMS,
};

along with

enum {
   MIPTREE_LAYOUT_ACCELERATED_UPLOAD   = 1 << 0,
   MIPTREE_LAYOUT_FORCE_ALL_SLICE_AT_LOD   = 1 << 1,
   MIPTREE_LAYOUT_FOR_BO   = 1 << 2,
   MIPTREE_LAYOUT_DISABLE_AUX  = 1 << 3,
   MIPTREE_LAYOUT_FORCE_HALIGN16   = 1 << 4,

   MIPTREE_LAYOUT_TILING_Y = 1 << 5,
   MIPTREE_LAYOUT_TILING_NONE  = 1 << 6,
   MIPTREE_LAYOUT_TILING_ANY   = MIPTREE_LAYOUT_TILING_Y |
 MIPTREE_LAYOUT_TILING_NONE,
};

in the same uint32_t. Values are conflicting and therefore one of the
enumerations would need to re-introduce the other with non-conflicting
values. Or did I misunderstand?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 01/23] i965: Let caller of intel_miptree_create_layout() decide msaa layout

2016-02-09 Thread Ben Widawsky 
On Mon, Feb 08, 2016 at 06:51:21PM +0200, Topi Pohjolainen wrote:
> Signed-off-by: Topi Pohjolainen 
> ---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 108dd87..0edd59f 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -64,8 +64,11 @@ intel_miptree_alloc_mcs(struct brw_context *brw,
>   */
>  static enum intel_msaa_layout
>  compute_msaa_layout(struct brw_context *brw, mesa_format format,
> -bool disable_aux_buffers)
> +unsigned num_samples, bool disable_aux_buffers)
>  {
> +   if (num_samples <= 1)
> +  return INTEL_MSAA_LAYOUT_NONE;
> +
> /* Prior to Gen7, all MSAA surfaces used IMS layout. */
> if (brw->gen < 7)
>return INTEL_MSAA_LAYOUT_IMS;
> @@ -299,6 +302,7 @@ intel_miptree_create_layout(struct brw_context *brw,
>  GLuint height0,
>  GLuint depth0,
>  GLuint num_samples,
> +enum intel_msaa_layout msaa_layout,

Is there a reason why you decided not to roll this into layout flags? It seems
to fit into that pretty well IMO.

>  uint32_t layout_flags)
>  {
> struct intel_mipmap_tree *mt = calloc(sizeof(*mt), 1);
> @@ -343,13 +347,11 @@ intel_miptree_create_layout(struct brw_context *brw,
> mt->cpp = _mesa_get_format_bytes(format);
> mt->num_samples = num_samples;
> mt->compressed = _mesa_is_format_compressed(format);
> -   mt->msaa_layout = INTEL_MSAA_LAYOUT_NONE;
> +   mt->msaa_layout = msaa_layout;
> mt->refcount = 1;
>  
> if (num_samples > 1) {
>/* Adjust width/height/depth for MSAA */
> -  mt->msaa_layout = compute_msaa_layout(brw, format,
> -mt->disable_aux_buffers);
>if (mt->msaa_layout == INTEL_MSAA_LAYOUT_IMS) {
>   /* From the Ivybridge PRM, Volume 1, Part 1, page 108:
>* "If the surface is multisampled and it is a depth or stencil
> @@ -636,6 +638,8 @@ 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);
> /*
>  * pitch == 0 || height == 0  indicates the null texture
> @@ -743,6 +747,7 @@ intel_miptree_create_for_bo(struct brw_context *brw,
> struct intel_mipmap_tree *mt;
> uint32_t tiling, swizzle;
> GLenum target;
> +   const bool disable_aux_buffers = layout_flags & 
> MIPTREE_LAYOUT_DISABLE_AUX;
>  
> drm_intel_bo_get_tiling(bo, , );
>  
> @@ -769,6 +774,8 @@ intel_miptree_create_for_bo(struct brw_context *brw,
> mt = intel_miptree_create_layout(brw, target, format,
>  0, 0,
>  width, height, depth, 0,
> +compute_msaa_layout(brw, format, 0,
> +disable_aux_buffers),

If you roll in to layout flags, you just pass layout flags to
compute_msaa_layout()

>  layout_flags);
> if (!mt)
>return NULL;

Patch makes sense to me though, and I don't spot anything wrong with the
mechanics. I'd prefer you use layout flags, but either way it's:
Reviewed-by: Ben Widawsky 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 01/23] i965: Let caller of intel_miptree_create_layout() decide msaa layout

2016-02-08 Thread Topi Pohjolainen 
Signed-off-by: Topi Pohjolainen 
---
 src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 108dd87..0edd59f 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -64,8 +64,11 @@ intel_miptree_alloc_mcs(struct brw_context *brw,
  */
 static enum intel_msaa_layout
 compute_msaa_layout(struct brw_context *brw, mesa_format format,
-bool disable_aux_buffers)
+unsigned num_samples, bool disable_aux_buffers)
 {
+   if (num_samples <= 1)
+  return INTEL_MSAA_LAYOUT_NONE;
+
/* Prior to Gen7, all MSAA surfaces used IMS layout. */
if (brw->gen < 7)
   return INTEL_MSAA_LAYOUT_IMS;
@@ -299,6 +302,7 @@ intel_miptree_create_layout(struct brw_context *brw,
 GLuint height0,
 GLuint depth0,
 GLuint num_samples,
+enum intel_msaa_layout msaa_layout,
 uint32_t layout_flags)
 {
struct intel_mipmap_tree *mt = calloc(sizeof(*mt), 1);
@@ -343,13 +347,11 @@ intel_miptree_create_layout(struct brw_context *brw,
mt->cpp = _mesa_get_format_bytes(format);
mt->num_samples = num_samples;
mt->compressed = _mesa_is_format_compressed(format);
-   mt->msaa_layout = INTEL_MSAA_LAYOUT_NONE;
+   mt->msaa_layout = msaa_layout;
mt->refcount = 1;
 
if (num_samples > 1) {
   /* Adjust width/height/depth for MSAA */
-  mt->msaa_layout = compute_msaa_layout(brw, format,
-mt->disable_aux_buffers);
   if (mt->msaa_layout == INTEL_MSAA_LAYOUT_IMS) {
  /* From the Ivybridge PRM, Volume 1, Part 1, page 108:
   * "If the surface is multisampled and it is a depth or stencil
@@ -636,6 +638,8 @@ 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);
/*
 * pitch == 0 || height == 0  indicates the null texture
@@ -743,6 +747,7 @@ intel_miptree_create_for_bo(struct brw_context *brw,
struct intel_mipmap_tree *mt;
uint32_t tiling, swizzle;
GLenum target;
+   const bool disable_aux_buffers = layout_flags & MIPTREE_LAYOUT_DISABLE_AUX;
 
drm_intel_bo_get_tiling(bo, , );
 
@@ -769,6 +774,8 @@ intel_miptree_create_for_bo(struct brw_context *brw,
mt = intel_miptree_create_layout(brw, target, format,
 0, 0,
 width, height, depth, 0,
+compute_msaa_layout(brw, format, 0,
+disable_aux_buffers),
 layout_flags);
if (!mt)
   return NULL;
-- 
2.5.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev