[Intel-gfx] [PATCH 01/12] drm/i915: Fix limited range csc matrix

[Nautiyal, Ankit K]

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.

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.

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 at linux.intel.com>
>
> 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 at linux.intel.com>
> ---
>   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] = {};