* [PATCH] drm/i915: Fix Limited Range Color Handling
@ 2017-12-22 15:04 Uma Shankar
2017-12-22 15:20 ` ✓ Fi.CI.BAT: success for " Patchwork
` (5 more replies)
0 siblings, 6 replies; 23+ messages in thread
From: Uma Shankar @ 2017-12-22 15:04 UTC (permalink / raw)
To: intel-gfx; +Cc: Johnson Lin, ville.syrjala, maarten.lankhorst
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;
- 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];
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;
- 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))
- 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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Fix Limited Range Color Handling
2017-12-22 15:04 [PATCH] drm/i915: Fix Limited Range Color Handling Uma Shankar
@ 2017-12-22 15:20 ` Patchwork
2017-12-22 15:54 ` [PATCH] " Ville Syrjälä
` (4 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2017-12-22 15:20 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Fix Limited Range Color Handling
URL : https://patchwork.freedesktop.org/series/35725/
State : success
== Summary ==
Series 35725v1 drm/i915: Fix Limited Range Color Handling
https://patchwork.freedesktop.org/api/1.0/series/35725/revisions/1/mbox/
Test gem_mmap_gtt:
Subgroup basic-small-bo-tiledx:
fail -> PASS (fi-gdg-551) fdo#102575
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-a:
dmesg-warn -> PASS (fi-kbl-r) fdo#104172 +1
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#104172 https://bugs.freedesktop.org/show_bug.cgi?id=104172
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:435s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:439s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:381s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:493s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:276s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:489s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:492s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:476s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:462s
fi-elk-e7500 total:224 pass:163 dwarn:15 dfail:0 fail:0 skip:45
fi-gdg-551 total:288 pass:180 dwarn:0 dfail:0 fail:0 skip:108 time:262s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:531s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:411s
fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:414s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:427s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:478s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:431s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:475s
fi-kbl-7560u total:288 pass:268 dwarn:1 dfail:0 fail:0 skip:19 time:522s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:470s
fi-kbl-r total:288 pass:260 dwarn:1 dfail:0 fail:0 skip:27 time:528s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:590s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:447s
fi-skl-6600u total:288 pass:260 dwarn:1 dfail:0 fail:0 skip:27 time:529s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:555s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:500s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:497s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:445s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:549s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:411s
Blacklisted hosts:
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:592s
fi-glk-dsi total:288 pass:176 dwarn:1 dfail:4 fail:0 skip:107 time:329s
34783c4253df6206c731735524b0297c6894384b drm-tip: 2017y-12m-22d-12h-46m-57s UTC integration manifest
48e20b9cbe1e drm/i915: Fix Limited Range Color Handling
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7568/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: Fix Limited Range Color Handling
2017-12-22 15:04 [PATCH] drm/i915: Fix Limited Range Color Handling Uma Shankar
2017-12-22 15:20 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2017-12-22 15:54 ` Ville Syrjälä
2017-12-22 16:12 ` Ville Syrjälä
2017-12-29 13:58 ` Shankar, Uma
2017-12-22 17:00 ` ✗ Fi.CI.IGT: warning for " Patchwork
` (3 subsequent siblings)
5 siblings, 2 replies; 23+ messages in thread
From: Ville Syrjälä @ 2017-12-22 15:54 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx, ville.syrjala, Johnson Lin, maarten.lankhorst
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.
> - 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: Fix Limited Range Color Handling
2017-12-22 15:54 ` [PATCH] " Ville Syrjälä
@ 2017-12-22 16:12 ` Ville Syrjälä
2018-01-30 15:23 ` Shankar, Uma
2017-12-29 13:58 ` Shankar, Uma
1 sibling, 1 reply; 23+ messages in thread
From: Ville Syrjälä @ 2017-12-22 16:12 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx, ville.syrjala, Johnson Lin, maarten.lankhorst
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.
>
>
> > - 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* ✗ Fi.CI.IGT: warning for drm/i915: Fix Limited Range Color Handling
2017-12-22 15:04 [PATCH] drm/i915: Fix Limited Range Color Handling Uma Shankar
2017-12-22 15:20 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-12-22 15:54 ` [PATCH] " Ville Syrjälä
@ 2017-12-22 17:00 ` Patchwork
2018-01-02 10:13 ` ✓ Fi.CI.BAT: success for drm/i915: Fix Limited Range Color Handling (rev2) Patchwork
` (2 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2017-12-22 17:00 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Fix Limited Range Color Handling
URL : https://patchwork.freedesktop.org/series/35725/
State : warning
== Summary ==
Test kms_flip:
Subgroup vblank-vs-modeset-suspend-interruptible:
pass -> SKIP (shard-snb)
skip -> PASS (shard-hsw) fdo#103540 +1
Test gem_mmap_wc:
Subgroup set-cache-level:
skip -> PASS (shard-snb)
Test kms_flip_event_leak:
skip -> PASS (shard-snb)
Test kms_chv_cursor_fail:
Subgroup pipe-c-128x128-top-edge:
incomplete -> PASS (shard-hsw)
Test pm_rpm:
Subgroup system-suspend-execbuf:
skip -> PASS (shard-hsw) fdo#103375
fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
shard-hsw total:2712 pass:1537 dwarn:1 dfail:0 fail:10 skip:1164 time:9460s
shard-snb total:2712 pass:1309 dwarn:1 dfail:0 fail:10 skip:1392 time:8043s
Blacklisted hosts:
shard-apl total:2712 pass:1687 dwarn:1 dfail:0 fail:23 skip:1001 time:13731s
shard-kbl total:2689 pass:1793 dwarn:1 dfail:0 fail:22 skip:872 time:10974s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7568/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: Fix Limited Range Color Handling
2017-12-22 15:54 ` [PATCH] " Ville Syrjälä
2017-12-22 16:12 ` Ville Syrjälä
@ 2017-12-29 13:58 ` Shankar, Uma
2017-12-29 14:59 ` Uma Shankar
1 sibling, 1 reply; 23+ messages in thread
From: Shankar, Uma @ 2017-12-29 13:58 UTC (permalink / raw)
To: Ville Syrjälä
Cc: intel-gfx, Syrjala, Ville, Lin, Johnson, Lankhorst, Maarten
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Friday, December 22, 2017 9:25 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 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.
>
Yes, will remove this.
>>
>> - 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.
Ok, will change this.
>
>> 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.
Yes, this should work. Will change this.
>
>>
>> - 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.
>
Sure, this can be changed. Will update this.
Thanks Ville for your comments.
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] drm/i915: Fix Limited Range Color Handling
2017-12-29 13:58 ` Shankar, Uma
@ 2017-12-29 14:59 ` Uma Shankar
2018-01-29 18:34 ` Ville Syrjälä
0 siblings, 1 reply; 23+ messages in thread
From: Uma Shankar @ 2017-12-29 14:59 UTC (permalink / raw)
To: intel-gfx; +Cc: Johnson Lin, ville.syrjala, maarten.lankhorst
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.
v2: Fixed Ville's review comments.
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 | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index aa66e95..55408e2 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -84,26 +84,27 @@ static bool crtc_state_is_legacy_gamma(struct drm_crtc_state *state)
/*
* When using limited range, multiply the matrix given by userspace by
- * the matrix that we would use for the limited range. We do the
- * multiplication in U2.30 format.
+ * the matrix that we would use for the limited range.
*/
static void ctm_mult_by_limited(uint64_t *result, int64_t *input)
{
int i;
- for (i = 0; i < 9; i++)
- result[i] = 0;
+ for (i = 0; i < 9; i++) {
+ uint64_t user_coeff = input[i];
+ uint32_t limited_coeff = CTM_COEFF_LIMITED_RANGE;
+ uint32_t abs_coeff = (uint32_t)(clamp_val(
+ CTM_COEFF_ABS(user_coeff),
+ 0,
+ CTM_COEFF_4_0 - 1) >> 2);
- for (i = 0; i < 3; i++) {
- int64_t user_coeff = input[i * 3 + i];
- 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;
-
- result[i * 3 + i] = (limited_coeff * abs_coeff) >> 27;
- if (CTM_COEFF_NEGATIVE(user_coeff))
- result[i * 3 + i] |= CTM_COEFF_SIGN;
+ /*
+ * By scaling every co-efficient with limited range (16-235)
+ * vs full range (0-255) the final o/p will be scaled down to
+ * fit in the limited range supported by the panel.
+ */
+ result[i] = mul_u32_u32(limited_coeff, abs_coeff) >> 30;
+ result[i] |= user_coeff & CTM_COEFF_SIGN;
}
}
--
1.7.9.5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Fix Limited Range Color Handling (rev2)
2017-12-22 15:04 [PATCH] drm/i915: Fix Limited Range Color Handling Uma Shankar
` (2 preceding siblings ...)
2017-12-22 17:00 ` ✗ Fi.CI.IGT: warning for " Patchwork
@ 2018-01-02 10:13 ` Patchwork
2018-01-30 18:47 ` ✓ Fi.CI.BAT: success for drm/i915: Fix Limited Range Color Handling (rev4) Patchwork
2018-01-30 22:48 ` ✗ Fi.CI.IGT: failure " Patchwork
5 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2018-01-02 10:13 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Fix Limited Range Color Handling (rev2)
URL : https://patchwork.freedesktop.org/series/35725/
State : success
== Summary ==
Series 35725v2 drm/i915: Fix Limited Range Color Handling
https://patchwork.freedesktop.org/api/1.0/series/35725/revisions/2/mbox/
Test debugfs_test:
Subgroup read_all_entries:
dmesg-warn -> DMESG-FAIL (fi-elk-e7500) fdo#103989 +1
Test gem_mmap_gtt:
Subgroup basic-small-bo-tiledx:
pass -> FAIL (fi-gdg-551) fdo#102575
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-a:
dmesg-warn -> PASS (fi-kbl-r) fdo#104172 +1
Test prime_vgem:
Subgroup basic-fence-flip:
fail -> PASS (fi-bwr-2160) fdo#103182
fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#104172 https://bugs.freedesktop.org/show_bug.cgi?id=104172
fdo#103182 https://bugs.freedesktop.org/show_bug.cgi?id=103182
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:435s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:381s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:491s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:275s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:488s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:493s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:473s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:472s
fi-elk-e7500 total:224 pass:163 dwarn:14 dfail:1 fail:0 skip:45
fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:261s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:536s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:403s
fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:412s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:421s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:474s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:428s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:472s
fi-kbl-7560u total:288 pass:268 dwarn:1 dfail:0 fail:0 skip:19 time:519s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:465s
fi-kbl-r total:288 pass:260 dwarn:1 dfail:0 fail:0 skip:27 time:522s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:583s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:444s
fi-skl-6600u total:288 pass:260 dwarn:1 dfail:0 fail:0 skip:27 time:523s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:550s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:506s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:494s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:446s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:546s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:409s
Blacklisted hosts:
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:592s
fi-cnl-y total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:628s
fi-bdw-gvtdvm failed to collect. IGT log at Patchwork_7592/fi-bdw-gvtdvm/igt.log
fi-glk-dsi failed to collect. IGT log at Patchwork_7592/fi-glk-dsi/igt.log
8cb9e9870b50ddf65ebe233d38194cafdd811d03 drm-tip: 2017y-12m-28d-17h-00m-27s UTC integration manifest
9f0a777e5ff0 drm/i915: Fix Limited Range Color Handling
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7592/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: Fix Limited Range Color Handling
2017-12-29 14:59 ` Uma Shankar
@ 2018-01-29 18:34 ` Ville Syrjälä
2018-01-30 15:19 ` [v3] " Uma Shankar
0 siblings, 1 reply; 23+ messages in thread
From: Ville Syrjälä @ 2018-01-29 18:34 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx, ville.syrjala, Johnson Lin, maarten.lankhorst
On Fri, Dec 29, 2017 at 08:29:11PM +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.
>
> v2: Fixed Ville's review comments.
>
> 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 | 29 +++++++++++++++--------------
> 1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index aa66e95..55408e2 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -84,26 +84,27 @@ static bool crtc_state_is_legacy_gamma(struct drm_crtc_state *state)
>
> /*
> * When using limited range, multiply the matrix given by userspace by
> - * the matrix that we would use for the limited range. We do the
> - * multiplication in U2.30 format.
> + * the matrix that we would use for the limited range.
> */
> static void ctm_mult_by_limited(uint64_t *result, int64_t *input)
Since your touching pretty much every line anyway, maybe
s/uint64/u64/ etc. all over.
'input' should really be const u64 *.
> {
> int i;
>
> - for (i = 0; i < 9; i++)
> - result[i] = 0;
> + for (i = 0; i < 9; i++) {
> + uint64_t user_coeff = input[i];
> + uint32_t limited_coeff = CTM_COEFF_LIMITED_RANGE;
> + uint32_t abs_coeff = (uint32_t)(clamp_val(
The cast is not needed.
Apart from those nits this lgtm.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> + CTM_COEFF_ABS(user_coeff),
> + 0,
> + CTM_COEFF_4_0 - 1) >> 2);
>
> - for (i = 0; i < 3; i++) {
> - int64_t user_coeff = input[i * 3 + i];
> - 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;
> -
> - result[i * 3 + i] = (limited_coeff * abs_coeff) >> 27;
> - if (CTM_COEFF_NEGATIVE(user_coeff))
> - result[i * 3 + i] |= CTM_COEFF_SIGN;
> + /*
> + * By scaling every co-efficient with limited range (16-235)
> + * vs full range (0-255) the final o/p will be scaled down to
> + * fit in the limited range supported by the panel.
> + */
> + result[i] = mul_u32_u32(limited_coeff, abs_coeff) >> 30;
> + result[i] |= user_coeff & 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* [v3] drm/i915: Fix Limited Range Color Handling
2018-01-29 18:34 ` Ville Syrjälä
@ 2018-01-30 15:19 ` Uma Shankar
2018-01-30 15:23 ` Ville Syrjälä
0 siblings, 1 reply; 23+ messages in thread
From: Uma Shankar @ 2018-01-30 15:19 UTC (permalink / raw)
To: intel-gfx; +Cc: Johnson Lin, ville.syrjala, maarten.lankhorst
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.
v2: Fixed Ville's review comments.
v3: Changed input to const and used correct data types as
suggested by Ville
Signed-off-by: Johnson Lin <johnson.lin@intel.com>
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
Reviewed-by: Ville Syrjä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_color.c | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 333a23b..3b708a9 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -84,26 +84,27 @@ static bool crtc_state_is_legacy_gamma(struct drm_crtc_state *state)
/*
* When using limited range, multiply the matrix given by userspace by
- * the matrix that we would use for the limited range. We do the
- * multiplication in U2.30 format.
+ * the matrix that we would use for the limited range.
*/
-static void ctm_mult_by_limited(uint64_t *result, int64_t *input)
+static void ctm_mult_by_limited(u64 *result, const u64 *input)
{
int i;
- for (i = 0; i < 9; i++)
- result[i] = 0;
+ for (i = 0; i < 9; i++) {
+ u64 user_coeff = input[i];
+ uint32_t limited_coeff = CTM_COEFF_LIMITED_RANGE;
+ uint32_t abs_coeff = clamp_val(
+ CTM_COEFF_ABS(user_coeff),
+ 0,
+ CTM_COEFF_4_0 - 1) >> 2;
- for (i = 0; i < 3; i++) {
- int64_t user_coeff = input[i * 3 + i];
- 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;
-
- result[i * 3 + i] = (limited_coeff * abs_coeff) >> 27;
- if (CTM_COEFF_NEGATIVE(user_coeff))
- result[i * 3 + i] |= CTM_COEFF_SIGN;
+ /*
+ * By scaling every co-efficient with limited range (16-235)
+ * vs full range (0-255) the final o/p will be scaled down to
+ * fit in the limited range supported by the panel.
+ */
+ result[i] = mul_u32_u32(limited_coeff, abs_coeff) >> 30;
+ result[i] |= user_coeff & CTM_COEFF_SIGN;
}
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [v3] drm/i915: Fix Limited Range Color Handling
2018-01-30 15:19 ` [v3] " Uma Shankar
@ 2018-01-30 15:23 ` Ville Syrjälä
2018-01-30 15:43 ` Shankar, Uma
2018-01-30 15:51 ` [v4] " Uma Shankar
0 siblings, 2 replies; 23+ messages in thread
From: Ville Syrjälä @ 2018-01-30 15:23 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx, ville.syrjala, Johnson Lin, maarten.lankhorst
On Tue, Jan 30, 2018 at 08:49:22PM +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.
>
> v2: Fixed Ville's review comments.
>
> v3: Changed input to const and used correct data types as
> suggested by Ville
>
> Signed-off-by: Johnson Lin <johnson.lin@intel.com>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> Reviewed-by: Ville Syrjä <ville.syrjala@linux.intel.com>
Name fail.
> ---
> drivers/gpu/drm/i915/intel_color.c | 31 ++++++++++++++++---------------
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 333a23b..3b708a9 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -84,26 +84,27 @@ static bool crtc_state_is_legacy_gamma(struct drm_crtc_state *state)
>
> /*
> * When using limited range, multiply the matrix given by userspace by
> - * the matrix that we would use for the limited range. We do the
> - * multiplication in U2.30 format.
> + * the matrix that we would use for the limited range.
> */
> -static void ctm_mult_by_limited(uint64_t *result, int64_t *input)
> +static void ctm_mult_by_limited(u64 *result, const u64 *input)
> {
> int i;
>
> - for (i = 0; i < 9; i++)
> - result[i] = 0;
> + for (i = 0; i < 9; i++) {
> + u64 user_coeff = input[i];
> + uint32_t limited_coeff = CTM_COEFF_LIMITED_RANGE;
You forgot s/uint32_t/u32/ here...
> + uint32_t abs_coeff = clamp_val(
and here. And aving a newline here seems weird.
> + CTM_COEFF_ABS(user_coeff),
> + 0,
> + CTM_COEFF_4_0 - 1) >> 2;
>
> - for (i = 0; i < 3; i++) {
> - int64_t user_coeff = input[i * 3 + i];
> - 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;
> -
> - result[i * 3 + i] = (limited_coeff * abs_coeff) >> 27;
> - if (CTM_COEFF_NEGATIVE(user_coeff))
> - result[i * 3 + i] |= CTM_COEFF_SIGN;
> + /*
> + * By scaling every co-efficient with limited range (16-235)
> + * vs full range (0-255) the final o/p will be scaled down to
> + * fit in the limited range supported by the panel.
> + */
> + result[i] = mul_u32_u32(limited_coeff, abs_coeff) >> 30;
> + result[i] |= user_coeff & CTM_COEFF_SIGN;
> }
> }
>
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: Fix Limited Range Color Handling
2017-12-22 16:12 ` Ville Syrjälä
@ 2018-01-30 15:23 ` Shankar, Uma
2018-01-30 20:13 ` Ville Syrjälä
0 siblings, 1 reply; 23+ messages in thread
From: Shankar, Uma @ 2018-01-30 15:23 UTC (permalink / raw)
To: Ville Syrjälä
Cc: intel-gfx, Syrjala, Ville, Lin, Johnson, Lankhorst, Maarten
>-----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.
Thanks & 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [v3] drm/i915: Fix Limited Range Color Handling
2018-01-30 15:23 ` Ville Syrjälä
@ 2018-01-30 15:43 ` Shankar, Uma
2018-01-30 15:51 ` [v4] " Uma Shankar
1 sibling, 0 replies; 23+ messages in thread
From: Shankar, Uma @ 2018-01-30 15:43 UTC (permalink / raw)
To: Ville Syrjälä
Cc: intel-gfx, Syrjala, Ville, Lin, Johnson, Lankhorst, Maarten
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Tuesday, January 30, 2018 8:53 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] [v3] drm/i915: Fix Limited Range Color Handling
>
>On Tue, Jan 30, 2018 at 08:49:22PM +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.
>>
>> v2: Fixed Ville's review comments.
>>
>> v3: Changed input to const and used correct data types as
>> suggested by Ville
>>
>> Signed-off-by: Johnson Lin <johnson.lin@intel.com>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> Reviewed-by: Ville Syrjä <ville.syrjala@linux.intel.com>
>
>Name fail.
>
Have updated this.
>> ---
>> drivers/gpu/drm/i915/intel_color.c | 31
>> ++++++++++++++++---------------
>> 1 file changed, 16 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 333a23b..3b708a9 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -84,26 +84,27 @@ static bool crtc_state_is_legacy_gamma(struct
>> drm_crtc_state *state)
>>
>> /*
>> * When using limited range, multiply the matrix given by userspace
>> by
>> - * the matrix that we would use for the limited range. We do the
>> - * multiplication in U2.30 format.
>> + * the matrix that we would use for the limited range.
>> */
>> -static void ctm_mult_by_limited(uint64_t *result, int64_t *input)
>> +static void ctm_mult_by_limited(u64 *result, const u64 *input)
>> {
>> int i;
>>
>> - for (i = 0; i < 9; i++)
>> - result[i] = 0;
>> + for (i = 0; i < 9; i++) {
>> + u64 user_coeff = input[i];
>> + uint32_t limited_coeff = CTM_COEFF_LIMITED_RANGE;
>
>You forgot s/uint32_t/u32/ here...
This file is using this extensively, have changed this function though. I guess it will
need a bigger cleanup.
>
>> + uint32_t abs_coeff = clamp_val(
>
>and here. And aving a newline here seems weird.
Have updated this and re-sent a new patch.
Thanks & Regards,
Uma Shankar
>
>> + CTM_COEFF_ABS(user_coeff),
>> + 0,
>> + CTM_COEFF_4_0 - 1) >> 2;
>>
>> - for (i = 0; i < 3; i++) {
>> - int64_t user_coeff = input[i * 3 + i];
>> - 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;
>> -
>> - result[i * 3 + i] = (limited_coeff * abs_coeff) >> 27;
>> - if (CTM_COEFF_NEGATIVE(user_coeff))
>> - result[i * 3 + i] |= CTM_COEFF_SIGN;
>> + /*
>> + * By scaling every co-efficient with limited range (16-235)
>> + * vs full range (0-255) the final o/p will be scaled down to
>> + * fit in the limited range supported by the panel.
>> + */
>> + result[i] = mul_u32_u32(limited_coeff, abs_coeff) >> 30;
>> + result[i] |= user_coeff & CTM_COEFF_SIGN;
>> }
>> }
>>
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Ville Syrjälä
>Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* [v4] drm/i915: Fix Limited Range Color Handling
2018-01-30 15:23 ` Ville Syrjälä
2018-01-30 15:43 ` Shankar, Uma
@ 2018-01-30 15:51 ` Uma Shankar
2018-02-22 16:15 ` Ville Syrjälä
1 sibling, 1 reply; 23+ messages in thread
From: Uma Shankar @ 2018-01-30 15:51 UTC (permalink / raw)
To: intel-gfx; +Cc: Johnson Lin, ville.syrjala, maarten.lankhorst
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.
v2: Fixed Ville's review comments.
v3: Changed input to const and used correct data types as
suggested by Ville
v4: Fixed some missing data type corrections.
Signed-off-by: Johnson Lin <johnson.lin@intel.com>
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_color.c | 29 ++++++++++++++---------------
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 333a23b..0b505d4 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -84,26 +84,25 @@ static bool crtc_state_is_legacy_gamma(struct drm_crtc_state *state)
/*
* When using limited range, multiply the matrix given by userspace by
- * the matrix that we would use for the limited range. We do the
- * multiplication in U2.30 format.
+ * the matrix that we would use for the limited range.
*/
-static void ctm_mult_by_limited(uint64_t *result, int64_t *input)
+static void ctm_mult_by_limited(u64 *result, const u64 *input)
{
int i;
- for (i = 0; i < 9; i++)
- result[i] = 0;
+ for (i = 0; i < 9; i++) {
+ u64 user_coeff = input[i];
+ u32 limited_coeff = CTM_COEFF_LIMITED_RANGE;
+ u32 abs_coeff = clamp_val(CTM_COEFF_ABS(user_coeff), 0,
+ CTM_COEFF_4_0 - 1) >> 2;
- for (i = 0; i < 3; i++) {
- int64_t user_coeff = input[i * 3 + i];
- 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;
-
- result[i * 3 + i] = (limited_coeff * abs_coeff) >> 27;
- if (CTM_COEFF_NEGATIVE(user_coeff))
- result[i * 3 + i] |= CTM_COEFF_SIGN;
+ /*
+ * By scaling every co-efficient with limited range (16-235)
+ * vs full range (0-255) the final o/p will be scaled down to
+ * fit in the limited range supported by the panel.
+ */
+ result[i] = mul_u32_u32(limited_coeff, abs_coeff) >> 30;
+ result[i] |= user_coeff & CTM_COEFF_SIGN;
}
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Fix Limited Range Color Handling (rev4)
2017-12-22 15:04 [PATCH] drm/i915: Fix Limited Range Color Handling Uma Shankar
` (3 preceding siblings ...)
2018-01-02 10:13 ` ✓ Fi.CI.BAT: success for drm/i915: Fix Limited Range Color Handling (rev2) Patchwork
@ 2018-01-30 18:47 ` Patchwork
2018-01-30 22:48 ` ✗ Fi.CI.IGT: failure " Patchwork
5 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2018-01-30 18:47 UTC (permalink / raw)
To: Shankar, Uma; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Fix Limited Range Color Handling (rev4)
URL : https://patchwork.freedesktop.org/series/35725/
State : success
== Summary ==
Series 35725v4 drm/i915: Fix Limited Range Color Handling
https://patchwork.freedesktop.org/api/1.0/series/35725/revisions/4/mbox/
Test gem_mmap_gtt:
Subgroup basic-small-bo-tiledx:
fail -> PASS (fi-gdg-551) fdo#102575
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-b:
incomplete -> PASS (fi-snb-2520m) fdo#103713
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:418s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:422s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:372s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:483s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:283s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:481s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:485s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:464s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:452s
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:563s
fi-elk-e7500 total:224 pass:168 dwarn:10 dfail:0 fail:0 skip:45
fi-gdg-551 total:288 pass:180 dwarn:0 dfail:0 fail:0 skip:108 time:279s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:513s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:391s
fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:400s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:413s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:461s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:414s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:458s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:495s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:454s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:505s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:582s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:425s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:512s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:527s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:490s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:476s
fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:422s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:431s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:525s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:394s
Blacklisted hosts:
fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:469s
9b156ad76b2e755bf527297486646cf27bbf1abd drm-tip: 2018y-01m-30d-17h-06m-58s UTC integration manifest
b61e18d62a90 drm/i915: Fix Limited Range Color Handling
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7827/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: Fix Limited Range Color Handling
2018-01-30 15:23 ` Shankar, Uma
@ 2018-01-30 20:13 ` Ville Syrjälä
2018-02-09 13:34 ` Shankar, Uma
0 siblings, 1 reply; 23+ messages in thread
From: Ville Syrjälä @ 2018-01-30 20:13 UTC (permalink / raw)
To: Shankar, Uma; +Cc: intel-gfx, Syrjala, Ville, Lin, Johnson, Lankhorst, Maarten
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.
> >>
> >>
> >> > - 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* ✗ Fi.CI.IGT: failure for drm/i915: Fix Limited Range Color Handling (rev4)
2017-12-22 15:04 [PATCH] drm/i915: Fix Limited Range Color Handling Uma Shankar
` (4 preceding siblings ...)
2018-01-30 18:47 ` ✓ Fi.CI.BAT: success for drm/i915: Fix Limited Range Color Handling (rev4) Patchwork
@ 2018-01-30 22:48 ` Patchwork
5 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2018-01-30 22:48 UTC (permalink / raw)
To: Shankar, Uma; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Fix Limited Range Color Handling (rev4)
URL : https://patchwork.freedesktop.org/series/35725/
State : failure
== Summary ==
Test pm_rc6_residency:
Subgroup rc6-accuracy:
skip -> PASS (shard-snb)
Test kms_vblank:
Subgroup pipe-c-accuracy-idle:
pass -> FAIL (shard-hsw)
Test gem_exec_schedule:
Subgroup reorder-wide-vebox:
pass -> FAIL (shard-apl) fdo#102848
Test kms_sysfs_edid_timing:
pass -> WARN (shard-apl) fdo#100047
Test perf:
Subgroup polling:
fail -> PASS (shard-hsw) fdo#102252
Test kms_plane:
Subgroup plane-panning-bottom-right-suspend-pipe-b-planes:
dmesg-warn -> PASS (shard-snb)
Test kms_flip:
Subgroup 2x-plain-flip-fb-recreate:
fail -> PASS (shard-hsw) fdo#100368
Test kms_frontbuffer_tracking:
Subgroup fbc-1p-primscrn-spr-indfb-draw-render:
pass -> FAIL (shard-apl) fdo#103167
fdo#102848 https://bugs.freedesktop.org/show_bug.cgi?id=102848
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
shard-apl total:2838 pass:1748 dwarn:1 dfail:0 fail:24 skip:1064 time:12755s
shard-hsw total:2838 pass:1735 dwarn:1 dfail:0 fail:11 skip:1090 time:12072s
shard-snb total:2838 pass:1331 dwarn:1 dfail:0 fail:10 skip:1496 time:6591s
Blacklisted hosts:
shard-kbl total:2824 pass:1867 dwarn:1 dfail:0 fail:21 skip:934 time:9561s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7827/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: Fix Limited Range Color Handling
2018-01-30 20:13 ` Ville Syrjälä
@ 2018-02-09 13:34 ` Shankar, Uma
2018-02-09 15:38 ` Ville Syrjälä
0 siblings, 1 reply; 23+ messages in thread
From: Shankar, Uma @ 2018-02-09 13:34 UTC (permalink / raw)
To: Ville Syrjälä
Cc: intel-gfx, Syrjala, Ville, Lin, Johnson, Lankhorst, Maarten
>-----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.
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: Fix Limited Range Color Handling
2018-02-09 13:34 ` Shankar, Uma
@ 2018-02-09 15:38 ` Ville Syrjälä
2018-02-13 10:37 ` Shankar, Uma
0 siblings, 1 reply; 23+ messages in thread
From: Ville Syrjälä @ 2018-02-09 15:38 UTC (permalink / raw)
To: Shankar, Uma; +Cc: intel-gfx, Syrjala, Ville, Lin, Johnson, Lankhorst, Maarten
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: Fix Limited Range Color Handling
2018-02-09 15:38 ` Ville Syrjälä
@ 2018-02-13 10:37 ` Shankar, Uma
2018-02-13 14:49 ` Ville Syrjälä
0 siblings, 1 reply; 23+ messages in thread
From: Shankar, Uma @ 2018-02-13 10:37 UTC (permalink / raw)
To: Ville Syrjälä
Cc: intel-gfx, Syrjala, Ville, Lin, Johnson, Lankhorst, Maarten
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Friday, February 9, 2018 9:09 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, 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.
>
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: Fix Limited Range Color Handling
2018-02-13 10:37 ` Shankar, Uma
@ 2018-02-13 14:49 ` Ville Syrjälä
2018-02-15 19:12 ` Shankar, Uma
0 siblings, 1 reply; 23+ messages in thread
From: Ville Syrjälä @ 2018-02-13 14:49 UTC (permalink / raw)
To: Shankar, Uma; +Cc: intel-gfx, Syrjala, Ville, Lin, Johnson, Lankhorst, Maarten
On Tue, Feb 13, 2018 at 10:37:03AM +0000, Shankar, Uma wrote:
>
>
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Friday, February 9, 2018 9:09 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, 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.
> >
>
> 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@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
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: Fix Limited Range Color Handling
2018-02-13 14:49 ` Ville Syrjälä
@ 2018-02-15 19:12 ` Shankar, Uma
0 siblings, 0 replies; 23+ messages in thread
From: Shankar, Uma @ 2018-02-15 19:12 UTC (permalink / raw)
To: Ville Syrjälä
Cc: intel-gfx, Syrjala, Ville, Lin, Johnson, Lankhorst, Maarten
>> >> >> >-----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.
>> >
>>
>> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [v4] drm/i915: Fix Limited Range Color Handling
2018-01-30 15:51 ` [v4] " Uma Shankar
@ 2018-02-22 16:15 ` Ville Syrjälä
0 siblings, 0 replies; 23+ messages in thread
From: Ville Syrjälä @ 2018-02-22 16:15 UTC (permalink / raw)
To: Uma Shankar; +Cc: intel-gfx, ville.syrjala, Johnson Lin, maarten.lankhorst
On Tue, Jan 30, 2018 at 09:21:29PM +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.
>
> v2: Fixed Ville's review comments.
>
> v3: Changed input to const and used correct data types as
> suggested by Ville
>
> v4: Fixed some missing data type corrections.
>
> Signed-off-by: Johnson Lin <johnson.lin@intel.com>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
OK, so maybe we can't get the test done or maybe we can. Either way
I've gone and pushed this to dinq. Thanks for the patch.
> ---
> drivers/gpu/drm/i915/intel_color.c | 29 ++++++++++++++---------------
> 1 file changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 333a23b..0b505d4 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -84,26 +84,25 @@ static bool crtc_state_is_legacy_gamma(struct drm_crtc_state *state)
>
> /*
> * When using limited range, multiply the matrix given by userspace by
> - * the matrix that we would use for the limited range. We do the
> - * multiplication in U2.30 format.
> + * the matrix that we would use for the limited range.
> */
> -static void ctm_mult_by_limited(uint64_t *result, int64_t *input)
> +static void ctm_mult_by_limited(u64 *result, const u64 *input)
> {
> int i;
>
> - for (i = 0; i < 9; i++)
> - result[i] = 0;
> + for (i = 0; i < 9; i++) {
> + u64 user_coeff = input[i];
> + u32 limited_coeff = CTM_COEFF_LIMITED_RANGE;
> + u32 abs_coeff = clamp_val(CTM_COEFF_ABS(user_coeff), 0,
> + CTM_COEFF_4_0 - 1) >> 2;
>
> - for (i = 0; i < 3; i++) {
> - int64_t user_coeff = input[i * 3 + i];
> - 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;
> -
> - result[i * 3 + i] = (limited_coeff * abs_coeff) >> 27;
> - if (CTM_COEFF_NEGATIVE(user_coeff))
> - result[i * 3 + i] |= CTM_COEFF_SIGN;
> + /*
> + * By scaling every co-efficient with limited range (16-235)
> + * vs full range (0-255) the final o/p will be scaled down to
> + * fit in the limited range supported by the panel.
> + */
> + result[i] = mul_u32_u32(limited_coeff, abs_coeff) >> 30;
> + result[i] |= user_coeff & CTM_COEFF_SIGN;
> }
> }
>
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2018-02-22 16:15 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-22 15:04 [PATCH] drm/i915: Fix Limited Range Color Handling Uma Shankar
2017-12-22 15:20 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-12-22 15:54 ` [PATCH] " Ville Syrjälä
2017-12-22 16:12 ` Ville Syrjälä
2018-01-30 15:23 ` Shankar, Uma
2018-01-30 20:13 ` Ville Syrjälä
2018-02-09 13:34 ` Shankar, Uma
2018-02-09 15:38 ` Ville Syrjälä
2018-02-13 10:37 ` Shankar, Uma
2018-02-13 14:49 ` Ville Syrjälä
2018-02-15 19:12 ` Shankar, Uma
2017-12-29 13:58 ` Shankar, Uma
2017-12-29 14:59 ` Uma Shankar
2018-01-29 18:34 ` Ville Syrjälä
2018-01-30 15:19 ` [v3] " Uma Shankar
2018-01-30 15:23 ` Ville Syrjälä
2018-01-30 15:43 ` Shankar, Uma
2018-01-30 15:51 ` [v4] " Uma Shankar
2018-02-22 16:15 ` Ville Syrjälä
2017-12-22 17:00 ` ✗ Fi.CI.IGT: warning for " Patchwork
2018-01-02 10:13 ` ✓ Fi.CI.BAT: success for drm/i915: Fix Limited Range Color Handling (rev2) Patchwork
2018-01-30 18:47 ` ✓ Fi.CI.BAT: success for drm/i915: Fix Limited Range Color Handling (rev4) Patchwork
2018-01-30 22:48 ` ✗ Fi.CI.IGT: failure " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.