Re: [PATCH v4 00/32] drm/amd/display: add AMD driver-specific properties for color mgmt

2023-11-06 Thread Melissa Wen
On 10/06, Sebastian Wick wrote:
> On Thu, Oct 05, 2023 at 04:14:55PM -0100, Melissa Wen wrote:
> > Hello,
> > 
> > Just another iteration for AMD driver-specific color properties.
> > Basically, addressing comments from the previous version.
> > 
> > Recap: this series extends the current KMS color management API with AMD
> > driver-specific properties to enhance the color management support on
> > AMD Steam Deck. The key additions to the color pipeline include:
> 
> Did you talk with the maintainers about this already? The last few times
> driver specific properties, and even kind of generic plane properties
> with a fixed position in the pipeline were proposed they were all
> NAKed. Just putting them behind a define doesn't sound great and I don't
> think there is any precedence for allowing this in. This is basically
> just more burden for upstream without any benefits for upstream.

Hi Sebastian,

Sorry for delaying. I believe it was already answered during the XDC KMS
color workshop, but: yes, I've talked with them and they are fine with
the cflags approach. Remember that we are not using here kconfig anymore.
The driver-specific properties (creation and attachment) are guarded by
cflags, not a kernel building option. We changed to cflags after Harry's
suggestion in the first version.

I think the main point for upstream it now is that we can actually
exercise the plane color properties and all machinery around new color
elements (as 3D LUT) with a userspace that cares about it (Gamescope).
Worth mentioning that I removed CRTC shaper/3D LUT support from previous
version, because, even with the internal implementation, we don't have
how to exercise these CRTC properties right now.

Finally, the main goal is ofc attaching it to the generic API from
Harry's series. We are all in the same page about it.

BR,

Melissa

> 
> Maybe you can separate the uAPI changes from the internal improvements
> to land at least parts of this faster.
> 
> > - plane degamma LUT and pre-defined TF;
> > - plane HDR multiplier;
> > - plane CTM 3x4;
> > - plane shaper LUT and pre-defined TF;
> > - plane 3D LUT;
> > - plane blend LUT and pre-defined TF;
> > - CRTC gamma pre-defined TF;
> > 
> > You can find the AMD HW color capabilities documented here:
> > https://dri.freedesktop.org/docs/drm/gpu/amdgpu/display/display-manager.html#color-management-properties
> > 
> > The userspace case is Gamescope[1], the compositor for SteamOS.
> > Gamescope has already adopted AMD driver-specific properties to
> > implement comprehensive color management support, including gamut
> > mapping, HDR rendering, SDR on HDR, HDR on SDR. Using these features in
> > the SteamOS 3.5[2] users can expect a significantly enhanced visual
> > experience. 
> > 
> > You can find a brief overview of the Steam Deck color pipeline here:
> > https://github.com/ValveSoftware/gamescope/blob/master/src/docs/Steam%20Deck%20Display%20Pipeline.png
> > 
> > Changes from:
> > 
> > [RFC] 
> > https://lore.kernel.org/dri-devel/20230423141051.702990-1-m...@igalia.com
> > - Remove KConfig and guard properties with `AMD_PRIVATE_COLOR`;
> > - Remove properties for post-blending/CRTC shaper TF+LUT and 3D LUT;
> > - Use color caps to improve the support of pre-defined curve;
> > 
> > [v1] 
> > https://lore.kernel.org/dri-devel/20230523221520.3115570-1-m...@igalia.com
> > - Replace DRM_ by AMDGPU_ prefix for transfer function (TF) enum; 
> > - Explicitly define EOTFs and inverse EOTFs and set props accordingly;
> > - Document pre-defined transfer functions;
> > - Remove HLG transfer function from supported TFs;
> > - Remove misleading comments;
> > - Remove post-blending shaper TF+LUT and 3D LUT support;
> > - Move driver-specific property operations from amdgpu_display.c to
> >   amdgpu_dm_color.c;
> > - Reset planes if any color props change;
> > - Add plane CTM 3x4 support;
> > - Removed two DC fixes already applied upstream;
> > 
> > [v2] 
> > https://lore.kernel.org/dri-devel/20230810160314.48225-1-m...@igalia.com
> > - Many documentation fixes: BT.709 OETF, description of sRGB and pure
> >   power functions, TF+1D LUT behavior;
> > - Rename CTM2 to CTM 3x4 and fix misleading comment about DC gamut remap;
> > - Squash `Linear` and `Unity` TF in `Identity`;
> > - Remove the `MPC gamut remap` patch already applied upstream[3];
> > - Remove outdated delta segmentation fix;
> > - Nits/small fixes;
> > 
> > [v3] 
> > https://lore.kernel.org/amd-gfx/20230925194932.1329483-1-m...@igalia.com
> > - Add table to describe value range in linear and non-linear forms
> > - Comment the PQ TF need after HDR multiplier
> > - Advertise the 3D LUT size as the size of a single-dimension (read-only)
> > - remove function to check expected size from 3DLUT caps
> > - cleanup comments
> > 
> > It's worth noting that driver-specific properties are guarded by
> > `AMD_PRIVATE_COLOR`. So, finally, this is the color management API when
> > driver-specific properties are enabled:
> > 
> > 

Re: [PATCH v4 00/32] drm/amd/display: add AMD driver-specific properties for color mgmt

2023-10-06 Thread Sebastian Wick
On Thu, Oct 05, 2023 at 04:14:55PM -0100, Melissa Wen wrote:
> Hello,
> 
> Just another iteration for AMD driver-specific color properties.
> Basically, addressing comments from the previous version.
> 
> Recap: this series extends the current KMS color management API with AMD
> driver-specific properties to enhance the color management support on
> AMD Steam Deck. The key additions to the color pipeline include:

Did you talk with the maintainers about this already? The last few times
driver specific properties, and even kind of generic plane properties
with a fixed position in the pipeline were proposed they were all
NAKed. Just putting them behind a define doesn't sound great and I don't
think there is any precedence for allowing this in. This is basically
just more burden for upstream without any benefits for upstream.

Maybe you can separate the uAPI changes from the internal improvements
to land at least parts of this faster.

> - plane degamma LUT and pre-defined TF;
> - plane HDR multiplier;
> - plane CTM 3x4;
> - plane shaper LUT and pre-defined TF;
> - plane 3D LUT;
> - plane blend LUT and pre-defined TF;
> - CRTC gamma pre-defined TF;
> 
> You can find the AMD HW color capabilities documented here:
> https://dri.freedesktop.org/docs/drm/gpu/amdgpu/display/display-manager.html#color-management-properties
> 
> The userspace case is Gamescope[1], the compositor for SteamOS.
> Gamescope has already adopted AMD driver-specific properties to
> implement comprehensive color management support, including gamut
> mapping, HDR rendering, SDR on HDR, HDR on SDR. Using these features in
> the SteamOS 3.5[2] users can expect a significantly enhanced visual
> experience. 
> 
> You can find a brief overview of the Steam Deck color pipeline here:
> https://github.com/ValveSoftware/gamescope/blob/master/src/docs/Steam%20Deck%20Display%20Pipeline.png
> 
> Changes from:
> 
> [RFC] 
> https://lore.kernel.org/dri-devel/20230423141051.702990-1-m...@igalia.com
> - Remove KConfig and guard properties with `AMD_PRIVATE_COLOR`;
> - Remove properties for post-blending/CRTC shaper TF+LUT and 3D LUT;
> - Use color caps to improve the support of pre-defined curve;
> 
> [v1] 
> https://lore.kernel.org/dri-devel/20230523221520.3115570-1-m...@igalia.com
> - Replace DRM_ by AMDGPU_ prefix for transfer function (TF) enum; 
> - Explicitly define EOTFs and inverse EOTFs and set props accordingly;
> - Document pre-defined transfer functions;
> - Remove HLG transfer function from supported TFs;
> - Remove misleading comments;
> - Remove post-blending shaper TF+LUT and 3D LUT support;
> - Move driver-specific property operations from amdgpu_display.c to
>   amdgpu_dm_color.c;
> - Reset planes if any color props change;
> - Add plane CTM 3x4 support;
> - Removed two DC fixes already applied upstream;
> 
> [v2] https://lore.kernel.org/dri-devel/20230810160314.48225-1-m...@igalia.com
> - Many documentation fixes: BT.709 OETF, description of sRGB and pure
>   power functions, TF+1D LUT behavior;
> - Rename CTM2 to CTM 3x4 and fix misleading comment about DC gamut remap;
> - Squash `Linear` and `Unity` TF in `Identity`;
> - Remove the `MPC gamut remap` patch already applied upstream[3];
> - Remove outdated delta segmentation fix;
> - Nits/small fixes;
> 
> [v3] https://lore.kernel.org/amd-gfx/20230925194932.1329483-1-m...@igalia.com
> - Add table to describe value range in linear and non-linear forms
> - Comment the PQ TF need after HDR multiplier
> - Advertise the 3D LUT size as the size of a single-dimension (read-only)
> - remove function to check expected size from 3DLUT caps
> - cleanup comments
> 
> It's worth noting that driver-specific properties are guarded by
> `AMD_PRIVATE_COLOR`. So, finally, this is the color management API when
> driver-specific properties are enabled:
> 
> +--+
> |   PLANE  |
> |  |
> |  ++  |
> |  | AMD Degamma|  |
> |  ||  |
> |  | EOTF | 1D LUT  |  |
> |  ++---+  |
> |   |  |
> |  +v---+  |
> |  |AMD HDR |  |
> |  |Multiply|  |
> |  ++---+  |
> |   |  |
> |  +v---+  |
> |  |  AMD CTM (3x4) |  |
> |  ++---+  |
> |   |  |
> |  +v---+  |
> |  | AMD Shaper |  |
> |  ||  |
> |  | inv_EOTF | |  |
> |  | Custom 1D LUT  |  |
> |  ++---+  |
> |   |  |
> |  +v---+  |
> |  |   AMD 3D LUT   |  |
> |  |   17^3/12-bit  |  |
> |  ++---+  |
> |   |  |
> |  +v---+  |
> |  | AMD Blend  |  |
> |  ||  |
> |  | EOTF | 1D LUT  |  |
> |  ++---+  |
> |   |  |
> ++--v-++
> ||  Blending  ||
> ++--+-++
> |CRTC   |  |
> |   |  |
> |   +---v---+  |
> |   | DRM Degamma   |  |
> |   |