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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Feb 09, 2018 at 01:34:40PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Wednesday, January 31, 2018 1:44 AM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Lin, Johnson <johnson.lin@intel.com>;
> >Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
> ><maarten.lankhorst@intel.com>
> >Subject: Re: [Intel-gfx] [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@linux.intel.com]
> >> >Sent: Friday, December 22, 2017 9:43 PM
> >> >To: Shankar, Uma <uma.shankar@intel.com>
> >> >Cc: intel-gfx@lists.freedesktop.org; Lin, Johnson
> >> ><johnson.lin@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>;
> >> >Lankhorst, Maarten <maarten.lankhorst@intel.com>
> >> >Subject: Re: [Intel-gfx] [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@intel.com>
> >> >> >
> >> >> > 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@intel.com>
> >> >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >> >> > ---
> >> >> >  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@lists.freedesktop.org
> >> >> > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx