From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH v2 3/4] drm/vc4: Add color transformation matrix (CTM) support Date: Fri, 30 Mar 2018 09:11:13 -0700 Message-ID: <87d0zlwnku.fsf@anholt.net> References: <20180325015240.75464-1-stschake@gmail.com> <20180325015240.75464-4-stschake@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1091402847==" Return-path: Received: from anholt.net (anholt.net [50.246.234.109]) by gabe.freedesktop.org (Postfix) with ESMTP id 317AA6E932 for ; Fri, 30 Mar 2018 16:11:17 +0000 (UTC) In-Reply-To: <20180325015240.75464-4-stschake@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Cc: airlied@linux.ie, linux-rpi-kernel@lists.infradead.org, dri-devel@lists.freedesktop.org, Stefan Schake List-Id: dri-devel@lists.freedesktop.org --===============1091402847== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Stefan Schake writes: > The hardware supports a CTM with S0.9 values. We therefore only allow > a value of 1.0 or fractional only and reject all others with integer > parts. This restriction is mostly inconsequential in practice since > commonly used transformation matrices have all scalars <=3D 1.0. > > Signed-off-by: Stefan Schake My primary concern with this patch is whether the OLEDCOEFFs apply before or after gamma, since atomic is specific about what order they happen in. I didn't find anything in the docs, so I'd have to pull up RTL to confirm. > --- > v2: Simplify CTM atomic check (Ville) > > drivers/gpu/drm/vc4/vc4_crtc.c | 97 ++++++++++++++++++++++++++++++++++++= ++++-- > 1 file changed, 94 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crt= c.c > index 8d71098d00c4..bafb0102fe1d 100644 > --- a/drivers/gpu/drm/vc4/vc4_crtc.c > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c > @@ -315,6 +315,79 @@ vc4_crtc_update_gamma_lut(struct drm_crtc *crtc) > vc4_crtc_lut_load(crtc); > } >=20=20 > +/* Converts a DRM S31.32 value to the HW S0.9 format. */ > +static u16 vc4_crtc_s31_32_to_s0_9(u64 in) > +{ > + u16 r; > + > + /* Sign bit. */ > + r =3D in & BIT_ULL(63) ? BIT(9) : 0; > + /* We have zero integer bits so we can only saturate here. */ > + if ((in & GENMASK_ULL(62, 32)) > 0) > + r |=3D GENMASK(8, 0); > + /* Otherwise take the 9 most important fractional bits. */ > + else > + r |=3D (in >> 22) & GENMASK(8, 0); > + return r; > +} > + > +static void > +vc4_crtc_update_ctm(struct drm_crtc *crtc) > +{ > + struct drm_device *dev =3D crtc->dev; > + struct vc4_dev *vc4 =3D to_vc4_dev(dev); > + struct vc4_crtc *vc4_crtc =3D to_vc4_crtc(crtc); > + struct drm_color_ctm *ctm =3D crtc->state->ctm->data; > + > + HVS_WRITE(SCALER_OLEDCOEF2, > + VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[0]), > + SCALER_OLEDCOEF2_R_TO_R) | > + VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[3]), > + SCALER_OLEDCOEF2_R_TO_G) | > + VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[6]), > + SCALER_OLEDCOEF2_R_TO_B)); > + HVS_WRITE(SCALER_OLEDCOEF1, > + VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[1]), > + SCALER_OLEDCOEF1_G_TO_R) | > + VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[4]), > + SCALER_OLEDCOEF1_G_TO_G) | > + VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[7]), > + SCALER_OLEDCOEF1_G_TO_B)); > + HVS_WRITE(SCALER_OLEDCOEF0, > + VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[2]), > + SCALER_OLEDCOEF0_B_TO_R) | > + VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[5]), > + SCALER_OLEDCOEF0_B_TO_G) | > + VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[8]), > + SCALER_OLEDCOEF0_B_TO_B)); > + > + /* Channel is 0-based but for DISPFIFO, 0 means disabled. */ > + HVS_WRITE(SCALER_OLEDOFFS, VC4_SET_FIELD(vc4_crtc->channel + 1, > + SCALER_OLEDOFFS_DISPFIFO)); > +} > + > +/* Check if the CTM contains valid input. > + * > + * DRM exposes CTM with S31.32 scalars, but the HW only supports S0.9. > + * We don't allow integer values >1, and 1 only without fractional part > + * to handle the common 1.0 value. > + */ > +static int vc4_crtc_atomic_check_ctm(struct drm_crtc_state *state) > +{ > + struct drm_color_ctm *ctm =3D state->ctm->data; > + u32 i; > + > + for (i =3D 0; i < ARRAY_SIZE(ctm->matrix); i++) { > + u64 val =3D ctm->matrix[i]; > + > + val &=3D ~BIT_ULL(63); > + if (val > BIT_ULL(32)) > + return -EINVAL; > + } > + > + return 0; > +} > + > static u32 vc4_get_fifo_full_level(u32 format) > { > static const u32 fifo_len_bytes =3D 64; > @@ -621,6 +694,15 @@ static int vc4_crtc_atomic_check(struct drm_crtc *cr= tc, > if (hweight32(state->connector_mask) > 1) > return -EINVAL; >=20=20 > + if (state->ctm) { > + /* The CTM hardware has no integer bits, so we check > + * and reject scalars >1.0 that we have no chance of > + * approximating. > + */ > + if (vc4_crtc_atomic_check_ctm(state)) > + return -EINVAL; > + } > + > drm_atomic_crtc_state_for_each_plane_state(plane, plane_state, state) > dlist_count +=3D vc4_plane_dlist_size(plane_state); >=20=20 > @@ -697,8 +779,17 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *c= rtc, > if (crtc->state->active && old_state->active) > vc4_crtc_update_dlist(crtc); >=20=20 > - if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut) > - vc4_crtc_update_gamma_lut(crtc); > + if (crtc->state->color_mgmt_changed) { > + if (crtc->state->gamma_lut) > + vc4_crtc_update_gamma_lut(crtc); > + > + if (crtc->state->ctm) > + vc4_crtc_update_ctm(crtc); > + /* We are transitioning to CTM disabled. */ > + else if (old_state->ctm) > + HVS_WRITE(SCALER_OLEDOFFS, > + VC4_SET_FIELD(0, SCALER_OLEDOFFS_DISPFIFO)); > + } I find splitting the if from else without braces weird and error-prone. Could we rewrite as: + if (crtc->state->ctm) + vc4_crtc_update_ctm(crtc); + else if (old_state->ctm) { + /* We are transitioning to CTM disabled. */ + HVS_WRITE(SCALER_OLEDOFFS, + VC4_SET_FIELD(0, SCALER_OLEDOFFS_DISPFIFO)); + } + } Also, I think there might be a problem if you swap from CTM on CRTC1 to CRTC 0 in a single commit -- CRTC0's CTM setup ends up getting disbaled by CRTC1. I think this should go away once you start tracking who's doing CTM at the atomic state level, and put the SCALER_OLEDOFFS update into the top-level atomic flush. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAlq+YaEACgkQtdYpNtH8 nuiaoQ/+JEk3J4kEiLth5AUTHen5o7r1p/xj9leP/Q6xV6eRkqHu7sMWzMPDdtQj gOUOdhlPb/wzXTL5sNgK2UMjOM9wvD+4i7LXl2AKnGVupLY+T/Xn2jGfajtC0QBq XrbN3eblt4l6oV1brn537xolzL68NBRXwmyKLgwlLyWmqqyyMKmgn1FrML4JsUgD E23oJqL7TGeIlPP1qe8T+kUpvtnlhjDLJHm5ChfBgez1PjfKYk/Ts7chxnEXe7VL KLwbxTJGZXPGlHMb/RuXGA3IW0TGerVBy/oAuuvdV4GtPhK8p2Iz2MigefBZahfv fNJLX0pBWLyqoxOC3N+KVPfZrgQpyK4jyPhJgzS7ovRSmbJxlg4F8YYcT5qUXmTm lJQqUW9ICKBjLJR1giFisKU4lhrIs9ls7Vxa0NLBEw/WjC0olgjJT5VcYfFphq1Y FO5/mwFT1so2aT4CqWOwQWuUlwORKLAewcUwDiJt5U+apVgKc4bSmA4iW8eREg03 kSr76fqXv4pYSQbmrNXMYVAHGaL+zmv7tP2alX9PctmngQMj9Sc7L/1k47IDGlD3 ia8DlcZcKH/jbY56Ip3xagCRqpp/Tq9uSPDtl9LO4iW8BDFcqr57ZZi5AWzMRfwy b37rk1yPP2/zMMp/zTJAsWpcaSQi+3srCEYV5+b+grGV2T9EIvw= =3TSR -----END PGP SIGNATURE----- --=-=-=-- --===============1091402847== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1091402847==--