diff mbox series

[v5,5/6] drm/i915/icl: Enable ICL Pipe CSC block

Message ID 1546933053-10731-6-git-send-email-uma.shankar@intel.com (mailing list archive)
State New, archived
Headers show
Series Add support for Gen 11 pipe color features | expand

Commit Message

Shankar, Uma Jan. 8, 2019, 7:37 a.m. UTC
Enable ICL pipe csc hardware. CSC block is enabled
in CSC_MODE register instead of PLANE_COLOR_CTL.

ToDO: Extend the ABI to accept 32 bit coefficient values
instead of 16bit for future platforms.

v2: Addressed Maarten's review comments.

v3: Addressed Matt's review comments. Removed rmw patterns
as suggested by Matt.

v4: Addressed Matt's review comments.

v5: Addressed Ville's review comments.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h    | 10 +++++++---
 drivers/gpu/drm/i915/intel_color.c | 12 ++++++++++--
 2 files changed, 17 insertions(+), 5 deletions(-)

Comments

Matt Roper Jan. 11, 2019, 10:59 p.m. UTC | #1
On Tue, Jan 08, 2019 at 01:07:32PM +0530, Uma Shankar wrote:
> Enable ICL pipe csc hardware. CSC block is enabled
> in CSC_MODE register instead of PLANE_COLOR_CTL.
> 
> ToDO: Extend the ABI to accept 32 bit coefficient values
> instead of 16bit for future platforms.
> 
> v2: Addressed Maarten's review comments.
> 
> v3: Addressed Matt's review comments. Removed rmw patterns
> as suggested by Matt.
> 
> v4: Addressed Matt's review comments.
> 
> v5: Addressed Ville's review comments.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    | 10 +++++++---
>  drivers/gpu/drm/i915/intel_color.c | 12 ++++++++++--
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f29eef7..5a262c0 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9861,10 +9861,14 @@ enum skl_power_gate {
>  #define _PIPE_A_CSC_COEFF_BU	0x4901c
>  #define _PIPE_A_CSC_COEFF_RV_GV	0x49020
>  #define _PIPE_A_CSC_COEFF_BV	0x49024
> +
>  #define _PIPE_A_CSC_MODE	0x49028
> -#define   CSC_BLACK_SCREEN_OFFSET	(1 << 2)
> -#define   CSC_POSITION_BEFORE_GAMMA	(1 << 1)
> -#define   CSC_MODE_YUV_TO_RGB		(1 << 0)
> +#define  ICL_CSC_ENABLE			(1 << 31)
> +#define  ICL_OUTPUT_CSC_ENABLE		(1 << 30)
> +#define  CSC_BLACK_SCREEN_OFFSET	(1 << 2)
> +#define  CSC_POSITION_BEFORE_GAMMA	(1 << 1)
> +#define  CSC_MODE_YUV_TO_RGB		(1 << 0)
> +
>  #define _PIPE_A_CSC_PREOFF_HI	0x49030
>  #define _PIPE_A_CSC_PREOFF_ME	0x49034
>  #define _PIPE_A_CSC_PREOFF_LO	0x49038
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 9cd4646..c3e4ff6 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -134,7 +134,11 @@ static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *crtc)
>  	I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI);
>  	I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), POSTOFF_RGB_TO_YUV_ME);
>  	I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO);
> -	I915_WRITE(PIPE_CSC_MODE(pipe), 0);
> +
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		I915_WRITE(PIPE_CSC_MODE(pipe), ICL_OUTPUT_CSC_ENABLE);

For gen11+, shouldn't we be programming OUTPUT_CSC_COEFF instead of
CSC_COEFF if we set this bit?

> +	else
> +		I915_WRITE(PIPE_CSC_MODE(pipe), 0);
>  }
>  
>  static void ilk_load_csc_matrix(struct intel_crtc_state *crtc_state)
> @@ -242,7 +246,10 @@ static void ilk_load_csc_matrix(struct intel_crtc_state *crtc_state)
>  		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff);
>  		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff);
>  
> -		I915_WRITE(PIPE_CSC_MODE(pipe), 0);
> +		if (INTEL_GEN(dev_priv) >= 11)
> +			I915_WRITE(PIPE_CSC_MODE(pipe), ICL_CSC_ENABLE);
> +		else
> +			I915_WRITE(PIPE_CSC_MODE(pipe), 0);

