From: Grant Grundler <grundler@chromium.org>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: "Grant Grundler" <grundler@chromium.org>,
"Jani Nikula" <jani.nikula@linux.intel.com>,
intel-gfx <intel-gfx@lists.freedesktop.org>,
"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
"dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>,
"Michael Davidson" <md@google.com>,
"Stéphane Marchesin" <stephane.marchesin@gmail.com>,
"Matthias Kaehlcke" <mka@chromium.org>,
"Daniel Vetter" <daniel.vetter@intel.com>
Subject: Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches
Date: Fri, 14 Jul 2017 15:43:35 -0700 [thread overview]
Message-ID: <CANEJEGtaaLcJdGL6Wh4dNDp_87suPtWDLA6TabXHFKz4uQbOhg@mail.gmail.com> (raw)
In-Reply-To: <CAKMK7uEZ2bL5nipVBUZOCHWvJG0oUvmbe8ouO0xHy=p7JABZ+g@mail.gmail.com>
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
next prev parent reply other threads:[~2017-07-14 22:43 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-20 21:56 [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches Matthias Kaehlcke
2017-05-05 17:26 ` Matthias Kaehlcke
2017-05-05 17:40 ` [Intel-gfx] " Ville Syrjälä
2017-05-05 19:12 ` Grant Grundler
2017-05-05 20:08 ` Ville Syrjälä
2017-05-05 20:29 ` Grant Grundler
2017-05-05 21:37 ` Matthias Kaehlcke
2017-05-08 7:21 ` Daniel Vetter
2017-05-08 8:17 ` Jani Nikula
2017-07-13 2:28 ` Stéphane Marchesin
2017-07-13 10:13 ` Ville Syrjälä
2017-07-13 12:24 ` Daniel Vetter
2017-07-13 16:23 ` Stéphane Marchesin
2017-07-13 17:42 ` Ville Syrjälä
2017-07-13 19:58 ` Stéphane Marchesin
2017-07-14 9:11 ` Jani Nikula
2017-07-14 17:32 ` Grant Grundler
2017-07-14 21:35 ` Daniel Vetter
2017-07-14 22:43 ` Grant Grundler [this message]
2017-07-14 23:38 ` Matthias Kaehlcke
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CANEJEGtaaLcJdGL6Wh4dNDp_87suPtWDLA6TabXHFKz4uQbOhg@mail.gmail.com \
--to=grundler@chromium.org \
--cc=daniel.vetter@intel.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=md@google.com \
--cc=mka@chromium.org \
--cc=stephane.marchesin@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).