On Fri, Feb 09, 2018 at 01:34:40PM +0000, Shankar, Uma wrote:
>
>
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
> >Sent: Wednesday, January 31, 2018 1:44 AM
> >To: Shankar, Uma <uma.shan...@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Lin, Johnson <johnson....@intel.com>;
> >Syrjala, Ville <ville.syrj...@intel.com>; Lankhorst, Maarten
> ><maarten.lankho...@intel.com>
> >Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix Limited Range Color Handling
> >
> >On Tue, Jan 30, 2018 at 03:23:32PM +0000, Shankar, Uma wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
> >> >Sent: Friday, December 22, 2017 9:43 PM
> >> >To: Shankar, Uma <uma.shan...@intel.com>
> >> >Cc: intel-gfx@lists.freedesktop.org; Lin, Johnson
> >> ><johnson....@intel.com>; Syrjala, Ville <ville.syrj...@intel.com>;
> >> >Lankhorst, Maarten <maarten.lankho...@intel.com>
> >> >Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix Limited Range Color
> >> >Handling
> >> >
> >> >On Fri, Dec 22, 2017 at 05:54:31PM +0200, Ville Syrjälä wrote:
> >> >> On Fri, Dec 22, 2017 at 08:34:47PM +0530, Uma Shankar wrote:
> >> >> > From: Johnson Lin <johnson....@intel.com>
> >> >> >
> >> >> > Some panels support limited range output (16-235) compared to
> >> >> > full range RGB values (0-255). Also userspace can control the RGB
> >> >> > range using "Broadcast RGB" property. Currently the code to
> >> >> > handle full range to limited range is broken. This patch fixes
> >> >> > the same by properly scaling down all the full range
> >> >> > co-efficients with limited range scaling factor.
> >> >> >
> >> >> > Signed-off-by: Johnson Lin <johnson....@intel.com>
> >> >> > Signed-off-by: Uma Shankar <uma.shan...@intel.com>
> >> >> > ---
> >> >> > drivers/gpu/drm/i915/intel_color.c | 16 ++++++++++++----
> >> >> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >> >> >
> >> >> > diff --git a/drivers/gpu/drm/i915/intel_color.c
> >> >> > b/drivers/gpu/drm/i915/intel_color.c
> >> >> > index aa66e95..777ce26 100644
> >> >> > --- a/drivers/gpu/drm/i915/intel_color.c
> >> >> > +++ b/drivers/gpu/drm/i915/intel_color.c
> >> >> > @@ -94,16 +94,24 @@ static void ctm_mult_by_limited(uint64_t
> >> >> > *result,
> >> >int64_t *input)
> >> >> > for (i = 0; i < 9; i++)
> >> >> > result[i] = 0;
> >> >>
> >> >> This can go.
> >> >>
> >> >> >
> >> >> > - for (i = 0; i < 3; i++) {
> >> >> > - int64_t user_coeff = input[i * 3 + i];
> >> >> > + for (i = 0; i < 9; i++) {
> >> >> > + int64_t user_coeff = input[i];
> >> >>
> >> >> That's not a two's complement number so should probably be u64.
> >> >>
> >> >> > uint64_t limited_coeff = CTM_COEFF_LIMITED_RANGE >> 2;
> >> >> > uint64_t abs_coeff =
> >> >> > clamp_val(CTM_COEFF_ABS(user_coeff),
> >> >> > 0,
> >> >> > CTM_COEFF_4_0 - 1) >> 2;
> >> >>
> >> >> Seems to me that we should be able to drop the >>2 from the
> >> >> limited_coeff since it's always < 1.0, make both of these u32 and
> >> >> then use
> >> >> 'mul_u32_u32() >> 30' to get the answer.
> >> >>
> >> >> >
> >> >> > - result[i * 3 + i] = (limited_coeff * abs_coeff) >> 27;
> >> >> > + /*
> >> >> > + * By scaling every co-efficient with limited range
> >> >> > (235-16)
> >> >> > + * vs full range (0-255) the final o/p will be scaled
> >> >> > down to
> >> >> > + * fit in the limited range supported by the panel.
> >> >> > + * Need to shift the multiplication result by 28 as the
> >> >> > floating
> >> >> > + * count expected is of 32bits, multiplication here is
> >> >> > done in
> >> >> > + * U2.30 so result need to be right shifted by 60-32 =
> >> >> > 28
> >> >> > + */
> >> >> > + result[i] = (limited_coeff * abs_coeff) >> 28;
> >> >> > if (CTM_COEFF_NEGATIVE(user_coeff))
> >> >>
> >> >> And this can be replaced with just
> >> >>
> >> >> result[i] |= user_coeff & CTM_COEFF_SIGN;
> >> >>
> >> >> to eliminate the silly branch.
> >> >
> >> >I wonder if we could also get some igts for this limited range stuff.
> >> >Maybe something like:
> >> >1. grab crc from black/white screen with limited range 2. grab cdc
> >> >from almost black/white screen with full range 3. compare the two
> >> >
> >> >Repeat with an identity csc matrix set.
> >> >
> >> >Not sure if we can get the crcs to match in these two cases though.
> >> >Hard to say without trying it out.
> >> >
> >>
> >> Hi Ville,
> >> We will try this approach and update. Ideally crc's should match, will
> >> confirm the behavior on hardware.
> >
> >I suspect it's not going to work on some older platforms because we use the
> >magic bit in the pipe/port register to do the range compression. The effect
> >of
> >that bit may not be visible in the pipe crc. But starting from hsw it should
> >be ok.
> >
>
> Hi Ville,
> We tried to test this on GLK and the crc results are not matching. Tried
> below scenarios:
> 1. Flip FB (full white)with Broadcast RGB property set to "Full Range" and
> then with "Limited Range".
> Without applying/changing the CTM.
> 2. Flip FB (full white) with CTM set as unity and, one flip with "Full Range"
> and one with "Limited Range".
>
> In both the cases, crc is not matching. Not sure if this is an expected
> behavior.
Well, if you want them to match you need a fb with 16,16,16 and
235,235,235 rgb data for the full range test.
>
> Below is the IGT code snippet used, for reference:
>
> static void test_pipe_limited_range_ctm(data_t *data, igt_plane_t *primary)
> {
> color_t white[] = {
> { 1.0, 1.0, 1.0 },
> { 1.0, 1.0, 1.0 },
> { 1.0, 1.0, 1.0 }
> };
> color_t black[] = {
> { 0.0, 0.0, 0.0 },
> { 0.0, 0.0, 0.0 },
> { 0.0, 0.0, 0.0 }
> };
> double ctm[] = {
> 1.0, 0.0, 0.0,
> 0.0, 1.0, 0.0,
> 0.0, 0.0, 1.0 };
> double *degamma_linear, *gamma_linear;
> igt_output_t *output;
> bool has_broadcast_rgb_output = false;
>
> degamma_linear = generate_table(data->degamma_lut_size, 1.0);
> gamma_linear = generate_table(data->gamma_lut_size, 1.0);
>
> for_each_valid_output_on_pipe(&data->display, primary->pipe->pipe,
> output) {
> drmModeModeInfo *mode;
> struct igt_fb fb_modeset, fb;
> igt_crc_t crc_full, crc_limited;
> int fb_id, fb_modeset_id;
>
> if (!igt_output_has_prop(output, IGT_CONNECTOR_BROADCAST_RGB))
> continue;
> has_broadcast_rgb_output = true;
> igt_output_set_pipe(output, primary->pipe->pipe);
> mode = igt_output_get_mode(output);
>
> /* Create a framebuffer at the size of the output. */
> fb_id = igt_create_fb(data->drm_fd, mode->hdisplay,
> mode->vdisplay,
> DRM_FORMAT_XRGB8888,
> LOCAL_DRM_FORMAT_MOD_NONE, &fb);
> igt_assert(fb_id);
>
> fb_modeset_id = igt_create_fb(data->drm_fd, mode->hdisplay,
> mode->vdisplay,
> DRM_FORMAT_XRGB8888,
> LOCAL_DRM_FORMAT_MOD_NONE,
> &fb_modeset);
> igt_assert(fb_modeset_id);
> igt_plane_set_fb(primary, &fb_modeset);
>
> set_degamma(data, primary->pipe, degamma_linear);
> set_gamma(data, primary->pipe, gamma_linear);
> set_ctm(primary->pipe, ctm);
>
> igt_output_set_prop_value(output,
> IGT_CONNECTOR_BROADCAST_RGB, BROADCAST_RGB_FULL);
> paint_rectangles(data, mode, black, &fb);
> igt_plane_set_fb(primary, &fb);
> igt_display_commit(&data->display);
> igt_wait_for_vblank(data->drm_fd, primary->pipe->pipe);
> igt_pipe_crc_collect_crc(data->pipe_crc, &crc_full);
>
> /* Set the output into limited range. */
> igt_output_set_prop_value(output,
> IGT_CONNECTOR_BROADCAST_RGB, BROADCAST_RGB_16_235);
> paint_rectangles(data, mode, black, &fb);
> igt_plane_set_fb(primary, &fb);
> igt_display_commit(&data->display);
> igt_wait_for_vblank(data->drm_fd, primary->pipe->pipe);
> igt_pipe_crc_collect_crc(data->pipe_crc, &crc_limited);
>
> igt_assert_crc_equal(&crc_full, &crc_limited);
>
> Regards,
> Uma Shankar
> >> >>
> >> >>
> >> >> > - result[i * 3 + i] |= CTM_COEFF_SIGN;
> >> >> > + result[i] |= CTM_COEFF_SIGN;
> >> >> > }
> >> >> > }
> >> >> >
> >> >> > --
> >> >> > 1.7.9.5
> >> >> >
> >> >> > _______________________________________________
> >> >> > Intel-gfx mailing list
> >> >> > Intel-gfx@lists.freedesktop.org
> >> >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >>
> >> >> --
> >> >> Ville Syrjälä
> >> >> Intel OTC
> >> >
> >> >--
> >> >Ville Syrjälä
> >> >Intel OTC
> >
> >--
> >Ville Syrjälä
> >Intel OTC
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
