>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
>Sent: Wednesday, January 31, 2018 1:44 AM
>To: Shankar, Uma <uma.shan...@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Lin, Johnson <johnson....@intel.com>;
>Syrjala, Ville <ville.syrj...@intel.com>; Lankhorst, Maarten
><maarten.lankho...@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.syrj...@linux.intel.com]
>> >Sent: Friday, December 22, 2017 9:43 PM
>> >To: Shankar, Uma <uma.shan...@intel.com>
>> >Cc: intel-gfx@lists.freedesktop.org; Lin, Johnson
>> ><johnson....@intel.com>; Syrjala, Ville <ville.syrj...@intel.com>;
>> >Lankhorst, Maarten <maarten.lankho...@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....@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....@intel.com>
>> >> > Signed-off-by: Uma Shankar <uma.shan...@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.
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
