| Message ID | 20220716222529.421115-2-mwen@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Documentation/amdgpu/display: describe color and blend mode properties mapping | expand |
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@amd.com> > Signed-off-by: Melissa Wen <mwen@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 /*. > -/* > - * 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. > * 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 > */ > 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
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@amd.com> > > Signed-off-by: Melissa Wen <mwen@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. > > > -/* > > - * 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. > > > * 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
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@amd.com> >>> Signed-off-by: Melissa Wen <mwen@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
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; } -/* - * 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 __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) { - /* + /** * 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. */ 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. * - * 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)