[Mesa-dev] [PATCH 00/53] i965: Eat libdrm_intel for breakfast

[Alex Deucher]

On Wed, Apr 5, 2017 at 2:27 PM, Kristian Høgsberg <hoegsberg at gmail.com> wrote:
> On Wed, Apr 5, 2017 at 11:11 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>> On Wed, Apr 5, 2017 at 11:03 AM, Emil Velikov <emil.l.velikov at gmail.com>
>> wrote:
>>>
>>> On 5 April 2017 at 18:55, Daniel Vetter <daniel at ffwll.ch> wrote:
>>> > On Wed, Apr 05, 2017 at 04:38:25PM +0100, Emil Velikov wrote:
>>> >> Hi Ken,
>>> >>
>>> >> On 5 April 2017 at 01:09, Kenneth Graunke <kenneth at whitecape.org>
>>> >> wrote:
>>> >> > Hello,
>>> >> >
>>> >> > This series imports libdrm_intel into the i965 driver, hacks and
>>> >> > slashes it down to size, and greatly simplifies our relocation
>>> >> > handling.
>>> >> >
>>> >> > Some of the patches may be held for moderation.  You can find the
>>> >> > series in git here:
>>> >> >
>>> >> > https://cgit.freedesktop.org/~kwg/mesa/log/?h=bacondrm
>>> >> >
>>> >> > A couple of us have been talking about this in person and IRC for
>>> >> > a while, but I realize I haven't mentioned anything about it on the
>>> >> > mailing list yet, so this may come as a bit of a surprise.
>>> >> >
>>> >> > libdrm_intel is about 15 source files and almost 13,000 lines of
>>> >> > code.
>>> >> > This series adds 3 files (one .c, two .h) and only 2,137 lines of
>>> >> > code:
>>> >> >
>>> >> >     60 files changed, 2784 insertions(+), 647 deletions(-)
>>> >> >
>>> >> > The rest of the library is basically useless to us.  It contains a
>>> >> > lot
>>> >> > of legacy cruft from the pre-GEM, DRI1, or 8xx/9xx era.  But even the
>>> >> > parts we do use are in bad shape.  BO offset tracking is
>>> >> > non-threadsafe.
>>> >> > Relocation handling is way too complicated.  These things waste
>>> >> > memory,
>>> >> > burn CPU time, and make it difficult for us to take advantage of new
>>> >> > kernel features like I915_EXEC_NO_RELOC which would reduce overhead
>>> >> > further.  The unsynchronized mapping API performs a synchronized
>>> >> > mapping
>>> >> > on non-LLC platforms, which can massively hurt performance on Atoms.
>>> >> > Mesa is also using uncached GTT mappings for almost everything on
>>> >> > Atoms,
>>> >> > rather than fast CPU or WC maps where possible.
>>> >> >
>>> >> > Evolving this code in libdrm is very painful, as we aren't allowed to
>>> >> > break the ABI.  All the legacy cruft and design mistakes (in
>>> >> > hindsight)
>>> >> > make it difficult to follow what's going on.  We could keep piling
>>> >> > new
>>> >> > layers on top, but that only makes it worse.  Furthermore, there's a
>>> >> > bunch of complexity that comes from defending against or supporting
>>> >> > broken or badly designed callers.
>>> >> >
>>> >> I believe I mentioned it a few days ago - there is no need to worry
>>> >> about API or ABI stability.
>>> >>
>>> >> Need new API - add it. Things getting fragile or too many layers - sed
>>> >> /libdrm_intel$(N)/libdrm_intel$(N+1)/ and rework as needed.
>>> >>
>>> >> I fear that Importing libdrm_intel will be detrimental to libva's
>>> >> intel-driver, Beignet and xf86-video-intel
>>
>>
>> I wouldn't worry about xf86-video-intel.  Chris has already copy+pasted half
>> of the X server, what's libdrm? :-)
>>
>> The others, yeah, they could possibly benefit from drm_intel3.  That said, I
>> think you significantly over-estimate how much a driver actually gets from
>> libdrm.  We chose to not use libdrm in Vulkan and it really hasn't caused us
>> all that much pain.
>>
>>>
>>> >> development.
>>> >> Those teams seem to be more resource contained than Mesa, thus they
>>> >> will trail behind even more.
>>> >>
>>> >> As an example - the intel-driver is missing some trivial winsys
>>> >> optimisations that landed in Mesa 3+ years ago. That could have been
>>> >> avoided if the helpers were shared with the help of
>>> >> libdrm_intel/other.
>>
>>
>> libdrm should *never* touch winsys.  Please, no.
>>
>>>
>>> >
>>> > That is kinda the longer-term goal with this. There's a lot more that
>>> > needs to be done besides Ken's series here, this is just the first step,
>>> > but in the end we'll probably move brw_batch back into libdrm_intel2 or
>>> > so, for consumption by beignet and libva.
>>> >
>>> > But for rewriting the world and getting rid of 10+ years of compat
>>> > garbage, having a split between libdrm and mesa isn't great.
>>> >
>>> So the goal is to have the code in mesa as a form of incubator until
>>> it reaches maturity.
>>> This way one will have a more rapid development and greater
>>> flexibility during that stage.
>>
>>
>> Yes, I think we'd eventually like to have some shared code again.  However,
>> at the moment, that code sharing is costing us dearly and it's time for a
>> step back and a complete re-evaluation of how we do things.  Once we've
>> settled on something we like then maybe we can consider sharing again.
>> Ideally, I'd like the Vulkan driver to be able to share at least some bits
>> with i965.  At the moment, however, we don't know what the new API should
>> look like or what bets we even want to share, so pulling the whole thing in
>> is the right next step.
>
> The concern about code quality of libva is valid, but there's much
> more code inside mesa that could be shared with libva to increase code
> quality than there is in libdrm. libva could benefit greatly from
> using nir and the compiler, libisl, blorp and the genxml structs.
> libdrm really is the least of your concerns. A better direction for
> this could be for libva to move into mesa.

FWIW, if it weren't for the need to support non-Mesa UMDs like OpenCL
and Vulkan, we probably would have just stuck with mesa as the hub of
our acceleration UMDs and just stuck with the winsys there.  That
said, having libdrm_amdgpu has made it much easier to write simple
test applications and develop a unit test suite.

Alex