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

[Chad Versace]

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.