| Message ID | 20170420215605.176722-1-mka@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <mka@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
On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote: > 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 <mka@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? 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. 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.
On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote: >> 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 <mka@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? > > 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. Is an enum better than coding an explicit conversion in an inline function? 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; } > 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 > > -- > Ville Syrjälä > Intel OTC
On Fri, May 05, 2017 at 12:12:49PM -0700, Grant Grundler wrote: > On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote: > >> 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 <mka@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? > > > > 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. > > 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. 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. > 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 > > > > -- > > Ville Syrjälä > > Intel OTC
On Fri, May 5, 2017 at 1:08 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> 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. >> >> 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. cheers, grant > >> 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 >> > >> > -- >> > Ville Syrjälä >> > Intel OTC > > -- > Ville Syrjälä > Intel OTC
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ä > <ville.syrjala@linux.intel.com> 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 > >> > > >
On Fri, May 05, 2017 at 08:40:43PM +0300, Ville Syrjälä wrote: > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote: > > 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 <mka@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? > > 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. 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. enum transcoder is wrong for the pch, that enum is only for cpu transcoders introduced in hsw+. PCH should always use enum pipe. So a patch to switch the enum to enum pipe for all the pch functions sounds like the right thing to do here. Plus maybe rename enum transcoder to enum cpu_transcoder, but that'd be tons of work. A comment instead might be easier ... -Daniel
On Mon, 08 May 2017, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, May 05, 2017 at 08:40:43PM +0300, Ville Syrjälä wrote: >> On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote: >> > 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 <mka@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? >> >> 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. 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. > > enum transcoder is wrong for the pch, that enum is only for cpu > transcoders introduced in hsw+. PCH should always use enum pipe. For background, below is the commit message for introduction of enum transcoder. > So a patch to switch the enum to enum pipe for all the pch functions > sounds like the right thing to do here. Plus maybe rename enum transcoder > to enum cpu_transcoder, but that'd be tons of work. A comment instead > might be easier ... The enum pipe conversion might be the right thing to do *if* you must do something. But I'm not convinced you must. It's a bunch of churn that's not just purely mechanical conversion. BR, Jani. commit a5c961d1f3a9ab5ba0e5706e866192f8108143fe Author: Paulo Zanoni <paulo.r.zanoni@intel.com> Date: Wed Oct 24 15:59:34 2012 -0200 drm/i915: add TRANSCODER_EDP Before Haswell we used to have the CPU pipes and the PCH transcoders. We had the same amount of pipes and transcoders, and there was a 1:1 mapping between them. After Haswell what we used to call CPU pipe was split into CPU pipe and CPU transcoder. So now we have 3 CPU pipes (A, B and C), 4 CPU transcoders (A, B, C and EDP) and 1 PCH transcoder (only used for VGA). For all the outputs except for EDP we have an 1:1 mapping on the CPU pipes and CPU transcoders, so if you're using CPU pipe A you have to use CPU transcoder A. When have an eDP output you have to use transcoder EDP and you can attach this CPU transcoder to any of the 3 CPU pipes. When using VGA you need to select a pair of matching CPU pipes/transcoders (A/A, B/B, C/C) and you also need to enable/use the PCH transcoder. For now we're just creating the cpu_transcoder definitions and setting cpu_transcoder to TRANSCODER_EDP on DDI eDP code, but none of the registers was ported to use transcoder instead of pipe. The goal is to keep the code backwards-compatible since on all cases except when using eDP we must have pipe == cpu_transcoder. V2: Comment the haswell_crtc_off chunk, suggested by Damien Lespiau and Daniel Vetter. We currently need the haswell_crtc_off chunk because TRANSCODER_EDP can be used by any CRTC, so when you stop using it you have to stop saying you're using it, otherwise you may have at some point 2 CRTCs claiming they're using TRANSCODER_EDP (a disabled CRTC and an enabled one), then the HW state readout code will get completely confused. In other words: Imagine the following case: xrandr --output eDP1 --auto --crtc 0 xrandr --output eDP1 --off xrandr --output eDP1 --auto --crtc 2 After the last command you could get a "pipe A assertion failure (expected off, current on)" because CRTC 0 still claims it's using TRANSCODER_EDP, so the HW state readout function will read it (through PIPECONF) and expect it to be off, when it's actually on because it's being used by CRTC 2. So when we make "intel_crtc->cpu_transcoder = intel_crtc->pipe" we make sure we're pointing to our own original CRTC which is certainly not used by any other CRTC. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote: > > 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 <mka@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? > > 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. 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. I don't follow -- these functions take an enum transcoder; what's wrong about passing what they expect? It seems like what you are asking for has nothing to do with the warning here... Stéphane > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jul 12, 2017 at 07:28:14PM -0700, Stéphane Marchesin wrote: > On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > > > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote: > > > 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 <mka@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? > > > > 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. 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. > > > I don't follow -- these functions take an enum transcoder; what's > wrong about passing what they expect? It seems like what you are > asking for has nothing to do with the warning here... There's a warning? I don't get any. Anyways, I just don't see much point in blindly changing the types because it doesn't actually solve the underlying confusion for human readers. It might even make it worse, not sure.
On Thu, Jul 13, 2017 at 01:13:51PM +0300, Ville Syrjälä wrote: > On Wed, Jul 12, 2017 at 07:28:14PM -0700, Stéphane Marchesin wrote: > > On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä > > <ville.syrjala@linux.intel.com> wrote: > > > > > > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote: > > > > 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 <mka@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? > > > > > > 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. 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. > > > > > > I don't follow -- these functions take an enum transcoder; what's > > wrong about passing what they expect? It seems like what you are > > asking for has nothing to do with the warning here... > > There's a warning? I don't get any. > > Anyways, I just don't see much point in blindly changing the types > because it doesn't actually solve the underlying confusion for human > readers. It might even make it worse, not sure. Yeah the current patch just makes it worse. Either enum pch_transcoder, or the enum pipe changes I suggested somewhere else. Blindly fixing type mismatches without actually fixing the underlying confusion isn't really useful work. And at least the switch to enum pipe for pch won't be (much) bigger. -Daniel
On Thu, Jul 13, 2017 at 3:13 AM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Jul 12, 2017 at 07:28:14PM -0700, Stéphane Marchesin wrote: >> On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä >> <ville.syrjala@linux.intel.com> wrote: >> > >> > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote: >> > > 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 <mka@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? >> > >> > 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. 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. >> >> >> I don't follow -- these functions take an enum transcoder; what's >> wrong about passing what they expect? It seems like what you are >> asking for has nothing to do with the warning here... > > There's a warning? I don't get any. Yup, clang generates a warning. > > Anyways, I just don't see much point in blindly changing the types > because it doesn't actually solve the underlying confusion for human > readers. It might even make it worse, not sure. The function expects type A, you pass type B, how can that ever be the right thing to do? Stéphane > > -- > Ville Syrjälä > Intel OTC
On Thu, Jul 13, 2017 at 09:23:11AM -0700, Stéphane Marchesin wrote: > On Thu, Jul 13, 2017 at 3:13 AM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > On Wed, Jul 12, 2017 at 07:28:14PM -0700, Stéphane Marchesin wrote: > >> On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä > >> <ville.syrjala@linux.intel.com> wrote: > >> > > >> > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote: > >> > > 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 <mka@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? > >> > > >> > 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. 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. > >> > >> > >> I don't follow -- these functions take an enum transcoder; what's > >> wrong about passing what they expect? It seems like what you are > >> asking for has nothing to do with the warning here... > > > > There's a warning? I don't get any. > > Yup, clang generates a warning. > > > > > Anyways, I just don't see much point in blindly changing the types > > because it doesn't actually solve the underlying confusion for human > > readers. It might even make it worse, not sure. > > The function expects type A, you pass type B, how can that ever be the > right thing to do? Because maybe the function should be taking in type B instead.
On Thu, Jul 13, 2017 at 10:42 AM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Jul 13, 2017 at 09:23:11AM -0700, Stéphane Marchesin wrote: >> On Thu, Jul 13, 2017 at 3:13 AM, Ville Syrjälä >> <ville.syrjala@linux.intel.com> wrote: >> > On Wed, Jul 12, 2017 at 07:28:14PM -0700, Stéphane Marchesin wrote: >> >> On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä >> >> <ville.syrjala@linux.intel.com> wrote: >> >> > >> >> > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote: >> >> > > 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 <mka@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? >> >> > >> >> > 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. 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. >> >> >> >> >> >> I don't follow -- these functions take an enum transcoder; what's >> >> wrong about passing what they expect? It seems like what you are >> >> asking for has nothing to do with the warning here... >> > >> > There's a warning? I don't get any. >> >> Yup, clang generates a warning. >> >> > >> > Anyways, I just don't see much point in blindly changing the types >> > because it doesn't actually solve the underlying confusion for human >> > readers. It might even make it worse, not sure. >> >> The function expects type A, you pass type B, how can that ever be the >> right thing to do? > > Because maybe the function should be taking in type B instead. So, if you think this is wrong, can you fix this warning in a way that you'd like? Stéphane > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, 13 Jul 2017, Stéphane Marchesin <stephane.marchesin@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. 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. 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. BR, Jani. [1] http://mid.mail-archive.com/87wp9rahjy.fsf@intel.com
On Fri, Jul 14, 2017 at 2:11 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Thu, 13 Jul 2017, Stéphane Marchesin <stephane.marchesin@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. cheers, grant > > BR, > Jani. > > [1] http://mid.mail-archive.com/87wp9rahjy.fsf@intel.com > > > -- > Jani Nikula, Intel Open Source Technology Center
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(&encoder->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_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; @@ -1473,7 +1474,8 @@ static void intel_disable_sdvo(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); } }
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 <mka@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(-)