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@xxxxxxxxx>
> >> ---
> >> 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.
> >
> >> + 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.
> >> + 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.
> >
> >> + 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@xxxxxxxxxxxxxxxxxxxxx
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
