Re: [PATCH 1/4] Documentation/amdgpu_dm: Add DM color correction documentation
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
Signed-off-by: Melissa Wen
---
.../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 0x
-/*
- * 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
Re: [PATCH 1/4] Documentation/amdgpu_dm: Add DM color correction documentation
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
> > Signed-off-by: Melissa Wen
> > ---
> > .../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 0x
> > -/*
> > - * 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.
>
> > -/*
> > - * 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
Re: [PATCH 1/4] Documentation/amdgpu_dm: Add DM color correction documentation
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
Signed-off-by: Melissa Wen
---
.../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 0x
-/*
- * 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 /*.
-/*
- * 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
[PATCH 1/4] Documentation/amdgpu_dm: Add DM color correction documentation
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
Signed-off-by: Melissa Wen
---
.../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 0x
-/*
- * 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;
}
-/*
- * 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
+ */
static int
