On 08/20/2015 04:33 PM, Rob Clark wrote:

On Thu, Aug 20, 2015 at 2:48 AM, Thomas Hellstrom thellstrom@vmware.com wrote:

...

Hi, Tiago!

On 08/20/2015 12:33 AM, Tiago Vignatti wrote:

...

Hey Thomas, you haven't answered my email about making SYNC_* mandatory:

http://lists.freedesktop.org/archives/dri-devel/2015-August/088376.html

Hmm, for some reason it doesn't show up in my mail app, but I found it in the archives. An attempt to explain the situation from the vmwgfx perspective.

The fact that the interface is generic means that people will start using it for the zero-copy case. There has been a couple of more or less hackish attempts to do this before, and if it's a _driver_ interface we don't need to be that careful but if it is a _generic_ interface we need to be very careful to make it fit *all* the hardware out there and that we make all potential users use the interface in a way that conforms with the interface specification.

What will happen otherwise is that apps written for coherent fast hardware might, for example, ignore calling the SYNC api, just because the app writer only cared about his own hardware on which the app works fine. That would fail miserably if the same app was run on incoherent hardware, or the incoherent hardware driver maintainers would be forced to base an implementation on page-faults which would be very slow.

So assume the following use case: An app updates a 10x10 area using the CPU on a 1600x1200 dma-buf, and it will then use the dma-buf for texturing. On some hardware the dma-buf might be tiled in a very specific way, on vmwgfx the dma-buf is a GPU buffer on the host, only accessible using DMA. On vmwgfx the SYNC operation must carry out a 10x10 DMA from the host GPU buffer to a guest CPU buffer before the CPU write and a DMA back again after the write, before GPU usage. On the tiled architecture the SYNC operation must untile before CPU access and probably tile again before GPU access.

If we now have a one-dimensional SYNC api, in this particular case we'd either need to sync a far too large area (1600x10) or call SYNC 10 times before writing, and then again after writing. If the app forgot to call SYNC we must error.

just curious, but couldn't you batch up the 10 10x1 sync's?

Yes that would work up to the first CPU access. Subsequent syncs would need to be carried out immediately or all ptes would need to be unmapped to detect the next CPU access. Write only syncs could probably be batched unconditionally.

/Thomas

On Thu, Aug 20, 2015 at 3:27 PM, Thomas Hellstrom thomas@shipmail.org wrote:

On 08/20/2015 04:33 PM, Rob Clark wrote:

...

On Thu, Aug 20, 2015 at 2:48 AM, Thomas Hellstrom thellstrom@vmware.com wrote:

...

Hi, Tiago!

On 08/20/2015 12:33 AM, Tiago Vignatti wrote:

...

Hey Thomas, you haven't answered my email about making SYNC_* mandatory:

http://lists.freedesktop.org/archives/dri-devel/2015-August/088376.html

Hmm, for some reason it doesn't show up in my mail app, but I found it in the archives. An attempt to explain the situation from the vmwgfx perspective.

The fact that the interface is generic means that people will start using it for the zero-copy case. There has been a couple of more or less hackish attempts to do this before, and if it's a _driver_ interface we don't need to be that careful but if it is a _generic_ interface we need to be very careful to make it fit *all* the hardware out there and that we make all potential users use the interface in a way that conforms with the interface specification.

What will happen otherwise is that apps written for coherent fast hardware might, for example, ignore calling the SYNC api, just because the app writer only cared about his own hardware on which the app works fine. That would fail miserably if the same app was run on incoherent hardware, or the incoherent hardware driver maintainers would be forced to base an implementation on page-faults which would be very slow.

So assume the following use case: An app updates a 10x10 area using the CPU on a 1600x1200 dma-buf, and it will then use the dma-buf for texturing. On some hardware the dma-buf might be tiled in a very specific way, on vmwgfx the dma-buf is a GPU buffer on the host, only accessible using DMA. On vmwgfx the SYNC operation must carry out a 10x10 DMA from the host GPU buffer to a guest CPU buffer before the CPU write and a DMA back again after the write, before GPU usage. On the tiled architecture the SYNC operation must untile before CPU access and probably tile again before GPU access.

If we now have a one-dimensional SYNC api, in this particular case we'd either need to sync a far too large area (1600x10) or call SYNC 10 times before writing, and then again after writing. If the app forgot to call SYNC we must error.

just curious, but couldn't you batch up the 10 10x1 sync's?

Yes that would work up to the first CPU access. Subsequent syncs would need to be carried out immediately or all ptes would need to be unmapped to detect the next CPU access. Write only syncs could probably be batched unconditionally.

hmm, maybe another cpu barrier ioctl?

I mean if we had a 2d sync API, then needed to update layers in a 3d or cubemap texture, then you need to do multiple 2d updates..

but what about instead having something like:

ioctl(SYNC) ioctl(SYNC) ioctl(SYNC) ioctl(PREP)

... cpu access

ioctl(FINI)

(or something roughly like that)

BR, -R

/Thomas

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On 08/20/2015 04:53 PM, Jerome Glisse wrote:

On Thu, Aug 20, 2015 at 08:48:23AM +0200, Thomas Hellstrom wrote:

...

Hi, Tiago!

On 08/20/2015 12:33 AM, Tiago Vignatti wrote:

...

Hey Thomas, you haven't answered my email about making SYNC_* mandatory:

http://lists.freedesktop.org/archives/dri-devel/2015-August/088376.html

Hmm, for some reason it doesn't show up in my mail app, but I found it in the archives. An attempt to explain the situation from the vmwgfx perspective.

The fact that the interface is generic means that people will start using it for the zero-copy case. There has been a couple of more or less hackish attempts to do this before, and if it's a _driver_ interface we don't need to be that careful but if it is a _generic_ interface we need to be very careful to make it fit *all* the hardware out there and that we make all potential users use the interface in a way that conforms with the interface specification.

What will happen otherwise is that apps written for coherent fast hardware might, for example, ignore calling the SYNC api, just because the app writer only cared about his own hardware on which the app works fine. That would fail miserably if the same app was run on incoherent hardware, or the incoherent hardware driver maintainers would be forced to base an implementation on page-faults which would be very slow.

So assume the following use case: An app updates a 10x10 area using the CPU on a 1600x1200 dma-buf, and it will then use the dma-buf for texturing. On some hardware the dma-buf might be tiled in a very specific way, on vmwgfx the dma-buf is a GPU buffer on the host, only accessible using DMA. On vmwgfx the SYNC operation must carry out a 10x10 DMA from the host GPU buffer to a guest CPU buffer before the CPU write and a DMA back again after the write, before GPU usage. On the tiled architecture the SYNC operation must untile before CPU access and probably tile again before GPU access.

If we now have a one-dimensional SYNC api, in this particular case we'd either need to sync a far too large area (1600x10) or call SYNC 10 times before writing, and then again after writing. If the app forgot to call SYNC we must error.

So to summarize, the fact that the interface is generic IMO means:

1. Any user must be able to make valid assumptions about the internal

format of the dma-buf. (untiled, color format, stride etc.) 2) Any user *must* call SYNC before and after CPU access. On coherent architectures, the SYNC is a NULL operation anyway, and that should be documented somewhere so that maintainers of drivers of uncoherent architectures have somewhere to point their fingers. 3) Driver-specific implementations must be allowed to error (segfault) if SYNC has not been used.

I think here you are too lax, the common code must segfault or error badly if SYNC has not been use in all cases even on cache coherent arch. The device driver sync callback can still be a no operation. But i think that we need to insist strongly on a proper sync call being made (and we should forbid empty area sync call). This would be the only way to make sure userspace behave properly as otherwise we endup in the situation you were describing above, where the app is design on a cache coherent arch and works fine there but broke in subtle way on non cache coherent arch and app developer is clueless of why.

I just do not trust userspace.

I agree and ideally i'd want it this way as well. The question is, is it possible? Can this be done without a fault() handler in the generic kernel code?

