[Mesa-dev] [PATCH v2 12/14] isl: Add support for color control surfaces

[Chad Versace]

On Wed 13 Jul 2016, Jason Ekstrand wrote:
> On Wed, Jul 13, 2016 at 10:16 AM, Chad Versace <chad.versace at intel.com>
> wrote:
> 
> > On Sat 09 Jul 2016, Jason Ekstrand wrote:
> > > ---
> > >  src/intel/isl/isl.c                 | 32
> > ++++++++++++++++++++++++++++++++
> > >  src/intel/isl/isl.h                 | 14 ++++++++++++++
> > >  src/intel/isl/isl_format_layout.csv |  9 +++++++++
> > >  src/intel/isl/isl_gen7.c            |  7 +++++++
> > >  src/intel/isl/isl_gen8.c            | 13 +++++++++++++
> > >  src/intel/isl/isl_gen9.c            | 12 ++++++++++++
> > >  6 files changed, 87 insertions(+)
> > >
> > > diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> > > index 9ccdea2..da9d3d4 100644
> > > --- a/src/intel/isl/isl.c
> > > +++ b/src/intel/isl/isl.c
> > > @@ -177,6 +177,15 @@ isl_tiling_get_info(const struct isl_device *dev,
> > >        phys_B = isl_extent2d(128, 32);
> > >        break;
> > >
> > > +   case ISL_TILING_CCS:
> > > +      /* CCS surfaces are required to have one of the GENX_CCS_*
> > formats which
> > > +       * have a block size of 1 or 2 bits per block.
> >
> > I'd like to see a comment here, similar to case ISL_TILING_HIZ, that
> > states ISL_TILING_CCS is related to Y-tiling. Such a comment would
> > immediately explain the magic numbers below.
> >
> 
> How about this:
> 
>       /* CCS surfaces are required to have one of the GENX_CCS_* formats
> which
>        * have a block size of 1 or 2 bits per block and each CCS element
>        * corresponds to one cache-line pair in the main surface.  From the
> Sky
>        * Lake PRM Vol. 12 in the section on planes:
>        *
>        *    "The Color Control Surface (CCS) contains the compression status
>        *    of the cache-line pairs. The compression state of the cache-line
>        *    pair is specified by 2 bits in the CCS.  Each CCS cache-line
>        *    represents an area on the main surface of 16x16 sets of 128 byte
>        *    Y-tiled cache-line-pairs. CCS is always Y tiled."
>        *
>        * The CCS being Y-tiled implies that it's an 8x8 grid of cache-lines.
>        * Since each cache line corresponds to a 16x16 set of cache-line
> pairs,
>        * that yields total tile area of 128x128 cache-line pairs or CCS
>        * elements.  On older hardware, each CCS element is 1 bit and the
> tile
>        * is 128x256 elements.
>        */

That comment looks great.


> > > @@ -844,6 +854,28 @@ isl_calc_array_pitch_el_rows(const struct
> > isl_device *dev,
> > >     assert(pitch_sa_rows % fmtl->bh == 0);
> > >     uint32_t pitch_el_rows = pitch_sa_rows / fmtl->bh;
> > >
> > > +   if (ISL_DEV_GEN(dev) >= 9 && fmtl->txc == ISL_TXC_CCS) {
> > > +      /*
> > > +       * From the Sky Lake PRM Vol 7, "MCS Buffer for Render Target(s)"
> > (p. 632):
> > > +       *
> > > +       *    "Mip-mapped and arrayed surfaces are supported with MCS
> > buffer
> > > +       *    layout with these alignments in the RT space: Horizontal
> > > +       *    Alignment = 128 and Vertical Alignment = 64."
> > > +       *
> > > +       * From the Sky Lake PRM Vol. 2d, "RENDER_SURFACE_STATE" (p. 435):
> > > +       *
> > > +       *    "For non-multisampled render target's CCS auxiliary surface,
> > > +       *    QPitch must be computed with Horizontal Alignment = 128 and
> > > +       *    Surface Vertical Alignment = 256. These alignments are only
> > for
> > > +       *    CCS buffer and not for associated render target."
> > > +       *
> > > +       * The alignment in the second PRM quotation only applies to the
> > qpitch
> > > +       * so it needs to be applied here.
> > > +       */
> >
> > I have a comment about the first restriction (HorizontalAlignment=64 and
> > VerticalAlignment=64). The code snippet below implicitly satisfies the
> > first restriction, and it would be good to explicitly call that out.
> >
> 
> I'm not sure what you mean.  Maybe something like this?
> 
> The first restriction is already handled by isl_choose_image_alignment_el
> but the second restriction, which is an extension of the first, only
> applies to qpitch and must be applied here.

(Your email client is mangling the patches with line-wrapping. It makes
them hard to read).

This updated comment looks good.


> > > diff --git a/src/intel/isl/isl_gen7.c b/src/intel/isl/isl_gen7.c


> > I expected to this patch to make changes to
> > gen7_choose_image_alignment_el(). Why is that missing? Does gen7 not
> > impose special alignment restrictions on the CCS?
> >
> 
> gen7 doesn't allow CCS for multi-LOD or multi-slice images so it doesn't
> matter.

Thanks. That makes sense.

This patch is
Reviewed-by: Chad Versace <chad.versace at intel.com>

I believe I've reviewed the whole series. Ping me if I forgot anything.