>> >> >> >-----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? We tried to test with gamma/degamma disable just to rule out any precision loss. Also, to even rule out CSC we tried disabling CTM block so as to make sure the reason of delta remains only the pipe precision (12 bits in case of GLK). And we used 219/256 as the element of the RGB matrix while creating the Frame Buffer. So we gave 219/256 as buffer (8bit)- Flip with limited range and with Full Range. In this case also, CRC's are not matching but the result was very close (not sure how the CRC algorithms are implemented). I don't think close is any good, till it matches exactly :( Another approach I can think of is - to use some golden references. Pre-computed CRC's (obtained experimentally) can be compared with CRC obtained while executing the tests to catch any regression if any. Not sure if this is a good idea though !!! Regards, Uma Shankar > >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
