Re: [PATCH 13/20] drm/i915: prepare csc unit for YCBCR420 output

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

Regards

Shashank


On 7/13/2017 6:20 PM, Ville Syrjälä wrote:
> On Thu, Jul 13, 2017 at 10:44:51AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 7/12/2017 10:47 PM, Ville Syrjälä wrote:
>>> On Mon, Jul 10, 2017 at 04:48:41PM +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)
>>>>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_color.c   | 47 +++++++++++++++++++++++++++++++++++-
>>>>    drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++
>>>>    2 files changed, 63 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>>>> index 306c6b0..8a5d211 100644
>>>> --- a/drivers/gpu/drm/i915/intel_color.c
>>>> +++ b/drivers/gpu/drm/i915/intel_color.c
>>>> @@ -41,6 +41,19 @@
>>>>    
>>>>    #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
>>>> +
>>>> +/* Direct spec values for RGB->YUV conversion matrix */
>>> Are these BT.601 or BT.709 or something else?
>> I thought I added some comment mentioning this being BT 709, but looks
>> like dealing with too many paptches :)
>> Will add those details.
>>>> +#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
>>> IIRC Ander didn't like these, and neither do I. I'd much prefer to reuse
>>> the code we alreayd have for dealing with the CSC. Except I think that's
>>> pretty broken in places so I guess we can go with this for now and try
>>> to clean up the color management stuff later.
>> Yes, that was the plan. I was planning to fix the complete CSC cleanup
>> stuff in a separate patch, but as that needs much
>> of validation before we can merge it, I added in a bottom half. I will
>> send a patch for the same soon.
>>>> +
>>>>    /*
>>>>     * 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 +104,35 @@ 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);
>>>> +
>>>> +	/* We don't use high values for conversion */
>>> I don't understand what this comment is trying so say.
>> Yeah, it should have been "we dont use pre-offsets for conversion"
> I think that's very obvious from the code. So no need for comment IMO.
Ok.
>>>> +	I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
>>>> +	I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
>>>> +	I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
>>>> +
>>>> +	/* Program direct spec values for RGB to YCBCR conversion matrix */
> In fact pretty much all the comments in this patch seem redundant.
> Comments should explain why we do something, if necessary. They
> should not just transcribe the actual code into words.
Ok.
>>>> +	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);
>>>> +
>>>> +	/* Spec postoffset values */
>>>> +	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);
>>>> +
>>>> +	/* CSC mode before gamma */
>>>> +	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 +143,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 b4a6415..c5ff568 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -6288,6 +6288,23 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>>>>    		return -EINVAL;
>>>>    	}
>>>>    
>>>> +	/* YCBCR420 feasibility check */
>>>> +	if (pipe_config->ycbcr420) {
>>>> +		struct drm_crtc_state *drm_state = &pipe_config->base;
>>>> +
>>>> +		/*
>>>> +		 * 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_ERROR("YCBCR420 and CTM is not possible\n");
>>> DRM_DEBUG_KMS
>> We are failing an atomic_check(), shouldn't this be highlighted ?
> No. Any user can trigger this at will, and we don't want to spam dmesg
> with user triggerable errors. If you'll look around you'll see pretty
> much all error cases in the modeset paths use DRM_DEBUG_KMS/ATOMIC
> rather than DRM_ERROR. DRM_ERROR/etc. should be reserved for cases where
> the error wasn't caused by the user but rather eg. due to failing
> hardware.
Ok, got it.
- Shashank
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>>>> +		DRM_DEBUG_KMS("YCBCR420 output is possible from CRTC\n");
>>> Seems like a fairly pointless debug print.
>> Yeah, ok.
>> - Shashank
>>>> +	}
>>>> +
>>>>    	/*
>>>>    	 * Pipe horizontal size must be even in:
>>>>    	 * - DVO ganged mode
>>>> -- 
>>>> 2.7.4
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel