[PATCH 1/2] drm/exynos: mixer: simplify mixer_cfg_rgb_fmt()

[PATCH 1/2] drm/exynos: mixer: simplify mixer_cfg_rgb_fmt()

[Tobias Jakobi]

Convert if-statements to switch statement. Removes
duplicated code.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 72143ac..41d0c36 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -382,29 +382,14 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
 	struct mixer_resources *res = &ctx->mixer_res;
 	u32 val;
 
-	if (height == 480) {
+	switch (height) {
+	case 480:
+	case 576:
 		val = MXR_CFG_RGB601_0_255;
-	} else if (height == 576) {
-		val = MXR_CFG_RGB601_0_255;
-	} else if (height == 720) {
-		val = MXR_CFG_RGB709_16_235;
-		mixer_reg_write(res, MXR_CM_COEFF_Y,
-				(1 << 30) | (94 << 20) | (314 << 10) |
-				(32 << 0));
-		mixer_reg_write(res, MXR_CM_COEFF_CB,
-				(972 << 20) | (851 << 10) | (225 << 0));
-		mixer_reg_write(res, MXR_CM_COEFF_CR,
-				(225 << 20) | (820 << 10) | (1004 << 0));
-	} else if (height == 1080) {
-		val = MXR_CFG_RGB709_16_235;
-		mixer_reg_write(res, MXR_CM_COEFF_Y,
-				(1 << 30) | (94 << 20) | (314 << 10) |
-				(32 << 0));
-		mixer_reg_write(res, MXR_CM_COEFF_CB,
-				(972 << 20) | (851 << 10) | (225 << 0));
-		mixer_reg_write(res, MXR_CM_COEFF_CR,
-				(225 << 20) | (820 << 10) | (1004 << 0));
-	} else {
+		break;
+	case 720:
+	case 1080:
+	default:
 		val = MXR_CFG_RGB709_16_235;
 		mixer_reg_write(res, MXR_CM_COEFF_Y,
 				(1 << 30) | (94 << 20) | (314 << 10) |
@@ -413,6 +398,7 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
 				(972 << 20) | (851 << 10) | (225 << 0));
 		mixer_reg_write(res, MXR_CM_COEFF_CR,
 				(225 << 20) | (820 << 10) | (1004 << 0));
+		break;
 	}
 
 	mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK);
-- 
2.7.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/exynos: mixer: document YCbCr magic numbers

[Tobias Jakobi]

The output stage of the mixer uses YCbCr for the internal
computations, which is the reason that some registers take
YCbCr related data as input. In particular this applies
to MXR_BG_COLOR{0,1,2} and MXR_CM_COEFF_{Y,CB,CR}.

Document the formatting of the data which we write to
these registers.

While at it, unify wording of comments in the register header.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 35 +++++++++++++++++++++++++++--------
 drivers/gpu/drm/exynos/regs-mixer.h   |  7 +++++--
 2 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 41d0c36..3b0b07d 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -45,6 +45,20 @@
 #define MIXER_WIN_NR		3
 #define VP_DEFAULT_WIN		2
 
+/*
+ * Mixer color space conversion coefficient triplet.
+ * Used for CSC from RGB to YCbCr.
+ * Each coefficient is a 10-bit fixed point number with
+ * sign and no integer part, i.e.
+ * [0:8] = fractional part (representing a value y = x / 2^9)
+ * [9] = sign
+ * Negative values are encoded with two's complement.
+ */
+#define MXR_CSC_CT(a0, a1, a2) (((a0) << 20) | ((a1) << 10) | ((a2) << 0))
+
+/* YCbCr value, used for mixer background color configuration. */
+#define MXR_YCBCR_VAL(y, cb, cr) (((y) << 16) | ((cb) << 8) | ((cr) << 0))
+
 /* The pixelformats that are natively supported by the mixer. */
 #define MXR_FORMAT_RGB565	4
 #define MXR_FORMAT_ARGB1555	5
@@ -391,13 +405,18 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
 	case 1080:
 	default:
 		val = MXR_CFG_RGB709_16_235;
