Re: [PATCH 2/2] drm/amd: move variable to local scope
With Christian's comments addressed both patches are
Reviewed-by: Harry Wentland
Harry
On 2021-12-10 10:54, Mario Limonciello wrote:
> `edp_stream` is only used when backend is enabled on eDP, don't
> declare the variable outside that scope.
>
> Signed-off-by: Mario Limonciello
> ---
> drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> index 0d6dc329dddb..fb578b311b98 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> @@ -1798,7 +1798,6 @@ void dce110_enable_accelerated_mode(struct dc *dc,
> struct dc_state *context)
> struct dc_stream_state *edp_streams[MAX_NUM_EDP];
> struct dc_link *edp_link_with_sink = NULL;
> struct dc_link *edp_link = NULL;
> - struct dc_stream_state *edp_stream = NULL;
> struct dce_hwseq *hws = dc->hwseq;
> int edp_with_sink_num;
> int edp_num;
> @@ -1830,7 +1829,7 @@ void dce110_enable_accelerated_mode(struct dc *dc,
> struct dc_state *context)
> if (edp_link->link_enc->funcs->is_dig_enabled &&
>
> edp_link->link_enc->funcs->is_dig_enabled(edp_link->link_enc) &&
> edp_link->link_status.link_active) {
> - edp_stream = edp_streams[0];
> + struct dc_stream_state *edp_stream =
> edp_streams[0];
> can_apply_edp_fast_boot =
> !is_edp_ilr_optimization_required(edp_stream->link, _stream->timing);
> edp_stream->apply_edp_fast_boot_optimization =
> can_apply_edp_fast_boot;
> if (can_apply_edp_fast_boot)
>
RE: [PATCH 2/2] drm/amd: move variable to local scope
[AMD Official Use Only]
> -Original Message-
> From: Koenig, Christian
> Sent: Friday, December 10, 2021 10:19
> To: Limonciello, Mario ; Koenig, Christian
> ; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 2/2] drm/amd: move variable to local scope
>
> Am 10.12.21 um 17:12 schrieb Limonciello, Mario:
> > [AMD Official Use Only]
> >
> >> -Original Message-
> >> From: Koenig, Christian
> >> Sent: Friday, December 10, 2021 10:07
> >> To: Limonciello, Mario ; amd-
> >> g...@lists.freedesktop.org
> >> Subject: Re: [PATCH 2/2] drm/amd: move variable to local scope
> >>
> >>
> >>
> >> Am 10.12.21 um 16:54 schrieb Mario Limonciello:
> >>> `edp_stream` is only used when backend is enabled on eDP, don't
> >>> declare the variable outside that scope.
> >>>
> >>> Signed-off-by: Mario Limonciello
> >>> ---
> >>>drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 3 +--
> >>>1 file changed, 1 insertion(+), 2 deletions(-)
> >>>
> >>> diff --git
> a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> >> b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> >>> index 0d6dc329dddb..fb578b311b98 100644
> >>> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> >>> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> >>> @@ -1798,7 +1798,6 @@ void dce110_enable_accelerated_mode(struct dc
> >> *dc, struct dc_state *context)
> >>> struct dc_stream_state *edp_streams[MAX_NUM_EDP];
> >>> struct dc_link *edp_link_with_sink = NULL;
> >>> struct dc_link *edp_link = NULL;
> >>> - struct dc_stream_state *edp_stream = NULL;
> >>> struct dce_hwseq *hws = dc->hwseq;
> >>> int edp_with_sink_num;
> >>> int edp_num;
> >>> @@ -1830,7 +1829,7 @@ void dce110_enable_accelerated_mode(struct dc
> >> *dc, struct dc_state *context)
> >>> if (edp_link->link_enc->funcs->is_dig_enabled &&
> >>>
> >>> edp_link->link_enc->funcs->is_dig_enabled(edp_link-
> >>> link_enc) &&
> >>> edp_link->link_status.link_active) {
> >>> - edp_stream = edp_streams[0];
> >>> + struct dc_stream_state *edp_stream =
> >> edp_streams[0];
> >>> can_apply_edp_fast_boot =
> >> !is_edp_ilr_optimization_required(edp_stream->link, _stream->timing);
> >>
> >> While you are at it, there should always be an empty line between
> >> declaration and code.
> > Very well, will fix it. Can I have a tag with assumption that fix in
> > place, or
> > should I re-send?
>
> Just fix inline, it's not a major issue anyway.
OK once I have a tag for the series will do so.
>
> >
> >> Running your patches through checkpatch.pl helps spotting such stuff.
> >>
> > Actually checkpatch didn't catch that.
> >
> > $ ./scripts/checkpatch.pl 0002-drm-amd-move-variable-to-local-scope.patch
> > total: 0 errors, 0 warnings, 15 lines checked
> >
> > 0002-drm-amd-move-variable-to-local-scope.patch has no obvious style
> problems and is ready for submission.
>
> Mhm, something is going wrong here. checkpatch.pl is complaining quite
> badly for that patch:
>
> ERROR: DOS line endings
> #169: FILE:
> drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c:1832:
> +^I^I^I^Istruct dc_stream_state *edp_stream = edp_streams[0];^M$
>
> Especially the DOS line ending is not something we can push like this,
> but could be that the AMD mail servers are messing up things once more.
That's gotta be Exchange or outlook doing it or something.
$ wget https://patchwork.freedesktop.org/patch/466268/mbox/ -O patch
--2021-12-10 10:21:23-- https://patchwork.freedesktop.org/patch/466268/mbox/
Resolving patchwork.freedesktop.org (patchwork.freedesktop.org)...
131.252.210.167, 2610:10:20:722:a800:ff:feee:56cf
Connecting to patchwork.freedesktop.org
(patchwork.freedesktop.org)|131.252.210.167|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: 1975 (1.9K) [text/plain]
Saving to: ‘patch’
patch
100%[>]
1.93K --.-KB/sin 0s
2021-12-10 10:21:26 (60.2 MB/s) - ‘patch’ saved [1975/1975]
$ ~/src/linux/scripts/checkpatch.pl patch
total: 0 errors, 0 warnings, 15 lines checked
patch has no obvious style problems and is ready for submission.
>
> Christian.
>
> >
> >> Christian.
> >>
> >>> edp_stream-
> >>> apply_edp_fast_boot_optimization = can_apply_edp_fast_boot;
> >>> if (can_apply_edp_fast_boot)
Re: [PATCH 2/2] drm/amd: move variable to local scope
Am 10.12.21 um 17:12 schrieb Limonciello, Mario:
[AMD Official Use Only]
-Original Message-
From: Koenig, Christian
Sent: Friday, December 10, 2021 10:07
To: Limonciello, Mario ; amd-
g...@lists.freedesktop.org
Subject: Re: [PATCH 2/2] drm/amd: move variable to local scope
Am 10.12.21 um 16:54 schrieb Mario Limonciello:
`edp_stream` is only used when backend is enabled on eDP, don't
declare the variable outside that scope.
Signed-off-by: Mario Limonciello
---
drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
index 0d6dc329dddb..fb578b311b98 100644
--- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
+++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
@@ -1798,7 +1798,6 @@ void dce110_enable_accelerated_mode(struct dc
*dc, struct dc_state *context)
struct dc_stream_state *edp_streams[MAX_NUM_EDP];
struct dc_link *edp_link_with_sink = NULL;
struct dc_link *edp_link = NULL;
- struct dc_stream_state *edp_stream = NULL;
struct dce_hwseq *hws = dc->hwseq;
int edp_with_sink_num;
int edp_num;
@@ -1830,7 +1829,7 @@ void dce110_enable_accelerated_mode(struct dc
*dc, struct dc_state *context)
if (edp_link->link_enc->funcs->is_dig_enabled &&
edp_link->link_enc->funcs->is_dig_enabled(edp_link-
link_enc) &&
edp_link->link_status.link_active) {
- edp_stream = edp_streams[0];
+ struct dc_stream_state *edp_stream =
edp_streams[0];
can_apply_edp_fast_boot =
!is_edp_ilr_optimization_required(edp_stream->link, _stream->timing);
While you are at it, there should always be an empty line between
declaration and code.
Very well, will fix it. Can I have a tag with assumption that fix in place, or
should I re-send?
Just fix inline, it's not a major issue anyway.
Running your patches through checkpatch.pl helps spotting such stuff.
Actually checkpatch didn't catch that.
$ ./scripts/checkpatch.pl 0002-drm-amd-move-variable-to-local-scope.patch
total: 0 errors, 0 warnings, 15 lines checked
0002-drm-amd-move-variable-to-local-scope.patch has no obvious style problems
and is ready for submission.
Mhm, something is going wrong here. checkpatch.pl is complaining quite
badly for that patch:
ERROR: DOS line endings
#169: FILE:
drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c:1832:
+^I^I^I^Istruct dc_stream_state *edp_stream = edp_streams[0];^M$
Especially the DOS line ending is not something we can push like this,
but could be that the AMD mail servers are messing up things once more.
Christian.
Christian.
edp_stream-
apply_edp_fast_boot_optimization = can_apply_edp_fast_boot;
if (can_apply_edp_fast_boot)
RE: [PATCH 2/2] drm/amd: move variable to local scope
[AMD Official Use Only]
> -Original Message-
> From: Koenig, Christian
> Sent: Friday, December 10, 2021 10:07
> To: Limonciello, Mario ; amd-
> g...@lists.freedesktop.org
> Subject: Re: [PATCH 2/2] drm/amd: move variable to local scope
>
>
>
> Am 10.12.21 um 16:54 schrieb Mario Limonciello:
> > `edp_stream` is only used when backend is enabled on eDP, don't
> > declare the variable outside that scope.
> >
> > Signed-off-by: Mario Limonciello
> > ---
> > drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> > index 0d6dc329dddb..fb578b311b98 100644
> > --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> > +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> > @@ -1798,7 +1798,6 @@ void dce110_enable_accelerated_mode(struct dc
> *dc, struct dc_state *context)
> > struct dc_stream_state *edp_streams[MAX_NUM_EDP];
> > struct dc_link *edp_link_with_sink = NULL;
> > struct dc_link *edp_link = NULL;
> > - struct dc_stream_state *edp_stream = NULL;
> > struct dce_hwseq *hws = dc->hwseq;
> > int edp_with_sink_num;
> > int edp_num;
> > @@ -1830,7 +1829,7 @@ void dce110_enable_accelerated_mode(struct dc
> *dc, struct dc_state *context)
> > if (edp_link->link_enc->funcs->is_dig_enabled &&
> > edp_link->link_enc->funcs->is_dig_enabled(edp_link-
> >link_enc) &&
> > edp_link->link_status.link_active) {
> > - edp_stream = edp_streams[0];
> > + struct dc_stream_state *edp_stream =
> edp_streams[0];
> > can_apply_edp_fast_boot =
> !is_edp_ilr_optimization_required(edp_stream->link, _stream->timing);
>
> While you are at it, there should always be an empty line between
> declaration and code.
Very well, will fix it. Can I have a tag with assumption that fix in place, or
should I re-send?
>
> Running your patches through checkpatch.pl helps spotting such stuff.
>
Actually checkpatch didn't catch that.
$ ./scripts/checkpatch.pl 0002-drm-amd-move-variable-to-local-scope.patch
total: 0 errors, 0 warnings, 15 lines checked
0002-drm-amd-move-variable-to-local-scope.patch has no obvious style problems
and is ready for submission.
> Christian.
>
> > edp_stream-
> >apply_edp_fast_boot_optimization = can_apply_edp_fast_boot;
> > if (can_apply_edp_fast_boot)
Re: [PATCH 2/2] drm/amd: move variable to local scope
Am 10.12.21 um 16:54 schrieb Mario Limonciello:
`edp_stream` is only used when backend is enabled on eDP, don't
declare the variable outside that scope.
Signed-off-by: Mario Limonciello
---
drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
index 0d6dc329dddb..fb578b311b98 100644
--- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
+++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
@@ -1798,7 +1798,6 @@ void dce110_enable_accelerated_mode(struct dc *dc, struct
dc_state *context)
struct dc_stream_state *edp_streams[MAX_NUM_EDP];
struct dc_link *edp_link_with_sink = NULL;
struct dc_link *edp_link = NULL;
- struct dc_stream_state *edp_stream = NULL;
struct dce_hwseq *hws = dc->hwseq;
int edp_with_sink_num;
int edp_num;
@@ -1830,7 +1829,7 @@ void dce110_enable_accelerated_mode(struct dc *dc, struct
dc_state *context)
if (edp_link->link_enc->funcs->is_dig_enabled &&
edp_link->link_enc->funcs->is_dig_enabled(edp_link->link_enc) &&
edp_link->link_status.link_active) {
- edp_stream = edp_streams[0];
+ struct dc_stream_state *edp_stream =
edp_streams[0];
can_apply_edp_fast_boot =
!is_edp_ilr_optimization_required(edp_stream->link, _stream->timing);
While you are at it, there should always be an empty line between
declaration and code.
Running your patches through checkpatch.pl helps spotting such stuff.
Christian.
edp_stream->apply_edp_fast_boot_optimization =
can_apply_edp_fast_boot;
if (can_apply_edp_fast_boot)
