On Wed, Jul 13, 2016 at 10:16 AM, Chad Versace <chad.vers...@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.
*/
>
> > + */
> > + assert(format_bpb == 1 || format_bpb == 2);
> > + logical_el = isl_extent2d(128, 256 / format_bpb);
> > + phys_B = isl_extent2d(128, 32);
> > + break;
> > +
> > default:
> > unreachable("not reached");
> > } /* end switch */
>
>
>
> > @@ -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.
>
> > + assert(fmtl->bh == 4);
> > + pitch_el_rows = isl_align(pitch_el_rows, 256 / 4);
> > + }
> > +
> > if (ISL_DEV_GEN(dev) >= 9 &&
> > info->dim == ISL_SURF_DIM_3D &&
> > tile_info->tiling != ISL_TILING_LINEAR) {
> > diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> > index 598ed2c..0ea19d1 100644
> > --- a/src/intel/isl/isl.h
> > +++ b/src/intel/isl/isl.h
> > @@ -356,6 +356,15 @@ enum isl_format {
> > ISL_FORMAT_MCS_4X,
> > ISL_FORMAT_MCS_8X,
> > ISL_FORMAT_MCS_16X,
> > + ISL_FORMAT_GEN7_CCS_32BPP_X,
> > + ISL_FORMAT_GEN7_CCS_64BPP_X,
> > + ISL_FORMAT_GEN7_CCS_128BPP_X,
> > + ISL_FORMAT_GEN7_CCS_32BPP_Y,
> > + ISL_FORMAT_GEN7_CCS_64BPP_Y,
> > + ISL_FORMAT_GEN7_CCS_128BPP_Y,
> > + ISL_FORMAT_GEN9_CCS_32BPP,
> > + ISL_FORMAT_GEN9_CCS_64BPP,
> > + ISL_FORMAT_GEN9_CCS_128BPP,
>
> The format names look good.
>
> > @@ -989,6 +1002,7 @@ isl_format_has_bc_compression(enum isl_format fmt)
> >
> > case ISL_TXC_HIZ:
> > case ISL_TXC_MCS:
> > + case ISL_TXC_CCS:
> > unreachable("Should not be called on an aux surface");
> > }
> >
> > diff --git a/src/intel/isl/isl_format_layout.csv
> b/src/intel/isl/isl_format_layout.csv
> > index 972d50f..f0f31c7 100644
> > --- a/src/intel/isl/isl_format_layout.csv
> > +++ b/src/intel/isl/isl_format_layout.csv
> > @@ -319,3 +319,12 @@ MCS_2X , 8, 1, 1, 1,
> , , , , ,
> > MCS_4X , 8, 1, 1, 1, , , , ,
> , , , , mcs
> > MCS_8X , 32, 1, 1, 1, , , , ,
> , , , , mcs
> > MCS_16X , 64, 1, 1, 1, , , , ,
> , , , , mcs
> > +GEN7_CCS_32BPP_X , 1, 16, 2, 1, , , , ,
> , , , , ccs
> > +GEN7_CCS_64BPP_X , 1, 8, 2, 1, , , , ,
> , , , , ccs
> > +GEN7_CCS_128BPP_X , 1, 4, 2, 1, , , , ,
> , , , , ccs
> > +GEN7_CCS_32BPP_Y , 1, 8, 4, 1, , , , ,
> , , , , ccs
> > +GEN7_CCS_64BPP_Y , 1, 4, 4, 1, , , , ,
> , , , , ccs
> > +GEN7_CCS_128BPP_Y , 1, 2, 4, 1, , , , ,
> , , , , ccs
> > +GEN9_CCS_32BPP , 2, 8, 4, 1, , , , ,
> , , , , ccs
> > +GEN9_CCS_64BPP , 2, 4, 4, 1, , , , ,
> , , , , ccs
> > +GEN9_CCS_128BPP , 2, 2, 4, 1, , , , ,
> , , , , ccs
>
> The block dimensions look right to me.
>
> > diff --git a/src/intel/isl/isl_gen7.c b/src/intel/isl/isl_gen7.c
> > index 4b511c8..1c31d98 100644
> > --- a/src/intel/isl/isl_gen7.c
> > +++ b/src/intel/isl/isl_gen7.c
> > @@ -242,6 +242,13 @@ gen7_filter_tiling(const struct isl_device *dev,
> > if (isl_format_get_layout(info->format)->txc == ISL_TXC_MCS)
> > *flags &= ISL_TILING_Y0_BIT;
> >
> > + /* The CCS formats and tiling always go together */
> > + if (isl_format_get_layout(info->format)->txc == ISL_TXC_CCS) {
> > + *flags &= ISL_TILING_CCS_BIT;
> > + } else {
> > + *flags &= ~ISL_TILING_CCS_BIT;
> > + }
> > +
> > if (info->usage & (ISL_SURF_USAGE_DISPLAY_ROTATE_90_BIT |
> > ISL_SURF_USAGE_DISPLAY_ROTATE_180_BIT |
> > ISL_SURF_USAGE_DISPLAY_ROTATE_270_BIT)) {
>
> 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.
>
> > diff --git a/src/intel/isl/isl_gen8.c b/src/intel/isl/isl_gen8.c
> > index 62c331c..ec40d6d 100644
> > --- a/src/intel/isl/isl_gen8.c
> > +++ b/src/intel/isl/isl_gen8.c
> > @@ -201,6 +201,19 @@ gen8_choose_image_alignment_el(const struct
> isl_device *dev,
> > {
> > assert(!isl_tiling_is_std_y(tiling));
> >
> > + const struct isl_format_layout *fmtl =
> isl_format_get_layout(info->format);
> > + if (fmtl->txc == ISL_TXC_CCS) {
> > + /*
> > + * Broadwell PRM Vol 7, "MCS Buffer for Render Target(s)" (p.
> 676):
> > + *
> > + * "Mip-mapped and arrayed surfaces are supported with MCS
> buffer
> > + * layout with these alignments in the RT space: Horizontal
> > + * Alignment = 256 and Vertical Alignment = 128.
> > + */
> > + *image_align_el = isl_extent3d(256 / fmtl->bw, 128 / fmtl->bh, 1);
> > + return;
> > + }
> > +
> > /* The below text from the Broadwell PRM provides some insight into
> the
> > * hardware's requirements for LOD alignment. From the Broadwell
> PRM >>
> > * Volume 5: Memory Views >> Surface Layout >> 2D Surfaces:
> > diff --git a/src/intel/isl/isl_gen9.c b/src/intel/isl/isl_gen9.c
> > index 39f4092..bb7f298 100644
> > --- a/src/intel/isl/isl_gen9.c
> > +++ b/src/intel/isl/isl_gen9.c
> > @@ -103,6 +103,18 @@ gen9_choose_image_alignment_el(const struct
> isl_device *dev,
> > enum isl_msaa_layout msaa_layout,
> > struct isl_extent3d *image_align_el)
> > {
> > + const struct isl_format_layout *fmtl =
> isl_format_get_layout(info->format);
> > + if (fmtl->txc == ISL_TXC_CCS) {
> > + /* 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."
> > + */
> > + *image_align_el = isl_extent3d(128 / fmtl->bw, 64 / fmtl->bh, 1);
> > + return;
> > + }
> > +
> > /* This BSpec text provides some insight into the hardware's
> alignment
> > * requirements [Skylake BSpec > Memory Views > Common Surface
> Formats >
> > * Surface Layout and Tiling > 2D Surfaces]:
> > --
> > 2.5.0.400.gff86faf
> >
> > _______________________________________________
> > 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
