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 > > 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 &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: >