On Mon, 2020-06-08 at 20:49 +0300, Dan Carpenter wrote:

On Mon, Jun 08, 2020 at 10:16:27AM -0700, Joe Perches wrote:

...

On Mon, 2020-06-08 at 17:16 +0300, Dan Carpenter wrote:

...

These lines are a part of the if statement and they are supposed to be indented one more tab.

Signed-off-by: Dan Carpenter dan.carpenter@oracle.com

drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c index ab20320ebc994..37c310dbb3665 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c +++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c @@ -203,9 +203,9 @@ bool dcn30_set_output_transfer_func(struct dc *dc, stream->out_transfer_func, &mpc->blender_params, false)) params = &mpc->blender_params;

•  /* there are no ROM LUTs in OUTGAM */
  

• if (stream->out_transfer_func->type == TF_TYPE_PREDEFINED)
  

• 	BREAK_TO_DEBUGGER();
  

• 	 /* there are no ROM LUTs in OUTGAM */
  

• 	if (stream->out_transfer_func->type == TF_TYPE_PREDEFINED)
  

• 		BREAK_TO_DEBUGGER();
  

  } }

Maybe the if is at the right indentation but the close brace below the if is misplaced instead?

Yeah. I considered that, but the code is correct, it's just the indenting is wrong. I normally leave drm/amd/ code alone but this indenting was so confusing that I though it was worth fixing.

This file seems to heavily use function pointers, multiple dereferences with visually similar identifiers, and it generally makes my eyes hurt reading the code.

There are lots of ugly stuff which is not confusing like this: (The line numbers are from next-20200605).

Ick. Don't give me line numbers. Now I might have to look...

drivers/gpu/drm/amd/amdgpu/../powerplay/amd_powerplay.c:1530 pp_asic_reset_mode_2() warn: inconsistent indenting

drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dce_calcs.c:3387 bw_calcs() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dcn20/dcn20_dwb.c:104 dwb2_enable() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dcn20/dcn20_dpp_cm.c:450 dpp20_get_blndgam_current() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dcn20/dcn20_dpp_cm.c:543 dpp20_get_shaper_current() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dcn20/dcn20_mpc.c:306 mpc20_get_ogam_current() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dp.c:1519 dc_link_dp_perform_link_training() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link.c:3137 core_link_enable_stream() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dcn30/dcn30_hwseq.c:207 dcn30_set_output_transfer_func() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dcn30/dcn30_dpp.c:650 dpp3_get_blndgam_current() warn: inconsistent indenting

OK, so I picked this one at random.

It looks like someone avoided using intentional programming along with copy/paste combined with being lazy.

It seems as if AMD should use more code reviewers and perhaps some automated code reformatters before submitting their code.

This code is:

static enum dc_lut_mode dpp3_get_blndgam_current(struct dpp *dpp_base) { enum dc_lut_mode mode; uint32_t mode_current = 0; uint32_t in_use = 0;

struct dcn3_dpp *dpp = TO_DCN30_DPP(dpp_base);

REG_GET(CM_BLNDGAM_CONTROL, CM_BLNDGAM_MODE_CURRENT, &mode_current); REG_GET(CM_BLNDGAM_CONTROL, CM_BLNDGAM_SELECT_CURRENT, &in_use);

switch (mode_current) { case 0: case 1: mode = LUT_BYPASS; break;

case 2: if (in_use == 0) mode = LUT_RAM_A; else mode = LUT_RAM_B; break; default: mode = LUT_BYPASS; break; } return mode; }

Generic style defects:

o unnecessary initializations o uint32_t where u32 is simpler o doesn't fill to 80 columns where reasonable o magic numbers o duplicated switch/case blocks o unnecessary code: in_use is only used by case 2 dpp doesn't seem used at all, but it is via a hidden CTX in the REG_GET macro

drivers/gpu/drm/amd/display/dc/inc/reg_helper.h:#define REG_GET(reg_name, field, val) \ drivers/gpu/drm/amd/display/dc/inc/reg_helper.h- generic_reg_get(CTX, REG(reg_name), \ drivers/gpu/drm/amd/display/dc/inc/reg_helper.h- FN(reg_name, field), val)

And no, I'm not going to look at the entire list...

But something like this could be simpler:

{ struct dcn3_dpp *dpp = TO_DCN30_DPP(dpp_base); u32 mode_current; u32 in_use;

REG_GET(CM_BLNDGAM_CONTROL, CM_BLNDGAM_MODE_CURRENT, &mode_current); if (mode_current != 2) return LUT_BYPASS;

REG_GET(CM_BLNDGAM_CONTROL, CM_BLNDGAM_SELECT_CURRENT, &in_use); return !in_use ? LUT_RAM_A : LUT_RAM_B; }