drm: mali-dp: Add CTM support
diff mbox

Message ID 1485950703-11163-1-git-send-email-mihail.atanassov@arm.com
State New
Headers show

Commit Message

Mihail Atanassov Feb. 1, 2017, 12:05 p.m. UTC
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(-)

Comments

Brian Starkey Feb. 1, 2017, 1:46 p.m. UTC | #1
+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
>

Patch
diff mbox

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