Re: [PATCH V11 06/47] drm/colorop: Add 1D Curve subtype

[Pekka Paalanen <pekka.paalanen@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 5433 bytes --]

On Tue, 23 Sep 2025 11:41:24 -0600
Alex Hung <alex.hung@amd.com> wrote:

> On 9/23/25 10:16, Alex Hung wrote:
> > 
> > 
> > On 9/23/25 01:59, Pekka Paalanen wrote:  
> >> On Mon, 22 Sep 2025 21:16:45 -0600
> >> Alex Hung <alex.hung@amd.com> wrote:
> >>  
> >>> On 9/18/25 02:40, Pekka Paalanen wrote:  

...

> >> The problem is that "H.273 TransferCharacteristics code point 13" a.k.a
> >> the sRGB curve means different things for different people (two-piece
> >> vs. power-2.2).
> >>
> >> The difference is minor but visible, and therefore I would not make
> >> two-piece and power-2.2 equivalent nor have one approximated by the
> >> other.
> >>
> >> They both need their own entries in the enum. Let's leave any decision
> >> about whether substituting one for the other is ok to the userspace.
> >>  
> >>>            */
> >>>           DRM_COLOROP_1D_CURVE_SRGB_EOTF,
> >>>
> >>>
> >>> It is also possible to add GAMMA 2.2 in addition to sRGB piece-wise
> >>> EOTF. But if I understand correctly, DRM_COLOROP_1D_CURVE_SRGB_EOTF may
> >>> not be used at all, right?  
> >>
> >> If hardware implements the two-piece curve, then there is reason to
> >> expose it, especially when it does not implement power-2.2. Userspace
> >> can choose to use it as an approximation when that is appropriate.
> >>
> >>
> >> Thanks,
> >> pq
> >>  
> > 
> > Does the following diff make sense?

Yes.


> > 
> > 1. Change "sRGB EOTF" -> "Piece-wise EOTF"
> > 2. Add "Gamma 2.2"
> > 
> > diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> > index e1b2b446faf2..823e39b8f3fe 100644
> > --- a/drivers/gpu/drm/drm_colorop.c
> > +++ b/drivers/gpu/drm/drm_colorop.c
> > @@ -71,12 +71,13 @@ static const struct drm_prop_enum_list 
> > drm_colorop_type_enum_list[] = {
> >   };
> > 
> >   static const char * const colorop_curve_1d_type_names[] = {
> > -    [DRM_COLOROP_1D_CURVE_SRGB_EOTF] = "sRGB EOTF",
> > +    [DRM_COLOROP_1D_CURVE_SRGB_EOTF] = "Piece-wise EOTF",
> >       [DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF] = "sRGB Inverse EOTF",
> >       [DRM_COLOROP_1D_CURVE_PQ_125_EOTF] = "PQ 125 EOTF",
> >       [DRM_COLOROP_1D_CURVE_PQ_125_INV_EOTF] = "PQ 125 Inverse EOTF",
> >       [DRM_COLOROP_1D_CURVE_BT2020_INV_OETF] = "BT.2020 Inverse OETF",
> >       [DRM_COLOROP_1D_CURVE_BT2020_OETF] = "BT.2020 OETF",
> > +    [DRM_COLOROP_1D_CURVE_GAMMA22] = "Gamma 2.2",

If I wanted to really nitpick, I'd propose "Power 2.2" instead of
"Gamma 2.2", but I suppose it's clear anyway. And Wayland already went
with GAMMA22.

> >   };
> > 
> >   static const struct drm_prop_enum_list 
> > drm_colorop_lut1d_interpolation_list[] = {
> > diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
> > index 3e70f66940e0..3428a27cd9ad 100644
> > --- a/include/drm/drm_colorop.h
> > +++ b/include/drm/drm_colorop.hsRGB EOTF
> > @@ -43,12 +43,9 @@ enum drm_colorop_curve_1d_type {
> >       /**
> >        * @DRM_COLOROP_1D_CURVE_SRGB_EOTF:
> >        *
> > -     * enum string "sRGB EOTF"
> > +     * enum string "Piece-wise EOTF"
> >        *
> > -     * sRGB piece-wise electro-optical transfer function. Transfer
> > -     * characteristics as defined by IEC 61966-2-1 sRGB. Equivalent
> > -     * to H.273 TransferCharacteristics code point 13 with
> > -     * MatrixCoefficients set to 0.
> > +     * sRGB piece-wise electro-optical transfer function.
> >        */
> >       DRM_COLOROP_1D_CURVE_SRGB_EOTF,
> > 
> > @@ -108,6 +105,16 @@ enum drm_colorop_curve_1d_type {
> >        */
> >       DRM_COLOROP_1D_CURVE_BT2020_OETF,
> > 
> > +    /**
> > +     * @DRM_COLOROP_1D_CURVE_GAMMA22:
> > +     *
> > +     * enum string "Gamma 2.2"
> > +     *
> > +     * A gamma 2.2 power function. This applies a power curve with
> > +     * gamma value of 2.2 to the input values.

I'd prefer "exponent" rather than "gamma value" to be more explicit.
Just in case. There is quite some confusion around the term "gamma".
I've used to call this function a "pure power-low with exponent 2.2" to
explain that there are no offsets or multipliers or anything else.

For documentation it would best to write down the mathematical formula
rather than describe it in words. I understand that may be problematic
in kerneldoc, and we didn't do it in Wayland XML either but in a
proposed Gitlab-Markdown appendix.


> > +     */
> > +    DRM_COLOROP_1D_CURVE_GAMMA22,
> > +
> >       /**
> >        * @DRM_COLOROP_1D_CURVE_COUNT:
> >        *
> >   
> 
> Both DRM_COLOROP_1D_CURVE_SRGB_EOTF and DRM_COLOROP_1D_CURVE_GAMMA22 are 
> defined and it should be clear that sRGB EOTF are piece-wise TF and 
> Gamma 2.2 is for power 2.2. Is it still a concern of using "sRGB" for as 
> the original patch?

Not enough of a concern for me to continue nagging about it. :-)

> More precisely, adding DRM_COLOROP_1D_CURVE_GAMMA22 with "Gamma 2.2" 
> string without touching "sRGB EOTF" should be sufficient. If a userspace 
> need to choose one or another it can precisely do so.
> 

As long as everyone reads the documentation, and the documentation is
clear. I failed at clarity in Wayland, so I'm perhaps a bit paranoid
here.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]