Stefan Schake <stsch...@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 <stsch...@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.
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
