[PATCH 1/4] Documentation/amdgpu_dm: Add DM color correction documentation

Rodrigo Siqueira Jordao Rodrigo.Siqueira at amd.com
Fri Jul 22 21:23:10 UTC 2022



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 at amd.com>
>>> Signed-off-by: Melissa Wen <mwen at igalia.com>
>>> ---
>>>    .../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



More information about the amd-gfx mailing list