* [PATCH] drm/amd/display: Fix indenting in dcn30_set_output_transfer_func() @ 2020-06-08 14:16 Dan Carpenter 2020-06-08 17:16 ` Joe Perches 2020-06-08 20:16 ` Alex Deucher 0 siblings, 2 replies; 5+ messages in thread From: Dan Carpenter @ 2020-06-08 14:16 UTC (permalink / raw) To: Harry Wentland Cc: Leo Li, kernel-janitors, linux-kernel, amd-gfx, David Airlie, dri-devel, Daniel Vetter, Alex Deucher, Bhawanpreet Lakha, Christian König, Dan Carpenter 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(); } } -- 2.26.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amd/display: Fix indenting in dcn30_set_output_transfer_func() 2020-06-08 14:16 [PATCH] drm/amd/display: Fix indenting in dcn30_set_output_transfer_func() Dan Carpenter @ 2020-06-08 17:16 ` Joe Perches 2020-06-08 17:49 ` Dan Carpenter 2020-06-08 20:16 ` Alex Deucher 1 sibling, 1 reply; 5+ messages in thread From: Joe Perches @ 2020-06-08 17:16 UTC (permalink / raw) To: Dan Carpenter, Harry Wentland Cc: Leo Li, kernel-janitors, linux-kernel, amd-gfx, David Airlie, dri-devel, Daniel Vetter, Alex Deucher, Bhawanpreet Lakha, Christian König 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) ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amd/display: Fix indenting in dcn30_set_output_transfer_func() 2020-06-08 17:16 ` Joe Perches @ 2020-06-08 17:49 ` Dan Carpenter 2020-06-08 19:10 ` Joe Perches 0 siblings, 1 reply; 5+ messages in thread From: Dan Carpenter @ 2020-06-08 17:49 UTC (permalink / raw) To: Joe Perches Cc: Leo Li, Bhawanpreet Lakha, kernel-janitors, linux-kernel, amd-gfx, David Airlie, dri-devel, Daniel Vetter, Alex Deucher, Harry Wentland, Christian König 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amd/display: Fix indenting in dcn30_set_output_transfer_func() 2020-06-08 17:49 ` Dan Carpenter @ 2020-06-08 19:10 ` Joe Perches 0 siblings, 0 replies; 5+ messages in thread From: Joe Perches @ 2020-06-08 19:10 UTC (permalink / raw) To: Dan Carpenter Cc: Leo Li, Bhawanpreet Lakha, kernel-janitors, linux-kernel, amd-gfx, David Airlie, dri-devel, Daniel Vetter, Alex Deucher, Harry Wentland, Christian König 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; } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amd/display: Fix indenting in dcn30_set_output_transfer_func() 2020-06-08 14:16 [PATCH] drm/amd/display: Fix indenting in dcn30_set_output_transfer_func() Dan Carpenter 2020-06-08 17:16 ` Joe Perches @ 2020-06-08 20:16 ` Alex Deucher 1 sibling, 0 replies; 5+ messages in thread From: Alex Deucher @ 2020-06-08 20:16 UTC (permalink / raw) To: Dan Carpenter Cc: Leo Li, Bhawanpreet Lakha, kernel-janitors, LKML, amd-gfx list, David Airlie, Maling list - DRI developers, Daniel Vetter, Alex Deucher, Harry Wentland, Christian König 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-06-08 20:16 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-08 14:16 [PATCH] drm/amd/display: Fix indenting in dcn30_set_output_transfer_func() Dan Carpenter 2020-06-08 17:16 ` Joe Perches 2020-06-08 17:49 ` Dan Carpenter 2020-06-08 19:10 ` Joe Perches 2020-06-08 20:16 ` Alex Deucher
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).