Re: [Mesa-dev] [PATCH 5/8] i965/miptree: Add space to store the clear value in the aux surface.

[Rafael Antognolli]

On Mon, Jan 08, 2018 at 03:14:54PM -0800, Nanley Chery wrote:
> On Fri, Dec 15, 2017 at 02:53:32PM -0800, Rafael Antognolli wrote:
> > Similarly to vulkan where we store the clear value in the aux surface,
> > we can do the same in GL.
> > 
> > Signed-off-by: Rafael Antognolli 
> > ---
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 25 
> > +
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
> > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index ead0c359c0f..6400a2a616a 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -1663,6 +1663,21 @@ intel_miptree_init_mcs(struct brw_context *brw,
> > brw_bo_unmap(mt->mcs_buf->bo);
> >  }
> >  
> > +static unsigned
> > +fast_clear_state_entry_size(const struct brw_context *brw)
> > +{
> > +   assert(brw);
> > +
> > +   /* Entry contents:
> > +*   ++
> > +*   | clear value dword(s)   |
> > +*   ++
> > +*/
> > +   assert(brw->isl_dev.ss.clear_value_size % 4 == 0);
> > +
> > +   return brw->isl_dev.ss.clear_value_size;
> > +}
> > +
> 
> Do you think more may fields may be added to this in the future? I'm
> trying to understand why we have an additional function here.

No, I just tried to make it similar to anv, but anv at least has a
reason to have such function. I'll remove it in the next iteration.

> >  static struct intel_miptree_aux_buffer *
> >  intel_alloc_aux_buffer(struct brw_context *brw,
> > const char *name,
> > @@ -1675,6 +1690,16 @@ intel_alloc_aux_buffer(struct brw_context *brw,
> >return false;
> >  
> > buf->size = aux_surf->size;
> > +
> > +   const struct gen_device_info *devinfo = >screen->devinfo;
> > +   if (devinfo->gen >= 10) {
> > +  /* On CNL, instead of setting the clear color in the SURFACE_STATE, 
> > we
> > +   * will set a pointer to a dword somewhere that contains the color. 
> > So,
> > +   * allocate the space for the clear color value here on the aux 
> > buffer.
> > +   */
> > +  buf->size += fast_clear_state_entry_size(brw);
> 
> I like the plan of allocating more space here.
> 
> > +   }
> > +
> > buf->pitch = aux_surf->row_pitch;
> > buf->qpitch = isl_surf_get_array_pitch_sa_rows(aux_surf);
> >  
> > -- 
> > 2.14.3
> > 
> > ___
> > 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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 5/8] i965/miptree: Add space to store the clear value in the aux surface.

[Nanley Chery]

On Fri, Dec 15, 2017 at 02:53:32PM -0800, Rafael Antognolli wrote:
> Similarly to vulkan where we store the clear value in the aux surface,
> we can do the same in GL.
> 
> Signed-off-by: Rafael Antognolli 
> ---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 25 +
>  1 file changed, 25 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index ead0c359c0f..6400a2a616a 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -1663,6 +1663,21 @@ intel_miptree_init_mcs(struct brw_context *brw,
> brw_bo_unmap(mt->mcs_buf->bo);
>  }
>  
> +static unsigned
> +fast_clear_state_entry_size(const struct brw_context *brw)
> +{
> +   assert(brw);
> +
> +   /* Entry contents:
> +*   ++
> +*   | clear value dword(s)   |
> +*   ++
> +*/
> +   assert(brw->isl_dev.ss.clear_value_size % 4 == 0);
> +
> +   return brw->isl_dev.ss.clear_value_size;
> +}
> +

Do you think more may fields may be added to this in the future? I'm
trying to understand why we have an additional function here.

>  static struct intel_miptree_aux_buffer *
>  intel_alloc_aux_buffer(struct brw_context *brw,
> const char *name,
> @@ -1675,6 +1690,16 @@ intel_alloc_aux_buffer(struct brw_context *brw,
>return false;
>  
> buf->size = aux_surf->size;
> +
> +   const struct gen_device_info *devinfo = >screen->devinfo;
> +   if (devinfo->gen >= 10) {
> +  /* On CNL, instead of setting the clear color in the SURFACE_STATE, we
> +   * will set a pointer to a dword somewhere that contains the color. So,
> +   * allocate the space for the clear color value here on the aux buffer.
> +   */
> +  buf->size += fast_clear_state_entry_size(brw);

I like the plan of allocating more space here.

> +   }
> +
> buf->pitch = aux_surf->row_pitch;
> buf->qpitch = isl_surf_get_array_pitch_sa_rows(aux_surf);
>  
> -- 
> 2.14.3
> 
> ___
> 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

[Mesa-dev] [PATCH 5/8] i965/miptree: Add space to store the clear value in the aux surface.

[Rafael Antognolli]

Similarly to vulkan where we store the clear value in the aux surface,
we can do the same in GL.

Signed-off-by: Rafael Antognolli 
---
 src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index ead0c359c0f..6400a2a616a 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -1663,6 +1663,21 @@ intel_miptree_init_mcs(struct brw_context *brw,
brw_bo_unmap(mt->mcs_buf->bo);
 }
 
+static unsigned
+fast_clear_state_entry_size(const struct brw_context *brw)
+{
+   assert(brw);
+
+   /* Entry contents:
+*   ++
+*   | clear value dword(s)   |
+*   ++
+*/
+   assert(brw->isl_dev.ss.clear_value_size % 4 == 0);
+
+   return brw->isl_dev.ss.clear_value_size;
+}
+
 static struct intel_miptree_aux_buffer *
 intel_alloc_aux_buffer(struct brw_context *brw,
const char *name,
@@ -1675,6 +1690,16 @@ intel_alloc_aux_buffer(struct brw_context *brw,
   return false;
 
buf->size = aux_surf->size;
+
+   const struct gen_device_info *devinfo = >screen->devinfo;
+   if (devinfo->gen >= 10) {
+  /* On CNL, instead of setting the clear color in the SURFACE_STATE, we
+   * will set a pointer to a dword somewhere that contains the color. So,
+   * allocate the space for the clear color value here on the aux buffer.
+   */
+  buf->size += fast_clear_state_entry_size(brw);
+   }
+
buf->pitch = aux_surf->row_pitch;
buf->qpitch = isl_surf_get_array_pitch_sa_rows(aux_surf);
 
-- 
2.14.3

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