+		/*
+		 * Configure the BT.709 CSC matrix M for full range RGB.
+		 *     | 0.1835  0.6132  0.0625|
+		 * M = |-0.1015 -0.3378  0.4394|
+		 *     | 0.4394 -0.3984 -0.0390|
+		 */
 		mixer_reg_write(res, MXR_CM_COEFF_Y,
-				(1 << 30) | (94 << 20) | (314 << 10) |
-				(32 << 0));
+			MXR_CSC_CT(0x05E, 0x13A, 0x020) | MXR_CM_COEFF_RGB_FULL);
 		mixer_reg_write(res, MXR_CM_COEFF_CB,
-				(972 << 20) | (851 << 10) | (225 << 0));
+			MXR_CSC_CT(0x3CC, 0x353, 0x0E1));
 		mixer_reg_write(res, MXR_CM_COEFF_CR,
-				(225 << 20) | (820 << 10) | (1004 << 0));
+			MXR_CSC_CT(0x0E1, 0x334, 0x3EC));
 		break;
 	}
 
@@ -715,10 +734,10 @@ static void mixer_win_reset(struct mixer_context *ctx)
 	/* reset default layer priority */
 	mixer_reg_write(res, MXR_LAYER_CFG, 0);
 
-	/* setting background color */
-	mixer_reg_write(res, MXR_BG_COLOR0, 0x008080);
-	mixer_reg_write(res, MXR_BG_COLOR1, 0x008080);
-	mixer_reg_write(res, MXR_BG_COLOR2, 0x008080);
+	/* set all background colors to RGB (0,0,0) */
+	mixer_reg_write(res, MXR_BG_COLOR0, MXR_YCBCR_VAL(0, 128, 128));
+	mixer_reg_write(res, MXR_BG_COLOR1, MXR_YCBCR_VAL(0, 128, 128));
+	mixer_reg_write(res, MXR_BG_COLOR2, MXR_YCBCR_VAL(0, 128, 128));
 
 	if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) {
 		/* configuration of Video Processor Registers */
diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
index 7f22df5..c311f57 100644
--- a/drivers/gpu/drm/exynos/regs-mixer.h
+++ b/drivers/gpu/drm/exynos/regs-mixer.h
@@ -140,11 +140,11 @@
 #define MXR_INT_EN_VSYNC		(1 << 11)
 #define MXR_INT_EN_ALL			(0x0f << 8)
 
-/* bit for MXR_INT_STATUS */
+/* bits for MXR_INT_STATUS */
 #define MXR_INT_CLEAR_VSYNC		(1 << 11)
 #define MXR_INT_STATUS_VSYNC		(1 << 0)
 
-/* bit for MXR_LAYER_CFG */
+/* bits for MXR_LAYER_CFG */
 #define MXR_LAYER_CFG_GRP1_VAL(x)	MXR_MASK_VAL(x, 11, 8)
 #define MXR_LAYER_CFG_GRP1_MASK		MXR_LAYER_CFG_GRP1_VAL(~0)
 #define MXR_LAYER_CFG_GRP0_VAL(x)	MXR_MASK_VAL(x, 7, 4)
@@ -152,5 +152,8 @@
 #define MXR_LAYER_CFG_VP_VAL(x)		MXR_MASK_VAL(x, 3, 0)
 #define MXR_LAYER_CFG_VP_MASK		MXR_LAYER_CFG_VP_VAL(~0)
 
+/* bits for MXR_CM_COEFF_Y */
+#define MXR_CM_COEFF_RGB_FULL		(1 << 30)
+
 #endif /* SAMSUNG_REGS_MIXER_H */
 
-- 
2.7.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/exynos: mixer: simplify mixer_cfg_rgb_fmt()

[Andrzej Hajda]

Hi Tobias,

On 03.03.2017 14:40, Tobias Jakobi wrote:
> Convert if-statements to switch statement. Removes
> duplicated code.
>
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 30 ++++++++----------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 72143ac..41d0c36 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -382,29 +382,14 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
>  	struct mixer_resources *res = &ctx->mixer_res;
>  	u32 val;
>  
> -	if (height == 480) {
> +	switch (height) {
> +	case 480:
> +	case 576:
>  		val = MXR_CFG_RGB601_0_255;
> -	} else if (height == 576) {
> -		val = MXR_CFG_RGB601_0_255;
> -	} else if (height == 720) {
> -		val = MXR_CFG_RGB709_16_235;
> -		mixer_reg_write(res, MXR_CM_COEFF_Y,
> -				(1 << 30) | (94 << 20) | (314 << 10) |
> -				(32 << 0));
> -		mixer_reg_write(res, MXR_CM_COEFF_CB,
> -				(972 << 20) | (851 << 10) | (225 << 0));
> -		mixer_reg_write(res, MXR_CM_COEFF_CR,
> -				(225 << 20) | (820 << 10) | (1004 << 0));
> -	} else if (height == 1080) {
> -		val = MXR_CFG_RGB709_16_235;
> -		mixer_reg_write(res, MXR_CM_COEFF_Y,
> -				(1 << 30) | (94 << 20) | (314 << 10) |
> -				(32 << 0));
> -		mixer_reg_write(res, MXR_CM_COEFF_CB,
> -				(972 << 20) | (851 << 10) | (225 << 0));
> -		mixer_reg_write(res, MXR_CM_COEFF_CR,
> -				(225 << 20) | (820 << 10) | (1004 << 0));
> -	} else {
> +		break;
> +	case 720:
> +	case 1080:
> +	default:
>  		val = MXR_CFG_RGB709_16_235;
>  		mixer_reg_write(res, MXR_CM_COEFF_Y,
>  				(1 << 30) | (94 << 20) | (314 << 10) |
> @@ -413,6 +398,7 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
>  				(972 << 20) | (851 << 10) | (225 << 0));
>  		mixer_reg_write(res, MXR_CM_COEFF_CR,
>  				(225 << 20) | (820 << 10) | (1004 << 0));
> +		break;
>  	}

Good change.
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

One small nitpick.
As I see this conditional/switch is to decide about BT standard
depending on the height. The similar problem is addressed in exynos-gsc
patches [1].
It would be good to have the same criteria to distinguish SD/HD mode. I
think ((height > 576) || (width > 720)) is more generic, in this case
even (height > 576) looks better, but as this changes logic of the code
it could be in separate patch.

[1]: https://lkml.org/lkml/2017/2/21/864

Regards
Andrzej

>  
>  	mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK);

Re: [PATCH 2/2] drm/exynos: mixer: document YCbCr magic numbers

[Andrzej Hajda]

On 03.03.2017 14:40, Tobias Jakobi wrote:
> The output stage of the mixer uses YCbCr for the internal
> computations, which is the reason that some registers take
> YCbCr related data as input. In particular this applies
> to MXR_BG_COLOR{0,1,2} and MXR_CM_COEFF_{Y,CB,CR}.
>
> Document the formatting of the data which we write to
> these registers.
>
> While at it, unify wording of comments in the register header.
>
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 35 +++++++++++++++++++++++++++--------
>  drivers/gpu/drm/exynos/regs-mixer.h   |  7 +++++--
>  2 files changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 41d0c36..3b0b07d 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -45,6 +45,20 @@
>  #define MIXER_WIN_NR		3
>  #define VP_DEFAULT_WIN		2
>  
> +/*
> + * Mixer color space conversion coefficient triplet.
> + * Used for CSC from RGB to YCbCr.
> + * Each coefficient is a 10-bit fixed point number with
> + * sign and no integer part, i.e.
Maybe it would be more clear to say in exclusive range (-1,1)
> + * [0:8] = fractional part (representing a value y = x / 2^9)
> + * [9] = sign
> + * Negative values are encoded with two's complement.
> + */
> +#define MXR_CSC_CT(a0, a1, a2) (((a0) << 20) | ((a1) << 10) | ((a2) << 0))

We can take advantage of the fact that floating point numbers are
allowed in compile time, aren't they?

#define MXR_CSC_C(x) ((int)((x) * 512) & 0x3ff)
#define MXR_CSC_CT(a0, a1, a2) ((MXR_CSC_C(a0) << 20) | (MXR_CSC_C(a1)
<< 10) | (MXR_CSC_C(a2) << 0))

and stop using magic hexadecimals, now we will use real coefficients.
    MXR_CSC_CT(0.1835,  0.6132,  0.0625)

> +
> +/* YCbCr value, used for mixer background color configuration. */
> +#define MXR_YCBCR_VAL(y, cb, cr) (((y) << 16) | ((cb) << 8) | ((cr) << 0))
> +
>  /* The pixelformats that are natively supported by the mixer. */
>  #define MXR_FORMAT_RGB565	4
>  #define MXR_FORMAT_ARGB1555	5
> @@ -391,13 +405,18 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
>  	case 1080:
>  	default:
>  		val = MXR_CFG_RGB709_16_235;
> +		/*
> +		 * Configure the BT.709 CSC matrix M for full range RGB.
> +		 *     | 0.1835  0.6132  0.0625|
> +		 * M = |-0.1015 -0.3378  0.4394|
> +		 *     | 0.4394 -0.3984 -0.0390|

Out of curiosity, where did you take those values from, datasheet for
Exynos5422 has little bit different matrix.

Regards
Andrzej

> +		 */
>  		mixer_reg_write(res, MXR_CM_COEFF_Y,
> -				(1 << 30) | (94 << 20) | (314 << 10) |
> -				(32 << 0));
> +			MXR_CSC_CT(0x05E, 0x13A, 0x020) | MXR_CM_COEFF_RGB_FULL);

