[Intel-gfx] [v6 5/6] drm/i915/icl: Enable pipe output csc

Shankar, Uma uma.shankar at intel.com
Tue Jan 29 15:52:05 UTC 2019



>-----Original Message-----
>From: Roper, Matthew D
>Sent: Tuesday, January 29, 2019 3:50 AM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: intel-gfx at lists.freedesktop.org; Lankhorst, Maarten
><maarten.lankhorst at intel.com>; Syrjala, Ville <ville.syrjala at intel.com>; Sharma,
>Shashank <shashank.sharma at intel.com>
>Subject: Re: [v6 5/6] drm/i915/icl: Enable pipe output csc
>
>On Wed, Jan 16, 2019 at 09:51:36PM +0530, Uma Shankar wrote:
>> GEN11+ onwards an output csc hardware block has been added.
>> This is after the pipe gamma block and is in addition to the legacy
>> pipe CSC block. Primary use case for this block is to convert RGB to
>> YUV in case sink supports YUV.
>> This patch adds supports for the same.
>>
>> v2: This is added after splitting the existing ICL pipe CSC handling.
>> As per Matt's suggestion, made this to co-exist with existing pipe
>> CSC, wherein both can be enabled if a certain usecase arises.
>>
>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h    | 41 +++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_color.c | 75 ++++++++++++++++++++++++++++-----
>-----
>>  drivers/gpu/drm/i915/intel_drv.h   |  3 ++
>>  3 files changed, 99 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h index 3c3a902..edd6b4d 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -9864,6 +9864,7 @@ enum skl_power_gate {
>>
>>  #define _PIPE_A_CSC_MODE	0x49028
>>  #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)
>> @@ -9903,6 +9904,46 @@ enum skl_power_gate {
>>  #define PIPE_CSC_POSTOFF_ME(pipe)	_MMIO_PIPE(pipe,
>_PIPE_A_CSC_POSTOFF_ME, _PIPE_B_CSC_POSTOFF_ME)
>>  #define PIPE_CSC_POSTOFF_LO(pipe)	_MMIO_PIPE(pipe,
>_PIPE_A_CSC_POSTOFF_LO, _PIPE_B_CSC_POSTOFF_LO)
>>
>> +/* Pipe Output CSC */
>> +#define _PIPE_A_OUTPUT_CSC_COEFF_RY_GY	0x49050
>> +#define _PIPE_A_OUTPUT_CSC_COEFF_BY	0x49054
>> +#define _PIPE_A_OUTPUT_CSC_COEFF_RU_GU	0x49058
>> +#define _PIPE_A_OUTPUT_CSC_COEFF_BU	0x4905c
>> +#define _PIPE_A_OUTPUT_CSC_COEFF_RV_GV	0x49060
>> +#define _PIPE_A_OUTPUT_CSC_COEFF_BV	0x49064
>> +#define _PIPE_A_OUTPUT_CSC_PREOFF_HI	0x49068
>> +#define _PIPE_A_OUTPUT_CSC_PREOFF_ME	0x4906c
>> +#define _PIPE_A_OUTPUT_CSC_PREOFF_LO	0x49070
>> +#define _PIPE_A_OUTPUT_CSC_POSTOFF_HI	0x49074
>> +#define _PIPE_A_OUTPUT_CSC_POSTOFF_ME	0x49078
>> +#define _PIPE_A_OUTPUT_CSC_POSTOFF_LO	0x4907c
>> +
>> +#define _PIPE_B_OUTPUT_CSC_COEFF_RY_GY	0x49150
>> +#define _PIPE_B_OUTPUT_CSC_COEFF_BY	0x49154
>> +#define _PIPE_B_OUTPUT_CSC_COEFF_RU_GU	0x49158
>> +#define _PIPE_B_OUTPUT_CSC_COEFF_BU	0x4915c
>> +#define _PIPE_B_OUTPUT_CSC_COEFF_RV_GV	0x49160
>> +#define _PIPE_B_OUTPUT_CSC_COEFF_BV	0x49164
>> +#define _PIPE_B_OUTPUT_CSC_PREOFF_HI	0x49168
>> +#define _PIPE_B_OUTPUT_CSC_PREOFF_ME	0x4916c
>> +#define _PIPE_B_OUTPUT_CSC_PREOFF_LO	0x49170
>> +#define _PIPE_B_OUTPUT_CSC_POSTOFF_HI	0x49174
>> +#define _PIPE_B_OUTPUT_CSC_POSTOFF_ME	0x49178
>> +#define _PIPE_B_OUTPUT_CSC_POSTOFF_LO	0x4917c
>> +
>> +#define PIPE_CSC_OUTPUT_COEFF_RY_GY(pipe)	_MMIO_PIPE(pipe,
>_PIPE_A_OUTPUT_CSC_COEFF_RY_GY, _PIPE_B_OUTPUT_CSC_COEFF_RY_GY)
>> +#define PIPE_CSC_OUTPUT_COEFF_BY(pipe)		_MMIO_PIPE(pipe,
>_PIPE_A_OUTPUT_CSC_COEFF_BY, _PIPE_B_OUTPUT_CSC_COEFF_BY)
>> +#define PIPE_CSC_OUTPUT_COEFF_RU_GU(pipe)	_MMIO_PIPE(pipe,
>_PIPE_A_OUTPUT_CSC_COEFF_RU_GU, _PIPE_B_OUTPUT_CSC_COEFF_RU_GU)
>> +#define PIPE_CSC_OUTPUT_COEFF_BU(pipe)		_MMIO_PIPE(pipe,
>_PIPE_A_OUTPUT_CSC_COEFF_BU, _PIPE_B_OUTPUT_CSC_COEFF_BU)
>> +#define PIPE_CSC_OUTPUT_COEFF_RV_GV(pipe)	_MMIO_PIPE(pipe,
>_PIPE_A_OUTPUT_CSC_COEFF_RV_GV, _PIPE_B_OUTPUT_CSC_COEFF_RV_GV)
>> +#define PIPE_CSC_OUTPUT_COEFF_BV(pipe)		_MMIO_PIPE(pipe,
>_PIPE_A_OUTPUT_CSC_COEFF_BV, _PIPE_B_OUTPUT_CSC_COEFF_BV)
>> +#define PIPE_CSC_OUTPUT_PREOFF_HI(pipe)		_MMIO_PIPE(pipe,
>_PIPE_A_OUTPUT_CSC_PREOFF_HI, _PIPE_B_OUTPUT_CSC_PREOFF_HI)
>> +#define PIPE_CSC_OUTPUT_PREOFF_ME(pipe)
>	_MMIO_PIPE(pipe, _PIPE_A_OUTPUT_CSC_PREOFF_ME,
>_PIPE_B_OUTPUT_CSC_PREOFF_ME)
>> +#define PIPE_CSC_OUTPUT_PREOFF_LO(pipe)		_MMIO_PIPE(pipe,
>_PIPE_A_OUTPUT_CSC_PREOFF_LO, _PIPE_B_OUTPUT_CSC_PREOFF_LO)
>> +#define PIPE_CSC_OUTPUT_POSTOFF_HI(pipe)	_MMIO_PIPE(pipe,
>_PIPE_A_OUTPUT_CSC_POSTOFF_HI, _PIPE_B_OUTPUT_CSC_POSTOFF_HI)
>> +#define PIPE_CSC_OUTPUT_POSTOFF_ME(pipe)	_MMIO_PIPE(pipe,
>_PIPE_A_OUTPUT_CSC_POSTOFF_ME, _PIPE_B_OUTPUT_CSC_POSTOFF_ME)
>> +#define PIPE_CSC_OUTPUT_POSTOFF_LO(pipe)	_MMIO_PIPE(pipe,
>_PIPE_A_OUTPUT_CSC_POSTOFF_LO, _PIPE_B_OUTPUT_CSC_POSTOFF_LO)
>> +
>>  /* pipe degamma/gamma LUTs on IVB+ */
>>  #define _PAL_PREC_INDEX_A	0x4A400
>>  #define _PAL_PREC_INDEX_B	0x4AC00
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 9b8d2de..c95adb9 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -113,29 +113,58 @@ static u64 *ctm_mult_by_limited(u64 *result, const
>u64 *input)
>>  	return result;
>>  }
>>
>> -static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *crtc)
>> +static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc_state
>> +					     *crtc_state)
>>  {
>> -	int pipe = crtc->pipe;
>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +	int pipe = crtc->pipe;
>>
>> -	I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
>> -	I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
>> -	I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
>> +	if (INTEL_GEN(dev_priv) < 11) {
>> +		I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
>> +		I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
>> +		I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
>>
>> -	I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe),
>CSC_RGB_TO_YUV_RU_GU);
>> -	I915_WRITE(PIPE_CSC_COEFF_BU(pipe), CSC_RGB_TO_YUV_BU);
>> +		I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe),
>CSC_RGB_TO_YUV_RU_GU);
>> +		I915_WRITE(PIPE_CSC_COEFF_BU(pipe),
>CSC_RGB_TO_YUV_BU);
>>
>> -	I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), CSC_RGB_TO_YUV_RY_GY);
>> -	I915_WRITE(PIPE_CSC_COEFF_BY(pipe), CSC_RGB_TO_YUV_BY);
>> +		I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe),
>CSC_RGB_TO_YUV_RY_GY);
>> +		I915_WRITE(PIPE_CSC_COEFF_BY(pipe), CSC_RGB_TO_YUV_BY);
>>
>> -	I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), CSC_RGB_TO_YUV_RV_GV);
>> -	I915_WRITE(PIPE_CSC_COEFF_BV(pipe), CSC_RGB_TO_YUV_BV);
>> +		I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe),
>CSC_RGB_TO_YUV_RV_GV);
>> +		I915_WRITE(PIPE_CSC_COEFF_BV(pipe), CSC_RGB_TO_YUV_BV);
>>
>> -	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_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);
>> +		crtc_state->csc_mode = 0;
>
>This should be set during the atomic check phase rather than during the commit.
>But I suspect that will happen anyway once Ville's series lands and you rebase.

