Re: [Mesa-dev] [PATCH 12/23] i965: Refactor resolving of auxiliary mode

2016-02-09 Thread Ben Widawsky 
On Mon, Feb 08, 2016 at 06:51:32PM +0200, Topi Pohjolainen wrote:
> Signed-off-by: Topi Pohjolainen 
> ---
>  src/mesa/drivers/dri/i965/gen8_surface_state.c | 62 
> --
>  1 file changed, 29 insertions(+), 33 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/gen8_surface_state.c 
> b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> index fc8f701..0a52815 100644
> --- a/src/mesa/drivers/dri/i965/gen8_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> @@ -197,6 +197,28 @@ gen8_emit_fast_clear_color(struct brw_context *brw,
>surf[7] |= mt->fast_clear_color_value;
>  }
>  
> +static uint32_t
> +gen8_get_aux_mode(const struct brw_context *brw,
> +  const struct intel_mipmap_tree *mt,
> +  uint32_t surf_type)
> +{
> +   if (mt->mcs_mt == NULL)
> +  return GEN8_SURFACE_AUX_MODE_NONE;

I do have some patches that kind of change this for aux_hiz support. I store the
AUX surface type in the aux_buf data structure. This is mostly a reminder to
myself. I think that actually works really well for your stuff here, but it
requires several of my patches before
aux-buf-v2 branch: i965: Save aux_mode at creation and use it

> +
> +   /*
> +* From the BDW PRM, Volume 2d, page 260 (RENDER_SURFACE_STATE):
> +* "When MCS is enabled for non-MSRT, HALIGN_16 must be used"
> +*
> +* From the hardware spec for GEN9:
> +* "When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E, HALIGN
> +*  16 must be used."
> +*/
> +   if (brw->gen >= 9 || mt->num_samples == 1)
> +  assert(mt->halign == 16);
> +
> +   return GEN8_SURFACE_AUX_MODE_MCS;
> +}
> +

There was a pretty good ascii table in intel_miptree_create_layout() that should
perhaps be moved here with this comment.

