Re: Steam Deck integrated display: saturation boosting or reduction?
On Sat, 4 Nov 2023 13:11:02 +
Joshua Ashton wrote:
> Hello,
Hi Joshua,
thanks for replying. The reason I started this discussion is that I
would like us to come to the same page on terminology and what the math
actually means. I cannot and do not want to claim anything wrong in
your math or implementation, but I would like to describe it from a
certain point of view.
> The existing behaviour before any of our colour work was that the native
> display's primaries were being used for SDR content. (Ie. just scanning
> out game's buffer directly)
Ok. That means it was using an implicit conversion from whatever the
games were emitting into the display's native primaries. Using an
(implicit) identity CTM unavoidably changes colorimetry when the source
and destination color spaces differ.
If games were graded to produce BT.2020 encoding, this would amount to
extreme saturation reduction. If games were graded to produce sRGB
(perhaps implicitly, like almost all SDR apps do), this would result in
some saturation reduction.
When I say "graded", I mean something that can also happen implicitly
by a developer thinking: "it looks good on my monitor, and I'd like
everyone to see it like I do". The developer may not even think about
it. We have no knowledge of what the content was graded for, but it
still happened.
> Games are not submitting us any primaries for the buffers they are sending.
> I mean they are saying they are sRGB so "technically 709", but
> colorimetry for SDR content (outside of mastering) is very wishy-washy.
Right.
> Deck Display Info:
> static constexpr displaycolorimetry_t displaycolorimetry_steamdeck_spec
> {
> .primaries = { { 0.602f, 0.355f }, { 0.340f, 0.574f }, { 0.164f, 0.121f
> } },
> .white = { 0.3070f, 0.3220f }, // not D65
> };
>
> static constexpr displaycolorimetry_t displaycolorimetry_steamdeck_measured
> {
> .primaries = { { 0.603f, 0.349f }, { 0.335f, 0.571f }, { 0.163f, 0.115f
> } },
> .white = { 0.296f, 0.307f }, // not D65
> };
>
> https://github.com/ValveSoftware/gamescope/blob/master/src/color_helpers.h#L451
>
> For the rest of this, consider displaycolorimetry_steamdeck_measured to
> be what we use for the internal display.
>
> To improve the rendering of content on the Deck's internal display with
> the modest gamut, we go from the display's native primaries (sub 709) to
> somewhere between the native primaries (0.0) and a hypothetical wider
> gamut display (1.0) that we made up.
What do you mean by "we go from ... to ..."? What does "go" stand for
here?
Do I understand right, that you choose the target primaries from
somewhere between the display's native primaries and the below
hypothetical display's wider gamut primaries, based on end user
vibrance setting?
It's really curious if it indeed is *target* primaries for your color
transformation, given that you are still driving the internal display.
> The hypothetical display's primaries were decided based by making
> content look appealing:
> static constexpr displaycolorimetry_t displaycolorimetry_widegamutgeneric
> {
> .primaries = { { 0.6825f, 0.3165f }, { 0.241f, 0.719f }, { 0.138f,
> 0.050f } },
> .white = { 0.3127f, 0.3290f }, // D65
> };
>
> We have a single knob for this in the UI, in code it's "SDR Gamut
> Wideness", but known in the UI as "Color Vibrance". It's the knob that
> picks the target color gamut that gets mapped to the native display.
>
> This is how that single value interacts to pick the target primaries:
>
> https://github.com/ValveSoftware/gamescope/blob/master/src/color_helpers.cpp#L798
>
> We then use the result there to do a simple saturation fit based on the
> kob and some additional parameters that control how we interpolate.
> (blendEnableMinSat, blendEnableMaxSat, blendAmountMin, blendAmountMax)
>
> Those parameters also change with the SDR Gamut Wideness value, based on
> things that "look nice". :P
>
> https://github.com/ValveSoftware/gamescope/blob/master/src/color_helpers.cpp#L769
Alright, that's another part of the color transformation you do, the
one with the most "perceptual intent" in it, I'd say.
> We also do some other things like Bradford chromatic adaptation to fix
> the slightly-off whitepoint too.
>
> We use all this to generate a 3D LUT with that saturation fit, chromatic
> adaptation and use Shaper + 3D LUT at scanout time to apply it.
> (We also have a shader based fallback path)
>
> The goal of all of this work is less 'color accuracy' and more 'making
> the display more inline with consumer expectations'.
Yes, that is called "perceptual rendering intent" and it is 'color
accurate' in that context, assuming your goal is to give the user an as
close experience as possible compared to a highly capable display or to
the game developer's (mastering) display. You might not know the facts
of what it should look like, but you do your best anyway.
> We wanted to try and make the display appear much
Re: [RFC PATCH 01/10] drm/doc/rfc: Describe why prescriptive color pipeline is needed
On 2023-10-20 06:36, Pekka Paalanen wrote: > On Thu, 19 Oct 2023 10:56:40 -0400 > Harry Wentland wrote: > >> On 2023-10-10 12:13, Melissa Wen wrote: >>> O 09/08, Harry Wentland wrote: Signed-off-by: Harry Wentland > > ... > >>> Also, with this new plane API in place, I understand that we will >>> already need think on how to deal with the mixing between old drm color >>> properties (color encoding and color range) and these new way of setting >>> plane color properties. IIUC, Pekka asked a related question about it >>> when talking about CRTC automatic RGB->YUV (?) >>> >> >> We'll still need to confirm whether we'll want to deprecate these >> existing properties. If we do that we'd want a client prop. Things >> should still work without deprecating them, if drivers just pick up >> after the initial encoding and range CSC. >> >> But realistically it might be better to deprecate them and turn them >> into explicit colorops. > > The existing properties would need to be explicitly reflected in the > new pipelines anyway, otherwise there would always be doubt at which > point of a pipeline the old properties apply, and they might even > need to change positions between pipelines. > > I think it is simply easier to just hide all old color related > properties when userspace sets the client-cap to enable pipelines. The > problem is to make sure to hide all old properties on all drivers that > support the client-cap. > > As a pipeline must be complete (describe everything that happens to > pixel values), it's going to be a flag day per driver. > > Btw. the plane FB YUV->RGB conversion needs a colorop in every pipeline > as well. Maybe it's purely informative and non-configurable, keyed by > FB pixel format, but still. > > We also need a colorop to represent sample filtering, e.g. bilinear, > like I think Sebastian may have mentioned in the past. Everything > before the sample filter happens "per tap" as Joshua Ashton put it, and > everything after it happens on the sample that was computed as a > weighted average of the filter tap inputs (texels). > > There could be colorops other than sample filtering that operate on > more than one sample at a time, like blur or sharpness. There could > even be colorops that change the image size like adding padding that > the following colorop hardware requires, and then yet another colorop > that clips that padding away. For an example, see > https://lists.freedesktop.org/archives/dri-devel/2023-October/427015.html > > If that padding and its color can affect the pipeline results of the > pixels near the padding (e.g. some convolution is applied with them, > which may be the reason why padding is necessary to begin with), then > it would be best to model it. > I hear you but I'm also somewhat shying away from defining this at this point. There are already too many things that need to happen and I will focus on the actual color blocks (LUTs, matrices) first. We'll always be able to add a new (read-only) colorop type to define scaling and tap behavior at any point and a client is free to ignore a color pipeline if it doesn't find any tap/scale info in it. Harry > > Thanks, > pq
Re: [RFC PATCH 01/10] drm/doc/rfc: Describe why prescriptive color pipeline is needed
On 2023-10-20 06:17, Pekka Paalanen wrote:
> On Thu, 19 Oct 2023 10:56:29 -0400
> Harry Wentland wrote:
>
>> On 2023-09-13 07:29, Pekka Paalanen wrote:
>>> On Fri, 8 Sep 2023 11:02:26 -0400
>>> Harry Wentland wrote:
>>>
Signed-off-by: Harry Wentland
>
> ...
>
+COLOR_PIPELINE Plane Property
+=
+
+Because we don't have existing KMS color properties in the pre-blending
+portion of display pipelines (i.e. on drm_planes) we are introducing
+color pipelines here first. Eventually we'll want to use the same
+concept for the post-blending portion, i.e. drm_crtcs.
>>>
>>> This paragraph might fit better in a cover letter.
>>>
+
+Color Pipelines are created by a driver and advertised via a new
+COLOR_PIPELINE enum property on each plane. Values of the property
+always include '0', which is the default and means all color processing
+is disabled. Additional values will be the object IDs of the first
+drm_colorop in a pipeline. A driver can create and advertise none, one,
+or more possible color pipelines. A DRM client will select a color
+pipeline by setting the COLOR PIPELINE to the respective value.
+
+In the case where drivers have custom support for pre-blending color
+processing those drivers shall reject atomic commits that are trying to
+set both the custom color properties, as well as the COLOR_PIPELINE
>>>
>>> s/set/use/ because one of them could be carried-over state from
>>> previous commits while not literally set in this one.
>>>
+property.
+
+An example of a COLOR_PIPELINE property on a plane might look like this::
+
+Plane 10
+├─ "type": immutable enum {Overlay, Primary, Cursor} = Primary
+├─ …
+└─ "color_pipeline": enum {0, 42, 52} = 0
>>>
>>> Enum values are string names. I presume the intention here is that the
>>> strings will never need to be parsed, and the uint64_t is always equal
>>> to the string representation, right?
>>>
>>> That needs a statement here. It differs from all previous uses of
>>> enums, and e.g. requires a little bit of new API in libweston's
>>> DRM-backend to handle since it has its own enums referring to the
>>> string names that get mapped to the uint64_t per owning KMS object.
>>>
>>
>> I'm currently putting the DRM object ID in the "value" and use the
>> "name" as a descriptive name.
>
> Would that string name be UAPI? I mean, if userspace hardcodes and
> looks for that name, will that keep working? If it's descriptive then I
> would assume not, but for every enum existing so far the string name is
> UAPI.
>
Yes, it's UAPI, as that's how userspace will set the property. The value
is still important to be able to find out which is the first colorop in
the pipeline.
Harry
>>> struct drm_mode_property_enum {
>>> __u64 value;
>>> char name[DRM_PROP_NAME_LEN];
>>> };
>>
>> This works well in IGT and gives us a nice descriptive name for
>> debugging, but I could consider changing this if it'd simplify
>> people's lives.
>
> It's nice if we can have a description string for each pipeline, but
> according to KMS UAPI conventions so far, userspace would look for the
> string name. I'm worried that could backfire to the kernel.
>
> Or, maybe we do want to make the string UAPI and accept that some
> userspace might look for it rather than an arrangement of colorops?
>
> Matching colorop sequences is "hard". A developer looking at pipelines,
> picking one, and hardcoding its name as a preferred choice would be too
> easy. "Works for my cards." IOW, if there is a useful looking string
> name, we can be sure that someone will depend on it.
>
>
> Thanks,
> pq
>
+References
+==
+
+1.
https://lore.kernel.org/dri-devel/QMers3awXvNCQlyhWdTtsPwkp5ie9bze_hD5nAccFW7a_RXlWjYB7MoUW_8CKLT2bSQwIXVi5H6VULYIxCdgvryZoAoJnC5lZgyK1QWn488=@emersion.fr/
\ No newline at end of file
>>>
>>
>
[ANNOUNCE] weston 12.0.93
Hi all, This is the RC1 release for Weston 13.0.0. Full commit history below. Marius Vlad (1): build: bump to version 12.0.93 for the RC1 release git tag: 12.0.93 https://gitlab.freedesktop.org/wayland/weston/-/releases/12.0.93/downloads/weston-12.0.93.tar.xz SHA256: 7a873b477dc4c6a6f46ee1e0b594f8132af530f7ec9b1d02614c5337e8d773c3 weston-12.0.93.tar.xz SHA512: 0fb5f447270605974a1de09ff8014e8647c6f54a7403f2c62644dd93bcc1c593892d0c2cc495a03b5e43dda472c463e141ff8f9535e1d1a92c3285cdd2cbfb93 weston-12.0.93.tar.xz PGP: https://gitlab.freedesktop.org/wayland/weston/-/releases/12.0.93/downloads/weston-12.0.93.tar.xz.sig signature.asc Description: PGP signature
Re: [RFC PATCH v2 05/17] drm/vkms: Avoid reading beyond LUT array
On 2023-10-30 09:29, Pekka Paalanen wrote:
> On Thu, 19 Oct 2023 17:21:21 -0400
> Harry Wentland wrote:
>
>> When the floor LUT index (drm_fixp2int(lut_index) is the last
>> index of the array the ceil LUT index will point to an entry
>> beyond the array. Make sure we guard against it and use the
>> value of the floot LUT index.
>>
>> Blurb about LUT creation and how first element should be 0x0 and
>> last one 0x.
>>
>> Hold on, is that even correct? What should the ends of a LUT be?
>> How does UNORM work and how does it apply to LUTs?
>
> Do you mean how should UNORM input value map to LUT entries for LUT
> indexing?
>
> I suppose UNORM 16-bit converts to nominal real values as:
> - 0x0: 0.0
> - 0x: 1.0
>
> And in LUT, you want 0.0 to map to the first LUT element exactly, and
> 1.0 to map to the last LUT element exactly, even if whatever
> interpolation may be in use, right?
>
> If so, it is important to make sure that, assuming linear interpolation
> for instance, there is no "dead zone" at either end. Given high
> interpolation precision, any step away from 0.0 or 1.0 needs to imply a
> change in the real-valued output, assuming e.g. identity LUT.
>
> If LUT has N elements, and 16-bit UNORM input value is I, then (in
> naive real-valued math, so no implicit truncation between operations)
>
> x = I / 0x * (N - 1)
> ia = floor(x)
> ib = min(ia + 1, N - 1)
>
> f = x - floor(x)
> y = (1 - f) * LUT[ia] + f * LUT[ib]
>
>
> Does that help?
>
Thanks. Yes, this is what the code is doing (with this commit).
The commit description was an oversight and only reflect my initial
thoughts when coding it, before I made sure this is the right way
to go about it. I'll update it.
Harry
> In my mind, I'm thinking of a uniformly distributed LUT as a 1-D
> texture, because that's how I have implemented them in GL. There you
> have to be careful so that input values 0.0 and 1.0 map to the *center*
> of the first and last texel, and not to the edges of the texture like
> texture coordinates do. Then you can use the GL linear texture
> interpolation as-is.
>
>
> Thanks,
> pq
>
>
>> Signed-off-by: Harry Wentland
>> Cc: Ville Syrjala
>> Cc: Pekka Paalanen
>> Cc: Simon Ser
>> Cc: Harry Wentland
>> Cc: Melissa Wen
>> Cc: Jonas Ådahl
>> Cc: Sebastian Wick
>> Cc: Shashank Sharma
>> Cc: Alexander Goins
>> Cc: Joshua Ashton
>> Cc: Michel Dänzer
>> Cc: Aleix Pol
>> Cc: Xaver Hugl
>> Cc: Victoria Brekenfeld
>> Cc: Sima
>> Cc: Uma Shankar
>> Cc: Naseer Ahmed
>> Cc: Christopher Braga
>> Cc: Abhinav Kumar
>> Cc: Arthur Grillo
>> Cc: Hector Martin
>> Cc: Liviu Dudau
>> Cc: Sasha McIntosh
>> ---
>> drivers/gpu/drm/vkms/vkms_composer.c | 14 ++
>> 1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
>> b/drivers/gpu/drm/vkms/vkms_composer.c
>> index a0a3a6fd2926..cf1dff162920 100644
>> --- a/drivers/gpu/drm/vkms/vkms_composer.c
>> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
>> @@ -123,6 +123,8 @@ static u16 apply_lut_to_channel_value(const struct
>> vkms_color_lut *lut, u16 chan
>>enum lut_channel channel)
>> {
>> s64 lut_index = get_lut_index(lut, channel_value);
>> +u16 *floor_lut_value, *ceil_lut_value;
>> +u16 floor_channel_value, ceil_channel_value;
>>
>> /*
>> * This checks if `struct drm_color_lut` has any gap added by the
>> compiler
>> @@ -130,11 +132,15 @@ static u16 apply_lut_to_channel_value(const struct
>> vkms_color_lut *lut, u16 chan
>> */
>> static_assert(sizeof(struct drm_color_lut) == sizeof(__u16) * 4);
>>
>> -u16 *floor_lut_value = (__u16 *)&lut->base[drm_fixp2int(lut_index)];
>> -u16 *ceil_lut_value = (__u16 *)&lut->base[drm_fixp2int_ceil(lut_index)];
>> +floor_lut_value = (__u16 *)&lut->base[drm_fixp2int(lut_index)];
>> +if (drm_fixp2int(lut_index) == (lut->lut_length - 1))
>> +/* We're at the end of the LUT array, use same value for ceil
>> and floor */
>> +ceil_lut_value = floor_lut_value;
>> +else
>> +ceil_lut_value = (__u16
>> *)&lut->base[drm_fixp2int_ceil(lut_index)];
>>
>> -u16 floor_channel_value = floor_lut_value[channel];
>> -u16 ceil_channel_value = ceil_lut_value[channel];
>> +floor_channel_value = floor_lut_value[channel];
>> +ceil_channel_value = ceil_lut_value[channel];
>>
>> return lerp_u16(floor_channel_value, ceil_channel_value,
>> lut_index & DRM_FIXED_DECIMAL_MASK);
>
