[Intel-gfx] [PATCH v3] drm/i915: Disable some extra clang warnings

[Matthias Kaehlcke]

Commit 39bf4de89ff7 ("drm/i915: Add -Wall -Wextra to our build, set
warnings to full") enabled extra warnings for i915 to spot possible
bugs in new code, and then disabled a subset of these warnings to keep
the current code building without warnings (with gcc). Enabling the
extra warnings also enabled some additional clang-only warnings, as a
result building i915 with clang currently is extremely noisy. For now
also disable the clang warnings sign-compare, sometimes-uninitialized,
unneeded-internal-declaration and initializer-overrides. If desired
they can be re-enabled after the code has been fixed.

Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
---
Changes in v3:
- don't disable -Wunneeded-internal-declaration , the only occurrence
  can be fixed in the code
- removed 'Fixes' tag, since backporting is not necessary

 drivers/gpu/drm/i915/Makefile | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 9bee52a949a9..dfe01452c8d1 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -18,6 +18,10 @@ subdir-ccflags-y += $(call cc-disable-warning, type-limits)
 subdir-ccflags-y += $(call cc-disable-warning, missing-field-initializers)
 subdir-ccflags-y += $(call cc-disable-warning, implicit-fallthrough)
 subdir-ccflags-y += $(call cc-disable-warning, unused-but-set-variable)
+# clang warnings
+subdir-ccflags-y += $(call cc-disable-warning, sign-compare)
+subdir-ccflags-y += $(call cc-disable-warning, sometimes-uninitialized)
+subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides)
 subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
 
 # Fine grained warnings disable
-- 
2.17.0.441.gb46fe60e1d-goog

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Disable some extra clang warnings

[Matthias Kaehlcke]

On Mon, Apr 30, 2018 at 10:01:50PM +0100, Chris Wilson wrote:
> Quoting Matthias Kaehlcke (2018-04-30 21:51:45)
> > On Mon, Apr 30, 2018 at 09:01:49PM +0100, Chris Wilson wrote:
> > > Quoting Matthias Kaehlcke (2018-04-30 20:31:19)
> > > > On Mon, Mar 19, 2018 at 12:14:51PM -0700, Matthias Kaehlcke wrote:
> > > > > Commit 39bf4de89ff7 ("drm/i915: Add -Wall -Wextra to our build, set
> > > > > warnings to full") enabled extra warnings for i915 to spot possible
> > > > > bugs in new code, and then disabled a subset of these warnings to keep
> > > > > the current code building without warnings (with gcc). Enabling the
> > > > > extra warnings also enabled some additional clang-only warnings, as a
> > > > > result building i915 with clang currently is extremely noisy. For now
> > > > > also disable the clang warnings sign-compare, sometimes-uninitialized,
> > > > > unneeded-internal-declaration and initializer-overrides. If desired
> > > > > they can be re-enabled after the code has been fixed.
> > > > > 
> > > > > Fixes: 39bf4de89ff7 ("drm/i915: Add -Wall -Wextra to our build, set
> > > > > warnings to full")
> > > 
> > > Do we need to backport this for a non-default build with a non-default
> > > compiler?
> > 
> > If it affected a LTS build I'd say yes, but since that isn't the case
> > I think it's not necessary.
> > 
> > > > > Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
> > > > > ---
> > > > > Changes in v2:
> > > > > - rebased on drm-tip
> > > > > - added comment indicating that disabled warnings are clang warnings
> > > > > 
> > > > >  drivers/gpu/drm/i915/Makefile | 5 +
> > > > >  1 file changed, 5 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/Makefile 
> > > > > b/drivers/gpu/drm/i915/Makefile
> > > > > index 4eee91a3a236..9717c037b582 100644
> > > > > --- a/drivers/gpu/drm/i915/Makefile
> > > > > +++ b/drivers/gpu/drm/i915/Makefile
> > > > > @@ -18,6 +18,11 @@ subdir-ccflags-y += $(call cc-disable-warning, 
> > > > > type-limits)
> > > > >  subdir-ccflags-y += $(call cc-disable-warning, 
> > > > > missing-field-initializers)
> > > > >  subdir-ccflags-y += $(call cc-disable-warning, implicit-fallthrough)
> > > > >  subdir-ccflags-y += $(call cc-disable-warning, 
> > > > > unused-but-set-variable)
> > > > > +# clang warnings
> > > > > +subdir-ccflags-y += $(call cc-disable-warning, sign-compare)
> > > 
> > > Too much mixup in the code to be fixed overnight indeed.
> > > 
> > > > > +subdir-ccflags-y += $(call cc-disable-warning, 
> > > > > sometimes-uninitialized)
> > > 
> > > Annoyingly it appears that clang has more false positives.
> > > 
> > > > > +subdir-ccflags-y += $(call cc-disable-warning, 
> > > > > unneeded-internal-declaration)
> > > 
> > > Example? I don't recall this one, so don't know if we should just not
> > > fix it rather than suppress. I've used ignored-attributes, perhaps that
> > > was for the same cause.
> > 
> > drivers/gpu/drm/i915/intel_guc_submission.c:183:13: warning: function
> >   'has_doorbell' is not needed and will not be emitted
> >   [-Wunneeded-internal-declaration]
> > static bool has_doorbell(struct intel_guc_client *client)
> > 
> > The function is only called within a GEM_BUG_ON macro, which does not
> > evaluate the expression unless CONFIG_DRM_I915_DEBUG_GEM is set.
> > 
> > Instead of disabling the warning it would probably be better to mark
> > has_doorbell as __maybe_unused.
> 
> Hmm, if it is just this one, I would remove the use from
> intel_guc_submission and move it into selftests/
> The single use case inside intel_guc_submission isn't that interesting
> and I doubt we would miss not having the assert.

Could you take care of this? I'm not really familiar with the i915
codebase and might not put it exactly where you want it ;-)

I'd then rebase this patch and leave -Wunneeded-internal-declaration enabled.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Disable some extra clang warnings

[Matthias Kaehlcke]

On Mon, Apr 30, 2018 at 09:01:49PM +0100, Chris Wilson wrote:
> Quoting Matthias Kaehlcke (2018-04-30 20:31:19)
> > On Mon, Mar 19, 2018 at 12:14:51PM -0700, Matthias Kaehlcke wrote:
> > > Commit 39bf4de89ff7 ("drm/i915: Add -Wall -Wextra to our build, set
> > > warnings to full") enabled extra warnings for i915 to spot possible
> > > bugs in new code, and then disabled a subset of these warnings to keep
> > > the current code building without warnings (with gcc). Enabling the
> > > extra warnings also enabled some additional clang-only warnings, as a
> > > result building i915 with clang currently is extremely noisy. For now
> > > also disable the clang warnings sign-compare, sometimes-uninitialized,
> > > unneeded-internal-declaration and initializer-overrides. If desired
> > > they can be re-enabled after the code has been fixed.
> > > 
> > > Fixes: 39bf4de89ff7 ("drm/i915: Add -Wall -Wextra to our build, set
> > > warnings to full")
> 
> Do we need to backport this for a non-default build with a non-default
> compiler?

If it affected a LTS build I'd say yes, but since that isn't the case
I think it's not necessary.

> > > Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
> > > ---
> > > Changes in v2:
> > > - rebased on drm-tip
> > > - added comment indicating that disabled warnings are clang warnings
> > > 
> > >  drivers/gpu/drm/i915/Makefile | 5 +
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > > index 4eee91a3a236..9717c037b582 100644
> > > --- a/drivers/gpu/drm/i915/Makefile
> > > +++ b/drivers/gpu/drm/i915/Makefile
> > > @@ -18,6 +18,11 @@ subdir-ccflags-y += $(call cc-disable-warning, 
> > > type-limits)
> > >  subdir-ccflags-y += $(call cc-disable-warning, 
> > > missing-field-initializers)
> > >  subdir-ccflags-y += $(call cc-disable-warning, implicit-fallthrough)
> > >  subdir-ccflags-y += $(call cc-disable-warning, unused-but-set-variable)
> > > +# clang warnings
> > > +subdir-ccflags-y += $(call cc-disable-warning, sign-compare)
> 
> Too much mixup in the code to be fixed overnight indeed.
> 
> > > +subdir-ccflags-y += $(call cc-disable-warning, sometimes-uninitialized)
> 
> Annoyingly it appears that clang has more false positives.
> 
> > > +subdir-ccflags-y += $(call cc-disable-warning, 
> > > unneeded-internal-declaration)
> 
> Example? I don't recall this one, so don't know if we should just not
> fix it rather than suppress. I've used ignored-attributes, perhaps that
> was for the same cause.

drivers/gpu/drm/i915/intel_guc_submission.c:183:13: warning: function
  'has_doorbell' is not needed and will not be emitted
  [-Wunneeded-internal-declaration]
static bool has_doorbell(struct intel_guc_client *client)

The function is only called within a GEM_BUG_ON macro, which does not
evaluate the expression unless CONFIG_DRM_I915_DEBUG_GEM is set.

Instead of disabling the warning it would probably be better to mark
has_doorbell as __maybe_unused.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Disable some extra clang warnings

[Matthias Kaehlcke]

On Mon, Mar 19, 2018 at 12:14:51PM -0700, Matthias Kaehlcke wrote:
> Commit 39bf4de89ff7 ("drm/i915: Add -Wall -Wextra to our build, set
> warnings to full") enabled extra warnings for i915 to spot possible
> bugs in new code, and then disabled a subset of these warnings to keep
> the current code building without warnings (with gcc). Enabling the
> extra warnings also enabled some additional clang-only warnings, as a
> result building i915 with clang currently is extremely noisy. For now
> also disable the clang warnings sign-compare, sometimes-uninitialized,
> unneeded-internal-declaration and initializer-overrides. If desired
> they can be re-enabled after the code has been fixed.
> 
> Fixes: 39bf4de89ff7 ("drm/i915: Add -Wall -Wextra to our build, set
> warnings to full")
> Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
> ---
> Changes in v2:
> - rebased on drm-tip
> - added comment indicating that disabled warnings are clang warnings
> 
>  drivers/gpu/drm/i915/Makefile | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 4eee91a3a236..9717c037b582 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -18,6 +18,11 @@ subdir-ccflags-y += $(call cc-disable-warning, type-limits)
>  subdir-ccflags-y += $(call cc-disable-warning, missing-field-initializers)
>  subdir-ccflags-y += $(call cc-disable-warning, implicit-fallthrough)
>  subdir-ccflags-y += $(call cc-disable-warning, unused-but-set-variable)
> +# clang warnings
> +subdir-ccflags-y += $(call cc-disable-warning, sign-compare)
> +subdir-ccflags-y += $(call cc-disable-warning, sometimes-uninitialized)
> +subdir-ccflags-y += $(call cc-disable-warning, unneeded-internal-declaration)
> +subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides)
>  subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
>  
>  # Fine grained warnings disable

Ping, it seems this one swept under the radar

Thanks

Matthias
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] drm/i915: Disable some extra clang warnings

[Matthias Kaehlcke]

Commit 39bf4de89ff7 ("drm/i915: Add -Wall -Wextra to our build, set
warnings to full") enabled extra warnings for i915 to spot possible
bugs in new code, and then disabled a subset of these warnings to keep
the current code building without warnings (with gcc). Enabling the
extra warnings also enabled some additional clang-only warnings, as a
result building i915 with clang currently is extremely noisy. For now
also disable the clang warnings sign-compare, sometimes-uninitialized,
unneeded-internal-declaration and initializer-overrides. If desired
they can be re-enabled after the code has been fixed.

Fixes: 39bf4de89ff7 ("drm/i915: Add -Wall -Wextra to our build, set
warnings to full")
Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
---
Changes in v2:
- rebased on drm-tip
- added comment indicating that disabled warnings are clang warnings

 drivers/gpu/drm/i915/Makefile | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 4eee91a3a236..9717c037b582 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -18,6 +18,11 @@ subdir-ccflags-y += $(call cc-disable-warning, type-limits)
 subdir-ccflags-y += $(call cc-disable-warning, missing-field-initializers)
 subdir-ccflags-y += $(call cc-disable-warning, implicit-fallthrough)
 subdir-ccflags-y += $(call cc-disable-warning, unused-but-set-variable)
+# clang warnings
+subdir-ccflags-y += $(call cc-disable-warning, sign-compare)
+subdir-ccflags-y += $(call cc-disable-warning, sometimes-uninitialized)
+subdir-ccflags-y += $(call cc-disable-warning, unneeded-internal-declaration)
+subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides)
 subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
 
 # Fine grained warnings disable
-- 
2.16.2.804.g6dcf76e118-goog

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Disable some extra clang warnings

[Matthias Kaehlcke]

Commit 39bf4de89ff7 ("drm/i915: Add -Wall -Wextra to our build, set
warnings to full") enabled extra warnings for i915 to spot possible
bugs in new code, and then disabled a subset of these warnings to keep
the current code building without warnings (with gcc). Enabling the
extra warnings also enabled some additional clang-only warnings, as a
result building i915 with clang currently is extremely noisy. For now
also disable the clang warnings sign-compare, sometimes-uninitialized,
unneeded-internal-declaration and initializer-overrides. If desired
they can be re-enabled after the code has been fixed.

Fixes: 39bf4de89ff7 ("drm/i915: Add -Wall -Wextra to our build, set
warnings to full")
Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
---
 drivers/gpu/drm/i915/Makefile | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 091aef281963..ad05796a96ba 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -17,6 +17,10 @@ subdir-ccflags-y += $(call cc-disable-warning, 
unused-parameter)
 subdir-ccflags-y += $(call cc-disable-warning, type-limits)
 subdir-ccflags-y += $(call cc-disable-warning, missing-field-initializers)
 subdir-ccflags-y += $(call cc-disable-warning, implicit-fallthrough)
+subdir-ccflags-y += $(call cc-disable-warning, sign-compare)
+subdir-ccflags-y += $(call cc-disable-warning, sometimes-uninitialized)
+subdir-ccflags-y += $(call cc-disable-warning, unneeded-internal-declaration)
+subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides)
 subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
 
 # Fine grained warnings disable
-- 
2.16.2.804.g6dcf76e118-goog

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Mark wait_for_engine() as maybe_unused

[Matthias Kaehlcke]

The only call of wait_for_engine() is wrapped in a GEM_WARN_ON macro,
which confusingly suppresses the call unless CONFIG_DRM_I915_DEBUG_GEM
is set.

According to http://www.spinics.net/lists/intel-gfx/msg128768.html the
current behavior is correct, even though it's not obvious. Different
solutions to improve GEM_WARN_ON were discussed, but no conclusion
was reached.

Mark wait_for_engine() as maybe_unused to avoid a compiler warning,
according to the above discussion this is still needed evein if
GEM_WARN_ON is eventually refactored.

Reported-by: Nick Desaulniers <nick.desaulni...@gmail.com>
Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
---
 drivers/gpu/drm/i915/i915_gem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 969bac8404f1..52d0b7d0082b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3364,7 +3364,8 @@ static int wait_for_timeline(struct i915_gem_timeline 
*tl, unsigned int flags)
return 0;
 }
 
-static int wait_for_engine(struct intel_engine_cs *engine, int timeout_ms)
+static __maybe_unused int wait_for_engine(
+   struct intel_engine_cs *engine, int timeout_ms)
 {
return wait_for(intel_engine_is_idle(engine), timeout_ms);
 }
-- 
2.14.1.342.g6490525c54-goog

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Pass enum pipe to intel_set_pch_fifo_underrun_reporting()

[Matthias Kaehlcke]

On Thu, Jul 20, 2017 at 1:27 AM, Daniel Vetter <dan...@ffwll.ch> wrote:
> On Wed, Jul 19, 2017 at 10:39:28AM -0700, Matthias Kaehlcke wrote:
>> Commit a21960339c8c ("drm/i915: Consistently use enum pipe for PCH
>> transcoders") misses some pieces, due to a problem with the patch
>> format, this patch adds the remaining bits.
>>
>> Fixes: a21960339c8c ("drm/i915: Consistently use enum pipe for PCH
>> transcoders")
>>
>> Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
>
> Applied, and this time successfully it seems!

Great, thanks!

> Thanks a lot, and sorry for the slight ordeal, I still have no clear idea
> what happened with your v2 patch.

Me neither, supposing that 'git send-email' works correctly I guess
that I somehow deleted the '@' in the editor session opened by 'git
send-email' ...
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Pass enum pipe to intel_set_pch_fifo_underrun_reporting()

[Matthias Kaehlcke]

Commit a21960339c8c ("drm/i915: Consistently use enum pipe for PCH
transcoders") misses some pieces, due to a problem with the patch
format, this patch adds the remaining bits.

Fixes: a21960339c8c ("drm/i915: Consistently use enum pipe for PCH
transcoders")

Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
---
 drivers/gpu/drm/i915/intel_display.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index a89d0fd1c2e1..5c7054c3be0e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5347,8 +5347,7 @@ static void haswell_crtc_enable(struct intel_crtc_state 
*pipe_config,
return;
 
if (intel_crtc->config->has_pch_encoder)
-   intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
- false);
+   intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
 
intel_encoders_pre_pll_enable(crtc, pipe_config, old_state);
 
@@ -5433,8 +5432,7 @@ static void haswell_crtc_enable(struct intel_crtc_state 
*pipe_config,
intel_wait_for_vblank(dev_priv, pipe);
intel_wait_for_vblank(dev_priv, pipe);
intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
-   intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
- true);
+   intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
}
 
/* If we change the relative order between pipe/planes enabling, we need
@@ -5531,8 +5529,7 @@ static void haswell_crtc_disable(struct intel_crtc_state 
*old_crtc_state,
enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
 
if (intel_crtc->config->has_pch_encoder)
-   intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
- false);
+   intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
 
intel_encoders_disable(crtc, old_crtc_state, old_state);
 
@@ -5560,8 +5557,7 @@ static void haswell_crtc_disable(struct intel_crtc_state 
*old_crtc_state,
intel_encoders_post_disable(crtc, old_crtc_state, old_state);
 
if (old_crtc_state->has_pch_encoder)
-   intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
- true);
+   intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
 }
 
 static void i9xx_pfit_enable(struct intel_crtc *crtc)
-- 
2.14.0.rc0.284.gd933b75aa4-goog

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Consistently use enum pipe for PCH transcoders

[Matthias Kaehlcke]

El Wed, Jul 19, 2017 at 08:30:36AM +0200 Daniel Vetter ha dit:

> On Tue, Jul 18, 2017 at 01:48:53PM -0700, Matthias Kaehlcke wrote:
> > Hi Daniel,
> > 
> > El Tue, Jul 18, 2017 at 08:39:50AM +0200 Daniel Vetter ha dit:
> > 
> > > On Mon, Jul 17, 2017 at 11:14:03AM -0700, Matthias Kaehlcke wrote:
> > > > The current code uses in some instances enum transcoder for PCH
> > > > transcoders and enum pipe in others. This is error prone and clang
> > > > raises warnings like this:
> > > > 
> > > > drivers/gpu/drm/i915/intel_dp.c:3546:51: warning: implicit conversion
> > > >   from enumeration type 'enum pipe' to different enumeration type
> > > >   'enum transcoder' [-Wenum-conversion]
> > > > intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> > > > 
> > > > Consistently use the type enum pipe for PCH transcoders.
> > > > 
> > > > Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
> > > 
> > > Somehow git apply-mbox could parse it, but manually applying using patch
> > > worked. Not sure what's going on, maybe double-check it's all right.
> > 
> > Not sure what happened, one of the patch fragments only has one '@'
> > instead of two, with that fixed the patch applies.
> > 
> > Unfortunately the manual application missed some fragments:
> > 
> > drivers/gpu/drm/i915/intel_display.c:5350:51: warning: implicit
> >   conversion from enumeration type 'enum transcoder' to different
> >   enumeration type 'enum pipe' [-Wenum-conversion]
> > intel_set_pch_fifo_underrun_reporting(dev_priv, 
> > TRANSCODER_A,
> > ~   ^~~~
> > drivers/gpu/drm/i915/intel_display.c:5436:51: warning: implicit
> >   conversion from enumeration type 'enum transcoder' to different
> >   enumeration type 'enum pipe' [-Wenum-conversion]
> > intel_set_pch_fifo_underrun_reporting(dev_priv, 
> > TRANSCODER_A,
> > ~   ^~~~
> > drivers/gpu/drm/i915/intel_display.c:5534:51: warning: implicit
> >   conversion from enumeration type 'enum transcoder' to different
> >   enumeration type 'enum pipe' [-Wenum-conversion]
> > intel_set_pch_fifo_underrun_reporting(dev_priv, 
> > TRANSCODER_A,
> > ~   ^~~~
> > drivers/gpu/drm/i915/intel_display.c:5563:51: warning: implicit
> >   conversion from enumeration type 'enum transcoder' to different
> >   enumeration type 'enum pipe' [-Wenum-conversion]
> > intel_set_pch_fifo_underrun_reporting(dev_priv, 
> > TRANSCODER_A,
> > 
> > 
> > What would be the best way forward from here? Revert the manual
> > application and apply again, or a fixup patch?
> 
> Drat I screwed up :-( drm-intel-next-queued is non-rebasing, that means I
> need a fixup patch. I should have checked more carefully that I have all
> the hunks, but patch -p1 seemed happy ...

Ok, I will send a fixup patch shortly

Matthias
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Consistently use enum pipe for PCH transcoders

[Matthias Kaehlcke]

Hi Daniel,

El Tue, Jul 18, 2017 at 08:39:50AM +0200 Daniel Vetter ha dit:

> On Mon, Jul 17, 2017 at 11:14:03AM -0700, Matthias Kaehlcke wrote:
> > The current code uses in some instances enum transcoder for PCH
> > transcoders and enum pipe in others. This is error prone and clang
> > raises warnings like this:
> > 
> > drivers/gpu/drm/i915/intel_dp.c:3546:51: warning: implicit conversion
> >   from enumeration type 'enum pipe' to different enumeration type
> >   'enum transcoder' [-Wenum-conversion]
> > intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> > 
> > Consistently use the type enum pipe for PCH transcoders.
> > 
> > Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
> 
> Somehow git apply-mbox could parse it, but manually applying using patch
> worked. Not sure what's going on, maybe double-check it's all right.

Not sure what happened, one of the patch fragments only has one '@'
instead of two, with that fixed the patch applies.

Unfortunately the manual application missed some fragments:

drivers/gpu/drm/i915/intel_display.c:5350:51: warning: implicit
  conversion from enumeration type 'enum transcoder' to different
  enumeration type 'enum pipe' [-Wenum-conversion]
intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
~   ^~~~
drivers/gpu/drm/i915/intel_display.c:5436:51: warning: implicit
  conversion from enumeration type 'enum transcoder' to different
  enumeration type 'enum pipe' [-Wenum-conversion]
intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
~   ^~~~
drivers/gpu/drm/i915/intel_display.c:5534:51: warning: implicit
  conversion from enumeration type 'enum transcoder' to different
  enumeration type 'enum pipe' [-Wenum-conversion]
intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
~   ^~~~
drivers/gpu/drm/i915/intel_display.c:5563:51: warning: implicit
  conversion from enumeration type 'enum transcoder' to different
  enumeration type 'enum pipe' [-Wenum-conversion]
intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,


What would be the best way forward from here? Revert the manual
application and apply again, or a fixup patch?

Matthias

> > ---
> > Changes in v2:
> > - rebased on drm-intel/drm-intel-next-queued
> > 
> >  drivers/gpu/drm/i915/i915_irq.c| 10 +-
> >  drivers/gpu/drm/i915/intel_display.c   | 24 ++--
> >  drivers/gpu/drm/i915/intel_drv.h   |  6 +++---
> >  drivers/gpu/drm/i915/intel_fifo_underrun.c |  6 +++---
> >  4 files changed, 21 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c 
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index 1d33cea01a1b..0b6f310101ee 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2086,10 +2086,10 @@ static void ibx_irq_handler(struct drm_i915_private 
> > *dev_priv, u32 pch_iir)
> > DRM_DEBUG_DRIVER("PCH transcoder CRC error interrupt\n");
> >  
> > if (pch_iir & SDE_TRANSA_FIFO_UNDER)
> > -   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_A);
> > +   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_A);
> >  
> > if (pch_iir & SDE_TRANSB_FIFO_UNDER)
> > -   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_B);
> > +   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_B);
> >  }
> >  
> >  static void ivb_err_int_handler(struct drm_i915_private *dev_priv)
> > @@ -2123,13 +2123,13 @@ static void cpt_serr_int_handler(struct 
> > drm_i915_private *dev_priv)
> > DRM_ERROR("PCH poison interrupt\n");
> >  
> > if (serr_int & SERR_INT_TRANS_A_FIFO_UNDERRUN)
> > -   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_A);
> > +   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_A);
> >  
> > if (serr_int & SERR_INT_TRANS_B_FIFO_UNDERRUN)
> > -   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_B);
> > +   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_B);
> >  
> > if (serr_int & SERR_INT_TRANS_C_FIFO_UNDERRUN)
> > -   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_C);
> > +   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_C);
> >  
> > I915_WRITE(SERR_INT, serr_int);
> >  }
> > diff --git a/drivers/gpu/

[Intel-gfx] [PATCH v2] drm/i915: Return correct EDP voltage swing table for 0.85V

[Matthias Kaehlcke]

For 0.85V cnl_get_buf_trans_edp() returns the DP table, instead of EDP.
Use the correct table.

The error was pointed out by this clang warning:

drivers/gpu/drm/i915/intel_ddi.c:392:39: warning: variable
  'cnl_ddi_translations_edp_0_85V' is not needed and will not be emitted
  [-Wunneeded-internal-declaration]
static const struct cnl_ddi_buf_trans cnl_ddi_translations_edp_0_85V[] = {

Fixes: cf54ca8bc567 ("drm/i915/cnl: Implement voltage swing sequence.")
Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
Reviewed-by: Manasi Navare <manasi.d.nav...@intel.com>
---
Changes in v2:
- Added 'Fixes' tag
- Added Reviewed-by: Manasi Navare <manasi.d.nav...@intel.com>

 drivers/gpu/drm/i915/intel_ddi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index efb13582dc73..6fa02e6a7a9b 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1873,7 +1873,7 @@ cnl_get_buf_trans_edp(struct drm_i915_private *dev_priv,
if (dev_priv->vbt.edp.low_vswing) {
if (voltage == VOLTAGE_INFO_0_85V) {
*n_entries = ARRAY_SIZE(cnl_ddi_translations_edp_0_85V);
-   return cnl_ddi_translations_dp_0_85V;
+   return cnl_ddi_translations_edp_0_85V;
} else if (voltage == VOLTAGE_INFO_0_95V) {
*n_entries = ARRAY_SIZE(cnl_ddi_translations_edp_0_95V);
return cnl_ddi_translations_edp_0_95V;
-- 
2.13.2.932.g7449e964c-goog

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Return correct EDP voltage swing table for 0.85V

[Matthias Kaehlcke]

For 0.85V cnl_get_buf_trans_edp() returns the DP table, instead of EDP.
Use the correct table.

The error was pointed out by this clang warning:

drivers/gpu/drm/i915/intel_ddi.c:392:39: warning: variable
  'cnl_ddi_translations_edp_0_85V' is not needed and will not be emitted
  [-Wunneeded-internal-declaration]
static const struct cnl_ddi_buf_trans cnl_ddi_translations_edp_0_85V[] = {

Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
---
 drivers/gpu/drm/i915/intel_ddi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index efb13582dc73..6fa02e6a7a9b 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1873,7 +1873,7 @@ cnl_get_buf_trans_edp(struct drm_i915_private *dev_priv,
if (dev_priv->vbt.edp.low_vswing) {
if (voltage == VOLTAGE_INFO_0_85V) {
*n_entries = ARRAY_SIZE(cnl_ddi_translations_edp_0_85V);
-   return cnl_ddi_translations_dp_0_85V;
+   return cnl_ddi_translations_edp_0_85V;
} else if (voltage == VOLTAGE_INFO_0_95V) {
*n_entries = ARRAY_SIZE(cnl_ddi_translations_edp_0_95V);
return cnl_ddi_translations_edp_0_95V;
-- 
2.13.2.932.g7449e964c-goog

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] drm/i915: Consistently use enum pipe for PCH transcoders

[Matthias Kaehlcke]

The current code uses in some instances enum transcoder for PCH
transcoders and enum pipe in others. This is error prone and clang
raises warnings like this:

drivers/gpu/drm/i915/intel_dp.c:3546:51: warning: implicit conversion
  from enumeration type 'enum pipe' to different enumeration type
  'enum transcoder' [-Wenum-conversion]
intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);

Consistently use the type enum pipe for PCH transcoders.

Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
---
Changes in v2:
- rebased on drm-intel/drm-intel-next-queued

 drivers/gpu/drm/i915/i915_irq.c| 10 +-
 drivers/gpu/drm/i915/intel_display.c   | 24 ++--
 drivers/gpu/drm/i915/intel_drv.h   |  6 +++---
 drivers/gpu/drm/i915/intel_fifo_underrun.c |  6 +++---
 4 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1d33cea01a1b..0b6f310101ee 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2086,10 +2086,10 @@ static void ibx_irq_handler(struct drm_i915_private 
*dev_priv, u32 pch_iir)
DRM_DEBUG_DRIVER("PCH transcoder CRC error interrupt\n");
 
if (pch_iir & SDE_TRANSA_FIFO_UNDER)
-   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_A);
+   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_A);
 
if (pch_iir & SDE_TRANSB_FIFO_UNDER)
-   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_B);
+   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_B);
 }
 
 static void ivb_err_int_handler(struct drm_i915_private *dev_priv)
@@ -2123,13 +2123,13 @@ static void cpt_serr_int_handler(struct 
drm_i915_private *dev_priv)
DRM_ERROR("PCH poison interrupt\n");
 
if (serr_int & SERR_INT_TRANS_A_FIFO_UNDERRUN)
-   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_A);
+   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_A);
 
if (serr_int & SERR_INT_TRANS_B_FIFO_UNDERRUN)
-   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_B);
+   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_B);
 
if (serr_int & SERR_INT_TRANS_C_FIFO_UNDERRUN)
-   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_C);
+   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_C);
 
I915_WRITE(SERR_INT, serr_int);
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index bb9c9c3c391f..5c7054c3be0e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1777,7 +1777,7 @@ static void lpt_enable_pch_transcoder(struct 
drm_i915_private *dev_priv,
 
/* FDI must be feeding us bits for PCH ports */
assert_fdi_tx_enabled(dev_priv, (enum pipe) cpu_transcoder);
-   assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
+   assert_fdi_rx_enabled(dev_priv, PIPE_A);
 
/* Workaround: set timing override bit. */
val = I915_READ(TRANS_CHICKEN2(PIPE_A));
@@ -1853,16 +1853,16 @@ void lpt_disable_pch_transcoder(struct drm_i915_private 
*dev_priv)
I915_WRITE(TRANS_CHICKEN2(PIPE_A), val);
 }
 
-enum transcoder intel_crtc_pch_transcoder(struct intel_crtc *crtc)
+enum pipe intel_crtc_pch_transcoder(struct intel_crtc *crtc)
 {
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 
WARN_ON(!crtc->config->has_pch_encoder);
 
if (HAS_PCH_LPT(dev_priv))
-   return TRANSCODER_A;
+   return PIPE_A;
else
-   return (enum transcoder) crtc->pipe;
+   return crtc->pipe;
 }
 
 /**
@@ -1901,7 +1901,7 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
if (crtc->config->has_pch_encoder) {
/* if driving the PCH, we need FDI enabled */
assert_fdi_rx_pll_enabled(dev_priv,
- (enum pipe) 
intel_crtc_pch_transcoder(crtc));
+ 
intel_crtc_pch_transcoder(crtc));
assert_fdi_tx_pll_enabled(dev_priv,
  (enum pipe) cpu_transcoder);
}
@@ -4579,7 +4579,7 @@ static void lpt_pch_enable(const struct intel_crtc_state 
*crtc_state)
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
 
-   assert_pch_transcoder_disabled(dev_priv, TRANSCODER_A);
+   assert_pch_transcoder_disabled(dev_priv, PIPE_A);
 
lpt_program_iclkip(crtc);
 
@ -5347,8 +5347,7 @@ static void haswell_crtc_enable(struct intel_crtc_state 
*pipe_config,
return;
 
if (intel_crtc->co

Re: [Intel-gfx] [PATCH] drm/i915: Consistently use enum pipe for PCH transcoders

[Matthias Kaehlcke]

El Mon, Jul 17, 2017 at 11:07:01AM +0200 Daniel Vetter ha dit:

> On Fri, Jul 14, 2017 at 06:04:03PM -0700, Matthias Kaehlcke wrote:
> > The current code uses two different enum types for PCH transcoders and
> > performs implicit conversions between the two types. This is error prone
> > and causes clang to raise warnings like this:
> > 
> > drivers/gpu/drm/i915/intel_dp.c:3546:51: warning: implicit conversion
> >   from enumeration type 'enum pipe' to different enumeration type
> >   'enum transcoder' [-Wenum-conversion]
> > intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> > 
> > Consistently use the type enum pipe for PCH transcoders.
> > 
> > Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
> 
> Thanks for respinning. Unfortunately it doesn't apply cleanly to
> drm-intel-next-queued, and I don't have a clang setup ready to confirm I
> didn't screw up anything. Can you pls rebase?

Thanks for having a look. The patch was against v4.12, I will rebase
it on drm-intel-next-queued.

> > ---
> >  drivers/gpu/drm/i915/i915_irq.c| 10 +-
> >  drivers/gpu/drm/i915/intel_display.c   | 24 ++--
> >  drivers/gpu/drm/i915/intel_drv.h   |  6 +++---
> >  drivers/gpu/drm/i915/intel_fifo_underrun.c |  6 +++---
> >  4 files changed, 21 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c 
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index 190f6aa5d15e..7960d2170750 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2132,10 +2132,10 @@ static void ibx_irq_handler(struct drm_i915_private 
> > *dev_priv, u32 pch_iir)
> > DRM_DEBUG_DRIVER("PCH transcoder CRC error interrupt\n");
> >  
> > if (pch_iir & SDE_TRANSA_FIFO_UNDER)
> > -   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_A);
> > +   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_A);
> >  
> > if (pch_iir & SDE_TRANSB_FIFO_UNDER)
> > -   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_B);
> > +   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_B);
> >  }
> >  
> >  static void ivb_err_int_handler(struct drm_i915_private *dev_priv)
> > @@ -2169,13 +2169,13 @@ static void cpt_serr_int_handler(struct 
> > drm_i915_private *dev_priv)
> > DRM_ERROR("PCH poison interrupt\n");
> >  
> > if (serr_int & SERR_INT_TRANS_A_FIFO_UNDERRUN)
> > -   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_A);
> > +   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_A);
> >  
> > if (serr_int & SERR_INT_TRANS_B_FIFO_UNDERRUN)
> > -   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_B);
> > +   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_B);
> >  
> > if (serr_int & SERR_INT_TRANS_C_FIFO_UNDERRUN)
> > -   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_C);
> > +   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_C);
> >  
> > I915_WRITE(SERR_INT, serr_int);
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 9106ea32b048..21a8fea46ad9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1782,7 +1782,7 @@ static void lpt_enable_pch_transcoder(struct 
> > drm_i915_private *dev_priv,
> >  
> > /* FDI must be feeding us bits for PCH ports */
> > assert_fdi_tx_enabled(dev_priv, (enum pipe) cpu_transcoder);
> > -   assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
> > +   assert_fdi_rx_enabled(dev_priv, PIPE_A);
> >  
> > /* Workaround: set timing override bit. */
> > val = I915_READ(TRANS_CHICKEN2(PIPE_A));
> > @@ -1858,16 +1858,16 @@ void lpt_disable_pch_transcoder(struct 
> > drm_i915_private *dev_priv)
> > I915_WRITE(TRANS_CHICKEN2(PIPE_A), val);
> >  }
> >  
> > -enum transcoder intel_crtc_pch_transcoder(struct intel_crtc *crtc)
> > +enum pipe intel_crtc_pch_transcoder(struct intel_crtc *crtc)
> >  {
> > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >  
> > WARN_ON(!crtc->config->has_pch_encoder);
> >  
> > if (HAS_PCH_LPT(dev_priv))
> > -   return TRANSCODER_A;
> > +   return PIPE_A;
> > else
> > -   return (enum transcoder) crtc->pipe;
> > +   return c

[Intel-gfx] [PATCH] drm/i915: Consistently use enum pipe for PCH transcoders

[Matthias Kaehlcke]

The current code uses two different enum types for PCH transcoders and
performs implicit conversions between the two types. This is error prone
and causes clang to raise warnings like this:

drivers/gpu/drm/i915/intel_dp.c:3546:51: warning: implicit conversion
  from enumeration type 'enum pipe' to different enumeration type
  'enum transcoder' [-Wenum-conversion]
intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);

Consistently use the type enum pipe for PCH transcoders.

Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
---
 drivers/gpu/drm/i915/i915_irq.c| 10 +-
 drivers/gpu/drm/i915/intel_display.c   | 24 ++--
 drivers/gpu/drm/i915/intel_drv.h   |  6 +++---
 drivers/gpu/drm/i915/intel_fifo_underrun.c |  6 +++---
 4 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 190f6aa5d15e..7960d2170750 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2132,10 +2132,10 @@ static void ibx_irq_handler(struct drm_i915_private 
*dev_priv, u32 pch_iir)
DRM_DEBUG_DRIVER("PCH transcoder CRC error interrupt\n");
 
if (pch_iir & SDE_TRANSA_FIFO_UNDER)
-   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_A);
+   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_A);
 
if (pch_iir & SDE_TRANSB_FIFO_UNDER)
-   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_B);
+   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_B);
 }
 
 static void ivb_err_int_handler(struct drm_i915_private *dev_priv)
@@ -2169,13 +2169,13 @@ static void cpt_serr_int_handler(struct 
drm_i915_private *dev_priv)
DRM_ERROR("PCH poison interrupt\n");
 
if (serr_int & SERR_INT_TRANS_A_FIFO_UNDERRUN)
-   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_A);
+   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_A);
 
