[Mesa-dev] [PATCH 5/8] i965/msaa: Set SURFACE_STATE properly when CMS MSAA is in use.

[Chad Versace]

On 07/06/2012 03:29 PM, Paul Berry wrote:
> When a buffer using Gen7's CMS MSAA layout is bound to a texture or a
> render target, the SURFACE_STATE structure needs to point to the MCS
> buffer and to indicate its pitch.  This patch updates the functions
> that emit SURFACE_STATE to handle CMS layout properly.
> ---
>  src/mesa/drivers/dri/i965/brw_state.h             |    5 ++
>  src/mesa/drivers/dri/i965/gen7_blorp.cpp          |    4 ++
>  src/mesa/drivers/dri/i965/gen7_wm_surface_state.c |   45 +++++++++++++++++++++
>  3 files changed, 54 insertions(+), 0 deletions(-)



>  void
> +gen7_set_surface_mcs_info(struct brw_context *brw,
> +                          struct gen7_surface_state *surf,
> +                          uint32_t surf_offset,
> +                          const struct intel_mipmap_tree *mcs_mt,
> +                          bool is_render_target)
> +{
> +   /* From the Ivy Bridge PRM, Vol4 Part1 p76, "MCS Base Address":
> +    *
> +    *     "The MCS surface must be stored as Tile Y."
> +    */
> +   assert(mcs_mt->region->tiling == I915_TILING_Y);
> +
> +   /* Compute the pitch in units of tiles.  To do this we need to divide the
> +    * pitch in bytes by 128, since a single Y-tile is 128 bytes wide.
> +    */
> +   unsigned pitch_bytes = mcs_mt->region->pitch * mcs_mt->cpp;
> +   unsigned pitch_tiles = pitch_bytes / 128;

Here, I first thought "A rounddown error will occur if pitch_bytes is not
128-aligned!" But I convinced myself that can't happen.



> +
> +   /* The upper 20 bits of surface state DWORD 6 are the upper 20 bits of the
> +    * GPU address of the MCS buffer; the lower 12 bits contain other control
> +    * information.  Since buffer addresses are always on 4k boundaries (and
> +    * thus have their lower 12 bits zero), we can use an ordinary reloc to do
> +    * the necessary address translation.
> +    */
> +   assert ((mcs_mt->region->bo->offset & 0xfff) == 0);
> +   surf->ss6.mcs_enabled.mcs_enable = 1;
> +   surf->ss6.mcs_enabled.mcs_surface_pitch = pitch_tiles - 1;
> +   surf->ss6.mcs_enabled.mcs_base_address = mcs_mt->region->bo->offset >> 12;
> +   drm_intel_bo_emit_reloc(brw->intel.batch.bo,
> +                           surf_offset +
> +                           offsetof(struct gen7_surface_state, ss6),
> +                           mcs_mt->region->bo,
> +                           surf->ss6.raw_data - mcs_mt->region->bo->offset,
> +                           is_render_target ? I915_GEM_DOMAIN_RENDER
> +                           : I915_GEM_DOMAIN_SAMPLER,
> +                           is_render_target ? I915_GEM_DOMAIN_RENDER : 0);
> +}

I'm confused. ss6.raw_data is not an address nor an offset; it's lower bits are
non-addressy things. So I see a unit conflict in the target_offset arg,
`surf->ss6.raw_data - mcs_mt->region->bo->offset`.

I tried to reconstruct what you may have intended, and I arrived at
`(surf->ss6.mcs_enabled.mcs_base_address << 12) - mcs_mt->region->bo->offset`.
But that just evaluates to a 0 offset, and I became confused again as to why you
would want to calculate 0 in such roundabout way.

What's happening there?

----
Chad Versace
chad.versace at linux.intel.com