Re: [PATCH 2/3] drm/vc4: Add color transformation matrix (CTM) support

[Ville Syrjälä]

On Fri, Mar 16, 2018 at 10:50:58PM +0100, Stefan Schake wrote:
> 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 
> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 99 
> --
>  1 file changed, 96 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 8d71098..5c83fd2 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -315,6 +315,81 @@ 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 >> 32) > 1)
> + return -EINVAL;
> + if ((val >> 32) == 1 && (val & GENMASK_ULL(31, 0)) != 0)
> + return -EINVAL;

'val > BIT_ULL(32)' ?

> + }
> +
> + return 0;
> +}
> +
>  static u32 vc4_get_fifo_full_level(u32 format)
>  {
>   static const u32 fifo_len_bytes = 64;
> @@ -621,6 +696,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 +781,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 

Re: [PATCH 2/3] drm/vc4: Add color transformation matrix (CTM) support

[kbuild test robot]

Hi Stefan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v4.16-rc4]
[also build test ERROR on next-20180316]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Stefan-Schake/drm-vc4-Atomic-color-management-support/20180318-120701
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   drivers/gpu/drm/vc4/vc4_crtc.c: In function 'vc4_crtc_update_gamma_lut':
   drivers/gpu/drm/vc4/vc4_crtc.c:305:30: error: initialization from 
incompatible pointer type [-Werror=incompatible-pointer-types]
 struct drm_color_lut *lut = crtc->state->gamma_lut->data;
 ^~~~
   drivers/gpu/drm/vc4/vc4_crtc.c: In function 'vc4_crtc_update_ctm':
   drivers/gpu/drm/vc4/vc4_crtc.c:340:30: error: initialization from 
incompatible pointer type [-Werror=incompatible-pointer-types]
 struct drm_color_ctm *ctm = crtc->state->ctm->data;
 ^~~~
   In file included from drivers/gpu/drm/vc4/vc4_crtc.c:42:0:
>> drivers/gpu/drm/vc4/vc4_crtc.c:344:5: error: 'SCALER_OLEDCOEF2_R_TO_R_SHIFT' 
>> undeclared (first use in this function); did you mean 
>> 'SCALER_CSC0_COEF_CR_OFS_SHIFT'?
SCALER_OLEDCOEF2_R_TO_R) |
^
   drivers/gpu/drm/vc4/vc4_drv.h:301:39: note: in definition of macro 
'HVS_WRITE'
#define HVS_WRITE(offset, val) writel(val, vc4->hvs->regs + offset)
  ^~~
>> drivers/gpu/drm/vc4/vc4_crtc.c:343:5: note: in expansion of macro 
>> 'VC4_SET_FIELD'
VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[0]),
^
   drivers/gpu/drm/vc4/vc4_crtc.c:344:5: note: each undeclared identifier is 
reported only once for each function it appears in
SCALER_OLEDCOEF2_R_TO_R) |
^
   drivers/gpu/drm/vc4/vc4_drv.h:301:39: note: in definition of macro 
'HVS_WRITE'
#define HVS_WRITE(offset, val) writel(val, vc4->hvs->regs + offset)
  ^~~
>> drivers/gpu/drm/vc4/vc4_crtc.c:343:5: note: in expansion of macro 
>> 'VC4_SET_FIELD'
VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[0]),
^
>> drivers/gpu/drm/vc4/vc4_crtc.c:344:5: error: 'SCALER_OLEDCOEF2_R_TO_R_MASK' 
>> undeclared (first use in this function); did you mean 
>> 'SCALER_OLEDCOEF2_R_TO_R_SHIFT'?
SCALER_OLEDCOEF2_R_TO_R) |
^
   drivers/gpu/drm/vc4/vc4_drv.h:301:39: note: in definition of macro 
'HVS_WRITE'
#define HVS_WRITE(offset, val) writel(val, vc4->hvs->regs + offset)
  ^~~
>> drivers/gpu/drm/vc4/vc4_regs.h:19:3: note: in expansion of macro 'WARN_ON'
  WARN_ON((fieldval & ~field##_MASK) != 0);  \
  ^~~
>> drivers/gpu/drm/vc4/vc4_crtc.c:343:5: note: in expansion of macro 
>> 'VC4_SET_FIELD'
VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[0]),
^
>> drivers/gpu/drm/vc4/vc4_crtc.c:346:5: error: 'SCALER_OLEDCOEF2_R_TO_G_SHIFT' 
>> undeclared (first use in this function); did you mean 
>> 'SCALER_OLEDCOEF2_R_TO_R_SHIFT'?
SCALER_OLEDCOEF2_R_TO_G) |
^
   drivers/gpu/drm/vc4/vc4_drv.h:301:39: note: in definition of macro 
'HVS_WRITE'
#define HVS_WRITE(offset, val) writel(val, vc4->hvs->regs + offset)
  ^~~
   drivers/gpu/drm/vc4/vc4_crtc.c:345:5: note: in expansion of macro 
'VC4_SET_FIELD'
VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[3]),
^
>> drivers/gpu/drm/vc4/vc4_crtc.c:346:5: error: 'SCALER_OLEDCOEF2_R_TO_G_MASK' 
>> undeclared (first use in this function); did you mean 
>> 'SCALER_OLEDCOEF2_R_TO_R_MASK'?
SCALER_OLEDCOEF2_R_TO_G) |
^
   drivers/gpu/drm/vc4/vc4_drv.h:301:39: note: in definition of macro 
'HVS_WRITE'
#define HVS_WRITE(offset, val) writel(val, vc4->hvs->regs + offset)
  ^~~
>> drivers/gpu/drm/vc4/vc4_regs.h:19:3: note: in expansion of macro 'WARN_ON'
  WARN_ON((fieldval & ~field##_MASK) != 0);  \
  ^~~
   drivers/gpu/drm/vc4/vc4_crtc.c:345:5: note: in expansion of macro 
'VC4_SET_FIELD'
VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[3]),
^
>> drivers/gpu/drm/vc4/vc4_crtc.c:348:5: error: 'SCALER_OLEDCOEF2_R_TO_B_SHIFT' 
>> undeclared (first use in this function); did you mean 
>> 'SCALER_OLEDCOEF2_R_TO_G_SHIFT'?
SCALER_OLEDCOEF2_R_TO_B));
^
   drivers/gpu/drm/vc4/vc4_drv.h:301:39: note: in definition of macro 
'HVS_WRITE'
#define HVS_WRITE(offset, val) writel(val, vc4->hvs->regs + offset)
  ^~~
   drivers/gpu/drm/vc4/vc4_crtc.c:347:5: note: in expansion of macro 
'VC4_SET_FIELD'

Re: [PATCH 2/3] drm/vc4: Add color transformation matrix (CTM) support

[kbuild test robot]

Hi Stefan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v4.16-rc4]
[also build test WARNING on next-20180316]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Stefan-Schake/drm-vc4-Atomic-color-management-support/20180318-120701
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sh 

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/vc4/vc4_crtc.c: In function 'vc4_crtc_update_gamma_lut':
   drivers/gpu/drm/vc4/vc4_crtc.c:305:30: error: initialization from 
incompatible pointer type [-Werror=incompatible-pointer-types]
 struct drm_color_lut *lut = crtc->state->gamma_lut->data;
 ^~~~
   drivers/gpu/drm/vc4/vc4_crtc.c: In function 'vc4_crtc_update_ctm':
   drivers/gpu/drm/vc4/vc4_crtc.c:340:30: error: initialization from 
incompatible pointer type [-Werror=incompatible-pointer-types]
 struct drm_color_ctm *ctm = crtc->state->ctm->data;
 ^~~~
   In file included from include/linux/fb.h:17:0,
from include/drm/drm_crtc.h:31,
from include/drm/drm_atomic.h:31,
from drivers/gpu/drm/vc4/vc4_crtc.c:35:
   drivers/gpu/drm/vc4/vc4_crtc.c:342:12: error: 'SCALER_OLEDCOEF2' undeclared 
(first use in this function); did you mean 'SCALER_DISPCTRL2'?
 HVS_WRITE(SCALER_OLEDCOEF2,
   ^
   arch/sh/include/asm/io.h:31:71: note: in definition of macro '__raw_writel'
#define __raw_writel(v,a) (__chk_io_ptr(a), *(volatile u32 __force *)(a) = 
(v))
  ^
   arch/sh/include/asm/io.h:56:32: note: in expansion of macro 'writel_relaxed'
#define writel(v,a)  ({ wmb(); writel_relaxed((v),(a)); })
   ^~
>> drivers/gpu/drm/vc4/vc4_drv.h:301:32: note: in expansion of macro 'writel'
#define HVS_WRITE(offset, val) writel(val, vc4->hvs->regs + offset)
   ^~
>> drivers/gpu/drm/vc4/vc4_crtc.c:342:2: note: in expansion of macro 'HVS_WRITE'
 HVS_WRITE(SCALER_OLEDCOEF2,
 ^
   drivers/gpu/drm/vc4/vc4_crtc.c:342:12: note: each undeclared identifier is 
reported only once for each function it appears in
 HVS_WRITE(SCALER_OLEDCOEF2,
   ^
   arch/sh/include/asm/io.h:31:71: note: in definition of macro '__raw_writel'
#define __raw_writel(v,a) (__chk_io_ptr(a), *(volatile u32 __force *)(a) = 
(v))
  ^
   arch/sh/include/asm/io.h:56:32: note: in expansion of macro 'writel_relaxed'
#define writel(v,a)  ({ wmb(); writel_relaxed((v),(a)); })
   ^~
>> drivers/gpu/drm/vc4/vc4_drv.h:301:32: note: in expansion of macro 'writel'
#define HVS_WRITE(offset, val) writel(val, vc4->hvs->regs + offset)
   ^~
>> drivers/gpu/drm/vc4/vc4_crtc.c:342:2: note: in expansion of macro 'HVS_WRITE'
 HVS_WRITE(SCALER_OLEDCOEF2,
 ^
   drivers/gpu/drm/vc4/vc4_crtc.c:344:5: error: 'SCALER_OLEDCOEF2_R_TO_R_SHIFT' 
undeclared (first use in this function); did you mean 
'SCALER_CSC0_COEF_CR_OFS_SHIFT'?
SCALER_OLEDCOEF2_R_TO_R) |
^
   arch/sh/include/asm/io.h:31:77: note: in definition of macro '__raw_writel'
#define __raw_writel(v,a) (__chk_io_ptr(a), *(volatile u32 __force *)(a) = 
(v))

^
   arch/sh/include/asm/io.h:46:62: note: in expansion of macro 'ioswabl'
#define writel_relaxed(v,c) ((void)__raw_writel((__force u32)ioswabl(v),c))
 ^~~
   arch/sh/include/asm/io.h:56:32: note: in expansion of macro 'writel_relaxed'
#define writel(v,a)  ({ wmb(); writel_relaxed((v),(a)); })
   ^~
>> drivers/gpu/drm/vc4/vc4_drv.h:301:32: note: in expansion of macro 'writel'
#define HVS_WRITE(offset, val) writel(val, vc4->hvs->regs + offset)
   ^~
>> drivers/gpu/drm/vc4/vc4_crtc.c:342:2: note: in expansion of macro 'HVS_WRITE'
 HVS_WRITE(SCALER_OLEDCOEF2,
 ^
   drivers/gpu/drm/vc4/vc4_crtc.c:343:5: note: in expansion of macro 
'VC4_SET_FIELD'
VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[0]),
^
   drivers/gpu/drm/vc4/vc4_crtc.c:344:5: error: 'SCALER_OLEDCOEF2_R_TO_R_MASK' 
undeclared (first use in this function); did you mean 
'SCALER_OLEDCOEF2_R_TO_R_SHIFT'?
  

[PATCH 2/3] drm/vc4: Add color transformation matrix (CTM) support

[Stefan Schake]

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 
---
 drivers/gpu/drm/vc4/vc4_crtc.c | 99 --
 1 file changed, 96 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 8d71098..5c83fd2 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -315,6 +315,81 @@ 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 >> 32) > 1)
+   return -EINVAL;
+   if ((val >> 32) == 1 && (val & GENMASK_ULL(31, 0)) != 0)
+   return -EINVAL;
+   }
+
+   return 0;
+}
+
 static u32 vc4_get_fifo_full_level(u32 format)
 {
static const u32 fifo_len_bytes = 64;
@@ -621,6 +696,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 +781,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)
+