virtio-gpu without ARCH_HAS_SG_CHAIN

virtio-gpu without ARCH_HAS_SG_CHAIN

[Michael Forney]

Hi,

I was looking at adding virtio-gpu support to tinyemu
(https://bellard.org/tinyemu/). I got it to work on x86, but just for
fun I tried it under riscv and ran into an issue with buffer
allocations (though, this should affect any arch without
ARCH_HAS_SG_CHAIN).

virtio-gpu uses ttm to allocate buffers, which swaps pages to ensure
that they aren't consecutive[0][1]. However, this causes
sg_alloc_table_from_pages to use a sg entry for every single page,
limiting buffers to only 170 pages (the number of sg entries that can
fit into a page). This is only 417x417 with 32bpp. I believe the page
swapping also makes TRANSFER_TO_HOST_2D inefficient by forcing the
host to do many memcpys instead of just a few.

Commenting out the swaps in [0] and [1] makes it work with larger
buffers. I don't know anything about huge pages, or how sg chaining
works, so I have a few questions:
- Is virtio-gpu intended to be usable without ARCH_HAS_SG_CHAIN?
- Is it still necessary to swap the pages when
CONFIG_TRANSPARENT_HUGEPAGE is disabled?
- Should riscv support ARCH_HAS_SG_CHAIN?

Thanks for any help!

[0] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/gpu/drm/ttm/ttm_page_alloc.c?id=fdb1a2236b07948e83e0a777e1795d4f07e52c33
[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/gpu/drm/ttm/ttm_page_alloc.c?id=ae937fe19636067ec5e20d7f1fa10c6cc6000b52

virtio-gpu without ARCH_HAS_SG_CHAIN

[Michael Forney]

Hi,

I was looking at adding virtio-gpu support to tinyemu
(https://bellard.org/tinyemu/). I got it to work on x86, but just for
fun I tried it under riscv and ran into an issue with buffer
allocations (though, this should affect any arch without
ARCH_HAS_SG_CHAIN).

virtio-gpu uses ttm to allocate buffers, which swaps pages to ensure
that they aren't consecutive[0][1]. However, this causes
sg_alloc_table_from_pages to use a sg entry for every single page,
limiting buffers to only 170 pages (the number of sg entries that can
fit into a page). This is only 417x417 with 32bpp. I believe the page
swapping also makes TRANSFER_TO_HOST_2D inefficient by forcing the
host to do many memcpys instead of just a few.

Commenting out the swaps in [0] and [1] makes it work with larger
buffers. I don't know anything about huge pages, or how sg chaining
works, so I have a few questions:
- Is virtio-gpu intended to be usable without ARCH_HAS_SG_CHAIN?
- Is it still necessary to swap the pages when
CONFIG_TRANSPARENT_HUGEPAGE is disabled?
- Should riscv support ARCH_HAS_SG_CHAIN?

Thanks for any help!

[0] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/gpu/drm/ttm/ttm_page_alloc.c?id=fdb1a2236b07948e83e0a777e1795d4f07e52c33
[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/gpu/drm/ttm/ttm_page_alloc.c?id=ae937fe19636067ec5e20d7f1fa10c6cc6000b52

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

virtio-gpu without ARCH_HAS_SG_CHAIN

[David Abdurachmanov]

On Mon, Oct 29, 2018 at 8:47 PM Michael Forney <mforney@mforney.org> wrote:
>
> - Should riscv support ARCH_HAS_SG_CHAIN?

Non-expert here, but it looks like ARCH_HAS_SG_CHAIN
support is generic, and a number of well known architectures
support it.

So it's basically two line patch someone needs to send and
at least make a test by building the kernel. I can do that if you want.

david

Re: virtio-gpu without ARCH_HAS_SG_CHAIN

[David Abdurachmanov]

On Mon, Oct 29, 2018 at 8:47 PM Michael Forney <mforney@mforney.org> wrote:
>
> - Should riscv support ARCH_HAS_SG_CHAIN?

Non-expert here, but it looks like ARCH_HAS_SG_CHAIN
support is generic, and a number of well known architectures
support it.

So it's basically two line patch someone needs to send and
at least make a test by building the kernel. I can do that if you want.

david

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

virtio-gpu without ARCH_HAS_SG_CHAIN

[Gerd Hoffmann]

On Mon, Oct 29, 2018 at 12:46:34PM -0700, Michael Forney wrote:
> Hi,
> 
> I was looking at adding virtio-gpu support to tinyemu
> (https://bellard.org/tinyemu/). I got it to work on x86, but just for
> fun I tried it under riscv and ran into an issue with buffer
> allocations (though, this should affect any arch without
> ARCH_HAS_SG_CHAIN).
> 
> virtio-gpu uses ttm to allocate buffers, which swaps pages to ensure
> that they aren't consecutive[0][1].

Interesting.

While hacking the virtio-gpu ttm code I've already noticed that I get
non-contignous memory even for small allocations (cursor, which is only
4 pages), but havn't found the time yet to look at this.

Christian, care to explain the background?  The commit message sounds a
bit like it papers over a bug somewhere else.

> However, this causes sg_alloc_table_from_pages to use a sg entry for
> every single page, limiting buffers to only 170 pages (the number of
> sg entries that can fit into a page). This is only 417x417 with 32bpp.
> I believe the page swapping also makes TRANSFER_TO_HOST_2D inefficient
> by forcing the host to do many memcpys instead of just a few.

Probably not *that* bad, the amount of data copyed doesn't change after
all.  But, yes, I'd prefer to have shorter sh lists too.

cheers,
  Gerd

> [0] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/gpu/drm/ttm/ttm_page_alloc.c?id=fdb1a2236b07948e83e0a777e1795d4f07e52c33
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/gpu/drm/ttm/ttm_page_alloc.c?id=ae937fe19636067ec5e20d7f1fa10c6cc6000b52

Re: virtio-gpu without ARCH_HAS_SG_CHAIN

[Gerd Hoffmann]

On Mon, Oct 29, 2018 at 12:46:34PM -0700, Michael Forney wrote:
> Hi,
> 
> I was looking at adding virtio-gpu support to tinyemu
> (https://bellard.org/tinyemu/). I got it to work on x86, but just for
> fun I tried it under riscv and ran into an issue with buffer
> allocations (though, this should affect any arch without
> ARCH_HAS_SG_CHAIN).
> 
> virtio-gpu uses ttm to allocate buffers, which swaps pages to ensure
> that they aren't consecutive[0][1].

Interesting.

While hacking the virtio-gpu ttm code I've already noticed that I get
non-contignous memory even for small allocations (cursor, which is only
4 pages), but havn't found the time yet to look at this.

Christian, care to explain the background?  The commit message sounds a
bit like it papers over a bug somewhere else.

> However, this causes sg_alloc_table_from_pages to use a sg entry for
> every single page, limiting buffers to only 170 pages (the number of
> sg entries that can fit into a page). This is only 417x417 with 32bpp.
> I believe the page swapping also makes TRANSFER_TO_HOST_2D inefficient
> by forcing the host to do many memcpys instead of just a few.

Probably not *that* bad, the amount of data copyed doesn't change after
all.  But, yes, I'd prefer to have shorter sh lists too.

cheers,
  Gerd

> [0] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/gpu/drm/ttm/ttm_page_alloc.c?id=fdb1a2236b07948e83e0a777e1795d4f07e52c33
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/gpu/drm/ttm/ttm_page_alloc.c?id=ae937fe19636067ec5e20d7f1fa10c6cc6000b52


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

virtio-gpu without ARCH_HAS_SG_CHAIN

[Koenig, Christian]

Am 30.10.18 um 08:23 schrieb Gerd Hoffmann:
> On Mon, Oct 29, 2018 at 12:46:34PM -0700, Michael Forney wrote:
>> Hi,
>>
>> I was looking at adding virtio-gpu support to tinyemu
>> (https://bellard.org/tinyemu/). I got it to work on x86, but just for
>> fun I tried it under riscv and ran into an issue with buffer
>> allocations (though, this should affect any arch without
>> ARCH_HAS_SG_CHAIN).
>>
>> virtio-gpu uses ttm to allocate buffers, which swaps pages to ensure
>> that they aren't consecutive[0][1].
> Interesting.
>
> While hacking the virtio-gpu ttm code I've already noticed that I get
> non-contignous memory even for small allocations (cursor, which is only
> 4 pages), but havn't found the time yet to look at this.
>
> Christian, care to explain the background?  The commit message sounds a
> bit like it papers over a bug somewhere else.

The problem is that the TTM pool handler thinks that we need to free 
pages to the huge page pool when it sees that they are consecutive.

The root problem of that in turn is that we can't use compound pages for 
device allocated memory, but a real fix for that would require quite 
some work in the MM.

What we can do rather easily is to paper over the problem by only 
swapping page 511 and 510 to avoid that the pool things that this is a 
huge page.

Regards,
Christian.

>
>> However, this causes sg_alloc_table_from_pages to use a sg entry for
>> every single page, limiting buffers to only 170 pages (the number of
>> sg entries that can fit into a page). This is only 417x417 with 32bpp.
>> I believe the page swapping also makes TRANSFER_TO_HOST_2D inefficient
>> by forcing the host to do many memcpys instead of just a few.
> Probably not *that* bad, the amount of data copyed doesn't change after
> all.  But, yes, I'd prefer to have shorter sh lists too.
>
> cheers,
>    Gerd
>
>> [0] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/gpu/drm/ttm/ttm_page_alloc.c?id=fdb1a2236b07948e83e0a777e1795d4f07e52c33
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/gpu/drm/ttm/ttm_page_alloc.c?id=ae937fe19636067ec5e20d7f1fa10c6cc6000b52

Re: virtio-gpu without ARCH_HAS_SG_CHAIN

[Koenig, Christian]

Am 30.10.18 um 08:23 schrieb Gerd Hoffmann:
> On Mon, Oct 29, 2018 at 12:46:34PM -0700, Michael Forney wrote:
>> Hi,
>>
>> I was looking at adding virtio-gpu support to tinyemu
>> (https://bellard.org/tinyemu/). I got it to work on x86, but just for
>> fun I tried it under riscv and ran into an issue with buffer
>> allocations (though, this should affect any arch without
>> ARCH_HAS_SG_CHAIN).
>>
>> virtio-gpu uses ttm to allocate buffers, which swaps pages to ensure
>> that they aren't consecutive[0][1].
> Interesting.
>
> While hacking the virtio-gpu ttm code I've already noticed that I get
> non-contignous memory even for small allocations (cursor, which is only
> 4 pages), but havn't found the time yet to look at this.
>
> Christian, care to explain the background?  The commit message sounds a
> bit like it papers over a bug somewhere else.

The problem is that the TTM pool handler thinks that we need to free 
pages to the huge page pool when it sees that they are consecutive.

The root problem of that in turn is that we can't use compound pages for 
device allocated memory, but a real fix for that would require quite 
some work in the MM.

What we can do rather easily is to paper over the problem by only 
swapping page 511 and 510 to avoid that the pool things that this is a 
huge page.

Regards,
Christian.

>
>> However, this causes sg_alloc_table_from_pages to use a sg entry for
>> every single page, limiting buffers to only 170 pages (the number of
>> sg entries that can fit into a page). This is only 417x417 with 32bpp.
>> I believe the page swapping also makes TRANSFER_TO_HOST_2D inefficient
>> by forcing the host to do many memcpys instead of just a few.
> Probably not *that* bad, the amount of data copyed doesn't change after
> all.  But, yes, I'd prefer to have shorter sh lists too.
>
> cheers,
>    Gerd
>
>> [0] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/gpu/drm/ttm/ttm_page_alloc.c?id=fdb1a2236b07948e83e0a777e1795d4f07e52c33
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/gpu/drm/ttm/ttm_page_alloc.c?id=ae937fe19636067ec5e20d7f1fa10c6cc6000b52

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

virtio-gpu without ARCH_HAS_SG_CHAIN

[Michael Forney]

On 2018-10-29, David Abdurachmanov <david.abdurachmanov@gmail.com> wrote:
> On Mon, Oct 29, 2018 at 8:47 PM Michael Forney <mforney@mforney.org> wrote:
>>
>> - Should riscv support ARCH_HAS_SG_CHAIN?
>
> Non-expert here, but it looks like ARCH_HAS_SG_CHAIN
> support is generic, and a number of well known architectures
> support it.
>
> So it's basically two line patch someone needs to send and
> at least make a test by building the kernel. I can do that if you want.

If that's all there is to it, then yeah, that would be appreciated. Thanks!

Though, I assume there's *some* architecture-specific consideration
that needs to be done, since it has to be selected on each
architecture that supports it.

Re: virtio-gpu without ARCH_HAS_SG_CHAIN

[Michael Forney]

On 2018-10-29, David Abdurachmanov <david.abdurachmanov@gmail.com> wrote:
> On Mon, Oct 29, 2018 at 8:47 PM Michael Forney <mforney@mforney.org> wrote:
>>
>> - Should riscv support ARCH_HAS_SG_CHAIN?
>
> Non-expert here, but it looks like ARCH_HAS_SG_CHAIN
> support is generic, and a number of well known architectures
> support it.
>
> So it's basically two line patch someone needs to send and
> at least make a test by building the kernel. I can do that if you want.

If that's all there is to it, then yeah, that would be appreciated. Thanks!

Though, I assume there's *some* architecture-specific consideration
that needs to be done, since it has to be selected on each
architecture that supports it.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

virtio-gpu without ARCH_HAS_SG_CHAIN

[Christoph Hellwig]

There is no good reason to not define ARCH_HAS_SG_CHAIN.  To fix
your immediate problem just select it from riscv, as riscv uses
the generic dma-direct code which is chained S/G safe by definition.

And if you want to get extra points do a quick audit of the remaning
iommu drivers on architectures that don't define it so that we can
kill the symbol.

Re: virtio-gpu without ARCH_HAS_SG_CHAIN

[Christoph Hellwig]

There is no good reason to not define ARCH_HAS_SG_CHAIN.  To fix
your immediate problem just select it from riscv, as riscv uses
the generic dma-direct code which is chained S/G safe by definition.

And if you want to get extra points do a quick audit of the remaning
iommu drivers on architectures that don't define it so that we can
kill the symbol.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

virtio-gpu without ARCH_HAS_SG_CHAIN

[David Abdurachmanov]

On Tue, Oct 30, 2018 at 6:25 PM Michael Forney <mforney@mforney.org> wrote:
>
> On 2018-10-29, David Abdurachmanov <david.abdurachmanov@gmail.com> wrote:
> > On Mon, Oct 29, 2018 at 8:47 PM Michael Forney <mforney@mforney.org> wrote:
> >>
> >> - Should riscv support ARCH_HAS_SG_CHAIN?
> >
> > Non-expert here, but it looks like ARCH_HAS_SG_CHAIN
> > support is generic, and a number of well known architectures
> > support it.
> >
> > So it's basically two line patch someone needs to send and
> > at least make a test by building the kernel. I can do that if you want.
>
> If that's all there is to it, then yeah, that would be appreciated. Thanks!
>
> Though, I assume there's *some* architecture-specific consideration
> that needs to be done, since it has to be selected on each
> architecture that supports it.

Done it yesterday morning. Compile-tested and I am running
the kernel w/o any issues.

Btw, if you get virtio-gpu support in TinyEMU it would be interesting
to see how that works with Fedora. I have some instructions about
that here:

https://fedoraproject.org/wiki/Architectures/RISC-V/Installing#Boot_under_TinyEMU_.28RISCVEMU.29

david

Re: virtio-gpu without ARCH_HAS_SG_CHAIN

[David Abdurachmanov]

On Tue, Oct 30, 2018 at 6:25 PM Michael Forney <mforney@mforney.org> wrote:
>
> On 2018-10-29, David Abdurachmanov <david.abdurachmanov@gmail.com> wrote:
> > On Mon, Oct 29, 2018 at 8:47 PM Michael Forney <mforney@mforney.org> wrote:
> >>
> >> - Should riscv support ARCH_HAS_SG_CHAIN?
> >
> > Non-expert here, but it looks like ARCH_HAS_SG_CHAIN
> > support is generic, and a number of well known architectures
> > support it.
> >
> > So it's basically two line patch someone needs to send and
> > at least make a test by building the kernel. I can do that if you want.
>
> If that's all there is to it, then yeah, that would be appreciated. Thanks!
>
> Though, I assume there's *some* architecture-specific consideration
> that needs to be done, since it has to be selected on each
> architecture that supports it.

Done it yesterday morning. Compile-tested and I am running
the kernel w/o any issues.

Btw, if you get virtio-gpu support in TinyEMU it would be interesting
to see how that works with Fedora. I have some instructions about
that here:

https://fedoraproject.org/wiki/Architectures/RISC-V/Installing#Boot_under_TinyEMU_.28RISCVEMU.29

david

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv