On 5/24/23 04:24, Pekka Paalanen wrote: > On Tue, 23 May 2023 21:14:50 -0100 > Melissa Wen <mwen@xxxxxxxxxx> 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 <joshua@xxxxxxxxx> >> Signed-off-by: Joshua Ashton <joshua@xxxxxxxxx> >> Signed-off-by: Melissa Wen <mwen@xxxxxxxxxx> >> --- >> 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 &amdgpu_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" }, >> +}; >> + >> +#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", > > Hi, > > is this REGAMMA element capable of applying only optical-to-electrical > direction of the listed TFs? > > I was expecting that the listed TF names would include an explanation > of the direction, for example "PQ EOTF" vs. "inverse PQ EOTF". > > Very specifically "inverse EOTF" and not "OETF" because they > generally are not the same concept. > > PQ defines only EOTF while HLG for example defines OETF (HLG defines > its EOTF as a combination of inverse HLG OETF and a parameterised HLG > OOTF). So if you say "PQ TF" I will assume it means > electrical-to-optical and if you say HLG TF I might assume > optical-to-electrical. I think these enum names should be more explicit > about what they refer to, to avoid any ambiguity. > > What kind of TF is "Unity"? > > This patch is not adding any docs for any of these. Is there another > patch that does? > > I'm still confused about how this "private" API should be thought of. > Should it be documented at all? Is it free to use for userspace? > Was the expectation that only the Steam Deck distribution would enable > these in the kernel, and no-one else? And if anyone builds their own > kernel with these enabled? So my ask for docs may or may not be > warranted. > The current plan is to put the API bits behind a #ifdef AMD_PRIVATE_COLOR (or a similar name) and not making it configurable via KConfig. Anyone that wants them would have to build the kernel with -DAMD_PRIVATE_COLOR. Thanks for re-iterating your naming and documentation concerns. It would be good to still fix that up, even if this doesn't become upstream API as-is. Harry > I don't like the names degamma/regamma/gamma at all. I don't like > calling something a LUT when it can have a parametric or enumerated > curve. I don't like calling an element a transfer function if it could > be a shaper or a combination of TF and shaper and maybe something else > (i.e. a LUT). > > But that's nothing new. If the expectation is that no-one should use > these, then it's fine, and you don't need to CC me. You know I will > always respond with similar comments about documenting things, having > good names, etc. that is important for generic userspace, which is just > not needed for "no-users UAPI". ;-) > > > Thanks, > pq > >> + 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 >> + >> const struct drm_mode_config_funcs amdgpu_mode_funcs = { >> .fb_create = amdgpu_display_user_framebuffer_create, >> }; >> @@ -1323,6 +1355,10 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev) >> return -ENOMEM; >> } >> >> +#ifdef AMD_PRIVATE_COLOR >> + if (amdgpu_display_create_color_properties(adev)) >> + return -ENOMEM; >> +#endif >> return 0; >> } >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h >> index b8633df418d4..881446c51b36 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h >> @@ -344,6 +344,14 @@ struct amdgpu_mode_info { >> int disp_priority; >> const struct amdgpu_display_funcs *funcs; >> const enum drm_plane_type *plane_type; >> + >> + /* Driver-private color mgmt props */ >> + >> + /* @regamma_tf_property: Transfer function for CRTC regamma >> + * (post-blending). Possible values are defined by `enum >> + * drm_transfer_function`. >> + */ >> + struct drm_property *regamma_tf_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 2e2413fd73a4..ad5ee28b83dc 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h >> @@ -699,6 +699,20 @@ static inline void amdgpu_dm_set_mst_status(uint8_t *status, >> >> extern const struct amdgpu_ip_block_version dm_ip_block; >> >> +enum drm_transfer_function { >> + DRM_TRANSFER_FUNCTION_DEFAULT, >> + DRM_TRANSFER_FUNCTION_SRGB, >> + DRM_TRANSFER_FUNCTION_BT709, >> + DRM_TRANSFER_FUNCTION_PQ, >> + DRM_TRANSFER_FUNCTION_LINEAR, >> + DRM_TRANSFER_FUNCTION_UNITY, >> + DRM_TRANSFER_FUNCTION_HLG, >> + DRM_TRANSFER_FUNCTION_GAMMA22, >> + DRM_TRANSFER_FUNCTION_GAMMA24, >> + DRM_TRANSFER_FUNCTION_GAMMA26, >> + DRM_TRANSFER_FUNCTION_MAX, >> +}; >> + >> struct dm_plane_state { >> struct drm_plane_state base; >> struct dc_plane_state *dc_state; >> @@ -726,6 +740,14 @@ struct dm_crtc_state { >> struct dc_info_packet vrr_infopacket; >> >> int abm_level; >> + >> + /** >> + * @regamma_tf: >> + * >> + * Pre-defined transfer function for converting internal FB -> wire >> + * encoding. >> + */ >> + enum drm_transfer_function regamma_tf; >> }; >> >> #define to_dm_crtc_state(x) container_of(x, struct dm_crtc_state, base) >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c >> index e3762e806617..1eb279d341c5 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c >> @@ -229,7 +229,6 @@ static void dm_crtc_destroy_state(struct drm_crtc *crtc, >> if (cur->stream) >> dc_stream_release(cur->stream); >> >> - >> __drm_atomic_helper_crtc_destroy_state(state); >> >> >> @@ -263,6 +262,7 @@ static struct drm_crtc_state *dm_crtc_duplicate_state(struct drm_crtc *crtc) >> state->freesync_config = cur->freesync_config; >> state->cm_has_degamma = cur->cm_has_degamma; >> state->cm_is_degamma_srgb = cur->cm_is_degamma_srgb; >> + state->regamma_tf = cur->regamma_tf; >> state->crc_skip_count = cur->crc_skip_count; >> state->mpo_requested = cur->mpo_requested; >> /* TODO Duplicate dc_stream after objects are stream object is flattened */ >> @@ -299,6 +299,69 @@ static int amdgpu_dm_crtc_late_register(struct drm_crtc *crtc) >> } >> #endif >> >> +#ifdef AMD_PRIVATE_COLOR >> +/** >> + * drm_crtc_additional_color_mgmt - enable additional color properties >> + * @crtc: DRM CRTC >> + * >> + * This function lets the driver enable the 3D LUT color correction property >> + * on a CRTC. This includes shaper LUT, 3D LUT and regamma TF. The shaper >> + * LUT and 3D LUT property is only attached if its size is not 0. >> + */ >> +static void >> +dm_crtc_additional_color_mgmt(struct drm_crtc *crtc) >> +{ >> + struct amdgpu_device *adev = drm_to_adev(crtc->dev); >> + >> + if(adev->dm.dc->caps.color.mpc.ogam_ram) >> + drm_object_attach_property(&crtc->base, >> + adev->mode_info.regamma_tf_property, >> + DRM_TRANSFER_FUNCTION_DEFAULT); >> +} >> + >> +static int >> +amdgpu_dm_atomic_crtc_set_property(struct drm_crtc *crtc, >> + struct drm_crtc_state *state, >> + struct drm_property *property, >> + uint64_t val) >> +{ >> + struct amdgpu_device *adev = drm_to_adev(crtc->dev); >> + struct dm_crtc_state *acrtc_state = to_dm_crtc_state(state); >> + >> + if (property == adev->mode_info.regamma_tf_property) { >> + if (acrtc_state->regamma_tf != val) { >> + acrtc_state->regamma_tf = val; >> + acrtc_state->base.color_mgmt_changed |= 1; >> + } >> + } else { >> + drm_dbg_atomic(crtc->dev, >> + "[CRTC:%d:%s] unknown property [PROP:%d:%s]]\n", >> + crtc->base.id, crtc->name, >> + property->base.id, property->name); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static int >> +amdgpu_dm_atomic_crtc_get_property(struct drm_crtc *crtc, >> + const struct drm_crtc_state *state, >> + struct drm_property *property, >> + uint64_t *val) >> +{ >> + struct amdgpu_device *adev = drm_to_adev(crtc->dev); >> + struct dm_crtc_state *acrtc_state = to_dm_crtc_state(state); >> + >> + if (property == adev->mode_info.regamma_tf_property) >> + *val = acrtc_state->regamma_tf; >> + else >> + return -EINVAL; >> + >> + return 0; >> +} >> +#endif >> + >> /* Implemented only the options currently available for the driver */ >> static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = { >> .reset = dm_crtc_reset_state, >> @@ -317,6 +380,10 @@ static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = { >> #if defined(CONFIG_DEBUG_FS) >> .late_register = amdgpu_dm_crtc_late_register, >> #endif >> +#ifdef AMD_PRIVATE_COLOR >> + .atomic_set_property = amdgpu_dm_atomic_crtc_set_property, >> + .atomic_get_property = amdgpu_dm_atomic_crtc_get_property, >> +#endif >> }; >> >> static void dm_crtc_helper_disable(struct drm_crtc *crtc) >> @@ -480,6 +547,9 @@ int amdgpu_dm_crtc_init(struct amdgpu_display_manager *dm, >> >> drm_mode_crtc_set_gamma_size(&acrtc->base, MAX_COLOR_LEGACY_LUT_ENTRIES); >> >> +#ifdef AMD_PRIVATE_COLOR >> + dm_crtc_additional_color_mgmt(&acrtc->base); >> +#endif >> return 0; >> >> fail: >