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

[Ville Syrjälä]

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.

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

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

-- 
Ville Syrjälä
Intel