On Thu, May 4, 2017 at 11:43 AM, Lionel Landwerlin <lionel.g.landwer...@intel.com> wrote: > On 04/05/17 07:52, Emil Velikov wrote: >> >> On 4 May 2017 at 14:46, Daniel Vetter <dan...@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.veli...@gmail.com> writes: >>>>> >>>>>> On 25 April 2017 at 23:56, Lionel Landwerlin >>>>>> <lionel.g.landwer...@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
anv was designed to not rely on libdrm or libdrm_intel. I see the commit that added the libdrm dependency is from you and was not Reviewed or acked by Jason or any other core anv developer. I suggest we revert that, I don't see anything in the drmGetDevices2 code that is better suited for anv than what was there before. Kristian > For i965 : > drmCommandNone > drmCommandWriteRead > drmIoctl > drmPrimeFDToHandle > drmPrimeHandleToFD > > You're right, I'll update the patch to just drop libdrm_intel. > >> >>>>>> 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 > > >> 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. > >> >>>>>> 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. > 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. > > > > >> >>> Of course still the same rules apply for merging new uapi: All parts must >>> be reviewed, then we merge the kernel, and only afterwards userspace. The >>> headers process in libdrm (see libdrm/include/drm/README) is imo the best >>> model for that. >> >> Right that's another part of my argument. We are just about keeping >> developers to follow those. >> Copying the headers here will make it even easier for people to ignore >> the procedure. >> >> Not saying that people intentionally ignore it - sometimes we're >> tired, having a bad day, etc. >> At the same time tracking the same thing twice is simply a waste of >> time - let's not do it. >> >> Thanks >> Emil >> > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