if (serr_int & SERR_INT_TRANS_B_FIFO_UNDERRUN)
-   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_B);
+   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_B);
 
if (serr_int & SERR_INT_TRANS_C_FIFO_UNDERRUN)
-   intel_pch_fifo_underrun_irq_handler(dev_priv, TRANSCODER_C);
+   intel_pch_fifo_underrun_irq_handler(dev_priv, PIPE_C);
 
I915_WRITE(SERR_INT, serr_int);
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 9106ea32b048..21a8fea46ad9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1782,7 +1782,7 @@ static void lpt_enable_pch_transcoder(struct 
drm_i915_private *dev_priv,
 
/* FDI must be feeding us bits for PCH ports */
assert_fdi_tx_enabled(dev_priv, (enum pipe) cpu_transcoder);
-   assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
+   assert_fdi_rx_enabled(dev_priv, PIPE_A);
 
/* Workaround: set timing override bit. */
val = I915_READ(TRANS_CHICKEN2(PIPE_A));
@@ -1858,16 +1858,16 @@ void lpt_disable_pch_transcoder(struct drm_i915_private 
*dev_priv)
I915_WRITE(TRANS_CHICKEN2(PIPE_A), val);
 }
 
-enum transcoder intel_crtc_pch_transcoder(struct intel_crtc *crtc)
+enum pipe intel_crtc_pch_transcoder(struct intel_crtc *crtc)
 {
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 
WARN_ON(!crtc->config->has_pch_encoder);
 
if (HAS_PCH_LPT(dev_priv))
-   return TRANSCODER_A;
+   return PIPE_A;
else
-   return (enum transcoder) crtc->pipe;
+   return crtc->pipe;
 }
 
 /**
@@ -1906,7 +1906,7 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
if (crtc->config->has_pch_encoder) {
/* if driving the PCH, we need FDI enabled */
assert_fdi_rx_pll_enabled(dev_priv,
- (enum pipe) 
intel_crtc_pch_transcoder(crtc));
+ 
intel_crtc_pch_transcoder(crtc));
assert_fdi_tx_pll_enabled(dev_priv,
  (enum pipe) cpu_transcoder);
}
@@ -4573,7 +4573,7 @@ static void lpt_pch_enable(const struct intel_crtc_state 
*crtc_state)
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
 