Yes, will rebase this on top of Ville's series and take care.

>> +	} else {
>> +		I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_HI(pipe), 0);
>> +		I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_ME(pipe), 0);
>> +		I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_LO(pipe), 0);
>> +
>> +		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RU_GU(pipe),
>> +			   CSC_RGB_TO_YUV_RU_GU);
>> +		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BU(pipe),
>CSC_RGB_TO_YUV_BU);
>> +
>> +		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RY_GY(pipe),
>> +			   CSC_RGB_TO_YUV_RY_GY);
>> +		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BY(pipe),
>CSC_RGB_TO_YUV_BY);
>> +
>> +		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RV_GV(pipe),
>> +			   CSC_RGB_TO_YUV_RV_GV);
>> +		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BV(pipe),
>CSC_RGB_TO_YUV_BV);
>> +
>> +		I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_HI(pipe),
>> +			   POSTOFF_RGB_TO_YUV_HI);
>> +		I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_ME(pipe),
>> +			   POSTOFF_RGB_TO_YUV_ME);
>> +		I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_LO(pipe),
>> +			   POSTOFF_RGB_TO_YUV_LO);
>> +
>> +		crtc_state->csc_mode = ICL_OUTPUT_CSC_ENABLE;
>> +	}
>>  }
>>
>>  static void ilk_load_csc_matrix(struct intel_crtc_state *crtc_state)
>> @@ -153,10 +182,14 @@ static void ilk_load_csc_matrix(struct
>intel_crtc_state *crtc_state)
>>  	if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
>>  		limited_color_range = crtc_state->limited_color_range;
>>
>> +	crtc_state->csc_mode = 0;
>>  	if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
>>  	    crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) {
>> -		ilk_load_ycbcr_conversion_matrix(crtc);
>> -		return;
>> +		ilk_load_ycbcr_conversion_matrix(crtc_state);
>> +		if (INTEL_GEN(dev_priv) < 11) {
>> +			I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state-
>>csc_mode);
>> +			return;
>> +		}
>
>Isn't this going to lead us to still exit immediately after programming the output
>CSC?  I.e., we'll never program the userspace matrix into the first CSC because
>that only happens in the 'else if (crtc_state->base.ctm)'
>branch below.  We probably need to remove the RGB->YUV logic from the if/else
>check that figures out what to program in the first CSC.

