>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
>Sent: Friday, February 9, 2018 9:09 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, Feb 09, 2018 at 01:34:40PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----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.
>
>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.

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@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

Reply via email to