| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 08 Jun 2020 12:10:57 -0700
From: Joe Perches <joe@...ches.com>
To: Dan Carpenter <dan.carpenter@...cle.com>
Cc: Harry Wentland <harry.wentland@....com>,
Leo Li <sunpeng.li@....com>,
Alex Deucher <alexander.deucher@....com>,
Christian König <christian.koenig@....com>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>,
Bhawanpreet Lakha <Bhawanpreet.Lakha@....com>,
amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] drm/amd/display: Fix indenting in
dcn30_set_output_transfer_func()
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@...cle.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;
}
Powered by blists - more mailing lists