Re: [PATCH] drm/i915: Fix Limited Range Color Handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux