[Mesa-dev] [PATCH 0/2] anv/i965: drop libdrm dependency completely

Fri May 5 16:00:05 UTC 2017

On 4 May 2017 at 19:43, Lionel Landwerlin <lionel.g.landwerlin at intel.com> wrote:
> On 04/05/17 07:52, Emil Velikov wrote:
>>
>> On 4 May 2017 at 14:46, Daniel Vetter <daniel at ffwll.ch> wrote:
>>>
>>> On Thu, Apr 27, 2017 at 10:58:42AM -0700, Lionel Landwerlin wrote:
>>>>
>>>> On 27/04/17 08:20, Eric Anholt wrote:
>>>>>
>>>>> Emil Velikov <emil.l.velikov at gmail.com> writes:
>>>>>
>>>>>> On 25 April 2017 at 23:56, Lionel Landwerlin
>>>>>> <lionel.g.landwerlin at intel.com> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> While working with changes that span from kernel to user space, I've
>>>>>>> been wondering whether we need to depend on libdrm at all for the anv
>>>>>>> & i965 drivers. Indeed with Ken's recent changes, we only depend on
>>>>>>> libdrm for its kernel header files which we could just embed
>>>>>>> ourselves.
>>>>>>>
>>>>>>> I've only included the minimal set of header files we need from the
>>>>>>> kernel for anv & i965. Maybe other drivers would be interested and
>>>>>>> maybe we should put all the kernel drm uapi headers into include?
>>>>>>>
>>>>>> AFAICT the goal behind the libdrm_intel fold was to allow rapid and
>>>>>> seamless rework of the interface.
>>>>>> With a potential goal to make it a shared one, as it gets stabilised.
>>>>>>
>>>>>> Currently ANV uses more than just the UAPI headers. But if we ignore
>>>>>> that, coping them is a bad idea.
>>>>
>>>> What else is anv using from libdrm? (maybe I missed something)
>>>>
>> Lionel, I stand corrected, we have instances in anv, wsi/x11, loader
>> and dri/i965 (the dri/common ones are just comments).
>> In total they seem to be over three dozen instances. Experiment with
>> the following:
>>
>> for i in `nm -CD --defined-only /lib/libdrm.so | cut -c 20-| grep -v
>> "^_" `; do git grep -w --name-only $i -- src ; done
>
>
> Thanks for the code snippet.
>
> For anv :
>     drmGetDevices2
> For i965 :
>     drmCommandNone
>     drmCommandWriteRead
>     drmIoctl
>     drmPrimeFDToHandle
>     drmPrimeHandleToFD
>
> You're right, I'll update the patch to just drop libdrm_intel.
>
Don't forget to remove the link against libdrm.

>>
>>>>>> Why - adds a, yet another, copy and making synchronisation more
>>>>>> annoying. First example - you blindly copied the files rather than
>>>>>> using `make headers_install' first ;-)
>>>>>> Not to mention that it makes the chicken and egg problem* even more
>>>>>> confusing and error prone.
>>>>
>>>> It doesn't feel like that to me. Actually instead of modifying 3
>>>> different
>>>> repos, you end up modifying just 2.
>>>> Sounds less error prone.
>>>>
>> Lionel, are you saying that we can ignore IGT? Or you're suggesting
>> that IGT should depend on Mesa copy of the headers?
>
>
> If you look closely at IGT, you'll notice quite a few tests actually define
> their own version of the structures/ioctl of the various driver interfaces.
> So it's more or less already happening.
>
> git grep DRM_IO ./tests/ | grep define
> git grep local_drm
>
>
IGT has local defines which get purged every so often. Still
"modifying just 2" is off-by one.

>> Seriously, your argument of "let's import because we can" is iffy. Why
>> stop with the DRM UAPI - let's import headers from glibc ;-)
>
>
> I think you have to look at what we're doing here. i965 & anv are graphics
> drivers tightly coupled with the kernel driver.
> libdrm_intel isn't, it's mostly generic enough code that is shared across
> some of our drivers.
> And since we drop that dependency, why bother with it at all?
>
> We don't really have the same relationship to other components (like glibc).
>
>>
>> If pulling new libdrm is that much of a nuisance to your workflow -
>> just copy the blob we have for the Travis instance.
>> It automatically tracks the libdrm version, builds and installs it as
>> needed.
>
>
> It's not about pulling, it's about maintaining.
>
The libdrm headers will still need to be synced and maintained
regardless of this series.
I wish we could make it disappear but sadly we cannot.

Speaking of maintenance can you comment on this patch [1]?
We have a similar instance in Mesa which we might want to address.

[1] https://patchwork.kernel.org/patch/9626861/

>>
>>>>>> Emil
>>>>>> *  Which patches land first - kernel or userspace
>>>>>
>>>>> I don't see how it does that at all.  It simplifies the process: Now
>>>>> you
>>>>> send out your proposed new userspace to one repo, instead of two.
>>>>>
>>>>> And, after the first commit, it'll be obvious when you screw up using
>>>>> make headers_install because there will be surprising diff.
>>>>
>>>> Right now we have to update libdrm, then update the mesa to depend on
>>>> the
>>>> right libdrm to actually get the header files we want.
>>>> If you depend on libdrm from more than just uapi headers, it might now
>>>> be
>>>> too much of a burden. But it seems we're clearly moving away from that
>>>> in
>>>> anv/i965.
>>>> As a developer it feels a lot easier to have just the update mesa with
>>>> both
>>>> the new kernel API you depend on & the changes to the user space driver
>>>> using that API.
>>>
>>> As long as the headers are never installed into the system I'm in
>>> principle ok with pulling all the i915.ko specific stuff into mesa. Seems
>>> like a reasonable thing to do.
>>>
>> Daniel, Lionel's earlier suggestion (see the "modifying just 2" part
>> above) implies that either a) Mesa should install these or b) we can
>> ignore other components such as IGT.
>> Neither of which sounds cool IMHO.
>
>
> Feel pretty cool to me.
Which part - a or b?

> I don't think I can put it better than Eric did :
>
> On 04/05/17 09:38, Eric Anholt wrote:
>>
>> And it works great, because kernel headers are backwards compatible.
>> When you need a feature, you just merge the header update necessary and
>> no other developers get disrupted.
>>
>> Have you done kernel API feature development?  I feel like this is the
>> kind of thing you need to do yourself several times, with several
>> revisions over the course of months, before understanding the
>> limitations of our current process.
>
Once we find a volunteer to fix the bugs that I go through, I would
gladly dive into kernel API development.

At the same time - the only downside AFAICT of the current process is
that it's not clear and concise enough.
Before I wrote a document for libdrm - close to nobody knew the
process, let alone have a change to follow it.

I wish that after our chat (which inspired the v4 of [2]) you would
trust me a little bit more.

Gents, do as you wish - I won't interfere with any of this. I do
reserve myself the right of "I told you so" should any of my
predictions come true ;-)

Thanks
Emil

[2] https://cgit.freedesktop.org/mesa/drm/commit/?id=3c717f61f885240980bfc4273dbd1fc837edc391