| Message ID | 20200608141657.GB1912173@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd/display: Fix indenting in dcn30_set_output_transfer_func() | expand |
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? Also, because this code uses very long identifiers, it would read better using wider columns as the logic in the code itself isn't complicated but the 80 column wrapping makes it seem so. Wrapping could be something like: --- 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 ab20320ebc99..56e91a73610f 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c +++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c @@ -190,18 +190,16 @@ bool dcn30_set_output_transfer_func(struct dc *dc, struct pwl_params *params = NULL; bool ret = false; - /* program OGAM or 3DLUT only for the top pipe*/ + /* program OGAM or 3DLUT only for the top pipe */ if (pipe_ctx->top_pipe == NULL) { - /*program rmu shaper and 3dlut in MPC*/ + /* program rmu shaper and 3dlut in MPC */ ret = dcn30_set_mpc_shaper_3dlut(pipe_ctx, stream); - if (ret == false && mpc->funcs->set_output_gamma && stream->out_transfer_func) { + if (!ret && mpc->funcs->set_output_gamma && stream->out_transfer_func) { if (stream->out_transfer_func->type == TF_TYPE_HWPWL) params = &stream->out_transfer_func->pwl; - else if (pipe_ctx->stream->out_transfer_func->type == - TF_TYPE_DISTRIBUTED_POINTS && - cm3_helper_translate_curve_to_hw_format( - stream->out_transfer_func, - &mpc->blender_params, false)) + else if (pipe_ctx->stream->out_transfer_func->type == TF_TYPE_DISTRIBUTED_POINTS && + cm3_helper_translate_curve_to_hw_format(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)
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. There are lots of ugly stuff which is not confusing like this: (The line numbers are from next-20200605). 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 drivers/gpu/drm/amd/amdgpu/../display/dc/dcn30/dcn30_dpp.c:747 dpp3_get_shaper_current() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dcn30/dcn30_dpp_cm.c:67 dpp30_get_gamcor_current() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dcn30/dcn30_mpc.c:116 mpc3_get_ogam_current() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dcn30/dcn30_mpc.c:432 mpc3_get_shaper_current() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dcn30/dcn30_resource.c:2351 dcn30_update_bw_bounding_box() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dcn30/dcn30_optc.c:178 optc3_set_dsc_config() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn20/display_mode_vba_20.c:2704 dml20_DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn20/display_mode_vba_20v2.c:2777 dml20v2_DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn21/display_mode_vba_21.c:2633 DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn21/display_mode_vba_21.c:5031 dml21_ModeSupportAndSystemConfigurationFull() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn21/display_mode_vba_21.c:5036 dml21_ModeSupportAndSystemConfigurationFull() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn21/display_mode_vba_21.c:5056 dml21_ModeSupportAndSystemConfigurationFull() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/../display/modules/power/power_helpers.c:731 dmcu_load_iram() warn: inconsistent indenting drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c:5062 gfx_v8_0_pre_soft_reset() warn: inconsistent indenting regards, dan carpenter
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; }
On Mon, Jun 8, 2020 at 10:17 AM Dan Carpenter <dan.carpenter@oracle.com> 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> Applied. thanks! Alex > --- > 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(); > } > } > > -- > 2.26.2 > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
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(); } }
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(-)