| Message ID | CAKMK7uEZ2bL5nipVBUZOCHWvJG0oUvmbe8ouO0xHy=p7JABZ+g@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 14, 2017 at 2:35 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Jul 14, 2017 at 7:32 PM, Grant Grundler <grundler@chromium.org> wrote: >> On Fri, Jul 14, 2017 at 2:11 AM, Jani Nikula >> <jani.nikula@linux.intel.com> wrote: >>> On Thu, 13 Jul 2017, Stéphane Marchesin <stephane.marchesin@gmail.com> 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 <mka@chromium.org>: 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
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 <daniel@ffwll.ch> wrote: > > On Fri, Jul 14, 2017 at 7:32 PM, Grant Grundler <grundler@chromium.org> wrote: > >> On Fri, Jul 14, 2017 at 2:11 AM, Jani Nikula > >> <jani.nikula@linux.intel.com> wrote: > >>> On Thu, 13 Jul 2017, Stéphane Marchesin <stephane.marchesin@gmail.com> 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 <mka@chromium.org>: > 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)) { > > > >
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) {