[1/4] drm: Fix color LUT rounding

Commit Message

Ville Syrjälä Oct. 13, 2023, 1:13 p.m. UTC

From: Ville Syrjälä <[e-mail]>

The current implementation of drm_color_lut_extract()
generates weird results. Eg. if we go through all the
values for 16->8bpc conversion we see the following pattern:

in            out (count)
   0 -   7f ->  0 (128)
  80 -  17f ->  1 (256)
 180 -  27f ->  2 (256)
 280 -  37f ->  3 (256)
...
fb80 - fc7f -> fc (256)
fc80 - fd7f -> fd (256)
fd80 - fe7f -> fe (256)
fe80 - ffff -> ff (384)

So less values map to 0 and more values map 0xff, which
doesn't seem particularly great.

To get just the same number of input values to map to
the same output values we'd just need to drop the rounding
entrirely. But perhaps a better idea would be to follow the
OpenGL int<->float conversion rules, in which case we get
the following results:

in            out (count)
   0 -   80 ->  0 (129)
  81 -  181 ->  1 (257)
 182 -  282 ->  2 (257)
 283 -  383 ->  3 (257)
...
fc7c - fd7c -> fc (257)
fd7d - fe7d -> fd (257)
fe7e - ff7e -> fe (257)
ff7f - ffff -> ff (129)

Note that since the divisor is constant the compiler
is able to optimize away the integer division in most
cases. The only exception is the _ULL() case on 32bit
architectures since that gets emitted as inline asm
via do_div() and thus the compiler doesn't get to
optimize it.

Signed-off-by: Ville Syrjälä <[e-mail]>
---
 include/drm/drm_color_mgmt.h | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

Comments

Jani Nikula Oct. 31, 2023, 9:15 a.m. UTC | #1

On Fri, 13 Oct 2023, Ville Syrjala <[e-mail]> wrote:
> entrirely. But perhaps a better idea would be to follow the
> OpenGL int<->float conversion rules, in which case we get
> the following results:

Do you have a pointer to the rules handy, I couldn't find it. :(

Might also add the reference to the commit message and/or comment.

BR,
Jani.

Ville Syrjälä Oct. 31, 2023, 4:06 p.m. UTC | #2

On Tue, Oct 31, 2023 at 11:15:35AM +0200, Jani Nikula wrote:
> On Fri, 13 Oct 2023, Ville Syrjala <[e-mail]> wrote:
> > entrirely. But perhaps a better idea would be to follow the
> > OpenGL int<->float conversion rules, in which case we get
> > the following results:
> 
> Do you have a pointer to the rules handy, I couldn't find it. :(

Eg. '2.3.5 Fixed-Point Data Conversions' in GL 4.6 spec. The section
number probably changes depending on which version of the spec you
look at.

> 
> Might also add the reference to the commit message and/or comment.
> 
> BR,
> Jani.
> 
> -- 
> Jani Nikula, Intel

Borah, Chaitanya Kumar Nov. 20, 2023, 1:03 p.m. UTC | #3

Hello Ville,

> -----Original Message-----
> From: dri-devel <[e-mail]> On Behalf Of Ville
> Syrjälä
> Sent: Tuesday, October 31, 2023 9:37 PM
> To: Jani Nikula <[e-mail]>
> Cc: [e-mail]; [e-mail]
> Subject: Re: [Intel-gfx] [PATCH 1/4] drm: Fix color LUT rounding
> 
> On Tue, Oct 31, 2023 at 11:15:35AM +0200, Jani Nikula wrote:
> > On Fri, 13 Oct 2023, Ville Syrjala <[e-mail]> wrote:
> > > entrirely. But perhaps a better idea would be to follow the OpenGL
> > > int<->float conversion rules, in which case we get the following
> > > results:
> >
> > Do you have a pointer to the rules handy, I couldn't find it. :(
> 
> Eg. '2.3.5 Fixed-Point Data Conversions' in GL 4.6 spec. The section number
> probably changes depending on which version of the spec you look at.
> 

This section particularly talks about conversion of normalized fixed point  to floating point numbers and vice versa.
Pardon my limited knowledge on the topic but aren't we just doing a scaling factor conversion(Q0.16 -> Q0.8) in these patches?

I could not draw a direct relation between the formulas in the section[1] and what we are doing here.(but it could be just me!)

Regards

Chaitanya

[1] https://registry.khronos.org/OpenGL/specs/gl/glspec46.core.pdf '2.3.5 Fixed-Point Data Conversions'

> >
> > Might also add the reference to the commit message and/or comment.
> >
> > BR,
> > Jani.
> >
> > --
> > Jani Nikula, Intel
> 
> --
> Ville Syrjälä
> Intel

Borah, Chaitanya Kumar Nov. 20, 2023, 1:17 p.m. UTC | #4

> -----Original Message-----
> From: Borah, Chaitanya Kumar
> Sent: Monday, November 20, 2023 6:33 PM
> To: Ville Syrjälä <[e-mail]>
> Cc: [e-mail]; [e-mail]; Jani
> Nikula <[e-mail]>
> Subject: RE: [Intel-gfx] [PATCH 1/4] drm: Fix color LUT rounding
> 
> Hello Ville,
> 
> > -----Original Message-----
> > From: dri-devel <[e-mail]> On Behalf Of
> > Ville Syrjälä
> > Sent: Tuesday, October 31, 2023 9:37 PM
> > To: Jani Nikula <[e-mail]>
> > Cc: [e-mail]; [e-mail]
> > Subject: Re: [Intel-gfx] [PATCH 1/4] drm: Fix color LUT rounding
> >
> > On Tue, Oct 31, 2023 at 11:15:35AM +0200, Jani Nikula wrote:
> > > On Fri, 13 Oct 2023, Ville Syrjala <[e-mail]> wrote:
> > > > entrirely. But perhaps a better idea would be to follow the OpenGL
> > > > int<->float conversion rules, in which case we get the following
> > > > results:
> > >
> > > Do you have a pointer to the rules handy, I couldn't find it. :(
> >
> > Eg. '2.3.5 Fixed-Point Data Conversions' in GL 4.6 spec. The section
> > number probably changes depending on which version of the spec you look
> at.
> >
> 
> This section particularly talks about conversion of normalized fixed point  to
> floating point numbers and vice versa.
> Pardon my limited knowledge on the topic but aren't we just doing a scaling
> factor conversion(Q0.16 -> Q0.8) in these patches?
> 
> I could not draw a direct relation between the formulas in the section[1] and
> what we are doing here.(but it could be just me!)

