[PATCH 1/2] Revert "include/uapi/drm/amdgpu_drm.h: use __u32 and __u64 from <linux/types.h>"

[Daniel Vetter]

On Sat, Aug 20, 2016 at 8:58 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 20 August 2016 at 16:08, Marek Olšák <maraeo at gmail.com> wrote:
>> On Sat, Aug 20, 2016 at 2:20 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>> On 20 August 2016 at 12:47, Marek Olšák <maraeo at gmail.com> wrote:
>>>> On Sat, Aug 20, 2016 at 1:08 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>> On 20 August 2016 at 11:05, Marek Olšák <maraeo at gmail.com> wrote:
>>>>>> On Sat, Aug 20, 2016 at 12:54 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>>>> On 19 August 2016 at 15:26, Christian König <deathsimple at vodafone.de> wrote:
>>>>>>>> Am 19.08.2016 um 15:50 schrieb Marek Olšák:
>>>>>>>>>
>>>>>>>>> From: Marek Olšák <marek.olsak at amd.com>
>>>>>>>>>
>>>>>>>>> This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f.
>>>>>>>>>
>>>>>>>>> See the comment in the code. Basically, don't do cleanups in this header.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Marek Olšák <marek.olsak at amd.com>
>>>>>>>>
>>>>>>>>
>>>>>>>> I completely agree with you that this was a bad move, but I fear that we
>>>>>>>> will run into opposition with that.
>>>>>>>>
>>>>>>> Please check the facts before introducing RATHER ANNOYING AND HARD TO
>>>>>>> READ COMMENT IN ALL CAPS.
>>>>>>>
>>>>>>> Story time:
>>>>>>> I was dreaming of a day were we can stop installing these headers,
>>>>>>> thus making deprecation a bit easier process.
>>>>>>> Yet after failing to convince Dave and Daniel on a number of occasions
>>>>>>> I've accepted that those headers _are_ here to stay. And yes they
>>>>>>> _are_ the UAPI, even though no applications are meant to use them but
>>>>>>> the libdrm 'version'.
>>>>>>> Thus any changes to the libdrm ones should be a mirror of the ones
>>>>>>> here and libdrm should _not_ differ.
>>>>>>>
>>>>>>> But let's ignore all that and imagine that those headers are _not_
>>>>>>> UAPI. That gives us even greater reason to _not_ use the uintx_t types
>>>>>>> but the kernel __uX ones. The series that did these changes had a fair
>>>>>>> few references why we want that.
>>>>>>>
>>>>>>> Yes, I can imagine that the situation isn't ideal, and/or not that
>>>>>>> clear. Then again a check with git log should have straightened things
>>>>>>> out.
>>>>>>> If not _please_ help us improve this (documentation and/or others).
>>>>>>>
>>>>>>>
>>>>>>> And last but not least, please share with up what inspired this -
>>>>>>> (build/runtime) regression, attempted sync with libdrm, other ?
>>>>>>
>>>>>> Syncing with libdrm became difficult.
>>>>> Actually it should be easier now. Perhaps the radeon one was always a
>>>>> good citizen, but sadly that was not the case for the rest.
>>>>>
>>>>>> I'd like the diff between kernel
>>>>>> and libdrm to be as small as possible.
>>>>>>
>>>>> I believe we all agree on this one :-)
>>>>>
>>>>>> We must take into account that the uapi headers can potentially be
>>>>>> implemented by a different OS.
>>>>> Agreed. Have you looked at the 'compatibility layer' in drm.h ?
>>>>>
>>>>>> That's why they are in libdrm and
>>>>>> that's why nobody should make random changes to them in the kernel
>>>>>> tree. Do not think like a kernel developer isolated in Linux and just
>>>>>> consider the broader use case. If you do, you'll realize that it
>>>>>> simply doesn't make sense to use the __uX types here.
>>>>>>
>>>>> Ftr, like Rob (and maybe others) I believe that using __uX (in the
>>>>> kernel) is a bit odd, and opting for the stdint.h types should happen.
>>>>> But until/if that happens we have to live with the __uX ones.
>>>>>
>>>>> That said, I have poked various BSD people on a number of occasions,
>>>>> (hopefully) inspiring them to upstream their changes in a compatible
>>>>> way. Thus the whole "don't think like a kernel developer" doesn't
>>>>> really apply here :-\
>>>>>
>>>>> I'm simply one of the few fools^wpeople trying to make things OK for
>>>>> most (since one can never please everyone, all the time).
>>>>>
>>>>> IIRC the FreeBSD/DragonFly people had some issues with their
>>>>> compatibility layer since the kernel and userspace drm.h were
>>>>> divergent "by design" [1]. To make it even 'better' there's even two
>>>>> difference versions of drm.h in their kernel itself [2].
>>>>>
>>>>> What I am for is a discussion how to resolve things. Although expect
>>>>> resistance if you're thinking about applying tape, in order to fix
>>>>> somethings that's 'broken' elsewhere.
>>>>>
>>>>> If you or any !Linux folks are around on XDC we should really sit down
>>>>> and untangle some/all of these issues.
>>>>
>>>> It's not 100% certain but it looks like we won't be there.
>>>>
>>>> We need the uapi headers to be the same as libdrm ones to make syncing
>>>> easier. There is not much else to discuss here really. We (AMD) are
>>>> also the ones who have to work with these headers the most, not you, not Mikko.
>>>>
>>> Agreed and agreed.
>>>
>>>> While I understand some people want to discuss this further, these
>>>> patches must land first in order bring back the compatibility with
>>>> libdrm.
>>> This is where the misunderstanding lies - there _must_ _not_ be
>>> compatible with the libdrm ones, but the other way around. Check the
>>> output of $ git log -p -- include/drm in libdrm. Pretty please ?
>>>
>>>> After that, we can discuss the possible solutions and
>>>> everybody interested in a better solution *that will take libdrm into
>>>> account* can join. For now, I have to expect that those discussions
>>>> might also lead nowhere and
>>>
>>>> I don't wanna be stuck with bad uapi
>>>> headers in the kernel forever.
>>>>
>>> As mentioned before - please clearly state what do you perceive as bad
>>> and/or why. Daniel, myself and Rob (to a point) have explained that
>>> things are not perfect as-is but they are definitely not bad or wrong.
>>
>> The problem is the diff is different, which has been said many times.
>>
> I see two things, neither of which implies any problems.
>  - "syncing became difficult" which should _not_ be the case if you're
> using make headers_install
>  - unease about usage of __uX types and misdirected finger pointing
> about compatibility with other OS.
>
> All I can think of is that you (?) are porting some changes from the
> kernel to libdrm or vice-versa. In the latter case please _don't_ do
> that. Work with your changes in upstream kernel, then pull them down
> to libdrm with `make headers_install`.

Same here, I'm honestly not clear on what the problem is. uapi flows
from the kernel, we _must_ change the kernel headers first before
libdrm, and we can do that as long as we don't change the api or abi.

Resyncing is trivial using make headers_install and the flow Emil laid
out. Definitely _never_ apply uapi changes to libdrm first (or only,
and iirc one of the radeon headers had done that).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch