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

From: Matthias Kaehlcke <mka@chromium.org>
To: Grant Grundler <grundler@chromium.org>
Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Daniel Vetter" <daniel.vetter@intel.com>,
	"Jani Nikula" <jani.nikula@linux.intel.com>,
	"David Airlie" <airlied@linux.ie>,
	intel-gfx@lists.freedesktop.org,
	LKML <linux-kernel@vger.kernel.org>,
	"Greg Hackmann" <ghackmann@google.com>,
	"Michael Davidson" <md@google.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches
Date: Fri, 5 May 2017 14:37:32 -0700	[thread overview]
Message-ID: <20170505213732.GE128305@google.com> (raw)
In-Reply-To: <CANEJEGs0WARN-w4Gm9O39uB53BfV35VXWgUibsCX3UYpMBNxfQ@mail.gmail.com>

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