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@xxxxxxxxxxxx> wrote: > > > On Mon, Mar 5, 2018 at 12:10 AM, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxxxxx> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
