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

[Nautiyal, Ankit K]

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 at intel.com>

>
>> 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] = {};