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

[Sean Paul]

On Tue, Mar 06, 2018 at 09:07:52PM +0200, Ville Syrjälä wrote:
> 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.

We haven't pulled the change back, so nothing has changed in cros. You're right
that the code already does the right thing. So perhaps I'm just tilting at
windmills here, but this type of churn seemed unsettling to me.

Sean

> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Sean Paul, Software Engineer, Google / Chromium OS