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

[Matthias Kaehlcke]

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@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:

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@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.

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)) {
> >
> >