Re: [PATCH] drm/amd/display: fix documentation warnings for mpc.h

[Rodrigo Siqueira Jordao <Rodrigo.Siqueira@xxxxxxx>]

Hi Marcelo,

First of all, thanks a lot for your patch! Please check some of my 
inline comments.

On 4/27/24 10:05 AM, Marcelo Mendes Spessoto Junior wrote:
> Fix most of the display documentation compile warnings by
> documenting struct mpc_funcs functions in dc/inc/hw/mpc.h file.

Could you add the warnings that you fixed in the commit message?

I think some of your changes in this patch address some of the issues 
reported by Stephan Rothwell:

https://lore.kernel.org/all/20240130134954.04fcf763@xxxxxxxxxxxxxxxx/

Please include Stephan in the new version of this patch.

Also, use the Fixes tags pointing to:

b8c1c3a82e75 ("Documentation/gpu: Add kernel doc entry for MPC")


> Signed-off-by: Marcelo Mendes Spessoto Junior <marcelomspessoto@xxxxxxxxx>
> ---
>   drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h | 372 +++++++++++++++++++-
>   1 file changed, 369 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h b/drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h
> index 34a398f23..388b96c32 100644
> --- a/drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h
> +++ b/drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h
> @@ -1,4 +1,5 @@
> -/* Copyright 2012-15 Advanced Micro Devices, Inc.
> +/*
> + * Copyright 2012-15 Advanced Micro Devices, Inc.
>    *
>    * Permission is hereby granted, free of charge, to any person obtaining a
>    * copy of this software and associated documentation files (the "Software"),
> @@ -282,6 +283,21 @@ struct mpcc_state {
>    * struct mpc_funcs - funcs
>    */
>   struct mpc_funcs {
> +  /**
> +   * @read_mpcc_state:
> +   *
> +   * Read register content from given MPCC physical instance.

It looks like your text editor left some extra spaces at the end of the 
string in many parts of this patch. Remove this extra space at the end 
of some of the strings in this patch for the next version. If you use 
git log -p after applying your patch, you can visually see where the 
extra space is at the end of each line.

> +   *
> +   * Parameters:
> +   *
> +   * - [in/out] mpc - MPC context
> +   * - [in] mpcc_instance - MPC context instance
> +   * - [in] mpcc_state - MPC context state
> +   *
> +   * Return:
> +   *
> +   * void
> +   */

Use the same indentation as the field/functions that you are documenting.

Finally, ensure you are using the latest amd-staging-drm-next to base 
your patch. I believe you have some merge conflicts since mpc.h has some 
changes.

Thanks
Siqueira

>   	void (*read_mpcc_state)(
>   			struct mpc *mpc,
>   			int mpcc_inst,
> @@ -346,12 +362,22 @@ struct mpc_funcs {
>   	 * Parameters:
>   	 *
>   	 * - [in/out] mpc - MPC context.
> -	 *
> +	 *
>   	 * Return:
>   	 *
>   	 * void
>   	 */
>   	void (*mpc_init)(struct mpc *mpc);
> +
> +  /**
> +   * @mpc_init_single_inst:
> +   *
> +   * Initialize given MPCC physical instance.
> +   *
> +   * Parameters:
> +   * - [in/out] mpc - MPC context.
> +   * - [in] mpcc_id - The MPCC physical instance to be initialized.
> +   */
>   	void (*mpc_init_single_inst)(
>   			struct mpc *mpc,
>   			unsigned int mpcc_id);
> @@ -449,62 +475,282 @@ struct mpc_funcs {
>   			struct mpc_tree *tree,
>   			struct mpcc *mpcc);
>   
> +  /**
> +   * @get_mpcc_for_dpp_from_secondary:
> +   *
> +   * Find, if it exists, a MPCC from a given 'secondary' MPC tree that
> +   * is associated with specified plane.
> +   *
> +   * Parameters:
> +   * - [in/out] tree - MPC tree structure to search for plane.
> +   * - [in] dpp_id - DPP to be searched.
> +   *
> +   * Return:
> +   *
> +   * struct mpcc* - pointer to plane or NULL if no plane found.
> +  */
>   	struct mpcc* (*get_mpcc_for_dpp_from_secondary)(
>   			struct mpc_tree *tree,
>   			int dpp_id);
>   
> +  /**
> +   * @get_mpcc_for_dpp:
> +   *
> +   * Find, if it exists, a MPCC from a given MPC tree that
> +   * is associated with specified plane.
> +   *
> +   * Parameters:
> +   * - [in/out] tree - MPC tree structure to search for plane.
> +   * - [in] dpp_id - DPP to be searched.
> +   *
> +   * Return:
> +   *
> +   * struct mpcc* - pointer to plane or NULL if no plane found.
> +  */
>   	struct mpcc* (*get_mpcc_for_dpp)(
>   			struct mpc_tree *tree,
>   			int dpp_id);
>   
> +  /**
> +   * @wait_for_idle:
> +   *
> +   * Wait for a MPCC in MPC context to enter idle state.
> +   *
> +   * Parameters:
> +   * - [in/out] mpc - MPC Context.
> +   * - [in] id - MPCC to wait for idle state.
> +   *
> +   * Return:
> +   *
> +   * void
> +  */
>   	void (*wait_for_idle)(struct mpc *mpc, int id);
>   
> +  /**
> +   * @assert_mpcc_idle_before_connect:
> +   *
> +   * Assert if MPCC in MPC context is in idle state.
> +   *
> +   * Parameters:
> +   * - [in/out] mpc - MPC context.
> +   * - [in] id - MPCC to assert idle state.
> +   *
> +   * Return:
> +   *
> +   * void
> +  */
>   	void (*assert_mpcc_idle_before_connect)(struct mpc *mpc, int mpcc_id);
>   
> +  /**
> +   * @init_mpcc_list_from_hw:
> +   *
> +   * Iterate through the MPCC array from a given MPC context struct
> +   * and configure each MPCC according to its registers' values.
> +   *
> +   * Parameters:
> +   * - [in/out] mpc - MPC context to initialize MPCC array.
> +   * - [in/out] tree - MPC tree structure containing MPCC contexts to initialize.
> +   *
> +   * Return:
> +   *
> +   * void
> +  */
>   	void (*init_mpcc_list_from_hw)(
>   		struct mpc *mpc,
>   		struct mpc_tree *tree);
>   
> +  /**
> +  * @set_denorm:
> +  *
> +  * Set corresponding OPP DENORM_CONTROL register value to specific denorm_mode
> +  * based on given color depth.
> +  *
> +  * Parameters:
> +  * - [in/out] mpc - MPC context.
> +  * - [in] opp_id - Corresponding OPP to update register.
> +  * - [in] output_depth - Arbitrary color depth to set denorm_mode.
> +  *
> +  * Return:
> +  *
> +  * void
> +  */
>   	void (*set_denorm)(struct mpc *mpc,
>   			int opp_id,
>   			enum dc_color_depth output_depth);
>   
> +  /**
> +  * @set_denorm_clamp:
> +  *
> +  * Set denorm clamp values on corresponding OPP DENORM CONTROL register.
> +  *
> +  * Parameters:
> +  * - [in/out] mpc - MPC context.
> +  * - [in] opp_id - Corresponding OPP to update register.
> +  * - [in] denorm_clamp - Arbitrary denorm clamp to be set.
> +  *
> +  * Return:
> +  *
> +  * void
> +  */
>   	void (*set_denorm_clamp)(
>   			struct mpc *mpc,
>   			int opp_id,
>   			struct mpc_denorm_clamp denorm_clamp);
>   
> +  /**
> +  * @set_output_csc:
> +  *
> +  * Set the Output Color Space Conversion matrix
> +  * with given values and mode.
> +  *
> +  * Parameters:
> +  * - [in/out] mpc - MPC context.
> +  * - [in] opp_id - Corresponding OPP to update register.
> +  * - [in] regval - Values to set in CSC matrix.
> +  * - [in] ocsc_mode - Mode to set CSC.
> +  *
> +  * Return:
> +  *
> +  * void
> +  */
>   	void (*set_output_csc)(struct mpc *mpc,
>   			int opp_id,
>   			const uint16_t *regval,
>   			enum mpc_output_csc_mode ocsc_mode);
>   
> +  /**
> +  * @set_ocsc_default:
> +  *
> +  * Set the Output Color Space Conversion matrix
> +  * to default values according to color space.
> +  *
> +  * Parameters:
> +  * - [in/out] mpc - MPC context.
> +  * - [in] opp_id - Corresponding OPP to update register.
> +  * - [in] color_space - OCSC color space.
> +  * - [in] ocsc_mode - Mode to set CSC.
> +  *
> +  * Return:
> +  *
> +  * void
> +  *
> +  */
>   	void (*set_ocsc_default)(struct mpc *mpc,
>   			int opp_id,
>   			enum dc_color_space color_space,
>   			enum mpc_output_csc_mode ocsc_mode);
>   
> +  /**
> +  * @set_output_gamma:
> +  *
> +  * Set Output Gamma with given curve parameters.
> +  *
> +  * Parameters:
> +  * - [in/out] mpc - MPC context.
> +  * - [in] mpcc_id - Corresponding MPC to update registers.
> +  * - [in] params - Parameters.
> +  *
> +  * Return:
> +  *
> +  * void
> +  *
> +  */
>   	void (*set_output_gamma)(
>   			struct mpc *mpc,
>   			int mpcc_id,
>   			const struct pwl_params *params);
> +  /**
> +  * @power_on_mpc_mem_pwr:
> +  *
> +  * Power on/off memory LUT for given MPCC.
> +  * Powering on enables LUT to be updated.
> +  * Powering off allows entering low power mode.
> +  *
> +  * Parameters:
> +  * - [in/out] mpc - MPC context.
> +  * - [in] mpcc_id - MPCC to power on.
> +  * - [in] power_on
> +  *
> +  * Return:
> +  *
> +  * void
> +  */
>   	void (*power_on_mpc_mem_pwr)(
>   			struct mpc *mpc,
>   			int mpcc_id,
>   			bool power_on);
> +  /**
> +  * @set_dwb_mux:
> +  *
> +  * Set corresponding Display Writeback mux
> +  * MPC register field to given MPCC id.
> +  *
> +  * Parameters:
> +  * - [in/out] mpc - MPC context.
> +  * - [in] dwb_id - DWB to be set.
> +  * - [in] mpcc_id - MPCC id to be stored in DWB mux register.
> +  *
> +  * Return:
> +  *
> +  * void
> +  */
>   	void (*set_dwb_mux)(
>   			struct mpc *mpc,
>   			int dwb_id,
>   			int mpcc_id);
>   
> +  /**
> +  * @disable_dwb_mux:
> +  *
> +  * Reset corresponding Display Writeback mux
> +  * MPC register field.
> +  *
> +  * Parameters:
> +  * - [in/out] mpc - MPC context.
> +  * - [in] dwb_id - DWB to be set.
> +  *
> +  * Return:
> +  *
> +  * void
> +  */
>   	void (*disable_dwb_mux)(
>   		struct mpc *mpc,
>   		int dwb_id);
> -
> +
> +  /**
> +  * @is_dwb_idle:
> +  *
> +  * Check DWB status on MPC_DWB0_MUX_STATUS register field.
> +  * Return if it is null.
> +  *
> +  * Parameters:
> +  * - [in/out] mpc - MPC context.
> +  * - [in] dwb_id - DWB to be checked.
> +  *
> +  * Return:
> +  *
> +  * bool - wheter DWB is idle or not
> +  */
>   	bool (*is_dwb_idle)(
>   		struct mpc *mpc,
>   		int dwb_id);
>   
> +  /**
> +  * @set_out_rate_control:
> +  *
> +  * Set display output rate control.
> +  *
> +  * Parameters:
> +  * - [in/out] mpc - MPC context.
> +  * - [in] opp_id - OPP to be set.
> +  * - [in] enable
> +  * - [in] rate_2x_mode
> +  * - [in] flow_control
> +  *
> +  * Return:
> +  *
> +  * void
> +  */
>   	void (*set_out_rate_control)(
>   		struct mpc *mpc,
>   		int opp_id,
> @@ -512,37 +758,157 @@ struct mpc_funcs {
>   		bool rate_2x_mode,
>   		struct mpc_dwb_flow_control *flow_control);
>   
> +  /**
> +  * @set_gamut_remap:
> +  *
> +  * Set post-blending CTM for given MPCC.
> +  *
> +  * Parameters:
> +  * - [in] mpc - MPC context.
> +  * - [in] mpcc_id - MPCC to set gamut map.
> +  * - [in] adjust
> +  *
> +  * Return:
> +  *
> +  * void
> +  */
>   	void (*set_gamut_remap)(
>   			struct mpc *mpc,
>   			int mpcc_id,
>   			const struct mpc_grph_gamut_adjustment *adjust);
>   
> +  /**
> +  * @program_1dlut:
> +  *
> +  * Set 1 dimensional Lookup Table.
> +  *
> +  * Parameters:
> +  * - [in/out] mpc - MPC context
> +  * - [in] params - curve parameters for the LUT configuration
> +  * - [in] rmu_idx
> +  *
> +  * bool - wheter LUT was set (set with given parameters) or not (params is NULL and LUT is disabled).
> +  */
>   	bool (*program_1dlut)(
>   			struct mpc *mpc,
>   			const struct pwl_params *params,
>   			uint32_t rmu_idx);
>   
> +  /**
> +  * @program_shaper:
> +  *
> +  * Set shaper.
> +  *
> +  * Parameters:
> +  * - [in/out] mpc - MPC context
> +  * - [in] params - curve parameters to be set
> +  * - [in] rmu_idx
> +  *
> +  * Return:
> +  *
> +  * bool - wheter shaper was set (set with given parameters) or not (params is NULL and LUT is disabled).
> +  */
>   	bool (*program_shaper)(
>   			struct mpc *mpc,
>   			const struct pwl_params *params,
>   			uint32_t rmu_idx);
>   
> +  /**
> +  * @acquire_rmu:
> +  *
> +  * Set given MPCC to be multiplexed to given RMU unit.
> +  *
> +  * Parameters:
> +  * - [in/out] mpc - MPC context
> +  * - [in] mpcc_id - MPCC
> +  * - [in] rmu_idx - Given RMU unit to set MPCC to be multiplexed to.
> +  *
> +  * Return:
> +  *
> +  * unit32_t - rmu_idx if operation was successful, -1 else.
> +  */
>   	uint32_t (*acquire_rmu)(struct mpc *mpc, int mpcc_id, int rmu_idx);
>   
> +  /**
> +  * @program_3dlut:
> +  *
> +  * Set 3 dimensional Lookup Table.
> +  *
> +  * Parameters:
> +  * - [in/out] mpc - MPC context
> +  * - [in] params - tetrahedral parameters for the LUT configuration
> +  * - [in] rmu_idx
> +  *
> +  * bool - wheter LUT was set (set with given parameters) or not (params is NULL and LUT is disabled).
> +  */
>   	bool (*program_3dlut)(
>   			struct mpc *mpc,
>   			const struct tetrahedral_params *params,
>   			int rmu_idx);
>   
> +  /**
> +  * @release_rmu:
> +  *
> +  * For a given MPCC, release the RMU unit it muliplexes to.
> +  *
> +  * Parameters:
> +  * - [in/out] mpc - MPC context
> +  * - [in] mpcc_id - MPCC
> +  *
> +  * Return:
> +  *
> +  * int - a valid rmu_idx representing released RMU unit or -1 if there was no RMU unit to release.
> +  */
>   	int (*release_rmu)(struct mpc *mpc, int mpcc_id);
>   
> +  /**
> +  * @get_mpc_out_mux:
> +  *
> +  * Return MPC out mux.
> +  *
> +  * Parameters:
> +  * - [in] mpc - MPC context.
> +  * - [in] opp_id - OPP
> +  *
> +  * Return:
> +  *
> +  * unsigned int - Out Mux
> +  */
>   	unsigned int (*get_mpc_out_mux)(
>   			struct mpc *mpc,
>   			int opp_id);
>   
> +  /**
> +   * @set_bg_color:
> +   *
> +   * Find corresponding bottommost MPCC and
> +   * set its bg color.
> +   *
> +   * Parameters:
> +   * - [in/out] mpc - MPC context.
> +   * - [in] bg_color - background color to be set.
> +   * - [in] mpcc_id
> +   *
> +   * Return:
> +   *
> +   * void
> +  */
>   	void (*set_bg_color)(struct mpc *mpc,
>   			struct tg_color *bg_color,
>   			int mpcc_id);
> +
> +  /**
> +  * @set_mpc_mem_lp_mode:
> +  *
> +  * Set mpc_mem_lp_mode
> +  *
> +  * Parameters:
> +  * - [in/out] mpc
> +  *
> +  * Return:
> +  *
> +  * void
> +  */
>   	void (*set_mpc_mem_lp_mode)(struct mpc *mpc);
>   };
>