>  		mixer_reg_write(res, MXR_CM_COEFF_CB,
> -				(972 << 20) | (851 << 10) | (225 << 0));
> +			MXR_CSC_CT(0x3CC, 0x353, 0x0E1));
>  		mixer_reg_write(res, MXR_CM_COEFF_CR,
> -				(225 << 20) | (820 << 10) | (1004 << 0));
> +			MXR_CSC_CT(0x0E1, 0x334, 0x3EC));
>  		break;
>  	}
>  
> @@ -715,10 +734,10 @@ static void mixer_win_reset(struct mixer_context *ctx)
>  	/* reset default layer priority */
>  	mixer_reg_write(res, MXR_LAYER_CFG, 0);
>  
> -	/* setting background color */
> -	mixer_reg_write(res, MXR_BG_COLOR0, 0x008080);
> -	mixer_reg_write(res, MXR_BG_COLOR1, 0x008080);
> -	mixer_reg_write(res, MXR_BG_COLOR2, 0x008080);
> +	/* set all background colors to RGB (0,0,0) */
> +	mixer_reg_write(res, MXR_BG_COLOR0, MXR_YCBCR_VAL(0, 128, 128));
> +	mixer_reg_write(res, MXR_BG_COLOR1, MXR_YCBCR_VAL(0, 128, 128));
> +	mixer_reg_write(res, MXR_BG_COLOR2, MXR_YCBCR_VAL(0, 128, 128));
>  
>  	if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) {
>  		/* configuration of Video Processor Registers */
> diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
> index 7f22df5..c311f57 100644
> --- a/drivers/gpu/drm/exynos/regs-mixer.h
> +++ b/drivers/gpu/drm/exynos/regs-mixer.h
> @@ -140,11 +140,11 @@
>  #define MXR_INT_EN_VSYNC		(1 << 11)
>  #define MXR_INT_EN_ALL			(0x0f << 8)
>  
> -/* bit for MXR_INT_STATUS */
> +/* bits for MXR_INT_STATUS */
>  #define MXR_INT_CLEAR_VSYNC		(1 << 11)
>  #define MXR_INT_STATUS_VSYNC		(1 << 0)
>  
> -/* bit for MXR_LAYER_CFG */
> +/* bits for MXR_LAYER_CFG */
>  #define MXR_LAYER_CFG_GRP1_VAL(x)	MXR_MASK_VAL(x, 11, 8)
>  #define MXR_LAYER_CFG_GRP1_MASK		MXR_LAYER_CFG_GRP1_VAL(~0)
>  #define MXR_LAYER_CFG_GRP0_VAL(x)	MXR_MASK_VAL(x, 7, 4)
> @@ -152,5 +152,8 @@
>  #define MXR_LAYER_CFG_VP_VAL(x)		MXR_MASK_VAL(x, 3, 0)
>  #define MXR_LAYER_CFG_VP_MASK		MXR_LAYER_CFG_VP_VAL(~0)
>  
> +/* bits for MXR_CM_COEFF_Y */
> +#define MXR_CM_COEFF_RGB_FULL		(1 << 30)
> +
>  #endif /* SAMSUNG_REGS_MIXER_H */
>  


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/exynos: mixer: simplify mixer_cfg_rgb_fmt()

[Tobias Jakobi]

Hello Andrzej,

Andrzej Hajda wrote:
> Hi Tobias,
> 
> On 03.03.2017 14:40, Tobias Jakobi wrote:
>> Convert if-statements to switch statement. Removes
>> duplicated code.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  drivers/gpu/drm/exynos/exynos_mixer.c | 30 ++++++++----------------------
>>  1 file changed, 8 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>> index 72143ac..41d0c36 100644
>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>> @@ -382,29 +382,14 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
>>  	struct mixer_resources *res = &ctx->mixer_res;
>>  	u32 val;
>>  
>> -	if (height == 480) {
>> +	switch (height) {
>> +	case 480:
>> +	case 576:
>>  		val = MXR_CFG_RGB601_0_255;
>> -	} else if (height == 576) {
>> -		val = MXR_CFG_RGB601_0_255;
>> -	} else if (height == 720) {
>> -		val = MXR_CFG_RGB709_16_235;
>> -		mixer_reg_write(res, MXR_CM_COEFF_Y,
>> -				(1 << 30) | (94 << 20) | (314 << 10) |
>> -				(32 << 0));
>> -		mixer_reg_write(res, MXR_CM_COEFF_CB,
>> -				(972 << 20) | (851 << 10) | (225 << 0));
>> -		mixer_reg_write(res, MXR_CM_COEFF_CR,
>> -				(225 << 20) | (820 << 10) | (1004 << 0));
>> -	} else if (height == 1080) {
>> -		val = MXR_CFG_RGB709_16_235;
>> -		mixer_reg_write(res, MXR_CM_COEFF_Y,
>> -				(1 << 30) | (94 << 20) | (314 << 10) |
>> -				(32 << 0));
>> -		mixer_reg_write(res, MXR_CM_COEFF_CB,
>> -				(972 << 20) | (851 << 10) | (225 << 0));
>> -		mixer_reg_write(res, MXR_CM_COEFF_CR,
>> -				(225 << 20) | (820 << 10) | (1004 << 0));
>> -	} else {
>> +		break;
>> +	case 720:
>> +	case 1080:
>> +	default:
>>  		val = MXR_CFG_RGB709_16_235;
>>  		mixer_reg_write(res, MXR_CM_COEFF_Y,
>>  				(1 << 30) | (94 << 20) | (314 << 10) |
>> @@ -413,6 +398,7 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
>>  				(972 << 20) | (851 << 10) | (225 << 0));
>>  		mixer_reg_write(res, MXR_CM_COEFF_CR,
>>  				(225 << 20) | (820 << 10) | (1004 << 0));
>> +		break;
>>  	}
> 
> Good change.
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Thanks for the review.

> 
> One small nitpick.
> As I see this conditional/switch is to decide about BT standard
> depending on the height. The similar problem is addressed in exynos-gsc
> patches [1].
> It would be good to have the same criteria to distinguish SD/HD mode. I
> think ((height > 576) || (width > 720)) is more generic, in this case
> even (height > 576) looks better, but as this changes logic of the code
> it could be in separate patch.
I'm not submitting functional changes anymore. You probably still
remember how that ended up last time. It's just not worth my time, sorry.

With best wishes,
Tobias


> [1]: https://lkml.org/lkml/2017/2/21/864
> 
> Regards
> Andrzej
> 
>>  
>>  	mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK);
> 
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/exynos: mixer: document YCbCr magic numbers

[Tobias Jakobi]

Hello Andrzej,


Andrzej Hajda wrote:
> On 03.03.2017 14:40, Tobias Jakobi wrote:
>> The output stage of the mixer uses YCbCr for the internal
>> computations, which is the reason that some registers take
>> YCbCr related data as input. In particular this applies
>> to MXR_BG_COLOR{0,1,2} and MXR_CM_COEFF_{Y,CB,CR}.
>>
>> Document the formatting of the data which we write to
>> these registers.
>>
>> While at it, unify wording of comments in the register header.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  drivers/gpu/drm/exynos/exynos_mixer.c | 35 +++++++++++++++++++++++++++--------
>>  drivers/gpu/drm/exynos/regs-mixer.h   |  7 +++++--
>>  2 files changed, 32 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>> index 41d0c36..3b0b07d 100644
>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>> @@ -45,6 +45,20 @@
>>  #define MIXER_WIN_NR		3
>>  #define VP_DEFAULT_WIN		2
>>  
>> +/*
>> + * Mixer color space conversion coefficient triplet.
>> + * Used for CSC from RGB to YCbCr.
>> + * Each coefficient is a 10-bit fixed point number with
>> + * sign and no integer part, i.e.
> Maybe it would be more clear to say in exclusive range (-1,1)
>> + * [0:8] = fractional part (representing a value y = x / 2^9)
>> + * [9] = sign
>> + * Negative values are encoded with two's complement.
>> + */
>> +#define MXR_CSC_CT(a0, a1, a2) (((a0) << 20) | ((a1) << 10) | ((a2) << 0))
> 
> We can take advantage of the fact that floating point numbers are
> allowed in compile time, aren't they?
> 
> #define MXR_CSC_C(x) ((int)((x) * 512) & 0x3ff)
> #define MXR_CSC_CT(a0, a1, a2) ((MXR_CSC_C(a0) << 20) | (MXR_CSC_C(a1)
> << 10) | (MXR_CSC_C(a2) << 0))
> 
> and stop using magic hexadecimals, now we will use real coefficients.
>     MXR_CSC_CT(0.1835,  0.6132,  0.0625)
I'm not sure if this change of base can be done in a lossless fashion
without typing out too many digits [see below]. I haven't done the math
here. Like I said, no functional changes.



