| Message ID | 20230523221520.3115570-7-mwen@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd/display: add AMD driver-specific properties for color mgmt | expand |
On Tue, 23 May 2023 21:14:50 -0100 Melissa Wen <mwen@igalia.com> 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@froggi.es> > Signed-off-by: Joshua Ashton <joshua@froggi.es> > Signed-off-by: Melissa Wen <mwen@igalia.com> > --- > 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. 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:
On 5/24/23 04:24, Pekka Paalanen wrote: > On Tue, 23 May 2023 21:14:50 -0100 > Melissa Wen <mwen@igalia.com> 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@froggi.es> >> Signed-off-by: Joshua Ashton <joshua@froggi.es> >> Signed-off-by: Melissa Wen <mwen@igalia.com> >> --- >> 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: >
Hi Melissa,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.4-rc3 next-20230525]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Melissa-Wen/drm-drm_mode_object-increase-max-objects-to-accommodate-new-color-props/20230524-062917
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20230523221520.3115570-7-mwen%40igalia.com
patch subject: [PATCH 06/36] drm/amd/display: add CRTC driver-specific property for gamma TF
config: i386-allyesconfig
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/3d87b0dd30324e0650cd5afb9e55fd204d9ce329
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Melissa-Wen/drm-drm_mode_object-increase-max-objects-to-accommodate-new-color-props/20230524-062917
git checkout 3d87b0dd30324e0650cd5afb9e55fd204d9ce329
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 olddefconfig
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/amd/amdgpu/ drivers/gpu/drm/amd/display/amdgpu_dm/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305260305.KbntIovo-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:1251:40: warning: 'drm_transfer_function_enum_list' defined but not used [-Wunused-const-variable=]
1251 | static const struct drm_prop_enum_list drm_transfer_function_enum_list[] = {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/drm_transfer_function_enum_list +1251 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
1250
> 1251 static const struct drm_prop_enum_list drm_transfer_function_enum_list[] = {
1252 { DRM_TRANSFER_FUNCTION_DEFAULT, "Default" },
1253 { DRM_TRANSFER_FUNCTION_SRGB, "sRGB" },
1254 { DRM_TRANSFER_FUNCTION_BT709, "BT.709" },
1255 { DRM_TRANSFER_FUNCTION_PQ, "PQ (Perceptual Quantizer)" },
1256 { DRM_TRANSFER_FUNCTION_LINEAR, "Linear" },
1257 { DRM_TRANSFER_FUNCTION_UNITY, "Unity" },
1258 { DRM_TRANSFER_FUNCTION_HLG, "HLG (Hybrid Log Gamma)" },
1259 { DRM_TRANSFER_FUNCTION_GAMMA22, "Gamma 2.2" },
1260 { DRM_TRANSFER_FUNCTION_GAMMA24, "Gamma 2.4" },
1261 { DRM_TRANSFER_FUNCTION_GAMMA26, "Gamma 2.6" },
1262 };
1263
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 <joshua@froggi.es> > Signed-off-by: Joshua Ashton <joshua@froggi.es> > Signed-off-by: Melissa Wen <mwen@igalia.com> > --- > 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" }, > +}; 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. 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 in amdgpu_dm_mode_config_init(). > 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; Again, let's avoid a drm_ prefix. Maybe name all this amdgpu_ instead. > }; > > #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); > > - nit: stray newline Harry > __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:
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 <joshua@froggi.es> >> Signed-off-by: Joshua Ashton <joshua@froggi.es> >> Signed-off-by: Melissa Wen <mwen@igalia.com> >> --- >> 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" }, >> +}; > > 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. - Joshie
On Tue, Jun 6, 2023 at 6:19 PM Joshua Ashton <joshua@froggi.es> 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 <joshua@froggi.es> > >> Signed-off-by: Joshua Ashton <joshua@froggi.es> > >> Signed-off-by: Melissa Wen <mwen@igalia.com> > >> --- > >> 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" }, > >> +}; > > > > 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
On 06/06, Sebastian Wick wrote: > On Tue, Jun 6, 2023 at 6:19 PM Joshua Ashton <joshua@froggi.es> 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 <joshua@froggi.es> > > >> Signed-off-by: Joshua Ashton <joshua@froggi.es> > > >> Signed-off-by: Melissa Wen <mwen@igalia.com> > > >> --- > > >> 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" }, > > >> +}; > > > > > > 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. I'm okay on using `AMD_` prefix instead of `DRM_` for this driver-specific implementation. I think the position of each block in the pipeline already describe the conversion type, since we only implemented one conversion type per-block. So, I agree we can keep it simple as Joshua suggested (don't extend enum, just document blocks - should it be a kernel doc?). Melissa > > > - Joshie
On 06/01, 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 <joshua@froggi.es> > > Signed-off-by: Joshua Ashton <joshua@froggi.es> > > Signed-off-by: Melissa Wen <mwen@igalia.com> > > --- > > 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" }, > > +}; > > 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. > > 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 in > amdgpu_dm_mode_config_init(). Ok. I'll move everything to amdgpu_dm_color. > > > 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; > > Again, let's avoid a drm_ prefix. Maybe name all this amdgpu_ instead. Ack > > > }; > > > > #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); > > > > - > > nit: stray newline oh, random code cleanup.. I'll remove it in the next version >.< thanks! > > Harry > > > __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: >
On 6/6/23 12:57, Melissa Wen wrote: > On 06/06, Sebastian Wick wrote: >> On Tue, Jun 6, 2023 at 6:19 PM Joshua Ashton <joshua@froggi.es> 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 <joshua@froggi.es> >>>>> Signed-off-by: Joshua Ashton <joshua@froggi.es> >>>>> Signed-off-by: Melissa Wen <mwen@igalia.com> >>>>> --- >>>>> 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" }, >>>>> +}; >>>> >>>> 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. > > I'm okay on using `AMD_` prefix instead of `DRM_` for this > driver-specific implementation. I think the position of each block in > the pipeline already describe the conversion type, since we only > implemented one conversion type per-block. So, I agree we can keep it > simple as Joshua suggested (don't extend enum, just document blocks - > should it be a kernel doc?). > I would still prefer if we can make the enum be explicit about whether an EOTF or inv_EOTF is intended. In theory many of these blocks in HW could support either. Harry > Melissa > >> >>> - Joshie
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", + 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: