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