| Message ID | 1485950703-11163-1-git-send-email-mihail.atanassov@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
+Daniel to weigh in on the coefficient conversion. On Wed, Feb 01, 2017 at 12:05:03PM +0000, Mihail Atanassov wrote: >All DPs have a COLORADJ matrix which is applied prior to output gamma. >Attach that to the CTM property. Also, ensure the input CTM's coefficients >can fit in the DP registers' Q3.12 format. > >Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com> >--- > >This patch depends on "[PATCH 2/2] drm: mali-dp: enable gamma support", sent >out earlier (https://lkml.org/lkml/2017/2/1/201). > > drivers/gpu/drm/arm/malidp_crtc.c | 59 +++++++++++++++++++++++++++++++++++++-- > drivers/gpu/drm/arm/malidp_drv.c | 35 +++++++++++++++++++++++ > drivers/gpu/drm/arm/malidp_drv.h | 1 + > drivers/gpu/drm/arm/malidp_regs.h | 2 ++ > 4 files changed, 95 insertions(+), 2 deletions(-) > >diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c >index 10f79b6..2f366e4 100644 >--- a/drivers/gpu/drm/arm/malidp_crtc.c >+++ b/drivers/gpu/drm/arm/malidp_crtc.c >@@ -190,6 +190,55 @@ static int malidp_crtc_atomic_check_gamma(struct drm_crtc *crtc, > return 0; > } > >+/* >+ * Check if there is a new CTM and if it contains valid input. Valid here means >+ * that the number is inside the representable range for a Q3.12 number, >+ * excluding truncating the fractional part of the input data. >+ * >+ * The COLORADJ registers can be changed atomically. >+ */ >+static int malidp_crtc_atomic_check_ctm(struct drm_crtc *crtc, >+ struct drm_crtc_state *state) >+{ >+ struct malidp_crtc_state *mc = to_malidp_crtc_state(state); >+ struct drm_color_ctm *ctm; >+ int i; >+ >+ if (!state->color_mgmt_changed) >+ return 0; >+ >+ if (!state->ctm) >+ return 0; >+ >+ if (crtc->state->ctm && (crtc->state->ctm->base.id == >+ state->ctm->base.id)) >+ return 0; >+ >+ /* >+ * The size of the ctm is checked in >+ * drm_atomic_replace_property_blob_from_id. >+ */ >+ ctm = (struct drm_color_ctm *)state->ctm->data; >+ for (i = 0; i < ARRAY_SIZE(ctm->matrix); ++i) { >+ /* Convert from S31.32 to Q3.12. */ >+ s64 val = ctm->matrix[i]; >+ u64 mag = ((((u64)val) & ~BIT_ULL(63)) >> 20) & >+ GENMASK_ULL(14, 0); nitpick: You don't need 64 bits for mag. Daniel was suggesting implementing a helper to change the S31.32 into something more useful (presumably Q32.32), as probably the sign+magnitude format isn't really useful to most drivers. I'm not sure what he had in mind exactly; whether we should convert the whole matrix up-front into a new copy, or just provide a helper for drivers to use when reading the matrix. >+ >+ /* Check the destination's top bit for overflow. */ ... and also convert to two's complement. For the overflow check though I think it would be more obvious to look at bit 34 in the original S31.32 value (or bit 14 before you mask it). There's no trickiness around inverting then. -Brian >+ if (val & BIT_ULL(63)) { >+ mag = ~mag + 1; >+ if (!(mag & BIT_ULL(14))) >+ return -EINVAL; >+ } else if (mag & BIT_ULL(14)) { >+ return -EINVAL; >+ } >+ mc->coloradj_coeffs[i] = mag; >+ } >+ >+ return 0; >+} >+ > static int malidp_crtc_atomic_check(struct drm_crtc *crtc, > struct drm_crtc_state *state) > { >@@ -270,6 +319,10 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc, > if (ret) > return ret; > >+ ret = malidp_crtc_atomic_check_ctm(crtc, state); >+ if (ret) >+ return ret; >+ > return 0; > } > >@@ -289,6 +342,8 @@ static struct drm_crtc_state *malidp_crtc_duplicate_state(struct drm_crtc *crtc) > __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base); > memcpy(state->gamma_coeffs, cs->gamma_coeffs, > sizeof(state->gamma_coeffs)); >+ memcpy(state->coloradj_coeffs, cs->coloradj_coeffs, >+ sizeof(state->coloradj_coeffs)); > > return &state->base; > } >@@ -359,10 +414,10 @@ int malidp_crtc_init(struct drm_device *drm) > > drm_crtc_helper_add(&malidp->crtc, &malidp_crtc_helper_funcs); > drm_mode_crtc_set_gamma_size(&malidp->crtc, 4096); >- /* No inverse-gamma and color adjustments yet. */ >+ /* No inverse-gamma: it is per-plane. */ > drm_crtc_enable_color_mgmt(&malidp->crtc, > 0, >- false, >+ true, > 4096); > return 0; > >diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c >index a9f787d..682901a 100644 >--- a/drivers/gpu/drm/arm/malidp_drv.c >+++ b/drivers/gpu/drm/arm/malidp_drv.c >@@ -78,6 +78,37 @@ static void malidp_atomic_commit_update_gamma(struct drm_crtc *crtc, > } > } > >+static >+void malidp_atomic_commit_update_coloradj(struct drm_crtc *crtc, >+ struct drm_crtc_state *old_state) >+{ >+ struct malidp_drm *malidp = crtc_to_malidp_device(crtc); >+ struct malidp_hw_device *hwdev = malidp->dev; >+ int i; >+ >+ if (!crtc->state->color_mgmt_changed) >+ return; >+ >+ if (!crtc->state->ctm) { >+ malidp_hw_clearbits(hwdev, MALIDP_DISP_FUNC_CADJ, >+ MALIDP_DE_DISPLAY_FUNC); >+ } else { >+ struct malidp_crtc_state *mc = >+ to_malidp_crtc_state(crtc->state); >+ >+ if (!old_state->ctm || (crtc->state->ctm->base.id != >+ old_state->ctm->base.id)) >+ for (i = 0; i < MALIDP_COLORADJ_NUM_COEFFS; ++i) >+ malidp_hw_write(hwdev, >+ mc->coloradj_coeffs[i], >+ hwdev->map.coeffs_base + >+ MALIDP_COLOR_ADJ_COEF + 4 * i); >+ >+ malidp_hw_setbits(hwdev, MALIDP_DISP_FUNC_CADJ, >+ MALIDP_DE_DISPLAY_FUNC); >+ } >+} >+ > /* > * set the "config valid" bit and wait until the hardware acts on it > */ >@@ -144,6 +175,10 @@ static void malidp_atomic_commit_tail(struct drm_atomic_state *state) > malidp_atomic_commit_update_gamma(crtc, old_crtc_state); > > drm_atomic_helper_commit_modeset_enables(drm, state); >+ >+ for_each_crtc_in_state(state, crtc, old_crtc_state, i) >+ malidp_atomic_commit_update_coloradj(crtc, old_crtc_state); >+ > drm_atomic_helper_commit_planes(drm, state, 0); > > malidp_atomic_commit_hw_done(state); >diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h >index 374d43e..55b0f8b 100644 >--- a/drivers/gpu/drm/arm/malidp_drv.h >+++ b/drivers/gpu/drm/arm/malidp_drv.h >@@ -50,6 +50,7 @@ struct malidp_plane_state { > struct malidp_crtc_state { > struct drm_crtc_state base; > u32 gamma_coeffs[MALIDP_COEFFTAB_NUM_COEFFS]; >+ u32 coloradj_coeffs[MALIDP_COLORADJ_NUM_COEFFS]; > }; > > #define to_malidp_crtc_state(x) container_of(x, struct malidp_crtc_state, base) >diff --git a/drivers/gpu/drm/arm/malidp_regs.h b/drivers/gpu/drm/arm/malidp_regs.h >index 2e213c3..b15ef50 100644 >--- a/drivers/gpu/drm/arm/malidp_regs.h >+++ b/drivers/gpu/drm/arm/malidp_regs.h >@@ -64,6 +64,7 @@ > /* bit masks that are common between products */ > #define MALIDP_CFG_VALID (1 << 0) > #define MALIDP_DISP_FUNC_GAM (1 << 0) >+#define MALIDP_DISP_FUNC_CADJ (1 << 4) > #define MALIDP_DISP_FUNC_ILACED (1 << 8) > > /* register offsets for IRQ management */ >@@ -93,6 +94,7 @@ > #define MALIDP_DE_H_ACTIVE(x) (((x) & 0x1fff) << 0) > #define MALIDP_DE_V_ACTIVE(x) (((x) & 0x1fff) << 16) > >+#define MALIDP_COLORADJ_NUM_COEFFS 12 > #define MALIDP_COEFFTAB_NUM_COEFFS 64 > /* register offsets relative to MALIDP5x0_COEFFS_BASE */ > #define MALIDP_COLOR_ADJ_COEF 0x00000 >-- >1.9.1 >
diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c index 10f79b6..2f366e4 100644 --- a/drivers/gpu/drm/arm/malidp_crtc.c +++ b/drivers/gpu/drm/arm/malidp_crtc.c @@ -190,6 +190,55 @@ static int malidp_crtc_atomic_check_gamma(struct drm_crtc *crtc, return 0; } +/* + * Check if there is a new CTM and if it contains valid input. Valid here means + * that the number is inside the representable range for a Q3.12 number, + * excluding truncating the fractional part of the input data. + * + * The COLORADJ registers can be changed atomically. + */ +static int malidp_crtc_atomic_check_ctm(struct drm_crtc *crtc, + struct drm_crtc_state *state) +{ + struct malidp_crtc_state *mc = to_malidp_crtc_state(state); + struct drm_color_ctm *ctm; + int i; + + if (!state->color_mgmt_changed) + return 0; + + if (!state->ctm) + return 0; + + if (crtc->state->ctm && (crtc->state->ctm->base.id == + state->ctm->base.id)) + return 0; + + /* + * The size of the ctm is checked in + * drm_atomic_replace_property_blob_from_id. + */ + ctm = (struct drm_color_ctm *)state->ctm->data; + for (i = 0; i < ARRAY_SIZE(ctm->matrix); ++i) { + /* Convert from S31.32 to Q3.12. */ + s64 val = ctm->matrix[i]; + u64 mag = ((((u64)val) & ~BIT_ULL(63)) >> 20) & + GENMASK_ULL(14, 0); + + /* Check the destination's top bit for overflow. */ + if (val & BIT_ULL(63)) { + mag = ~mag + 1; + if (!(mag & BIT_ULL(14))) + return -EINVAL; + } else if (mag & BIT_ULL(14)) { + return -EINVAL; + } + mc->coloradj_coeffs[i] = mag; + } + + return 0; +} + static int malidp_crtc_atomic_check(struct drm_crtc *crtc, struct drm_crtc_state *state) { @@ -270,6 +319,10 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc, if (ret) return ret; + ret = malidp_crtc_atomic_check_ctm(crtc, state); + if (ret) + return ret; + return 0; } @@ -289,6 +342,8 @@ static struct drm_crtc_state *malidp_crtc_duplicate_state(struct drm_crtc *crtc) __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base); memcpy(state->gamma_coeffs, cs->gamma_coeffs, sizeof(state->gamma_coeffs)); + memcpy(state->coloradj_coeffs, cs->coloradj_coeffs, + sizeof(state->coloradj_coeffs)); return &state->base; } @@ -359,10 +414,10 @@ int malidp_crtc_init(struct drm_device *drm) drm_crtc_helper_add(&malidp->crtc, &malidp_crtc_helper_funcs); drm_mode_crtc_set_gamma_size(&malidp->crtc, 4096); - /* No inverse-gamma and color adjustments yet. */ + /* No inverse-gamma: it is per-plane. */ drm_crtc_enable_color_mgmt(&malidp->crtc, 0, - false, + true, 4096); return 0; diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c index a9f787d..682901a 100644 --- a/drivers/gpu/drm/arm/malidp_drv.c +++ b/drivers/gpu/drm/arm/malidp_drv.c @@ -78,6 +78,37 @@ static void malidp_atomic_commit_update_gamma(struct drm_crtc *crtc, } } +static +void malidp_atomic_commit_update_coloradj(struct drm_crtc *crtc, + struct drm_crtc_state *old_state) +{ + struct malidp_drm *malidp = crtc_to_malidp_device(crtc); + struct malidp_hw_device *hwdev = malidp->dev; + int i; + + if (!crtc->state->color_mgmt_changed) + return; + + if (!crtc->state->ctm) { + malidp_hw_clearbits(hwdev, MALIDP_DISP_FUNC_CADJ, + MALIDP_DE_DISPLAY_FUNC); + } else { + struct malidp_crtc_state *mc = + to_malidp_crtc_state(crtc->state); + + if (!old_state->ctm || (crtc->state->ctm->base.id != + old_state->ctm->base.id)) + for (i = 0; i < MALIDP_COLORADJ_NUM_COEFFS; ++i) + malidp_hw_write(hwdev, + mc->coloradj_coeffs[i], + hwdev->map.coeffs_base + + MALIDP_COLOR_ADJ_COEF + 4 * i); + + malidp_hw_setbits(hwdev, MALIDP_DISP_FUNC_CADJ, + MALIDP_DE_DISPLAY_FUNC); + } +} + /* * set the "config valid" bit and wait until the hardware acts on it */ @@ -144,6 +175,10 @@ static void malidp_atomic_commit_tail(struct drm_atomic_state *state) malidp_atomic_commit_update_gamma(crtc, old_crtc_state); drm_atomic_helper_commit_modeset_enables(drm, state); + + for_each_crtc_in_state(state, crtc, old_crtc_state, i) + malidp_atomic_commit_update_coloradj(crtc, old_crtc_state); + drm_atomic_helper_commit_planes(drm, state, 0); malidp_atomic_commit_hw_done(state); diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h index 374d43e..55b0f8b 100644 --- a/drivers/gpu/drm/arm/malidp_drv.h +++ b/drivers/gpu/drm/arm/malidp_drv.h @@ -50,6 +50,7 @@ struct malidp_plane_state { struct malidp_crtc_state { struct drm_crtc_state base; u32 gamma_coeffs[MALIDP_COEFFTAB_NUM_COEFFS]; + u32 coloradj_coeffs[MALIDP_COLORADJ_NUM_COEFFS]; }; #define to_malidp_crtc_state(x) container_of(x, struct malidp_crtc_state, base) diff --git a/drivers/gpu/drm/arm/malidp_regs.h b/drivers/gpu/drm/arm/malidp_regs.h index 2e213c3..b15ef50 100644 --- a/drivers/gpu/drm/arm/malidp_regs.h +++ b/drivers/gpu/drm/arm/malidp_regs.h @@ -64,6 +64,7 @@ /* bit masks that are common between products */ #define MALIDP_CFG_VALID (1 << 0) #define MALIDP_DISP_FUNC_GAM (1 << 0) +#define MALIDP_DISP_FUNC_CADJ (1 << 4) #define MALIDP_DISP_FUNC_ILACED (1 << 8) /* register offsets for IRQ management */ @@ -93,6 +94,7 @@ #define MALIDP_DE_H_ACTIVE(x) (((x) & 0x1fff) << 0) #define MALIDP_DE_V_ACTIVE(x) (((x) & 0x1fff) << 16) +#define MALIDP_COLORADJ_NUM_COEFFS 12 #define MALIDP_COEFFTAB_NUM_COEFFS 64 /* register offsets relative to MALIDP5x0_COEFFS_BASE */ #define MALIDP_COLOR_ADJ_COEF 0x00000
All DPs have a COLORADJ matrix which is applied prior to output gamma. Attach that to the CTM property. Also, ensure the input CTM's coefficients can fit in the DP registers' Q3.12 format. Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com> --- This patch depends on "[PATCH 2/2] drm: mali-dp: enable gamma support", sent out earlier (https://lkml.org/lkml/2017/2/1/201). drivers/gpu/drm/arm/malidp_crtc.c | 59 +++++++++++++++++++++++++++++++++++++-- drivers/gpu/drm/arm/malidp_drv.c | 35 +++++++++++++++++++++++ drivers/gpu/drm/arm/malidp_drv.h | 1 + drivers/gpu/drm/arm/malidp_regs.h | 2 ++ 4 files changed, 95 insertions(+), 2 deletions(-)