Re: [V7 08/45] Documentation/gpu: document drm_colorop
On 2025-02-21 11:42, Simon Ser wrote: > On Friday, February 21st, 2025 at 17:18, Harry Wentland > wrote: > >> I did a brief survey of other enum properties and noticed >> that this isn't well documented for others, such as the Content >> Protection connector property, or the COLOR_RANGE and COLOR_ENCODING >> plane properties. > > Isn't the Content Protection property documented here? > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-connector-properties > Yes, but I don't see the actual strings. The doc mentions UNDESIRED, DESIRED, and ENABLED but the strings are "Undesired", "Desired", and "Enabled". > COLOR_RANGE and COLOR_ENCODING are documented here, but indeed they > are missing docs for enum entries: > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#color-management-properties > > Would be nice to fix. > >> On the IGT front, some tests set enum properties via enum strings, >> and other set and get enum properties based on the prop uint64_t >> prop_values[i] entry from the drmModeObjectPropertiesPtr. >> >> Do you know if there's a best practice for enum usage or is it mostly >> a case of "use what works for you"? > > It's an old debate. Some user-space uses enum integer values, some > user-space uses enum name strings. > > In theory, each KMS object could have a different name-value map for > a given property. However, this is very theoretical and last time we've > discussed this we haven't found any case where this would be desirable. > > IMHO, strings make it painful for user-space because it needs to go > through another level of indirection (to convert names to values right > before setting a property) for no benefit. Strings are more annoying to > handle in general (memory management, typos, etc). > > Kernel has a no user-space regression policy anyways, so when user-space > starts using values, the kernel won't be able to break these users. > Makes sense and thanks for some background on previous discussions on this. > Other people have argued that strings make it easier for user-space to > start using a new KMS property without deploying new kernel uAPI headers. > I don't understand this argument. You would either need to define the strings or the ints in your user-space app. You could do either without deploying new uAPI headers. > In the end, it's up to user-space to use their preferred way to set > properties. Makes sense. Harry
Re: [V7 08/45] Documentation/gpu: document drm_colorop
On 2025-02-15 09:40, Simon Ser wrote: > On Monday, February 10th, 2025 at 23:03, Harry Wentland > wrote: > + * DOC: overview + * + * A colorop represents a single color operation. Colorops are chained + * via the NEXT property and make up color pipelines. Color pipelines + * are advertised and selected via the COLOR_PIPELINE &drm_plane + * property. + * + * A colorop will be of a certain type, advertised by the read-only TYPE + * property. Each type of colorop will advertise a different set of + * properties and is programmed in a different manner. Types can be + * enumerated 1D curves, 1D LUTs, 3D LUTs, matrices, etc. See the + * &drm_colorop_type documentation for information on each type. >>> >>> It's not super nice to refer to internal kernel docs here, because AFAIU >>> this section is mostly written towards user-space developers. User-space >>> developers have no idea how internal kernel structs work. >>> >>> It would be nicer to have a list of colorop types here, without referring >>> to kernel internals. For instance, we have a list of >> >> I'm not sure I follow. This is linking to the drm_colorop_type >> (from drm_mode.h) enum documentation in drm-uapi.html. >> >> Duplicating it here would mean that sooner or later the two >> docs will get out of sync. > > Oh, I thought this was an internal kernel enum. I guess the only missing > thing is the string exposed to user-space for each enum entry then? > Good point. We'll add that to the enum drm_colorop_type docs. I did a brief survey of other enum properties and noticed that this isn't well documented for others, such as the Content Protection connector property, or the COLOR_RANGE and COLOR_ENCODING plane properties. On the IGT front, some tests set enum properties via enum strings, and other set and get enum properties based on the prop uint64_t prop_values[i] entry from the drmModeObjectPropertiesPtr. Do you know if there's a best practice for enum usage or is it mostly a case of "use what works for you"? Harry > In any case, sounds good to me. > >> I agree with the rest and we'll reflect that in v8. > > Sweet!
Re: [V7 08/45] Documentation/gpu: document drm_colorop
On Friday, February 21st, 2025 at 19:41, Harry Wentland wrote: > > Other people have argued that strings make it easier for user-space to > > start using a new KMS property without deploying new kernel uAPI headers. > > I don't understand this argument. You would either need to define the > strings or the ints in your user-space app. You could do either without > deploying new uAPI headers. I suppose a string is less likely to be wrong/mixed up than a magic number? I'm probably not the best person to explain this side of the debate. :P Looking at Weston, it seems to just re-define its own enums matching the kernel strings: https://gitlab.freedesktop.org/wayland/weston/-/blob/6d9c42a7d294a1435822541ab4b3e12a8998dabb/libweston/backend-drm/kms.c#L49 For reference, one of the previous discussions: https://lists.freedesktop.org/archives/dri-devel/2020-April/thread.html#261055 I think there was another one, but can't find it back…
Re: [V7 34/45] drm/amd/display: add shaper and blend colorops for 1D Curve Custom LUT
On 2025-02-12 18:44, Leo Li wrote:
>
>
> On 2024-12-19 23:33, Alex Hung wrote:
>> This patch adds colorops for custom 1D LUTs in the SHAPER and
>> BLND HW blocks.
>>
>> With this change the following IGT tests pass:
>> kms_colorop --run plane-XR30-XR30-srgb_inv_eotf_lut
>> kms_colorop --run plane-XR30-XR30-srgb_inv_eotf_lut-srgb_eotf_lut
>>
>> The color pipeline now consists of the following colorops:
>> 1. 1D curve colorop
>> 2. 1D curve colorop
>> 3. 1D LUT
>> 4. 1D curve colorop
>> 5. 1D LUT
>>
>> The 1D curve colorops support sRGB, BT2020, and PQ scaled to 125.0.
>>
>> Signed-off-by: Alex Hung
>> Signed-off-by: Harry Wentland
>> ---
>> v7:
>> - Initialize uint32_t blend_size = 0 by default (kernel test robot)
>> - Modify state->size to colorop->lut_size
>>
>> .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 166 ++
>> .../amd/display/amdgpu_dm/amdgpu_dm_colorop.c | 32
>> 2 files changed, 120 insertions(+), 78 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
>> index 1765402bc122..0bea52eede39 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
>> @@ -1211,38 +1211,6 @@ __set_dm_plane_colorop_degamma(struct drm_plane_state
>> *plane_state,
>> return __set_colorop_in_tf_1d_curve(dc_plane_state, colorop_state);
>> }
>> -static int
>> -__set_colorop_in_shaper_1d_curve(struct dc_plane_state *dc_plane_state,
>> - struct drm_colorop_state *colorop_state)
>> -{
>> - struct dc_transfer_func *tf = &dc_plane_state->in_shaper_func;
>> - struct drm_colorop *colorop = colorop_state->colorop;
>> - struct drm_device *drm = colorop->dev;
>> -
>> - if (colorop->type != DRM_COLOROP_1D_CURVE)
>> - return -EINVAL;
>> -
>> - if (!(BIT(colorop_state->curve_1d_type) &
>> amdgpu_dm_supported_shaper_tfs))
>> - return -EINVAL;
>> -
>> - if (colorop_state->bypass) {
>> - tf->type = TF_TYPE_BYPASS;
>> - tf->tf = TRANSFER_FUNCTION_LINEAR;
>> - return 0;
>> - }
>> -
>> - drm_dbg(drm, "Shaper colorop with ID: %d\n", colorop->base.id);
>> -
>> - if (colorop->type == DRM_COLOROP_1D_CURVE) {
>> - tf->type = TF_TYPE_DISTRIBUTED_POINTS;
>> - tf->tf = amdgpu_colorop_tf_to_dc_tf(colorop_state->curve_1d_type);
>> - tf->sdr_ref_white_level = SDR_WHITE_LEVEL_INIT_VALUE;
>> - return __set_output_tf(tf, 0, 0, false);
>> - }
>> -
>> - return -EINVAL;
>> -}
>> -
>> static int
>> __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
>> struct dc_plane_state *dc_plane_state,
>> @@ -1251,64 +1219,61 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state
>> *plane_state,
>> struct drm_colorop *old_colorop;
>> struct drm_colorop_state *colorop_state = NULL, *new_colorop_state;
>> struct drm_atomic_state *state = plane_state->state;
>> + enum dc_transfer_func_predefined default_tf = TRANSFER_FUNCTION_LINEAR;
>> + struct dc_transfer_func *tf = &dc_plane_state->in_shaper_func;
>> + const struct drm_color_lut *shaper_lut;
>> + struct drm_device *dev = colorop->dev;
>> + uint32_t shaper_size;
>> int i = 0;
>> + /* 1D Curve - SHAPER TF */
>> old_colorop = colorop;
>> -
>> - /* 2nd op: 1d curve - shaper */
>> for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
>> if (new_colorop_state->colorop == old_colorop &&
>> (BIT(new_colorop_state->curve_1d_type) &
>> amdgpu_dm_supported_shaper_tfs)) {
>> colorop_state = new_colorop_state;
>> break;
>> }
>> -
>> - if (new_colorop_state->colorop == old_colorop) {
>> - colorop_state = new_colorop_state;
>> - break;
>> - }
>> }
>> - if (!colorop_state)
>> - return -EINVAL;
>> -
>> - return __set_colorop_in_shaper_1d_curve(dc_plane_state, colorop_state);
>> -}
>> -
>> -
>> -static int
>> -__set_colorop_1d_curve_blend_tf_lut(struct dc_plane_state *dc_plane_state,
>> - struct drm_colorop_state *colorop_state)
>> -{
>> -
>> - struct dc_transfer_func *tf = &dc_plane_state->blend_tf;
>> - struct drm_colorop *colorop = colorop_state->colorop;
>> - struct drm_device *drm = colorop->dev;
>> - const struct drm_color_lut *blend_lut;
>> - uint32_t blend_size = 0;
>> -
>> - if (colorop->type != DRM_COLOROP_1D_CURVE)
>> - return -EINVAL;
>> + if (colorop_state && !colorop_state->bypass && colorop->type ==
>> DRM_COLOROP_1D_CURVE) {
>> + drm_dbg(dev, "Shaper TF colorop with ID: %d\n", colorop->base.id);
>> + tf->type = TF_TYPE_DISTRIBUTED_POINTS;
>> + tf->tf = default_tf =
>> amdgpu_colorop_tf_to_dc_tf(colorop_state->curve_1d_type);
>> + tf->sdr_ref_white_level = SDR_WHITE_LEVEL_INIT_VALUE;
>>
Re: [V7 08/45] Documentation/gpu: document drm_colorop
On Friday, February 21st, 2025 at 17:18, Harry Wentland wrote: > I did a brief survey of other enum properties and noticed > that this isn't well documented for others, such as the Content > Protection connector property, or the COLOR_RANGE and COLOR_ENCODING > plane properties. Isn't the Content Protection property documented here? https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-connector-properties COLOR_RANGE and COLOR_ENCODING are documented here, but indeed they are missing docs for enum entries: https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#color-management-properties Would be nice to fix. > On the IGT front, some tests set enum properties via enum strings, > and other set and get enum properties based on the prop uint64_t > prop_values[i] entry from the drmModeObjectPropertiesPtr. > > Do you know if there's a best practice for enum usage or is it mostly > a case of "use what works for you"? It's an old debate. Some user-space uses enum integer values, some user-space uses enum name strings. In theory, each KMS object could have a different name-value map for a given property. However, this is very theoretical and last time we've discussed this we haven't found any case where this would be desirable. IMHO, strings make it painful for user-space because it needs to go through another level of indirection (to convert names to values right before setting a property) for no benefit. Strings are more annoying to handle in general (memory management, typos, etc). Kernel has a no user-space regression policy anyways, so when user-space starts using values, the kernel won't be able to break these users. Other people have argued that strings make it easier for user-space to start using a new KMS property without deploying new kernel uAPI headers. In the end, it's up to user-space to use their preferred way to set properties.
Re: [V7 42/45] drm/amd/display: add 3D LUT colorop
On 2025-02-13 13:21, Leo Li wrote:
>
>
>
> On 2024-12-19 23:33, Alex Hung wrote:
>> This adds support for a 3D LUT.
>>
>> The color pipeline now consists of the following colorops:
>> 1. 1D curve colorop
>> 2. Multiplier
>> 3. 3x4 CTM
>> 4. 1D curve colorop
>> 5. 1D LUT
>> 6. 3D LUT
>> 7. 1D curve colorop
>> 8. 1D LUT
>>
>> Signed-off-by: Alex Hung
>> ---
>> v7:
>> - Simplify 3D LUT according to drm_colorop changes (Simon Ser)
>>
>> .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 100 +-
>> .../amd/display/amdgpu_dm/amdgpu_dm_colorop.c | 19
>> 2 files changed, 116 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
>> index 54ec12c1352f..5e8c5c0657c4 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
>> @@ -1282,7 +1282,8 @@ __set_dm_plane_colorop_multiplier(struct
>> drm_plane_state *plane_state,
>> static int
>> __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
>> struct dc_plane_state *dc_plane_state,
>> - struct drm_colorop *colorop)
>> + struct drm_colorop *colorop,
>> + bool *enabled)
>> {
>> struct drm_colorop *old_colorop;
>> struct drm_colorop_state *colorop_state = NULL, *new_colorop_state;
>> @@ -1310,6 +1311,7 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state
>> *plane_state,
>> tf->tf = default_tf =
>> amdgpu_colorop_tf_to_dc_tf(colorop_state->curve_1d_type);
>> tf->sdr_ref_white_level = SDR_WHITE_LEVEL_INIT_VALUE;
>> __set_output_tf(tf, 0, 0, false);
>> + *enabled = true;
>> }
>> /* 1D LUT - SHAPER LUT */
>> @@ -1337,8 +1339,88 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state
>> *plane_state,
>> shaper_size = shaper_lut != NULL ? shaper_size : 0;
>> /* Custom LUT size must be the same as supported size */
>> - if (shaper_size == colorop->lut_size)
>> + if (shaper_size == colorop->lut_size) {
>> __set_output_tf(tf, shaper_lut, shaper_size, false);
>> + *enabled = true;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/* __set_colorop_3dlut - set DRM 3D LUT to DC stream
>> + * @drm_lut3d: user 3D LUT
>> + * @drm_lut3d_size: size of 3D LUT
>> + * @lut3d: DC 3D LUT
>> + *
>> + * Map user 3D LUT data to DC 3D LUT and all necessary bits to program it
>> + * on DCN accordingly.
>> + */
>> +static void __set_colorop_3dlut(const struct drm_color_lut *drm_lut3d,
>> + uint32_t drm_lut3d_size,
>> + struct dc_3dlut *lut)
>> +{
>> + if (!drm_lut3d_size)
>> + return;
>> +
>> + lut->state.bits.initialized = 0;
>> +
Shouldn't we clear this bit if !drm_lut3d_size? I.e.,
if (!drm_lut3d_size) {
lut->state.bits.initialized = 0;
return;
}
>> + /* Only supports 17x17x17 3D LUT (12-bit) now */
>> + lut->lut_3d.use_12bits = true;
>> + lut->lut_3d.use_tetrahedral_9 = false;
>> +
>> + lut->state.bits.initialized = 1;
>> + __drm_3dlut_to_dc_3dlut(drm_lut3d, drm_lut3d_size, &lut->lut_3d,
>> + lut->lut_3d.use_tetrahedral_9, 12);
>> +
>> +}
>> +
>> +static int
>> +__set_dm_plane_colorop_3dlut(struct drm_plane_state *plane_state,
>> + struct dc_plane_state *dc_plane_state,
>> + struct drm_colorop *colorop,
>> + bool shaper_enabled)
>> +{
>> + struct drm_colorop *old_colorop;
>> + struct drm_colorop_state *colorop_state = NULL, *new_colorop_state;
>> + struct dc_transfer_func *tf = &dc_plane_state->in_shaper_func;
>> + struct drm_atomic_state *state = plane_state->state;
>> + const struct amdgpu_device *adev = drm_to_adev(colorop->dev);
>> + const struct drm_device *dev = colorop->dev;
>> + const struct drm_color_lut *lut3d;
>> + uint32_t lut3d_size;
>> + int i = 0;
>> +
>> + /* 3D LUT */
>> + old_colorop = colorop;
>> + for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
>> + if (new_colorop_state->colorop == old_colorop &&
>> + new_colorop_state->colorop->type == DRM_COLOROP_3D_LUT) {
>> + colorop_state = new_colorop_state;
>> + break;
>> + }
>> + }
>> +
>> + if (colorop_state && !colorop_state->bypass && colorop->type ==
>> DRM_COLOROP_3D_LUT) {
>> + if (!adev->dm.dc->caps.color.dpp.hw_3d_lut) {
>> + drm_dbg(dev, "3D LUT is not supported by hardware\n");
>> + return 0;
>
> Should an error be returned instead?
> -Leo
>
>> + }
>> +
>> + drm_dbg(dev, "3D LUT colorop with ID: %d\n", colorop->base.id);
>> + lut3d = __extract_blob_lut(colorop_state->data, &lut3d_size);
>> + lut3d_size = lut3d != NULL ? lut3d_size : 0;
>> + __set_colorop_3dlut(l
