[1/2] drm/i915: add gem/gt TODO

Commit Message

Daniel Vetter March 23, 2021, 8:44 a.m. UTC

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.

Cc: Jani Nikula <[e-mail]>
Cc: Joonas Lahtinen <[e-mail]>
Cc: Rodrigo Vivi <[e-mail]>
Cc: Jason Ekstrand <[e-mail]>
Cc: Dave Airlie <[e-mail]>
Signed-off-by: Daniel Vetter <[e-mail]>
---
 drivers/gpu/drm/i915/TODO.txt | 36 +++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/TODO.txt

Comments

Jani Nikula March 23, 2021, 10:13 a.m. UTC | #1

On Tue, 23 Mar 2021, Daniel Vetter <[e-mail]> 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.

BR,
Jani.


[1] https://gitlab.freedesktop.org/drm/intel/-/issues



>
> Cc: Jani Nikula <[e-mail]>
> Cc: Joonas Lahtinen <[e-mail]>
> Cc: Rodrigo Vivi <[e-mail]>
> Cc: Jason Ekstrand <[e-mail]>
> Cc: Dave Airlie <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>
> ---
>  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/[e-mail]/

Daniel Vetter March 23, 2021, 11:57 a.m. UTC | #2

On Tue, Mar 23, 2021 at 12:13:11PM +0200, Jani Nikula wrote:
> On Tue, 23 Mar 2021, Daniel Vetter <[e-mail]> 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.

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.
-Daniel

> 
> BR,
> Jani.
> 
> 
> [1] https://gitlab.freedesktop.org/drm/intel/-/issues
> 
> 
> 
> >
> > Cc: Jani Nikula <[e-mail]>
> > Cc: Joonas Lahtinen <[e-mail]>
> > Cc: Rodrigo Vivi <[e-mail]>
> > Cc: Jason Ekstrand <[e-mail]>
> > Cc: Dave Airlie <[e-mail]>
> > Signed-off-by: Daniel Vetter <[e-mail]>
> > ---
> >  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/[e-mail]/
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

Rodrigo Vivi March 23, 2021, 12:34 p.m. UTC | #3

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 <[e-mail]> 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 <[e-mail]>

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 <[e-mail]>
> > > Cc: Joonas Lahtinen <[e-mail]>
> > > Cc: Rodrigo Vivi <[e-mail]>
> > > Cc: Jason Ekstrand <[e-mail]>
> > > Cc: Dave Airlie <[e-mail]>
> > > Signed-off-by: Daniel Vetter <[e-mail]>
> > > ---
> > >  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/[e-mail]/
> > 
> > -- 
> > Jani Nikula, Intel Open Source Graphics Center
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Daniel Vetter March 23, 2021, 1:17 p.m. UTC | #4

On Tue, Mar 23, 2021 at 09:44:52AM +0100, Daniel Vetter 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.
> 
> Cc: Jani Nikula <[e-mail]>
> Cc: Joonas Lahtinen <[e-mail]>
> Cc: Rodrigo Vivi <[e-mail]>
> Cc: Jason Ekstrand <[e-mail]>
> Cc: Dave Airlie <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>
> ---
>  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/[e-mail]/

Jani just pointed me at another small helper that we should look at:

- tasklet helpers in i915_gem.h also look a bit misplaced and should
  probably be moved to tasklet headers.

Spotted through https://lore.kernel.org/lkml/20210323092221.awq7g5b2muzypjw3@flow/

I'll add that one in v2.

There is also the i915 ww locking context helpers from Maarten in there,
but right now those truly are i915 specific. Plus as part of the ttm
conversion they're already on the list of things we have to move. So I
think redundant to add the entire file.
-Daniel

Daniel Vetter March 23, 2021, 1:18 p.m. UTC | #5

On Tue, Mar 23, 2021 at 08:34:55AM -0400, Rodrigo Vivi wrote:
> 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 <[e-mail]> 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 <[e-mail]>
> 
> 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?

Yeah that sounds reasonable. tbh we haven't started any of the
intel-internal planning on most of these (ttm and scheduler are started),
so none of these tracking things exist yet at all ...
-Daniel

> 
> > -Daniel
> > 
> > > 
> > > BR,
> > > Jani.
> > > 
> > > 
> > > [1] https://gitlab.freedesktop.org/drm/intel/-/issues
> > > 
> > > 
> > > 
> > > >
> > > > Cc: Jani Nikula <[e-mail]>
> > > > Cc: Joonas Lahtinen <[e-mail]>
> > > > Cc: Rodrigo Vivi <[e-mail]>
> > > > Cc: Jason Ekstrand <[e-mail]>
> > > > Cc: Dave Airlie <[e-mail]>
> > > > Signed-off-by: Daniel Vetter <[e-mail]>
> > > > ---
> > > >  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/[e-mail]/
> > > 
> > > -- 
> > > Jani Nikula, Intel Open Source Graphics Center
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

Jason Ekstrand March 23, 2021, 6:07 p.m. UTC | #6

On Tue, Mar 23, 2021 at 8:18 AM Daniel Vetter <[e-mail]> wrote:
>
> On Tue, Mar 23, 2021 at 08:34:55AM -0400, Rodrigo Vivi wrote:
> > 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 <[e-mail]> 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 <[e-mail]>
> >
> > 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?
>
> Yeah that sounds reasonable. tbh we haven't started any of the
> intel-internal planning on most of these (ttm and scheduler are started),
> so none of these tracking things exist yet at all ...

I'm a fan of this.  GitLab issues provide a good place to organize the
chatter on any particular ToDo item.  I'd also rather see people
chattering about this stuff on public GitLab than JIRA, when possible.
The last patch in the series closing out a ToDo can be a patch to this
file to remove the bullet point.

--Jason

> -Daniel
>
> >
> > > -Daniel
> > >
> > > >
> > > > BR,
> > > > Jani.
> > > >
> > > >
> > > > [1] https://gitlab.freedesktop.org/drm/intel/-/issues
> > > >
> > > >
> > > >
> > > > >
> > > > > Cc: Jani Nikula <[e-mail]>
> > > > > Cc: Joonas Lahtinen <[e-mail]>
> > > > > Cc: Rodrigo Vivi <[e-mail]>
> > > > > Cc: Jason Ekstrand <[e-mail]>
> > > > > Cc: Dave Airlie <[e-mail]>
> > > > > Signed-off-by: Daniel Vetter <[e-mail]>
> > > > > ---
> > > > >  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/[e-mail]/
> > > >
> > > > --
> > > > Jani Nikula, Intel Open Source Graphics Center
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Dave Airlie March 24, 2021, 6:34 a.m. UTC | #7

On Tue, 23 Mar 2021 at 23:17, Daniel Vetter <[e-mail]> wrote:
>
> On Tue, Mar 23, 2021 at 09:44:52AM +0100, Daniel Vetter 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 think we mentioned in the past about having better annotations for
dma_fence critical sections,

Otherwise I think this is a great list to get us out of the woods and
seeing how to move forward again.

Dave.

Patch

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/[e-mail]/