[Intel-gfx] [PATCH 1/2] drm/i915: add gem/gt TODO

[Rodrigo Vivi]

On Tue, Mar 23, 2021 at 12:57:39PM +0100, Daniel Vetter wrote:
> On Tue, Mar 23, 2021 at 12:13:11PM +0200, Jani Nikula wrote:
> > On Tue, 23 Mar 2021, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> > > We've discussed a bit how to get the gem/gt team better integrated
> > > and collaborate more with the wider community and agreed to the
> > > following:
> > >
> > > - all gem/gt patches are reviewed on dri-devel for now. That's
> > >   overkill, but in the past there was definitely too little of that.
> > >
> > > - i915-gem folks are encouraged to cross review core patches from
> > >   other teams
> > >
> > > - big features (especially uapi changes) need to be discussed in an
> > >   rfc patch that documents the interface and big picture design,
> > >   before we get lost in the details of the code
> > >
> > > - Also a rough TODO (can be refined as we go ofc) to get gem/gt back
> > >   on track, like we've e.g. done with DAL/DC to get that in shape.
> > 
> > I personally think there should be a lower bar for discussing and
> > editing the TODO items than via patches on the mailing list. Granted,
> > the TODO file enforces the discussion happens at a large enough
> > audience, but for at least some of the items I'd suggest filing gitlab
> > issues [1], with todo label, and tracking there.

I also don't like the todo list in files and I agree that gitlab issues
section should be better...

> In general yes, and I'd go even further: it's up to each team/contributor
> how they track review feedback and further work, whether that's gitlab or
> some notes or just in their heads.
> 
> This is a different situation here, and the "changes require big audience"
> is a feature, not a bug. But it is a very exceptional situation, I think
> this is only the 2nd time we're using a formal TODO for a gpu driver. If
> we ignore gma500 in staging, which for me only showed that the separate
> staging tree doesn't work so well for complex drivers like we have.

... but I understand the motivation, so

Acked-by: Rodrigo Vivi <rodrigo.vivi at intel.com>

However... what about:

1. moving the smaller items to gitlab at least?
2. having both, all the entries in the todo file have gitlab issue
associated and the number-id is also here in the todo file?

> -Daniel
> 
> > 
> > BR,
> > Jani.
> > 
> > 
> > [1] https://gitlab.freedesktop.org/drm/intel/-/issues
> > 
> > 
> > 
> > >
> > > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > Cc: Jason Ekstrand <jason at jlekstrand.net>
> > > Cc: Dave Airlie <airlied at redhat.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/TODO.txt | 36 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 36 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/i915/TODO.txt
> > >
> > > diff --git a/drivers/gpu/drm/i915/TODO.txt b/drivers/gpu/drm/i915/TODO.txt
> > > new file mode 100644
> > > index 000000000000..d2e5bbb6339d
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/TODO.txt
> > > @@ -0,0 +1,36 @@
> > > +gem/gt TODO items
> > > +-----------------
> > > +
> > > +- For discrete memory manager, merge enough dg1 to be able to refactor it to
> > > +  TTM. Then land pci ids (just in case that turns up an uapi problem). TTM has
> > > +  improved a lot the past 2 years, there's no reason anymore not to use it.
> > > +
> > > +- Come up with a plan what to do with drm/scheduler and how to get there.
> > > +
> > > +- There's a lot of complexity added past few years to make relocations faster.
> > > +  That doesn't make sense given hw and gpu apis moved away from this model years
> > > +  ago:
> > > +  1. Land a modern pre-bound uapi like VM_BIND
> > > +  2. Any complexity added in this area past few years which can't be justified
> > > +  with VM_BIND using userspace should be removed. Looking at amdgpu dma_resv on
> > > +  the bo and vm, plus some lru locks is all that needed. No complex rcu,
> > > +  refcounts, caching, ... on everything.
> > > +  This is the matching task on the vm side compared to ttm/dma_resv on the
> > > +  backing storage side.
> > > +
> > > +- i915_sw_fence seems to be the main structure for the i915-gem dma_fence model.
> > > +  How-to-dma_fence is core and drivers really shouldn't build their own world
> > > +  here, treating everything else as a fixed platform. i915_sw_fence concepts
> > > +  should be moved to dma_fence, drm/scheduler or atomic commit helpers. Or
> > > +  removed if dri-devel consensus is that it's not a good idea. Once that's done
> > > +  maybe even remove it if there's nothing left.
> > > +
> > > +Smaller things:
> > > +- i915_utils.h needs to be moved to the right places.
> > > +
> > > +- dma_fence_work should be in drivers/dma-buf
> > > +
> > > +- i915_mm.c should be moved to the right places. Some of the helpers also look a
> > > +  bit fishy:
> > > +
> > > +  https://lore.kernel.org/linux-mm/20210301083320.943079-1-hch@lst.de/
> > 
> > -- 
> > Jani Nikula, Intel Open Source Graphics Center
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch