[Intel-gfx] [PULL] drm-misc-next

[Ville Syrjälä]

On Tue, Mar 06, 2018 at 02:01:21PM -0500, Sean Paul wrote:
> On Tue, Mar 06, 2018 at 07:42:53AM +0100, Daniel Vetter wrote:
> > On Tue, Mar 6, 2018 at 12:20 AM, Sean Paul <seanpaul at chromium.org> wrote:
> > > On Mon, Mar 5, 2018 at 12:10 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> > >> On Fri, Mar 02, 2018 at 04:22:15PM -0500, Sean Paul wrote:
> > >>> On Wed, Feb 28, 2018 at 3:34 PM, Sean Paul <seanpaul at chromium.org> wrote:
> > >>> >
> > >>> > Hi Dave,
> > >>> > Here's this weeks pull, relatively small when you pull out the trivial fixes.
> > >>> >
> > >>> > drm-misc-next-2018-02-28:
> > >>> > drm-misc-next for 4.17:
> > >>> >
> > >>> > UAPI Changes:
> > >>> >  Fix drm_color_ctm matrix docs to match usage and change the type to
> > >>> >  __u64 make it obvious (Ville)
> > >>>
> > >>> Hi Dave,
> > >>> Could you please hold off on pulling this? I'd like to sort out this
> > >>> change a bit more. We're already using the __s64 in chrome, and not
> > >>> explicitly sign-magnitude. I think it would be prudent to hash this
> > >>> out a little more.
> > >>>
> > >>> https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?l=161
> > >>
> > >> That code seems to be doing the exact same fun s.u63 math. This all looks
> > >> consistent to me.
> > >
> > > Hmm, yeah, I skimmed too quickly last week.
> > >
> > >>
> > >> Now in hindsight ofc we've screwed up the uapi, but well can't fix that
> > >> now again ...
> > >
> > > Yeah, I'm not convinced we should be changing the type. It's great to
> > > clarify the documentation to let userspace know it's sign-magnitude,
> > > but changing the type in-flight with users seems wrong.
> > 
> > But everyone must do unsigned bit ops to get this right, the s64 is
> > completely meaningless at best, and very likely will confuse someone.
> 
> It's definitely a good change to clarify the usage of the field, I'm not arguing
> against that.
> 
> > What do we benefit by not changing it? 
> 
> In the kernel, nothing. However changing uapi structs out from under userspace
> requires userspace updates. In order for that to happen, they need to be
> aware of the change and coordinate its rollout kernel/libdrm/compositor. At
> least in CrOS, Chrome people don't follow kernel changes, so they'll discover
> this with a compiler warning (hopefully) and have to backtrack what happened.
> 
> Given that everybody seems to be using this struct correctly, what do we benefit
> from changing it? Wouldn't a comment be sufficient? Perhaps a union of
> __s64/__u64 would be less disruptive.

Umm. What exactly broke? The code behind your link even casts the final
value to unsigned. So now the struct actually matches the code.

-- 
Ville Syrjälä
Intel OTC