From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches Date: Fri, 5 May 2017 23:08:10 +0300 Message-ID: <20170505200810.GL12629@intel.com> References: <20170420215605.176722-1-mka@chromium.org> <20170505172636.GA128305@google.com> <20170505174043.GK12629@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: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Grant Grundler Cc: dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, LKML , Michael Davidson , Matthias Kaehlcke , Daniel Vetter List-Id: dri-devel@lists.freedesktop.org T24gRnJpLCBNYXkgMDUsIDIwMTcgYXQgMTI6MTI6NDlQTSAtMDcwMCwgR3JhbnQgR3J1bmRsZXIg d3JvdGU6Cj4gT24gRnJpLCBNYXkgNSwgMjAxNyBhdCAxMDo0MCBBTSwgVmlsbGUgU3lyasOkbMOk Cj4gPHZpbGxlLnN5cmphbGFAbGludXguaW50ZWwuY29tPiB3cm90ZToKPiA+IE9uIEZyaSwgTWF5 IDA1LCAyMDE3IGF0IDEwOjI2OjM2QU0gLTA3MDAsIE1hdHRoaWFzIEthZWhsY2tlIHdyb3RlOgo+ ID4+IEVsIFRodSwgQXByIDIwLCAyMDE3IGF0IDAyOjU2OjA1UE0gLTA3MDAgTWF0dGhpYXMgS2Fl aGxja2UgaGEgZGl0Ogo+ID4+Cj4gPj4gPiBJbiBzZXZlcmFsIGluc3RhbmNlcyB0aGUgZHJpdmVy IHBhc3NlcyBhbiAnZW51bSBwaXBlJyB2YWx1ZSB0byBhCj4gPj4gPiBmdW5jdGlvbiBleHBlY3Rp bmcgYW4gJ2VudW0gdHJhbnNjb2RlcicgYW5kIHZpY2V2ZXJzYS4gU2luY2UgUElQRV94IGFuZAo+ ID4+ID4gVFJBTlNDT0RFUl94IGhhdmUgdGhlIHNhbWUgdmFsdWVzIHRoaXMgZG9lc24ndCBjYXVz ZSBmdW5jdGlvbmFsCj4gPj4gPiBwcm9ibGVtcy4gU3RpbGwgaXQgaXMgaW5jb3JyZWN0IGFuZCBj YXVzZXMgY2xhbmcgdG8gZ2VuZXJhdGUgd2FybmluZ3MKPiA+PiA+IGxpa2UgdGhpczoKPiA+PiA+ Cj4gPj4gPiBkcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9kaXNwbGF5LmM6MTg0NDozNDogd2Fy bmluZzogaW1wbGljaXQKPiA+PiA+ICAgY29udmVyc2lvbiBmcm9tIGVudW1lcmF0aW9uIHR5cGUg J2VudW0gdHJhbnNjb2RlcicgdG8gZGlmZmVyZW50Cj4gPj4gPiAgIGVudW1lcmF0aW9uIHR5cGUg J2VudW0gcGlwZScgWy1XZW51bS1jb252ZXJzaW9uXQo+ID4+ID4gICAgIGFzc2VydF9mZGlfcnhf ZW5hYmxlZChkZXZfcHJpdiwgVFJBTlNDT0RFUl9BKTsKPiA+PiA+Cj4gPj4gPiBDaGFuZ2UgdGhl IGNvZGUgdG8gcGFzcyB2YWx1ZXMgb2YgdGhlIHR5cGUgZXhwZWN0ZWQgYnkgdGhlIGNhbGxlZS4K PiA+PiA+Cj4gPj4gPiBTaWduZWQtb2ZmLWJ5OiBNYXR0aGlhcyBLYWVobGNrZSA8bWthQGNocm9t aXVtLm9yZz4KPiA+PiA+IC0tLQo+ID4+ID4gIGRyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2Rp c3BsYXkuYyB8IDQgKystLQo+ID4+ID4gIGRyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2RwLmMg ICAgICB8IDYgKysrKy0tCj4gPj4gPiAgZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfaGRtaS5j ICAgIHwgNiArKysrLS0KPiA+PiA+ICBkcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9zZHZvLmMg ICAgfCA2ICsrKystLQo+ID4+ID4gIDQgZmlsZXMgY2hhbmdlZCwgMTQgaW5zZXJ0aW9ucygrKSwg OCBkZWxldGlvbnMoLSkKPiA+Pgo+ID4+IFBpbmcsIGFueSBjb21tZW50cyBvbiB0aGlzIHBhdGNo Pwo+ID4KPiA+IEknbSBub3QgY29udmluY2VkIHRoZSBwYXRjaCBpcyBtYWtpbmcgdGhpbmdzIGFu eSBiZXR0ZXIgcmVhbGx5LiBUbwo+ID4gZml4IHRoaXMgcmVhbGx5IHByb3Blcmx5LCBJIHRoaW5r IHdlJ2QgbmVlZCB0byBpbnRyb2R1Y2UgYSBuZXcgZW51bQo+ID4gcGNoX3RyYW5zY29kZXIgYW5k IHRodXMgYXZvaWQgdGhlIGNvbmZ1c2lvbiBvZiB3aGljaCB0eXBlIG9mCj4gPiB0cmFuc2NvZGVy IHdlJ3JlIHRhbGtpbmcgYWJvdXQuCj4gCj4gSXMgYW4gZW51bSBiZXR0ZXIgdGhhbiBjb2Rpbmcg YW4gZXhwbGljaXQgY29udmVyc2lvbiBpbiBhbiBpbmxpbmUgZnVuY3Rpb24/CgpUaGUgcG9pbnQg b2YgdGhlIGVudW0gd291bGQgYmUgdG8gbWFrZSBpdCBtb3JlIGNsZWFyIHdoaWNoIHBpZWNlIG9m CmhhcmR3YXJlIHdlJ3JlIHRhbGtpbmcgdG8gaW4gZWFjaCBjYXNlLiBCdXQgdGhpcyB3b3VsZCBy ZXF1aXJlIGdvaW5nCnRocm91Z2ggdGhlIGVudGlyZSBQQ0ggY29kZSBhbmQgY2hhbmdpbmcgdGhp bmdzIHRvIHVzZSB0aGUgcmlnaHQgdHlwZQppbiBlYWNoIGNhc2UuIFF1aXRlIGEgYml0IG9mIHdv cmsgd2l0aCBsaXR0bGUgbWVhc3VyYWJsZSBnYWluIEknZCBzYXkuCgo+IFRoZW4gdGhlIGNvZGUg Y2FuIGRvIHNvbWUgc2FuaXR5IGNoZWNraW5nIGFzIHdlbGwuIFNvbWV0aGluZyBsaWtlOgo+IAo+ IGVudW0gdHJhbnNjb2RlciBwY2hfdG9fY3B1X2VudW0oZW51bSBwaXBlKQo+IHsKPiAgICAgV0FS Tl9PTihwaXBlID4gRk9PKTsKPiAgICAgcmV0dXJuIChlbnVtIHRyYW5zY29kZXIpIHBpcGU7Cj4g fQoKVGhhdCB3b3VsZCBoYXZlIHRvIGJlIGNhbGxlZCBwaXBlX3RvX3BjaF90cmFuc2NvZGVyKCkg b3Igc29tZXRoaW5nIGxpa2UKdGhhdC4KCj4gCj4gPiBDdXJyZW50bHkgbW9zdCBwbGFjZXMgZXhw ZWN0IGFuCj4gPiBlbnVtIHBpcGUgd2hlbiBkZWFsaW5nIHdpdGggUENIIHRyYW5zY29kZXJzLCBh bmQgZW51bSB0cmFuc2NvZGVyCj4gPiB3aGVuIGRlYWxpbmcgd2l0aCBDUFUgdHJhbnNjb2RlcnMu IEJ1dCB0aGVyZSBhcmUgc29tZSBleGNlcHRpb25zCj4gPiBvZiBjb3Vyc2UuCj4gCj4gY2hlZXJz LAo+IGdyYW50Cj4gPgo+ID4gLS0KPiA+IFZpbGxlIFN5cmrDpGzDpAo+ID4gSW50ZWwgT1RDCgot LSAKVmlsbGUgU3lyasOkbMOkCkludGVsIE9UQwpfX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0 cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9s aXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755606AbdEEUJ5 (ORCPT ); Fri, 5 May 2017 16:09:57 -0400 Received: from mga01.intel.com ([192.55.52.88]:45262 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755478AbdEEUIV (ORCPT ); Fri, 5 May 2017 16:08:21 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,294,1491289200"; d="scan'208";a="83152780" Date: Fri, 5 May 2017 23:08:10 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Grant Grundler Cc: Matthias Kaehlcke , 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: <20170505200810.GL12629@intel.com> References: <20170420215605.176722-1-mka@chromium.org> <20170505172636.GA128305@google.com> <20170505174043.GK12629@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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ä > 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 > >> > --- > >> > 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