[PATCH 06/36] drm/amd/display: add CRTC driver-specific property for gamma TF

Harry Wentland harry.wentland at amd.com
Tue Jun 6 20:03:08 UTC 2023



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 at 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 at froggi.es>
>>>>> Signed-off-by: Joshua Ashton <joshua at froggi.es>
>>>>> Signed-off-by: Melissa Wen <mwen at 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 🐸✨
>>>
>>>>
>>>> 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:
>>>>
>>>
>>



More information about the dri-devel mailing list