Yes you are right. I will de-couple this making sure both output csc for RGB-YUV and
user controlled normal pipe csc co-work together.

Regards,
Uma Shankar

>
>Matt
>
>>  	} else if (crtc_state->base.ctm) {
>>  		struct drm_color_ctm *ctm = crtc_state->base.ctm->data;
>>  		const u64 *input;
>> @@ -243,10 +276,12 @@ 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);
>>
>> -		if (INTEL_GEN(dev_priv) >= 11)
>> -			I915_WRITE(PIPE_CSC_MODE(pipe), ICL_CSC_ENABLE);
>> -		else
>> +		if (INTEL_GEN(dev_priv) >= 11) {
>> +			crtc_state->csc_mode |= ICL_CSC_ENABLE;
>> +			I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state-
>>csc_mode);
>> +		} else {
>>  			I915_WRITE(PIPE_CSC_MODE(pipe), 0);
>> +		}
>>  	} else {
>>  		uint32_t mode = CSC_MODE_YUV_TO_RGB;
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index e5a436c..320a413 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -930,6 +930,9 @@ struct intel_crtc_state {
>>  	/* Gamma mode programmed on the pipe */
>>  	uint32_t gamma_mode;
>>
>> +	/* CSC mode programmed on the pipe */
>> +	uint32_t csc_mode;
>> +
>>  	/* bitmask of visible planes (enum plane_id) */
>>  	u8 active_planes;
>>  	u8 nv12_planes;
>> --
>> 1.9.1
>>
>
>--
>Matt Roper
>Graphics Software Engineer
>IoTG Platform Enabling & Development
>Intel Corporation
>(916) 356-2795


More information about the Intel-gfx mailing list