...

4. The use-case stated above clearly shows the benefit of a

2-dimensional sync interface (we want to sync the 10x10 region), but what if someone in the future wants to use this interface for a 3D texture? Will a 2D sync suffice? Can we make the SYNC interface extendable in a way that an enum sync_type member defines the layout of the argument, and initially we implement only 1d, 2d sync, leaving 3d for the future?

Also, I agree there is probably no good way to generically implement an error if SYNC has not been called. That needs to be left as an option to drivers.

I think there is, just forbid any further use of the dma buffer, mark it as invalid and printk a big error. Userspace app developer will quickly see that something is wrong and looking at kernel log should explain why.

The problem is if someone calls mmap() and then decides to not access the buffer before munmap() or GPU usage. How do we recognize that case and separate it from a CPU access occured outside a sync? We could, as mentioned above have a fault() handler in the generic kernel code and make sure drivers don't populate PTEs until the first fault(). Another option would be to scan all PTEs for an "accessed" flag, but I'm not even sure all CPU architectures have such a flag?

But there might be a simpler solution that I have overlooked?

/Thomas

Cheers, Jérôme _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On 08/20/2015 10:34 PM, Jerome Glisse wrote:

On Thu, Aug 20, 2015 at 09:39:12PM +0200, Thomas Hellstrom wrote:

...

On 08/20/2015 04:53 PM, Jerome Glisse wrote:

...

On Thu, Aug 20, 2015 at 08:48:23AM +0200, Thomas Hellstrom wrote:

...

Hi, Tiago!

On 08/20/2015 12:33 AM, Tiago Vignatti wrote:

...

Hey Thomas, you haven't answered my email about making SYNC_* mandatory:

http://lists.freedesktop.org/archives/dri-devel/2015-August/088376.html

Hmm, for some reason it doesn't show up in my mail app, but I found it in the archives. An attempt to explain the situation from the vmwgfx perspective.

The fact that the interface is generic means that people will start using it for the zero-copy case. There has been a couple of more or less hackish attempts to do this before, and if it's a _driver_ interface we don't need to be that careful but if it is a _generic_ interface we need to be very careful to make it fit *all* the hardware out there and that we make all potential users use the interface in a way that conforms with the interface specification.

What will happen otherwise is that apps written for coherent fast hardware might, for example, ignore calling the SYNC api, just because the app writer only cared about his own hardware on which the app works fine. That would fail miserably if the same app was run on incoherent hardware, or the incoherent hardware driver maintainers would be forced to base an implementation on page-faults which would be very slow.

So assume the following use case: An app updates a 10x10 area using the CPU on a 1600x1200 dma-buf, and it will then use the dma-buf for texturing. On some hardware the dma-buf might be tiled in a very specific way, on vmwgfx the dma-buf is a GPU buffer on the host, only accessible using DMA. On vmwgfx the SYNC operation must carry out a 10x10 DMA from the host GPU buffer to a guest CPU buffer before the CPU write and a DMA back again after the write, before GPU usage. On the tiled architecture the SYNC operation must untile before CPU access and probably tile again before GPU access.

If we now have a one-dimensional SYNC api, in this particular case we'd either need to sync a far too large area (1600x10) or call SYNC 10 times before writing, and then again after writing. If the app forgot to call SYNC we must error.

So to summarize, the fact that the interface is generic IMO means:

1. Any user must be able to make valid assumptions about the internal

format of the dma-buf. (untiled, color format, stride etc.) 2) Any user *must* call SYNC before and after CPU access. On coherent architectures, the SYNC is a NULL operation anyway, and that should be documented somewhere so that maintainers of drivers of uncoherent architectures have somewhere to point their fingers. 3) Driver-specific implementations must be allowed to error (segfault) if SYNC has not been used.

I think here you are too lax, the common code must segfault or error badly if SYNC has not been use in all cases even on cache coherent arch. The device driver sync callback can still be a no operation. But i think that we need to insist strongly on a proper sync call being made (and we should forbid empty area sync call). This would be the only way to make sure userspace behave properly as otherwise we endup in the situation you were describing above, where the app is design on a cache coherent arch and works fine there but broke in subtle way on non cache coherent arch and app developer is clueless of why.

I just do not trust userspace.

I agree and ideally i'd want it this way as well. The question is, is it possible? Can this be done without a fault() handler in the generic kernel code?

...

...

4. The use-case stated above clearly shows the benefit of a

2-dimensional sync interface (we want to sync the 10x10 region), but what if someone in the future wants to use this interface for a 3D texture? Will a 2D sync suffice? Can we make the SYNC interface extendable in a way that an enum sync_type member defines the layout of the argument, and initially we implement only 1d, 2d sync, leaving 3d for the future?

Also, I agree there is probably no good way to generically implement an error if SYNC has not been called. That needs to be left as an option to drivers.

I think there is, just forbid any further use of the dma buffer, mark it as invalid and printk a big error. Userspace app developer will quickly see that something is wrong and looking at kernel log should explain why.

The problem is if someone calls mmap() and then decides to not access the buffer before munmap() or GPU usage. How do we recognize that case and separate it from a CPU access occured outside a sync? We could, as mentioned above have a fault() handler in the generic kernel code and make sure drivers don't populate PTEs until the first fault(). Another option would be to scan all PTEs for an "accessed" flag, but I'm not even sure all CPU architectures have such a flag?

But there might be a simpler solution that I have overlooked?

All arch have a dirty flag, so you could check that, as all we really care about is CPU write access. So all you need is clearing the dirty bit after each successfull GPU command stream ioctl and checking that no dirty bit is set in a region not cover by a flush(). Note that the clear and check can happen in the same function as part of buffer validation for the GPU command stream.

Actually, I do think we care about reading as well, since reading without flushing anything written by the GPU will yield garbage on a non-coherent architecture. Instead we might check for the PAGE_ACCESSED bit.

So the full version of this would keep track of all synced regions and at buffer validation time, or unmap time error if there is a page completely outside the union of all synced regions in any VMA belonging to the dma-buf address space, then unmark all accessed PTEs and invalidate TLBs accordingly, typically causing a global TLB flush. Yeah, we could probably put together a helper function that does this, but it will be expensive to run and the whole point of this API is to be able to improve performance.

IMO we should probably do this and after relevant performance testing decide whether to either expose a way to turn it off, either using a compile-time option or run-time, like a flag in the SYNC ioctl.

/Thomas

Cheers, Jérôme

On 08/21/2015 12:28 PM, Lucas Stach wrote:

Am Freitag, den 21.08.2015, 07:25 +0200 schrieb Thomas Hellstrom:

...

On 08/20/2015 10:34 PM, Jerome Glisse wrote:

...

On Thu, Aug 20, 2015 at 09:39:12PM +0200, Thomas Hellstrom wrote:

...

On 08/20/2015 04:53 PM, Jerome Glisse wrote:

...

On Thu, Aug 20, 2015 at 08:48:23AM +0200, Thomas Hellstrom wrote:

...

Hi, Tiago!

On 08/20/2015 12:33 AM, Tiago Vignatti wrote: > Hey Thomas, you haven't answered my email about making SYNC_* mandatory: > > https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_ar... Hmm, for some reason it doesn't show up in my mail app, but I found it in the archives. An attempt to explain the situation from the vmwgfx perspective.

The fact that the interface is generic means that people will start using it for the zero-copy case. There has been a couple of more or less hackish attempts to do this before, and if it's a _driver_ interface we don't need to be that careful but if it is a _generic_ interface we need to be very careful to make it fit *all* the hardware out there and that we make all potential users use the interface in a way that conforms with the interface specification.

What will happen otherwise is that apps written for coherent fast hardware might, for example, ignore calling the SYNC api, just because the app writer only cared about his own hardware on which the app works fine. That would fail miserably if the same app was run on incoherent hardware, or the incoherent hardware driver maintainers would be forced to base an implementation on page-faults which would be very slow.

So assume the following use case: An app updates a 10x10 area using the CPU on a 1600x1200 dma-buf, and it will then use the dma-buf for texturing. On some hardware the dma-buf might be tiled in a very specific way, on vmwgfx the dma-buf is a GPU buffer on the host, only accessible using DMA. On vmwgfx the SYNC operation must carry out a 10x10 DMA from the host GPU buffer to a guest CPU buffer before the CPU write and a DMA back again after the write, before GPU usage. On the tiled architecture the SYNC operation must untile before CPU access and probably tile again before GPU access.

If we now have a one-dimensional SYNC api, in this particular case we'd either need to sync a far too large area (1600x10) or call SYNC 10 times before writing, and then again after writing. If the app forgot to call SYNC we must error.

So to summarize, the fact that the interface is generic IMO means:

1. Any user must be able to make valid assumptions about the internal

format of the dma-buf. (untiled, color format, stride etc.) 2) Any user *must* call SYNC before and after CPU access. On coherent architectures, the SYNC is a NULL operation anyway, and that should be documented somewhere so that maintainers of drivers of uncoherent architectures have somewhere to point their fingers. 3) Driver-specific implementations must be allowed to error (segfault) if SYNC has not been used.

I think here you are too lax, the common code must segfault or error badly if SYNC has not been use in all cases even on cache coherent arch. The device driver sync callback can still be a no operation. But i think that we need to insist strongly on a proper sync call being made (and we should forbid empty area sync call). This would be the only way to make sure userspace behave properly as otherwise we endup in the situation you were describing above, where the app is design on a cache coherent arch and works fine there but broke in subtle way on non cache coherent arch and app developer is clueless of why.

I just do not trust userspace.

I agree and ideally i'd want it this way as well. The question is, is it possible? Can this be done without a fault() handler in the generic kernel code?

...

...

4. The use-case stated above clearly shows the benefit of a

2-dimensional sync interface (we want to sync the 10x10 region), but what if someone in the future wants to use this interface for a 3D texture? Will a 2D sync suffice? Can we make the SYNC interface extendable in a way that an enum sync_type member defines the layout of the argument, and initially we implement only 1d, 2d sync, leaving 3d for the future?

Also, I agree there is probably no good way to generically implement an error if SYNC has not been called. That needs to be left as an option to drivers.

I think there is, just forbid any further use of the dma buffer, mark it as invalid and printk a big error. Userspace app developer will quickly see that something is wrong and looking at kernel log should explain why.

The problem is if someone calls mmap() and then decides to not access the buffer before munmap() or GPU usage. How do we recognize that case and separate it from a CPU access occured outside a sync? We could, as mentioned above have a fault() handler in the generic kernel code and make sure drivers don't populate PTEs until the first fault(). Another option would be to scan all PTEs for an "accessed" flag, but I'm not even sure all CPU architectures have such a flag?

But there might be a simpler solution that I have overlooked?

All arch have a dirty flag, so you could check that, as all we really care about is CPU write access. So all you need is clearing the dirty bit after each successfull GPU command stream ioctl and checking that no dirty bit is set in a region not cover by a flush(). Note that the clear and check can happen in the same function as part of buffer validation for the GPU command stream.

Actually, I do think we care about reading as well, since reading without flushing anything written by the GPU will yield garbage on a non-coherent architecture. Instead we might check for the PAGE_ACCESSED bit.

So the full version of this would keep track of all synced regions and at buffer validation time, or unmap time error if there is a page completely outside the union of all synced regions in any VMA belonging to the dma-buf address space, then unmark all accessed PTEs and invalidate TLBs accordingly, typically causing a global TLB flush. Yeah, we could probably put together a helper function that does this, but it will be expensive to run and the whole point of this API is to be able to improve performance.

IMO we should probably do this and after relevant performance testing decide whether to either expose a way to turn it off, either using a compile-time option or run-time, like a flag in the SYNC ioctl.

Can't we just specify that any access without a SYNC may yield undefined results? A segfault is one of the nicer reactions in that case and totally valid.

But that will, as Jerome points out, probably leave valid results on common coherent hardware, and the lazy app writer would be clueless to or wouldn't care about why it doesn't work on other hardware.

I don't think we want to slow down the common path by PTE fiddling just to validate userspace input. But it may be possible to have a debug option where we just expose an explicit non-coherent copy of the dma-buf through mmap and only copy data to the real buffer on SYNC. Then specify that an application must work with this option enabled, or it isn't conforming to the spec.

Another option that wouldn't degrade performance much would be to require drivers not to populate PTEs until first access. Then in that first access check for sync. (We wrap the fault())

Regards, Lucas

Thomas

On 08/21/2015 03:32 PM, Jerome Glisse wrote:

On Fri, Aug 21, 2015 at 07:25:07AM +0200, Thomas Hellstrom wrote:

...

On 08/20/2015 10:34 PM, Jerome Glisse wrote:

...

On Thu, Aug 20, 2015 at 09:39:12PM +0200, Thomas Hellstrom wrote:

...

On 08/20/2015 04:53 PM, Jerome Glisse wrote:

...

On Thu, Aug 20, 2015 at 08:48:23AM +0200, Thomas Hellstrom wrote:

...

Hi, Tiago!

On 08/20/2015 12:33 AM, Tiago Vignatti wrote: > Hey Thomas, you haven't answered my email about making SYNC_* mandatory: > > https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_ar... Hmm, for some reason it doesn't show up in my mail app, but I found it in the archives. An attempt to explain the situation from the vmwgfx perspective.

The fact that the interface is generic means that people will start using it for the zero-copy case. There has been a couple of more or less hackish attempts to do this before, and if it's a _driver_ interface we don't need to be that careful but if it is a _generic_ interface we need to be very careful to make it fit *all* the hardware out there and that we make all potential users use the interface in a way that conforms with the interface specification.

What will happen otherwise is that apps written for coherent fast hardware might, for example, ignore calling the SYNC api, just because the app writer only cared about his own hardware on which the app works fine. That would fail miserably if the same app was run on incoherent hardware, or the incoherent hardware driver maintainers would be forced to base an implementation on page-faults which would be very slow.

So assume the following use case: An app updates a 10x10 area using the CPU on a 1600x1200 dma-buf, and it will then use the dma-buf for texturing. On some hardware the dma-buf might be tiled in a very specific way, on vmwgfx the dma-buf is a GPU buffer on the host, only accessible using DMA. On vmwgfx the SYNC operation must carry out a 10x10 DMA from the host GPU buffer to a guest CPU buffer before the CPU write and a DMA back again after the write, before GPU usage. On the tiled architecture the SYNC operation must untile before CPU access and probably tile again before GPU access.

If we now have a one-dimensional SYNC api, in this particular case we'd either need to sync a far too large area (1600x10) or call SYNC 10 times before writing, and then again after writing. If the app forgot to call SYNC we must error.

So to summarize, the fact that the interface is generic IMO means:

1. Any user must be able to make valid assumptions about the internal

format of the dma-buf. (untiled, color format, stride etc.) 2) Any user *must* call SYNC before and after CPU access. On coherent architectures, the SYNC is a NULL operation anyway, and that should be documented somewhere so that maintainers of drivers of uncoherent architectures have somewhere to point their fingers. 3) Driver-specific implementations must be allowed to error (segfault) if SYNC has not been used.

I think here you are too lax, the common code must segfault or error badly if SYNC has not been use in all cases even on cache coherent arch. The device driver sync callback can still be a no operation. But i think that we need to insist strongly on a proper sync call being made (and we should forbid empty area sync call). This would be the only way to make sure userspace behave properly as otherwise we endup in the situation you were describing above, where the app is design on a cache coherent arch and works fine there but broke in subtle way on non cache coherent arch and app developer is clueless of why.

I just do not trust userspace.

I agree and ideally i'd want it this way as well. The question is, is it possible? Can this be done without a fault() handler in the generic kernel code?

