On Wed 13 Jul 2016, Jason Ekstrand wrote: > 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. > */
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.vers...@intel.com> I believe I've reviewed the whole series. Ping me if I forgot anything. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev