From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches Date: Mon, 08 May 2017 11:17:37 +0300 Message-ID: <87wp9rahjy.fsf@intel.com> References: <20170420215605.176722-1-mka@chromium.org> <20170505172636.GA128305@google.com> <20170505174043.GK12629@intel.com> <20170508072144.vcydqrmty7zkscw6@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20170508072144.vcydqrmty7zkscw6@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter , Ville =?utf-8?B?U3lyasOkbMOk?= Cc: Grant Grundler , intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Michael Davidson , Matthias Kaehlcke , Daniel Vetter List-Id: dri-devel@lists.freedesktop.org T24gTW9uLCAwOCBNYXkgMjAxNywgRGFuaWVsIFZldHRlciA8ZGFuaWVsQGZmd2xsLmNoPiB3cm90 ZToKPiBPbiBGcmksIE1heSAwNSwgMjAxNyBhdCAwODo0MDo0M1BNICswMzAwLCBWaWxsZSBTeXJq w6Rsw6Qgd3JvdGU6Cj4+IE9uIEZyaSwgTWF5IDA1LCAyMDE3IGF0IDEwOjI2OjM2QU0gLTA3MDAs IE1hdHRoaWFzIEthZWhsY2tlIHdyb3RlOgo+PiA+IEVsIFRodSwgQXByIDIwLCAyMDE3IGF0IDAy OjU2OjA1UE0gLTA3MDAgTWF0dGhpYXMgS2FlaGxja2UgaGEgZGl0Ogo+PiA+IAo+PiA+ID4gSW4g c2V2ZXJhbCBpbnN0YW5jZXMgdGhlIGRyaXZlciBwYXNzZXMgYW4gJ2VudW0gcGlwZScgdmFsdWUg dG8gYQo+PiA+ID4gZnVuY3Rpb24gZXhwZWN0aW5nIGFuICdlbnVtIHRyYW5zY29kZXInIGFuZCB2 aWNldmVyc2EuIFNpbmNlIFBJUEVfeCBhbmQKPj4gPiA+IFRSQU5TQ09ERVJfeCBoYXZlIHRoZSBz YW1lIHZhbHVlcyB0aGlzIGRvZXNuJ3QgY2F1c2UgZnVuY3Rpb25hbAo+PiA+ID4gcHJvYmxlbXMu IFN0aWxsIGl0IGlzIGluY29ycmVjdCBhbmQgY2F1c2VzIGNsYW5nIHRvIGdlbmVyYXRlIHdhcm5p bmdzCj4+ID4gPiBsaWtlIHRoaXM6Cj4+ID4gPiAKPj4gPiA+IGRyaXZlcnMvZ3B1L2RybS9pOTE1 L2ludGVsX2Rpc3BsYXkuYzoxODQ0OjM0OiB3YXJuaW5nOiBpbXBsaWNpdAo+PiA+ID4gICBjb252 ZXJzaW9uIGZyb20gZW51bWVyYXRpb24gdHlwZSAnZW51bSB0cmFuc2NvZGVyJyB0byBkaWZmZXJl bnQKPj4gPiA+ICAgZW51bWVyYXRpb24gdHlwZSAnZW51bSBwaXBlJyBbLVdlbnVtLWNvbnZlcnNp b25dCj4+ID4gPiAgICAgYXNzZXJ0X2ZkaV9yeF9lbmFibGVkKGRldl9wcml2LCBUUkFOU0NPREVS X0EpOwo+PiA+ID4gCj4+ID4gPiBDaGFuZ2UgdGhlIGNvZGUgdG8gcGFzcyB2YWx1ZXMgb2YgdGhl IHR5cGUgZXhwZWN0ZWQgYnkgdGhlIGNhbGxlZS4KPj4gPiA+IAo+PiA+ID4gU2lnbmVkLW9mZi1i eTogTWF0dGhpYXMgS2FlaGxja2UgPG1rYUBjaHJvbWl1bS5vcmc+Cj4+ID4gPiAtLS0KPj4gPiA+ ICBkcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9kaXNwbGF5LmMgfCA0ICsrLS0KPj4gPiA+ICBk cml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9kcC5jICAgICAgfCA2ICsrKystLQo+PiA+ID4gIGRy aXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2hkbWkuYyAgICB8IDYgKysrKy0tCj4+ID4gPiAgZHJp dmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfc2R2by5jICAgIHwgNiArKysrLS0KPj4gPiA+ICA0IGZp bGVzIGNoYW5nZWQsIDE0IGluc2VydGlvbnMoKyksIDggZGVsZXRpb25zKC0pCj4+ID4gCj4+ID4g UGluZywgYW55IGNvbW1lbnRzIG9uIHRoaXMgcGF0Y2g/Cj4+IAo+PiBJJ20gbm90IGNvbnZpbmNl ZCB0aGUgcGF0Y2ggaXMgbWFraW5nIHRoaW5ncyBhbnkgYmV0dGVyIHJlYWxseS4gVG8KPj4gZml4 IHRoaXMgcmVhbGx5IHByb3Blcmx5LCBJIHRoaW5rIHdlJ2QgbmVlZCB0byBpbnRyb2R1Y2UgYSBu ZXcgZW51bSAKPj4gcGNoX3RyYW5zY29kZXIgYW5kIHRodXMgYXZvaWQgdGhlIGNvbmZ1c2lvbiBv ZiB3aGljaCB0eXBlIG9mCj4+IHRyYW5zY29kZXIgd2UncmUgdGFsa2luZyBhYm91dC4gQ3VycmVu dGx5IG1vc3QgcGxhY2VzIGV4cGVjdCBhbiAKPj4gZW51bSBwaXBlIHdoZW4gZGVhbGluZyB3aXRo IFBDSCB0cmFuc2NvZGVycywgYW5kIGVudW0gdHJhbnNjb2Rlcgo+PiB3aGVuIGRlYWxpbmcgd2l0 aCBDUFUgdHJhbnNjb2RlcnMuIEJ1dCB0aGVyZSBhcmUgc29tZSBleGNlcHRpb25zCj4+IG9mIGNv dXJzZS4KPgo+IGVudW0gdHJhbnNjb2RlciBpcyB3cm9uZyBmb3IgdGhlIHBjaCwgdGhhdCBlbnVt IGlzIG9ubHkgZm9yIGNwdQo+IHRyYW5zY29kZXJzIGludHJvZHVjZWQgaW4gaHN3Ky4gUENIIHNo b3VsZCBhbHdheXMgdXNlIGVudW0gcGlwZS4KCkZvciBiYWNrZ3JvdW5kLCBiZWxvdyBpcyB0aGUg Y29tbWl0IG1lc3NhZ2UgZm9yIGludHJvZHVjdGlvbiBvZiBlbnVtCnRyYW5zY29kZXIuCgo+IFNv IGEgcGF0Y2ggdG8gc3dpdGNoIHRoZSBlbnVtIHRvIGVudW0gcGlwZSBmb3IgYWxsIHRoZSBwY2gg ZnVuY3Rpb25zCj4gc291bmRzIGxpa2UgdGhlIHJpZ2h0IHRoaW5nIHRvIGRvIGhlcmUuIFBsdXMg bWF5YmUgcmVuYW1lIGVudW0gdHJhbnNjb2Rlcgo+IHRvIGVudW0gY3B1X3RyYW5zY29kZXIsIGJ1 dCB0aGF0J2QgYmUgdG9ucyBvZiB3b3JrLiBBIGNvbW1lbnQgaW5zdGVhZAo+IG1pZ2h0IGJlIGVh c2llciAuLi4KClRoZSBlbnVtIHBpcGUgY29udmVyc2lvbiBtaWdodCBiZSB0aGUgcmlnaHQgdGhp bmcgdG8gZG8gKmlmKiB5b3UgbXVzdCBkbwpzb21ldGhpbmcuIEJ1dCBJJ20gbm90IGNvbnZpbmNl ZCB5b3UgbXVzdC4gSXQncyBhIGJ1bmNoIG9mIGNodXJuIHRoYXQncwpub3QganVzdCBwdXJlbHkg bWVjaGFuaWNhbCBjb252ZXJzaW9uLgoKQlIsCkphbmkuCgoKY29tbWl0IGE1Yzk2MWQxZjNhOWFi NWJhMGU1NzA2ZTg2NjE5MmY4MTA4MTQzZmUKQXV0aG9yOiBQYXVsbyBaYW5vbmkgPHBhdWxvLnIu emFub25pQGludGVsLmNvbT4KRGF0ZTogICBXZWQgT2N0IDI0IDE1OjU5OjM0IDIwMTIgLTAyMDAK CiAgICBkcm0vaTkxNTogYWRkIFRSQU5TQ09ERVJfRURQCiAgICAKICAgIEJlZm9yZSBIYXN3ZWxs IHdlIHVzZWQgdG8gaGF2ZSB0aGUgQ1BVIHBpcGVzIGFuZCB0aGUgUENIIHRyYW5zY29kZXJzLgog ICAgV2UgaGFkIHRoZSBzYW1lIGFtb3VudCBvZiBwaXBlcyBhbmQgdHJhbnNjb2RlcnMsIGFuZCB0 aGVyZSB3YXMgYSAxOjEKICAgIG1hcHBpbmcgYmV0d2VlbiB0aGVtLiBBZnRlciBIYXN3ZWxsIHdo YXQgd2UgdXNlZCB0byBjYWxsIENQVSBwaXBlIHdhcwogICAgc3BsaXQgaW50byBDUFUgcGlwZSBh bmQgQ1BVIHRyYW5zY29kZXIuIFNvIG5vdyB3ZSBoYXZlIDMgQ1BVIHBpcGVzIChBLAogICAgQiBh bmQgQyksIDQgQ1BVIHRyYW5zY29kZXJzIChBLCBCLCBDIGFuZCBFRFApIGFuZCAxIFBDSCB0cmFu c2NvZGVyCiAgICAob25seSB1c2VkIGZvciBWR0EpLgogICAgCiAgICBGb3IgYWxsIHRoZSBvdXRw dXRzIGV4Y2VwdCBmb3IgRURQIHdlIGhhdmUgYW4gMToxIG1hcHBpbmcgb24gdGhlIENQVQogICAg cGlwZXMgYW5kIENQVSB0cmFuc2NvZGVycywgc28gaWYgeW91J3JlIHVzaW5nIENQVSBwaXBlIEEg eW91IGhhdmUgdG8KICAgIHVzZSBDUFUgdHJhbnNjb2RlciBBLiBXaGVuIGhhdmUgYW4gZURQIG91 dHB1dCB5b3UgaGF2ZSB0byB1c2UKICAgIHRyYW5zY29kZXIgRURQIGFuZCB5b3UgY2FuIGF0dGFj aCB0aGlzIENQVSB0cmFuc2NvZGVyIHRvIGFueSBvZiB0aGUgMwogICAgQ1BVIHBpcGVzLiBXaGVu IHVzaW5nIFZHQSB5b3UgbmVlZCB0byBzZWxlY3QgYSBwYWlyIG9mIG1hdGNoaW5nIENQVQogICAg cGlwZXMvdHJhbnNjb2RlcnMgKEEvQSwgQi9CLCBDL0MpIGFuZCB5b3UgYWxzbyBuZWVkIHRvIGVu YWJsZS91c2UgdGhlCiAgICBQQ0ggdHJhbnNjb2Rlci4KICAgIAogICAgRm9yIG5vdyB3ZSdyZSBq dXN0IGNyZWF0aW5nIHRoZSBjcHVfdHJhbnNjb2RlciBkZWZpbml0aW9ucyBhbmQgc2V0dGluZwog ICAgY3B1X3RyYW5zY29kZXIgdG8gVFJBTlNDT0RFUl9FRFAgb24gRERJIGVEUCBjb2RlLCBidXQg bm9uZSBvZiB0aGUKICAgIHJlZ2lzdGVycyB3YXMgcG9ydGVkIHRvIHVzZSB0cmFuc2NvZGVyIGlu c3RlYWQgb2YgcGlwZS4gVGhlIGdvYWwgaXMgdG8KICAgIGtlZXAgdGhlIGNvZGUgYmFja3dhcmRz LWNvbXBhdGlibGUgc2luY2Ugb24gYWxsIGNhc2VzIGV4Y2VwdCB3aGVuCiAgICB1c2luZyBlRFAg d2UgbXVzdCBoYXZlIHBpcGUgPT0gY3B1X3RyYW5zY29kZXIuCiAgICAKICAgIFYyOiBDb21tZW50 IHRoZSBoYXN3ZWxsX2NydGNfb2ZmIGNodW5rLCBzdWdnZXN0ZWQgYnkgRGFtaWVuIExlc3BpYXUK ICAgIGFuZCBEYW5pZWwgVmV0dGVyLgogICAgCiAgICBXZSBjdXJyZW50bHkgbmVlZCB0aGUgaGFz d2VsbF9jcnRjX29mZiBjaHVuayBiZWNhdXNlIFRSQU5TQ09ERVJfRURQCiAgICBjYW4gYmUgdXNl ZCBieSBhbnkgQ1JUQywgc28gd2hlbiB5b3Ugc3RvcCB1c2luZyBpdCB5b3UgaGF2ZSB0byBzdG9w CiAgICBzYXlpbmcgeW91J3JlIHVzaW5nIGl0LCBvdGhlcndpc2UgeW91IG1heSBoYXZlIGF0IHNv bWUgcG9pbnQgMiBDUlRDcwogICAgY2xhaW1pbmcgdGhleSdyZSB1c2luZyBUUkFOU0NPREVSX0VE UCAoYSBkaXNhYmxlZCBDUlRDIGFuZCBhbiBlbmFibGVkCiAgICBvbmUpLCB0aGVuIHRoZSBIVyBz dGF0ZSByZWFkb3V0IGNvZGUgd2lsbCBnZXQgY29tcGxldGVseSBjb25mdXNlZC4KICAgIAogICAg SW4gb3RoZXIgd29yZHM6CiAgICAKICAgIEltYWdpbmUgdGhlIGZvbGxvd2luZyBjYXNlOgogICAg ICB4cmFuZHIgLS1vdXRwdXQgZURQMSAtLWF1dG8gLS1jcnRjIDAKICAgICAgeHJhbmRyIC0tb3V0 cHV0IGVEUDEgLS1vZmYKICAgICAgeHJhbmRyIC0tb3V0cHV0IGVEUDEgLS1hdXRvIC0tY3J0YyAy CiAgICAKICAgIEFmdGVyIHRoZSBsYXN0IGNvbW1hbmQgeW91IGNvdWxkIGdldCBhICJwaXBlIEEg YXNzZXJ0aW9uIGZhaWx1cmUKICAgIChleHBlY3RlZCBvZmYsIGN1cnJlbnQgb24pIiBiZWNhdXNl IENSVEMgMCBzdGlsbCBjbGFpbXMgaXQncyB1c2luZwogICAgVFJBTlNDT0RFUl9FRFAsIHNvIHRo ZSBIVyBzdGF0ZSByZWFkb3V0IGZ1bmN0aW9uIHdpbGwgcmVhZCBpdAogICAgKHRocm91Z2ggUElQ RUNPTkYpIGFuZCBleHBlY3QgaXQgdG8gYmUgb2ZmLCB3aGVuIGl0J3MgYWN0dWFsbHkgb24KICAg IGJlY2F1c2UgaXQncyBiZWluZyB1c2VkIGJ5IENSVEMgMi4KICAgIAogICAgU28gd2hlbiB3ZSBt YWtlICJpbnRlbF9jcnRjLT5jcHVfdHJhbnNjb2RlciA9IGludGVsX2NydGMtPnBpcGUiIHdlCiAg ICBtYWtlIHN1cmUgd2UncmUgcG9pbnRpbmcgdG8gb3VyIG93biBvcmlnaW5hbCBDUlRDIHdoaWNo IGlzIGNlcnRhaW5seQogICAgbm90IHVzZWQgYnkgYW55IG90aGVyIENSVEMuCiAgICAKICAgIFNp Z25lZC1vZmYtYnk6IFBhdWxvIFphbm9uaSA8cGF1bG8uci56YW5vbmlAaW50ZWwuY29tPgogICAg UmV2aWV3ZWQtYnk6IERhbWllbiBMZXNwaWF1IDxkYW1pZW4ubGVzcGlhdUBpbnRlbC5jb20+CiAg ICBTaWduZWQtb2ZmLWJ5OiBEYW5pZWwgVmV0dGVyIDxkYW5pZWwudmV0dGVyQGZmd2xsLmNoPgoK CgoKLS0gCkphbmkgTmlrdWxhLCBJbnRlbCBPcGVuIFNvdXJjZSBUZWNobm9sb2d5IENlbnRlcgpf X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpJbnRlbC1nZngg bWFpbGluZyBsaXN0CkludGVsLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753346AbdEHIR4 convert rfc822-to-8bit (ORCPT ); Mon, 8 May 2017 04:17:56 -0400 Received: from mga06.intel.com ([134.134.136.31]:5675 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750836AbdEHIRx (ORCPT ); Mon, 8 May 2017 04:17:53 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,308,1491289200"; d="scan'208";a="854121686" From: Jani Nikula To: Daniel Vetter , Ville =?utf-8?B?U3lyasOkbMOk?= Cc: intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Michael Davidson , Grant Grundler , Matthias Kaehlcke , Daniel Vetter Subject: Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches In-Reply-To: <20170508072144.vcydqrmty7zkscw6@phenom.ffwll.local> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20170420215605.176722-1-mka@chromium.org> <20170505172636.GA128305@google.com> <20170505174043.GK12629@intel.com> <20170508072144.vcydqrmty7zkscw6@phenom.ffwll.local> Date: Mon, 08 May 2017 11:17:37 +0300 Message-ID: <87wp9rahjy.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 08 May 2017, Daniel Vetter 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 >> > > --- >> > > 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 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 Reviewed-by: Damien Lespiau Signed-off-by: Daniel Vetter -- Jani Nikula, Intel Open Source Technology Center