Scratch that! As I understand, in effect we are doing a Q0.16 Fixed Point -> Floating point -> Q0.8 Fixed Point conversion.
Correct me if I am wrong! Otherwise

LGTM.

Reviewed-by: Chaitanya Kumar Borah <[e-mail]>


> 
> Regards
> 
> Chaitanya
> 
> [1] https://registry.khronos.org/OpenGL/specs/gl/glspec46.core.pdf '2.3.5
> Fixed-Point Data Conversions'
> 
> > >
> > > Might also add the reference to the commit message and/or comment.
> > >
> > > BR,
> > > Jani.
> > >
> > > --
> > > Jani Nikula, Intel
> >
> > --
> > Ville Syrjälä
> > Intel

Ville Syrjälä Nov. 20, 2023, 2:27 p.m. UTC | #5

On Mon, Nov 20, 2023 at 01:17:05PM +0000, Borah, Chaitanya Kumar wrote:
> 
> 
> > -----Original Message-----
> > From: Borah, Chaitanya Kumar
> > Sent: Monday, November 20, 2023 6:33 PM
> > To: Ville Syrjälä <[e-mail]>
> > Cc: [e-mail]; [e-mail]; Jani
> > Nikula <[e-mail]>
> > Subject: RE: [Intel-gfx] [PATCH 1/4] drm: Fix color LUT rounding
> > 
> > Hello Ville,
> > 
> > > -----Original Message-----
> > > From: dri-devel <[e-mail]> On Behalf Of
> > > Ville Syrjälä
> > > Sent: Tuesday, October 31, 2023 9:37 PM
> > > To: Jani Nikula <[e-mail]>
> > > Cc: [e-mail]; [e-mail]
> > > Subject: Re: [Intel-gfx] [PATCH 1/4] drm: Fix color LUT rounding
> > >
> > > On Tue, Oct 31, 2023 at 11:15:35AM +0200, Jani Nikula wrote:
> > > > On Fri, 13 Oct 2023, Ville Syrjala <[e-mail]> wrote:
> > > > > entrirely. But perhaps a better idea would be to follow the OpenGL
> > > > > int<->float conversion rules, in which case we get the following
> > > > > results:
> > > >
> > > > Do you have a pointer to the rules handy, I couldn't find it. :(
> > >
> > > Eg. '2.3.5 Fixed-Point Data Conversions' in GL 4.6 spec. The section
> > > number probably changes depending on which version of the spec you look
> > at.
> > >
> > 
> > This section particularly talks about conversion of normalized fixed point  to
> > floating point numbers and vice versa.
> > Pardon my limited knowledge on the topic but aren't we just doing a scaling
> > factor conversion(Q0.16 -> Q0.8) in these patches?
> > 
> > I could not draw a direct relation between the formulas in the section[1] and
> > what we are doing here.(but it could be just me!)
> 
> Scratch that! As I understand, in effect we are doing a Q0.16 Fixed Point -> Floating point -> Q0.8 Fixed Point conversion.

Yep, that's it.

> Correct me if I am wrong! Otherwise
> 
> LGTM.
> 
> Reviewed-by: Chaitanya Kumar Borah <[e-mail]>
> 
> 
> > 
> > Regards
> > 
> > Chaitanya
> > 
> > [1] https://registry.khronos.org/OpenGL/specs/gl/glspec46.core.pdf '2.3.5
> > Fixed-Point Data Conversions'
> > 
> > > >
> > > > Might also add the reference to the commit message and/or comment.
> > > >
> > > > BR,
> > > > Jani.
> > > >
> > > > --
> > > > Jani Nikula, Intel
> > >
> > > --
> > > Ville Syrjälä
> > > Intel

Patch

diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index 81c298488b0c..6be3cbe18944 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -36,20 +36,16 @@  struct drm_plane;
  *
  * Extract a degamma/gamma LUT value provided by user (in the form of
  * &drm_color_lut entries) and round it to the precision supported by the
- * hardware.
+ * hardware, following OpenGL int<->float conversion rules.
  */
 static inline u32 drm_color_lut_extract(u32 user_input, int bit_precision)
 {
-	u32 val = user_input;
-	u32 max = 0xffff >> (16 - bit_precision);
-
-	/* Round only if we're not using full precision. */
-	if (bit_precision < 16) {
-		val += 1UL << (16 - bit_precision - 1);
-		val >>= 16 - bit_precision;
-	}
-
-	return clamp_val(val, 0, max);
+	if (bit_precision > 16)
+		return DIV_ROUND_CLOSEST_ULL(mul_u32_u32(user_input, (1 << bit_precision) - 1),
+					     (1 << 16) - 1);
+	else
+		return DIV_ROUND_CLOSEST(user_input * ((1 << bit_precision) - 1),
+					 (1 << 16) - 1);
 }
 
 u64 drm_color_ctm_s31_32_to_qm_n(u64 user_input, u32 m, u32 n);