<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jan 17, 2018 at 12:20 PM, Ville Syrjälä <<a href="mailto:ville.syrjala@linux.intel.com" target="_blank">ville.syrjala@linux.intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, Jan 11, 2018 at 09:25:47PM -0800, Jason Ekstrand wrote: > On Wed, Jan 10, 2018 at 9:48 AM, Ville Syrjälä < > <a href="mailto:ville.syrjala@linux.intel.com">ville.syrjala@linux.intel.com</a>> wrote: > > > On Wed, Jan 10, 2018 at 09:03:14AM -0800, Jason Ekstrand wrote: > > > On Fri, Dec 22, 2017 at 11:22 AM, Ville Syrjala < > > > <a href="mailto:ville.syrjala@linux.intel.com">ville.syrjala@linux.intel.com</a>> wrote: > > > > > > > From: Ville Syrjälä <<a href="mailto:ville.syrjala@linux.intel.com">ville.syrjala@linux.intel.com</a>> > > > > > > > > Let's document why we claim hsub==8,vsub==16 for CCS even though the > > > > memory layout would suggest that we use 16x8 instead. > > > > > > > > Cc: Daniel Vetter <<a href="mailto:daniel@ffwll.ch">daniel@ffwll.ch</a>> > > > > Cc: Ben Widawsky <<a href="mailto:ben@bwidawsk.net">ben@bwidawsk.net</a>> > > > > Cc: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> > > > > Cc: Daniel Stone <<a href="mailto:daniels@collabora.com">daniels@collabora.com</a>> > > > > Signed-off-by: Ville Syrjälä <<a href="mailto:ville.syrjala@linux.intel.com">ville.syrjala@linux.intel.com</a>> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index 0cd355978ab4..83aec68537b4 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -2387,6 +2387,13 @@ static unsigned int intel_fb_modifier_to_tiling( > > uint64_t > > > > fb_modifier) > > > > } > > > > } > > > > > > > > +/* > > > > + * 1 byte of CCS actually corresponds to 16x8 pixels on the main > > > > + * surface, and the memory layout for the CCS tile is 64x64 bytes. > > > > + * But since we're pretending the CCS tile is 128 bytes wide we > > > > + * adjust hsub/vsub here accordingly to 8x16 so that the > > > > + * bytes<->x/y conversions come out correct. > > > > > > > > > > I'm not particularly happy with this comment as I think it pushes the > > > mental model for these calculations in the wrong direction. The PRM > > says: > > > > > > 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 16 x16 sets of 128 byte Y-tiled > > > cache-line-pairs. CCS is always Y tiled. > > > > > > If you understand that a "cache line pair" in the main surface is a > > > horizontally adjacent cache line pair (cl1_addr = cl0_addr + 512) and you > > > just accept the statement about Y-tiling, this is the correct > > calculation. > > > Calculating these things in terms of pixels is occasionally useful but is > > > the wrong mental model. The cache line statement above both accurately > > > describes the layout of the CCS (at the cache line granularity) and > > scales > > > to other pixel formats which are not 32-bit. > > > > > > I know that Ville and I have disagreed on this in the past but I don't > > > think adding comments about how we're "pretending the CCS tile is 128 > > bytes > > > wide" is making anything more clear. > > > > I don't really see how talk about cachelines is going to help explain > > the hsub/vsub values we use here. > > > > But I don't really care that much. We could just leave them as magic > > numbers if no one can come up with a clear explanation for them. > > > > How about a comment like this: > > /*From the Sky Lake PRM: > * > * 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 16 x16 sets of 128 byte Y-tiled > cache-line-pairs. CCS is always Y tiled. > * > * Since cache line pairs refers to horizontally adjacent cache lines, each > cache line in the CCS > * corresponds to an area of 32x16 cache lines on the main surface. Since > each pixel is 4 bytes, > * this gives us a ratio of one byte in the CCS for each 8x16 pixels in the > main surface. > */ </div></div>Works for me. Should I just suck that into my patch, or you want to submit it as a proper patch? </blockquote><div> </div><div>Feel free to suck it in. </div></div> </div></div>