From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Ekstrand Subject: Re: [PATCH 1/8] drm/i915: Add a comment exlaining CCS hsub/vsub Date: Wed, 10 Jan 2018 09:03:14 -0800 Message-ID: References: <20171222192231.17981-1-ville.syrjala@linux.intel.com> <20171222192231.17981-2-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1632312774==" Return-path: Received: from mail-wm0-x243.google.com (mail-wm0-x243.google.com [IPv6:2a00:1450:400c:c09::243]) by gabe.freedesktop.org (Postfix) with ESMTPS id 9B9E46E31E for ; Wed, 10 Jan 2018 17:03:16 +0000 (UTC) Received: by mail-wm0-x243.google.com with SMTP id b141so339036wme.1 for ; Wed, 10 Jan 2018 09:03:16 -0800 (PST) In-Reply-To: <20171222192231.17981-2-ville.syrjala@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Ville Syrjala Cc: Intel GFX , Ben Widawsky , Daniel Stone List-Id: intel-gfx@lists.freedesktop.org --===============1632312774== Content-Type: multipart/alternative; boundary="001a1146acded3527805626f014b" --001a1146acded3527805626f014b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, Dec 22, 2017 at 11:22 AM, Ville Syrjala < ville.syrjala@linux.intel.com> wrote: > From: Ville Syrj=C3=A4l=C3=A4 > > Let's document why we claim hsub=3D=3D8,vsub=3D=3D16 for CCS even though = the > memory layout would suggest that we use 16x8 instead. > > Cc: Daniel Vetter > Cc: Ben Widawsky > Cc: Jason Ekstrand > Cc: Daniel Stone > Signed-off-by: Ville Syrj=C3=A4l=C3=A4 > --- > 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(ui= nt64_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 =3D 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[] =3D { > { .format =3D DRM_FORMAT_XRGB8888, .depth =3D 24, .num_planes =3D= 2, > .cpp =3D { 4, 1, }, .hsub =3D 8, .vsub =3D 16, }, > { .format =3D DRM_FORMAT_XBGR8888, .depth =3D 24, .num_planes =3D= 2, > .cpp =3D { 4, 1, }, .hsub =3D 8, .vsub =3D 16, }, > -- > 2.13.6 > > --001a1146acded3527805626f014b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On F= ri, Dec 22, 2017 at 11:22 AM, Ville Syrjala <ville.syrjala@l= inux.intel.com> wrote:
From: Ville Syrj=C3=A4l=C3=A4 <ville.syrjala@linux.intel.com>

Let's document why we claim hsub=3D=3D8,vsub=3D=3D16 for CCS even thoug= h the
memory layout would suggest that we use 16x8 instead.

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Jason Ekstrand <jason@jlekst= rand.net>
Cc: Daniel Stone <daniels@colla= bora.com>
Signed-off-by: Ville Syrj=C3=A4l=C3=A4 <ville.syrjala@linux.intel.com>
---
=C2=A0drivers/gpu/drm/i915/intel_display.c | 7 +++++++
=C2=A01 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i9= 15/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)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
=C2=A0}

+/*
+ * 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.=C2= =A0 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 c= ache-line represents
an area on the main surface of 16 x16 sets of 128 b= yte 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 =3D cl0_addr + 512) a= nd you just accept the statement about Y-tiling, this is the correct calcul= ation.=C2=A0 Calculating these things in terms of pixels is occasionally us= eful but is the wrong mental model.=C2=A0 The cache line statement above bo= th accurately describes the layout of the CCS (at the cache line granularit= y) 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 d= on't think adding comments about how we're "pretending the CCS= tile is 128 bytes wide" is making anything more clear.

=
--Jason
=C2=A0
+ */
=C2=A0static const struct drm_format_info ccs_formats[] =3D {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 { .format =3D DRM_FORMAT_XRGB8888, .depth =3D 2= 4, .num_planes =3D 2, .cpp =3D { 4, 1, }, .hsub =3D 8, .vsub =3D 16, },
=C2=A0 =C2=A0 =C2=A0 =C2=A0 { .format =3D DRM_FORMAT_XBGR8888, .depth =3D 2= 4, .num_planes =3D 2, .cpp =3D { 4, 1, }, .hsub =3D 8, .vsub =3D 16, },
--
2.13.6


--001a1146acded3527805626f014b-- --===============1632312774== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============1632312774==--