On Wed, Jan 10, 2018 at 9:48 AM, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
I don't really see how talk about cachelines is going to help explainOn Wed, Jan 10, 2018 at 09:03:14AM -0800, Jason Ekstrand wrote:
> On Fri, Dec 22, 2017 at 11:22 AM, Ville Syrjala <
> ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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 <daniel@xxxxxxxx>
> > Cc: Ben Widawsky <ben@xxxxxxxxxxxx>
> > Cc: Jason Ekstrand <jason@xxxxxxxxxxxxxx>
> > Cc: Daniel Stone <daniels@xxxxxxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > 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.
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.
* 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.
*/
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
