Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751178AbdGNWnj (ORCPT ); Fri, 14 Jul 2017 18:43:39 -0400 Received: from mail-yb0-f181.google.com ([209.85.213.181]:33555 "EHLO mail-yb0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751115AbdGNWnh (ORCPT ); Fri, 14 Jul 2017 18:43:37 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170420215605.176722-1-mka@chromium.org> <20170505172636.GA128305@google.com> <20170505174043.GK12629@intel.com> <20170713101351.GS12629@intel.com> <20170713174232.GK12629@intel.com> <87eftjl6xg.fsf@nikula.org> From: Grant Grundler Date: Fri, 14 Jul 2017 15:43:35 -0700 X-Google-Sender-Auth: OcpNjy_EnXBD8iHYl03vdWE4Q6s Message-ID: Subject: Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches To: Daniel Vetter Cc: Grant Grundler , Jani Nikula , intel-gfx , Linux Kernel Mailing List , "dri-devel@lists.freedesktop.org" , Michael Davidson , =?UTF-8?Q?St=C3=A9phane_Marchesin?= , Matthias Kaehlcke , Daniel Vetter Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v6EMhiKj012448 Content-Length: 5714 Lines: 137 On Fri, Jul 14, 2017 at 2:35 PM, Daniel Vetter wrote: > On Fri, Jul 14, 2017 at 7:32 PM, Grant Grundler wrote: >> On Fri, Jul 14, 2017 at 2:11 AM, Jani Nikula >> wrote: >>> On Thu, 13 Jul 2017, Stéphane Marchesin wrote: >>>> So, if you think this is wrong, can you fix this warning in a way that >>>> you'd like? >>> >>> As I replied previously [1], with more background, fixing the warnings >>> properly, in a way that actually improves the code instead of making it >>> worse, would mean a bunch of churn that's not just purely mechanical >>> conversion. >> >> That's fair. >> >>> Unless you can point out a bug which is actually caused by mixing the >>> types (which is mostly intentional, see the background) I have a hard >>> time telling people this should be a priority. >> >> This feels like "can't see the forest because of the trees". >> >> The original patch was submitted in order to compile cleanly using >> clang and the above suggests using clang is not important. Using >> clang is important to Matthias and the Chrome OS organization for many >> good reasons - including better warnings. >> >> The original patch message was clear that clang was generating the >> warning. This isn't the only patch mka has sent to kernel devs. What >> one can infer is Chrome OS is trying to move to clang (like other >> Google products _already_ have.) My impression is all these products >> are a priority to Intel - but it would be good to know otherwise. >> >>> Definitely something we'd >>> like to do in the long run and pedantically correct (and I tend to >>> prefer code that way) but we certainly have more important things to do. >> >> The long run is now. Everyone agrees the code should change and you >> don't have to do it. Matthias submitted an unacceptable patch and >> giving him some concrete guidance on what would be acceptable would >> enable him to implement/test it (or anyone else could for that >> matter). Can you do that? >> >> Just give an example of what the "right" API looks like and see where it goes. > > We've replied and discussed on May 5th what that roughly should be, > right when Matthias pinged us. The original submission unfortunately > fell through the cracks (it happens, not much we can do with this > flood). Matthias didn't seem to have any questions about the proposed > solutions (we laid out both the minimal short-term fix to unconfuse > things, and what might be done on top), I think a reasonable > assumption was that it's all clear. Otherwise he should have asked. Indeed! After briefly chatting with Stephane and mka, it seems the difference between short-term fix and "done on top" were not clear. > Now, over 2 months later (and complete silence from your side) there's > suddenly mass panic and multiple escalations on all available > channels, which feels like a rather decent overreaction and not a > terrible constructive way to collaborate on the upstream codebase. I'm sorry - I'm not on the other channels and I didn't see any mass panic. I agree that's not a collaborative. The previous answer in this thread didn't seem particularly collaborative either though. The silence was partly due to mka working on other "clang enablement" patches: $ pwclient list -w mka@chromium.org Patches submitted by Matthias Kaehlcke : ID State Name -- ----- ---- 9668095 Superseded mac80211: Fix clang warning about constant operand in logical operation 9668479 Accepted ath9k: Add cast to u8 to FREQ2FBIN macro 9668643 Accepted [v2] mac80211: Fix clang warning about constant operand in logical operation 9679753 Accepted [v2] cfg80211: Fix array-bounds warning in fragment copy 9684547 Accepted mac80211: ibss: Fix channel type enum in ieee80211_sta_join_ibss() 9684629 Accepted nl80211: Fix enum type of variable in nl80211_put_sta_rate() > Anyway, I've done the quick draft for the function declaration changes > that would clear up the confusion, just needs a clang run to update > all the parameters to match, and passed that on to Stéphane Marchesin. Awesome - thanks! :) > I expect you to follow up with the corresponding patch right away. mka said "he would take a look at it". But knowing how he understates things in a typical "German Engineer" way, I'm optimistic it will be more than that. Thanks! cheers, grant > > Thanks a lot. > > Yours, Daniel > > For reference the diff, but probably whitespace mangled because the > real machine is down already: > > diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c > b/drivers/gpu/drm/i915/intel_fifo_underrun.c > index d484862cc7df..21c221b4ae57 100644 > --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c > +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c > @@ -313,7 +313,7 @@ bool intel_set_cpu_fifo_underrun_reporting(struct > drm_i915_private *dev_priv, > * Returns the previous state of underrun reporting. > */ > bool intel_set_pch_fifo_underrun_reporting(struct drm_i915_private *dev_priv, > - enum transcoder pch_transcoder, > + enum pipe pch_transcoder, > bool enable) > { > struct intel_crtc *crtc = > @@ -390,7 +390,7 @@ void intel_cpu_fifo_underrun_irq_handler(struct > drm_i915_private *dev_priv, > * interrupt to avoid an irq storm. > */ > void intel_pch_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv, > - enum transcoder pch_transcoder) > + enum pipe pch_transcoder) > { > if (intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder, > false)) { > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch