On 4/6/2023 4:40 PM, Ville Syrjälä wrote:
On Thu, Apr 06, 2023 at 04:26:48PM +0530, Nautiyal, Ankit K wrote:
Hi Ville,
HDMI1.4b indeed says max value for 16bpc as 60160 (0xeb00)
And black level of 4096.
Got me thinking that we might need to consider bpc for getting the
Coeffs and the offsets.
IIUC for CSC Full range to Limited range:
out = in * gain + offset
Gain :
So for 8 bpc, as you have mentioned
multiplier or gain will be: (235-16) / 255 = 0.8588 ~0.86
offset will be 16, as range is from 16-235
16 bpc
Multiplier: (60160-4096)/65535 = 0.8555 ~0.86
Offset for 16bit: should be 4096
So it seems Multiplier of 0.86 should be alright for different bpc, but
offset would vary.
It's all still in the pipe's internal precision. So any 16 vs. 4096
distinction doesn't exist.
Hmm alright.
Also CSC Postoff programming for the offset doesn’t seem very clear to me.
For CSC BT709 RGB Full range->YCbCr Limited Range, we use offset of {16,
128, 128} for Y, Cb, Cr, and we write 0x800, 0x100, 0x100 for these values.
Y is the middle channel. We write 0x800,0x100,0x800
Ah ok.. so offset of 16 is indeed 0x100, and not 0x800. (facepalm) I
misread and created unnecessary confusion.
Thanks for your patience to clear the confusion.
Changes to coeff and offset seem to be correct now.
Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
But below for Limited range Post offset 16, we seem to be shifting by
(12 - 8) i.e 4. Am I missing something?
Regards,
Ankit
On 3/29/2023 7:19 PM, Ville Syrjala wrote:
From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Our current limited range matrix is a bit off. I think it
was originally calculated with rounding, as if we wanted
the normal pixel replication type of behaviour.
That is, since the 8bpc max value is 0xeb we assumed the
16bpc max value should be 0xebeb, but what the HDMI spec
actually says it should be is 0xeb00.
So to get what we want we make the formula
out = in * (235-16) << (12-8) / in_max + 16 << (12-8),
with 12 being precision of the csc, 8 being the precision
of the constants we used.
The hardware takes its coefficients as floating point
values, but the (235−16)/255 = ~.86, so exponent 0
is what we want anyway, so it works out perfectly without
having to hardcode it in hex or start playing with floats.
In terms of raw numbers we are feeding the hardware the
post offset changes from 0x101 to 0x100, and the coefficient
changes from 0xdc0 to 0xdb0 (~.860->~.855). So this should
make everything come out just a tad darker.
I already used better constants in lut_limited_range() earlier
so the output of the two paths should be closer now.
Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/display/intel_color.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 36aac88143ac..3c3e2f5a5cde 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -116,10 +116,9 @@ struct intel_color_funcs {
#define ILK_CSC_COEFF_FP(coeff, fbits) \
(clamp_val(((coeff) >> (32 - (fbits) - 3)) + 4, 0, 0xfff) & 0xff8)
-#define ILK_CSC_COEFF_LIMITED_RANGE 0x0dc0
#define ILK_CSC_COEFF_1_0 0x7800
-
-#define ILK_CSC_POSTOFF_LIMITED_RANGE (16 * (1 << 12) / 255)
+#define ILK_CSC_COEFF_LIMITED_RANGE ((235 - 16) << (12 - 8)) /* exponent 0 */
+#define ILK_CSC_POSTOFF_LIMITED_RANGE (16 << (12 - 8))
/* Nop pre/post offsets */
static const u16 ilk_csc_off_zero[3] = {};
