Re: [PATCH v2 12/14] drm/i915: prepare csc unit for YCBCR420 output

["Sharma, Shashank" <shashank.sharma@intel.com>]

Regards

Shashank


On 7/15/2017 12:06 AM, Ville Syrjälä wrote:
> On Thu, Jul 13, 2017 at 09:03:18PM +0530, Shashank Sharma wrote:
>> To support ycbcr output, we need a pipe CSC block to do
>> RGB->YCBCR conversion.
>>
>> Current Intel platforms have only one pipe CSC unit, so
>> we can either do color correction using it, or we can perform
>> RGB->YCBCR conversion.
>>
>> This function adds a csc handler, which uses recommended bspec
>> values to perform RGB->YCBCR conversion (target color space BT709)
>>
>> V2: Rebase
>> V3: Rebase
>> V4: Rebase
>> V5: Addressed review comments from Ander
>>      - Remove extra line added in the patch
>>      - Add the spec details in the commit message
>>      - Combine two if(cond) while calling intel_crtc_compute_config
>> V6: Handle YCBCR420 outputs only (Ville)
>> V7: Addressed review comments from Ville:
>>      - Add description about target colorspace
>>      - Remove the comments from CSC function
>>      - DRM_DEBUG->DEBUG_KMS for atomic failure due to CSC unit busy
>>      - Remove unnecessary debug message about YCBCR420 possibe
>>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_color.c   | 46 +++++++++++++++++++++++++++++++++++-
>>   drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++
>>   2 files changed, 60 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>> index f85d575..8698691 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -41,6 +41,22 @@
>>   
>>   #define LEGACY_LUT_LENGTH		(sizeof(struct drm_color_lut) * 256)
>>   
>> +/* Post offset values for RGB->YCBCR conversion */
>> +#define POSTOFF_RGB_TO_YUV_HI 0x800
>> +#define POSTOFF_RGB_TO_YUV_ME 0x100
>> +#define POSTOFF_RGB_TO_YUV_LO 0x800
>> +
>> +/*
>> + * These values are direct register values specified in the Bspec,
>> + * for RGB->YUV conversion matrix (colorspace BT709)
>> + */
>> +#define CSC_RGB_TO_YUV_RU_GU 0x2ba809d8
>> +#define CSC_RGB_TO_YUV_BU 0x37e80000
>> +#define CSC_RGB_TO_YUV_RY_GY 0x1e089cc0
>> +#define CSC_RGB_TO_YUV_BY 0xb5280000
>> +#define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8
>> +#define CSC_RGB_TO_YUV_BV 0x1e080000
>> +
>>   /*
>>    * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
>>    * format). This macro takes the coefficient we want transformed and the
>> @@ -91,6 +107,31 @@ static void ctm_mult_by_limited(uint64_t *result, int64_t *input)
>>   	}
>>   }
>>   
>> +void i9xx_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc)
>> +{
>> +	int pipe = intel_crtc->pipe;
>> +	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
>> +
>> +
>> +	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_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_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);
>> +}
>> +
>>   /* Set up the pipe CSC unit. */
>>   static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
>>   {
>> @@ -101,7 +142,10 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
>>   	uint16_t coeffs[9] = { 0, };
>>   	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
>>   
>> -	if (crtc_state->ctm) {
>> +	if (intel_crtc_state->ycbcr420) {
>> +		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
>> +		return;
>> +	} else if (crtc_state->ctm) {
>>   		struct drm_color_ctm *ctm =
>>   			(struct drm_color_ctm *)crtc_state->ctm->data;
>>   		uint64_t input[9] = { 0, };
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 1a23ec0..9c6a1ed 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6283,6 +6283,21 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>>   		return -EINVAL;
>>   	}
>>   
>> +	/* YCBCR420 feasibility check */
> Not a useful comment.
Got it.
>> +	if (pipe_config->ycbcr420) {
>> +		struct drm_crtc_state *drm_state = &pipe_config->base;
> Having aliasing pointer for the same thing is just annoying.
> We do have quite a lot of that going around, but IMO we need
> to clean that all up at some point.
>
> So pls just do this instead:
>
> if (pipe_config->ycbcr420 && pipe_config->base.ctm) {
> 	...
> }
Most of the time its done to cover for 80 char limit, and to make code 
look good.
but let me see if I can accommodate this one.

- Shashank
>
>> +
>> +		/*
>> +		 * There is only one pipe CSC unit per pipe, and we need that
>> +		 * for output conversion from RGB->YCBCR. So if CTM is already
>> +		 * applied we can't support YCBCR420 output.
>> +		 */
>> +		if (drm_state->ctm) {
>> +			DRM_DEBUG_KMS("YCBCR420 and CTM is not possible\n");
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>>   	/*
>>   	 * Pipe horizontal size must be even in:
>>   	 * - DVO ganged mode
>> -- 
>> 2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx