Re: [Mesa-dev] [PATCH] i965/miptree: Initialize mcs buffer only until clear color

2018-04-10 Thread Nanley Chery 
On Fri, Apr 06, 2018 at 07:04:01PM +0300, Pohjolainen, Topi wrote:
> On Fri, Apr 06, 2018 at 08:53:39AM -0700, Jason Ekstrand wrote:
> > On Fri, Apr 6, 2018 at 8:22 AM, Rafael Antognolli <
> > rafael.antogno...@intel.com> wrote:
> > 
> > > On Fri, Apr 06, 2018 at 06:07:52PM +0300, Topi Pohjolainen wrote:
> > > > Otherwise even the clear color gets initialised to 0xFF. This
> > > > allows enabling of color fast clears on ICL without regressing
> > > > multisampling tests.
> > > >
> > > > CC: Rafael Antognolli 
> > > > CC: Jason Ekstrand 
> > > > CC: Nanley Chery 
> > > > Signed-off-by: Topi Pohjolainen 
> > > > ---
> > > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 ++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > index 89074a6..25f901d 100644
> > > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > @@ -1680,7 +1680,12 @@ intel_miptree_init_mcs(struct brw_context *brw,
> > > >return;
> > > > }
> > > > void *data = map;
> > > > -   memset(data, init_value, mt->mcs_buf->size);
> > > > +
> > > > +   /* Only initialize until clear color (if present). */
> > > > +   const unsigned aux_size = mt->mcs_buf->clear_color_offset ?
> > > > +mt->mcs_buf->clear_color_offset :
> > > > +mt->mcs_buf->size;
> > > > +   memset(data, init_value, aux_size);
> > >
> > 
> > Why not just use mt->mcs_buf->aux_surf.size?
> > 
> > Also, I think we probably want to memset the clear color to 0 in case we
> > get a recycled BO with unknown garbage in the clear value.
> 
> Good thinking, both points.
> 

I also agree with those points.

-Nanley

> > 
> > 
> > > Hmm... that's a good catch, and I think we definitely should not
> > > overwrite the clear color here.
> > >
> > > However, the initial value of the clear color shouldn't matter, right? I
> > > think there might still be a bug hidden somewhere...
> 
> I agree. I started to look into MCS in more detail - I don't think I fully
> understand how the clear color works there.
> 
> > >
> > > Regardless of that, this patch is
> > >
> > > Reviewed-by: Rafael Antognolli 
> > >
> > > > brw_bo_unmap(mt->mcs_buf->bo);
> > > >  }
> > > >
> > > > --
> > > > 2.7.4
> > > >
> > >
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965/miptree: Initialize mcs buffer only until clear color

2018-04-06 Thread Pohjolainen, Topi 
On Fri, Apr 06, 2018 at 08:53:39AM -0700, Jason Ekstrand wrote:
> On Fri, Apr 6, 2018 at 8:22 AM, Rafael Antognolli <
> rafael.antogno...@intel.com> wrote:
> 
> > On Fri, Apr 06, 2018 at 06:07:52PM +0300, Topi Pohjolainen wrote:
> > > Otherwise even the clear color gets initialised to 0xFF. This
> > > allows enabling of color fast clears on ICL without regressing
> > > multisampling tests.
> > >
> > > CC: Rafael Antognolli 
> > > CC: Jason Ekstrand 
> > > CC: Nanley Chery 
> > > Signed-off-by: Topi Pohjolainen 
> > > ---
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 ++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > index 89074a6..25f901d 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > @@ -1680,7 +1680,12 @@ intel_miptree_init_mcs(struct brw_context *brw,
> > >return;
> > > }
> > > void *data = map;
> > > -   memset(data, init_value, mt->mcs_buf->size);
> > > +
> > > +   /* Only initialize until clear color (if present). */
> > > +   const unsigned aux_size = mt->mcs_buf->clear_color_offset ?
> > > +mt->mcs_buf->clear_color_offset :
> > > +mt->mcs_buf->size;
> > > +   memset(data, init_value, aux_size);
> >
> 
> Why not just use mt->mcs_buf->aux_surf.size?
> 
> Also, I think we probably want to memset the clear color to 0 in case we
> get a recycled BO with unknown garbage in the clear value.

Good thinking, both points.

> 
> 
> > Hmm... that's a good catch, and I think we definitely should not
> > overwrite the clear color here.
> >
> > However, the initial value of the clear color shouldn't matter, right? I
> > think there might still be a bug hidden somewhere...

I agree. I started to look into MCS in more detail - I don't think I fully
understand how the clear color works there.