...

...

4. The use-case stated above clearly shows the benefit of a

2-dimensional sync interface (we want to sync the 10x10 region), but what if someone in the future wants to use this interface for a 3D texture? Will a 2D sync suffice? Can we make the SYNC interface extendable in a way that an enum sync_type member defines the layout of the argument, and initially we implement only 1d, 2d sync, leaving 3d for the future?

Also, I agree there is probably no good way to generically implement an error if SYNC has not been called. That needs to be left as an option to drivers.

I think there is, just forbid any further use of the dma buffer, mark it as invalid and printk a big error. Userspace app developer will quickly see that something is wrong and looking at kernel log should explain why.

The problem is if someone calls mmap() and then decides to not access the buffer before munmap() or GPU usage. How do we recognize that case and separate it from a CPU access occured outside a sync? We could, as mentioned above have a fault() handler in the generic kernel code and make sure drivers don't populate PTEs until the first fault(). Another option would be to scan all PTEs for an "accessed" flag, but I'm not even sure all CPU architectures have such a flag?

But there might be a simpler solution that I have overlooked?

All arch have a dirty flag, so you could check that, as all we really care about is CPU write access. So all you need is clearing the dirty bit after each successfull GPU command stream ioctl and checking that no dirty bit is set in a region not cover by a flush(). Note that the clear and check can happen in the same function as part of buffer validation for the GPU command stream.

Actually, I do think we care about reading as well, since reading without flushing anything written by the GPU will yield garbage on a non-coherent architecture. Instead we might check for the PAGE_ACCESSED bit.

I do not think the read access after GPU matter. If people think it does then it is simple, at buffer validation in buffer mapping we invalidate CPU page table mapping and the prepare access ioctl populate them after checking that GPU is flush.

...

So the full version of this would keep track of all synced regions and at buffer validation time, or unmap time error if there is a page completely outside the union of all synced regions in any VMA belonging to the dma-buf address space, then unmark all accessed PTEs and invalidate TLBs accordingly, typically causing a global TLB flush. Yeah, we could probably put together a helper function that does this, but it will be expensive to run and the whole point of this API is to be able to improve performance.

No need for such complexity, the sync ioctl would go over the page table and clear dirty bit for all page in range that is synchronized. So once you get to the command ioctl you know that all dirty bit must be clear. Note this is only a page table walk in all case. I do not consider this to be that expensive.

The expensive part is the TLB flushing after the PTE update, which typically needs to be performed on all cores. Although without a benchmark I'm not sure how expensive. It might be that it doesn't matter. After all, the vmwgfx- and I believe the qxl driver both use a similar approach for fbdev user-space access, but that has never been cosidered performance-critical...

/Thomas

On 08/21/2015 06:00 PM, Jerome Glisse wrote:

On Fri, Aug 21, 2015 at 04:15:53PM +0200, Thomas Hellstrom wrote:

...

On 08/21/2015 03:32 PM, Jerome Glisse wrote:

...

On Fri, Aug 21, 2015 at 07:25:07AM +0200, Thomas Hellstrom wrote:

...

On 08/20/2015 10:34 PM, Jerome Glisse wrote:

...

On Thu, Aug 20, 2015 at 09:39:12PM +0200, Thomas Hellstrom wrote:

...

On 08/20/2015 04:53 PM, Jerome Glisse wrote: > On Thu, Aug 20, 2015 at 08:48:23AM +0200, Thomas Hellstrom wrote: >> Hi, Tiago! >> >> On 08/20/2015 12:33 AM, Tiago Vignatti wrote: >>> Hey Thomas, you haven't answered my email about making SYNC_* mandatory: >>> >>> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_ar... >> Hmm, for some reason it doesn't show up in my mail app, but I found it >> in the archives. An attempt to explain the situation from the vmwgfx >> perspective. >> >> The fact that the interface is generic means that people will start >> using it for the zero-copy case. There has been a couple of more or less >> hackish attempts to do this before, and if it's a _driver_ interface we >> don't need to be that careful but if it is a _generic_ interface we need >> to be very careful to make it fit *all* the hardware out there and that >> we make all potential users use the interface in a way that conforms >> with the interface specification. >> >> What will happen otherwise is that apps written for coherent fast >> hardware might, for example, ignore calling the SYNC api, just because >> the app writer only cared about his own hardware on which the app works >> fine. That would fail miserably if the same app was run on incoherent >> hardware, or the incoherent hardware driver maintainers would be forced >> to base an implementation on page-faults which would be very slow. >> >> So assume the following use case: An app updates a 10x10 area using the >> CPU on a 1600x1200 dma-buf, and it will then use the dma-buf for >> texturing. On some hardware the dma-buf might be tiled in a very >> specific way, on vmwgfx the dma-buf is a GPU buffer on the host, only >> accessible using DMA. On vmwgfx the SYNC operation must carry out a >> 10x10 DMA from the host GPU buffer to a guest CPU buffer before the CPU >> write and a DMA back again after the write, before GPU usage. On the >> tiled architecture the SYNC operation must untile before CPU access and >> probably tile again before GPU access. >> >> If we now have a one-dimensional SYNC api, in this particular case we'd >> either need to sync a far too large area (1600x10) or call SYNC 10 times >> before writing, and then again after writing. If the app forgot to call >> SYNC we must error. >> >> So to summarize, the fact that the interface is generic IMO means: >> >> 1) Any user must be able to make valid assumptions about the internal >> format of the dma-buf. (untiled, color format, stride etc.) >> 2) Any user *must* call SYNC before and after CPU access. On coherent >> architectures, the SYNC is a NULL operation anyway, and that should be >> documented somewhere so that maintainers of drivers of uncoherent >> architectures have somewhere to point their fingers. >> 3) Driver-specific implementations must be allowed to error (segfault) >> if SYNC has not been used. > I think here you are too lax, the common code must segfault or > error badly if SYNC has not been use in all cases even on cache > coherent arch. The device driver sync callback can still be a > no operation. But i think that we need to insist strongly on a > proper sync call being made (and we should forbid empty area > sync call). This would be the only way to make sure userspace > behave properly as otherwise we endup in the situation you were > describing above, where the app is design on a cache coherent > arch and works fine there but broke in subtle way on non cache > coherent arch and app developer is clueless of why. > > I just do not trust userspace. I agree and ideally i'd want it this way as well. The question is, is it possible? Can this be done without a fault() handler in the generic kernel code?

>> 4) The use-case stated above clearly shows the benefit of a >> 2-dimensional sync interface (we want to sync the 10x10 region), but >> what if someone in the future wants to use this interface for a 3D >> texture? Will a 2D sync suffice? Can we make the SYNC interface >> extendable in a way that an enum sync_type member defines the layout of >> the argument, and initially we implement only 1d, 2d sync, leaving 3d >> for the future? >> >> Also, I agree there is probably no good way to generically implement an >> error if SYNC has not been called. That needs to be left as an option to >> drivers. > I think there is, just forbid any further use of the dma buffer, mark > it as invalid and printk a big error. Userspace app developer will > quickly see that something is wrong and looking at kernel log should > explain why. The problem is if someone calls mmap() and then decides to not access the buffer before munmap() or GPU usage. How do we recognize that case and separate it from a CPU access occured outside a sync? We could, as mentioned above have a fault() handler in the generic kernel code and make sure drivers don't populate PTEs until the first fault(). Another option would be to scan all PTEs for an "accessed" flag, but I'm not even sure all CPU architectures have such a flag?

But there might be a simpler solution that I have overlooked?

All arch have a dirty flag, so you could check that, as all we really care about is CPU write access. So all you need is clearing the dirty bit after each successfull GPU command stream ioctl and checking that no dirty bit is set in a region not cover by a flush(). Note that the clear and check can happen in the same function as part of buffer validation for the GPU command stream.

Actually, I do think we care about reading as well, since reading without flushing anything written by the GPU will yield garbage on a non-coherent architecture. Instead we might check for the PAGE_ACCESSED bit.

I do not think the read access after GPU matter. If people think it does then it is simple, at buffer validation in buffer mapping we invalidate CPU page table mapping and the prepare access ioctl populate them after checking that GPU is flush.

...

So the full version of this would keep track of all synced regions and at buffer validation time, or unmap time error if there is a page completely outside the union of all synced regions in any VMA belonging to the dma-buf address space, then unmark all accessed PTEs and invalidate TLBs accordingly, typically causing a global TLB flush. Yeah, we could probably put together a helper function that does this, but it will be expensive to run and the whole point of this API is to be able to improve performance.

No need for such complexity, the sync ioctl would go over the page table and clear dirty bit for all page in range that is synchronized. So once you get to the command ioctl you know that all dirty bit must be clear. Note this is only a page table walk in all case. I do not consider this to be that expensive.

The expensive part is the TLB flushing after the PTE update, which typically needs to be performed on all cores. Although without a benchmark I'm not sure how expensive. It might be that it doesn't matter. After all, the vmwgfx- and I believe the qxl driver both use a similar approach for fbdev user-space access, but that has never been cosidered performance-critical...

There is no need to flush the TLB ! At least not for the write case. Clearing the dirty bit is cache coherent. TLB are about caching page table directory tree. To convince yourself look at page_mkclean_one() in mm/rmap.c which, by the way, is almost what we want (remove the mmu notifier part and the write protection part and force real CPU cache flush if needed base on device flag).

Hmm? Last time I visited this code, page_mkclean_one() was eventually calling flush_tlb_page() through ptep_clear_flush_notify(). I was under the impression that you always have to flush the TLB when you clear a PTE status bit, but typically not when you set it. Are you saying that that's not the case? The question as I see it is rather whether you want to flush TLB on a per-PTE basis or do it as a global flush after a batched PTE status bit clearing.

/Thomas

On 08/24/2015 03:41 AM, Jerome Glisse wrote:

On Sat, Aug 22, 2015 at 12:00:21AM +0200, Thomas Hellstrom wrote:

...

On 08/21/2015 06:00 PM, Jerome Glisse wrote:

...

On Fri, Aug 21, 2015 at 04:15:53PM +0200, Thomas Hellstrom wrote:

...

On 08/21/2015 03:32 PM, Jerome Glisse wrote:

...

On Fri, Aug 21, 2015 at 07:25:07AM +0200, Thomas Hellstrom wrote:

...

On 08/20/2015 10:34 PM, Jerome Glisse wrote: > On Thu, Aug 20, 2015 at 09:39:12PM +0200, Thomas Hellstrom wrote: >> On 08/20/2015 04:53 PM, Jerome Glisse wrote: >>> On Thu, Aug 20, 2015 at 08:48:23AM +0200, Thomas Hellstrom wrote: >>>> Hi, Tiago! >>>> >>>> On 08/20/2015 12:33 AM, Tiago Vignatti wrote: >>>>> Hey Thomas, you haven't answered my email about making SYNC_* mandatory: >>>>> >>>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_ar... >>>> Hmm, for some reason it doesn't show up in my mail app, but I found it >>>> in the archives. An attempt to explain the situation from the vmwgfx >>>> perspective. >>>> >>>> The fact that the interface is generic means that people will start >>>> using it for the zero-copy case. There has been a couple of more or less >>>> hackish attempts to do this before, and if it's a _driver_ interface we >>>> don't need to be that careful but if it is a _generic_ interface we need >>>> to be very careful to make it fit *all* the hardware out there and that >>>> we make all potential users use the interface in a way that conforms >>>> with the interface specification. >>>> >>>> What will happen otherwise is that apps written for coherent fast >>>> hardware might, for example, ignore calling the SYNC api, just because >>>> the app writer only cared about his own hardware on which the app works >>>> fine. That would fail miserably if the same app was run on incoherent >>>> hardware, or the incoherent hardware driver maintainers would be forced >>>> to base an implementation on page-faults which would be very slow. >>>> >>>> So assume the following use case: An app updates a 10x10 area using the >>>> CPU on a 1600x1200 dma-buf, and it will then use the dma-buf for >>>> texturing. On some hardware the dma-buf might be tiled in a very >>>> specific way, on vmwgfx the dma-buf is a GPU buffer on the host, only >>>> accessible using DMA. On vmwgfx the SYNC operation must carry out a >>>> 10x10 DMA from the host GPU buffer to a guest CPU buffer before the CPU >>>> write and a DMA back again after the write, before GPU usage. On the >>>> tiled architecture the SYNC operation must untile before CPU access and >>>> probably tile again before GPU access. >>>> >>>> If we now have a one-dimensional SYNC api, in this particular case we'd >>>> either need to sync a far too large area (1600x10) or call SYNC 10 times >>>> before writing, and then again after writing. If the app forgot to call >>>> SYNC we must error. >>>> >>>> So to summarize, the fact that the interface is generic IMO means: >>>> >>>> 1) Any user must be able to make valid assumptions about the internal >>>> format of the dma-buf. (untiled, color format, stride etc.) >>>> 2) Any user *must* call SYNC before and after CPU access. On coherent >>>> architectures, the SYNC is a NULL operation anyway, and that should be >>>> documented somewhere so that maintainers of drivers of uncoherent >>>> architectures have somewhere to point their fingers. >>>> 3) Driver-specific implementations must be allowed to error (segfault) >>>> if SYNC has not been used. >>> I think here you are too lax, the common code must segfault or >>> error badly if SYNC has not been use in all cases even on cache >>> coherent arch. The device driver sync callback can still be a >>> no operation. But i think that we need to insist strongly on a >>> proper sync call being made (and we should forbid empty area >>> sync call). This would be the only way to make sure userspace >>> behave properly as otherwise we endup in the situation you were >>> describing above, where the app is design on a cache coherent >>> arch and works fine there but broke in subtle way on non cache >>> coherent arch and app developer is clueless of why. >>> >>> I just do not trust userspace. >> I agree and ideally i'd want it this way as well. The question is, is it >> possible? Can this be done without a fault() handler in the generic >> kernel code? >> >>>> 4) The use-case stated above clearly shows the benefit of a >>>> 2-dimensional sync interface (we want to sync the 10x10 region), but >>>> what if someone in the future wants to use this interface for a 3D >>>> texture? Will a 2D sync suffice? Can we make the SYNC interface >>>> extendable in a way that an enum sync_type member defines the layout of >>>> the argument, and initially we implement only 1d, 2d sync, leaving 3d >>>> for the future? >>>> >>>> Also, I agree there is probably no good way to generically implement an >>>> error if SYNC has not been called. That needs to be left as an option to >>>> drivers. >>> I think there is, just forbid any further use of the dma buffer, mark >>> it as invalid and printk a big error. Userspace app developer will >>> quickly see that something is wrong and looking at kernel log should >>> explain why. >> The problem is if someone calls mmap() and then decides to not access >> the buffer before munmap() or GPU usage. How do we recognize that case >> and separate it from a CPU access occured outside a sync? We could, as >> mentioned above have a fault() handler in the generic kernel code and >> make sure drivers don't populate PTEs until the first fault(). Another >> option would be to scan all PTEs for an "accessed" flag, but I'm not >> even sure all CPU architectures have such a flag? >> >> But there might be a simpler solution that I have overlooked? > All arch have a dirty flag, so you could check that, as all we > really care about is CPU write access. So all you need is clearing > the dirty bit after each successfull GPU command stream ioctl > and checking that no dirty bit is set in a region not cover by > a flush(). Note that the clear and check can happen in the same > function as part of buffer validation for the GPU command stream. Actually, I do think we care about reading as well, since reading without flushing anything written by the GPU will yield garbage on a non-coherent architecture. Instead we might check for the PAGE_ACCESSED bit.

I do not think the read access after GPU matter. If people think it does then it is simple, at buffer validation in buffer mapping we invalidate CPU page table mapping and the prepare access ioctl populate them after checking that GPU is flush.

...

So the full version of this would keep track of all synced regions and at buffer validation time, or unmap time error if there is a page completely outside the union of all synced regions in any VMA belonging to the dma-buf address space, then unmark all accessed PTEs and invalidate TLBs accordingly, typically causing a global TLB flush. Yeah, we could probably put together a helper function that does this, but it will be expensive to run and the whole point of this API is to be able to improve performance.

No need for such complexity, the sync ioctl would go over the page table and clear dirty bit for all page in range that is synchronized. So once you get to the command ioctl you know that all dirty bit must be clear. Note this is only a page table walk in all case. I do not consider this to be that expensive.

The expensive part is the TLB flushing after the PTE update, which typically needs to be performed on all cores. Although without a benchmark I'm not sure how expensive. It might be that it doesn't matter. After all, the vmwgfx- and I believe the qxl driver both use a similar approach for fbdev user-space access, but that has never been cosidered performance-critical...

There is no need to flush the TLB ! At least not for the write case. Clearing the dirty bit is cache coherent. TLB are about caching page table directory tree. To convince yourself look at page_mkclean_one() in mm/rmap.c which, by the way, is almost what we want (remove the mmu notifier part and the write protection part and force real CPU cache flush if needed base on device flag).

Hmm? Last time I visited this code, page_mkclean_one() was eventually calling flush_tlb_page() through ptep_clear_flush_notify(). I was under the impression that you always have to flush the TLB when you clear a PTE status bit, but typically not when you set it. Are you saying that that's not the case?

Well this is very down to the implementation details of hw, consideration and my memory is that the AMD/Intel doc says that write is a locked read modify write operation ie it is atomic. Now there is a bit of unknown on how exactly this happens (does the hardware retraverse the full directory hierarchy ? does it keeps a pointer to the page table entry ? ...).

So basicly this means that at time of validation if any CPU did write to any page inside the range we would see the dirty set, we do not need to flush anything to see that happening (unless things are mapped write combine).

Well there is a race window if CPU is writing to the buffer while we are doing the validation on another CPU. But there is no way around that race except to unmap and flush TLB. Which is not what we want.

Now, there is something i am not sure, does CPU keep TLB entry around with dirty bit set and do not redo the RMW locked sequence, or does it do the RMW locked sequence everytime it write to a cacheline. I would guess the former if only for perf reasons. In this case we would need TLB flush but linux kernel already offer helpers to optimize this away.

All this kind of make me think that there is no sane way around this. I mean we are trying to work around potentialy broken userspace mostly because most of the GPU driver is in userspace and we can't trust it. I think this is just a lost battle and it would be better to just provide the API and says that you must sync and userspace that do not will get to bear the consequence sooner or latter.

Cheers, Jérôme

It would still be possible to track down those cases where SYNC is completely ignored, by traversing the page tables looking for PAGE_ACCESSED without any SYNC whatsoever. Sure, userspace could find ways around that but it would still force people to read up on the SYNC functionality.

/Thomas

Hi!

On 08/22/2015 01:06 AM, Tiago Vignatti wrote:

Hi back!

On 08/20/2015 03:48 AM, Thomas Hellstrom wrote:

...

Hi, Tiago!

Something that the Chrome OS folks told me today is whether we could change the sync API to use a syscall for that instead. Reason for that is to eventually fit this in nicely in their sandbox architecture requirements, so yet another ioctl wouldn't need to be white-listed for the unpriviledged process.

How does that change the game, for example regarding the details we been talking here about make the sync mandatory? Can we reuse, say msync(2) for that for example?

To me, the mechanism itself is the most important, but I think it should be much easier to have an IOCTL accepted since the functionality is limited to dma-bufs.

In any case, Ideally I'd want the struct dma_buf_sync look something like

enum dma_buf_sync_flags { DMA_BUF_SYNC_READ = (1 << 0), DMA_BUF_SYNC_WRITE = (2 << 0), DMA_BUF_SYNC_RW = (3 << 0), DMA_BUF_SYNC_START = (0 << 2), DMA_BUF_SYNC_END = (1 << 2),

DMA_BUF_SYNC_VALID_FLAGS_MASK = DMA_BUF_SYNC_RW | DMA_BUF_SYNC_END };

/* begin/end dma-buf functions used for userspace mmap. */ struct dma_buf_sync { enum dma_buf_sync_flags flags; __u64 x; __u64 width;

/* 2D Sync parameters */ __u64 stride; __u64 y; __u64 height;

}

So again, why use a 2D sync in the first place when it's probably possible to batch 1D syncs in the driver? It all boils down to the number of syscalls required, and also the fact that it's probably not trivial to batch sync calls in the read-write case. One can, again, argue that there might be 3D cases or other obscure sync cases, but in that case, let's implement them in the future if the overhead becomes unbearable. If the sync mechanism gets established as mandatory, the problem will solve itself. And at least now we cover the most common case and we don't need to add a new multiplexing mechanism in the sync IOCTL since we already have one in the IOCTL mechanism. We can simply add a new sync IOCTL for new obscure sync cases if desired.

Also, in fact I believe this also gives us a hint whether to use sycalls or IOCTLs: If we want to maintain the possibility to extend the synchronization mechanism, I strongly believe we need to use an IOCTL.

/Thomas

Best Regards,

Tiago _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

Hi, Daniel,

On 08/24/2015 05:52 PM, Daniel Stone wrote:

Hi Thomas,

On 24 August 2015 at 10:50, Thomas Hellstrom thomas@shipmail.org wrote:

...

In any case, Ideally I'd want the struct dma_buf_sync look something like

enum dma_buf_sync_flags { DMA_BUF_SYNC_READ = (1 << 0), DMA_BUF_SYNC_WRITE = (2 << 0), DMA_BUF_SYNC_RW = (3 << 0), DMA_BUF_SYNC_START = (0 << 2), DMA_BUF_SYNC_END = (1 << 2),

    DMA_BUF_SYNC_VALID_FLAGS_MASK = DMA_BUF_SYNC_RW |
            DMA_BUF_SYNC_END

};

/* begin/end dma-buf functions used for userspace mmap. */ struct dma_buf_sync { enum dma_buf_sync_flags flags; __u64 x; __u64 width;

    /* 2D Sync parameters */
    __u64 stride;
    __u64 y;
    __u64 height;

}

So again, why use a 2D sync in the first place when it's probably possible to batch 1D syncs in the driver? It all boils down to the number of syscalls required, and also the fact that it's probably not trivial to batch sync calls in the read-write case. One can, again, argue that there might be 3D cases or other obscure sync cases, but in that case, let's implement them in the future if the overhead becomes unbearable. If the sync mechanism gets established as mandatory, the problem will solve itself. And at least now we cover the most common case and we don't need to add a new multiplexing mechanism in the sync IOCTL since we already have one in the IOCTL mechanism. We can simply add a new sync IOCTL for new obscure sync cases if desired.

I still don't think this ameliorates the need for batching: consider the case where you update two disjoint screen regions and want them both flushed. Either you issue two separate sync calls (which can be disadvantageous performance-wise on some hardware / setups), or you accumulate the regions and only flush later. So either two ioctls (one in the style of dirtyfb and one to perform the sync/flush; you can shortcut to assume the full buffer was damaged if the former is missing), or one like this:

struct dma_buf_sync_2d { enum dma_buf_sync_flags flags;

    __u64 stride_bytes;
    __u32 bytes_per_pixel;
    __u32 num_regions;

    struct {
            __u64 x;
            __u64 y;
            __u64 width;
            __u64 height;
    } regions[];

};

Cheers, Daniel

Fine with me, although perhaps bytes_per_pixel is a bit redundant?

/Thomas

On 08/24/2015 07:04 PM, Daniel Stone wrote:

Hi,

On 24 August 2015 at 17:56, Thomas Hellstrom thellstrom@vmware.com wrote:

...

On 08/24/2015 05:52 PM, Daniel Stone wrote:

...

I still don't think this ameliorates the need for batching: consider the case where you update two disjoint screen regions and want them both flushed. Either you issue two separate sync calls (which can be disadvantageous performance-wise on some hardware / setups), or you accumulate the regions and only flush later. So either two ioctls (one in the style of dirtyfb and one to perform the sync/flush; you can shortcut to assume the full buffer was damaged if the former is missing), or one like this:

struct dma_buf_sync_2d { enum dma_buf_sync_flags flags;

    __u64 stride_bytes;
    __u32 bytes_per_pixel;
    __u32 num_regions;

    struct {
            __u64 x;
            __u64 y;
            __u64 width;
            __u64 height;
    } regions[];

};

Cheers, Daniel

Fine with me, although perhaps bytes_per_pixel is a bit redundant?

Redundant how? It's not implicit in stride.

For flushing purposes, isn't it possible to cover all cases by assuming bytes_per_pixel=1? Not that it matters much.

/Thomas

On 08/24/2015 07:12 PM, Daniel Stone wrote:

Hi,

On 24 August 2015 at 18:10, Thomas Hellstrom thellstrom@vmware.com wrote:

...

On 08/24/2015 07:04 PM, Daniel Stone wrote:

...

On 24 August 2015 at 17:56, Thomas Hellstrom thellstrom@vmware.com wrote:

...

On 08/24/2015 05:52 PM, Daniel Stone wrote:

...

I still don't think this ameliorates the need for batching: consider the case where you update two disjoint screen regions and want them both flushed. Either you issue two separate sync calls (which can be disadvantageous performance-wise on some hardware / setups), or you accumulate the regions and only flush later. So either two ioctls (one in the style of dirtyfb and one to perform the sync/flush; you can shortcut to assume the full buffer was damaged if the former is missing), or one like this:

struct dma_buf_sync_2d { enum dma_buf_sync_flags flags;

    __u64 stride_bytes;
    __u32 bytes_per_pixel;
    __u32 num_regions;

    struct {
            __u64 x;
            __u64 y;
            __u64 width;
            __u64 height;
    } regions[];

};

Fine with me, although perhaps bytes_per_pixel is a bit redundant?

Redundant how? It's not implicit in stride.

For flushing purposes, isn't it possible to cover all cases by assuming bytes_per_pixel=1? Not that it matters much.

Sure, though in that case best to replace x with line_byte_offset or something, because otherwise I guarantee you everyone will get it wrong, and it'll be a pain to track down. Like how I managed to misread it now. :)

OK, yeah you have a point. IMO let's go for your proposal.

Tiago, is this OK with you?

/Thomas

Cheers, Daniel

Hi,

On 08/24/2015 07:56 PM, Michael Spang wrote:

On Mon, Aug 24, 2015 at 1:42 PM, Thomas Hellstrom thellstrom@vmware.com wrote:

...

On 08/24/2015 07:12 PM, Daniel Stone wrote:

...

Hi,

On 24 August 2015 at 18:10, Thomas Hellstrom thellstrom@vmware.com wrote:

...

On 08/24/2015 07:04 PM, Daniel Stone wrote:

...

On 24 August 2015 at 17:56, Thomas Hellstrom thellstrom@vmware.com wrote:

...

On 08/24/2015 05:52 PM, Daniel Stone wrote: > I still don't think this ameliorates the need for batching: consider > the case where you update two disjoint screen regions and want them > both flushed. Either you issue two separate sync calls (which can be > disadvantageous performance-wise on some hardware / setups), or you > accumulate the regions and only flush later. So either two ioctls (one > in the style of dirtyfb and one to perform the sync/flush; you can > shortcut to assume the full buffer was damaged if the former is > missing), or one like this: > > struct dma_buf_sync_2d { > enum dma_buf_sync_flags flags; > > __u64 stride_bytes; > __u32 bytes_per_pixel; > __u32 num_regions; > > struct { > __u64 x; > __u64 y; > __u64 width; > __u64 height; > } regions[]; > }; Fine with me, although perhaps bytes_per_pixel is a bit redundant?

Redundant how? It's not implicit in stride.

For flushing purposes, isn't it possible to cover all cases by assuming bytes_per_pixel=1? Not that it matters much.

Sure, though in that case best to replace x with line_byte_offset or something, because otherwise I guarantee you everyone will get it wrong, and it'll be a pain to track down. Like how I managed to misread it now. :)

OK, yeah you have a point. IMO let's go for your proposal.

Tiago, is this OK with you?

Is there any obstacle to making the above API a new syscall?

I think that whether this API can be accepted as a syscall or not is really up to Dave and in the end Linus. What speaks against is that it's not applicable to all file types, only dma-bufs and also that we might want to extend it in the future.

The reason we have issues with ioctls is because it's not possible to whitelist them properly with seccomp BPF - there's no single namespace for ioctls.

If this API is merged as a ioctl, we may not be able to actually use it. Which is a bit unfortunate when it has been proposed with the chrome renderer use case in mind.

I don't think mmap() on dma-bufs can be accepted as a _generic_ API without a mandatory sync mechanism, and I think there is a need to specify a sync api regardless of who is the first user.

Thanks, Thomas

Michael

On 08/24/2015 03:01 PM, Tiago Vignatti wrote:

yup, I think so. So IIUC the main changes needed for the drivers implement 2D sync lies in the dma_buf_sync_2d structure only. I.e. there's nothing really to be changed in the common code, right?

Do we have any special requirements in how we want pass the sync information to the drivers? I was thinking to push the whole responsibility for them, something like:

+int dma_buf_begin_cpu_access(struct dma_buf *dma_buf, size_t stride_bytes, + size_t bytes_per_pixel, size_t num_regions, + struct dma_buf_sync_region regions[], enum dma_data_direction dir);

Daniel Vetter mentioned about dma-buf design that should not track metadata but I haven't read anything about it, so do you think this looks alright?

Tiago

On 08/25/2015 11:02 AM, Daniel Vetter wrote:

On Mon, Aug 24, 2015 at 04:52:13PM +0100, Daniel Stone wrote:

...

Hi Thomas,

On 24 August 2015 at 10:50, Thomas Hellstrom thomas@shipmail.org wrote:

...

In any case, Ideally I'd want the struct dma_buf_sync look something like

enum dma_buf_sync_flags { DMA_BUF_SYNC_READ = (1 << 0), DMA_BUF_SYNC_WRITE = (2 << 0), DMA_BUF_SYNC_RW = (3 << 0), DMA_BUF_SYNC_START = (0 << 2), DMA_BUF_SYNC_END = (1 << 2),

    DMA_BUF_SYNC_VALID_FLAGS_MASK = DMA_BUF_SYNC_RW |
            DMA_BUF_SYNC_END

};

/* begin/end dma-buf functions used for userspace mmap. */ struct dma_buf_sync { enum dma_buf_sync_flags flags; __u64 x; __u64 width;

    /* 2D Sync parameters */
    __u64 stride;
    __u64 y;
    __u64 height;

}

So again, why use a 2D sync in the first place when it's probably possible to batch 1D syncs in the driver? It all boils down to the number of syscalls required, and also the fact that it's probably not trivial to batch sync calls in the read-write case. One can, again, argue that there might be 3D cases or other obscure sync cases, but in that case, let's implement them in the future if the overhead becomes unbearable. If the sync mechanism gets established as mandatory, the problem will solve itself. And at least now we cover the most common case and we don't need to add a new multiplexing mechanism in the sync IOCTL since we already have one in the IOCTL mechanism. We can simply add a new sync IOCTL for new obscure sync cases if desired.

I still don't think this ameliorates the need for batching: consider the case where you update two disjoint screen regions and want them both flushed. Either you issue two separate sync calls (which can be disadvantageous performance-wise on some hardware / setups), or you accumulate the regions and only flush later. So either two ioctls (one in the style of dirtyfb and one to perform the sync/flush; you can shortcut to assume the full buffer was damaged if the former is missing), or one like this:

