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