Re: [RFC PATCH 00/20] Initial Xe driver submission

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Feb 17, 2023 at 10:51 PM Daniel Vetter <daniel@xxxxxxxx> wrote:
>
> Hi all,
>
> [I thought I've sent this out earlier this week, but alas got stuck, kinda
> bad timing now since I'm out next week but oh well]
>
> So xe is a quite substantial thing, and I think we need a clear plan how to land
> this or it will take forever, and managers will panic. Also I'm not a big fan of
> "Dave/me reviews everything", we defacto had that for amd's dc/dal and it was
> not fun. The idea here is how to get everything reviewed without having two
> people end up somewhat arbitrary as deciders.
>
> I've compiled a bunch of topics on what I think the important areas are, first
> code that should be consistent about new-style render drivers that are aimed for
> vk/compute userspace as the primary feature driver:
>
> - figure out consensus solution for fw scheduler and drm/sched frontend among
>   interested driver parties (probably xe, amdgpu, nouveau, new panfrost)
>
> - for the interface itself it might be good to have the drm_gpu_scheduler as the
>   single per-hw-engine driver api object (but internally a new structure), while
>   renaming the current drm_gpu_scheduler to drm_gpu_sched_internal. That way I
>   think we can address the main critique of the current xe scheduler plan
>   - keep the drm_gpu_sched_internal : drm_sched_entity 1:1 relationship for fw
>     scheduler
>   - keep the driver api relationship of drm_gpu_scheduler : drm_sched_entity
>     1:n, the api functions simply iterate over a mutex protect list of internal
>     schedulers. this should also help drivers with locking mistakes around
>     setup/teardown and gpu reset.
>   - drivers select with a flag or something between the current mode (where the
>     drm_gpu_sched_internal is attached to the drm_gpu_scheduler api object) or
>     the new fw scheduler mode (where drm_gpu_sched_internal is attached to the
>     drm_sched_entity)
>   - overall still no fundamental changes (like the current patches) to drm/sched
>     data structures and algorithms. But unlike the current patches we keep the
>     possibility open for eventual refactoring without having to again refactor
>     all the drivers. Even better, we can delay such refactoring until we have a
>     handful of real-word drivers test-driving this all so we know we actually do
>     the right thing. This should allow us to address all the
>     fairness/efficiency/whatever concerns that have been floating around without
>     having to fix them all up upfront, before we actually know what needs to be
>     fixed.
>
> - the generic scheduler code should also including the handling of endless
>   compute contexts, with the minimal scaffolding for preempt-ctx fences
>   (probably on the drm_sched_entity) and making sure drm/sched can cope with the
>   lack of job completion fence. This is very minimal amounts of code, but it
>   helps a lot for cross-driver review if this works the same (with the same
>   locking and all that) for everyone. Ideally this gets extracted from amdkfd,
>   but as long as it's going to be used by all drivers supporting
>   endless/compute context going forward it's good enough.
>
> - I'm assuming this also means Matt Brost will include a patch to add himself as
>   drm/sched reviewer in MAINTAINERS, or at least something like that
>
> - adopt the gem_exec/vma helpers. again we probably want consensus here among
>   the same driver projects. I don't care whether these helpers specify the ioctl
>   structs or not, but they absolutely need to enforce the overall locking scheme
>   for all major structs and list (so vm and vma).
>
> - we also should have cross-driver consensus on async vm_bind support. I think
>   everyone added in-syncobj support, the real fun is probably more in/out
>   userspace memory fences (and personally I'm still not sure that's a good idea
>   but ... *eh*). I think cross driver consensus on how this should work (ideally
>   with helper support so people don't get it wrong in all the possible ways)
>   would be best.
>
> - this also means some userptr integration and some consensus how userptr should
>   work for vm_bind across drivers. I don't think allowing drivers to reinvent
>   that wheel is a bright idea, there's just a bit too much to get wrong here.
>
> - for some of these the consensus might land on more/less shared code than what
>   I sketched out above, the important part really is that we have consensus on
>   these. Kinda similar to how the atomic kms infrastructure move a _lot_ more of
>   the code back into drivers, because they really just needed the flexibility to
>   program the hw correctly. Right now we definitely don't have enough shared
>   code, for sure with i915-gem, but we also need to make sure we're not
>   overcorrecting too badly (a bit of overcorrecting generally doesn't hurt).
>
> All the above will make sure that the driver overall is in concepts and design
> aligned with the overall community direction, but I think it'd still be good if
> someone outside of the intel gpu group reviews the driver code itself. Last time
> we had a huge driver submission (amd's DC/DAL) this fell on Dave&me, but this
> time around I think we have a perfect candidate with Oded:
>
> - Oded needs/wants to spend some time on ramping up on how drm render drivers
>   work anyway, and xe is probably the best example of a driver that's both
>   supposed to be full-featured, but also doesn't contain an entire display
>   driver on the side.
>
> - Oded is in Habana, which is legally part of Intel. Bean counter budget
>   shuffling to make this happen should be possible.
>
> - Habana is still fairly distinct entity within Intel, so that is probably the
>   best approach for some independent review, without making the xe team
>   beholden to some non-Intel people.
Hi Daniel,
Thanks for suggesting it, I'll gladly do it.
I guess I'll have more feedback on the plan itself after I'll start
going over the current Xe driver code.

Oded

>
> The above should yield some pretty clear road towards landing xe, without any
> big review fights with Dave/me like we had with amd's DC/DAL, which took a
> rather long time to land unfortunately :-(
>
> These are just my thoughts, let the bikeshed commence!
>
> Ideally we put them all into a TODO like we've done for DC/DAL, once we have
> some consensus.
>
> Cheers, Daniel
>
> On Thu, Dec 22, 2022 at 02:21:07PM -0800, Matthew Brost wrote:
> > Hello,
> >
> > This is a submission for Xe, a new driver for Intel GPUs that supports both
> > integrated and discrete platforms starting with Tiger Lake (first platform with
> > Intel Xe Architecture). The intention of this new driver is to have a fresh base
> > to work from that is unencumbered by older platforms, whilst also taking the
> > opportunity to rearchitect our driver to increase sharing across the drm
> > subsystem, both leveraging and allowing us to contribute more towards other
> > shared components like TTM and drm/scheduler. The memory model is based on VM
> > bind which is similar to the i915 implementation. Likewise the execbuf
> > implementation for Xe is very similar to execbuf3 in the i915 [1].
> >
> > The code is at a stage where it is already functional and has experimental
> > support for multiple platforms starting from Tiger Lake, with initial support
> > implemented in Mesa (for Iris and Anv, our OpenGL and Vulkan drivers), as well
> > as in NEO (for OpenCL and Level0). A Mesa MR has been posted [2] and NEO
> > implementation will be released publicly early next year. We also have a suite
> > of IGTs for XE that will appear on the IGT list shortly.
> >
> > It has been built with the assumption of supporting multiple architectures from
> > the get-go, right now with tests running both on X86 and ARM hosts. And we
> > intend to continue working on it and improving on it as part of the kernel
> > community upstream.
> >
> > The new Xe driver leverages a lot from i915 and work on i915 continues as we
> > ready Xe for production throughout 2023.
> >
> > As for display, the intent is to share the display code with the i915 driver so
> > that there is maximum reuse there. Currently this is being done by compiling the
> > display code twice, but alternatives to that are under consideration and we want
> > to have more discussion on what the best final solution will look like over the
> > next few months. Right now, work is ongoing in refactoring the display codebase
> > to remove as much as possible any unnecessary dependencies on i915 specific data
> > structures there..
> >
> > We currently have 2 submission backends, execlists and GuC. The execlist is
> > meant mostly for testing and is not fully functional while GuC backend is fully
> > functional. As with the i915 and GuC submission, in Xe the GuC firmware is
> > required and should be placed in /lib/firmware/xe.
> >
> > The GuC firmware can be found in the below location:
> > https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/i915
> >
> > The easiest way to setup firmware is:
> > cp -r /lib/firmware/i915 /lib/firmware/xe
> >
> > The code has been organized such that we have all patches that touch areas
> > outside of drm/xe first for review, and then the actual new driver in a separate
> > commit. The code which is outside of drm/xe is included in this RFC while
> > drm/xe is not due to the size of the commit. The drm/xe is code is available in
> > a public repo listed below.
> >
> > Xe driver commit:
> > https://cgit.freedesktop.org/drm/drm-xe/commit/?h=drm-xe-next&id=9cb016ebbb6a275f57b1cb512b95d5a842391ad7
> >
> > Xe kernel repo:
> > https://cgit.freedesktop.org/drm/drm-xe/
> >
> > There's a lot of work still to happen on Xe but we're very excited about it and
> > wanted to share it early and welcome feedback and discussion.
> >
> > Cheers,
> > Matthew Brost
> >
> > [1] https://patchwork.freedesktop.org/series/105879/
> > [2] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20418
> >
> > Maarten Lankhorst (12):
> >   drm/amd: Convert amdgpu to use suballocation helper.
> >   drm/radeon: Use the drm suballocation manager implementation.
> >   drm/i915: Remove gem and overlay frontbuffer tracking
> >   drm/i915/display: Neuter frontbuffer tracking harder
> >   drm/i915/display: Add more macros to remove all direct calls to uncore
> >   drm/i915/display: Remove all uncore mmio accesses in favor of intel_de
> >   drm/i915: Rename find_section to find_bdb_section
> >   drm/i915/regs: Set DISPLAY_MMIO_BASE to 0 for xe
> >   drm/i915/display: Fix a use-after-free when intel_edp_init_connector
> >     fails
> >   drm/i915/display: Remaining changes to make xe compile
> >   sound/hda: Allow XE as i915 replacement for sound
> >   mei/hdcp: Also enable for XE
> >
> > Matthew Brost (5):
> >   drm/sched: Convert drm scheduler to use a work queue rather than
> >     kthread
> >   drm/sched: Add generic scheduler message interface
> >   drm/sched: Start run wq before TDR in drm_sched_start
> >   drm/sched: Submit job before starting TDR
> >   drm/sched: Add helper to set TDR timeout
> >
> > Thomas Hellström (3):
> >   drm/suballoc: Introduce a generic suballocation manager
> >   drm: Add a gpu page-table walker helper
> >   drm/ttm: Don't print error message if eviction was interrupted
> >
> >  drivers/gpu/drm/Kconfig                       |   5 +
> >  drivers/gpu/drm/Makefile                      |   4 +
> >  drivers/gpu/drm/amd/amdgpu/Kconfig            |   1 +
> >  drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  26 +-
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c   |  14 +-
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    |  12 +-
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c        |   5 +-
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h    |  23 +-
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h      |   3 +-
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_sa.c        | 320 +-----------------
> >  drivers/gpu/drm/drm_pt_walk.c                 | 159 +++++++++
> >  drivers/gpu/drm/drm_suballoc.c                | 301 ++++++++++++++++
> >  drivers/gpu/drm/i915/Makefile                 |   2 +-
> >  drivers/gpu/drm/i915/display/hsw_ips.c        |   7 +-
> >  drivers/gpu/drm/i915/display/i9xx_plane.c     |   1 +
> >  drivers/gpu/drm/i915/display/intel_atomic.c   |   2 +
> >  .../gpu/drm/i915/display/intel_atomic_plane.c |  25 +-
> >  .../gpu/drm/i915/display/intel_backlight.c    |   2 +-
> >  drivers/gpu/drm/i915/display/intel_bios.c     |  71 ++--
> >  drivers/gpu/drm/i915/display/intel_bw.c       |  36 +-
> >  drivers/gpu/drm/i915/display/intel_cdclk.c    |  68 ++--
> >  drivers/gpu/drm/i915/display/intel_color.c    |   1 +
> >  drivers/gpu/drm/i915/display/intel_crtc.c     |  14 +-
> >  drivers/gpu/drm/i915/display/intel_cursor.c   |  14 +-
> >  drivers/gpu/drm/i915/display/intel_de.h       |  38 +++
> >  drivers/gpu/drm/i915/display/intel_display.c  | 155 +++++++--
> >  drivers/gpu/drm/i915/display/intel_display.h  |   9 +-
> >  .../gpu/drm/i915/display/intel_display_core.h |   5 +-
> >  .../drm/i915/display/intel_display_debugfs.c  |   8 +
> >  .../drm/i915/display/intel_display_power.c    |  40 ++-
> >  .../drm/i915/display/intel_display_power.h    |   6 +
> >  .../i915/display/intel_display_power_map.c    |   7 +
> >  .../i915/display/intel_display_power_well.c   |  24 +-
> >  .../drm/i915/display/intel_display_reg_defs.h |   4 +
> >  .../drm/i915/display/intel_display_trace.h    |   6 +
> >  .../drm/i915/display/intel_display_types.h    |  32 +-
> >  drivers/gpu/drm/i915/display/intel_dmc.c      |  17 +-
> >  drivers/gpu/drm/i915/display/intel_dp.c       |  11 +-
> >  drivers/gpu/drm/i915/display/intel_dp_aux.c   |   6 +
> >  drivers/gpu/drm/i915/display/intel_dpio_phy.c |   9 +-
> >  drivers/gpu/drm/i915/display/intel_dpio_phy.h |  15 +
> >  drivers/gpu/drm/i915/display/intel_dpll.c     |   8 +-
> >  drivers/gpu/drm/i915/display/intel_dpll_mgr.c |   4 +
> >  drivers/gpu/drm/i915/display/intel_drrs.c     |   1 +
> >  drivers/gpu/drm/i915/display/intel_dsb.c      | 124 +++++--
> >  drivers/gpu/drm/i915/display/intel_dsi_vbt.c  |  26 +-
> >  drivers/gpu/drm/i915/display/intel_fb.c       | 108 ++++--
> >  drivers/gpu/drm/i915/display/intel_fb_pin.c   |   6 -
> >  drivers/gpu/drm/i915/display/intel_fbc.c      |  49 ++-
> >  drivers/gpu/drm/i915/display/intel_fbdev.c    | 108 +++++-
> >  .../gpu/drm/i915/display/intel_frontbuffer.c  | 103 +-----
> >  .../gpu/drm/i915/display/intel_frontbuffer.h  |  67 +---
> >  drivers/gpu/drm/i915/display/intel_gmbus.c    |   2 +-
> >  drivers/gpu/drm/i915/display/intel_hdcp.c     |   9 +-
> >  drivers/gpu/drm/i915/display/intel_hdmi.c     |   1 -
> >  .../gpu/drm/i915/display/intel_lpe_audio.h    |   8 +
> >  .../drm/i915/display/intel_modeset_setup.c    |  11 +-
> >  drivers/gpu/drm/i915/display/intel_opregion.c |   2 +-
> >  drivers/gpu/drm/i915/display/intel_overlay.c  |  14 -
> >  .../gpu/drm/i915/display/intel_pch_display.h  |  16 +
> >  .../gpu/drm/i915/display/intel_pch_refclk.h   |   8 +
> >  drivers/gpu/drm/i915/display/intel_pipe_crc.c |   1 +
> >  .../drm/i915/display/intel_plane_initial.c    |   3 +-
> >  drivers/gpu/drm/i915/display/intel_psr.c      |   1 +
> >  drivers/gpu/drm/i915/display/intel_sprite.c   |  21 ++
> >  drivers/gpu/drm/i915/display/intel_vbt_defs.h |   2 +-
> >  drivers/gpu/drm/i915/display/intel_vga.c      |   5 +
> >  drivers/gpu/drm/i915/display/skl_scaler.c     |   2 +
> >  .../drm/i915/display/skl_universal_plane.c    |  52 ++-
> >  drivers/gpu/drm/i915/display/skl_watermark.c  |  25 +-
> >  drivers/gpu/drm/i915/gem/i915_gem_clflush.c   |   4 -
> >  drivers/gpu/drm/i915/gem/i915_gem_domain.c    |   7 -
> >  .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |   2 -
> >  drivers/gpu/drm/i915/gem/i915_gem_object.c    |  25 --
> >  drivers/gpu/drm/i915/gem/i915_gem_object.h    |  22 --
> >  drivers/gpu/drm/i915/gem/i915_gem_phys.c      |   4 -
> >  drivers/gpu/drm/i915/gt/intel_gt_regs.h       |   3 +-
> >  drivers/gpu/drm/i915/i915_driver.c            |   1 +
> >  drivers/gpu/drm/i915/i915_gem.c               |   8 -
> >  drivers/gpu/drm/i915/i915_gem_gtt.c           |   1 -
> >  drivers/gpu/drm/i915/i915_reg_defs.h          |   8 +
> >  drivers/gpu/drm/i915/i915_vma.c               |  12 -
> >  drivers/gpu/drm/radeon/radeon.h               |  55 +--
> >  drivers/gpu/drm/radeon/radeon_ib.c            |  12 +-
> >  drivers/gpu/drm/radeon/radeon_object.h        |  25 +-
> >  drivers/gpu/drm/radeon/radeon_sa.c            | 314 ++---------------
> >  drivers/gpu/drm/radeon/radeon_semaphore.c     |   6 +-
> >  drivers/gpu/drm/scheduler/sched_main.c        | 182 +++++++---
> >  drivers/gpu/drm/ttm/ttm_bo.c                  |   3 +-
> >  drivers/misc/mei/hdcp/Kconfig                 |   2 +-
> >  drivers/misc/mei/hdcp/mei_hdcp.c              |   3 +-
> >  include/drm/drm_pt_walk.h                     | 161 +++++++++
> >  include/drm/drm_suballoc.h                    | 112 ++++++
> >  include/drm/gpu_scheduler.h                   |  41 ++-
> >  sound/hda/hdac_i915.c                         |  17 +-
> >  sound/pci/hda/hda_intel.c                     |  56 +--
> >  sound/soc/intel/avs/core.c                    |  13 +-
> >  sound/soc/sof/intel/hda.c                     |   7 +-
> >  98 files changed, 2076 insertions(+), 1325 deletions(-)
> >  create mode 100644 drivers/gpu/drm/drm_pt_walk.c
> >  create mode 100644 drivers/gpu/drm/drm_suballoc.c
> >  create mode 100644 include/drm/drm_pt_walk.h
> >  create mode 100644 include/drm/drm_suballoc.h
> >
> > --
> > 2.37.3
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux