Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751194AbdGNXio (ORCPT ); Fri, 14 Jul 2017 19:38:44 -0400 Received: from mail-pf0-f171.google.com ([209.85.192.171]:33279 "EHLO mail-pf0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751004AbdGNXim (ORCPT ); Fri, 14 Jul 2017 19:38:42 -0400 Date: Fri, 14 Jul 2017 16:38:40 -0700 From: Matthias Kaehlcke To: Grant Grundler Cc: Daniel Vetter , Jani Nikula , intel-gfx , Linux Kernel Mailing List , "dri-devel@lists.freedesktop.org" , Michael Davidson , =?utf-8?B?U3TDqXBoYW5l?= Marchesin , Daniel Vetter Subject: Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches Message-ID: <20170714233840.GK95735@google.com> References: <20170505174043.GK12629@intel.com> <20170713101351.GS12629@intel.com> <20170713174232.GK12629@intel.com> <87eftjl6xg.fsf@nikula.org> 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 Content-Length: 6172 Lines: 143 El Fri, Jul 14, 2017 at 03:43:35PM -0700 Grant Grundler ha dit: > 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: Yes, sorry for the silence :( With my lack of expertise with this driver and graphics in general I wasn't sure if I'd take up the "done on top" solution and shifted my attention to other clang related issues. > $ 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. Thanks, that is helpful! > 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)) { > > > >