Re: [PATCH] drm/i915: Remove unreachable code
On Sat, Jan 30, 2021 at 9:45 AM Chris Wilson wrote:
>
> Quoting Vinicius Tinti (2021-01-30 12:34:11)
> > On Fri, Jan 29, 2021 at 08:55:54PM +, Chris Wilson wrote:
> > > Quoting Vinicius Tinti (2021-01-29 18:15:19)
> > > > By enabling -Wunreachable-code-aggressive on Clang the following code
> > > > paths are unreachable.
> > >
> > > That code exists as commentary and, especially for sdvo, library
> > > functions that we may need in future.
> >
> > I would argue that this code could be removed since it is in git history.
> > It can be restored when needed.
> >
> > This will make the code cleaner.
>
> It doesn't change the control flow, so no complexity argument. It
> removes documentation from the code, so I have the opposite opinion.
The last change in sdvo related to this function is from commit
ce22c320b8ca ("drm/i915/sdvo: convert to encoder disable/enable"), which
dates from 2012.
It has not been used or changed for a long time. I think it could be
converted to a block comment. This will preserve the documentation
purpose. What do you think?
All this will avoid having "if (0)".
> > > The ivb-gt1 case => as we now set the gt level for ivb, should we not
> > > enable the optimisation for ivb unaffected by the w/a? Just no one has
> > > taken the time to see if it causes a regression.
> >
> > I don't know. I just found out that the code is unreachable.
> >
> > > For error state, the question remains whether we should revert to
> > > uncompressed data if the compressed stream is larger than the original.
> >
> > I don't know too.
> >
> > In this last two cases the code could be commented and the decisions
> > and problems explained in the comment section.
>
> They already are, that is the point.
I meant making them a block comment. For example:
/*
* Enabling HiZ Raw Stall Optimization, at this point, causes corruption.
*
* Calling wa_masked_dis with the arguments wal, CACHE_MODE_0_GEN7,
* HIZ_RAW_STALL_OPT_DISABLE will cause an HiZ corruption on ivb:g1.
*/
/*
* Should fallback to uncompressed if we increase size
* (zstream->total_out > zstream->total_in) by returning -E2BIG?
*/
> -Chris
Re: [PATCH] drm/i915: Remove unreachable code
Quoting Vinicius Tinti (2021-01-30 12:34:11) > On Fri, Jan 29, 2021 at 08:55:54PM +, Chris Wilson wrote: > > Quoting Vinicius Tinti (2021-01-29 18:15:19) > > > By enabling -Wunreachable-code-aggressive on Clang the following code > > > paths are unreachable. > > > > That code exists as commentary and, especially for sdvo, library > > functions that we may need in future. > > I would argue that this code could be removed since it is in git history. > It can be restored when needed. > > This will make the code cleaner. It doesn't change the control flow, so no complexity argument. It removes documentation from the code, so I have the opposite opinion. > > The ivb-gt1 case => as we now set the gt level for ivb, should we not > > enable the optimisation for ivb unaffected by the w/a? Just no one has > > taken the time to see if it causes a regression. > > I don't know. I just found out that the code is unreachable. > > > For error state, the question remains whether we should revert to > > uncompressed data if the compressed stream is larger than the original. > > I don't know too. > > In this last two cases the code could be commented and the decisions > and problems explained in the comment section. They already are, that is the point. -Chris
Re: [PATCH] drm/i915: Remove unreachable code
On Fri, Jan 29, 2021 at 08:55:54PM +, Chris Wilson wrote: > Quoting Vinicius Tinti (2021-01-29 18:15:19) > > By enabling -Wunreachable-code-aggressive on Clang the following code > > paths are unreachable. > > That code exists as commentary and, especially for sdvo, library > functions that we may need in future. I would argue that this code could be removed since it is in git history. It can be restored when needed. This will make the code cleaner. > The ivb-gt1 case => as we now set the gt level for ivb, should we not > enable the optimisation for ivb unaffected by the w/a? Just no one has > taken the time to see if it causes a regression. I don't know. I just found out that the code is unreachable. > For error state, the question remains whether we should revert to > uncompressed data if the compressed stream is larger than the original. I don't know too. In this last two cases the code could be commented and the decisions and problems explained in the comment section. > -Chris
Re: [PATCH] drm/i915: Remove unreachable code
Quoting Vinicius Tinti (2021-01-29 18:15:19) > By enabling -Wunreachable-code-aggressive on Clang the following code > paths are unreachable. That code exists as commentary and, especially for sdvo, library functions that we may need in future. The ivb-gt1 case => as we now set the gt level for ivb, should we not enable the optimisation for ivb unaffected by the w/a? Just no one has taken the time to see if it causes a regression. For error state, the question remains whether we should revert to uncompressed data if the compressed stream is larger than the original. -Chris
[PATCH] drm/i915: Remove unreachable code
By enabling -Wunreachable-code-aggressive on Clang the following code
paths are unreachable.
Commit ce22c320b8ca ("drm/i915/sdvo: convert to encoder disable/enable")
Commit 19f1f627b333 ("drm/i915/gt: Move ivb GT workarounds from
init_clock_gating to workarounds")
Commit 0a97015d45ee ("drm/i915: Compress GPU objects in error state")
By removing the unreachable code at
drivers/gpu/drm/i915/display/intel_sdvo.c the function
intel_sdvo_set_encoder_power_state becomes unused.
Commit ea5b213ad4b1 ("drm/i915: Subclass intel_encoder.")
Clang warns unreachable:
drivers/gpu/drm/i915/display/intel_sdvo.c:1768:3: warning: code will never
be executed [-Wunreachable-code]
intel_sdvo_set_encoder_power_state(intel_sdvo,
^~
drivers/gpu/drm/i915/display/intel_sdvo.c:1767:6: note: silence by adding
parentheses to mark code as explicitly dead
if (0)
^
/* DISABLES CODE */ ( )
drivers/gpu/drm/i915/display/intel_sdvo.c:1852:3: warning: code will never
be executed [-Wunreachable-code]
intel_sdvo_set_encoder_power_state(intel_sdvo,
^~
drivers/gpu/drm/i915/display/intel_sdvo.c:1851:6: note: silence by adding
parentheses to mark code as explicitly dead
if (0)
^
/* DISABLES CODE */ ( )
drivers/gpu/drm/i915/gt/intel_workarounds.c:884:3: warning: code will never
be executed [-Wunreachable-code]
wa_masked_dis(wal, CACHE_MODE_0_GEN7,
HIZ_RAW_STALL_OPT_DISABLE);
^
drivers/gpu/drm/i915/gt/intel_workarounds.c:882:6: note: silence by adding
parentheses to mark code as explicitly dead
if (0) { /* causes HiZ corruption on ivb:gt1 */
^
/* DISABLES CODE */ ( )
drivers/gpu/drm/i915/i915_gpu_error.c:319:11: warning: code will never be
executed [-Wunreachable-code]
if (0 && zstream->total_out > zstream->total_in)
^~~
drivers/gpu/drm/i915/i915_gpu_error.c:319:6: note: silence by adding
parentheses to mark code as explicitly dead
if (0 && zstream->total_out > zstream->total_in)
^
/* DISABLES CODE */ ( )
Clang warns unused after removing unreachable:
drivers/gpu/drm/i915/display/intel_sdvo.c:696:13: warning: unused function
'intel_sdvo_set_encoder_power_state' [-Wunused-function]
static bool intel_sdvo_set_encoder_power_state(struct intel_sdvo *intel_sdvo,
^
Signed-off-by: Vinicius Tinti
---
drivers/gpu/drm/i915/display/intel_sdvo.c | 30 -
drivers/gpu/drm/i915/gt/intel_workarounds.c | 5
drivers/gpu/drm/i915/i915_gpu_error.c | 4 ---
3 files changed, 39 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
b/drivers/gpu/drm/i915/display/intel_sdvo.c
index 4eaa4aa86ecd..45d03b09f8f0 100644
--- a/drivers/gpu/drm/i915/display/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
@@ -693,30 +693,6 @@ static bool intel_sdvo_get_active_outputs(struct
intel_sdvo *intel_sdvo,
outputs, sizeof(*outputs));
}
-static bool intel_sdvo_set_encoder_power_state(struct intel_sdvo *intel_sdvo,
- int mode)
-{
- u8 state = SDVO_ENCODER_STATE_ON;
-
- switch (mode) {
- case DRM_MODE_DPMS_ON:
- state = SDVO_ENCODER_STATE_ON;
- break;
- case DRM_MODE_DPMS_STANDBY:
- state = SDVO_ENCODER_STATE_STANDBY;
- break;
- case DRM_MODE_DPMS_SUSPEND:
- state = SDVO_ENCODER_STATE_SUSPEND;
- break;
- case DRM_MODE_DPMS_OFF:
- state = SDVO_ENCODER_STATE_OFF;
- break;
- }
-
- return intel_sdvo_set_value(intel_sdvo,
- SDVO_CMD_SET_ENCODER_POWER_STATE, &state,
sizeof(state));
-}
-
static bool intel_sdvo_get_input_pixel_clock_range(struct intel_sdvo
*intel_sdvo,
int *clock_min,
int *clock_max)
@@ -1764,9 +1740,6 @@ static void intel_disable_sdvo(struct intel_atomic_state
*state,
intel_sdvo_disable_audio(intel_sdvo);
intel_sdvo_set_active_outputs(intel_sdvo, 0);
- if (0)
- intel_sdvo_set_encoder_power_state(intel_sdvo,
- DRM_MODE_DPMS_OFF);
temp = intel_de_read(dev_priv, intel_sdvo->sdvo_reg);
@@ -1848,9 +1821,6 @@ static void intel_enable_sdvo(struct intel_atomic_state
*state,
"sync\n", SDVO_NAME(intel_sdvo));
}
- if (0)
- intel_sdvo_set_encoder_power_state(intel_sdvo,
- DRM_MODE_DPMS_ON);
intel_sdvo_set_active_outputs(intel_sdvo, intel_sdvo->attached_output);
if (pipe_config->has_
