Stefan Schake stschake@gmail.com 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 <= 1.0.
Signed-off-by: Stefan Schake stschake@gmail.com
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_crtc.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); }
+/* 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 = 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 |= GENMASK(8, 0);- /* Otherwise take the 9 most important fractional bits. */
- else
r |= (in >> 22) & GENMASK(8, 0);- return r;
+}
+static void +vc4_crtc_update_ctm(struct drm_crtc *crtc) +{
- struct drm_device *dev = crtc->dev;
- struct vc4_dev *vc4 = to_vc4_dev(dev);
- struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
- struct drm_color_ctm *ctm = 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 = state->ctm->data;
- u32 i;
- for (i = 0; i < ARRAY_SIZE(ctm->matrix); i++) {
u64 val = ctm->matrix[i];val &= ~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 = 64; @@ -621,6 +694,15 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc, if (hweight32(state->connector_mask) > 1) return -EINVAL;
- 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 += vc4_plane_dlist_size(plane_state);
@@ -697,8 +779,17 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc, if (crtc->state->active && old_state->active) vc4_crtc_update_dlist(crtc);
- 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.