I might be misinterpreting how the hardware works, but my impression was
that we had two distinct CSC units now on gen11+.  So we could use the
traditional CSC registers to hold the userspace-provided CTM and the new
output CSC registers to hold the fixed RGB->YUV matrix, and they could
both be used at the same time if necessary.  It looks like this function
is still assuming that the two are mutually exclusive and that if we
need RGB->YUV we never bother programming the userspace matrix.  Is that
intentional or an oversight?  Aside from the register definitions
themselves, the bspec seems to be pretty sparse on how the whole
pipeline goes together...


Matt

>  	} else {
>  		uint32_t mode = CSC_MODE_YUV_TO_RGB;
>  
> @@ -700,6 +707,7 @@ void intel_color_init(struct intel_crtc *crtc)
>  		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
>  		dev_priv->display.load_luts = glk_load_luts;
>  	} else if (IS_ICELAKE(dev_priv)) {
> +		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
>  		dev_priv->display.load_luts = icl_load_luts;
>  	} else {
>  		dev_priv->display.load_luts = i9xx_load_luts;
> -- 
> 1.9.1
>
Shankar, Uma Jan. 16, 2019, 3:55 p.m. UTC | #2
>-----Original Message-----
>From: Roper, Matthew D
>Sent: Saturday, January 12, 2019 4:30 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
>Shashank <shashank.sharma@intel.com>
>Subject: Re: [v5 5/6] drm/i915/icl: Enable ICL Pipe CSC block
>
>On Tue, Jan 08, 2019 at 01:07:32PM +0530, Uma Shankar wrote:
>> Enable ICL pipe csc hardware. CSC block is enabled in CSC_MODE
>> register instead of PLANE_COLOR_CTL.
>>
>> ToDO: Extend the ABI to accept 32 bit coefficient values instead of
>> 16bit for future platforms.
>>
>> v2: Addressed Maarten's review comments.
>>
>> v3: Addressed Matt's review comments. Removed rmw patterns as
>> suggested by Matt.
>>
>> v4: Addressed Matt's review comments.
>>
>> v5: Addressed Ville's review comments.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h    | 10 +++++++---
>>  drivers/gpu/drm/i915/intel_color.c | 12 ++++++++++--
>>  2 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h index f29eef7..5a262c0 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -9861,10 +9861,14 @@ enum skl_power_gate {
>>  #define _PIPE_A_CSC_COEFF_BU	0x4901c
>>  #define _PIPE_A_CSC_COEFF_RV_GV	0x49020
>>  #define _PIPE_A_CSC_COEFF_BV	0x49024
>> +
>>  #define _PIPE_A_CSC_MODE	0x49028
>> -#define   CSC_BLACK_SCREEN_OFFSET	(1 << 2)
>> -#define   CSC_POSITION_BEFORE_GAMMA	(1 << 1)
>> -#define   CSC_MODE_YUV_TO_RGB		(1 << 0)
>> +#define  ICL_CSC_ENABLE			(1 << 31)
>> +#define  ICL_OUTPUT_CSC_ENABLE		(1 << 30)
>> +#define  CSC_BLACK_SCREEN_OFFSET	(1 << 2)
>> +#define  CSC_POSITION_BEFORE_GAMMA	(1 << 1)
>> +#define  CSC_MODE_YUV_TO_RGB		(1 << 0)
>> +
>>  #define _PIPE_A_CSC_PREOFF_HI	0x49030
>>  #define _PIPE_A_CSC_PREOFF_ME	0x49034
>>  #define _PIPE_A_CSC_PREOFF_LO	0x49038
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 9cd4646..c3e4ff6 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -134,7 +134,11 @@ static void ilk_load_ycbcr_conversion_matrix(struct
>intel_crtc *crtc)
>>  	I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI);
>>  	I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe),
>POSTOFF_RGB_TO_YUV_ME);
>>  	I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO);
>> -	I915_WRITE(PIPE_CSC_MODE(pipe), 0);
>> +
>> +	if (INTEL_GEN(dev_priv) >= 11)
>> +		I915_WRITE(PIPE_CSC_MODE(pipe),
>ICL_OUTPUT_CSC_ENABLE);
>
>For gen11+, shouldn't we be programming OUTPUT_CSC_COEFF instead of
>CSC_COEFF if we set this bit?

Yeah you are right, ideally output CSC coeff should be programmed to utilize this.
Will add that support.

