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

From: Grant Grundler <grundler@chromium.org>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Grant Grundler <grundler@chromium.org>,
	Matthias Kaehlcke <mka@chromium.org>,
	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 13:29:32 -0700	[thread overview]
Message-ID: <CANEJEGs0WARN-w4Gm9O39uB53BfV35VXWgUibsCX3UYpMBNxfQ@mail.gmail.com> (raw)
In-Reply-To: <20170505200810.GL12629@intel.com>

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