> >
> > Regardless of that, this patch is
> >
> > Reviewed-by: Rafael Antognolli 
> >
> > > brw_bo_unmap(mt->mcs_buf->bo);
> > >  }
> > >
> > > --
> > > 2.7.4
> > >
> >
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965/miptree: Initialize mcs buffer only until clear color

2018-04-06 Thread Jason Ekstrand 
On Fri, Apr 6, 2018 at 8:22 AM, Rafael Antognolli <
rafael.antogno...@intel.com> wrote:

> On Fri, Apr 06, 2018 at 06:07:52PM +0300, Topi Pohjolainen wrote:
> > Otherwise even the clear color gets initialised to 0xFF. This
> > allows enabling of color fast clears on ICL without regressing
> > multisampling tests.
> >
> > CC: Rafael Antognolli 
> > CC: Jason Ekstrand 
> > CC: Nanley Chery 
> > Signed-off-by: Topi Pohjolainen 
> > ---
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 ++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 89074a6..25f901d 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -1680,7 +1680,12 @@ intel_miptree_init_mcs(struct brw_context *brw,
> >return;
> > }
> > void *data = map;
> > -   memset(data, init_value, mt->mcs_buf->size);
> > +
> > +   /* Only initialize until clear color (if present). */
> > +   const unsigned aux_size = mt->mcs_buf->clear_color_offset ?
> > +mt->mcs_buf->clear_color_offset :
> > +mt->mcs_buf->size;
> > +   memset(data, init_value, aux_size);
>

Why not just use mt->mcs_buf->aux_surf.size?

Also, I think we probably want to memset the clear color to 0 in case we
get a recycled BO with unknown garbage in the clear value.


> Hmm... that's a good catch, and I think we definitely should not
> overwrite the clear color here.
>
> However, the initial value of the clear color shouldn't matter, right? I
> think there might still be a bug hidden somewhere...
>
> Regardless of that, this patch is
>
> Reviewed-by: Rafael Antognolli 
>
> > brw_bo_unmap(mt->mcs_buf->bo);
> >  }
> >
> > --
> > 2.7.4
> >
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965/miptree: Initialize mcs buffer only until clear color

2018-04-06 Thread Rafael Antognolli 
On Fri, Apr 06, 2018 at 06:07:52PM +0300, Topi Pohjolainen wrote:
> Otherwise even the clear color gets initialised to 0xFF. This
> allows enabling of color fast clears on ICL without regressing
> multisampling tests.
> 
> CC: Rafael Antognolli 
> CC: Jason Ekstrand 
> CC: Nanley Chery 
> Signed-off-by: Topi Pohjolainen 
> ---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 89074a6..25f901d 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -1680,7 +1680,12 @@ intel_miptree_init_mcs(struct brw_context *brw,
>return;
> }
> void *data = map;
> -   memset(data, init_value, mt->mcs_buf->size);
> +
> +   /* Only initialize until clear color (if present). */
> +   const unsigned aux_size = mt->mcs_buf->clear_color_offset ?
> +mt->mcs_buf->clear_color_offset :
> +mt->mcs_buf->size;
> +   memset(data, init_value, aux_size);

Hmm... that's a good catch, and I think we definitely should not
overwrite the clear color here.

However, the initial value of the clear color shouldn't matter, right? I
think there might still be a bug hidden somewhere...

Regardless of that, this patch is

Reviewed-by: Rafael Antognolli 

> brw_bo_unmap(mt->mcs_buf->bo);
>  }
>  
> -- 
> 2.7.4
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] i965/miptree: Initialize mcs buffer only until clear color

2018-04-06 Thread Topi Pohjolainen 
Otherwise even the clear color gets initialised to 0xFF. This
allows enabling of color fast clears on ICL without regressing
multisampling tests.

CC: Rafael Antognolli 
CC: Jason Ekstrand 
CC: Nanley Chery 
Signed-off-by: Topi Pohjolainen 
---
 src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 89074a6..25f901d 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -1680,7 +1680,12 @@ intel_miptree_init_mcs(struct brw_context *brw,
   return;
}
void *data = map;
-   memset(data, init_value, mt->mcs_buf->size);
+
+   /* Only initialize until clear color (if present). */
+   const unsigned aux_size = mt->mcs_buf->clear_color_offset ?
+mt->mcs_buf->clear_color_offset :
+mt->mcs_buf->size;
+   memset(data, init_value, aux_size);
brw_bo_unmap(mt->mcs_buf->bo);
 }
 
-- 
2.7.4

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