[PATCH] drm/amd/display/dc: Remove dc code repetition

[Rodrigo Siqueira Jordao]

Hi Joao,

First of all, thanks for your patch. Follows some of my comments:

 > On 6/13/24 10:05 AM, Joao Paulo Pereira da Silva wrote:

You can drop the dc part in the commit title. Also, the title should 
highlight that you are decoupling one part of the code in a single place 
to avoid duplication.

> Code is repeated in functions optc1_enable_crtc
> (dc/optc/dcn10/dcn10_optc.c) and optc2_enable_crtc
> (dc/optc/dcn20/dcn20_optc.c).
> 
> So, remove it with the creation of a macro.
> 
> Signed-off-by: Joao Paulo Pereira da Silva <jppaulo11 at usp.br>
> ---
>   .../amd/display/dc/optc/dcn10/dcn10_optc.c    | 29 ++-----------------
>   .../amd/display/dc/optc/dcn10/dcn10_optc.h    | 27 +++++++++++++++++
>   .../amd/display/dc/optc/dcn20/dcn20_optc.c    | 29 ++-----------------
>   3 files changed, 33 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/optc/dcn10/dcn10_optc.c b/drivers/gpu/drm/amd/display/dc/optc/dcn10/dcn10_optc.c
> index 5574bc628053..facdeeb41250 100644
> --- a/drivers/gpu/drm/amd/display/dc/optc/dcn10/dcn10_optc.c
> +++ b/drivers/gpu/drm/amd/display/dc/optc/dcn10/dcn10_optc.c
> @@ -41,6 +41,8 @@
>   
>   #define STATIC_SCREEN_EVENT_MASK_RANGETIMING_DOUBLE_BUFFER_UPDATE_EN 0x100
>   
> +#define OPTC_SRC_SEL_FIELD OPTC_SRC_SEL
> +
>   /**
>    * apply_front_porch_workaround() - This is a workaround for a bug that has
>    *                                  existed since R5xx and has not been fixed
> @@ -517,32 +519,7 @@ void optc1_enable_optc_clock(struct timing_generator *optc, bool enable)
>    */
>   static bool optc1_enable_crtc(struct timing_generator *optc)
>   {
> -	/* TODO FPGA wait for answer
> -	 * OTG_MASTER_UPDATE_MODE != CRTC_MASTER_UPDATE_MODE
> -	 * OTG_MASTER_UPDATE_LOCK != CRTC_MASTER_UPDATE_LOCK
> -	 */
> -	struct optc *optc1 = DCN10TG_FROM_TG(optc);
> -
> -	/* opp instance for OTG. For DCN1.0, ODM is remoed.
> -	 * OPP and OPTC should 1:1 mapping
> -	 */
> -	REG_UPDATE(OPTC_DATA_SOURCE_SELECT,
> -			OPTC_SRC_SEL, optc->inst);
> -
> -	/* VTG enable first is for HW workaround */
> -	REG_UPDATE(CONTROL,
> -			VTG0_ENABLE, 1);
> -
> -	REG_SEQ_START();
> -
> -	/* Enable CRTC */
> -	REG_UPDATE_2(OTG_CONTROL,
> -			OTG_DISABLE_POINT_CNTL, 3,
> -			OTG_MASTER_EN, 1);
> -
> -	REG_SEQ_SUBMIT();
> -	REG_SEQ_WAIT_DONE();
> -
> +	_optc1_enable_crtc(optc);
>   	return true;
>   }
>   
> diff --git a/drivers/gpu/drm/amd/display/dc/optc/dcn10/dcn10_optc.h b/drivers/gpu/drm/amd/display/dc/optc/dcn10/dcn10_optc.h
> index 2f3bd7648ba7..aea80fa6fe91 100644
> --- a/drivers/gpu/drm/amd/display/dc/optc/dcn10/dcn10_optc.h
> +++ b/drivers/gpu/drm/amd/display/dc/optc/dcn10/dcn10_optc.h
> @@ -604,4 +604,31 @@ struct dcn_optc_mask {
>   
>   void dcn10_timing_generator_init(struct optc *optc);
>   
> +#define _optc1_enable_crtc(optc)					\

Let's avoid the introduction of a macro. Just create a function for that.

> +	do {								\
> +		/* TODO FPGA wait for answer */				\

You can drop this comment.

> +		/* OTG_MASTER_UPDATE_MODE != CRTC_MASTER_UPDATE_MODE */	\
> +		/* OTG_MASTER_UPDATE_LOCK != CRTC_MASTER_UPDATE_LOCK */	\
> +		struct optc *optc1 = DCN10TG_FROM_TG(optc);		\
> +									\
> +		/* opp instance for OTG. For DCN1.0, ODM is remoed. */	\

I think the original comment had a typo, I guess it should be "removed" 
instead of "remoed". Can you also fix this?

> +		/* OPP and OPTC should 1:1 mapping */			\
> +		REG_UPDATE(OPTC_DATA_SOURCE_SELECT,			\
> +				OPTC_SRC_SEL_FIELD, optc->inst);	\
> +									\
> +		/* VTG enable first is for HW workaround */		\
> +		REG_UPDATE(CONTROL,					\
> +				VTG0_ENABLE, 1);			\
> +									\
> +		REG_SEQ_START();					\
> +									\
> +		/* Enable CRTC */					\
> +		REG_UPDATE_2(OTG_CONTROL,				\
> +				OTG_DISABLE_POINT_CNTL, 3,		\
> +				OTG_MASTER_EN, 1);			\

Maybe you can convert this patch into a patchset, where the first patch 
moves code around, and the following patches fix typos and code style, 
such as the parenthesis aligment with the parameters.

> +									\
> +		REG_SEQ_SUBMIT();					\
> +		REG_SEQ_WAIT_DONE();					\
> +	} while (0)
> +

I was thinking... do we have more cases like this one? If so, maybe we 
can create a generic optc.c file. Anyway, this is just one idea to add 
to your radar for future patches.

Thanks
Siqueira

>   #endif /* __DC_TIMING_GENERATOR_DCN10_H__ */
> diff --git a/drivers/gpu/drm/amd/display/dc/optc/dcn20/dcn20_optc.c b/drivers/gpu/drm/amd/display/dc/optc/dcn20/dcn20_optc.c
> index d6f095b4555d..012e0c52aeec 100644
> --- a/drivers/gpu/drm/amd/display/dc/optc/dcn20/dcn20_optc.c
> +++ b/drivers/gpu/drm/amd/display/dc/optc/dcn20/dcn20_optc.c
> @@ -37,6 +37,8 @@
>   #define FN(reg_name, field_name) \
>   	optc1->tg_shift->field_name, optc1->tg_mask->field_name
>   
> +#define OPTC_SRC_SEL_FIELD OPTC_SEG0_SRC_SEL
> +
>   /**
>    * optc2_enable_crtc() - Enable CRTC - call ASIC Control Object to enable Timing generator.
>    *
> @@ -47,32 +49,7 @@
>    */
>   bool optc2_enable_crtc(struct timing_generator *optc)
>   {
> -	/* TODO FPGA wait for answer
> -	 * OTG_MASTER_UPDATE_MODE != CRTC_MASTER_UPDATE_MODE
> -	 * OTG_MASTER_UPDATE_LOCK != CRTC_MASTER_UPDATE_LOCK
> -	 */
> -	struct optc *optc1 = DCN10TG_FROM_TG(optc);
> -
> -	/* opp instance for OTG. For DCN1.0, ODM is remoed.
> -	 * OPP and OPTC should 1:1 mapping
> -	 */
> -	REG_UPDATE(OPTC_DATA_SOURCE_SELECT,
> -			OPTC_SEG0_SRC_SEL, optc->inst);
> -
> -	/* VTG enable first is for HW workaround */
> -	REG_UPDATE(CONTROL,
> -			VTG0_ENABLE, 1);
> -
> -	REG_SEQ_START();
> -
> -	/* Enable CRTC */
> -	REG_UPDATE_2(OTG_CONTROL,
> -			OTG_DISABLE_POINT_CNTL, 3,
> -			OTG_MASTER_EN, 1);
> -
> -	REG_SEQ_SUBMIT();
> -	REG_SEQ_WAIT_DONE();
> -
> +	_optc1_enable_crtc(optc);
>   	return true;
>   }
>