Re: [PATCH v2 2/4] drm/uAPI: Add "force color format" drm property as setting for userspace
On Wed, Jan 17, 2024 at 12:58:15PM +, Andri Yngvason wrote: > mið., 17. jan. 2024 kl. 09:21 skrifaði Pekka Paalanen : > > > > On Tue, 16 Jan 2024 14:11:43 + > > Andri Yngvason wrote: > > > > > þri., 16. jan. 2024 kl. 13:29 skrifaði Sebastian Wick > > > : > > > > > > > > On Tue, Jan 16, 2024 at 01:13:13PM +, Andri Yngvason wrote: > > > [...] > > > > > şri., 16. jan. 2024 kl. 11:42 skrifaği Sebastian Wick > > > > > : > > > > > > > > > > > > On Mon, Jan 15, 2024 at 04:05:52PM +, Andri Yngvason wrote: > > > > > > > From: Werner Sembach > > > > > > > > > > > > > > Add a new general drm property "force color format" which can be > > > > > > > used > > > > > > > by userspace to tell the graphics driver which color format to > > > > > > > use. > > > > > > > > > > > > I don't like the "force" in the name. This just selects the color > > > > > > format, let's just call it "color format" then. > > > > > > > > > > > > > > > > In previous revisions, this was "preferred color format" and "actual > > > > > color format", of which the latter has been dropped. I recommend > > > > > reading the discussion for previous revisions. > > > > > > > > Please don't imply that I didn't read the thread I'm answering to. > > > > FYI, I have not read this thread. > > > > pq, You did not read this summary? > https://lore.kernel.org/dri-devel/cafnqbqwjejax6b4oewpgasmud5_nxzymxufdog294ctvgbt...@mail.gmail.com/ > > You partook in the discussion on IRC. Please read it and tell me if I > misunderstood anything. > > Sebastian, I apologise. You clearly read it as you even replied to it! Thank you :) > > > > > > > > > There are arguments for adding "actual color format" later and if it > > > > > is added later, we'd end up with "color format" and "actual color > > > > > format", which might be confusing, and it is why I chose to call it > > > > > "force color format" because it clearly communicates intent and > > > > > disambiguates it from "actual color format". > > > > > > > > There is no such thing as "actual color format" in upstream though. > > > > Basing your naming on discarded ideas is not useful. The thing that sets > > > > the color space for example is called "Colorspace", not "force > > > > colorspace". > > > > > > > > > > Sure, I'm happy with calling it whatever people want. Maybe we can > > > have a vote on it? > > > > It would sound strange to say "force color format" = "auto". Just drop > > the "force" of it. > > > > If and when we need the feedback counterpart, it could be an immutable > > prop called "active color format" where "auto" is not a valid value, or > > something in the new "output properties" design Sima has been thinking > > of. > > There seems to be consensus for calling it "color format" > > > > > > > > [...] > > > > > > > @@ -1396,6 +1404,15 @@ static const u32 dp_colorspaces = > > > > > > > * drm_connector_attach_max_bpc_property() to create and > > > > > > > attach the > > > > > > > * property to the connector during initialization. > > > > > > > * > > > > > > > + * force color format: > > > > > > > + * This property is used by userspace to change the used color > > > > > > > format. When > > > > > > > + * used the driver will use the selected format if valid for > > > > > > > the hardware, > > > > > > > > > > > > All properties are always "used", they just can have different > > > > > > values. > > > > > > You probably want to talk about the auto mode here. > > > > > > > > > > Maybe we can say something like: If userspace does not set the > > > > > property or if it is explicitly set to zero, the driver will select > > > > > the appropriate color format based on other constraints. > > > > &g
Re: [PATCH v2 2/4] drm/uAPI: Add "force color format" drm property as setting for userspace
On Tue, Jan 16, 2024 at 01:13:13PM +, Andri Yngvason wrote: > Hi Sebastian, > > þri., 16. jan. 2024 kl. 11:42 skrifaði Sebastian Wick > : > > > > On Mon, Jan 15, 2024 at 04:05:52PM +, Andri Yngvason wrote: > > > From: Werner Sembach > > > > > > Add a new general drm property "force color format" which can be used > > > by userspace to tell the graphics driver which color format to use. > > > > I don't like the "force" in the name. This just selects the color > > format, let's just call it "color format" then. > > > > In previous revisions, this was "preferred color format" and "actual > color format", of which the latter has been dropped. I recommend > reading the discussion for previous revisions. Please don't imply that I didn't read the thread I'm answering to. > There are arguments for adding "actual color format" later and if it > is added later, we'd end up with "color format" and "actual color > format", which might be confusing, and it is why I chose to call it > "force color format" because it clearly communicates intent and > disambiguates it from "actual color format". There is no such thing as "actual color format" in upstream though. Basing your naming on discarded ideas is not useful. The thing that sets the color space for example is called "Colorspace", not "force colorspace". > [...] > > > @@ -1396,6 +1404,15 @@ static const u32 dp_colorspaces = > > > * drm_connector_attach_max_bpc_property() to create and attach the > > > * property to the connector during initialization. > > > * > > > + * force color format: > > > + * This property is used by userspace to change the used color format. > > > When > > > + * used the driver will use the selected format if valid for the > > > hardware, > > > > All properties are always "used", they just can have different values. > > You probably want to talk about the auto mode here. > > Maybe we can say something like: If userspace does not set the > property or if it is explicitly set to zero, the driver will select > the appropriate color format based on other constraints. The property can be in any state without involvement from user space. Don't talk about setting it, talk about the state it is in: When the color format is auto, the driver will select a format. > > > > > + * sink, and current resolution and refresh rate combination. Drivers > > > to > > > > If valid? So when a value is not actually supported user space can still > > set it? What happens then? How should user space figure out if the > > driver and the sink support the format? > > The kernel does not expose this property unless it's implemented in the > driver. If the driver simply doesn't support *one format*, the enum value for that format should not be exposed, period. This isn't about the property on its own. > This was originally "preferred color format". Perhaps the > documentation should better reflect that it is now a mandatory > constraint which fails the modeset if not satisfied. That would definitely help. > > > > For the Colorspace prop, the kernel just exposes all formats it supports > > (independent of the sink) and then makes it the job of user space to > > figure out if the sink supports it. > > > > The same could be done here. Property value is exposed if the driver > > supports it in general, commits can fail if the driver can't support it > > for a specific commit because e.g. the resolution or refresh rate. User > > space must look at the EDID/DisplayID/mode to figure out the supported > > format for the sink. > > Yes, we can make it possible for userspace to discover which modes are > supported by the monitor, but there are other constraints that need to > be satisfied. This was discussed in the previous revision. I mean, yes, that's what I said. User space would then only be responsible for checking the sink capabilities and the atomic check would take into account other (non-sink) constraints. > In any case, these things can be added later and need not be a part of > this change set. No, this is the contract between the kernel and user space and has to be figured out before we can merge new uAPI. > > [...] > > Regards, > Andri >
Re: [PATCH v2 2/4] drm/uAPI: Add "force color format" drm property as setting for userspace
On Mon, Jan 15, 2024 at 04:05:52PM +, Andri Yngvason wrote:
> From: Werner Sembach
>
> Add a new general drm property "force color format" which can be used
> by userspace to tell the graphics driver which color format to use.
I don't like the "force" in the name. This just selects the color
format, let's just call it "color format" then.
> Possible options are:
> - auto (default/current behaviour)
> - rgb
> - ycbcr444
> - ycbcr422 (supported by neither amdgpu or i915)
> - ycbcr420
>
> In theory the auto option should choose the best available option for the
> current setup, but because of bad internal conversion some monitors look
> better with rgb and some with ycbcr444.
>
> Also, because of bad shielded connectors and/or cables, it might be
> preferable to use the less bandwidth heavy ycbcr422 and ycbcr420 formats
> for a signal that is less susceptible to interference.
>
> In the future, automatic color calibration for screens might also depend on
> this option being available.
>
> Signed-off-by: Werner Sembach
> Signed-off-by: Andri Yngvason
> Tested-by: Andri Yngvason
> ---
>
> Changes in v2:
> - Renamed to "force color format" from "preferred color format"
> - Removed Reported-by pointing to invalid email address
>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 4 +++
> drivers/gpu/drm/drm_atomic_uapi.c | 4 +++
> drivers/gpu/drm/drm_connector.c | 48 +
> include/drm/drm_connector.h | 16 ++
> 4 files changed, 72 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 39ef0a6addeba..1dabd164c4f09 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -707,6 +707,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> if (old_connector_state->max_requested_bpc !=
> new_connector_state->max_requested_bpc)
> new_crtc_state->connectors_changed = true;
> +
> + if (old_connector_state->force_color_format !=
> + new_connector_state->force_color_format)
> + new_crtc_state->connectors_changed = true;
> }
>
> if (funcs->atomic_check)
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index 29d4940188d49..e45949bf4615f 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -776,6 +776,8 @@ static int drm_atomic_connector_set_property(struct
> drm_connector *connector,
> state->max_requested_bpc = val;
> } else if (property == connector->privacy_screen_sw_state_property) {
> state->privacy_screen_sw_state = val;
> + } else if (property == connector->force_color_format_property) {
> + state->force_color_format = val;
> } else if (connector->funcs->atomic_set_property) {
> return connector->funcs->atomic_set_property(connector,
> state, property, val);
> @@ -859,6 +861,8 @@ drm_atomic_connector_get_property(struct drm_connector
> *connector,
> *val = state->max_requested_bpc;
> } else if (property == connector->privacy_screen_sw_state_property) {
> *val = state->privacy_screen_sw_state;
> + } else if (property == connector->force_color_format_property) {
> + *val = state->force_color_format;
> } else if (connector->funcs->atomic_get_property) {
> return connector->funcs->atomic_get_property(connector,
> state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index b0516505f7ae9..e0535e58b4535 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1061,6 +1061,14 @@ static const struct drm_prop_enum_list
> drm_dp_subconnector_enum_list[] = {
> { DRM_MODE_SUBCONNECTOR_Native, "Native"}, /* DP */
> };
>
> +static const struct drm_prop_enum_list drm_force_color_format_enum_list[] = {
> + { 0, "auto" },
> + { DRM_COLOR_FORMAT_RGB444, "rgb" },
> + { DRM_COLOR_FORMAT_YCBCR444, "ycbcr444" },
> + { DRM_COLOR_FORMAT_YCBCR422, "ycbcr422" },
> + { DRM_COLOR_FORMAT_YCBCR420, "ycbcr420" },
> +};
> +
> DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
>drm_dp_subconnector_enum_list)
>
> @@ -1396,6 +1404,15 @@ static const u32 dp_colorspaces =
> * drm_connector_attach_max_bpc_property() to create and attach the
> * property to the connector during initialization.
> *
> + * force color format:
> + * This property is used by userspace to change the used color format. When
> + * used the driver will use the selected format if valid for the hardware,
All properties are always "used", they just can have different values.
Re: [PATCH 2/7] drm/uAPI: Add "active color format" drm property as feedback for userspace
On Thu, Jan 11, 2024 at 05:17:46PM +, Andri Yngvason wrote: > mið., 10. jan. 2024 kl. 13:26 skrifaði Daniel Stone : > > > > > > This thing here works entirely differently, and I think we need somewhat > > > new semantics for this: > > > > > > - I agree it should be read-only for userspace, so immutable sounds right. > > > > > > - But I also agree with Daniel Stone that this should be tied more > > > directly to the modeset state. > > > > > > So I think the better approach would be to put the output type into > > > drm_connector_state, require that drivers compute it in their > > > ->atomic_check code (which in the future would allow us to report it out > > > for TEST_ONLY commits too), and so guarantee that the value is updated > > > right after the kms ioctl returns (and not somewhen later for non-blocking > > > commits). > > > > That's exactly the point. Whether userspace gets an explicit > > notification or it has to 'know' when to read isn't much of an issue - > > just as long as it's well defined. I think the suggestion of 'do it in > > atomic_check and then it's guaranteed to be readable when the commit > > completes' is a good one. > > > > I do still have some reservations - for instance, why do we have the > > fallback to auto when userspace has explicitly requested a certain > > type? - but they may have been covered previously. > > > > We discussed this further on IRC and this is summary of that discussion: > > Sima proposed a new type of property that can be used to git feedback to > userspace after atomic ioctl. The user supplies a list of output properties > that they want to query and the kernel fills it in before returning from the > ioctl. This would help to get some information about why things failed > during TEST_ONLY. > > Emersion raised the point that you might not know how much memory is needed > beforehand for the returned properties, to which sima replied: blob > property. There was some discussion about how that makes it possible to leak > kernel memory, especially if userspace does not know about a new property > blob. Emersion pointed out that userspace should only request properties > that it understands and pq agreed. > > Emersion asked how the user should inform the kernel that it's done with the > blob, to which sima replied: DRM_IOCTL_MODE_DESTROYPROPBLOB. Sima also > mentioned using some sort of weak reference garbage collection scheme for > properties and there was some further discussion, but I'm not sure there was > any conclusion. > > I asked if it made sense to add color format capabilities to the mode info > struct, but the conclusion was that it wouldn't really be useful because we > need TEST_ONLY anyway to see if the color format setting is compatible with > other settings. > > I asked again if we should drop the "active color format" property as it > seems to be more trouble than it's worth for now. pq mentioned that there > are 2 separate use cases (in his words): > - People playing with setting UI would like to know what "auto" would result > in, but that's just nice to have > - The other use case is the flicker-free boot into known configuration He > went on to point out that the main problem with "auto" is that any modeset > could make the driver decide differently. This means that we cannot fully > rely on the previously set property. > > However, leaving out "active color property" did not put us in a worse > situation than before, so the conclusion was that we should leave it out for > now. For GUI selectors, the current TEST_ONLY should be good enough, and all > the fancy stuff discussed previously isn't needed for now. > > To summarise the summary: this means that we will drop the "active > color format" property and rename the "preferred color format" > property to "force color format" or just "color format" and failing to > satisfy that constraint will fail the modeset rather than falling back > to the "auto" behaviour. That's a good idea. Anything else won't work with the new color pipeline API anyways because user space will be responsible for setting up the color pipeline API in a way so that the monitor will receive the correctly encoded data. > Cheers, > Andri >
Re: [PATCH v4 00/32] drm/amd/display: add AMD driver-specific properties for color mgmt
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 | | > | |
Re: [PATCH v4 09/32] drm/amd/display: add plane 3D LUT driver-specific properties
On Thu, Oct 05, 2023 at 04:15:04PM -0100, Melissa Wen wrote:
> Add 3D LUT property for plane color transformations using a 3D lookup
> table. 3D LUT allows for highly accurate and complex color
> transformations and is suitable to adjust the balance between color
> channels. It's also more complex to manage and require more
> computational resources.
>
> Since a 3D LUT has a limited number of entries in each dimension we want
> to use them in an optimal fashion. This means using the 3D LUT in a
> colorspace that is optimized for human vision, such as sRGB, PQ, or
> another non-linear space. Therefore, userpace may need one 1D LUT
> (shaper) before it to delinearize content and another 1D LUT after 3D
> LUT (blend) to linearize content again for blending. The next patches
> add these 1D LUTs to the plane color mgmt pipeline.
>
> v3:
> - improve commit message about 3D LUT
> - describe the 3D LUT entries and size (Harry)
>
> v4:
> - advertise 3D LUT max size as the size of a single-dimension
>
> Signed-off-by: Melissa Wen
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 18 +++
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 9
> .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 14 +++
> .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 23 +++
> 4 files changed, 64 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index 62044d41da75..f7adaa52c23f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -363,6 +363,24 @@ struct amdgpu_mode_info {
>* @plane_hdr_mult_property:
>*/
> struct drm_property *plane_hdr_mult_property;
> + /**
> + * @plane_lut3d_property: Plane property for color transformation using
> + * a 3D LUT (pre-blending), a three-dimensional array where each
> + * element is an RGB triplet. Each dimension has a size of the cubed
> + * root of lut3d_size. The array contains samples from the approximated
This should be "Each dimension has a size of lut3d_size" now.
> + * function. On AMD, values between samples are estimated by
> + * tetrahedral interpolation. The array is accessed with three indices,
> + * one for each input dimension (color channel), blue being the
> + * outermost dimension, red the innermost.
> + */
> + struct drm_property *plane_lut3d_property;
> + /**
> + * @plane_degamma_lut_size_property: Plane property to define the max
> + * size of 3D LUT as supported by the driver (read-only). The max size
> + * is the max size of one dimension and, therefore, the max number of
> + * entries for 3D LUT array is the 3D LUT size cubed;
> + */
> + struct drm_property *plane_lut3d_size_property;
> };
>
> #define AMDGPU_MAX_BL_LEVEL 0xFF
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index bb2ce843369d..7a2350c62cf1 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -784,6 +784,11 @@ struct dm_plane_state {
>* TF is needed for any subsequent linear-to-non-linear transforms.
>*/
> __u64 hdr_mult;
> + /**
> + * @lut3d: 3D lookup table blob. The blob (if not NULL) is an array of
> + * drm_color_lut.
> + */
> + struct drm_property_blob *lut3d;
> };
>
> struct dm_crtc_state {
> @@ -869,6 +874,10 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector
> *connector,
>
> void amdgpu_dm_trigger_timing_sync(struct drm_device *dev);
>
> +/* 3D LUT max size is 17x17x17 (4913 entries) */
> +#define MAX_COLOR_3DLUT_SIZE 17
> +#define MAX_COLOR_3DLUT_BITDEPTH 12
> +/* 1D LUT size */
> #define MAX_COLOR_LUT_ENTRIES 4096
> /* Legacy gamm LUT users such as X doesn't like large LUT sizes */
> #define MAX_COLOR_LEGACY_LUT_ENTRIES 256
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> index caf49a044ab4..011f2f9ec890 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> @@ -230,6 +230,20 @@ amdgpu_dm_create_color_properties(struct amdgpu_device
> *adev)
> return -ENOMEM;
> adev->mode_info.plane_hdr_mult_property = prop;
>
> + prop = drm_property_create(adev_to_drm(adev),
> +DRM_MODE_PROP_BLOB,
> +"AMD_PLANE_LUT3D", 0);
> + if (!prop)
> + return -ENOMEM;
> + adev->mode_info.plane_lut3d_property = prop;
> +
> + prop = drm_property_create_range(adev_to_drm(adev),
> + DRM_MODE_PROP_IMMUTABLE,
> + "AMD_PLANE_LUT3D_SIZE", 0, UINT_MAX);
> + if (!prop)
> +
Re: [PATCH v7] drm/doc: Document DRM device reset expectations
; +Vulkan > +~~ > + > +Apps using Vulkan should check for ``VK_ERROR_DEVICE_LOST`` for submissions. > +This error code means, among other things, that a device reset has happened > and > +it needs to recreate the contexts to keep going. > + > +Reporting causes of resets > +-- > + > +Apart from propagating the reset through the stack so apps can recover, it's > +really useful for driver developers to learn more about what caused the > reset in > +the first place. DRM devices should make use of devcoredump to store relevant > +information about the reset, so this information can be added to user bug > +reports. > + > .. _drm_driver_ioctl: > > IOCTL Support on Device Nodes Acked-by: Sebastian Wick > -- > 2.41.0 >
Re: [PATCH v6 5/6] drm: Refuse to async flip with atomic prop changes
On Tue, Aug 15, 2023 at 03:57:09PM -0300, André Almeida wrote:
> Given that prop changes may lead to modesetting, which would defeat the
> fast path of the async flip, refuse any atomic prop change for async
> flips in atomic API. The only exceptions are the framebuffer ID to flip
> to and the mode ID, that could be referring to an identical mode.
FYI, the solid fill series adds an enum drm_plane_pixel_source and and a
new solid fill pixel source. Changing the solid fill color would be
effectively the same as changing the FB_ID. On the other hand, changing
the FB_ID no longer necessarily results in an update when the pixel
source is set to solid fill.
> Signed-off-by: André Almeida
> ---
> v5: no changes
> v4: new patch
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 5 +++
> drivers/gpu/drm/drm_atomic_uapi.c | 52 +++--
> drivers/gpu/drm/drm_crtc_internal.h | 2 +-
> drivers/gpu/drm/drm_mode_object.c | 2 +-
> 4 files changed, 56 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 292e38eb6218..b34e3104afd1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -629,6 +629,11 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> WARN_ON(!drm_modeset_is_locked(>mutex));
>
> if (!drm_mode_equal(_crtc_state->mode,
> _crtc_state->mode)) {
> + if (new_crtc_state->async_flip) {
> + drm_dbg_atomic(dev, "[CRTC:%d:%s] no mode
> changes allowed during async flip\n",
> +crtc->base.id, crtc->name);
> + return -EINVAL;
> + }
> drm_dbg_atomic(dev, "[CRTC:%d:%s] mode changed\n",
> crtc->base.id, crtc->name);
> new_crtc_state->mode_changed = true;
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index a15121e75a0a..6c423a7e8c7b 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1006,13 +1006,28 @@ int drm_atomic_connector_commit_dpms(struct
> drm_atomic_state *state,
> return ret;
> }
>
> +static int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t
> prop_value,
> + struct drm_property *prop)
> +{
> + if (ret != 0 || old_val != prop_value) {
> + drm_dbg_atomic(prop->dev,
> +"[PROP:%d:%s] No prop can be changed during
> async flip\n",
> +prop->base.id, prop->name);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> int drm_atomic_set_property(struct drm_atomic_state *state,
> struct drm_file *file_priv,
> struct drm_mode_object *obj,
> struct drm_property *prop,
> - uint64_t prop_value)
> + uint64_t prop_value,
> + bool async_flip)
> {
> struct drm_mode_object *ref;
> + uint64_t old_val;
> int ret;
>
> if (!drm_property_change_valid_get(prop, prop_value, ))
> @@ -1029,6 +1044,13 @@ int drm_atomic_set_property(struct drm_atomic_state
> *state,
> break;
> }
>
> + if (async_flip) {
> + ret = drm_atomic_connector_get_property(connector,
> connector_state,
> + prop, _val);
> + ret = drm_atomic_check_prop_changes(ret, old_val,
> prop_value, prop);
> + break;
> + }
> +
> ret = drm_atomic_connector_set_property(connector,
> connector_state, file_priv,
> prop, prop_value);
> @@ -1037,6 +1059,7 @@ int drm_atomic_set_property(struct drm_atomic_state
> *state,
> case DRM_MODE_OBJECT_CRTC: {
> struct drm_crtc *crtc = obj_to_crtc(obj);
> struct drm_crtc_state *crtc_state;
> + struct drm_mode_config *config = >dev->mode_config;
>
> crtc_state = drm_atomic_get_crtc_state(state, crtc);
> if (IS_ERR(crtc_state)) {
> @@ -1044,6 +1067,18 @@ int drm_atomic_set_property(struct drm_atomic_state
> *state,
> break;
> }
>
> + /*
> + * We allow mode_id changes here for async flips, because we
> + * check later on drm_atomic_helper_check_modeset() callers if
> + * there are modeset changes or they are equal
> + */
> + if (async_flip && prop != config->prop_mode_id) {
> + ret = drm_atomic_crtc_get_property(crtc, crtc_state,
> +
Re: [PATCH v5 1/1] drm/doc: Document DRM device reset expectations
On Fri, Aug 4, 2023 at 3:03 PM Daniel Vetter wrote: > > On Tue, Jun 27, 2023 at 10:23:23AM -0300, André Almeida wrote: > > Create a section that specifies how to deal with DRM device resets for > > kernel and userspace drivers. > > > > Acked-by: Pekka Paalanen > > Signed-off-by: André Almeida > > --- > > > > v4: > > https://lore.kernel.org/lkml/20230626183347.55118-1-andrealm...@igalia.com/ > > > > Changes: > > - Grammar fixes (Randy) > > > > Documentation/gpu/drm-uapi.rst | 68 ++ > > 1 file changed, 68 insertions(+) > > > > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst > > index 65fb3036a580..3cbffa25ed93 100644 > > --- a/Documentation/gpu/drm-uapi.rst > > +++ b/Documentation/gpu/drm-uapi.rst > > @@ -285,6 +285,74 @@ for GPU1 and GPU2 from different vendors, and a third > > handler for > > mmapped regular files. Threads cause additional pain with signal > > handling as well. > > > > +Device reset > > + > > + > > +The GPU stack is really complex and is prone to errors, from hardware bugs, > > +faulty applications and everything in between the many layers. Some errors > > +require resetting the device in order to make the device usable again. This > > +sections describes the expectations for DRM and usermode drivers when a > > +device resets and how to propagate the reset status. > > + > > +Kernel Mode Driver > > +-- > > + > > +The KMD is responsible for checking if the device needs a reset, and to > > perform > > +it as needed. Usually a hang is detected when a job gets stuck executing. > > KMD > > +should keep track of resets, because userspace can query any time about the > > +reset stats for an specific context. This is needed to propagate to the > > rest of > > +the stack that a reset has happened. Currently, this is implemented by each > > +driver separately, with no common DRM interface. > > + > > +User Mode Driver > > + > > + > > +The UMD should check before submitting new commands to the KMD if the > > device has > > +been reset, and this can be checked more often if the UMD requires it. > > After > > +detecting a reset, UMD will then proceed to report it to the application > > using > > +the appropriate API error code, as explained in the section below about > > +robustness. > > + > > +Robustness > > +-- > > + > > +The only way to try to keep an application working after a reset is if it > > +complies with the robustness aspects of the graphical API that it is using. > > + > > +Graphical APIs provide ways to applications to deal with device resets. > > However, > > +there is no guarantee that the app will use such features correctly, and > > the > > +UMD can implement policies to close the app if it is a repeating offender, > > Not sure whether this one here is due to my input, but s/UMD/KMD. Repeat > offender killing is more a policy where the kernel enforces policy, and no > longer up to userspace to dtrt (because very clearly userspace is not > really doing the right thing anymore when it's just hanging the gpu in an > endless loop). Also maybe tune it down further to something like "the > kernel driver may implemnent ..." > > In my opinion the umd shouldn't implement these kind of magic guesses, the > entire point of robustness apis is to delegate responsibility for > correctly recovering to the application. And the kernel is left with > enforcing fair resource usage policies (which eventually might be a > cgroups limit on how much gpu time you're allowed to waste with gpu > resets). Killing apps that the kernel thinks are misbehaving really doesn't seem like a good idea to me. What if the process is a service getting restarted after getting killed? What if killing that process leaves the system in a bad state? Can't the kernel provide some information to user space so that e.g. systemd can handle those situations? > > +likely in a broken loop. This is done to ensure that it does not keep > > blocking > > +the user interface from being correctly displayed. This should be done > > even if > > +the app is correct but happens to trigger some bug in the hardware/driver. > > + > > +OpenGL > > +~~ > > + > > +Apps using OpenGL should use the available robust interfaces, like the > > +extension ``GL_ARB_robustness`` (or ``GL_EXT_robustness`` for OpenGL ES). > > This > > +interface tells if a reset has happened, and if so, all the context state > > is > > +considered lost and the app proceeds by creating new ones. If it is > > possible to > > +determine that robustness is not in use, the UMD will terminate the app > > when a > > +reset is detected, giving that the contexts are lost and the app won't be > > able > > +to figure this out and recreate the contexts. > > + > > +Vulkan > > +~~ > > + > > +Apps using Vulkan should check for ``VK_ERROR_DEVICE_LOST`` for > > submissions. > > +This error code means, among other things, that a device reset has > >
Re: [PATCH v4 1/6] drm: allow DRM_MODE_PAGE_FLIP_ASYNC for atomic commits
On Sat, Jul 1, 2023 at 4:09 AM André Almeida wrote:
>
> From: Simon Ser
>
> If the driver supports it, allow user-space to supply the
> DRM_MODE_PAGE_FLIP_ASYNC flag to request an async page-flip.
> Set drm_crtc_state.async_flip accordingly.
>
> Document that drivers will reject atomic commits if an async
> flip isn't possible. This allows user-space to fall back to
> something else. For instance, Xorg falls back to a blit.
> Another option is to wait as close to the next vblank as
> possible before performing the page-flip to reduce latency.
>
> Signed-off-by: Simon Ser
> Reviewed-by: Alex Deucher
> Co-developed-by: André Almeida
> Signed-off-by: André Almeida
> ---
> v4: no changes
> ---
> drivers/gpu/drm/drm_atomic_uapi.c | 28 +---
> include/uapi/drm/drm_mode.h | 9 +
> 2 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index d867e7f9f2cd..dfd4cf7169df 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1286,6 +1286,18 @@ static void complete_signaling(struct drm_device *dev,
> kfree(fence_state);
> }
>
> +static void
> +set_async_flip(struct drm_atomic_state *state)
> +{
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *crtc_state;
> + int i;
> +
> + for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> + crtc_state->async_flip = true;
> + }
> +}
> +
> int drm_mode_atomic_ioctl(struct drm_device *dev,
> void *data, struct drm_file *file_priv)
> {
> @@ -1326,9 +1338,16 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> }
>
> if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC) {
> - drm_dbg_atomic(dev,
> - "commit failed: invalid flag
> DRM_MODE_PAGE_FLIP_ASYNC\n");
> - return -EINVAL;
> + if (!dev->mode_config.async_page_flip) {
> + drm_dbg_atomic(dev,
> + "commit failed:
> DRM_MODE_PAGE_FLIP_ASYNC not supported\n");
> + return -EINVAL;
> + }
> + if (dev->mode_config.atomic_async_page_flip_not_supported) {
> + drm_dbg_atomic(dev,
> + "commit failed:
> DRM_MODE_PAGE_FLIP_ASYNC not supported with atomic\n");
> + return -EINVAL;
> + }
> }
>
> /* can't test and expect an event at the same time. */
> @@ -1426,6 +1445,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> if (ret)
> goto out;
>
> + if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC)
> + set_async_flip(state);
> +
> if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
> ret = drm_atomic_check_only(state);
> } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 46becedf5b2f..56342ba2c11a 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -949,6 +949,15 @@ struct hdr_output_metadata {
> * Request that the page-flip is performed as soon as possible, ie. with no
> * delay due to waiting for vblank. This may cause tearing to be visible on
> * the screen.
> + *
> + * When used with atomic uAPI, the driver will return an error if the
> hardware
> + * doesn't support performing an asynchronous page-flip for this update.
> + * User-space should handle this, e.g. by falling back to a regular
> page-flip.
> + *
> + * Note, some hardware might need to perform one last synchronous page-flip
> + * before being able to switch to asynchronous page-flips. As an exception,
> + * the driver will return success even though that first page-flip is not
> + * asynchronous.
What would happen if one commits another async KMS update before the
first page flip? Does one receive EAGAIN, does it amend the previous
commit? What happens to the timing feedback?
This seems really risky to include tbh. I would prefer if we would not
add such special cases for now.
> */
> #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
> #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> --
> 2.41.0
>
Re: [PATCH v5 1/1] drm/doc: Document DRM device reset expectations
On Fri, Jun 30, 2023 at 4:59 PM Alex Deucher wrote: > > On Fri, Jun 30, 2023 at 10:49 AM Sebastian Wick > wrote: > > > > On Tue, Jun 27, 2023 at 3:23 PM André Almeida > > wrote: > > > > > > Create a section that specifies how to deal with DRM device resets for > > > kernel and userspace drivers. > > > > > > Acked-by: Pekka Paalanen > > > Signed-off-by: André Almeida > > > --- > > > > > > v4: > > > https://lore.kernel.org/lkml/20230626183347.55118-1-andrealm...@igalia.com/ > > > > > > Changes: > > > - Grammar fixes (Randy) > > > > > > Documentation/gpu/drm-uapi.rst | 68 ++ > > > 1 file changed, 68 insertions(+) > > > > > > diff --git a/Documentation/gpu/drm-uapi.rst > > > b/Documentation/gpu/drm-uapi.rst > > > index 65fb3036a580..3cbffa25ed93 100644 > > > --- a/Documentation/gpu/drm-uapi.rst > > > +++ b/Documentation/gpu/drm-uapi.rst > > > @@ -285,6 +285,74 @@ for GPU1 and GPU2 from different vendors, and a > > > third handler for > > > mmapped regular files. Threads cause additional pain with signal > > > handling as well. > > > > > > +Device reset > > > + > > > + > > > +The GPU stack is really complex and is prone to errors, from hardware > > > bugs, > > > +faulty applications and everything in between the many layers. Some > > > errors > > > +require resetting the device in order to make the device usable again. > > > This > > > +sections describes the expectations for DRM and usermode drivers when a > > > +device resets and how to propagate the reset status. > > > + > > > +Kernel Mode Driver > > > +-- > > > + > > > +The KMD is responsible for checking if the device needs a reset, and to > > > perform > > > +it as needed. Usually a hang is detected when a job gets stuck > > > executing. KMD > > > +should keep track of resets, because userspace can query any time about > > > the > > > +reset stats for an specific context. This is needed to propagate to the > > > rest of > > > +the stack that a reset has happened. Currently, this is implemented by > > > each > > > +driver separately, with no common DRM interface. > > > + > > > +User Mode Driver > > > + > > > + > > > +The UMD should check before submitting new commands to the KMD if the > > > device has > > > +been reset, and this can be checked more often if the UMD requires it. > > > After > > > +detecting a reset, UMD will then proceed to report it to the application > > > using > > > +the appropriate API error code, as explained in the section below about > > > +robustness. > > > + > > > +Robustness > > > +-- > > > + > > > +The only way to try to keep an application working after a reset is if it > > > +complies with the robustness aspects of the graphical API that it is > > > using. > > > + > > > +Graphical APIs provide ways to applications to deal with device resets. > > > However, > > > +there is no guarantee that the app will use such features correctly, and > > > the > > > +UMD can implement policies to close the app if it is a repeating > > > offender, > > > +likely in a broken loop. This is done to ensure that it does not keep > > > blocking > > > +the user interface from being correctly displayed. This should be done > > > even if > > > +the app is correct but happens to trigger some bug in the > > > hardware/driver. > > > > I still don't think it's good to let the kernel arbitrarily kill > > processes that it thinks are not well-behaved based on some heuristics > > and policy. > > > > Can't this be outsourced to user space? Expose the information about > > processes causing a device and let e.g. systemd deal with coming up > > with a policy and with killing stuff. > > I don't think it's the kernel doing the killing, it would be the UMD. > E.g., if the app is guilty and doesn't support robustness the UMD can > just call exit(). Ah, right, completely skipped over the UMD part. That makes more sense. > > Alex > > > > > > + > > > +OpenGL > > > +~~ > > > + > > > +Apps using OpenGL should use the available robust interfaces,
Re: [PATCH v5 1/1] drm/doc: Document DRM device reset expectations
On Tue, Jun 27, 2023 at 3:23 PM André Almeida wrote: > > Create a section that specifies how to deal with DRM device resets for > kernel and userspace drivers. > > Acked-by: Pekka Paalanen > Signed-off-by: André Almeida > --- > > v4: > https://lore.kernel.org/lkml/20230626183347.55118-1-andrealm...@igalia.com/ > > Changes: > - Grammar fixes (Randy) > > Documentation/gpu/drm-uapi.rst | 68 ++ > 1 file changed, 68 insertions(+) > > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst > index 65fb3036a580..3cbffa25ed93 100644 > --- a/Documentation/gpu/drm-uapi.rst > +++ b/Documentation/gpu/drm-uapi.rst > @@ -285,6 +285,74 @@ for GPU1 and GPU2 from different vendors, and a third > handler for > mmapped regular files. Threads cause additional pain with signal > handling as well. > > +Device reset > + > + > +The GPU stack is really complex and is prone to errors, from hardware bugs, > +faulty applications and everything in between the many layers. Some errors > +require resetting the device in order to make the device usable again. This > +sections describes the expectations for DRM and usermode drivers when a > +device resets and how to propagate the reset status. > + > +Kernel Mode Driver > +-- > + > +The KMD is responsible for checking if the device needs a reset, and to > perform > +it as needed. Usually a hang is detected when a job gets stuck executing. KMD > +should keep track of resets, because userspace can query any time about the > +reset stats for an specific context. This is needed to propagate to the rest > of > +the stack that a reset has happened. Currently, this is implemented by each > +driver separately, with no common DRM interface. > + > +User Mode Driver > + > + > +The UMD should check before submitting new commands to the KMD if the device > has > +been reset, and this can be checked more often if the UMD requires it. After > +detecting a reset, UMD will then proceed to report it to the application > using > +the appropriate API error code, as explained in the section below about > +robustness. > + > +Robustness > +-- > + > +The only way to try to keep an application working after a reset is if it > +complies with the robustness aspects of the graphical API that it is using. > + > +Graphical APIs provide ways to applications to deal with device resets. > However, > +there is no guarantee that the app will use such features correctly, and the > +UMD can implement policies to close the app if it is a repeating offender, > +likely in a broken loop. This is done to ensure that it does not keep > blocking > +the user interface from being correctly displayed. This should be done even > if > +the app is correct but happens to trigger some bug in the hardware/driver. I still don't think it's good to let the kernel arbitrarily kill processes that it thinks are not well-behaved based on some heuristics and policy. Can't this be outsourced to user space? Expose the information about processes causing a device and let e.g. systemd deal with coming up with a policy and with killing stuff. > + > +OpenGL > +~~ > + > +Apps using OpenGL should use the available robust interfaces, like the > +extension ``GL_ARB_robustness`` (or ``GL_EXT_robustness`` for OpenGL ES). > This > +interface tells if a reset has happened, and if so, all the context state is > +considered lost and the app proceeds by creating new ones. If it is possible > to > +determine that robustness is not in use, the UMD will terminate the app when > a > +reset is detected, giving that the contexts are lost and the app won't be > able > +to figure this out and recreate the contexts. > + > +Vulkan > +~~ > + > +Apps using Vulkan should check for ``VK_ERROR_DEVICE_LOST`` for submissions. > +This error code means, among other things, that a device reset has happened > and > +it needs to recreate the contexts to keep going. > + > +Reporting causes of resets > +-- > + > +Apart from propagating the reset through the stack so apps can recover, it's > +really useful for driver developers to learn more about what caused the > reset in > +first place. DRM devices should make use of devcoredump to store relevant > +information about the reset, so this information can be added to user bug > +reports. > + > .. _drm_driver_ioctl: > > IOCTL Support on Device Nodes > -- > 2.41.0 >
Re: [PATCH 06/36] drm/amd/display: add CRTC driver-specific property for gamma TF
On Tue, Jun 6, 2023 at 6:19 PM Joshua Ashton wrote:
>
>
>
> On 6/1/23 20:17, Harry Wentland wrote:
> >
> >
> > On 5/23/23 18:14, Melissa Wen wrote:
> >> Hook up driver-specific atomic operations for managing AMD color
> >> properties and create AMD driver-specific color management properties
> >> and attach them according to HW capabilities defined by `struct
> >> dc_color_caps`. Add enumerated transfer function property to DRM CRTC
> >> gamma to convert to wire encoding with or without a user gamma LUT.
> >> Enumerated TFs are not supported yet by the DRM color pipeline,
> >> therefore, create a DRM enum list with the predefined TFs supported by
> >> the AMD display driver.
> >>
> >> Co-developed-by: Joshua Ashton
> >> Signed-off-by: Joshua Ashton
> >> Signed-off-by: Melissa Wen
> >> ---
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 36 ++
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 8 +++
> >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 22 ++
> >> .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c| 72 ++-
> >> 4 files changed, 137 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> >> index 389396eac222..88af075e6c18 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> >> @@ -1247,6 +1247,38 @@ amdgpu_display_user_framebuffer_create(struct
> >> drm_device *dev,
> >> return _fb->base;
> >> }
> >>
> >> +static const struct drm_prop_enum_list drm_transfer_function_enum_list[]
> >> = {
> >> +{ DRM_TRANSFER_FUNCTION_DEFAULT, "Default" },
> >> +{ DRM_TRANSFER_FUNCTION_SRGB, "sRGB" },
> >> +{ DRM_TRANSFER_FUNCTION_BT709, "BT.709" },
> >> +{ DRM_TRANSFER_FUNCTION_PQ, "PQ (Perceptual Quantizer)" },
> >> +{ DRM_TRANSFER_FUNCTION_LINEAR, "Linear" },
> >> +{ DRM_TRANSFER_FUNCTION_UNITY, "Unity" },
> >> +{ DRM_TRANSFER_FUNCTION_HLG, "HLG (Hybrid Log Gamma)" },
> >> +{ DRM_TRANSFER_FUNCTION_GAMMA22, "Gamma 2.2" },
> >> +{ DRM_TRANSFER_FUNCTION_GAMMA24, "Gamma 2.4" },
> >> +{ DRM_TRANSFER_FUNCTION_GAMMA26, "Gamma 2.6" },
> >> +};
> >
> > Let's not use the DRM_/drm_ prefix here. It might clash later when
> > we introduce DRM_ core interfaces for enumerated transfer functions.
> >
> > We'll want to specify whether something is an EOTF or an inverse EOTF,
> > or possibly an OETF. Of course that wouldn't apply to "Linear" or
> > "Unity" (I'm assuming the two are the same?).
> >
> > EOTF = electro-optical transfer function
> > This is the transfer function to go from the encoded value to an
> > optical (linear) value.
> >
> > Inverse EOTF = simply the inverse of the EOTF
> > This is usually intended to go from an optical/linear space (which
> > might have been used for blending) back to the encoded values.
> >
> > OETF = opto-electronic transfer function
> > This is usually used for converting optical signals into encoded
> > values. Usually that's done on the camera side but I think HLG might
> > define the OETF instead of the EOTF. A bit fuzzy on that. If you're
> > unclear about HLG I recommend we don't include it yet.
> >
> > It would also be good to document a bit more what each of the TFs
> > mean, but I'm fine if that comes later with a "driver-agnostic"
> > API. The key thing to clarify is whether we have an EOTF or inv_EOTF,
> > i.e. whether we use the TF to go from encoded to optical or vice
> > versa.
>
> Whether we use the inverse or not is just determined per-block though.
>
> I think we should definitely document it per-block very explicitly
> (whether it is EOTF or inv EOTF) but I don't think we need to touch the
> enums here.
Either the drm prefix has to be removed or the enum variant names have
to be explicitly be the EOTF, OETF, inverse EOTF or inverse OETF.
> - Joshie ✨
>
> >
> > I know DC is sloppy and doesn't define those but it will still use
> > them as either EOTF or inv_EOTF, based on which block they're being
> > programmed, if I'm not mistaken.
> >
> >> +
> >> +#ifdef AMD_PRIVATE_COLOR
> >> +static int
> >> +amdgpu_display_create_color_properties(struct amdgpu_device *adev)
> >> +{
> >> +struct drm_property *prop;
> >> +
> >> +prop = drm_property_create_enum(adev_to_drm(adev),
> >> +DRM_MODE_PROP_ENUM,
> >> +"AMD_REGAMMA_TF",
> >> +drm_transfer_function_enum_list,
> >> +
> >> ARRAY_SIZE(drm_transfer_function_enum_list));
> >> +if (!prop)
> >> +return -ENOMEM;
> >> +adev->mode_info.regamma_tf_property = prop;
> >> +
> >> +return 0;
> >> +}
> >> +#endif
> >> +
> >
> > It'd be nice if we have this function and the above enum_list
> > in amdgpu_dm, possibly in amdgpu_dm_color.c. You could call it
> > directly after the amdgpu_display_modeset_create_prop() call
Re: [PATCH v4 00/13] Enable Colorspace connector property in amdgpu
With the documentation about RGB and YCC variants added the drm core patches are Reviewed-by: Sebastian Wick On Fri, May 26, 2023 at 6:24 PM Sebastian Wick wrote: > > On Fri, May 26, 2023 at 3:16 PM Pekka Paalanen wrote: > > > > On Thu, 25 May 2023 15:17:56 -0400 > > Harry Wentland wrote: > > > > > This patchset is based on Joshua's previous patchset [1], as well > > > as my previous patchset [2]. > > > > > > It is > > > - enabling support for the colorspace property in amdgpu, as well as > > > - allowing drivers to specify the supported set of colorspaces, and > > > > > > Colorspace, Infoframes, and YCbCr matrix > > > --- > > > > > > Even though the initial intent of the colorspace property was to set the > > > colorspace field in the respective HDMI AVI and DP SDP infoframes that > > > is not sufficient in all scenarios. For DP the colorspace information > > > also affects the MSA (main stream attribute) packet. For YUV output the > > > colorspace affects the RGB-to-YCbCr conversion matrix. The colorspace > > > field of the infopackets also depends on the encoding used, which is > > > something that is decided by the driver and not known to userspace. > > > > Hi Harry, > > > > the "deprecation" of BT2020 RGB vs. YCC is now dropped completely. > > Should there still be a patch that adds some UAPI documentation only, > > saying that drivers are free to swap e.g. between BT2020 RGB and YCC > > based which encoding they actually chose? > > Yes please! > > > Even if just BT2020 variant specifically. > > > > I have nothing against with this series now. > > > > > > Thanks, > > pq > > > > > > > > For these reasons a driver will need to be able to select the supported > > > colorspaces at property creation. > > > > > > Note: There seems to be an understanding that the colorspace property > > > should ONLY modify the infoframe. While this is current behavior and > > > sufficient in some cases it is nowhere specified that this should be the > > > only use of this property. As outlined above this limitation is not > > > going to work in all cases. > > > > > > This patchset does not affect current behavior for the drivers that > > > implement this property: i915 and vc4. > > > > > > In the future we might want to give userspace control over the encoding > > > format on the wire, in particular to avoid use of YUV420 when image > > > fidelity is important. This work would likely go hand in hand with a > > > min_bpc property and wouldn't conflict with the work done in this > > > patchset. I would expect this future work to tag along with a drm_crtc > > > or drm_connector's Color Pipeline, similar to the one propsed for > > > drm_plane [3]. > > > > > > Colorspace on crtc or connector? > > > > > > > > > There have been suggestions of programming 'colorspace' on the drm_crtc > > > but I don't think the crtc is the right place for this property. The > > > drm_plane and drm_crtc will be used to offload color processing that > > > would normally be done via the GFX or other pipelines. The drm_connector > > > controls the signalling with the display and ensures the wire format is > > > appropriate for the encoding by programming the RGB-to-YCbCr matrix. > > > > > > [1] https://patchwork.freedesktop.org/series/113632/ > > > [2] https://patchwork.freedesktop.org/series/111865/ > > > [3] https://lists.freedesktop.org/archives/dri-devel/2023-May/403173.html
Re: [PATCH v4 02/13] drm/connector: Add enum documentation to drm_colorspace
On Fri, May 26, 2023 at 6:37 PM Simon Ser wrote: > > On Friday, May 26th, 2023 at 18:27, Sebastian Wick > wrote: > > > > + * @DRM_MODE_COLORIMETRY_DEFAULT: > > > + * Driver specific behavior. > > > + * @DRM_MODE_COLORIMETRY_NO_DATA: > > > + * Driver specific behavior. > > > > TBH this is still confusing me. Is DEFAULT really just driver specific > > behavior? What's the difference to NO_DATA? Is NO_DATA actually not > > driver-specific but display-specific? I.e. the default colorimetry of > > the display? > > DEFAULT == NO_DATA == 0 > *sight* and backwards compat... Alright, not much we can do then.
Re: [PATCH v4 08/13] drm/amd/display: Register Colorspace property for DP and HDMI
On Thu, May 25, 2023 at 9:18 PM Harry Wentland wrote:
>
> We want compositors to be able to set the output
> colorspace on DP and HDMI outputs, based on the
> caps reported from the receiver via EDID.
This commit message seems wrong for multiple reasons. The Colorspace
property documentation explicitly says that user space has to check
the sink EDID for support:
> The expectation from userspace is that it should parse the EDID and get
> supported colorspaces.
The code doesn't seem to care about EDID at all. Instead it exposes
the variants it knows how to support with e.g. the appropriate YCC
transform when necessary, independent of the sink support.
> Signed-off-by: Harry Wentland
> Cc: Pekka Paalanen
> Cc: Sebastian Wick
> Cc: vitaly.pros...@amd.com
> Cc: Joshua Ashton
> Cc: Simon Ser
> Cc: Ville Syrjälä
> Cc: Melissa Wen
> Cc: dri-de...@lists.freedesktop.org
> Cc: amd-gfx@lists.freedesktop.org
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index ca093396d1ac..dc99a8ffac70 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -7238,6 +7238,12 @@ static int amdgpu_dm_connector_get_modes(struct
> drm_connector *connector)
> return amdgpu_dm_connector->num_modes;
> }
>
> +static const u32 supported_colorspaces =
> + BIT(DRM_MODE_COLORIMETRY_BT709_YCC) |
> + BIT(DRM_MODE_COLORIMETRY_OPRGB) |
> + BIT(DRM_MODE_COLORIMETRY_BT2020_RGB) |
> + BIT(DRM_MODE_COLORIMETRY_BT2020_YCC);
> +
> void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
> struct amdgpu_dm_connector *aconnector,
> int connector_type,
> @@ -7318,6 +7324,15 @@ void amdgpu_dm_connector_init_helper(struct
> amdgpu_display_manager *dm,
> adev->mode_info.abm_level_property, 0);
> }
>
> + if (connector_type == DRM_MODE_CONNECTOR_HDMIA) {
> + if
> (!drm_mode_create_hdmi_colorspace_property(>base,
> supported_colorspaces))
> +
> drm_connector_attach_colorspace_property(>base);
> + } else if (connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> + connector_type == DRM_MODE_CONNECTOR_eDP) {
> + if
> (!drm_mode_create_dp_colorspace_property(>base,
> supported_colorspaces))
> +
> drm_connector_attach_colorspace_property(>base);
> + }
> +
> if (connector_type == DRM_MODE_CONNECTOR_HDMIA ||
> connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> connector_type == DRM_MODE_CONNECTOR_eDP) {
> --
> 2.40.1
>
Re: [PATCH v4 02/13] drm/connector: Add enum documentation to drm_colorspace
On Thu, May 25, 2023 at 9:18 PM Harry Wentland wrote:
>
> From: Joshua Ashton
>
> To match the other enums, and add more information about these values.
>
> v2:
> - Specify where an enum entry comes from
> - Clarify DEFAULT and NO_DATA behavior
> - BT.2020 CYCC is "constant luminance"
> - correct type for BT.601
>
> v4:
> - drop DP/HDMI clarifications that might create
> more questions than answers
>
> Signed-off-by: Joshua Ashton
> Signed-off-by: Harry Wentland
> Reviewed-by: Harry Wentland
>
> Cc: Pekka Paalanen
> Cc: Sebastian Wick
> Cc: vitaly.pros...@amd.com
> Cc: Uma Shankar
> Cc: Ville Syrjälä
> Cc: Joshua Ashton
> Cc: Simon Ser
> Cc: Ville Syrjälä
> Cc: Melissa Wen
> Cc: dri-de...@lists.freedesktop.org
> Cc: amd-gfx@lists.freedesktop.org
> ---
> include/drm/drm_connector.h | 62 +++--
> 1 file changed, 60 insertions(+), 2 deletions(-)
>
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 77401e425341..ee597593d7e6 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -363,13 +363,71 @@ enum drm_privacy_screen_status {
> PRIVACY_SCREEN_ENABLED_LOCKED,
> };
>
> -/*
> - * This is a consolidated colorimetry list supported by HDMI and
> +/**
> + * enum drm_colorspace - color space
> + *
> + * This enum is a consolidated colorimetry list supported by HDMI and
> * DP protocol standard. The respective connectors will register
> * a property with the subset of this list (supported by that
> * respective protocol). Userspace will set the colorspace through
> * a colorspace property which will be created and exposed to
> * userspace.
> + *
> + * DP definitions come from the DP v2.0 spec
> + * HDMI definitions come from the CTA-861-H spec
> + *
> + * @DRM_MODE_COLORIMETRY_DEFAULT:
> + * Driver specific behavior.
> + * @DRM_MODE_COLORIMETRY_NO_DATA:
> + * Driver specific behavior.
TBH this is still confusing me. Is DEFAULT really just driver specific
behavior? What's the difference to NO_DATA? Is NO_DATA actually not
driver-specific but display-specific? I.e. the default colorimetry of
the display?
> + * @DRM_MODE_COLORIMETRY_SMPTE_170M_YCC:
> + * (HDMI)
> + * SMPTE ST 170M colorimetry format
> + * @DRM_MODE_COLORIMETRY_BT709_YCC:
> + * (HDMI, DP)
> + * ITU-R BT.709 colorimetry format
> + * @DRM_MODE_COLORIMETRY_XVYCC_601:
> + * (HDMI, DP)
> + * xvYCC601 colorimetry format
> + * @DRM_MODE_COLORIMETRY_XVYCC_709:
> + * (HDMI, DP)
> + * xvYCC709 colorimetry format
> + * @DRM_MODE_COLORIMETRY_SYCC_601:
> + * (HDMI, DP)
> + * sYCC601 colorimetry format
> + * @DRM_MODE_COLORIMETRY_OPYCC_601:
> + * (HDMI, DP)
> + * opYCC601 colorimetry format
> + * @DRM_MODE_COLORIMETRY_OPRGB:
> + * (HDMI, DP)
> + * opRGB colorimetry format
> + * @DRM_MODE_COLORIMETRY_BT2020_CYCC:
> + * (HDMI, DP)
> + * ITU-R BT.2020 Y'c C'bc C'rc (constant luminance) colorimetry format
> + * @DRM_MODE_COLORIMETRY_BT2020_RGB:
> + * (HDMI, DP)
> + * ITU-R BT.2020 R' G' B' colorimetry format
> + * @DRM_MODE_COLORIMETRY_BT2020_YCC:
> + * (HDMI, DP)
> + * ITU-R BT.2020 Y' C'b C'r colorimetry format
> + * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
> + * (HDMI)
> + * SMPTE ST 2113 P3D65 colorimetry format
> + * @DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
> + * (HDMI)
> + * SMPTE ST 2113 P3DCI colorimetry format
> + * @DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED:
> + * (DP)
> + * RGB wide gamut fixed point colorimetry format
> + * @DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT:
> + * (DP)
> + * RGB wide gamut floating point
> + * (scRGB (IEC 61966-2-2)) colorimetry format
> + * @DRM_MODE_COLORIMETRY_BT601_YCC:
> + * (DP)
> + * ITU-R BT.601 colorimetry format
> + * The DP spec does not say whether this is the 525 or the 625
> + * line version.
> */
> enum drm_colorspace {
> /* For Default case, driver will set the colorspace */
> --
> 2.40.1
>
Re: [PATCH v4 00/13] Enable Colorspace connector property in amdgpu
On Fri, May 26, 2023 at 3:16 PM Pekka Paalanen wrote: > > On Thu, 25 May 2023 15:17:56 -0400 > Harry Wentland wrote: > > > This patchset is based on Joshua's previous patchset [1], as well > > as my previous patchset [2]. > > > > It is > > - enabling support for the colorspace property in amdgpu, as well as > > - allowing drivers to specify the supported set of colorspaces, and > > > > Colorspace, Infoframes, and YCbCr matrix > > --- > > > > Even though the initial intent of the colorspace property was to set the > > colorspace field in the respective HDMI AVI and DP SDP infoframes that > > is not sufficient in all scenarios. For DP the colorspace information > > also affects the MSA (main stream attribute) packet. For YUV output the > > colorspace affects the RGB-to-YCbCr conversion matrix. The colorspace > > field of the infopackets also depends on the encoding used, which is > > something that is decided by the driver and not known to userspace. > > Hi Harry, > > the "deprecation" of BT2020 RGB vs. YCC is now dropped completely. > Should there still be a patch that adds some UAPI documentation only, > saying that drivers are free to swap e.g. between BT2020 RGB and YCC > based which encoding they actually chose? Yes please! > Even if just BT2020 variant specifically. > > I have nothing against with this series now. > > > Thanks, > pq > > > > > For these reasons a driver will need to be able to select the supported > > colorspaces at property creation. > > > > Note: There seems to be an understanding that the colorspace property > > should ONLY modify the infoframe. While this is current behavior and > > sufficient in some cases it is nowhere specified that this should be the > > only use of this property. As outlined above this limitation is not > > going to work in all cases. > > > > This patchset does not affect current behavior for the drivers that > > implement this property: i915 and vc4. > > > > In the future we might want to give userspace control over the encoding > > format on the wire, in particular to avoid use of YUV420 when image > > fidelity is important. This work would likely go hand in hand with a > > min_bpc property and wouldn't conflict with the work done in this > > patchset. I would expect this future work to tag along with a drm_crtc > > or drm_connector's Color Pipeline, similar to the one propsed for > > drm_plane [3]. > > > > Colorspace on crtc or connector? > > > > > > There have been suggestions of programming 'colorspace' on the drm_crtc > > but I don't think the crtc is the right place for this property. The > > drm_plane and drm_crtc will be used to offload color processing that > > would normally be done via the GFX or other pipelines. The drm_connector > > controls the signalling with the display and ensures the wire format is > > appropriate for the encoding by programming the RGB-to-YCbCr matrix. > > > > [1] https://patchwork.freedesktop.org/series/113632/ > > [2] https://patchwork.freedesktop.org/series/111865/ > > [3] https://lists.freedesktop.org/archives/dri-devel/2023-May/403173.html
Re: [PATCH v2 00/21] Enable Colorspace connector property in amdgpu
FWIW, I still think this series is good (minus the UAPI changes) and would allow us to work on user space HDR support without custom kernels. On Fri, Jan 13, 2023 at 5:24 PM Harry Wentland wrote: > > This patchset enables the DP and HDMI infoframe properties > in amdgpu. > > The first two patches are not completely related to the rest. The > first patch allows for HDR_OUTPUT_METADATA with EOTFs that are > unknown in the kernel. > > The second one prints a connector's max_bpc as part of the atomic > state debugfs print. > > The following patches rework the connector colorspace code to > 1) allow for easy printing of the colorspace in the drm_atomic >state debugfs, and > 2) allow drivers to specify the supported colorspaces on a >connector. > > The rest of the patches deal with the Colorspace enablement > in amdgpu. > > Why do drivers need to specify supported colorspaces? The amdgpu > driver needs support for RGB-to-YCbCr conversion when we drive > the display in YCbCr. This is currently not implemented for all > colorspaces. > > Since the Colorspace property didn't have an IGT test I added > one to kms_hdr. The relevant patchset can be found on the IGT > mailing list or on > https://gitlab.freedesktop.org/hwentland/igt-gpu-tools/-/tree/hdr-colorimetry > > We tested v1 of the patchset and confirmed that the infoframes > are as expected for both DP and HDMI when running the IGT > colorimetry tests. > > Open Items > -- > > A couple comments from Pekka about colorspace documentation are > left unaddressed. I hope they won't block merging this set but > should still be addressed separately. > > Pekka's questions really got me thinking of how this colorspace > property should be used and working with it more closely with > Joshua who is enabling HDR in gamescope made me wonder even more. > > Uma, is there a (canonical, upstream) userspace that uses this > property that I can look at to understand more? > > One of the key challenges that is currently not addressed is that > userspace is expected to pick a colorspace format straight from the > list of definitions out of the DP or HDMI spec. But the kernel > driver are the ones deciding on the output encoding (RGB, YCBCR444, > YCBCR420, etc.). So there is no way for userspace to decide correctly > between, for example, BT2020_RGB, BT2020_CYCC, BT2020_YCC. > > So we end up in a scenario where gamescope sets BT2020_RGB but we > output YCBCR444 so have to correct the colorspace value to > BT2020_YCC. This in turn breaks the colorspace IGT tests I > wrote. I don't think "fixing" the IGT tests to accept this is > the right thing to do. > > The way it stands this patchset allows us to specify the output > colorspace on amdgpu and we try to do the right thing, but I don't > thing the way the colorspace property is defined is right. We're trying > to expose things to userspace that should be under driver control. A > much better approach would be to give userspace options for colorspace > that are not tied to DP or HDMI specs, i.e., sRGB, BT709, BT2020, etc., > and have the driver do the right thing to fill the infoframe, e.g., by > picking BT2020_YCC if the requested colorspace is BT2020 and the > is YCBCR444. > > If no upstream userspace currently makes use of this property I > can make that change, i.e., no longer tie the colorspace property > directly to the infoframe and reduce the options to sRGB, BT709, > BT601, and BT2020 (and possibly opRGB). > > v2: > - Tested with DP and HDMI analyzers > - Confirmed driver will fallback to lower bpc when needed > - Dropped hunk to set HDMI AVI infoframe as it was a no-op > - Fixed BT.2020 YCbCr colorimetry (JoshuaAshton) > - Simplify initialization of supported colorspaces (Jani) > - Fix kerneldoc (kernel test robot) > > Cc: Pekka Paalanen > Cc: Sebastian Wick > Cc: vitaly.pros...@amd.com > Cc: Uma Shankar > Cc: Ville Syrjälä > Cc: Joshua Ashton > Cc: Jani Nikula > Cc: Michel Dänzer > Cc: dri-de...@lists.freedesktop.org > Cc: amd-gfx@lists.freedesktop.org > > Harry Wentland (16): > drm/display: Don't block HDR_OUTPUT_METADATA on unknown EOTF > drm/connector: print max_requested_bpc in state debugfs > drm/connector: Drop COLORIMETRY_NO_DATA > drm/connector: Convert DRM_MODE_COLORIMETRY to enum > drm/connector: Pull out common create_colorspace_property code > drm/connector: Allow drivers to pass list of supported colorspaces > drm/connector: Print connector colorspace in state debugfs > drm/amd/display: Always pass connector_state to stream validation > drm/amd/display: Register Colorspace property for DP and HDMI > drm/amd/display: Signal mode_changed if color
Re: [PATCH v3 09/17] drm/amd/display: Register Colorspace property for DP and HDMI
On Fri, Mar 17, 2023 at 7:38 PM Ville Syrjälä wrote: > > On Fri, Mar 17, 2023 at 06:40:53PM +0100, Sebastian Wick wrote: > > On Fri, Mar 17, 2023 at 5:34 PM Ville Syrjälä > > wrote: > > > > > > On Fri, Mar 17, 2023 at 05:37:51PM +0200, Pekka Paalanen wrote: > > > > On Fri, 17 Mar 2023 16:14:38 +0200 > > > > Ville Syrjälä wrote: > > > > > > > > > On Fri, Mar 17, 2023 at 03:35:53PM +0200, Pekka Paalanen wrote: > > > > > > On Fri, 17 Mar 2023 14:50:40 +0200 > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > On Fri, Mar 17, 2023 at 10:53:35AM +0200, Pekka Paalanen wrote: > > > > > > > > On Fri, 17 Mar 2023 01:01:38 +0200 > > > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 10:13:54PM +0100, Sebastian Wick > > > > > > > > > wrote: > > > > > > > > > > On Thu, Mar 16, 2023 at 1:35 PM Ville Syrjälä > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 01:34:49PM +0200, Pekka Paalanen > > > > > > > > > > > wrote: > > > > > > > > > > > > On Thu, 16 Mar 2023 12:47:51 +0200 > > > > > > > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 12:07:01PM +0200, Pekka > > > > > > > > > > > > > Paalanen wrote: > > > > > > > > > > > > > > On Thu, 16 Mar 2023 11:50:27 +0200 > > > > > > > > > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 01:37:24AM +0100, > > > > > > > > > > > > > > > Sebastian Wick wrote: > > > > > > > > > > > > > > > > On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > We want compositors to be able to set the > > > > > > > > > > > > > > > > > output > > > > > > > > > > > > > > > > > colorspace on DP and HDMI outputs, based on > > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > caps reported from the receiver via EDID. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > About that... The documentation says that user > > > > > > > > > > > > > > > > space has to check the > > > > > > > > > > > > > > > > EDID for what the sink actually supports. So > > > > > > > > > > > > > > > > whatever is in > > > > > > > > > > > > > > > > supported_colorspaces is just what the > > > > > > > > > > > > > > > > driver/hardware is able to set > > > > > > > > > > > > > > > > but doesn't actually indicate that the sink > > > > > > > > > > > > > > > > supports it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > So the only way to enable bt2020 is by checking > > > > > > > > > > > > > > > > if the sink supports > > > > > > > > > > > > > > > > both RGB and YUV variants because both could be > > > > > > > > > > > > > > > > used by the driver. > > > > > > > > > > > > > > > > Not great at all. Something to remember for the > > > > > > > > > > > > > > > > new property. > >
Re: [PATCH v3 09/17] drm/amd/display: Register Colorspace property for DP and HDMI
On Fri, Mar 17, 2023 at 5:34 PM Ville Syrjälä wrote: > > On Fri, Mar 17, 2023 at 05:37:51PM +0200, Pekka Paalanen wrote: > > On Fri, 17 Mar 2023 16:14:38 +0200 > > Ville Syrjälä wrote: > > > > > On Fri, Mar 17, 2023 at 03:35:53PM +0200, Pekka Paalanen wrote: > > > > On Fri, 17 Mar 2023 14:50:40 +0200 > > > > Ville Syrjälä wrote: > > > > > > > > > On Fri, Mar 17, 2023 at 10:53:35AM +0200, Pekka Paalanen wrote: > > > > > > On Fri, 17 Mar 2023 01:01:38 +0200 > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 10:13:54PM +0100, Sebastian Wick wrote: > > > > > > > > On Thu, Mar 16, 2023 at 1:35 PM Ville Syrjälä > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 01:34:49PM +0200, Pekka Paalanen > > > > > > > > > wrote: > > > > > > > > > > On Thu, 16 Mar 2023 12:47:51 +0200 > > > > > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 12:07:01PM +0200, Pekka Paalanen > > > > > > > > > > > wrote: > > > > > > > > > > > > On Thu, 16 Mar 2023 11:50:27 +0200 > > > > > > > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 01:37:24AM +0100, Sebastian > > > > > > > > > > > > > Wick wrote: > > > > > > > > > > > > > > On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > We want compositors to be able to set the output > > > > > > > > > > > > > > > colorspace on DP and HDMI outputs, based on the > > > > > > > > > > > > > > > caps reported from the receiver via EDID. > > > > > > > > > > > > > > > > > > > > > > > > > > > > About that... The documentation says that user > > > > > > > > > > > > > > space has to check the > > > > > > > > > > > > > > EDID for what the sink actually supports. So > > > > > > > > > > > > > > whatever is in > > > > > > > > > > > > > > supported_colorspaces is just what the > > > > > > > > > > > > > > driver/hardware is able to set > > > > > > > > > > > > > > but doesn't actually indicate that the sink > > > > > > > > > > > > > > supports it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > So the only way to enable bt2020 is by checking if > > > > > > > > > > > > > > the sink supports > > > > > > > > > > > > > > both RGB and YUV variants because both could be > > > > > > > > > > > > > > used by the driver. > > > > > > > > > > > > > > Not great at all. Something to remember for the new > > > > > > > > > > > > > > property. > > > > > > > > > > > > > > > > > > > > > > > > > > Hmm. I wonder if that's even legal... Looks like > > > > > > > > > > > > > maybe it > > > > > > > > > > > > > is since I can't immediately spot anything in CTA-861 > > > > > > > > > > > > > to > > > > > > > > > > > > > forbid it :/ > > > > > > > > > > > > > > > > > > > > > > > > Wouldn't the driver do the same EDID check before > > > > > > > > > > > > choosing whether it > > > > > > > > > > > > uses RGB or YCbCr signalling? > > > &
Re: [PATCH v3 09/17] drm/amd/display: Register Colorspace property for DP and HDMI
On Thu, Mar 16, 2023 at 1:35 PM Ville Syrjälä wrote: > > On Thu, Mar 16, 2023 at 01:34:49PM +0200, Pekka Paalanen wrote: > > On Thu, 16 Mar 2023 12:47:51 +0200 > > Ville Syrjälä wrote: > > > > > On Thu, Mar 16, 2023 at 12:07:01PM +0200, Pekka Paalanen wrote: > > > > On Thu, 16 Mar 2023 11:50:27 +0200 > > > > Ville Syrjälä wrote: > > > > > > > > > On Thu, Mar 16, 2023 at 01:37:24AM +0100, Sebastian Wick wrote: > > > > > > On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland > > > > > > wrote: > > > > > > > > > > > > > > We want compositors to be able to set the output > > > > > > > colorspace on DP and HDMI outputs, based on the > > > > > > > caps reported from the receiver via EDID. > > > > > > > > > > > > About that... The documentation says that user space has to check > > > > > > the > > > > > > EDID for what the sink actually supports. So whatever is in > > > > > > supported_colorspaces is just what the driver/hardware is able to > > > > > > set > > > > > > but doesn't actually indicate that the sink supports it. > > > > > > > > > > > > So the only way to enable bt2020 is by checking if the sink supports > > > > > > both RGB and YUV variants because both could be used by the driver. > > > > > > Not great at all. Something to remember for the new property. > > > > > > > > > > Hmm. I wonder if that's even legal... Looks like maybe it > > > > > is since I can't immediately spot anything in CTA-861 to > > > > > forbid it :/ > > > > > > > > Wouldn't the driver do the same EDID check before choosing whether it > > > > uses RGB or YCbCr signalling? > > > > > > I suppose it could. The modeset would then fail, which is perhaps > > > > Could? What are they missing? > > The fact that the new property that also affects the rgb->ycbcr matrix > doesn't even exist? I think the question was about the current Colorspace property. > > > > I mean, drivers are already automatically choosing between RGB and YCbCr > > signalling based on e.g. available bandwidth. Surely they already will > > not attempt to send a signal format to a monitor that does not say it > > supports that? That's exactly what they do. The drivers don't check the EDID for the colorimetry the sink supports and the responsibility is punted off to user space. > > We just signal default/bt.709 colorimetry. There is nothing to > check for those IIRC. You do support bt.2020, no? > > > > > not a huge issue, except maybe for suspend+resume if we fail in > > > the resume path. Although I guess the EDID/etc. should not yet > > > be refreshed at that point so if the modeset worked before suspend > > > resume should be able to restore it without failures. > > > > I assumed that if a monitor can be driven, and it supports any BT2020 > > format, then it always supports the BT2020 format it is being driven > > in (RGB vs. YCbCr flavors). Bad assumption? > > I didn't spot any rule that both must be there. But didn't look > too hard either. Didn't see anything like that either and I looked a bit harder as well. > > -- > Ville Syrjälä > Intel >
Re: [PATCH v3 09/17] drm/amd/display: Register Colorspace property for DP and HDMI
On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland wrote:
>
> We want compositors to be able to set the output
> colorspace on DP and HDMI outputs, based on the
> caps reported from the receiver via EDID.
About that... The documentation says that user space has to check the
EDID for what the sink actually supports. So whatever is in
supported_colorspaces is just what the driver/hardware is able to set
but doesn't actually indicate that the sink supports it.
So the only way to enable bt2020 is by checking if the sink supports
both RGB and YUV variants because both could be used by the driver.
Not great at all. Something to remember for the new property.
> Signed-off-by: Harry Wentland
> Cc: Pekka Paalanen
> Cc: Sebastian Wick
> Cc: vitaly.pros...@amd.com
> Cc: Joshua Ashton
> Cc: dri-de...@lists.freedesktop.org
> Cc: amd-gfx@lists.freedesktop.org
> Reviewed-By: Joshua Ashton
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index f91b2ea13d96..2d883c6dae90 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -7184,6 +7184,12 @@ static int amdgpu_dm_connector_get_modes(struct
> drm_connector *connector)
> return amdgpu_dm_connector->num_modes;
> }
>
> +static const u32 supported_colorspaces =
> + BIT(DRM_MODE_COLORIMETRY_BT709_YCC) |
> + BIT(DRM_MODE_COLORIMETRY_OPRGB) |
> + BIT(DRM_MODE_COLORIMETRY_BT2020) |
> + BIT(DRM_MODE_COLORIMETRY_BT2020_DEPRECATED);
> +
> void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
> struct amdgpu_dm_connector *aconnector,
> int connector_type,
> @@ -7264,6 +7270,15 @@ void amdgpu_dm_connector_init_helper(struct
> amdgpu_display_manager *dm,
> adev->mode_info.abm_level_property, 0);
> }
>
> + if (connector_type == DRM_MODE_CONNECTOR_HDMIA) {
> + if
> (!drm_mode_create_hdmi_colorspace_property(>base,
> supported_colorspaces))
> +
> drm_connector_attach_colorspace_property(>base);
> + } else if (connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> + connector_type == DRM_MODE_CONNECTOR_eDP) {
> + if
> (!drm_mode_create_dp_colorspace_property(>base,
> supported_colorspaces))
> +
> drm_connector_attach_colorspace_property(>base);
> + }
> +
> if (connector_type == DRM_MODE_CONNECTOR_HDMIA ||
> connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> connector_type == DRM_MODE_CONNECTOR_eDP) {
> --
> 2.39.2
>
Re: [PATCH v3 02/17] drm/connector: Add enum documentation to drm_colorspace
On Thu, Mar 9, 2023 at 11:03 AM Pekka Paalanen wrote:
>
> On Thu, 9 Mar 2023 01:56:11 +0100
> Sebastian Wick wrote:
>
> > On Wed, Mar 8, 2023 at 9:59 AM Pekka Paalanen wrote:
> > >
> > > On Tue, 7 Mar 2023 10:10:52 -0500
> > > Harry Wentland wrote:
> > >
> > > > From: Joshua Ashton
> > > >
> > > > To match the other enums, and add more information about these values.
> > > >
> > > > v2:
> > > > - Specify where an enum entry comes from
> > > > - Clarify DEFAULT and NO_DATA behavior
> > > > - BT.2020 CYCC is "constant luminance"
> > > > - correct type for BT.601
> > > >
> > > > Signed-off-by: Joshua Ashton
> > > > Signed-off-by: Harry Wentland
> > > > Reviewed-by: Harry Wentland
> > >
> > > Hi,
> > >
> > > this effort is really good, but of course I still find things to
> > > nitpick about. If there is no answer to my questions, then I would
> > > prefer the documentation to spell out the unknowns and ambiguities.
> > >
> > > > Cc: Pekka Paalanen
> > > > Cc: Sebastian Wick
> > > > Cc: vitaly.pros...@amd.com
> > > > Cc: Uma Shankar
> > > > Cc: Ville Syrjälä
> > > > Cc: Joshua Ashton
> > > > Cc: dri-de...@lists.freedesktop.org
> > > > Cc: amd-gfx@lists.freedesktop.org
> > > > ---
> > > > include/drm/drm_connector.h | 67 +++--
> > > > 1 file changed, 65 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > index 6d6a53a6b010..bb078666dc34 100644
> > > > --- a/include/drm/drm_connector.h
> > > > +++ b/include/drm/drm_connector.h
> > > > @@ -363,13 +363,76 @@ enum drm_privacy_screen_status {
> > > > PRIVACY_SCREEN_ENABLED_LOCKED,
> > > > };
> > > >
> > > > -/*
> > > > - * This is a consolidated colorimetry list supported by HDMI and
> > > > +/**
> > > > + * enum drm_colorspace - color space
> > > > + *
> > > > + * This enum is a consolidated colorimetry list supported by HDMI and
> > > > * DP protocol standard. The respective connectors will register
> > > > * a property with the subset of this list (supported by that
> > > > * respective protocol). Userspace will set the colorspace through
> > > > * a colorspace property which will be created and exposed to
> > > > * userspace.
> > > > + *
> > > > + * DP definitions come from the DP v2.0 spec
> > > > + * HDMI definitions come from the CTA-861-H spec
> > > > + *
> > > > + * @DRM_MODE_COLORIMETRY_DEFAULT:
> > > > + * Driver specific behavior.
> > > > + * For DP:
> > > > + * RGB encoded: sRGB (IEC 61966-2-1)
> > > > + * YCbCr encoded: ITU-R BT.601 colorimetry format
> > >
> > > Does this mean that HDMI behavior is driver-specific while DP behavior
> > > is as defined?
> > >
> > > Is it intentional that YCbCr encoding also uses different RGB-primaries
> > > than RGB-encoded signal? (BT.601 vs. BT.709/sRGB)
> > >
> > > Or do you need to be more explicit on which parts of each spec apply
> > > (ColourPrimaries vs. TransferCharacteristics vs. MatrixCoefficients in
> > > CICP parlance)?
> > >
> > > E.g. BT.709/sRGB ColourPrimaries with BT.601 MatrixCoefficients.
> >
> > Yeah, just adding to this: The Default Colorspace is something well
> > defined. CTA-861 says:
> >
> > "If bits C0 and C1 are zero, the colorimetry shall correspond to the
> > default colorimetry defined in Section 5.1"
> >
> > and in Section 5.1
> >
> > "In all cases described above, the RGB color space used should be the
> > RGB color space the Sink declares in the Basic Display Parameters and
> > Feature Block of its EDID."
> >
> > If I set DRM_MODE_COLORIMETRY_DEFAULT, I expect the Colorimetry the
> > EDID reports to be in effect and not some driver specific nonsense.
>
> Does that also define the MatrixCoefficients for YCbCr signal with
> DRM_MODE_COLORIMETRY_DEFAULT?
Good question. It doesn't seem like it does, which would make
supporting YCC with the default color space impossible.
> Not that userspace would even care, since RGB-to-YCbCr is all
> driver-internal.
>
> It is interesting you point that out. I guess it means that the basic
> colorimetry from EDID is supposed to be really only the default
> colorimetry and might not have anything to do with the actual panel
> primaries.
>
>
> Thanks,
> pq
Re: [PATCH v3 14/17] drm/amd/display: Add debugfs for testing output colorspace
On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland wrote:
>
> In order to IGT test colorspace we'll want to print
> the currently enabled colorspace on a stream. We add
> a new debugfs to do so, using the same scheme as
> current bpc reporting.
>
> This might also come in handy when debugging display
> issues.
>
> Signed-off-by: Harry Wentland
> Cc: Pekka Paalanen
> Cc: Sebastian Wick
> Cc: vitaly.pros...@amd.com
> Cc: Joshua Ashton
> Cc: dri-de...@lists.freedesktop.org
> Cc: amd-gfx@lists.freedesktop.org
> Reviewed-By: Joshua Ashton
> ---
> .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 57 +++
> 1 file changed, 57 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> index 4a5dae578d97..f0022c16b708 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> @@ -906,6 +906,61 @@ static int amdgpu_current_bpc_show(struct seq_file *m,
> void *data)
> }
> DEFINE_SHOW_ATTRIBUTE(amdgpu_current_bpc);
>
> +/*
> + * Returns the current bpc for the crtc.
> + * Example usage: cat
> /sys/kernel/debug/dri/0/crtc-0/amdgpu_current_colorspace
> + */
> +static int amdgpu_current_colorspace_show(struct seq_file *m, void *data)
> +{
> + struct drm_crtc *crtc = m->private;
> + struct drm_device *dev = crtc->dev;
> + struct dm_crtc_state *dm_crtc_state = NULL;
> + int res = -ENODEV;
> +
> + mutex_lock(>mode_config.mutex);
> + drm_modeset_lock(>mutex, NULL);
> + if (crtc->state == NULL)
> + goto unlock;
> +
> + dm_crtc_state = to_dm_crtc_state(crtc->state);
> + if (dm_crtc_state->stream == NULL)
> + goto unlock;
> +
> + switch (dm_crtc_state->stream->output_color_space) {
> + case COLOR_SPACE_SRGB:
> + seq_printf(m, "RGB");
> + break;
Why does it print "RGB" when it says the color space is sRGB? Looking
at the value when I didn't specify any color space it says RGB. Why is
your default color space sRGB?
> + case COLOR_SPACE_YCBCR601:
> + case COLOR_SPACE_YCBCR601_LIMITED:
> + seq_printf(m, "BT601_YCC");
> + break;
> + case COLOR_SPACE_YCBCR709:
> + case COLOR_SPACE_YCBCR709_LIMITED:
> + seq_printf(m, "BT709_YCC");
> + break;
> + case COLOR_SPACE_ADOBERGB:
> + seq_printf(m, "opRGB");
> + break;
> + case COLOR_SPACE_2020_RGB_FULLRANGE:
> + seq_printf(m, "BT2020_RGB");
> + break;
> + case COLOR_SPACE_2020_YCBCR:
> + seq_printf(m, "BT2020_YCC");
> + break;
> + default:
> + goto unlock;
> + }
> + res = 0;
> +
> +unlock:
> + drm_modeset_unlock(>mutex);
> + mutex_unlock(>mode_config.mutex);
> +
> + return res;
> +}
> +DEFINE_SHOW_ATTRIBUTE(amdgpu_current_colorspace);
> +
> +
> /*
> * Example usage:
> * Disable dsc passthrough, i.e.,: have dsc decoding at converver, not
> external RX
> @@ -3235,6 +3290,8 @@ void crtc_debugfs_init(struct drm_crtc *crtc)
> #endif
> debugfs_create_file("amdgpu_current_bpc", 0644, crtc->debugfs_entry,
> crtc, _current_bpc_fops);
> + debugfs_create_file("amdgpu_current_colorspace", 0644,
> crtc->debugfs_entry,
> + crtc, _current_colorspace_fops);
> }
>
> /*
> --
> 2.39.2
>
Re: [PATCH v3 11/17] drm/amd/display: Send correct DP colorspace infopacket
On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland wrote:
>
> Look at connector->colorimetry to determine output colorspace.
>
> We don't want to impact current SDR behavior, so
> DRM_MODE_COLORIMETRY_DEFAULT preserves current behavior.
>
> Signed-off-by: Harry Wentland
> Cc: Pekka Paalanen
> Cc: Sebastian Wick
> Cc: vitaly.pros...@amd.com
> Cc: Joshua Ashton
> Cc: dri-de...@lists.freedesktop.org
> Cc: amd-gfx@lists.freedesktop.org
> Reviewed-By: Joshua Ashton
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 38 +++
> 1 file changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 58fc719bec8d..cdfd09d50ee6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5302,21 +5302,21 @@ get_aspect_ratio(const struct drm_display_mode
> *mode_in)
> }
>
> static enum dc_color_space
> -get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
> +get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
> + const struct drm_connector_state *connector_state)
> {
> enum dc_color_space color_space = COLOR_SPACE_SRGB;
>
> - switch (dc_crtc_timing->pixel_encoding) {
> - case PIXEL_ENCODING_YCBCR422:
> - case PIXEL_ENCODING_YCBCR444:
> - case PIXEL_ENCODING_YCBCR420:
> - {
> + switch (connector_state->colorspace) {
> + case DRM_MODE_COLORIMETRY_DEFAULT: // ITU601
So, I do get random behavior with DRM_MODE_COLORIMETRY_DEFAULT instead
of the colorimetry that the EDID specifies? That doesn't sound good at
all.
> + if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB) {
> + color_space = COLOR_SPACE_SRGB;
> /*
> * 27030khz is the separation point between HDTV and SDTV
> * according to HDMI spec, we use YCbCr709 and YCbCr601
> * respectively
> */
> - if (dc_crtc_timing->pix_clk_100hz > 270300) {
> + } else if (dc_crtc_timing->pix_clk_100hz > 270300) {
> if (dc_crtc_timing->flags.Y_ONLY)
> color_space =
> COLOR_SPACE_YCBCR709_LIMITED;
> @@ -5329,15 +5329,21 @@ get_output_color_space(const struct dc_crtc_timing
> *dc_crtc_timing)
> else
> color_space = COLOR_SPACE_YCBCR601;
> }
> -
> - }
> - break;
> - case PIXEL_ENCODING_RGB:
> - color_space = COLOR_SPACE_SRGB;
> break;
> -
> - default:
> - WARN_ON(1);
> + case DRM_MODE_COLORIMETRY_BT709_YCC:
> + if (dc_crtc_timing->flags.Y_ONLY)
> + color_space = COLOR_SPACE_YCBCR709_LIMITED;
> + else
> + color_space = COLOR_SPACE_YCBCR709;
> + break;
> + case DRM_MODE_COLORIMETRY_OPRGB:
> + color_space = COLOR_SPACE_ADOBERGB;
> + break;
> + case DRM_MODE_COLORIMETRY_BT2020:
> + color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
> + break;
> + case DRM_MODE_COLORIMETRY_BT2020_DEPRECATED:
> + color_space = COLOR_SPACE_2020_YCBCR;
> break;
> }
>
> @@ -5476,7 +5482,7 @@ static void
> fill_stream_properties_from_drm_display_mode(
> }
> }
>
> - stream->output_color_space = get_output_color_space(timing_out);
> + stream->output_color_space = get_output_color_space(timing_out,
> connector_state);
> }
>
> static void fill_audio_info(struct audio_info *audio_info,
> --
> 2.39.2
>
Re: [PATCH v3 05/17] drm/connector: Use common colorspace_names array
On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland wrote:
>
> We an use bitfields to track the support ones for HDMI
> and DP. This allows us to print colorspaces in a consistent
> manner without needing to know whether we're dealing with
> DP or HDMI.
>
> Signed-off-by: Harry Wentland
> Cc: Pekka Paalanen
> Cc: Sebastian Wick
> Cc: vitaly.pros...@amd.com
> Cc: Uma Shankar
> Cc: Ville Syrjälä
> Cc: Joshua Ashton
> Cc: Jani Nikula
> Cc: dri-de...@lists.freedesktop.org
> Cc: amd-gfx@lists.freedesktop.org
> ---
> drivers/gpu/drm/drm_connector.c | 131 +++-
> include/drm/drm_connector.h | 1 +
> 2 files changed, 78 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index ff4af48c029a..7649f0ac454f 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1012,64 +1012,70 @@ static const struct drm_prop_enum_list
> drm_dp_subconnector_enum_list[] = {
> DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
> drm_dp_subconnector_enum_list)
>
> -static const struct drm_prop_enum_list hdmi_colorspaces[] = {
> +
> +static const char * const colorspace_names[] = {
> /* For Default case, driver will set the colorspace */
> - { DRM_MODE_COLORIMETRY_DEFAULT, "Default" },
> + [DRM_MODE_COLORIMETRY_DEFAULT] = "Default",
> /* Standard Definition Colorimetry based on CEA 861 */
> - { DRM_MODE_COLORIMETRY_SMPTE_170M_YCC, "SMPTE_170M_YCC" },
> - { DRM_MODE_COLORIMETRY_BT709_YCC, "BT709_YCC" },
> + [DRM_MODE_COLORIMETRY_SMPTE_170M_YCC] = "SMPTE_170M_YCC",
> + [DRM_MODE_COLORIMETRY_BT709_YCC] = "BT709_YCC",
> /* Standard Definition Colorimetry based on IEC 61966-2-4 */
> - { DRM_MODE_COLORIMETRY_XVYCC_601, "XVYCC_601" },
> + [DRM_MODE_COLORIMETRY_XVYCC_601] = "XVYCC_601",
> /* High Definition Colorimetry based on IEC 61966-2-4 */
> - { DRM_MODE_COLORIMETRY_XVYCC_709, "XVYCC_709" },
> + [DRM_MODE_COLORIMETRY_XVYCC_709] = "XVYCC_709",
> /* Colorimetry based on IEC 61966-2-1/Amendment 1 */
> - { DRM_MODE_COLORIMETRY_SYCC_601, "SYCC_601" },
> + [DRM_MODE_COLORIMETRY_SYCC_601] = "SYCC_601",
> /* Colorimetry based on IEC 61966-2-5 [33] */
> - { DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
> + [DRM_MODE_COLORIMETRY_OPYCC_601] = "opYCC_601",
> /* Colorimetry based on IEC 61966-2-5 */
> - { DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
> + [DRM_MODE_COLORIMETRY_OPRGB] = "opRGB",
> /* Colorimetry based on ITU-R BT.2020 */
> - { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> + [DRM_MODE_COLORIMETRY_BT2020_CYCC] = "BT2020_CYCC",
> /* Colorimetry based on ITU-R BT.2020 */
> - { DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
> + [DRM_MODE_COLORIMETRY_BT2020] = "BT2020",
> /* Colorimetry based on ITU-R BT.2020 */
> - { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED, "BT2020_DEPRECATED" },
> - /* Added as part of Additional Colorimetry Extension in 861.G */
> - { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
> - { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-P3_RGB_Theater" },
> + [DRM_MODE_COLORIMETRY_BT2020_DEPRECATED] = "BT2020_DEPRECATED",
> + /* Colorimetry based on SMPTE RP 431-2 */
> + [DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65] = "P3_RGB_D65",
> + [DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER] = "P3_RGB_Theater",
> + [DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED] = "RGB_WIDE_FIXED",
> + /* Colorimetry based on scRGB (IEC 61966-2-2) */
> + [DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT] = "RGB_WIDE_FLOAT",
> + [DRM_MODE_COLORIMETRY_BT601_YCC] = "BT601_YCC",
> };
>
> +static const u32 hdmi_colorspaces =
> + BIT(DRM_MODE_COLORIMETRY_SMPTE_170M_YCC) |
> + BIT(DRM_MODE_COLORIMETRY_BT709_YCC) |
> + BIT(DRM_MODE_COLORIMETRY_XVYCC_601) |
> + BIT(DRM_MODE_COLORIMETRY_XVYCC_709) |
> + BIT(DRM_MODE_COLORIMETRY_SYCC_601) |
> + BIT(DRM_MODE_COLORIMETRY_OPYCC_601) |
> + BIT(DRM_MODE_COLORIMETRY_OPRGB) |
> + BIT(DRM_MODE_COLORIMETRY_BT2020_CYCC) |
> + BIT(DRM_MODE_COLORIMETRY_BT2020) |
> + BIT(DRM_MODE_COLORIMETRY_BT2020_DEPRECATED) |
> + BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65) |
> + BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER);
> +
&g
Re: [PATCH v3 03/17] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Wed, Mar 8, 2023 at 10:09 AM Pekka Paalanen wrote:
>
> On Tue, 7 Mar 2023 10:10:53 -0500
> Harry Wentland wrote:
>
> > From: Joshua Ashton
> >
> > Userspace has no way of controlling or knowing the pixel encoding
> > currently, so there is no way for it to ever get the right values here.
> >
> > When we do add pixel_encoding control from userspace,we can pick the
> > right value for the colorimetry packet based on the
> > pixel_encoding + the colorspace.
> >
> > Let's deprecate these values, and have one BT.2020 colorspace entry
> > that userspace can use.
> >
> > v2:
> > - leave CYCC alone for now; it serves a purpose
> > - leave BT2020_RGB the new default BT2020
> >
> > Signed-off-by: Joshua Ashton
> > Signed-off-by: Harry Wentland
> > Reviewed-by: Harry Wentland
> >
> > Cc: Pekka Paalanen
> > Cc: Sebastian Wick
> > Cc: vitaly.pros...@amd.com
> > Cc: Uma Shankar
> > Cc: Ville Syrjälä
> > Cc: Joshua Ashton
> > Cc: dri-de...@lists.freedesktop.org
> > Cc: amd-gfx@lists.freedesktop.org
> > ---
> > drivers/gpu/drm/display/drm_hdmi_helper.c | 7 +++
> > drivers/gpu/drm/drm_connector.c | 8
> > drivers/gpu/drm/i915/display/intel_dp.c | 14 +++---
> > include/drm/drm_connector.h | 15 +--
> > 4 files changed, 23 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > index faf5e9efa7d3..05a0d03ffcda 100644
> > --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > @@ -97,8 +97,7 @@ EXPORT_SYMBOL(drm_hdmi_infoframe_set_hdr_metadata);
> > #define HDMI_COLORIMETRY_OPYCC_601 (C(3) | EC(3) | ACE(0))
> > #define HDMI_COLORIMETRY_OPRGB (C(3) | EC(4) |
> > ACE(0))
> > #define HDMI_COLORIMETRY_BT2020_CYCC (C(3) | EC(5) | ACE(0))
> > -#define HDMI_COLORIMETRY_BT2020_RGB (C(3) | EC(6) | ACE(0))
> > -#define HDMI_COLORIMETRY_BT2020_YCC (C(3) | EC(6) | ACE(0))
> > +#define HDMI_COLORIMETRY_BT2020 (C(3) | EC(6) |
> > ACE(0))
> > #define HDMI_COLORIMETRY_DCI_P3_RGB_D65 (C(3) | EC(7) |
> > ACE(0))
> > #define HDMI_COLORIMETRY_DCI_P3_RGB_THEATER (C(3) | EC(7) | ACE(1))
> >
> > @@ -112,8 +111,8 @@ static const u32 hdmi_colorimetry_val[] = {
> > [DRM_MODE_COLORIMETRY_OPYCC_601] = HDMI_COLORIMETRY_OPYCC_601,
> > [DRM_MODE_COLORIMETRY_OPRGB] = HDMI_COLORIMETRY_OPRGB,
> > [DRM_MODE_COLORIMETRY_BT2020_CYCC] = HDMI_COLORIMETRY_BT2020_CYCC,
> > - [DRM_MODE_COLORIMETRY_BT2020_RGB] = HDMI_COLORIMETRY_BT2020_RGB,
> > - [DRM_MODE_COLORIMETRY_BT2020_YCC] = HDMI_COLORIMETRY_BT2020_YCC,
> > + [DRM_MODE_COLORIMETRY_BT2020_DEPRECATED] = HDMI_COLORIMETRY_BT2020,
> > + [DRM_MODE_COLORIMETRY_BT2020] = HDMI_COLORIMETRY_BT2020,
> > };
> >
> > #undef C
> > diff --git a/drivers/gpu/drm/drm_connector.c
> > b/drivers/gpu/drm/drm_connector.c
> > index 61c29ce74b03..fe7eab15f727 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -1031,9 +1031,9 @@ static const struct drm_prop_enum_list
> > hdmi_colorspaces[] = {
> > /* Colorimetry based on ITU-R BT.2020 */
> > { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> > /* Colorimetry based on ITU-R BT.2020 */
> > - { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> > + { DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
> > /* Colorimetry based on ITU-R BT.2020 */
> > - { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> > + { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED, "BT2020_DEPRECATED" },
> > /* Added as part of Additional Colorimetry Extension in 861.G */
> > { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
> > { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-P3_RGB_Theater" },
> > @@ -1054,7 +1054,7 @@ static const struct drm_prop_enum_list
> > dp_colorspaces[] = {
> > /* Colorimetry based on SMPTE RP 431-2 */
> > { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
> > /* Colorimetry based on ITU-R BT.2020 */
> > - { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> > + { DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
> > { DRM_MODE_COLORIMETRY_BT601_YCC, "BT601_YCC"
Re: [PATCH v3 02/17] drm/connector: Add enum documentation to drm_colorspace
On Wed, Mar 8, 2023 at 9:59 AM Pekka Paalanen wrote:
>
> On Tue, 7 Mar 2023 10:10:52 -0500
> Harry Wentland wrote:
>
> > From: Joshua Ashton
> >
> > To match the other enums, and add more information about these values.
> >
> > v2:
> > - Specify where an enum entry comes from
> > - Clarify DEFAULT and NO_DATA behavior
> > - BT.2020 CYCC is "constant luminance"
> > - correct type for BT.601
> >
> > Signed-off-by: Joshua Ashton
> > Signed-off-by: Harry Wentland
> > Reviewed-by: Harry Wentland
>
> Hi,
>
> this effort is really good, but of course I still find things to
> nitpick about. If there is no answer to my questions, then I would
> prefer the documentation to spell out the unknowns and ambiguities.
>
> > Cc: Pekka Paalanen
> > Cc: Sebastian Wick
> > Cc: vitaly.pros...@amd.com
> > Cc: Uma Shankar
> > Cc: Ville Syrjälä
> > Cc: Joshua Ashton
> > Cc: dri-de...@lists.freedesktop.org
> > Cc: amd-gfx@lists.freedesktop.org
> > ---
> > include/drm/drm_connector.h | 67 +++--
> > 1 file changed, 65 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 6d6a53a6b010..bb078666dc34 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -363,13 +363,76 @@ enum drm_privacy_screen_status {
> > PRIVACY_SCREEN_ENABLED_LOCKED,
> > };
> >
> > -/*
> > - * This is a consolidated colorimetry list supported by HDMI and
> > +/**
> > + * enum drm_colorspace - color space
> > + *
> > + * This enum is a consolidated colorimetry list supported by HDMI and
> > * DP protocol standard. The respective connectors will register
> > * a property with the subset of this list (supported by that
> > * respective protocol). Userspace will set the colorspace through
> > * a colorspace property which will be created and exposed to
> > * userspace.
> > + *
> > + * DP definitions come from the DP v2.0 spec
> > + * HDMI definitions come from the CTA-861-H spec
> > + *
> > + * @DRM_MODE_COLORIMETRY_DEFAULT:
> > + * Driver specific behavior.
> > + * For DP:
> > + * RGB encoded: sRGB (IEC 61966-2-1)
> > + * YCbCr encoded: ITU-R BT.601 colorimetry format
>
> Does this mean that HDMI behavior is driver-specific while DP behavior
> is as defined?
>
> Is it intentional that YCbCr encoding also uses different RGB-primaries
> than RGB-encoded signal? (BT.601 vs. BT.709/sRGB)
>
> Or do you need to be more explicit on which parts of each spec apply
> (ColourPrimaries vs. TransferCharacteristics vs. MatrixCoefficients in
> CICP parlance)?
>
> E.g. BT.709/sRGB ColourPrimaries with BT.601 MatrixCoefficients.
Yeah, just adding to this: The Default Colorspace is something well
defined. CTA-861 says:
"If bits C0 and C1 are zero, the colorimetry shall correspond to the
default colorimetry defined in Section 5.1"
and in Section 5.1
"In all cases described above, the RGB color space used should be the
RGB color space the Sink declares in the Basic Display Parameters and
Feature Block of its EDID."
If I set DRM_MODE_COLORIMETRY_DEFAULT, I expect the Colorimetry the
EDID reports to be in effect and not some driver specific nonsense.
> > + * @DRM_MODE_COLORIMETRY_NO_DATA:
> > + * Driver specific behavior.
> > + * For HDMI:
> > + * Sets "No Data" in infoframe
>
> Does DEFAULT mean that something else than "No Data" may be set in the
> HDMI infoframe?
>
> If so, since these two have the same value, where is the difference? Is
> DEFAULT purely an UAPI token, and NO_DATA used internally? Or NO_DATA
> used only when crafting actual infoframe packets?
>
> Should NO_DATA be documented to be a strictly driver-internal value,
> and not documented with UAPI?
>
> I am unclear if userspace is using these enum values directly, or do
> they use the string names only.
>
> > + * @DRM_MODE_COLORIMETRY_SMPTE_170M_YCC:
> > + * (HDMI)
> > + * SMPTE ST 170M colorimetry format
>
> Does "colorimetry format" mean that the spec is used in full, for all
> of ColourPrimaries, TransferCharacteristics and MatrixCoefficients?
>
> If yes, good. If not, the wording misleads me.
>
> > + * @DRM_MODE_COLORIMETRY_BT709_YCC:
> > + * (HDMI, DP)
> > + * ITU-R BT.709 colorimetry format
> > + * @DRM_MODE_COLORIMETRY_XVYCC_601:
> > + * (HDMI, DP)
> > + * xvYCC601 colorimetry format
> > + * @DRM_MODE_COLORIMETRY_XV
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Tue, Feb 14, 2023 at 9:10 PM Ville Syrjälä wrote: > > On Tue, Feb 14, 2023 at 08:45:00PM +0100, Sebastian Wick wrote: > > On Tue, Feb 14, 2023 at 5:57 PM Harry Wentland > > wrote: > > > > > > > > > > > > On 2/14/23 10:49, Sebastian Wick wrote: > > > > On Fri, Feb 3, 2023 at 5:00 PM Ville Syrjälä > > > > wrote: > > > >> > > > >> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote: > > > >>> > > > >>> > > > >>> On 2/3/23 10:19, Ville Syrjälä wrote: > > > >>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote: > > > >>>>> > > > >>>>> > > > >>>>> On 2/3/23 07:59, Sebastian Wick wrote: > > > >>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä > > > >>>>>> wrote: > > > >>>>>>> > > > >>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote: > > > >>>>>>>> Userspace has no way of controlling or knowing the pixel encoding > > > >>>>>>>> currently, so there is no way for it to ever get the right > > > >>>>>>>> values here. > > > >>>>>>> > > > >>>>>>> That applies to a lot of the other values as well (they are > > > >>>>>>> explicitly RGB or YCC). The idea was that this property sets the > > > >>>>>>> infoframe/MSA/SDP value exactly, and other properties should be > > > >>>>>>> added to for use userspace to control the pixel > > > >>>>>>> encoding/colorspace > > > >>>>>>> conversion(if desired, or userspace just makes sure to > > > >>>>>>> directly feed in correct kind of data). > > > >>>>>> > > > >>>>>> I'm all for getting userspace control over pixel encoding but even > > > >>>>>> then the kernel always knows which pixel encoding is selected and > > > >>>>>> which InfoFrame has to be sent. Is there a reason why userspace > > > >>>>>> would > > > >>>>>> want to control the variant explicitly to the wrong value? > > > >>>>>> > > > >>>>> > > > >>>>> I've asked this before but haven't seen an answer: Is there an > > > >>>>> existing > > > >>>>> upstream userspace project that makes use of this property (other > > > >>>>> than > > > >>>>> what Joshua is working on in gamescope right now)? That would help > > > >>>>> us > > > >>>>> understand the intent better. > > > >>>> > > > >>>> The intent was to control the infoframe colorimetry bits, > > > >>>> nothing more. No idea what real userspace there was, if any. > > > >>>> > > > >>>>> > > > >>>>> I don't think giving userspace explicit control over the exact > > > >>>>> infoframe > > > >>>>> values is the right thing to do. > > > >>>> > > > >>>> Only userspace knows what kind of data it's stuffing into > > > >>>> the pixels (and/or how it configures the csc units/etc.) to > > > >>>> generate them. > > > >>>> > > > >>> > > > >>> Yes, but userspace doesn't control or know whether we drive > > > >>> RGB or YCbCr on the wire. In fact, in some cases our driver > > > >>> needs to fallback to YCbCr420 for bandwidth reasons. There > > > >>> is currently no way for userspace to know that and I don't > > > >>> think it makes sense. > > > >> > > > >> People want that control as well for whatever reason. We've > > > >> been asked to allow YCbCr 4:4:4 output many times. > > > > > > > > I don't really think it's a question of if we want it but rather how > > > > we get there. Harry is completely right that if we would make the > > > > subsampling controllable by user space instead of the kernel handling > > > > it magically, user space which does not adapt to
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Tue, Feb 14, 2023 at 5:57 PM Harry Wentland wrote: > > > > On 2/14/23 10:49, Sebastian Wick wrote: > > On Fri, Feb 3, 2023 at 5:00 PM Ville Syrjälä > > wrote: > >> > >> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote: > >>> > >>> > >>> On 2/3/23 10:19, Ville Syrjälä wrote: > >>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote: > >>>>> > >>>>> > >>>>> On 2/3/23 07:59, Sebastian Wick wrote: > >>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä > >>>>>> wrote: > >>>>>>> > >>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote: > >>>>>>>> Userspace has no way of controlling or knowing the pixel encoding > >>>>>>>> currently, so there is no way for it to ever get the right values > >>>>>>>> here. > >>>>>>> > >>>>>>> That applies to a lot of the other values as well (they are > >>>>>>> explicitly RGB or YCC). The idea was that this property sets the > >>>>>>> infoframe/MSA/SDP value exactly, and other properties should be > >>>>>>> added to for use userspace to control the pixel encoding/colorspace > >>>>>>> conversion(if desired, or userspace just makes sure to > >>>>>>> directly feed in correct kind of data). > >>>>>> > >>>>>> I'm all for getting userspace control over pixel encoding but even > >>>>>> then the kernel always knows which pixel encoding is selected and > >>>>>> which InfoFrame has to be sent. Is there a reason why userspace would > >>>>>> want to control the variant explicitly to the wrong value? > >>>>>> > >>>>> > >>>>> I've asked this before but haven't seen an answer: Is there an existing > >>>>> upstream userspace project that makes use of this property (other than > >>>>> what Joshua is working on in gamescope right now)? That would help us > >>>>> understand the intent better. > >>>> > >>>> The intent was to control the infoframe colorimetry bits, > >>>> nothing more. No idea what real userspace there was, if any. > >>>> > >>>>> > >>>>> I don't think giving userspace explicit control over the exact infoframe > >>>>> values is the right thing to do. > >>>> > >>>> Only userspace knows what kind of data it's stuffing into > >>>> the pixels (and/or how it configures the csc units/etc.) to > >>>> generate them. > >>>> > >>> > >>> Yes, but userspace doesn't control or know whether we drive > >>> RGB or YCbCr on the wire. In fact, in some cases our driver > >>> needs to fallback to YCbCr420 for bandwidth reasons. There > >>> is currently no way for userspace to know that and I don't > >>> think it makes sense. > >> > >> People want that control as well for whatever reason. We've > >> been asked to allow YCbCr 4:4:4 output many times. > > > > I don't really think it's a question of if we want it but rather how > > we get there. Harry is completely right that if we would make the > > subsampling controllable by user space instead of the kernel handling > > it magically, user space which does not adapt to the new control won't > > be able to light up some modes which worked before. > > > > Thanks for continuing this discussion and touching on the model of how > we get to where we want to go. > > > This is obviously a problem and not one we can easily fix. We would > > need a new cap for user space to signal "I know that I can control > > bpc, subsampling and compression to lower the bandwidth and light up > > modes which otherwise fail". That cap would also remove all the > > properties which require kernel magic to work (that's also what I > > proposed for my KMS color pipeline API). > > > > We all want to expose more of the scanout capability and give user > > space more control but I don't think an incremental approach works > > here and we would all do better if we accept that the current API > > requires kernel magic to work and has a few implicit assumptions baked > > in. > > > > With all that being said, I think the
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Fri, Feb 3, 2023 at 5:00 PM Ville Syrjälä wrote: > > On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote: > > > > > > On 2/3/23 10:19, Ville Syrjälä wrote: > > > On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote: > > >> > > >> > > >> On 2/3/23 07:59, Sebastian Wick wrote: > > >>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä > > >>> wrote: > > >>>> > > >>>> On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote: > > >>>>> Userspace has no way of controlling or knowing the pixel encoding > > >>>>> currently, so there is no way for it to ever get the right values > > >>>>> here. > > >>>> > > >>>> That applies to a lot of the other values as well (they are > > >>>> explicitly RGB or YCC). The idea was that this property sets the > > >>>> infoframe/MSA/SDP value exactly, and other properties should be > > >>>> added to for use userspace to control the pixel encoding/colorspace > > >>>> conversion(if desired, or userspace just makes sure to > > >>>> directly feed in correct kind of data). > > >>> > > >>> I'm all for getting userspace control over pixel encoding but even > > >>> then the kernel always knows which pixel encoding is selected and > > >>> which InfoFrame has to be sent. Is there a reason why userspace would > > >>> want to control the variant explicitly to the wrong value? > > >>> > > >> > > >> I've asked this before but haven't seen an answer: Is there an existing > > >> upstream userspace project that makes use of this property (other than > > >> what Joshua is working on in gamescope right now)? That would help us > > >> understand the intent better. > > > > > > The intent was to control the infoframe colorimetry bits, > > > nothing more. No idea what real userspace there was, if any. > > > > > >> > > >> I don't think giving userspace explicit control over the exact infoframe > > >> values is the right thing to do. > > > > > > Only userspace knows what kind of data it's stuffing into > > > the pixels (and/or how it configures the csc units/etc.) to > > > generate them. > > > > > > > Yes, but userspace doesn't control or know whether we drive > > RGB or YCbCr on the wire. In fact, in some cases our driver > > needs to fallback to YCbCr420 for bandwidth reasons. There > > is currently no way for userspace to know that and I don't > > think it makes sense. > > People want that control as well for whatever reason. We've > been asked to allow YCbCr 4:4:4 output many times. I don't really think it's a question of if we want it but rather how we get there. Harry is completely right that if we would make the subsampling controllable by user space instead of the kernel handling it magically, user space which does not adapt to the new control won't be able to light up some modes which worked before. This is obviously a problem and not one we can easily fix. We would need a new cap for user space to signal "I know that I can control bpc, subsampling and compression to lower the bandwidth and light up modes which otherwise fail". That cap would also remove all the properties which require kernel magic to work (that's also what I proposed for my KMS color pipeline API). We all want to expose more of the scanout capability and give user space more control but I don't think an incremental approach works here and we would all do better if we accept that the current API requires kernel magic to work and has a few implicit assumptions baked in. With all that being said, I think the right decision here is to 1. Ignore subsampling for now 2. Let the kernel select YCC or RGB on the cable 3. Let the kernel figure out the conversion between RGB and YCC based on the color space selected 4. Let the kernel send the correct infoframe based on the selected color space and cable encoding 5. Only expose color spaces for which the kernel can do the conversion and send the infoframe 6. Work on the new API which is hidden behind a cap > The automagic 4:2:0 fallback I think is rather fundementally > incompatible with fancy color management. How would we even > know whether to use eg. BT.2020 vs. BT.709 matrix? In i915 > that stuff is just always BT.709 limited range, no questions > asked. > > So I think if userspace wants real color management it's > going to have to set up the whole pipeline. And for that > we
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Fri, Feb 3, 2023 at 2:35 PM Ville Syrjälä wrote: > > On Fri, Feb 03, 2023 at 01:59:07PM +0100, Sebastian Wick wrote: > > On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä > > wrote: > > > > > > On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote: > > > > Userspace has no way of controlling or knowing the pixel encoding > > > > currently, so there is no way for it to ever get the right values here. > > > > > > That applies to a lot of the other values as well (they are > > > explicitly RGB or YCC). The idea was that this property sets the > > > infoframe/MSA/SDP value exactly, and other properties should be > > > added to for use userspace to control the pixel encoding/colorspace > > > conversion(if desired, or userspace just makes sure to > > > directly feed in correct kind of data). > > > > I'm all for getting userspace control over pixel encoding but even > > then the kernel always knows which pixel encoding is selected and > > which InfoFrame has to be sent. Is there a reason why userspace would > > want to control the variant explicitly to the wrong value? > > What do you mean wrong value? Userspace sets it based on what > kind of data it has generated (or asked the display hardware > to generate if/when we get explicit control over that part). Wrong in the sense of sending the YCC variant when the pixel encoding is RGB for example. Maybe I'm missing something here but my assumption is that the kernel always has to know the pixel encoding anyway. The color pipeline also assumes that the pixel values are RGB. User space might be able to generate YCC content but for subsampling etc the pixel encoding still has to be explicitly set. So with the kernel always knowing exactly what pixel encoding is sent, why do we need those variants? I just don't see why this is necessary. > > -- > Ville Syrjälä > Intel >
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä
wrote:
>
> On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote:
> > Userspace has no way of controlling or knowing the pixel encoding
> > currently, so there is no way for it to ever get the right values here.
>
> That applies to a lot of the other values as well (they are
> explicitly RGB or YCC). The idea was that this property sets the
> infoframe/MSA/SDP value exactly, and other properties should be
> added to for use userspace to control the pixel encoding/colorspace
> conversion(if desired, or userspace just makes sure to
> directly feed in correct kind of data).
I'm all for getting userspace control over pixel encoding but even
then the kernel always knows which pixel encoding is selected and
which InfoFrame has to be sent. Is there a reason why userspace would
want to control the variant explicitly to the wrong value?
>
> >
> > When we do add pixel_encoding control from userspace,we can pick the
> > right value for the colorimetry packet based on the
> > pixel_encoding + the colorspace.
> >
> > Let's deprecate these values, and have one BT.2020 colorspace entry
> > that userspace can use.
> >
> > Note: _CYCC was effectively 'removed' by this change, but that was not
> > possible to be taken advantage of anyway, as there is currently no
> > pixel_encoding control so it would not be possible to output
> > linear YCbCr.
> >
> > Signed-off-by: Joshua Ashton
> >
> > Cc: Pekka Paalanen
> > Cc: Sebastian Wick
> > Cc: vitaly.pros...@amd.com
> > Cc: Uma Shankar
> > Cc: Ville Syrjälä
> > Cc: Joshua Ashton
> > Cc: dri-de...@lists.freedesktop.org
> > Cc: amd-gfx@lists.freedesktop.org
> > ---
> > drivers/gpu/drm/display/drm_hdmi_helper.c | 9 -
> > drivers/gpu/drm/drm_connector.c | 12 ++--
> > drivers/gpu/drm/i915/display/intel_dp.c | 20 +---
> > include/drm/drm_connector.h | 19 ++-
> > 4 files changed, 29 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > index 0264abe55278..c85860600395 100644
> > --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > @@ -99,8 +99,7 @@ EXPORT_SYMBOL(drm_hdmi_infoframe_set_hdr_metadata);
> > #define HDMI_COLORIMETRY_OPYCC_601 (C(3) | EC(3) | ACE(0))
> > #define HDMI_COLORIMETRY_OPRGB (C(3) | EC(4) |
> > ACE(0))
> > #define HDMI_COLORIMETRY_BT2020_CYCC (C(3) | EC(5) | ACE(0))
> > -#define HDMI_COLORIMETRY_BT2020_RGB (C(3) | EC(6) | ACE(0))
> > -#define HDMI_COLORIMETRY_BT2020_YCC (C(3) | EC(6) | ACE(0))
> > +#define HDMI_COLORIMETRY_BT2020 (C(3) | EC(6) |
> > ACE(0))
> > #define HDMI_COLORIMETRY_DCI_P3_RGB_D65 (C(3) | EC(7) |
> > ACE(0))
> > #define HDMI_COLORIMETRY_DCI_P3_RGB_THEATER (C(3) | EC(7) | ACE(1))
> >
> > @@ -113,9 +112,9 @@ static const u32 hdmi_colorimetry_val[] = {
> > [DRM_MODE_COLORIMETRY_SYCC_601] = HDMI_COLORIMETRY_SYCC_601,
> > [DRM_MODE_COLORIMETRY_OPYCC_601] = HDMI_COLORIMETRY_OPYCC_601,
> > [DRM_MODE_COLORIMETRY_OPRGB] = HDMI_COLORIMETRY_OPRGB,
> > - [DRM_MODE_COLORIMETRY_BT2020_CYCC] = HDMI_COLORIMETRY_BT2020_CYCC,
> > - [DRM_MODE_COLORIMETRY_BT2020_RGB] = HDMI_COLORIMETRY_BT2020_RGB,
> > - [DRM_MODE_COLORIMETRY_BT2020_YCC] = HDMI_COLORIMETRY_BT2020_YCC,
> > + [DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1] = HDMI_COLORIMETRY_BT2020,
> > + [DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2] = HDMI_COLORIMETRY_BT2020,
> > + [DRM_MODE_COLORIMETRY_BT2020] = HDMI_COLORIMETRY_BT2020,
> > };
> >
> > #undef C
> > diff --git a/drivers/gpu/drm/drm_connector.c
> > b/drivers/gpu/drm/drm_connector.c
> > index 61c29ce74b03..58699ab15a6a 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -1029,11 +1029,11 @@ static const struct drm_prop_enum_list
> > hdmi_colorspaces[] = {
> > /* Colorimetry based on IEC 61966-2-5 */
> > { DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
> > /* Colorimetry based on ITU-R BT.2020 */
> > - { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> > + { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1, "BT2020_DEPRECATED_1" },
> > /* Colorimetry based on ITU-R BT.2020 */
> > - { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB"
Re: [PATCH v2 18/21] drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not RGB
On Tue, Jan 24, 2023 at 7:57 PM Harry Wentland wrote: > > > > On 1/24/23 10:37, Harry Wentland wrote: > > > > > > On 1/23/23 15:30, Sebastian Wick wrote: > >> A new property to control YCC and subsampling would be the more > >> complete path here. If we actually want to fix this in the short-term > >> though, we should handle the YCC and RGB Colorspace values as > >> equivalent, everywhere. Technically we're breaking the user space API > >> here so it should be documented on the KMS property and other drivers > >> must be adjusted accordingly as well. > >> > > > > Could someone point me to a userspace that uses this currently? > > > > To elaborate a bit more... > > A driver has always had the ability to pick the wire format, whether > it'd be RGB or YCbCr (444, or 420). In some cases that selection > is required in order to satisfy bandwidth requirements. In others > we follow a certain policy to ensure similar behaviors between our > Windows and Linux drivers. I don't think it makes sense for userspace > to control this. I disagree. The subsampling is degrading the image considerably in some cases. We need control over this. It does mean that user space has to be smart and try to reduce the bandwidth if a KMS commit fails, but the same is true for resolution and refresh rate right now and will be true for a min bpc property as well. > Based on what I see I am not convinced the entirety of the > colorspace definition has a corresponding implementation in an > upstream, canonical userspace, hence my question. Not even an IGT > test existed when I started looking at this. In the absence of a > missing userspace implementation I am not convinced we're breaking > anything. Even then, this was never implemented in amdgpu so > there is no way this regresses any existing behavior. I don't think this breaks anything in practice. The change would only break use cases where you want to set the infoframe to a variant which does not match the wire format and that would be broken. So yes, I agree. We can't just remove the enum values though. If we deprecate the YCC variants that must be documented and user space has to understand that choosing the RGB variant will result in the kernel just doing the "right thing". > > Harry > > > Harry > > > >> On Fri, Jan 13, 2023 at 5:26 PM Harry Wentland > >> wrote: > >>> > >>> From: Joshua Ashton > >>> > >>> Userspace might not aware whether we're sending RGB or YCbCr > >>> data to the display. If COLOR_SPACE_2020_RGB_FULLRANGE is > >>> requested but the output encoding is YCbCr we should > >>> send COLOR_SPACE_2020_YCBCR. > >>> > >>> Signed-off-by: Joshua Ashton > >>> Signed-off-by: Harry Wentland > >>> Cc: Pekka Paalanen > >>> Cc: Sebastian Wick > >>> Cc: vitaly.pros...@amd.com > >>> Cc: Joshua Ashton > >>> Cc: dri-de...@lists.freedesktop.org > >>> Cc: amd-gfx@lists.freedesktop.org > >>> Reviewed-by: Harry Wentland > >>> --- > >>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 - > >>> 1 file changed, 4 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >>> index f74b125af31f..16940ea61b59 100644 > >>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >>> @@ -5184,7 +5184,10 @@ get_output_color_space(const struct dc_crtc_timing > >>> *dc_crtc_timing, > >>> color_space = COLOR_SPACE_ADOBERGB; > >>> break; > >>> case DRM_MODE_COLORIMETRY_BT2020_RGB: > >>> - color_space = COLOR_SPACE_2020_RGB_FULLRANGE; > >>> + if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB) > >>> + color_space = COLOR_SPACE_2020_RGB_FULLRANGE; > >>> + else > >>> + color_space = COLOR_SPACE_2020_YCBCR; > >>> break; > >>> case DRM_MODE_COLORIMETRY_BT2020_YCC: > >>> color_space = COLOR_SPACE_2020_YCBCR; > >>> -- > >>> 2.39.0 > >>> > >> > > >
Re: [PATCH v2 18/21] drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not RGB
On Wed, Jan 25, 2023 at 2:00 PM Joshua Ashton wrote: > > > > On 1/23/23 20:30, Sebastian Wick wrote: > > A new property to control YCC and subsampling would be the more > > complete path here. If we actually want to fix this in the short-term > > though, we should handle the YCC and RGB Colorspace values as > > equivalent, everywhere. Technically we're breaking the user space API > > here so it should be documented on the KMS property and other drivers > > must be adjusted accordingly as well. > > I am happy with treating 2020_YCC and 2020_RGB as the same. > > I think having the YCC/RGB split here in Colorspace is a mistake. > Pixel encoding should be completely separate from colorspace from a uAPI > perspective when we want to expose that. > It's just a design flaw from when it was added as it just mirrors the > values in the colorimetry packets 1:1. I understand why this happened, > and I don't think it's a big deal for us to correct ourselves now: > > I suggest we deprecate the _YCC variants, treat them the same as the RGB > enum to avoid any potential uAPI breakage and key the split entirely off > the pixel_encoding value. Yeah, I agree. The kernel must know the wire encoding and can thus always choose the correct variant. If anyone wants to provide a patch I'll review it. > That way when we do want to plumb more explicit pixel encoding down the > line, userspace only has to manage one thing. There's no advantage for > anything more here. > > - Joshie ✨ > > > > > On Fri, Jan 13, 2023 at 5:26 PM Harry Wentland > > wrote: > >> > >> From: Joshua Ashton > >> > >> Userspace might not aware whether we're sending RGB or YCbCr > >> data to the display. If COLOR_SPACE_2020_RGB_FULLRANGE is > >> requested but the output encoding is YCbCr we should > >> send COLOR_SPACE_2020_YCBCR. > >> > >> Signed-off-by: Joshua Ashton > >> Signed-off-by: Harry Wentland > >> Cc: Pekka Paalanen > >> Cc: Sebastian Wick > >> Cc: vitaly.pros...@amd.com > >> Cc: Joshua Ashton > >> Cc: dri-de...@lists.freedesktop.org > >> Cc: amd-gfx@lists.freedesktop.org > >> Reviewed-by: Harry Wentland > >> --- > >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 - > >> 1 file changed, 4 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >> index f74b125af31f..16940ea61b59 100644 > >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >> @@ -5184,7 +5184,10 @@ get_output_color_space(const struct dc_crtc_timing > >> *dc_crtc_timing, > >> color_space = COLOR_SPACE_ADOBERGB; > >> break; > >> case DRM_MODE_COLORIMETRY_BT2020_RGB: > >> - color_space = COLOR_SPACE_2020_RGB_FULLRANGE; > >> + if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB) > >> + color_space = COLOR_SPACE_2020_RGB_FULLRANGE; > >> + else > >> + color_space = COLOR_SPACE_2020_YCBCR; > >> break; > >> case DRM_MODE_COLORIMETRY_BT2020_YCC: > >> color_space = COLOR_SPACE_2020_YCBCR; > >> -- > >> 2.39.0 > >> > > >
Re: [PATCH v2 18/21] drm/amd/display: Fallback to 2020_YCBCR if the pixel encoding is not RGB
A new property to control YCC and subsampling would be the more complete path here. If we actually want to fix this in the short-term though, we should handle the YCC and RGB Colorspace values as equivalent, everywhere. Technically we're breaking the user space API here so it should be documented on the KMS property and other drivers must be adjusted accordingly as well. On Fri, Jan 13, 2023 at 5:26 PM Harry Wentland wrote: > > From: Joshua Ashton > > Userspace might not aware whether we're sending RGB or YCbCr > data to the display. If COLOR_SPACE_2020_RGB_FULLRANGE is > requested but the output encoding is YCbCr we should > send COLOR_SPACE_2020_YCBCR. > > Signed-off-by: Joshua Ashton > Signed-off-by: Harry Wentland > Cc: Pekka Paalanen > Cc: Sebastian Wick > Cc: vitaly.pros...@amd.com > Cc: Joshua Ashton > Cc: dri-de...@lists.freedesktop.org > Cc: amd-gfx@lists.freedesktop.org > Reviewed-by: Harry Wentland > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index f74b125af31f..16940ea61b59 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -5184,7 +5184,10 @@ get_output_color_space(const struct dc_crtc_timing > *dc_crtc_timing, > color_space = COLOR_SPACE_ADOBERGB; > break; > case DRM_MODE_COLORIMETRY_BT2020_RGB: > - color_space = COLOR_SPACE_2020_RGB_FULLRANGE; > + if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB) > + color_space = COLOR_SPACE_2020_RGB_FULLRANGE; > + else > + color_space = COLOR_SPACE_2020_YCBCR; > break; > case DRM_MODE_COLORIMETRY_BT2020_YCC: > color_space = COLOR_SPACE_2020_YCBCR; > -- > 2.39.0 >
Re: [PATCH 16/16] drm/amd/display: Don't restrict bpc to 8 bpc
On Fri, Dec 23, 2022 at 8:10 PM Harry Wentland wrote: > > > > On 12/14/22 04:01, Pekka Paalanen wrote: > > On Tue, 13 Dec 2022 18:20:59 +0100 > > Michel Dänzer wrote: > > > >> On 12/12/22 19:21, Harry Wentland wrote: > >>> This will let us pass kms_hdr.bpc_switch. > >>> > >>> I don't see any good reasons why we still need to > >>> limit bpc to 8 bpc and doing so is problematic when > >>> we enable HDR. > >>> > >>> If I remember correctly there might have been some > >>> displays out there where the advertised link bandwidth > >>> was not large enough to drive the default timing at > >>> max bpc. This would leave to an atomic commit/check > >>> failure which should really be handled in compositors > >>> with some sort of fallback mechanism. > >>> > >>> If this somehow turns out to still be an issue I > >>> suggest we add a module parameter to allow users to > >>> limit the max_bpc to a desired value. > >> > >> While leaving the fallback for user space to handle makes some sense > >> in theory, in practice most KMS display servers likely won't handle > >> it. > >> > >> Another issue is that if mode validation is based on the maximum bpc > >> value, it may reject modes which would work with lower bpc. > >> > >> > >> What Ville (CC'd) suggested before instead (and what i915 seems to be > >> doing already) is that the driver should do mode validation based on > >> the *minimum* bpc, and automatically make the effective bpc lower > >> than the maximum as needed to make the rest of the atomic state work. > > > > A driver is always allowed to choose a bpc lower than max_bpc, so it > > very well should do so when necessary due to *known* hardware etc. > > limitations. > > > > I spent a bunch of time to figure out how this actually pans out in > amdgpu and it looks like we're doing the right thing, i.e. if bandwidth > limitations require it we'll downgrade bpc appropriately. These changes > happened over the last couple years or so. So while raising the default > max_bpc wasn't safe in amdgpu years ago it is completely fine now. > > As for the relevant code it's mostly handled in > create_validate_stream_for_sink > in amdgpu_dm.c where we iterate over a stream's mode validation with > decreasing bpc if it fails (down to a bpc of 6). > > For HDMI we also have a separate adjust_colour_depth_from_display_info > function that downgrades bpc in order to fit within the max_tmds_clock. > > So, in short, this change should not lead to displays not lighting up > because we no longer force a given bpc. Very good! > > > So things like mode validation cannot just look at a single max or min > > bpc, but it needs to figure out if there is any usable bpc value that > > makes the mode work. > > > > The max_bpc knob exists only for the cases where the sink undetectably > > malfunctions unless the bpc is artificially limited more than seems > > necessary. That malfunction requires a human to detect, and reconfigure > > their system as we don't have a quirk database for this I think. > > > > The question of userspace wanting a specific bpc is a different matter > > and an unsolved one. It also ties to userspace wanting to use the > > current mode to avoid a mode switch between e.g. hand-off from firmware > > boot splash to proper userspace. That's also unsolved AFAIK. > > > > Agreed, the current "max bpc" just sets a max. We'd probably want a > "min bpc" if userspace needs a minimum (e.g., for HDR). To be clear: we need this. I've argued before that we need a min bpc setting because we'd rather not enable HDR if we can't get the bit depth required for it and there is no other mechanism to control this. The other remaining kernel problem for HDR is that we still have no control over YCC/RGB selection on the cable and thus can't choose the correct color space infoframe. > > Harry > > > OTOH, we have the discussion that concluded as > > https://gitlab.freedesktop.org/wayland/weston/-/issues/612#note_1359898 > > which really puts userspace in charge of max_bpc, so the driver-chosen > > default value does not have much impact as long as it makes the > > firmware-chosen video mode to continue, as requested in > > https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/995 > > given that userspace cannot know what the actual bpc currently is nor > > set the exact bpc to keep it the same. > > > > > > Thanks, > > pq >
Re: [RFC PATCH 0/3] A drm_plane API to support HDR planes
On 2021-05-18 16:19, Harry Wentland wrote: On 2021-05-18 3:56 a.m., Pekka Paalanen wrote: On Mon, 17 May 2021 15:39:03 -0400 Vitaly Prosyak wrote: On 2021-05-17 12:48 p.m., Sebastian Wick wrote: On 2021-05-17 10:57, Pekka Paalanen wrote: On Fri, 14 May 2021 17:05:11 -0400 Harry Wentland wrote: On 2021-04-27 10:50 a.m., Pekka Paalanen wrote: On Mon, 26 Apr 2021 13:38:49 -0400 Harry Wentland wrote: ... ## Mastering Luminances Now we are able to use the PQ 2084 EOTF to define the luminance of pixels in absolute terms. Unfortunately we're again presented with physical limitations of the display technologies on the market today. Here are a few examples of luminance ranges of displays. | Display | Luminance range in nits | | | --- | | Typical PC display | 0.3 - 200 | | Excellent LCD HDTV | 0.3 - 400 | | HDR LCD w/ local dimming | 0.05 - 1,500 | Since no display can currently show the full 0.0005 to 10,000 nits luminance range the display will need to tonemap the HDR content, i.e to fit the content within a display's capabilities. To assist with tonemapping HDR content is usually accompanied with a metadata that describes (among other things) the minimum and maximum mastering luminance, i.e. the maximum and minimum luminance of the display that was used to master the HDR content. The HDR metadata is currently defined on the drm_connector via the hdr_output_metadata blob property. It might be useful to define per-plane hdr metadata, as different planes might have been mastered differently. I don't think this would directly help with the dynamic range blending problem. You still need to establish the mapping between the optical values from two different EOTFs and dynamic ranges. Or can you know which optical values match the mastering display maximum and minimum luminances for not-PQ? My understanding of this is probably best illustrated by this example: Assume HDR was mastered on a display with a maximum white level of 500 nits and played back on a display that supports a max white level of 400 nits. If you know the mastering white level of 500 you know that this is the maximum value you need to compress down to 400 nits, allowing you to use the full extent of the 400 nits panel. Right, but in the kernel, where do you get these nits values from? hdr_output_metadata blob is infoframe data to the monitor. I think this should be independent of the metadata used for color transformations in the display pipeline before the monitor. EDID may tell us the monitor HDR metadata, but again what is used in the color transformations should be independent, because EDIDs lie, lighting environments change, and users have different preferences. What about black levels? Do you want to do black level adjustment? How exactly should the compression work? Where do you map the mid-tones? What if the end user wants something different? I suspect that this is not about tone mapping at all. The use cases listed always have the display in PQ mode and just assume that no content exceeds the PQ limitations. Then you can simply bring all content to the color space with a matrix multiplication and then map the linear light content somewhere into the PQ range. Tone mapping is performed in the display only. The use cases do use the word "desktop" though. Harry, could you expand on this, are you seeking a design that is good for generic desktop compositors too, or one that is more tailored to "embedded" video player systems taking the most advantage of (potentially fixed-function) hardware? The goal is to enable this on a generic desktop, such as generic Wayland implementations or ChromeOS. We're not looking for a custom solution for some embedded systems, though the solution we end up with should obviously not prevent an implementation on embedded video players. What matrix would one choose? Which render intent would it correspond to? If you need to adapt different dynamic ranges into the blending dynamic range, would a simple linear transformation really be enough? From a generic wayland compositor point of view this is uninteresting. It a compositor's decision to provide or not the metadata property to the kernel. The metadata can be available from one or multiple clients or most likely not available at all. Compositors may put a display in HDR10 ( when PQ 2084 INV EOTF and TM occurs in display ) or NATIVE mode and do not attach any metadata to the connector and do TM in compositor. It is all about user preference or compositor design, or a combination of both options. Indeed. The thing here is that you cannot just add KMS UAPI, you also need to have the FOSS userspace to go with it. So you need to have your audience defined, userspace patches written and reviewed and agreed to be a good idea. I'm afraid this particular UAPI design would be
Re: [RFC PATCH v2 1/6] drm/doc: Color Management and HDR10 RFC
On 2021-05-14 23:07, Harry Wentland wrote: Use the new DRM RFC doc section to capture the RFC previously only described in the cover letter at https://patchwork.freedesktop.org/series/89506/ Update the RFC based on feedback received: * don't use color_encoding property to define color space * expand on reason for SDR luminance definition * define input/output transfer functions for luminance space transforms, rather than defining the luminance space of an input directly Signed-off-by: Harry Wentland --- Documentation/gpu/rfc/hdr-wide-gamut.rst | 416 +++ Documentation/gpu/rfc/index.rst | 4 + 2 files changed, 420 insertions(+) create mode 100644 Documentation/gpu/rfc/hdr-wide-gamut.rst diff --git a/Documentation/gpu/rfc/hdr-wide-gamut.rst b/Documentation/gpu/rfc/hdr-wide-gamut.rst new file mode 100644 index ..132898668eac --- /dev/null +++ b/Documentation/gpu/rfc/hdr-wide-gamut.rst @@ -0,0 +1,416 @@ +== +HDR & Wide Color Gamut Support +== + +.. role:: wy-text-strike + +ToDo + + +* :wy-text-strike:`Reformat as RST kerneldoc` - done +* :wy-text-strike:`Don't use color_encoding for color_space definitions` - done +* :wy-text-strike:`Update SDR luminance description and reasoning` - done +* :wy-text-strike:`Clarify 3D LUT required for some color space transformations` - done +* :wy-text-strike:`Highlight need for named color space and EOTF definitions` - done +* :wy-text-strike:`Define transfer function API` - done +* :wy-text-strike:`Draft upstream plan` - done +* Reference to wayland and/or Chrome plans +* Sketch view of HW pipeline for couple of HW implementations + + +Upstream Plan += + +* Reach consensus on DRM/KMS API +* Implement support in amdgpu +* Implement IGT tests +* Add API support to Weston, ChromiumOS, or other canonical open-source project interested in HDR +* Merge user-space +* Merge kernel patches + + +Introduction + + +We are looking to enable HDR support for a couple of single-plane and +multi-plane scenarios. To do this effectively we recommend new interfaces +to drm_plane. Below I'll give a bit of background on HDR and why we +propose these interfaces. + + +Overview and background +=== + +Defining a pixel's luminance + + +The luminance space of pixels in a framebuffer/plane presented to the +display is not well defined in the DRM/KMS APIs. It is usually assumed to +be in a 2.2 or 2.4 gamma space and has no mapping to an absolute luminance +value; it is interpreted in relative terms. + +Luminance can be measured and described in absolute terms as candela +per meter squared, or cd/m2, or nits. Even though a pixel value can be +mapped to luminance in a linear fashion to do so without losing a lot of +detail requires 16-bpc color depth. The reason for this is that human +perception can distinguish roughly between a 0.5-1% luminance delta. A +linear representation is suboptimal, wasting precision in the highlights +and losing precision in the shadows. + +A gamma curve is a decent approximation to a human's perception of +luminance, but the PQ (perceptual quantizer) function [1] improves on +it. It also defines the luminance values in absolute terms, with the +highest value being 10,000 nits and the lowest 0.0005 nits. + +Using a content that's defined in PQ space we can approximate the real +world in a much better way. + +Here are some examples of real-life objects and their approximate +luminance values: + +.. flat-table:: + :header-rows: 1 + + * - Object + - Luminance in nits + + * - Fluorescent light + - 10,000 + + * - Highlights + - 1,000 - sunlight + + * - White Objects + - 250 - 1,000 + + * - Typical Objects + - 1 - 250 + + * - Shadows + - 0.01 - 1 + + * - Ultra Blacks + - 0 - 0.0005 + +Transfer functions +-- + +Traditionally we used the terms gamma and de-gamma to describe the +encoding of a pixel's luminance value and the operation to transfer from +a linear luminance space to the non-linear space used to encode the +pixels. Since some newer encodings don't use a gamma curve I suggest +we refer to non-linear encodings using the terms EOTF, and OETF[2], or +simply as transfer function in general. + +The EOTF (Electro-Optical Transfer Function) describes how to transfer +from an electrical signal to an optical signal. This was traditionally +done by the de-gamma function. + +The OETF (Opto Electronic Transfer Function) describes how to transfer +from an optical signal to an electronic signal. This was traditionally +done by the gamma function. + +More generally we can name the transfer function describing the transform +between scanout and blending space as the **input transfer function**, and +the transfer function describing the transform from blending space to the +output space as **output transfer
Re: [RFC PATCH 0/3] A drm_plane API to support HDR planes
On 2021-05-17 10:57, Pekka Paalanen wrote: On Fri, 14 May 2021 17:05:11 -0400 Harry Wentland wrote: On 2021-04-27 10:50 a.m., Pekka Paalanen wrote: > On Mon, 26 Apr 2021 13:38:49 -0400 > Harry Wentland wrote: ... >> ## Mastering Luminances >> >> Now we are able to use the PQ 2084 EOTF to define the luminance of >> pixels in absolute terms. Unfortunately we're again presented with >> physical limitations of the display technologies on the market today. >> Here are a few examples of luminance ranges of displays. >> >> | Display | Luminance range in nits | >> | | --- | >> | Typical PC display | 0.3 - 200 | >> | Excellent LCD HDTV | 0.3 - 400 | >> | HDR LCD w/ local dimming | 0.05 - 1,500| >> >> Since no display can currently show the full 0.0005 to 10,000 nits >> luminance range the display will need to tonemap the HDR content, i.e >> to fit the content within a display's capabilities. To assist with >> tonemapping HDR content is usually accompanied with a metadata that >> describes (among other things) the minimum and maximum mastering >> luminance, i.e. the maximum and minimum luminance of the display that >> was used to master the HDR content. >> >> The HDR metadata is currently defined on the drm_connector via the >> hdr_output_metadata blob property. >> >> It might be useful to define per-plane hdr metadata, as different >> planes might have been mastered differently. > > I don't think this would directly help with the dynamic range blending > problem. You still need to establish the mapping between the optical > values from two different EOTFs and dynamic ranges. Or can you know > which optical values match the mastering display maximum and minimum > luminances for not-PQ? > My understanding of this is probably best illustrated by this example: Assume HDR was mastered on a display with a maximum white level of 500 nits and played back on a display that supports a max white level of 400 nits. If you know the mastering white level of 500 you know that this is the maximum value you need to compress down to 400 nits, allowing you to use the full extent of the 400 nits panel. Right, but in the kernel, where do you get these nits values from? hdr_output_metadata blob is infoframe data to the monitor. I think this should be independent of the metadata used for color transformations in the display pipeline before the monitor. EDID may tell us the monitor HDR metadata, but again what is used in the color transformations should be independent, because EDIDs lie, lighting environments change, and users have different preferences. What about black levels? Do you want to do black level adjustment? How exactly should the compression work? Where do you map the mid-tones? What if the end user wants something different? I suspect that this is not about tone mapping at all. The use cases listed always have the display in PQ mode and just assume that no content exceeds the PQ limitations. Then you can simply bring all content to the color space with a matrix multiplication and then map the linear light content somewhere into the PQ range. Tone mapping is performed in the display only. From a generic wayland compositor point of view this is uninteresting. I completely agree with what you said below though. I would even argue that all generic KMS abstract pipeline elements must have a well defined place in the pipeline and follow an exact specified formula. If you do not know the mastering luminance is 500 nits you would have to compress 10,000 nits down to 400 (assuming PQ), losing quite a bit of the full 400 nits available as you'll need room to map the 500 to 10,000 nits range which in reality is completely unused. You might end up with mapping 500 nits to 350 nits, instead of mapping it to 400. The quality of the result depends on the compression (tone-mapping) algorithm. I believe no-one will ever do a simple linear compression of ranges. Instead, you probably do something smooth in the black end, keep mid-tones roughly as they are, and the again do a smooth saturation to some "reasonable" level that goes well with the monitor in the current lighting environment without introducing coloring artifacts, and just clip the huge overshoot of the full PQ-range. There are many big and small decisions to be made in how to map out-of-gamut or out-of-brightness values into the displayable range, and no algorithm fits all use cases. I believe this is why e.g. ICC has several different "render intents", some of which are so vaguely defined that they are practically undefined - just like what "a good picture" means. You have essentially three dimensions: luminance, hue, and saturation. Which one will you sacrifice, shift or emphasize and to what degree to fit the square content peg into the round display hole? A naive example: Let's say content has 300 nits red. Your display can
Re: [RFC PATCH 1/3] drm/color: Add RGB Color encodings
On 2021-04-26 20:56, Harry Wentland wrote:
On 2021-04-26 2:07 p.m., Ville Syrjälä wrote:
On Mon, Apr 26, 2021 at 01:38:50PM -0400, Harry Wentland wrote:
From: Bhawanpreet Lakha
Add the following color encodings
- RGB versions for BT601, BT709, BT2020
- DCI-P3: Used for digital movies
Signed-off-by: Bhawanpreet Lakha
Signed-off-by: Harry Wentland
---
drivers/gpu/drm/drm_color_mgmt.c | 4
include/drm/drm_color_mgmt.h | 4
2 files changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/drm_color_mgmt.c
b/drivers/gpu/drm/drm_color_mgmt.c
index bb14f488c8f6..a183ebae2941 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -469,6 +469,10 @@ static const char * const color_encoding_name[]
= {
[DRM_COLOR_YCBCR_BT601] = "ITU-R BT.601 YCbCr",
[DRM_COLOR_YCBCR_BT709] = "ITU-R BT.709 YCbCr",
[DRM_COLOR_YCBCR_BT2020] = "ITU-R BT.2020 YCbCr",
+ [DRM_COLOR_RGB_BT601] = "ITU-R BT.601 RGB",
+ [DRM_COLOR_RGB_BT709] = "ITU-R BT.709 RGB",
+ [DRM_COLOR_RGB_BT2020] = "ITU-R BT.2020 RGB",
+ [DRM_COLOR_P3] = "DCI-P3",
These are a totally different thing than the YCbCr stuff.
The YCbCr stuff just specifies the YCbCr<->RGB converison matrix,
whereas these are I guess supposed to specify the
primaries/whitepoint?
But without specifying what we're converting *to* these mean
absolutely
nothing. Ie. I don't think they belong in this property.
If this is the intention I don't see it documented.
I might have overlooked something but do we have a way to explicitly
specify today what *to* format the YCbCr color encodings convert into?
Would that be a combination of the output color encoding specified via
colorspace_property and the color space encoded in the primaries and
whitepoint of the hdr_output_metadata?
Fundamentally I don't see how the use of this property differs,
whether you translate from YCbCr or from RGB. In either case you're
converting from the defined input color space and pixel format to an
output color space and pixel format.
The previous proposals around this topic have suggested a new
property to specify a conversion matrix either explicitly, or
via a separate enum (which would specify both the src and dst
colorspaces). I've always argued the enum approach is needed
anyway since not all hardware has a programmable matrix for
this. But a fully programmable matrix could be nice for tone
mapping purposes/etc, so we may want to make sure both are
possible.
As for the transfer func, the proposals so far have mostly just
been to expose a programmable degamma/gamma LUTs for each plane.
But considering how poor the current gamma uapi is we've thrown
around some ideas how to allow the kernel to properly expose the
hw capabilities. This is one of those ideas:
https://lists.freedesktop.org/archives/dri-devel/2019-April/212886.html>>
I think that basic idea could be also be extended to allow fixed
curves in case the hw doesn't have a fully programmable LUT. But
dunno if that's relevant for your hw.
The problem with exposing gamma, whether per-plane or per-crtc, is
that it is hard to define an API that works for all the HW out there.
The capabilities for different HW differ a lot, not just between
vendors but also between generations of a vendor's HW.
Introducing another API if hardware is sufficiently different doesn't
seem like the worst idea. At least it sounds a lot more tractable than
teaching the kernel about all the different use cases, opinions and
nuances that arise from color management.
In the end generic user space must always be able to fall back to
software so the worst case is that it won't be able to offload an
operation if it doesn't know about a new API.
Another reason I'm proposing to define the color space (and gamma) of
a plane is to make this explicit. Up until the color space and gamma
of a plane or framebuffer are not well defined, which leads to drivers
assuming the color space and gamma of a buffer (for blending and other
purposes) and might lead to sub-optimal outcomes.
Blending only is "correct" with linear light so that property of the
color space is important. However, why does the kernel have to be
involved here? As long as user space knows that for correct blending the
data must represent linear light and knows when in the pipeline blending
happens it can make sure that the data at that point in the pipeline
contains linear light.
What other purposes are there?
In general I agree with the others that user space only wants a pipeline
of transformations where the mechanism, the order and ideally the
precision is defined.
Harry
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