>> +	else
>> +		I915_WRITE(PIPE_CSC_MODE(pipe), 0);
>>  }
>>
>>  static void ilk_load_csc_matrix(struct intel_crtc_state *crtc_state)
>> @@ -242,7 +246,10 @@ static void ilk_load_csc_matrix(struct intel_crtc_state
>*crtc_state)
>>  		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff);
>>  		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff);
>>
>> -		I915_WRITE(PIPE_CSC_MODE(pipe), 0);
>> +		if (INTEL_GEN(dev_priv) >= 11)
>> +			I915_WRITE(PIPE_CSC_MODE(pipe), ICL_CSC_ENABLE);
>> +		else
>> +			I915_WRITE(PIPE_CSC_MODE(pipe), 0);
>
>I might be misinterpreting how the hardware works, but my impression was that
>we had two distinct CSC units now on gen11+.  So we could use the traditional
>CSC registers to hold the userspace-provided CTM and the new output CSC
>registers to hold the fixed RGB->YUV matrix, and they could both be used at the
>same time if necessary.  It looks like this function is still assuming that the two are
>mutually exclusive and that if we need RGB->YUV we never bother programming
>the userspace matrix.  Is that intentional or an oversight?  Aside from the register
>definitions themselves, the bspec seems to be pretty sparse on how the whole
>pipeline goes together...

Ideally both CSC blocks can co-exist, current design is indeed limiting this. Will modify
to handle this properly.

Regards,
Uma Shankar
>
>Matt
>
>>  	} else {
>>  		uint32_t mode = CSC_MODE_YUV_TO_RGB;
>>
>> @@ -700,6 +707,7 @@ void intel_color_init(struct intel_crtc *crtc)
>>  		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
>>  		dev_priv->display.load_luts = glk_load_luts;
>>  	} else if (IS_ICELAKE(dev_priv)) {
>> +		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
>>  		dev_priv->display.load_luts = icl_load_luts;
>>  	} else {
>>  		dev_priv->display.load_luts = i9xx_load_luts;
>> --
>> 1.9.1
>>
>
>--
>Matt Roper
>Graphics Software Engineer
>IoTG Platform Enabling & Development
>Intel Corporation
>(916) 356-2795
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f29eef7..5a262c0 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9861,10 +9861,14 @@  enum skl_power_gate {
 #define _PIPE_A_CSC_COEFF_BU	0x4901c
 #define _PIPE_A_CSC_COEFF_RV_GV	0x49020
 #define _PIPE_A_CSC_COEFF_BV	0x49024
+
 #define _PIPE_A_CSC_MODE	0x49028
-#define   CSC_BLACK_SCREEN_OFFSET	(1 << 2)
-#define   CSC_POSITION_BEFORE_GAMMA	(1 << 1)
-#define   CSC_MODE_YUV_TO_RGB		(1 << 0)
+#define  ICL_CSC_ENABLE			(1 << 31)
+#define  ICL_OUTPUT_CSC_ENABLE		(1 << 30)
+#define  CSC_BLACK_SCREEN_OFFSET	(1 << 2)
+#define  CSC_POSITION_BEFORE_GAMMA	(1 << 1)
+#define  CSC_MODE_YUV_TO_RGB		(1 << 0)
+
 #define _PIPE_A_CSC_PREOFF_HI	0x49030
 #define _PIPE_A_CSC_PREOFF_ME	0x49034
 #define _PIPE_A_CSC_PREOFF_LO	0x49038
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 9cd4646..c3e4ff6 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -134,7 +134,11 @@  static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *crtc)
 	I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI);
 	I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), POSTOFF_RGB_TO_YUV_ME);
 	I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO);
-	I915_WRITE(PIPE_CSC_MODE(pipe), 0);
+
+	if (INTEL_GEN(dev_priv) >= 11)
+		I915_WRITE(PIPE_CSC_MODE(pipe), ICL_OUTPUT_CSC_ENABLE);
+	else
+		I915_WRITE(PIPE_CSC_MODE(pipe), 0);
 }
 
 static void ilk_load_csc_matrix(struct intel_crtc_state *crtc_state)
@@ -242,7 +246,10 @@  static void ilk_load_csc_matrix(struct intel_crtc_state *crtc_state)
 		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff);
 		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff);
 
-		I915_WRITE(PIPE_CSC_MODE(pipe), 0);
+		if (INTEL_GEN(dev_priv) >= 11)
+			I915_WRITE(PIPE_CSC_MODE(pipe), ICL_CSC_ENABLE);
+		else
+			I915_WRITE(PIPE_CSC_MODE(pipe), 0);
 	} else {
 		uint32_t mode = CSC_MODE_YUV_TO_RGB;
 
@@ -700,6 +707,7 @@  void intel_color_init(struct intel_crtc *crtc)
 		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
 		dev_priv->display.load_luts = glk_load_luts;
 	} else if (IS_ICELAKE(dev_priv)) {
+		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
 		dev_priv->display.load_luts = icl_load_luts;
 	} else {
 		dev_priv->display.load_luts = i9xx_load_luts;