+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