>  static void
>  gen8_emit_texture_surface_state(struct brw_context *brw,
>  struct intel_mipmap_tree *mt,
> @@ -209,13 +231,13 @@ gen8_emit_texture_surface_state(struct brw_context *brw,
>  bool rw, bool for_gather)
>  {
> const unsigned depth = max_layer - min_layer;
> -   struct intel_mipmap_tree *aux_mt = NULL;
> -   uint32_t aux_mode = GEN8_SURFACE_AUX_MODE_NONE;
> +   struct intel_mipmap_tree *aux_mt = mt->mcs_mt;
> uint32_t mocs_wb = brw->gen >= 9 ? SKL_MOCS_WB : BDW_MOCS_WB;
> int surf_index = surf_offset - >wm.base.surf_offset[0];
> unsigned tiling_mode, pitch;
> const unsigned tr_mode = surface_tiling_resource_mode(mt->tr_mode);
> const uint32_t surf_type = translate_tex_target(target);
> +   uint32_t aux_mode = gen8_get_aux_mode(brw, mt, surf_type);
>  
> if (mt->format == MESA_FORMAT_S_UINT8) {
>tiling_mode = GEN8_SURFACE_TILING_W;
> @@ -229,20 +251,9 @@ gen8_emit_texture_surface_state(struct brw_context *brw,
>  * buffer should always have been resolved before it is used as a texture
>  * so there is no need for it.
>  */
> -   if (mt->mcs_mt && mt->num_samples > 1) {
> -  aux_mt = mt->mcs_mt;
> -  aux_mode = GEN8_SURFACE_AUX_MODE_MCS;
> -
> -  /*
> -   * From the BDW PRM, Volume 2d, page 260 (RENDER_SURFACE_STATE):
> -   * "When MCS is enabled for non-MSRT, HALIGN_16 must be used"
> -   *
> -   * From the hardware spec for GEN9:
> -   * "When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E, 
> HALIGN
> -   *  16 must be used."
> -   */
> -  if (brw->gen >= 9 || mt->num_samples == 1)
> - assert(mt->halign == 16);
> +   if (mt->num_samples <= 1) {
> +  aux_mt = NULL;
> +  aux_mode = GEN8_SURFACE_AUX_MODE_NONE;

hmm. Shouldn't this consideration just be part of gen8_get_aux_mode()? See my
aux-buf-v2 branch for where I rework this function a bit (i965/gen8: Consolidate
MCS logic). I think that patch might make sense before this one, and then expand
gen8_get_aux_mode() to include num_samples.

I possible you change this all later, and its moot.

> }
>  
> uint32_t *surf = allocate_surface_state(brw, surf_offset, surf_index);
> @@ -418,8 +429,6 @@ gen8_update_renderbuffer_surface(struct brw_context *brw,
> struct gl_context *ctx = >ctx;
> struct intel_renderbuffer *irb = intel_renderbuffer(rb);
> struct intel_mipmap_tree *mt = irb->mt;
> -   struct intel_mipmap_tree *aux_mt = NULL;
> -   uint32_t aux_mode = GEN8_SURFACE_AUX_MODE_NONE;
> unsigned width = mt->logical_width0;
> unsigned height = mt->logical_height0;
> unsigned pitch = mt->pitch;
> @@ -472,21 +481,8 @@ gen8_update_renderbuffer_surface(struct brw_context *brw,
> __func__, _mesa_get_format_name(rb_format));
> }
>  
> -   if (mt->mcs_mt) {
> -  aux_mt = mt->mcs_mt;
> -  aux_mode = GEN8_SURFACE_AUX_MODE_MCS;
> -
> -  /*
> -   * From the BDW PRM, Volume 2d, page 260 (RENDER_SURFACE_STATE):
> -   * "When MCS is enabled for non-MSRT, HALIGN_16 must be used"
> -   *
> -   * From the

Re: [Mesa-dev] [PATCH 12/23] i965: Refactor resolving of auxiliary mode

2016-02-09 Thread Pohjolainen, Topi 
On Tue, Feb 09, 2016 at 03:50:04PM -0800, Ben Widawsky wrote:
> On Mon, Feb 08, 2016 at 06:51:32PM +0200, Topi Pohjolainen wrote:
> > Signed-off-by: Topi Pohjolainen 
> > ---
> >  src/mesa/drivers/dri/i965/gen8_surface_state.c | 62 
> > --
> >  1 file changed, 29 insertions(+), 33 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/gen8_surface_state.c 
> > b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> > index fc8f701..0a52815 100644
> > --- a/src/mesa/drivers/dri/i965/gen8_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> > @@ -197,6 +197,28 @@ gen8_emit_fast_clear_color(struct brw_context *brw,
> >surf[7] |= mt->fast_clear_color_value;
> >  }
> >  
> > +static uint32_t
> > +gen8_get_aux_mode(const struct brw_context *brw,
> > +  const struct intel_mipmap_tree *mt,
> > +  uint32_t surf_type)
> > +{
> > +   if (mt->mcs_mt == NULL)
> > +  return GEN8_SURFACE_AUX_MODE_NONE;
> 
> I do have some patches that kind of change this for aux_hiz support. I store 
> the
> AUX surface type in the aux_buf data structure. This is mostly a reminder to
> myself. I think that actually works really well for your stuff here, but it
> requires several of my patches before
> aux-buf-v2 branch: i965: Save aux_mode at creation and use it
> 
> > +
> > +   /*
> > +* From the BDW PRM, Volume 2d, page 260 (RENDER_SURFACE_STATE):
> > +* "When MCS is enabled for non-MSRT, HALIGN_16 must be used"
> > +*
> > +* From the hardware spec for GEN9:
> > +* "When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E, HALIGN
> > +*  16 must be used."
> > +*/
> > +   if (brw->gen >= 9 || mt->num_samples == 1)
> > +  assert(mt->halign == 16);
> > +
> > +   return GEN8_SURFACE_AUX_MODE_MCS;
> > +}
> > +
> 
> There was a pretty good ascii table in intel_miptree_create_layout() that 
> should
> perhaps be moved here with this comment.
> 
> >  static void
> >  gen8_emit_texture_surface_state(struct brw_context *brw,
> >  struct intel_mipmap_tree *mt,
> > @@ -209,13 +231,13 @@ gen8_emit_texture_surface_state(struct brw_context 
> > *brw,
> >  bool rw, bool for_gather)
> >  {
> > const unsigned depth = max_layer - min_layer;
> > -   struct intel_mipmap_tree *aux_mt = NULL;
> > -   uint32_t aux_mode = GEN8_SURFACE_AUX_MODE_NONE;
> > +   struct intel_mipmap_tree *aux_mt = mt->mcs_mt;
> > uint32_t mocs_wb = brw->gen >= 9 ? SKL_MOCS_WB : BDW_MOCS_WB;
> > int surf_index = surf_offset - >wm.base.surf_offset[0];
> > unsigned tiling_mode, pitch;
> > const unsigned tr_mode = surface_tiling_resource_mode(mt->tr_mode);
> > const uint32_t surf_type = translate_tex_target(target);
> > +   uint32_t aux_mode = gen8_get_aux_mode(brw, mt, surf_type);
> >  
> > if (mt->format == MESA_FORMAT_S_UINT8) {
> >tiling_mode = GEN8_SURFACE_TILING_W;
> > @@ -229,20 +251,9 @@ gen8_emit_texture_surface_state(struct brw_context 
> > *brw,
> >  * buffer should always have been resolved before it is used as a 
> > texture
> >  * so there is no need for it.
> >  */
> > -   if (mt->mcs_mt && mt->num_samples > 1) {
> > -  aux_mt = mt->mcs_mt;
> > -  aux_mode = GEN8_SURFACE_AUX_MODE_MCS;
> > -
> > -  /*
> > -   * From the BDW PRM, Volume 2d, page 260 (RENDER_SURFACE_STATE):
> > -   * "When MCS is enabled for non-MSRT, HALIGN_16 must be used"
> > -   *
> > -   * From the hardware spec for GEN9:
> > -   * "When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E, 
> > HALIGN
> > -   *  16 must be used."
> > -   */
> > -  if (brw->gen >= 9 || mt->num_samples == 1)
> > - assert(mt->halign == 16);
> > +   if (mt->num_samples <= 1) {
> > +  aux_mt = NULL;
> > +  aux_mode = GEN8_SURFACE_AUX_MODE_NONE;
> 
> hmm. Shouldn't this consideration just be part of gen8_get_aux_mode()? See my
> aux-buf-v2 branch for where I rework this function a bit (i965/gen8: 
> Consolidate
> MCS logic). I think that patch might make sense before this one, and then 
> expand
> gen8_get_aux_mode() to include num_samples.

While gen8_get_aux_mode() is shared by texture and render buffer setup,
this is a special case for textures only. There is no reason to give the
auxiliary buffer to sampling engine if the egnine doesn't understand it.
(Note that this is existing logic, only now written explicitly. Previous
logic initialised aux_mt to NULL and left it untouched for single sampled.
Here we initialise it, and overwrite it back to NULL for single sampled).

For render target it has to be there as data port uses it.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 12/23] i965: Refactor resolving of auxiliary mode

2016-02-08 Thread Topi Pohjolainen 
Signed-off-by: Topi Pohjolainen 
---
 src/mesa/drivers/dri/i965/gen8_surface_state.c | 62 --
 1 file changed, 29 insertions(+), 33 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/gen8_surface_state.c 
b/src/mesa/drivers/dri/i965/gen8_surface_state.c
index fc8f701..0a52815 100644
--- a/src/mesa/drivers/dri/i965/gen8_surface_state.c
+++ b/src/mesa/drivers/dri/i965/gen8_surface_state.c
@@ -197,6 +197,28 @@ gen8_emit_fast_clear_color(struct brw_context *brw,
   surf[7] |= mt->fast_clear_color_value;
 }
 
+static uint32_t
+gen8_get_aux_mode(const struct brw_context *brw,
+  const struct intel_mipmap_tree *mt,
+  uint32_t surf_type)
+{
+   if (mt->mcs_mt == NULL)
+  return GEN8_SURFACE_AUX_MODE_NONE;
+
+   /*
+* From the BDW PRM, Volume 2d, page 260 (RENDER_SURFACE_STATE):
+* "When MCS is enabled for non-MSRT, HALIGN_16 must be used"
+*
+* From the hardware spec for GEN9:
+* "When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E, HALIGN
+*  16 must be used."
+*/
+   if (brw->gen >= 9 || mt->num_samples == 1)
+  assert(mt->halign == 16);
+
+   return GEN8_SURFACE_AUX_MODE_MCS;
+}
+
 static void
 gen8_emit_texture_surface_state(struct brw_context *brw,
 struct intel_mipmap_tree *mt,
@@ -209,13 +231,13 @@ gen8_emit_texture_surface_state(struct brw_context *brw,
 bool rw, bool for_gather)
 {
const unsigned depth = max_layer - min_layer;
-   struct intel_mipmap_tree *aux_mt = NULL;
-   uint32_t aux_mode = GEN8_SURFACE_AUX_MODE_NONE;
+   struct intel_mipmap_tree *aux_mt = mt->mcs_mt;
uint32_t mocs_wb = brw->gen >= 9 ? SKL_MOCS_WB : BDW_MOCS_WB;
int surf_index = surf_offset - >wm.base.surf_offset[0];
unsigned tiling_mode, pitch;
const unsigned tr_mode = surface_tiling_resource_mode(mt->tr_mode);
const uint32_t surf_type = translate_tex_target(target);
+   uint32_t aux_mode = gen8_get_aux_mode(brw, mt, surf_type);
 
if (mt->format == MESA_FORMAT_S_UINT8) {
   tiling_mode = GEN8_SURFACE_TILING_W;
@@ -229,20 +251,9 @@ gen8_emit_texture_surface_state(struct brw_context *brw,
 * buffer should always have been resolved before it is used as a texture
 * so there is no need for it.
 */
-   if (mt->mcs_mt && mt->num_samples > 1) {
-  aux_mt = mt->mcs_mt;
-  aux_mode = GEN8_SURFACE_AUX_MODE_MCS;
-
-  /*
-   * From the BDW PRM, Volume 2d, page 260 (RENDER_SURFACE_STATE):
-   * "When MCS is enabled for non-MSRT, HALIGN_16 must be used"
-   *
-   * From the hardware spec for GEN9:
-   * "When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E, HALIGN
-   *  16 must be used."
-   */
-  if (brw->gen >= 9 || mt->num_samples == 1)
- assert(mt->halign == 16);
+   if (mt->num_samples <= 1) {
+  aux_mt = NULL;
+  aux_mode = GEN8_SURFACE_AUX_MODE_NONE;
}
 
uint32_t *surf = allocate_surface_state(brw, surf_offset, surf_index);
@@ -418,8 +429,6 @@ gen8_update_renderbuffer_surface(struct brw_context *brw,
struct gl_context *ctx = >ctx;
struct intel_renderbuffer *irb = intel_renderbuffer(rb);
struct intel_mipmap_tree *mt = irb->mt;
-   struct intel_mipmap_tree *aux_mt = NULL;
-   uint32_t aux_mode = GEN8_SURFACE_AUX_MODE_NONE;
unsigned width = mt->logical_width0;
unsigned height = mt->logical_height0;
unsigned pitch = mt->pitch;
@@ -472,21 +481,8 @@ gen8_update_renderbuffer_surface(struct brw_context *brw,
__func__, _mesa_get_format_name(rb_format));
}
 
-   if (mt->mcs_mt) {
-  aux_mt = mt->mcs_mt;
-  aux_mode = GEN8_SURFACE_AUX_MODE_MCS;
-
-  /*
-   * From the BDW PRM, Volume 2d, page 260 (RENDER_SURFACE_STATE):
-   * "When MCS is enabled for non-MSRT, HALIGN_16 must be used"
-   *
-   * From the hardware spec for GEN9:
-   * "When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E, HALIGN
-   *  16 must be used."
-   */
-  if (brw->gen >= 9 || mt->num_samples == 1)
- assert(mt->halign == 16);
-   }
+   struct intel_mipmap_tree *aux_mt = mt->mcs_mt;
+   const uint32_t aux_mode = gen8_get_aux_mode(brw, mt, surf_type);
 
uint32_t *surf = allocate_surface_state(brw, , surf_index);
 
-- 
2.5.0

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