-   assert_pch_transcoder_disabled(dev_priv, TRANSCODER_A);
+   assert_pch_transcoder_disabled(dev_priv, PIPE_A);
 
lpt_program_iclkip(crtc);
 
@@ -5329,8 +5329,7 @@ static void haswell_crtc_enable(struct intel_crtc_state 
*pipe_config,
return;
 
if (intel_crtc->config->has_pch_encode

Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches

[Matthias Kaehlcke]

El Fri, Jul 14, 2017 at 03:43:35PM -0700 Grant Grundler ha dit:

> On Fri, Jul 14, 2017 at 2:35 PM, Daniel Vetter <dan...@ffwll.ch> wrote:
> > On Fri, Jul 14, 2017 at 7:32 PM, Grant Grundler <grund...@chromium.org> 
> > wrote:
> >> On Fri, Jul 14, 2017 at 2:11 AM, Jani Nikula
> >> <jani.nik...@linux.intel.com> wrote:
> >>> On Thu, 13 Jul 2017, Stéphane Marchesin <stephane.marche...@gmail.com> 
> >>> wrote:
> >>>> So, if you think this is wrong, can you fix this warning in a way that
> >>>> you'd like?
> >>>
> >>> As I replied previously [1], with more background, fixing the warnings
> >>> properly, in a way that actually improves the code instead of making it
> >>> worse, would mean a bunch of churn that's not just purely mechanical
> >>> conversion.
> >>
> >> That's fair.
> >>
> >>> Unless you can point out a bug which is actually caused by mixing the
> >>> types (which is mostly intentional, see the background) I have a hard
> >>> time telling people this should be a priority.
> >>
> >> This feels like "can't see the forest because of the trees".
> >>
> >> The original patch was submitted in order to compile cleanly using
> >> clang and the above suggests using clang is not important.  Using
> >> clang is important to Matthias and the Chrome OS organization for many
> >> good reasons - including better warnings.
> >>
> >> The original patch message was clear that clang was generating the
> >> warning. This isn't the only patch mka has sent to kernel devs. What
> >> one can infer is Chrome OS is trying to move to clang (like other
> >> Google products _already_ have.)  My impression is all these products
> >> are a priority to Intel - but it would be good to know otherwise.
> >>
> >>> Definitely something we'd
> >>> like to do in the long run and pedantically correct (and I tend to
> >>> prefer code that way) but we certainly have more important things to do.
> >>
> >> The long run is now. Everyone agrees the code should change and you
> >> don't have to do it. Matthias submitted an unacceptable patch and
> >> giving him some concrete guidance on what would be acceptable would
> >> enable him to implement/test it (or anyone else could for that
> >> matter).  Can you do that?
> >>
> >> Just give an example of what the "right" API looks like and see where it 
> >> goes.
> >
> > We've replied and discussed on May 5th what that roughly should be,
> > right when Matthias pinged us. The original submission unfortunately
> > fell through the cracks (it happens, not much we can do with this
> > flood). Matthias didn't seem to have any questions about the proposed
> > solutions (we laid out both the minimal short-term fix to unconfuse
> > things, and what might be done on top), I think a reasonable
> > assumption was that it's all clear. Otherwise he should have asked.
> 
> Indeed!
> 
> After briefly chatting with Stephane and mka, it seems the difference
> between short-term fix and "done on top" were not clear.
> 
> > Now, over 2 months later (and complete silence from your side) there's
> > suddenly mass panic and multiple escalations on all available
> > channels, which feels like a rather decent overreaction and not a
> > terrible constructive way to collaborate on the upstream codebase.
> 
> I'm sorry - I'm not on the other channels and I didn't see any mass
> panic. I agree that's not a collaborative. The previous answer in this
> thread didn't seem particularly collaborative either though.
> 
> The silence was partly due to mka working on other "clang enablement" patches:

Yes, sorry for the silence :(

With my lack of expertise with this driver and graphics in general I
wasn't sure if I'd take up the "done on top" solution and shifted my
attention to other clang related issues.

> $ pwclient list -w m...@chromium.org
> Patches submitted by Matthias Kaehlcke <m...@chromium.org>:
> ID  StateName
> --  -
> 9668095 Superseded   mac80211: Fix clang warning about constant
> operand in logical operation
> 9668479 Accepted ath9k: Add cast to u8 to FREQ2FBIN macro
> 9668643 Accepted [v2] mac80211: Fix clang warning about constant
> operand in logical operation
> 9679753 Accepted [v2] cfg80211: Fix array-bounds warning in fragment copy
> 9684547 Accepted mac80211: ibss: Fix channel type enum in
> 

Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches

[Matthias Kaehlcke]

Hi,

El Fri, May 05, 2017 at 01:29:32PM -0700 Grant Grundler ha dit:

> On Fri, May 5, 2017 at 1:08 PM, Ville Syrjälä
>  wrote:
> ...
> >> > I'm not convinced the patch is making things any better really. To
> >> > fix this really properly, I think we'd need to introduce a new enum
> >> > pch_transcoder and thus avoid the confusion of which type of
> >> > transcoder we're talking about.

I agree, the patch certainly doesn't improve the confusing use of the enums.

> >> Is an enum better than coding an explicit conversion in an inline function?
> >
> > The point of the enum would be to make it more clear which piece of
> > hardware we're talking to in each case.
> 
> Ah ok - I misunderstood - I thought this was already the case.
> 
> > But this would require going
> > through the entire PCH code and changing things to use the right type
> > in each case. Quite a bit of work with little measurable gain I'd say.
> 
> IMHO, one of the best things that happened to C standard was addition
> of strong type checking. It's helped prevent developers from making
> one class of "stupid mistakes". So while this change wouldn't improve
> performance, it would allow a form of automated correctness checking
> that can be enforced with every patch you get (every time the code
> base is compiled).
> 
> In other words, the gain isn't currently measurable the same way
> performance is but I believe it's worth doing.  Given the number of
> typedefs and enums in kernel code, I suspect most kernel developers
> would agree.

I also think that proper use of enums is an additional line of defense
against "stupid mistakes". While weeding out these warnings in
different drivers I came across a few cases were the code was working
out of sheer luck because the (unintentionally) mismatched enums
resolved to the same value.

Cheers

Matthias

> >> Then the code can do some sanity checking as well. Something like:
> >>
> >> enum transcoder pch_to_cpu_enum(enum pipe)
> >> {
> >> WARN_ON(pipe > FOO);
> >> return (enum transcoder) pipe;
> >> }
> >
> > That would have to be called pipe_to_pch_transcoder() or something like
> > that.
> >
> >>
> >> > Currently most places expect an
> >> > enum pipe when dealing with PCH transcoders, and enum transcoder
> >> > when dealing with CPU transcoders. But there are some exceptions
> >> > of course.
> >>
> >> cheers,
> >> grant
> >> >
> >
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches

[Matthias Kaehlcke]

El Thu, Apr 20, 2017 at 02:56:05PM -0700 Matthias Kaehlcke ha dit:

> In several instances the driver passes an 'enum pipe' value to a
> function expecting an 'enum transcoder' and viceversa. Since PIPE_x and
> TRANSCODER_x have the same values this doesn't cause functional
> problems. Still it is incorrect and causes clang to generate warnings
> like this:
> 
> drivers/gpu/drm/i915/intel_display.c:1844:34: warning: implicit
>   conversion from enumeration type 'enum transcoder' to different
>   enumeration type 'enum pipe' [-Wenum-conversion]
> assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
> 
> Change the code to pass values of the type expected by the callee.
> 
> Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>  drivers/gpu/drm/i915/intel_dp.c  | 6 --
>  drivers/gpu/drm/i915/intel_hdmi.c| 6 --
>  drivers/gpu/drm/i915/intel_sdvo.c| 6 --
>  4 files changed, 14 insertions(+), 8 deletions(-)

Ping, any comments on this patch?

(also excuses for the unintended use of the RESEND tag)

Cheers

Matthias
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches

[Matthias Kaehlcke]

In several instances the driver passes an 'enum pipe' value to a
function expecting an 'enum transcoder' and viceversa. Since PIPE_x and
TRANSCODER_x have the same values this doesn't cause functional
problems. Still it is incorrect and causes clang to generate warnings
like this:

drivers/gpu/drm/i915/intel_display.c:1844:34: warning: implicit
  conversion from enumeration type 'enum transcoder' to different
  enumeration type 'enum pipe' [-Wenum-conversion]
assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);

Change the code to pass values of the type expected by the callee.

Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 drivers/gpu/drm/i915/intel_dp.c  | 6 --
 drivers/gpu/drm/i915/intel_hdmi.c| 6 --
 drivers/gpu/drm/i915/intel_sdvo.c| 6 --
 4 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index ed1f4f272b4f..23484f042fae 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1841,7 +1841,7 @@ static void lpt_enable_pch_transcoder(struct 
drm_i915_private *dev_priv,
 
/* FDI must be feeding us bits for PCH ports */
assert_fdi_tx_enabled(dev_priv, (enum pipe) cpu_transcoder);
-   assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
+   assert_fdi_rx_enabled(dev_priv, PIPE_A);
 
/* Workaround: set timing override bit. */
val = I915_READ(TRANS_CHICKEN2(PIPE_A));
@@ -4607,7 +4607,7 @@ static void lpt_pch_enable(struct drm_crtc *crtc)
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
 
-   assert_pch_transcoder_disabled(dev_priv, TRANSCODER_A);
+   assert_pch_transcoder_disabled(dev_priv, PIPE_A);
 
lpt_program_iclkip(crtc);
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d1670b8afbf5..454c2d3dfdd6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3568,7 +3568,8 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 * doing the workaround. Sweep them under the rug.
 */
intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
-   intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+   intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
+ false);
 
/* always enable with pattern 1 (as per spec) */
DP &= ~(DP_PIPEB_SELECT | DP_LINK_TRAIN_MASK);
@@ -3582,7 +3583,8 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 
intel_wait_for_vblank_if_active(dev_priv, PIPE_A);
intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
-   intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
+   intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
+ true);
}
 
msleep(intel_dp->panel_power_down_delay);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c 
b/drivers/gpu/drm/i915/intel_hdmi.c
index 24b2fa5b6282..48b1f5d37204 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1153,7 +1153,8 @@ static void intel_disable_hdmi(struct intel_encoder 
*encoder,
 * doing the workaround. Sweep them under the rug.
 */
intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
-   intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+   intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
+ false);
 
temp &= ~SDVO_PIPE_B_SELECT;
temp |= SDVO_ENABLE;
@@ -1172,7 +1173,8 @@ static void intel_disable_hdmi(struct intel_encoder 
*encoder,
 
intel_wait_for_vblank_if_active(dev_priv, PIPE_A);
intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
-   intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
+   intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
+ true);
}
 
intel_hdmi->set_infoframes(>base, false, old_crtc_state, 
old_conn_state);
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c 
b/drivers/gpu/drm/i915/intel_sdvo.c
index 2ad13903a054..0568a9950f7f 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1462,7 +1462,8 @@ static void intel_disable_sdvo(struct intel_encoder 
*encoder,
 * doing the workaround. Sweep them under the rug.
 */
intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
-   intel_se