[Intel-gfx] [PATCH] drm/i915/display: Prevent double YUV range correction on HDR planes

Wed Dec 16 06:46:58 UTC 2020

On Tue, Dec 15, 2020 at 3:13 PM Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
>
> On Tue, Dec 15, 2020 at 03:06:30PM -0500, Andres Calderon Jaramillo wrote:
> > On Tue, Dec 15, 2020 at 1:01 PM Ville Syrjälä
> > <ville.syrjala at linux.intel.com> wrote:
> > >
> > > On Mon, Dec 14, 2020 at 10:57:03PM +0000, Shankar, Uma wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: andrescj via sendgmr <andrescj at andrescj-cros-specialist.c.googlers.com>
> > > > > On Behalf Of Andres Calderon Jaramillo
> > > > > Sent: Tuesday, December 15, 2020 3:50 AM
> > > > > To: intel-gfx at lists.freedesktop.org
> > > > > Cc: Shankar, Uma <uma.shankar at intel.com>; Venkatesh Reddy, Sushma
> > > > > <sushma.venkatesh.reddy at intel.com>; seanpaul at chromium.org; Andres
> > > > > Calderon Jaramillo <andrescj at chromium.org>
> > > > > Subject: [PATCH] drm/i915/display: Prevent double YUV range correction on HDR
> > > > > planes
> > > > >
> > > > > From: Andres Calderon Jaramillo <andrescj at chromium.org>
> > > > >
> > > > > Prevent the ICL HDR plane pipeline from performing YUV color range correction
> > > > > twice when the input is in limited range.
> > > > >
> > > > > Before this patch the following could happen: user space gives us a YUV buffer in
> > > > > limited range; per the pipeline in [1], the plane would first go through a "YUV
> > > > > Range correct" stage that expands the range; the plane would then go through
> > > > > the "Input CSC" stage which would also expand the range because
> > > > > icl_program_input_csc() would use a matrix and an offset that assume limited-
> > > > > range input; this would ultimately cause dark and light colors to appear darker
> > > > > and lighter than they should respectively.
> > > > >
> > > > > This is an issue because if a buffer switches between being scanned out and
> > > > > being composited with the GPU, the user will see a color difference.
> > > > > If this switching happens quickly and frequently, the user will perceive this as a
> > > > > flickering.
> > > > >
> > > > > [1] https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-icllp-
> > > > > vol12-displayengine_0.pdf#page=281
> > > >
> > > > Change looks good to me, double conversion should not be done.
> > > > Plane input csc coefficients take care of the limited range.
> > >
> > > Might make sense to actually use the hw range correction instead.
> > > Would avoid having to keep around two sets of coefficients.
> > >
> > > Also the question now becomes: How can our tests be passing if
> > > we're doing the range correction twice?
> > >
> >
> > I also considered just removing the limited-range matrix/offsets from
> > icl_program_input_csc(). However, I figured that since the "Input CSC"
> > stage must happen regardless of range correction, maybe it would be a
> > gain to disable the "YUV Range Correction" stage. However, I'm not
> > familiar with the hardware details, so I don't know for sure. I don't
> > feel strongly either way; let me know what you'd prefer.
>
> IIRC we use the fixed function range compression + full range csc
> approach on chv as well. Wouldn't hurt to do the same thing on
> icl+ IMO.

Sounds good! Thanks for the review.

>
> >
> > I'm also curious about the testing. How do the tests work? I assume
> > they are in Intel's CI and not open sourced? Do they use the DRM
> > writeback connector (I didn't think this was implemented for i915
> > yet)?
>
> Tests are here:
> git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>
> In particular kms_plane/pixel-format tests should rather be failing I
> think. Unless we've relaxed things a bit too much to get the crcs to
> match when the results are close enough.
>

Nice. I took a look at the tests you referenced. It seems that we may
have just gotten unlucky with the color choice, but I'm not sure why
(I kind of expected to still get a color difference with the current
colors). I tried changing the color in [1] to {0.0f, 0.2f, 0.2f}, and
I got "CRC mismatches" failures for multiple formats including NV12,
C8, and YUYV. Interestingly, I had tried something like this multiple
times, and I couldn't get the CRC mismatch at first. I don't know what
changed. Maybe the mode was in a bad state and when I rebooted the
device, it was good.

Note, with that color change and with this patch, I still get "CRC
mismatches" failures but only for C8. I'm unfamiliar with that format,
but it seems like a different problem.

[1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/igt-gpu-tools/tests/kms_plane.c;l=383;drc=c8a9aa95e2c5c8d9d39f4f9388a7d602a2e64311

> >
> > > >
> > > > Reviewed-by: Uma Shankar <uma.shankar at intel.com>
> > > >
> > > > > Signed-off-by: Andres Calderon Jaramillo <andrescj at chromium.org>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_display.c | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index 761be8deaa9b..aeea344b06ad 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -4811,6 +4811,13 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state
> > > > > *crtc_state,
> > > > >                     plane_color_ctl |=
> > > > > PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
> > > > >     } else if (fb->format->is_yuv) {
> > > > >             plane_color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE;
> > > > > +
> > > > > +           /*
> > > > > +            * Disable the YUV range correction stage because the input CSC
> > > > > +            * stage already takes care of range conversion by using separate
> > > > > +            * matrices and offsets depending on the color range.
> > > > > +            */
> > > > > +           plane_color_ctl |=
> > > > > PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
> > > > >     }
> > > > >
> > > > >     return plane_color_ctl;
> > > > > --
> > > > > 2.29.2.684.gfbc64c5ab5-goog
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
>
> --
> Ville Syrjälä
> Intel