Nouveau - Sep 2019 - [PATCH 3/6] drm/nouveau: Remove bogus gk20a aperture callback

On Tue, 17 Sep 2019 at 01:18, Thierry Reding <thierry.reding at gmail.com>
wrote:
>
> From: Thierry Reding <treding at nvidia.com>
>
> The gk20a (as well as all subsequent Tegra instantiations of the GPU) do
> in fact use the same apertures as regular GPUs. Prior to gv11b there are
> no checks in hardware for the aperture, so we get away with setting VRAM
> as the aperture for buffers that are actually in system memory.
Can GK20A take comptags with aperture set to system memory?  For some
reason I can recall, I was under the impression PTEs needed to be
pointed at "vidmem" (despite them actually accessing system memory
anyway) on Tegra parts for compression to work?  I could be mistaken
though.

Ben.
>
> Signed-off-by: Thierry Reding <treding at nvidia.com>
> ---
>  drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h      |  1 -
>  drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c | 10 ----------
>  drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c |  4 ++--
>  drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c |  2 +-
>  4 files changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h
b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h
> index fb3a9e8bb9cd..9862f44ac8b5 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h
> @@ -212,7 +212,6 @@ void gf100_vmm_flush(struct nvkm_vmm *, int);
>  void gf100_vmm_invalidate(struct nvkm_vmm *, u32 type);
>  void gf100_vmm_invalidate_pdb(struct nvkm_vmm *, u64 addr);
>
> -int gk20a_vmm_aper(enum nvkm_memory_target);
>  int gk20a_vmm_valid(struct nvkm_vmm *, void *, u32, struct nvkm_vmm_map
*);
>
>  int gm200_vmm_new_(const struct nvkm_vmm_func *, const struct
nvkm_vmm_func *,
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c
> index 16d7bf727292..999b953505b3 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c
> @@ -25,16 +25,6 @@
>
>  #include <core/memory.h>
>
> -int
> -gk20a_vmm_aper(enum nvkm_memory_target target)
> -{
> -       switch (target) {
> -       case NVKM_MEM_TARGET_NCOH: return 0;
> -       default:
> -               return -EINVAL;
> -       }
> -}
> -
>  int
>  gk20a_vmm_valid(struct nvkm_vmm *vmm, void *argv, u32 argc,
>                 struct nvkm_vmm_map *map)
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c
> index 7a6066d886cd..f5d7819c4a40 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c
> @@ -25,7 +25,7 @@ static const struct nvkm_vmm_func
>  gm20b_vmm_17 = {
>         .join = gm200_vmm_join,
>         .part = gf100_vmm_part,
> -       .aper = gk20a_vmm_aper,
> +       .aper = gf100_vmm_aper,
>         .valid = gk20a_vmm_valid,
>         .flush = gf100_vmm_flush,
>         .invalidate_pdb = gf100_vmm_invalidate_pdb,
> @@ -41,7 +41,7 @@ static const struct nvkm_vmm_func
>  gm20b_vmm_16 = {
>         .join = gm200_vmm_join,
>         .part = gf100_vmm_part,
> -       .aper = gk20a_vmm_aper,
> +       .aper = gf100_vmm_aper,
>         .valid = gk20a_vmm_valid,
>         .flush = gf100_vmm_flush,
>         .invalidate_pdb = gf100_vmm_invalidate_pdb,
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c
> index 180c8f006e32..ffe84ea2f7d9 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c
> @@ -43,7 +43,7 @@ static const struct nvkm_vmm_func
>  gp10b_vmm = {
>         .join = gp100_vmm_join,
>         .part = gf100_vmm_part,
> -       .aper = gk20a_vmm_aper,
> +       .aper = gf100_vmm_aper,
>         .valid = gp10b_vmm_valid,
>         .flush = gp100_vmm_flush,
>         .mthd = gp100_vmm_mthd,
> --
> 2.23.0
>
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

On Tue, Sep 17, 2019 at 01:43:13PM +1000, Ben Skeggs
wrote:
> On Tue, 17 Sep 2019 at 01:18, Thierry Reding <thierry.reding at
gmail.com> wrote:
> >
> > From: Thierry Reding <treding at nvidia.com>
> >
> > The gk20a (as well as all subsequent Tegra instantiations of the GPU)
do
> > in fact use the same apertures as regular GPUs. Prior to gv11b there
are
> > no checks in hardware for the aperture, so we get away with setting
VRAM
> > as the aperture for buffers that are actually in system memory.
> Can GK20A take comptags with aperture set to system memory?  For some
> reason I can recall, I was under the impression PTEs needed to be
> pointed at "vidmem" (despite them actually accessing system
memory
> anyway) on Tegra parts for compression to work?  I could be mistaken
> though.
I don't think GK20A supports comptags at all. I think this wasn't
introduced until GM20B. nvgpu has some "gk20a" code to flush comptags,
but that's only used on GM20B and later.

Anyway, my understanding is that on all of GK20A, GM20B and GP10B the
aperture field is completely ignored. I think that also goes for
comptags. nvgpu in particular never requests comptags to be allocated
from vidmem. See:

	https://nv-tegra.nvidia.com/gitweb/?p=linux-nvgpu.git;a=blob;f=drivers/gpu/nvgpu/os/linux/ltc.c;h=baeb20b2e539cc6cb70667ce168603546678dc73;hb=2081ce686bfd4deb461b4130df424d592000ff88#l30

There are two callers of that, both passing "false" for the
vidmem_alloc
parameter, so comptags always do end up in system memory for Tegra.

That said, I'll go confirm that with one of our experts and get back to
you.

Thierry
> 
> Ben.
> 
> >
> > Signed-off-by: Thierry Reding <treding at nvidia.com>
> > ---
> >  drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h      |  1 -
> >  drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c | 10 ----------
> >  drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c |  4 ++--
> >  drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c |  2 +-
> >  4 files changed, 3 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h
b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h
> > index fb3a9e8bb9cd..9862f44ac8b5 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h
> > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h
> > @@ -212,7 +212,6 @@ void gf100_vmm_flush(struct nvkm_vmm *, int);
> >  void gf100_vmm_invalidate(struct nvkm_vmm *, u32 type);
> >  void gf100_vmm_invalidate_pdb(struct nvkm_vmm *, u64 addr);
> >
> > -int gk20a_vmm_aper(enum nvkm_memory_target);
> >  int gk20a_vmm_valid(struct nvkm_vmm *, void *, u32, struct
nvkm_vmm_map *);
> >
> >  int gm200_vmm_new_(const struct nvkm_vmm_func *, const struct
nvkm_vmm_func *,
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c
> > index 16d7bf727292..999b953505b3 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c
> > @@ -25,16 +25,6 @@
> >
> >  #include <core/memory.h>
> >
> > -int
> > -gk20a_vmm_aper(enum nvkm_memory_target target)
> > -{
> > -       switch (target) {
> > -       case NVKM_MEM_TARGET_NCOH: return 0;
> > -       default:
> > -               return -EINVAL;
> > -       }
> > -}
> > -
> >  int
> >  gk20a_vmm_valid(struct nvkm_vmm *vmm, void *argv, u32 argc,
> >                 struct nvkm_vmm_map *map)
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c
> > index 7a6066d886cd..f5d7819c4a40 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c
> > @@ -25,7 +25,7 @@ static const struct nvkm_vmm_func
> >  gm20b_vmm_17 = {
> >         .join = gm200_vmm_join,
> >         .part = gf100_vmm_part,
> > -       .aper = gk20a_vmm_aper,
> > +       .aper = gf100_vmm_aper,
> >         .valid = gk20a_vmm_valid,
> >         .flush = gf100_vmm_flush,
> >         .invalidate_pdb = gf100_vmm_invalidate_pdb,
> > @@ -41,7 +41,7 @@ static const struct nvkm_vmm_func
> >  gm20b_vmm_16 = {
> >         .join = gm200_vmm_join,
> >         .part = gf100_vmm_part,
> > -       .aper = gk20a_vmm_aper,
> > +       .aper = gf100_vmm_aper,
> >         .valid = gk20a_vmm_valid,
> >         .flush = gf100_vmm_flush,
> >         .invalidate_pdb = gf100_vmm_invalidate_pdb,
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c
> > index 180c8f006e32..ffe84ea2f7d9 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c
> > @@ -43,7 +43,7 @@ static const struct nvkm_vmm_func
> >  gp10b_vmm = {
> >         .join = gp100_vmm_join,
> >         .part = gf100_vmm_part,
> > -       .aper = gk20a_vmm_aper,
> > +       .aper = gf100_vmm_aper,
> >         .valid = gp10b_vmm_valid,
> >         .flush = gp100_vmm_flush,
> >         .mthd = gp100_vmm_mthd,
> > --
> > 2.23.0
> >
> > _______________________________________________
> > Nouveau mailing list
> > Nouveau at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/nouveau
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20190917/12b6051d/attachment-0001.sig>

On Tue, Sep 17, 2019 at 11:02:54AM +0200, Thierry Reding
wrote:
> On Tue, Sep 17, 2019 at 01:43:13PM +1000, Ben Skeggs wrote:
> > On Tue, 17 Sep 2019 at 01:18, Thierry Reding <thierry.reding at
gmail.com> wrote:
> > >
> > > From: Thierry Reding <treding at nvidia.com>
> > >
> > > The gk20a (as well as all subsequent Tegra instantiations of the
GPU) do
> > > in fact use the same apertures as regular GPUs. Prior to gv11b
there are
> > > no checks in hardware for the aperture, so we get away with
setting VRAM
> > > as the aperture for buffers that are actually in system memory.
> > Can GK20A take comptags with aperture set to system memory?  For some
> > reason I can recall, I was under the impression PTEs needed to be
> > pointed at "vidmem" (despite them actually accessing system
memory
> > anyway) on Tegra parts for compression to work?  I could be mistaken
> > though.
> 
> I don't think GK20A supports comptags at all. I think this wasn't
> introduced until GM20B. nvgpu has some "gk20a" code to flush
comptags,
> but that's only used on GM20B and later.
> 
> Anyway, my understanding is that on all of GK20A, GM20B and GP10B the
> aperture field is completely ignored. I think that also goes for
> comptags. nvgpu in particular never requests comptags to be allocated
> from vidmem. See:
> 
> 
https://nv-tegra.nvidia.com/gitweb/?p=linux-nvgpu.git;a=blob;f=drivers/gpu/nvgpu/os/linux/ltc.c;h=baeb20b2e539cc6cb70667ce168603546678dc73;hb=2081ce686bfd4deb461b4130df424d592000ff88#l30
> 
> There are two callers of that, both passing "false" for the
vidmem_alloc
> parameter, so comptags always do end up in system memory for Tegra.
> 
> That said, I'll go confirm that with one of our experts and get back to
> you.
So it looks like you're right and indeed GK20A and later (up to, but not
including, GV11B that is) comptags do indeed need to be mapped as vidmem
even if they reside in sysmem.

Conceptually I think what we want to do is decide about the aperture at
allocation time. So if we allocate comptags we would need to force the
aperture to be VRAM on the iGPUs where necessary, even if they are
really allocated in system memory. Ultimately the end result is the same
of course, but I think this way around is a better representation of
this particular hardware quirk and allows us to keep the unification
that this patch series achieves.

But I'll have to look into this and see what I can come up with.

Thierry
> 
> Thierry
> 
> > 
> > Ben.
> > 
> > >
> > > Signed-off-by: Thierry Reding <treding at nvidia.com>
> > > ---
> > >  drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h      |  1 -
> > >  drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c | 10
----------
> > >  drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c |  4 ++--
> > >  drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c |  2 +-
> > >  4 files changed, 3 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h
b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h
> > > index fb3a9e8bb9cd..9862f44ac8b5 100644
> > > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h
> > > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h
> > > @@ -212,7 +212,6 @@ void gf100_vmm_flush(struct nvkm_vmm *, int);
> > >  void gf100_vmm_invalidate(struct nvkm_vmm *, u32 type);
> > >  void gf100_vmm_invalidate_pdb(struct nvkm_vmm *, u64 addr);
> > >
> > > -int gk20a_vmm_aper(enum nvkm_memory_target);
> > >  int gk20a_vmm_valid(struct nvkm_vmm *, void *, u32, struct
nvkm_vmm_map *);
> > >
> > >  int gm200_vmm_new_(const struct nvkm_vmm_func *, const struct
nvkm_vmm_func *,
> > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c
> > > index 16d7bf727292..999b953505b3 100644
> > > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c
> > > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c
> > > @@ -25,16 +25,6 @@
> > >
> > >  #include <core/memory.h>
> > >
> > > -int
> > > -gk20a_vmm_aper(enum nvkm_memory_target target)
> > > -{
> > > -       switch (target) {
> > > -       case NVKM_MEM_TARGET_NCOH: return 0;
> > > -       default:
> > > -               return -EINVAL;
> > > -       }
> > > -}
> > > -
> > >  int
> > >  gk20a_vmm_valid(struct nvkm_vmm *vmm, void *argv, u32 argc,
> > >                 struct nvkm_vmm_map *map)
> > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c
> > > index 7a6066d886cd..f5d7819c4a40 100644
> > > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c
> > > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c
> > > @@ -25,7 +25,7 @@ static const struct nvkm_vmm_func
> > >  gm20b_vmm_17 = {
> > >         .join = gm200_vmm_join,
> > >         .part = gf100_vmm_part,
> > > -       .aper = gk20a_vmm_aper,
> > > +       .aper = gf100_vmm_aper,
> > >         .valid = gk20a_vmm_valid,
> > >         .flush = gf100_vmm_flush,
> > >         .invalidate_pdb = gf100_vmm_invalidate_pdb,
> > > @@ -41,7 +41,7 @@ static const struct nvkm_vmm_func
> > >  gm20b_vmm_16 = {
> > >         .join = gm200_vmm_join,
> > >         .part = gf100_vmm_part,
> > > -       .aper = gk20a_vmm_aper,
> > > +       .aper = gf100_vmm_aper,
> > >         .valid = gk20a_vmm_valid,
> > >         .flush = gf100_vmm_flush,
> > >         .invalidate_pdb = gf100_vmm_invalidate_pdb,
> > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c
> > > index 180c8f006e32..ffe84ea2f7d9 100644
> > > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c
> > > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c
> > > @@ -43,7 +43,7 @@ static const struct nvkm_vmm_func
> > >  gp10b_vmm = {
> > >         .join = gp100_vmm_join,
> > >         .part = gf100_vmm_part,
> > > -       .aper = gk20a_vmm_aper,
> > > +       .aper = gf100_vmm_aper,
> > >         .valid = gp10b_vmm_valid,
> > >         .flush = gp100_vmm_flush,
> > >         .mthd = gp100_vmm_mthd,
> > > --
> > > 2.23.0
> > >
> > > _______________________________________________
> > > Nouveau mailing list
> > > Nouveau at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/nouveau

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20190920/c4babcdd/attachment.sig>