[Intel-gfx] [PATCH 13/20] drm/i915: prepare csc unit for YCBCR420 output
Sharma, Shashank
shashank.sharma at intel.com
Thu Jul 13 05:14:51 UTC 2017
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 at 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"
>
>> + 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 */
>> + 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 ?
>
>> + 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the Intel-gfx
mailing list