>> +
>> +/* YCbCr value, used for mixer background color configuration. */
>> +#define MXR_YCBCR_VAL(y, cb, cr) (((y) << 16) | ((cb) << 8) | ((cr) << 0))
>> +
>>  /* The pixelformats that are natively supported by the mixer. */
>>  #define MXR_FORMAT_RGB565	4
>>  #define MXR_FORMAT_ARGB1555	5
>> @@ -391,13 +405,18 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
>>  	case 1080:
>>  	default:
>>  		val = MXR_CFG_RGB709_16_235;
>> +		/*
>> +		 * Configure the BT.709 CSC matrix M for full range RGB.
>> +		 *     | 0.1835  0.6132  0.0625|
>> +		 * M = |-0.1015 -0.3378  0.4394|
>> +		 *     | 0.4394 -0.3984 -0.0390|
> 
> Out of curiosity, where did you take those values from, datasheet for
> Exynos5422 has little bit different matrix.
I don't have access to any datasheets. This was done by reverse
engineering. M is just the result of putting the current values in a
calculator. M is not exact though, e.g. 94 / (2^9) = .18359375. Same for
the other values.

With best wishes,
Tobias


> 
> Regards
> Andrzej
> 
>> +		 */
>>  		mixer_reg_write(res, MXR_CM_COEFF_Y,
>> -				(1 << 30) | (94 << 20) | (314 << 10) |
>> -				(32 << 0));
>> +			MXR_CSC_CT(0x05E, 0x13A, 0x020) | MXR_CM_COEFF_RGB_FULL);
> 
>>  		mixer_reg_write(res, MXR_CM_COEFF_CB,
>> -				(972 << 20) | (851 << 10) | (225 << 0));
>> +			MXR_CSC_CT(0x3CC, 0x353, 0x0E1));
>>  		mixer_reg_write(res, MXR_CM_COEFF_CR,
>> -				(225 << 20) | (820 << 10) | (1004 << 0));
>> +			MXR_CSC_CT(0x0E1, 0x334, 0x3EC));
>>  		break;
>>  	}
>>  
>> @@ -715,10 +734,10 @@ static void mixer_win_reset(struct mixer_context *ctx)
>>  	/* reset default layer priority */
>>  	mixer_reg_write(res, MXR_LAYER_CFG, 0);
>>  
>> -	/* setting background color */
>> -	mixer_reg_write(res, MXR_BG_COLOR0, 0x008080);
>> -	mixer_reg_write(res, MXR_BG_COLOR1, 0x008080);
>> -	mixer_reg_write(res, MXR_BG_COLOR2, 0x008080);
>> +	/* set all background colors to RGB (0,0,0) */
>> +	mixer_reg_write(res, MXR_BG_COLOR0, MXR_YCBCR_VAL(0, 128, 128));
>> +	mixer_reg_write(res, MXR_BG_COLOR1, MXR_YCBCR_VAL(0, 128, 128));
>> +	mixer_reg_write(res, MXR_BG_COLOR2, MXR_YCBCR_VAL(0, 128, 128));
>>  
>>  	if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) {
>>  		/* configuration of Video Processor Registers */
>> diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
>> index 7f22df5..c311f57 100644
>> --- a/drivers/gpu/drm/exynos/regs-mixer.h
>> +++ b/drivers/gpu/drm/exynos/regs-mixer.h
>> @@ -140,11 +140,11 @@
>>  #define MXR_INT_EN_VSYNC		(1 << 11)
>>  #define MXR_INT_EN_ALL			(0x0f << 8)
>>  
>> -/* bit for MXR_INT_STATUS */
>> +/* bits for MXR_INT_STATUS */
>>  #define MXR_INT_CLEAR_VSYNC		(1 << 11)
>>  #define MXR_INT_STATUS_VSYNC		(1 << 0)
>>  
>> -/* bit for MXR_LAYER_CFG */
>> +/* bits for MXR_LAYER_CFG */
>>  #define MXR_LAYER_CFG_GRP1_VAL(x)	MXR_MASK_VAL(x, 11, 8)
>>  #define MXR_LAYER_CFG_GRP1_MASK		MXR_LAYER_CFG_GRP1_VAL(~0)
>>  #define MXR_LAYER_CFG_GRP0_VAL(x)	MXR_MASK_VAL(x, 7, 4)
>> @@ -152,5 +152,8 @@
>>  #define MXR_LAYER_CFG_VP_VAL(x)		MXR_MASK_VAL(x, 3, 0)
>>  #define MXR_LAYER_CFG_VP_MASK		MXR_LAYER_CFG_VP_VAL(~0)
>>  
>> +/* bits for MXR_CM_COEFF_Y */
>> +#define MXR_CM_COEFF_RGB_FULL		(1 << 30)
>> +
>>  #endif /* SAMSUNG_REGS_MIXER_H */
>>  
> 
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/exynos: mixer: document YCbCr magic numbers

[Andrzej Hajda]

On 06.03.2017 10:43, Tobias Jakobi wrote:
> Hello Andrzej,
>
>
> Andrzej Hajda wrote:
>> On 03.03.2017 14:40, Tobias Jakobi wrote:
>>> The output stage of the mixer uses YCbCr for the internal
>>> computations, which is the reason that some registers take
>>> YCbCr related data as input. In particular this applies
>>> to MXR_BG_COLOR{0,1,2} and MXR_CM_COEFF_{Y,CB,CR}.
>>>
>>> Document the formatting of the data which we write to
>>> these registers.
>>>
>>> While at it, unify wording of comments in the register header.
>>>
>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_mixer.c | 35 +++++++++++++++++++++++++++--------
>>>  drivers/gpu/drm/exynos/regs-mixer.h   |  7 +++++--
>>>  2 files changed, 32 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> index 41d0c36..3b0b07d 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> @@ -45,6 +45,20 @@
>>>  #define MIXER_WIN_NR		3
>>>  #define VP_DEFAULT_WIN		2
>>>  
>>> +/*
>>> + * Mixer color space conversion coefficient triplet.
>>> + * Used for CSC from RGB to YCbCr.
>>> + * Each coefficient is a 10-bit fixed point number with
>>> + * sign and no integer part, i.e.
>> Maybe it would be more clear to say in exclusive range (-1,1)
>>> + * [0:8] = fractional part (representing a value y = x / 2^9)
>>> + * [9] = sign
>>> + * Negative values are encoded with two's complement.
>>> + */
>>> +#define MXR_CSC_CT(a0, a1, a2) (((a0) << 20) | ((a1) << 10) | ((a2) << 0))
>> We can take advantage of the fact that floating point numbers are
>> allowed in compile time, aren't they?
>>
>> #define MXR_CSC_C(x) ((int)((x) * 512) & 0x3ff)
>> #define MXR_CSC_CT(a0, a1, a2) ((MXR_CSC_C(a0) << 20) | (MXR_CSC_C(a1)
>> << 10) | (MXR_CSC_C(a2) << 0))
>>
>> and stop using magic hexadecimals, now we will use real coefficients.
>>     MXR_CSC_CT(0.1835,  0.6132,  0.0625)
> I'm not sure if this change of base can be done in a lossless fashion
> without typing out too many digits [see below]. I haven't done the math
> here. 

Difference by one in representation corresponds to 0.0019, so four
digits after dot is enough.

> Like I said, no functional changes.

Decision is yours, I can only advice to do not give up.

Regards
Andrzej

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel