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. > > 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
