[Intel-gfx] [PATCH 1/8] drm/i915: Add a comment exlaining CCS hsub/vsub

[Jason Ekstrand]

On Wed, Jan 17, 2018 at 12:20 PM, Ville Syrjälä <
ville.syrjala at linux.intel.com> wrote:

> 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ä <
> > ville.syrjala at linux.intel.com> 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 <
> > > > ville.syrjala at linux.intel.com> wrote:
> > > >
> > > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > >
> > > > > 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 at ffwll.ch>
> > > > > Cc: Ben Widawsky <ben at bwidawsk.net>
> > > > > Cc: Jason Ekstrand <jason at jlekstrand.net>
> > > > > Cc: Daniel Stone <daniels at collabora.com>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > ---
> > > > >  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.
> >  */
>
> Works for me. Should I just suck that into my patch, or you want to
> submit it as a proper patch?
>

Feel free to suck it in.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20180117/9435299d/attachment-0001.html>