[Mesa-dev] [PATCH 1/2] RFC i965: Bypass a couple of libraries for syscall on x84_64

[Dave Airlie]

On 20 June 2017 at 05:22, Chad Versace <chadversary at chromium.org> wrote:
> On Mon 19 Jun 2017, Eric Engestrom wrote:
>> On Monday, 2017-06-19 09:41:13 -0700, Jason Ekstrand wrote:
>> > On Mon, Jun 19, 2017 at 7:43 AM, Chris Wilson <chris at chris-wilson.co.uk>
>> > wrote:
>> > > > > Just a cosmetic improvement, but quite straightforward.
>> >
>> > I think it's a bit hard to call replacing a standard library function with
>> > inline assembly a "cosmetic improvement". :-)
>>
>> Agreed :P
>
> Personally, I think Chris's patch is an *aesthetic* improvement. It's
> superficially ugly, but contains inner beauty.
>
>> > > > I'm confused, if this doesn't improve performances, why reimplement
>> > > > drmIoctl() in Mesa? Or was the point that you made it `static inline`?
>> > > > Should your assembly hunk be applied in drmIoctl() in libdrm, maybe?
>> > >
>> > > Because the profile was spending more time in drmIoctl() + __GI_ioctl()
>> > > than
>> > > in the kernel, which annoyed me. drmIoctl() still has the impedance
>> > > mismatch, and is not used by anv.
>> >
>> >
>> > Getting rid of the impedance mismatch in error handling seems like the only
>> > good reason to do this.  I'll have to give it more thought before I have a
>> > solid opinion on whether errno or assembly is worse.  I do like pulling the
>> > anv_ioctl logic into some place common though.
>
> The other reason for removing all uses of drmIoctl() is, imo, it's
> excessive to jump through the PLT into a shared object for a mere ioctl
> wrapper.
>
>> >
>> > At some point, I'd like to move the special-case for I915_GEM_SET_TILING to
>> > the common helper so we can stop hand-rolling things just for that.
>>
>> So to be clear, there are two issues:
>> - profiler showing more time spent in userspace handling of the ioctl
>>   than the actual kernel call
>> - return value handling mismatch
>>
>> For the time issue, how about we move drmIoctl() directly in xf86drm.h
>> and make it `static inline` like your functions, or even a macro?
>>
>> For the return value issue, how about introducing a drmIoctl2() (with
>> a better name :P) with your assembly code and the -errno return value,
>> and maybe even the `EINTR || EAGAIN` loop integrated?
>>
>> drmIoctl2() would of course have the improvements from drmIoctl() :)
>>
>> This should give you all the improvements of your patches here, while
>> still using the standard libdrm functions.
>
> We should have less libdrm, not more, in the Intel drivers. I don't
> object to anyone adding drmIoctl2() to libdrm headers, but the Intel
> drivers shouldn't use it. i965 should be doing the same thing as anv
> regarding ioctls: implement the ioctl wrapper in-tree.
>
> Chris, if you rewrote this patch with the controversial part omitted
> [that is, without re-implementing libc's ioctl()], then I would give
> a r-b. That patch would be a definite improvement over the status quo.
> Then we could debate the merits of "libc's ioctl() vs raw syscall"
> after that patch landed.

Unless someone can get some numbers, this seems pointless.

Maintaining glibc in mesa isn't a direction we should be heading,

I'm not sure why avoiding drmIoctl is even a thing, there are plenty
of huge optimisation opportunities, this just seems like a feel good
pointless microptimisation.

Dave.