Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches

[Grant Grundler]

On Fri, Jul 14, 2017 at 2:35 PM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Fri, Jul 14, 2017 at 7:32 PM, Grant Grundler <grundler@xxxxxxxxxxxx> wrote:
>> On Fri, Jul 14, 2017 at 2:11 AM, Jani Nikula
>> <jani.nikula@xxxxxxxxxxxxxxx> wrote:
>>> On Thu, 13 Jul 2017, StÃphane Marchesin <stephane.marchesin@xxxxxxxxx> 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@xxxxxxxxxxxx
Patches submitted by Matthias Kaehlcke <mka@xxxxxxxxxxxx>:
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