On Tue, Feb 13, 2018 at 10:37:03AM +0000, Shankar, Uma wrote: > > > >-----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. Hmm. I suppose there may be two factors here: the csc's limited precision, and the pipe's internally using more than 8bpc. I think it's 12bpc internally always. Looks like the csc should have 9 bit mantissa, so the closest to 219/255 we're going to get is 0x1b7. So assuming 12bpc internal precision we'll get 0xfff*0x1b7/0x200+0x100=0xeb7. Which actually overshoots the 0xeb0 max specified by the HDMI spec. So looks like we should perhaps be using 219/256 as the coefficient instead, which would give us a final value of 0xeaf, which is only one off from the 0xeb0. And the 0x100 I used in the calculation is also based on on the HDMI spec and matches 16/256 instead of 16/255, so that too should be changed in the code. Anyways even with the coefficients/postoffset fixed, we'll be one off from the expected 12bpc value. And presumably if we use the a 8bpc framebuffer we'll overshoot again to 0xebe (assuming the hw expands the precision in the way I expect). If we filled the fb with 0xea instead we'd get 0xeae, which is one off from the csc value, so not good either. With 10bpc fb we should be able to match the csc output of 0xeaf if we fill the fb with 0x3ab instead of the correct 0x3ac. Oh and we should actually have to consider the precision of the gamma LUT as well. By default we use the 8bpc one which presumably will chop off the low bits from the data. Hmm. Actually that should make things match even if we overshoot the target value a bit. Are you frobbing with the GAMMA_LUT property in your test? Anyway it seems to me that we should change the csc programming to use 219/256 and 16/256 values instead of the current ones. Whether that can help us get the crc to match I don't really know. > > 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 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
