[Intel-gfx] [PATCH 3/3] drm/i915/icl: Enable ICL Pipe CSC block

[Shankar, Uma]

>-----Original Message-----
>From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
>Sent: Wednesday, October 24, 2018 7:08 PM
>To: Shankar, Uma <uma.shankar at intel.com>; intel-gfx at lists.freedesktop.org
>Cc: Syrjala, Ville <ville.syrjala at intel.com>; Lankhorst, Maarten
><maarten.lankhorst at intel.com>
>Subject: Re: [Intel-gfx] [PATCH 3/3] drm/i915/icl: Enable ICL Pipe CSC block
>
>Op 23-10-18 om 22:40 schreef Uma Shankar:
>> Enable ICL pipe csc hardware. CSC block is enabled in CSC_MODE
>> register instead of PLANE_COLOR_CTL.
>>
>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h    | 1 +
>>  drivers/gpu/drm/i915/intel_color.c | 7 ++++++-
>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h index dd0514e..0178761 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -9700,6 +9700,7 @@ enum skl_power_gate {
>>  #define _PIPE_A_CSC_COEFF_RV_GV	0x49020
>>  #define _PIPE_A_CSC_COEFF_BV	0x49024
>>  #define _PIPE_A_CSC_MODE	0x49028
>> +#define   CSC_ENABLE			(1 << 31)
>>  #define   CSC_BLACK_SCREEN_OFFSET	(1 << 2)
>>  #define   CSC_POSITION_BEFORE_GAMMA	(1 << 1)
>>  #define   CSC_MODE_YUV_TO_RGB		(1 << 0)
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 7860244..2ebfe3a 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -239,7 +239,11 @@ static void ilk_load_csc_matrix(struct drm_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),
>> +				I915_READ(PIPE_CSC_MODE(pipe)) |
>CSC_ENABLE);
>Just write CSC_ENABLE here, I think.

Yes, this could be done as of now since PIPE OUTPUT CSC is not enabled.
Was keeping it as a failsafe for future once that feature gets enabled, this
may start overwriting that and will cause some bug and debug to hunt where
the value got overwritten :)

>> +		else
>> +			I915_WRITE(PIPE_CSC_MODE(pipe), 0);
>>  	} else {
>>  		uint32_t mode = CSC_MODE_YUV_TO_RGB;
>Should there be a case in  ilk_load_ycbcr_conversion_matrix as well?

>Though it seems wrong that function ignore crtc_state->ctm in that case.

This is a good catch and it should be added there as well. Since this was a case
of hardcoded co-efficients, I believe it doesn't bother about userspace LUT blob,
so that should be ok.

Thanks Maarten for the review and very useful comments. I will update the patches
and refloat.

Regards,
Uma Shankar

>> @@ -736,6 +740,7 @@ void intel_color_init(struct drm_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;
>
>