>-----Original Message-----
>From: Roper, Matthew D
>Sent: Tuesday, January 29, 2019 3:50 AM
>To: Shankar, Uma <uma.shankar@xxxxxxxxx>
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Lankhorst, Maarten
><maarten.lankhorst@xxxxxxxxx>; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Sharma,
>Shashank <shashank.sharma@xxxxxxxxx>
>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@xxxxxxxxx>
>> ---
>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
