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

Thu May 4 20:56:43 UTC 2017

On Thu, May 4, 2017 at 12:21 PM, Kristian Høgsberg <hoegsberg at gmail.com>
wrote:

> On Thu, May 4, 2017 at 11:43 AM, 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
>
> 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.
>

I did comment on e-mail that I was begrudgingly ok with it.  In retrospect,
it looks pretty pointless.  As far as I can tell, drmGetDevices2 gains us
exactly two things over the old method of just trial and error:

 1. It stats the file to make sure that it's an actual DRM device before
opening the file.  Does this actually matter?  If so, it's easy enough to
add the dozen or so lines of code to do it in ANV.
 2. It walks over all files in /dev/dri rather than just
/dev/dri/renderD#.  That's also very easy to add.

I agree with Kristian that the right thing to do is to revert that patch
and just write the dozen lines of code needed to do it "properly" if it
even matters.

--Jason

> 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170504/80de4f09/attachment-0001.html>