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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxx>
---
  .../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;
  }




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux