On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä
<[email protected]> wrote:
>
> On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote:
> > El Thu, Apr 20, 2017 at 02:56:05PM -0700 Matthias Kaehlcke ha dit:
> >
> > > In several instances the driver passes an 'enum pipe' value to a
> > > function expecting an 'enum transcoder' and viceversa. Since PIPE_x and
> > > TRANSCODER_x have the same values this doesn't cause functional
> > > problems. Still it is incorrect and causes clang to generate warnings
> > > like this:
> > >
> > > drivers/gpu/drm/i915/intel_display.c:1844:34: warning: implicit
> > > conversion from enumeration type 'enum transcoder' to different
> > > enumeration type 'enum pipe' [-Wenum-conversion]
> > > assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
> > >
> > > Change the code to pass values of the type expected by the callee.
> > >
> > > Signed-off-by: Matthias Kaehlcke <[email protected]>
> > > ---
> > > drivers/gpu/drm/i915/intel_display.c | 4 ++--
> > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> > > drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++--
> > > drivers/gpu/drm/i915/intel_sdvo.c | 6 ++++--
> > > 4 files changed, 14 insertions(+), 8 deletions(-)
> >
> > Ping, any comments on this patch?
>
> I'm not convinced the patch is making things any better really. To
> fix this really properly, I think we'd need to introduce a new enum
> pch_transcoder and thus avoid the confusion of which type of
> transcoder we're talking about. Currently most places expect an
> enum pipe when dealing with PCH transcoders, and enum transcoder
> when dealing with CPU transcoders. But there are some exceptions
> of course.
I don't follow -- these functions take an enum transcoder; what's
wrong about passing what they expect? It seems like what you are
asking for has nothing to do with the warning here...
Stéphane
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jul 12, 2017 at 07:28:14PM -0700, St?phane Marchesin wrote:
> On Fri, May 5, 2017 at 10:40 AM, Ville Syrj?l?
> <[email protected]> wrote:
> >
> > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote:
> > > El Thu, Apr 20, 2017 at 02:56:05PM -0700 Matthias Kaehlcke ha dit:
> > >
> > > > In several instances the driver passes an 'enum pipe' value to a
> > > > function expecting an 'enum transcoder' and viceversa. Since PIPE_x and
> > > > TRANSCODER_x have the same values this doesn't cause functional
> > > > problems. Still it is incorrect and causes clang to generate warnings
> > > > like this:
> > > >
> > > > drivers/gpu/drm/i915/intel_display.c:1844:34: warning: implicit
> > > > conversion from enumeration type 'enum transcoder' to different
> > > > enumeration type 'enum pipe' [-Wenum-conversion]
> > > > assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
> > > >
> > > > Change the code to pass values of the type expected by the callee.
> > > >
> > > > Signed-off-by: Matthias Kaehlcke <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_display.c | 4 ++--
> > > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> > > > drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++--
> > > > drivers/gpu/drm/i915/intel_sdvo.c | 6 ++++--
> > > > 4 files changed, 14 insertions(+), 8 deletions(-)
> > >
> > > Ping, any comments on this patch?
> >
> > I'm not convinced the patch is making things any better really. To
> > fix this really properly, I think we'd need to introduce a new enum
> > pch_transcoder and thus avoid the confusion of which type of
> > transcoder we're talking about. Currently most places expect an
> > enum pipe when dealing with PCH transcoders, and enum transcoder
> > when dealing with CPU transcoders. But there are some exceptions
> > of course.
>
>
> I don't follow -- these functions take an enum transcoder; what's
> wrong about passing what they expect? It seems like what you are
> asking for has nothing to do with the warning here...
There's a warning? I don't get any.
Anyways, I just don't see much point in blindly changing the types
because it doesn't actually solve the underlying confusion for human
readers. It might even make it worse, not sure.
--
Ville Syrj?l?
Intel OTC
On Thu, Jul 13, 2017 at 01:13:51PM +0300, Ville Syrj?l? wrote:
> On Wed, Jul 12, 2017 at 07:28:14PM -0700, St?phane Marchesin wrote:
> > On Fri, May 5, 2017 at 10:40 AM, Ville Syrj?l?
> > <[email protected]> wrote:
> > >
> > > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote:
> > > > El Thu, Apr 20, 2017 at 02:56:05PM -0700 Matthias Kaehlcke ha dit:
> > > >
> > > > > In several instances the driver passes an 'enum pipe' value to a
> > > > > function expecting an 'enum transcoder' and viceversa. Since PIPE_x and
> > > > > TRANSCODER_x have the same values this doesn't cause functional
> > > > > problems. Still it is incorrect and causes clang to generate warnings
> > > > > like this:
> > > > >
> > > > > drivers/gpu/drm/i915/intel_display.c:1844:34: warning: implicit
> > > > > conversion from enumeration type 'enum transcoder' to different
> > > > > enumeration type 'enum pipe' [-Wenum-conversion]
> > > > > assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
> > > > >
> > > > > Change the code to pass values of the type expected by the callee.
> > > > >
> > > > > Signed-off-by: Matthias Kaehlcke <[email protected]>
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_display.c | 4 ++--
> > > > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> > > > > drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++--
> > > > > drivers/gpu/drm/i915/intel_sdvo.c | 6 ++++--
> > > > > 4 files changed, 14 insertions(+), 8 deletions(-)
> > > >
> > > > Ping, any comments on this patch?
> > >
> > > I'm not convinced the patch is making things any better really. To
> > > fix this really properly, I think we'd need to introduce a new enum
> > > pch_transcoder and thus avoid the confusion of which type of
> > > transcoder we're talking about. Currently most places expect an
> > > enum pipe when dealing with PCH transcoders, and enum transcoder
> > > when dealing with CPU transcoders. But there are some exceptions
> > > of course.
> >
> >
> > I don't follow -- these functions take an enum transcoder; what's
> > wrong about passing what they expect? It seems like what you are
> > asking for has nothing to do with the warning here...
>
> There's a warning? I don't get any.
>
> Anyways, I just don't see much point in blindly changing the types
> because it doesn't actually solve the underlying confusion for human
> readers. It might even make it worse, not sure.
Yeah the current patch just makes it worse. Either enum pch_transcoder, or
the enum pipe changes I suggested somewhere else. Blindly fixing type
mismatches without actually fixing the underlying confusion isn't really
useful work. And at least the switch to enum pipe for pch won't be (much)
bigger.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, Jul 13, 2017 at 3:13 AM, Ville Syrjälä
<[email protected]> wrote:
> On Wed, Jul 12, 2017 at 07:28:14PM -0700, Stéphane Marchesin wrote:
>> On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä
>> <[email protected]> wrote:
>> >
>> > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote:
>> > > El Thu, Apr 20, 2017 at 02:56:05PM -0700 Matthias Kaehlcke ha dit:
>> > >
>> > > > In several instances the driver passes an 'enum pipe' value to a
>> > > > function expecting an 'enum transcoder' and viceversa. Since PIPE_x and
>> > > > TRANSCODER_x have the same values this doesn't cause functional
>> > > > problems. Still it is incorrect and causes clang to generate warnings
>> > > > like this:
>> > > >
>> > > > drivers/gpu/drm/i915/intel_display.c:1844:34: warning: implicit
>> > > > conversion from enumeration type 'enum transcoder' to different
>> > > > enumeration type 'enum pipe' [-Wenum-conversion]
>> > > > assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
>> > > >
>> > > > Change the code to pass values of the type expected by the callee.
>> > > >
>> > > > Signed-off-by: Matthias Kaehlcke <[email protected]>
>> > > > ---
>> > > > drivers/gpu/drm/i915/intel_display.c | 4 ++--
>> > > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
>> > > > drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++--
>> > > > drivers/gpu/drm/i915/intel_sdvo.c | 6 ++++--
>> > > > 4 files changed, 14 insertions(+), 8 deletions(-)
>> > >
>> > > Ping, any comments on this patch?
>> >
>> > I'm not convinced the patch is making things any better really. To
>> > fix this really properly, I think we'd need to introduce a new enum
>> > pch_transcoder and thus avoid the confusion of which type of
>> > transcoder we're talking about. Currently most places expect an
>> > enum pipe when dealing with PCH transcoders, and enum transcoder
>> > when dealing with CPU transcoders. But there are some exceptions
>> > of course.
>>
>>
>> I don't follow -- these functions take an enum transcoder; what's
>> wrong about passing what they expect? It seems like what you are
>> asking for has nothing to do with the warning here...
>
> There's a warning? I don't get any.
Yup, clang generates a warning.
>
> Anyways, I just don't see much point in blindly changing the types
> because it doesn't actually solve the underlying confusion for human
> readers. It might even make it worse, not sure.
The function expects type A, you pass type B, how can that ever be the
right thing to do?
Stéphane
>
> --
> Ville Syrjälä
> Intel OTC
On Thu, Jul 13, 2017 at 09:23:11AM -0700, St?phane Marchesin wrote:
> On Thu, Jul 13, 2017 at 3:13 AM, Ville Syrj?l?
> <[email protected]> wrote:
> > On Wed, Jul 12, 2017 at 07:28:14PM -0700, St?phane Marchesin wrote:
> >> On Fri, May 5, 2017 at 10:40 AM, Ville Syrj?l?
> >> <[email protected]> wrote:
> >> >
> >> > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote:
> >> > > El Thu, Apr 20, 2017 at 02:56:05PM -0700 Matthias Kaehlcke ha dit:
> >> > >
> >> > > > In several instances the driver passes an 'enum pipe' value to a
> >> > > > function expecting an 'enum transcoder' and viceversa. Since PIPE_x and
> >> > > > TRANSCODER_x have the same values this doesn't cause functional
> >> > > > problems. Still it is incorrect and causes clang to generate warnings
> >> > > > like this:
> >> > > >
> >> > > > drivers/gpu/drm/i915/intel_display.c:1844:34: warning: implicit
> >> > > > conversion from enumeration type 'enum transcoder' to different
> >> > > > enumeration type 'enum pipe' [-Wenum-conversion]
> >> > > > assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
> >> > > >
> >> > > > Change the code to pass values of the type expected by the callee.
> >> > > >
> >> > > > Signed-off-by: Matthias Kaehlcke <[email protected]>
> >> > > > ---
> >> > > > drivers/gpu/drm/i915/intel_display.c | 4 ++--
> >> > > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> >> > > > drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++--
> >> > > > drivers/gpu/drm/i915/intel_sdvo.c | 6 ++++--
> >> > > > 4 files changed, 14 insertions(+), 8 deletions(-)
> >> > >
> >> > > Ping, any comments on this patch?
> >> >
> >> > I'm not convinced the patch is making things any better really. To
> >> > fix this really properly, I think we'd need to introduce a new enum
> >> > pch_transcoder and thus avoid the confusion of which type of
> >> > transcoder we're talking about. Currently most places expect an
> >> > enum pipe when dealing with PCH transcoders, and enum transcoder
> >> > when dealing with CPU transcoders. But there are some exceptions
> >> > of course.
> >>
> >>
> >> I don't follow -- these functions take an enum transcoder; what's
> >> wrong about passing what they expect? It seems like what you are
> >> asking for has nothing to do with the warning here...
> >
> > There's a warning? I don't get any.
>
> Yup, clang generates a warning.
>
> >
> > Anyways, I just don't see much point in blindly changing the types
> > because it doesn't actually solve the underlying confusion for human
> > readers. It might even make it worse, not sure.
>
> The function expects type A, you pass type B, how can that ever be the
> right thing to do?
Because maybe the function should be taking in type B instead.
--
Ville Syrj?l?
Intel OTC
On Thu, Jul 13, 2017 at 10:42 AM, Ville Syrjälä
<[email protected]> wrote:
> On Thu, Jul 13, 2017 at 09:23:11AM -0700, Stéphane Marchesin wrote:
>> On Thu, Jul 13, 2017 at 3:13 AM, Ville Syrjälä
>> <[email protected]> wrote:
>> > On Wed, Jul 12, 2017 at 07:28:14PM -0700, Stéphane Marchesin wrote:
>> >> On Fri, May 5, 2017 at 10:40 AM, Ville Syrjälä
>> >> <[email protected]> wrote:
>> >> >
>> >> > On Fri, May 05, 2017 at 10:26:36AM -0700, Matthias Kaehlcke wrote:
>> >> > > El Thu, Apr 20, 2017 at 02:56:05PM -0700 Matthias Kaehlcke ha dit:
>> >> > >
>> >> > > > In several instances the driver passes an 'enum pipe' value to a
>> >> > > > function expecting an 'enum transcoder' and viceversa. Since PIPE_x and
>> >> > > > TRANSCODER_x have the same values this doesn't cause functional
>> >> > > > problems. Still it is incorrect and causes clang to generate warnings
>> >> > > > like this:
>> >> > > >
>> >> > > > drivers/gpu/drm/i915/intel_display.c:1844:34: warning: implicit
>> >> > > > conversion from enumeration type 'enum transcoder' to different
>> >> > > > enumeration type 'enum pipe' [-Wenum-conversion]
>> >> > > > assert_fdi_rx_enabled(dev_priv, TRANSCODER_A);
>> >> > > >
>> >> > > > Change the code to pass values of the type expected by the callee.
>> >> > > >
>> >> > > > Signed-off-by: Matthias Kaehlcke <[email protected]>
>> >> > > > ---
>> >> > > > drivers/gpu/drm/i915/intel_display.c | 4 ++--
>> >> > > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
>> >> > > > drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++--
>> >> > > > drivers/gpu/drm/i915/intel_sdvo.c | 6 ++++--
>> >> > > > 4 files changed, 14 insertions(+), 8 deletions(-)
>> >> > >
>> >> > > Ping, any comments on this patch?
>> >> >
>> >> > I'm not convinced the patch is making things any better really. To
>> >> > fix this really properly, I think we'd need to introduce a new enum
>> >> > pch_transcoder and thus avoid the confusion of which type of
>> >> > transcoder we're talking about. Currently most places expect an
>> >> > enum pipe when dealing with PCH transcoders, and enum transcoder
>> >> > when dealing with CPU transcoders. But there are some exceptions
>> >> > of course.
>> >>
>> >>
>> >> I don't follow -- these functions take an enum transcoder; what's
>> >> wrong about passing what they expect? It seems like what you are
>> >> asking for has nothing to do with the warning here...
>> >
>> > There's a warning? I don't get any.
>>
>> Yup, clang generates a warning.
>>
>> >
>> > Anyways, I just don't see much point in blindly changing the types
>> > because it doesn't actually solve the underlying confusion for human
>> > readers. It might even make it worse, not sure.
>>
>> The function expects type A, you pass type B, how can that ever be the
>> right thing to do?
>
> Because maybe the function should be taking in type B instead.
So, if you think this is wrong, can you fix this warning in a way that
you'd like?
Stéphane
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, 13 Jul 2017, Stéphane Marchesin <[email protected]> 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.
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. 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.
BR,
Jani.
[1] http://mid.mail-archive.com/[email protected]
--
Jani Nikula, Intel Open Source Technology Center
On Fri, Jul 14, 2017 at 2:11 AM, Jani Nikula
<[email protected]> wrote:
> On Thu, 13 Jul 2017, Stéphane Marchesin <[email protected]> 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.
cheers,
grant
>
> BR,
> Jani.
>
> [1] http://mid.mail-archive.com/[email protected]
>
>
> --
> Jani Nikula, Intel Open Source Technology Center
On Fri, Jul 14, 2017 at 7:32 PM, Grant Grundler <[email protected]> wrote:
> On Fri, Jul 14, 2017 at 2:11 AM, Jani Nikula
> <[email protected]> wrote:
>> On Thu, 13 Jul 2017, Stéphane Marchesin <[email protected]> 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.
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.
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.
I expect you to follow up with the corresponding patch right away.
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
On Fri, Jul 14, 2017 at 2:35 PM, Daniel Vetter <[email protected]> wrote:
> On Fri, Jul 14, 2017 at 7:32 PM, Grant Grundler <[email protected]> wrote:
>> On Fri, Jul 14, 2017 at 2:11 AM, Jani Nikula
>> <[email protected]> wrote:
>>> On Thu, 13 Jul 2017, Stéphane Marchesin <[email protected]> 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 [email protected]
Patches submitted by Matthias Kaehlcke <[email protected]>:
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 <[email protected]> wrote:
> > On Fri, Jul 14, 2017 at 7:32 PM, Grant Grundler <[email protected]> wrote:
> >> On Fri, Jul 14, 2017 at 2:11 AM, Jani Nikula
> >> <[email protected]> wrote:
> >>> On Thu, 13 Jul 2017, Stéphane Marchesin <[email protected]> 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 [email protected]
> Patches submitted by Matthias Kaehlcke <[email protected]>:
> 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)) {
> >
> >