struct dma_buf_sync_2d { enum dma_buf_sync_flags flags;

    __u64 stride_bytes;
    __u32 bytes_per_pixel;
    __u32 num_regions;

    struct {
            __u64 x;
            __u64 y;
            __u64 width;
            __u64 height;
    } regions[];

};

I really feel like any kind of multi-range flush interface is feature bloat, and if we do it then we should only do it later on when there's a clear need for it.

IMO all the use-cases so far that wanted to do this have been 2D updates. and having only a 1D sync will most probably scare people away from this interface.

Afaiui dma-buf mmap will mostly be used for up/downloads, which means there will be some explicit copy involved somewhere anyway. So similar to userptr usage. And for those most often you just slurp in a linear block and then let the blitter rearrange it, at least on i915.

Also thus far the consensus was that dma-buf doesn't care/know about content layout, adding strides/bytes/whatever does bend this quite a bit.

I think with a 2D interface, the stride only applies to the sync operation itself and is only a parameter for that sync operation. Whether we should have multiple regions or not is not a big deal for me, but I think at least a 2D sync is crucial.

And finally if we concentrate on the simple-case of one byte range we can focus the discussion on correctness, which imo is the important bit here really.

If we only focus on correctness there are other ways to do this without a sync: We can DMA based on accessed and dirty pages, but performance will crawl, so IMO the sync interface is purely for performance...

-Daniel

Thanks, Thomas

On Wed, Aug 26, 2015 at 08:49:00AM +0200, Thomas Hellstrom wrote:

Hi, Tiago.

On 08/26/2015 02:02 AM, Tiago Vignatti wrote:

...

From: Daniel Vetter daniel.vetter@ffwll.ch

The userspace might need some sort of cache coherency management e.g. when CPU and GPU domains are being accessed through dma-buf at the same time. To circumvent this problem there are begin/end coherency markers, that forward directly to existing dma-buf device drivers vfunc hooks. Userspace can make use of those markers through the DMA_BUF_IOCTL_SYNC ioctl. The sequence would be used like following:

• mmap dma-buf fd
• for each drawing/upload cycle in CPU
  1. SYNC_START ioctl
  2. read/write to mmap area or a 2d sub-region of it
  3. SYNC_END ioctl.
• munamp once you don't need the buffer any more

v2 (Tiago): Fix header file type names (u64 -> __u64) v3 (Tiago): Add documentation. Use enum dma_buf_sync_flags to the begin/end dma-buf functions. Check for overflows in start/length. v4 (Tiago): use 2d regions for sync.

Daniel V had issues with the sync argument proposed by Daniel S. I'm fine with that argument or an argument containing only a single sync rect. I'm not sure whether Daniel V will find it easier to accept only a single sync rect...

I'm kinda against all the 2d rect sync proposals ;-) At least for the current stuff it's all about linear subranges afaik, and even there we don't bother with flushing them precisely right now.

My expectation would be that if you _really_ want to etch out that last bit of performance with a list of 2d sync ranges then probably you want to do the cpu cache flushing in userspace anyway, with 100% machine-specific trickery. -Daniel

On Wed, Aug 26, 2015 at 02:28:30PM +0200, Thomas Hellstrom wrote:

On 08/26/2015 02:10 PM, Daniel Vetter wrote:

...

On Wed, Aug 26, 2015 at 08:49:00AM +0200, Thomas Hellstrom wrote:

...

Hi, Tiago.

On 08/26/2015 02:02 AM, Tiago Vignatti wrote:

...

From: Daniel Vetter daniel.vetter@ffwll.ch

The userspace might need some sort of cache coherency management e.g. when CPU and GPU domains are being accessed through dma-buf at the same time. To circumvent this problem there are begin/end coherency markers, that forward directly to existing dma-buf device drivers vfunc hooks. Userspace can make use of those markers through the DMA_BUF_IOCTL_SYNC ioctl. The sequence would be used like following:

• mmap dma-buf fd
• for each drawing/upload cycle in CPU
  1. SYNC_START ioctl
  2. read/write to mmap area or a 2d sub-region of it
  3. SYNC_END ioctl.
• munamp once you don't need the buffer any more

v2 (Tiago): Fix header file type names (u64 -> __u64) v3 (Tiago): Add documentation. Use enum dma_buf_sync_flags to the begin/end dma-buf functions. Check for overflows in start/length. v4 (Tiago): use 2d regions for sync.

Daniel V had issues with the sync argument proposed by Daniel S. I'm fine with that argument or an argument containing only a single sync rect. I'm not sure whether Daniel V will find it easier to accept only a single sync rect...

I'm kinda against all the 2d rect sync proposals ;-) At least for the current stuff it's all about linear subranges afaik, and even there we don't bother with flushing them precisely right now.

My expectation would be that if you _really_ want to etch out that last bit of performance with a list of 2d sync ranges then probably you want to do the cpu cache flushing in userspace anyway, with 100% machine-specific trickery.

Daniel,

I might be misunderstanding things, but isn't this about finally accepting a dma-buf mmap() generic interface for people who want to use it for zero-copy applications (like people have been trying to do for years but never bothered to specify an interface that actually performed on incoherent hardware)?

If it's only about exposing the kernel 1D sync interface to user-space for correctness, then why isn't that done transparently to the user?

Mostly pragmatic reasons - we could do the page-fault trickery, but that means i915 needs another mmap implementation. At least I didn't figure out how to do faulting in a completely generic way. And we already have 3 other mmap implementations so I prefer not to do that.

The other is that right now there's no user nor implementation in sight which actually does range-based flush optimizations, so I'm pretty much expecting we'll get it wrong. Maybe instead we should go one step further and remove the range from the internal dma-buf interface and also drop it from the ioctl? With the flags we can always add something later on once we have a real user with a clear need for it. But afaik cros only wants to shuffle around entire tiles and has a buffer-per-tile approach. -Daniel

On 08/26/2015 04:32 PM, Tiago Vignatti wrote:

On 08/26/2015 09:58 AM, Daniel Vetter wrote:

...

The other is that right now there's no user nor implementation in sight which actually does range-based flush optimizations, so I'm pretty much expecting we'll get it wrong. Maybe instead we should go one step further and remove the range from the internal dma-buf interface and also drop it from the ioctl? With the flags we can always add something later on once we have a real user with a clear need for it. But afaik cros only wants to shuffle around entire tiles and has a buffer-per-tile approach.

Thomas, I think Daniel has a point here and also, I wouldn't mind removing all range control from the dma-buf ioctl either.

In this last patch we can see that it's not complicated to add the 2d-sync if we eventually decides to want it. But right now there's no way we can test it. Therefore in that case I'm all in favour of doing this work gradually, adding the basics first.

Sure. Unfortunately I wasn't completely clear about the use-case here. IMO adding a user-space sync functionality should be purely for performance.

/Thomas

On 08/26/2015 11:51 AM, Daniel Vetter wrote:

On Wed, Aug 26, 2015 at 11:32:30AM -0300, Tiago Vignatti wrote:

...

On 08/26/2015 09:58 AM, Daniel Vetter wrote:

...

The other is that right now there's no user nor implementation in sight which actually does range-based flush optimizations, so I'm pretty much expecting we'll get it wrong. Maybe instead we should go one step further and remove the range from the internal dma-buf interface and also drop it from the ioctl? With the flags we can always add something later on once we have a real user with a clear need for it. But afaik cros only wants to shuffle around entire tiles and has a buffer-per-tile approach.

Thomas, I think Daniel has a point here and also, I wouldn't mind removing all range control from the dma-buf ioctl either.

if we go with nuking it from the ioctl I'd suggest to also nuke it from the dma-buf internal inferface first too.

yep, I can do it.

Thomas, so we leave 2d sync out now?

Tiago