Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1636509 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches Matthias Kaehlcke <mka@chromium.org> - 2017-05-05 19:30 +0200
    Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum  mismatches Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-05-05 19:50 +0200
      Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches Grant Grundler <grundler@chromium.org> - 2017-05-05 21:20 +0200
        Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum  mismatches Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-05-05 22:20 +0200
          Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches Grant Grundler <grundler@chromium.org> - 2017-05-05 22:30 +0200
            Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum  mismatches Matthias Kaehlcke <mka@chromium.org> - 2017-05-05 23:40 +0200
      Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum  mismatches Daniel Vetter <daniel@ffwll.ch> - 2017-05-08 09:30 +0200
        Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches Jani Nikula <jani.nikula@linux.intel.com> - 2017-05-08 10:20 +0200

#1636509 — Re: [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches

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

[toc] | [next] | [standalone]

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

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.

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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
> >> >
> >

[toc] | [prev] | [next] | [standalone]

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

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

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

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>




-- 
Jani Nikula, Intel Open Source Technology Center

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web