>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] >Sent: Friday, February 9, 2018 9:09 PM >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Lin, Johnson <johnson.lin@xxxxxxxxx>; >Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten ><maarten.lankhorst@xxxxxxxxx> >Subject: Re: [PATCH] drm/i915: Fix Limited Range Color Handling > >On Fri, Feb 09, 2018 at 01:34:40PM +0000, Shankar, Uma wrote: >> >> >> >-----Original Message----- >> >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] >> >Sent: Wednesday, January 31, 2018 1:44 AM >> >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Lin, Johnson >> ><johnson.lin@xxxxxxxxx>; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; >> >Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx> >> >Subject: Re: [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.syrjala@xxxxxxxxxxxxxxx] >> >> >Sent: Friday, December 22, 2017 9:43 PM >> >> >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >> >> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Lin, Johnson >> >> ><johnson.lin@xxxxxxxxx>; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; >> >> >Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx> >> >> >Subject: Re: [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.lin@xxxxxxxxx> >> >> >> > >> >> >> > 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.lin@xxxxxxxxx> >> >> >> > Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx> >> >> >> > --- >> >> >> > 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. > We tried to give input as lr = ((1.0/255)*235) in order to scale down all RGB values to 235. color_t white_lr[] = { { lr, lr, lr }, { lr, lr, lr }, { lr, lr, lr } }; We flipped this with Full Range but still CRC didn't matched on GLK. I am not sure if h/w is modifying the data wrt Full Range and Limited Range. Regards, Uma Shankar >> 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@xxxxxxxxxxxxxxxxxxxxx >> >> >> > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
