From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthias Kaehlcke Subject: Re: [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches Date: Fri, 5 May 2017 14:37:32 -0700 Message-ID: <20170505213732.GE128305@google.com> References: <20170420215605.176722-1-mka@chromium.org> <20170505172636.GA128305@google.com> <20170505174043.GK12629@intel.com> <20170505200810.GL12629@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Grant Grundler Cc: dri-devel@lists.freedesktop.org, David Airlie , intel-gfx@lists.freedesktop.org, LKML , Michael Davidson , Greg Hackmann , Daniel Vetter List-Id: dri-devel@lists.freedesktop.org SGksCgpFbCBGcmksIE1heSAwNSwgMjAxNyBhdCAwMToyOTozMlBNIC0wNzAwIEdyYW50IEdydW5k bGVyIGhhIGRpdDoKCj4gT24gRnJpLCBNYXkgNSwgMjAxNyBhdCAxOjA4IFBNLCBWaWxsZSBTeXJq w6Rsw6QKPiA8dmlsbGUuc3lyamFsYUBsaW51eC5pbnRlbC5jb20+IHdyb3RlOgo+IC4uLgo+ID4+ ID4gSSdtIG5vdCBjb252aW5jZWQgdGhlIHBhdGNoIGlzIG1ha2luZyB0aGluZ3MgYW55IGJldHRl ciByZWFsbHkuIFRvCj4gPj4gPiBmaXggdGhpcyByZWFsbHkgcHJvcGVybHksIEkgdGhpbmsgd2Un ZCBuZWVkIHRvIGludHJvZHVjZSBhIG5ldyBlbnVtCj4gPj4gPiBwY2hfdHJhbnNjb2RlciBhbmQg dGh1cyBhdm9pZCB0aGUgY29uZnVzaW9uIG9mIHdoaWNoIHR5cGUgb2YKPiA+PiA+IHRyYW5zY29k ZXIgd2UncmUgdGFsa2luZyBhYm91dC4KCkkgYWdyZWUsIHRoZSBwYXRjaCBjZXJ0YWlubHkgZG9l c24ndCBpbXByb3ZlIHRoZSBjb25mdXNpbmcgdXNlIG9mIHRoZSBlbnVtcy4KCj4gPj4gSXMgYW4g ZW51bSBiZXR0ZXIgdGhhbiBjb2RpbmcgYW4gZXhwbGljaXQgY29udmVyc2lvbiBpbiBhbiBpbmxp bmUgZnVuY3Rpb24/Cj4gPgo+ID4gVGhlIHBvaW50IG9mIHRoZSBlbnVtIHdvdWxkIGJlIHRvIG1h a2UgaXQgbW9yZSBjbGVhciB3aGljaCBwaWVjZSBvZgo+ID4gaGFyZHdhcmUgd2UncmUgdGFsa2lu ZyB0byBpbiBlYWNoIGNhc2UuCj4gCj4gQWggb2sgLSBJIG1pc3VuZGVyc3Rvb2QgLSBJIHRob3Vn aHQgdGhpcyB3YXMgYWxyZWFkeSB0aGUgY2FzZS4KPiAKPiA+IEJ1dCB0aGlzIHdvdWxkIHJlcXVp cmUgZ29pbmcKPiA+IHRocm91Z2ggdGhlIGVudGlyZSBQQ0ggY29kZSBhbmQgY2hhbmdpbmcgdGhp bmdzIHRvIHVzZSB0aGUgcmlnaHQgdHlwZQo+ID4gaW4gZWFjaCBjYXNlLiBRdWl0ZSBhIGJpdCBv ZiB3b3JrIHdpdGggbGl0dGxlIG1lYXN1cmFibGUgZ2FpbiBJJ2Qgc2F5Lgo+IAo+IElNSE8sIG9u ZSBvZiB0aGUgYmVzdCB0aGluZ3MgdGhhdCBoYXBwZW5lZCB0byBDIHN0YW5kYXJkIHdhcyBhZGRp dGlvbgo+IG9mIHN0cm9uZyB0eXBlIGNoZWNraW5nLiBJdCdzIGhlbHBlZCBwcmV2ZW50IGRldmVs b3BlcnMgZnJvbSBtYWtpbmcKPiBvbmUgY2xhc3Mgb2YgInN0dXBpZCBtaXN0YWtlcyIuIFNvIHdo aWxlIHRoaXMgY2hhbmdlIHdvdWxkbid0IGltcHJvdmUKPiBwZXJmb3JtYW5jZSwgaXQgd291bGQg YWxsb3cgYSBmb3JtIG9mIGF1dG9tYXRlZCBjb3JyZWN0bmVzcyBjaGVja2luZwo+IHRoYXQgY2Fu IGJlIGVuZm9yY2VkIHdpdGggZXZlcnkgcGF0Y2ggeW91IGdldCAoZXZlcnkgdGltZSB0aGUgY29k ZQo+IGJhc2UgaXMgY29tcGlsZWQpLgo+IAo+IEluIG90aGVyIHdvcmRzLCB0aGUgZ2FpbiBpc24n dCBjdXJyZW50bHkgbWVhc3VyYWJsZSB0aGUgc2FtZSB3YXkKPiBwZXJmb3JtYW5jZSBpcyBidXQg SSBiZWxpZXZlIGl0J3Mgd29ydGggZG9pbmcuICBHaXZlbiB0aGUgbnVtYmVyIG9mCj4gdHlwZWRl ZnMgYW5kIGVudW1zIGluIGtlcm5lbCBjb2RlLCBJIHN1c3BlY3QgbW9zdCBrZXJuZWwgZGV2ZWxv cGVycwo+IHdvdWxkIGFncmVlLgoKSSBhbHNvIHRoaW5rIHRoYXQgcHJvcGVyIHVzZSBvZiBlbnVt cyBpcyBhbiBhZGRpdGlvbmFsIGxpbmUgb2YgZGVmZW5zZQphZ2FpbnN0ICJzdHVwaWQgbWlzdGFr ZXMiLiBXaGlsZSB3ZWVkaW5nIG91dCB0aGVzZSB3YXJuaW5ncyBpbgpkaWZmZXJlbnQgZHJpdmVy cyBJIGNhbWUgYWNyb3NzIGEgZmV3IGNhc2VzIHdlcmUgdGhlIGNvZGUgd2FzIHdvcmtpbmcKb3V0 IG9mIHNoZWVyIGx1Y2sgYmVjYXVzZSB0aGUgKHVuaW50ZW50aW9uYWxseSkgbWlzbWF0Y2hlZCBl bnVtcwpyZXNvbHZlZCB0byB0aGUgc2FtZSB2YWx1ZS4KCkNoZWVycwoKTWF0dGhpYXMKCj4gPj4g VGhlbiB0aGUgY29kZSBjYW4gZG8gc29tZSBzYW5pdHkgY2hlY2tpbmcgYXMgd2VsbC4gU29tZXRo aW5nIGxpa2U6Cj4gPj4KPiA+PiBlbnVtIHRyYW5zY29kZXIgcGNoX3RvX2NwdV9lbnVtKGVudW0g cGlwZSkKPiA+PiB7Cj4gPj4gICAgIFdBUk5fT04ocGlwZSA+IEZPTyk7Cj4gPj4gICAgIHJldHVy biAoZW51bSB0cmFuc2NvZGVyKSBwaXBlOwo+ID4+IH0KPiA+Cj4gPiBUaGF0IHdvdWxkIGhhdmUg dG8gYmUgY2FsbGVkIHBpcGVfdG9fcGNoX3RyYW5zY29kZXIoKSBvciBzb21ldGhpbmcgbGlrZQo+ ID4gdGhhdC4KPiA+Cj4gPj4KPiA+PiA+IEN1cnJlbnRseSBtb3N0IHBsYWNlcyBleHBlY3QgYW4K PiA+PiA+IGVudW0gcGlwZSB3aGVuIGRlYWxpbmcgd2l0aCBQQ0ggdHJhbnNjb2RlcnMsIGFuZCBl bnVtIHRyYW5zY29kZXIKPiA+PiA+IHdoZW4gZGVhbGluZyB3aXRoIENQVSB0cmFuc2NvZGVycy4g QnV0IHRoZXJlIGFyZSBzb21lIGV4Y2VwdGlvbnMKPiA+PiA+IG9mIGNvdXJzZS4KPiA+Pgo+ID4+ IGNoZWVycywKPiA+PiBncmFudAo+ID4+ID4KPiA+Cl9fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fCkludGVsLWdmeCBtYWlsaW5nIGxpc3QKSW50ZWwtZ2Z4QGxp c3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFu L2xpc3RpbmZvL2ludGVsLWdmeAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753123AbdEEVhf (ORCPT ); Fri, 5 May 2017 17:37:35 -0400 Received: from mail-pf0-f174.google.com ([209.85.192.174]:36054 "EHLO mail-pf0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751290AbdEEVhe (ORCPT ); Fri, 5 May 2017 17:37:34 -0400 Date: Fri, 5 May 2017 14:37:32 -0700 From: Matthias Kaehlcke To: Grant Grundler Cc: Ville =?utf-8?B?U3lyasOkbMOk?= , Daniel Vetter , Jani Nikula , David Airlie , intel-gfx@lists.freedesktop.org, LKML , Greg Hackmann , Michael Davidson , dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches Message-ID: <20170505213732.GE128305@google.com> References: <20170420215605.176722-1-mka@chromium.org> <20170505172636.GA128305@google.com> <20170505174043.GK12629@intel.com> <20170505200810.GL12629@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >> > > >