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

[Jason Ekstrand]

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.

--Jason


> + */
>  static const struct drm_format_info ccs_formats[] = {
>         { .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 2,
> .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
>         { .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 2,
> .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
> --
> 2.13.6
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20180110/70d0c65f/attachment.html>