On 2022-07-20 18:54, Melissa Wen wrote:
On 07/17, Tales Lelo da Aparecida wrote:
On 16/07/2022 19:25, Melissa Wen wrote:
AMDGPU DM maps DRM color management properties (degamma, ctm and gamma)
to DC color correction entities. Part of this mapping is already
documented as code comments and can be converted as kernel docs.
v2:
- rebase to amd-staging-drm-next
Reviewed-by: Harry Wentland <harry.wentland@xxxxxxx>
Signed-off-by: Melissa Wen <mwen@xxxxxxxxxx>
---
.../gpu/amdgpu/display/display-manager.rst | 9 ++
.../amd/display/amdgpu_dm/amdgpu_dm_color.c | 121 +++++++++++++-----
2 files changed, 98 insertions(+), 32 deletions(-)
diff --git a/Documentation/gpu/amdgpu/display/display-manager.rst b/Documentation/gpu/amdgpu/display/display-manager.rst
index 7ce31f89d9a0..b1b0f11aed83 100644
--- a/Documentation/gpu/amdgpu/display/display-manager.rst
+++ b/Documentation/gpu/amdgpu/display/display-manager.rst
@@ -40,3 +40,12 @@ Atomic Implementation
.. kernel-doc:: drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
:functions: amdgpu_dm_atomic_check amdgpu_dm_atomic_commit_tail
+
+Color Management Properties
+===========================
+
+.. kernel-doc:: drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+ :doc: overview
+
+.. kernel-doc:: drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+ :internal:
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 a71177305bcd..93c813089bff 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
@@ -29,7 +29,9 @@
#include "modules/color/color_gamma.h"
#include "basics/conversion.h"
-/*
+/**
+ * DOC: overview
+ *
* The DC interface to HW gives us the following color management blocks
* per pipe (surface):
*
@@ -71,8 +73,8 @@
#define MAX_DRM_LUT_VALUE 0xFFFF
-/*
- * Initialize the color module.
+/**
+ * amdgpu_dm_init_color_mod - Initialize the color module.
*
* We're not using the full color module, only certain components.
* Only call setup functions for components that we need.
@@ -82,7 +84,14 @@ void amdgpu_dm_init_color_mod(void)
setup_x_points_distribution();
}
-/* Extracts the DRM lut and lut size from a blob. */
+/**
+ * __extract_blob_lut - Extracts the DRM lut and lut size from a blob.
+ * @blob: DRM color mgmt property blob
+ * @size: lut size
+ *
+ * Returns:
+ * DRM LUT or NULL
+ */
static const struct drm_color_lut *
__extract_blob_lut(const struct drm_property_blob *blob, uint32_t *size)
{
@@ -90,13 +99,18 @@ __extract_blob_lut(const struct drm_property_blob *blob, uint32_t *size)
return blob ? (struct drm_color_lut *)blob->data : NULL;
}
I don't think everyone would approve using actual kernel-doc for these
static functions, but I can appreciate they being formatted as such.
Consider replacing /** with /*.
IMHO, although they are static, they provide info to understand the AMD
DM programming of DRM color correction properties. I see the value for
external contributors, but I'm not sure about kernel-doc rules about it.
Yeah... I agree, I don't mind seeing kernel-doc for some static
functions. Iirc, DRM documentation also documents some static functions.
-/*
- * Return true if the given lut is a linear mapping of values, i.e. it acts
- * like a bypass LUT.
+/**
+ * __is_lut_linear - check if the given lut is a linear mapping of values
+ * @lut: given lut to check values
+ * @size: lut size
*
* It is considered linear if the lut represents:
- * f(a) = (0xFF00/MAX_COLOR_LUT_ENTRIES-1)a; for integer a in
- * [0, MAX_COLOR_LUT_ENTRIES)
+ * f(a) = (0xFF00/MAX_COLOR_LUT_ENTRIES-1)a; for integer a in [0,
+ * MAX_COLOR_LUT_ENTRIES)
+ *
+ * Returns:
+ * True if the given lut is a linear mapping of values, i.e. it acts like a
+ * bypass LUT. Otherwise, false.
*/
static bool __is_lut_linear(const struct drm_color_lut *lut, uint32_t size)
{
@@ -119,9 +133,13 @@ static bool __is_lut_linear(const struct drm_color_lut *lut, uint32_t size)
return true;
}
-/*
- * Convert the drm_color_lut to dc_gamma. The conversion depends on the size
- * of the lut - whether or not it's legacy.
+/**
+ * __drm_lut_to_dc_gamma - convert the drm_color_lut to dc_gamma.
+ * @lut: DRM lookup table for color conversion
+ * @gamma: DC gamma to set entries
+ * @is_legacy: legacy or atomic gamma
+ *
+ * The conversion depends on the size of the lut - whether or not it's legacy.
*/
static void __drm_lut_to_dc_gamma(const struct drm_color_lut *lut,
struct dc_gamma *gamma, bool is_legacy)
@@ -154,8 +172,11 @@ static void __drm_lut_to_dc_gamma(const struct drm_color_lut *lut,
}
}
-/*
- * Converts a DRM CTM to a DC CSC float matrix.
+/**
+ * __drm_ctm_to_dc_matrix - converts a DRM CTM to a DC CSC float matrix
+ * @ctm: DRM color transformation matrix
+ * @matrix: DC CSC float matrix
+ *
* The matrix needs to be a 3x4 (12 entry) matrix.
*/
static void __drm_ctm_to_dc_matrix(const struct drm_color_ctm *ctm,
@@ -189,7 +210,18 @@ static void __drm_ctm_to_dc_matrix(const struct drm_color_ctm *ctm,
}
}
-/* Calculates the legacy transfer function - only for sRGB input space. */
+/**
+ * __set_legacy_tf - Calculates the legacy transfer function
+ * @func: transfer function
+ * @lut: lookup table that defines the color space
+ * @lut_size: size of respective lut
+ * @has_rom: if ROM can be used for hardcoded curve
+ *
+ * Only for sRGB input space
+ *
+ * Returns:
+ * 0 in case of sucess, -ENOMEM if fails
Typo sucess -> success
+ */
static int __set_legacy_tf(struct dc_transfer_func *func,
const struct drm_color_lut *lut, uint32_t lut_size,
bool has_rom)
@@ -218,7 +250,16 @@ static int __set_legacy_tf(struct dc_transfer_func *func,
return res ? 0 : -ENOMEM;
}
-/* Calculates the output transfer function based on expected input space. */
+/**
+ * __set_output_tf - calculates the output transfer function based on expected input space.
+ * @func: transfer function
+ * @lut: lookup table that defines the color space
+ * @lut_size: size of respective lut
+ * @has_rom: if ROM can be used for hardcoded curve
+ *
+ * Returns:
+ * 0 in case of success. -ENOMEM if fails.
+ */
static int __set_output_tf(struct dc_transfer_func *func,
const struct drm_color_lut *lut, uint32_t lut_size,
bool has_rom)
@@ -239,7 +280,7 @@ static int __set_output_tf(struct dc_transfer_func *func,
__drm_lut_to_dc_gamma(lut, gamma, false);
if (func->tf == TRANSFER_FUNCTION_LINEAR) {
- /*
+ /**
I don't think kernel-doc should be used inside functions, as well,
maybe keep the "/*" from before. This occurs in more places in this patch,
remember to replace them as well, if you concur.
hmm.. I think inline doc is good to avoid repetitions, at the same time we
expose this info and keep it near its context in the code. This is why I
chose this path.. I'll think about it
I'm not sure, but I think that kernel-doc inside a function is ignored.
In that case, I would follow the kernel guideline for code comments.
Maybe you can move part of this documentation to the function description?
Btw, nice patch!
Thanks
Siqueira
* Color module doesn't like calculating regamma params
* on top of a linear input. But degamma params can be used
* instead to simulate this.
@@ -262,7 +303,16 @@ static int __set_output_tf(struct dc_transfer_func *func,
return res ? 0 : -ENOMEM;
}
-/* Caculates the input transfer function based on expected input space. */
+/**
+ * __set_input_tf - calculates the input transfer function based on expected
+ * input space.
+ * @func: transfer function
+ * @lut: lookup table that defines the color space
+ * @lut_size: size of respective lut.
+ *
+ * Returns:
+ * 0 in case of success. -ENOMEM if fails.
+ */
static int __set_input_tf(struct dc_transfer_func *func,
const struct drm_color_lut *lut, uint32_t lut_size)
{
@@ -285,13 +335,15 @@ static int __set_input_tf(struct dc_transfer_func *func,
}
/**
- * amdgpu_dm_verify_lut_sizes
+ * amdgpu_dm_verify_lut_sizes - verifies if DRM luts match the hw supported sizes
* @crtc_state: the DRM CRTC state
*
- * Verifies that the Degamma and Gamma LUTs attached to the |crtc_state| are of
- * the expected size.
+ * Verifies that the Degamma and Gamma LUTs attached to the &crtc_state
+ * are of the expected size.
*
- * Returns 0 on success.
+ * Returns:
+ * 0 on success.
+ * -EINVAL if any lut sizes are invalid.
I don't know if you expect this to be rendered in two lines, given that you
wrote something equivalent in a single line in other docstrings above, but
if you do, use instead:
```
* * 0 on success.
* * -EINVAL if any lut sizes are invalid.
```
As described at: https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#return-values
I expected they are in the same line, separated by dot. I'll put the
together for clarity.
Thanks,
Melissa
*/
int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state)
{
@@ -327,9 +379,9 @@ int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state)
* of the HW blocks as long as the CRTC CTM always comes before the
* CRTC RGM and after the CRTC DGM.
*
- * The CRTC RGM block will be placed in the RGM LUT block if it is non-linear.
- * The CRTC DGM block will be placed in the DGM LUT block if it is non-linear.
- * The CRTC CTM will be placed in the gamut remap block if it is non-linear.
+ * * The CRTC RGM block will be placed in the RGM LUT block if it is non-linear.
+ * * The CRTC DGM block will be placed in the DGM LUT block if it is non-linear.
+ * * The CRTC CTM will be placed in the gamut remap block if it is non-linear.
*
* The RGM block is typically more fully featured and accurate across
* all ASICs - DCE can't support a custom non-linear CRTC DGM.
@@ -338,7 +390,9 @@ int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state)
* management at once we have to either restrict the usage of CRTC properties
* or blend adjustments together.
*
- * Returns 0 on success.
+ * Returns:
+ * 0 on success.
+ * Error code if setup fails.
*/
int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
{
@@ -373,7 +427,7 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
/* Setup regamma and degamma. */
if (is_legacy) {
- /*
+ /**
* Legacy regamma forces us to use the sRGB RGM as a base.
* This also means we can't use linear DGM since DGM needs
* to use sRGB as a base as well, resulting in incorrect CRTC
@@ -393,7 +447,8 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
if (r)
return r;
} else if (has_regamma) {
- /* CRTC RGM goes into RGM LUT. */
+ /**
+ * If atomic regamma, CRTC RGM goes into RGM LUT. */
stream->out_transfer_func->type = TF_TYPE_DISTRIBUTED_POINTS;
stream->out_transfer_func->tf = TRANSFER_FUNCTION_LINEAR;
@@ -402,7 +457,7 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
if (r)
return r;
} else {
- /*
+ /**
* No CRTC RGM means we can just put the block into bypass
* since we don't have any plane level adjustments using it.
*/
@@ -410,7 +465,7 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
stream->out_transfer_func->tf = TRANSFER_FUNCTION_LINEAR;
}
- /*
+ /**
* CRTC DGM goes into DGM LUT. It would be nice to place it
* into the RGM since it's a more featured block but we'd
* have to place the CTM in the OCSC in that case.
@@ -421,7 +476,7 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
if (crtc->base.ctm) {
ctm = (struct drm_color_ctm *)crtc->base.ctm->data;
- /*
+ /**
* Gamut remapping must be used for gamma correction
* since it comes before the regamma correction.
*
@@ -452,7 +507,9 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
* preparation for hardware commit. The transfer function used depends on
* the prepartion done on the stream for color management.
Could you fix this typo while you are here? prepartion -> preparation
*
- * Returns 0 on success.
+ * Returns:
+ * 0 on success.
+ * ENOMEM if mem allocation fails.
*/
int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
struct dc_plane_state *dc_plane_state)
Thanks for creating more documentation!
Kind regards,
Tales
