Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Tiago Vignatti]

On 03/29/2016 06:47 AM, David Herrmann wrote:

Hi

On Mon, Mar 28, 2016 at 9:42 PM, Tiago Vignatti
 wrote:

Do we have an agreement here after all? David? I need to know whether this
fixup is okay to go cause I'll need to submit to Chrome OS then.


Sure it is fine. The code is already there, we cannot change it.


ah true. Only now that I've noticed it's already in Linus tree. Thanks 
anyway!


Tiago

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[David Herrmann]

Hi

On Mon, Mar 28, 2016 at 9:42 PM, Tiago Vignatti
 wrote:
> Do we have an agreement here after all? David? I need to know whether this
> fixup is okay to go cause I'll need to submit to Chrome OS then.

Sure it is fine. The code is already there, we cannot change it.

Thanks
David
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Tiago Vignatti]

On 03/23/2016 12:42 PM, Chris Wilson wrote:

On Wed, Mar 23, 2016 at 04:32:59PM +0100, David Herrmann wrote:

Hi

On Wed, Mar 23, 2016 at 12:56 PM, Chris Wilson  wrote:

On Wed, Mar 23, 2016 at 12:30:42PM +0100, David Herrmann wrote:

My question was rather about why we do this? Semantics for EINTR are
well defined, and with SA_RESTART (default on linux) user-space can
ignore it. However, looping on EAGAIN is very uncommon, and it is not
at all clear why it is needed?

Returning an error to user-space makes sense if user-space has a
reason to react to it. I fail to see how EAGAIN on a cache-flush/sync
operation helps user-space at all? As someone without insight into the
driver implementation, it is hard to tell why.. Any hints?


The reason we return EAGAIN is to workaround a deadlock we face when
blocking on the GPU holding the struct_mutex (inside the client's
process), but the GPU is dead. As our locking is very, very coarse we
cannot restart the GPU without acquiring the struct_mutex being held by
the client so we wake the client up and tell them the resource they are
waiting on (the flush of the object from the GPU into the CPU domain) is
temporarily unavailable. If they try to immediately wait upon the ioctl
again, they are blocked waiting for the reset to occur before they may
complete their flush. There are a few other possible deadlocks that are
also avoided with EAGAIN (again, the issue is more or less the lack of
fine grained locking).


...so you hijacked EAGAIN for all DRM ioctls just for a driver
workaround?


No, we utilized the fact that EAGAIN was already enshrined by libdrm as
the defacto mechanism for repeating the ioctl in order to repeat the
ioctl for a driver workaround.


Do we have an agreement here after all? David? I need to know whether 
this fixup is okay to go cause I'll need to submit to Chrome OS then.


Best Regards,

Tiago

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Chris Wilson]

On Wed, Mar 23, 2016 at 04:32:59PM +0100, David Herrmann wrote:
> Hi
> 
> On Wed, Mar 23, 2016 at 12:56 PM, Chris Wilson  
> wrote:
> > On Wed, Mar 23, 2016 at 12:30:42PM +0100, David Herrmann wrote:
> >> My question was rather about why we do this? Semantics for EINTR are
> >> well defined, and with SA_RESTART (default on linux) user-space can
> >> ignore it. However, looping on EAGAIN is very uncommon, and it is not
> >> at all clear why it is needed?
> >>
> >> Returning an error to user-space makes sense if user-space has a
> >> reason to react to it. I fail to see how EAGAIN on a cache-flush/sync
> >> operation helps user-space at all? As someone without insight into the
> >> driver implementation, it is hard to tell why.. Any hints?
> >
> > The reason we return EAGAIN is to workaround a deadlock we face when
> > blocking on the GPU holding the struct_mutex (inside the client's
> > process), but the GPU is dead. As our locking is very, very coarse we
> > cannot restart the GPU without acquiring the struct_mutex being held by
> > the client so we wake the client up and tell them the resource they are
> > waiting on (the flush of the object from the GPU into the CPU domain) is
> > temporarily unavailable. If they try to immediately wait upon the ioctl
> > again, they are blocked waiting for the reset to occur before they may
> > complete their flush. There are a few other possible deadlocks that are
> > also avoided with EAGAIN (again, the issue is more or less the lack of
> > fine grained locking).
> 
> ...so you hijacked EAGAIN for all DRM ioctls just for a driver
> workaround?

No, we utilized the fact that EAGAIN was already enshrined by libdrm as
the defacto mechanism for repeating the ioctl in order to repeat the
ioctl for a driver workaround.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[David Herrmann]

Hi

On Wed, Mar 23, 2016 at 12:56 PM, Chris Wilson  wrote:
> On Wed, Mar 23, 2016 at 12:30:42PM +0100, David Herrmann wrote:
>> My question was rather about why we do this? Semantics for EINTR are
>> well defined, and with SA_RESTART (default on linux) user-space can
>> ignore it. However, looping on EAGAIN is very uncommon, and it is not
>> at all clear why it is needed?
>>
>> Returning an error to user-space makes sense if user-space has a
>> reason to react to it. I fail to see how EAGAIN on a cache-flush/sync
>> operation helps user-space at all? As someone without insight into the
>> driver implementation, it is hard to tell why.. Any hints?
>
> The reason we return EAGAIN is to workaround a deadlock we face when
> blocking on the GPU holding the struct_mutex (inside the client's
> process), but the GPU is dead. As our locking is very, very coarse we
> cannot restart the GPU without acquiring the struct_mutex being held by
> the client so we wake the client up and tell them the resource they are
> waiting on (the flush of the object from the GPU into the CPU domain) is
> temporarily unavailable. If they try to immediately wait upon the ioctl
> again, they are blocked waiting for the reset to occur before they may
> complete their flush. There are a few other possible deadlocks that are
> also avoided with EAGAIN (again, the issue is more or less the lack of
> fine grained locking).

...so you hijacked EAGAIN for all DRM ioctls just for a driver
workaround? EAGAIN is universally used to signal the caller about a
blocking resource. It is very much linked to O_NONBLOCK. Why not use
EBUSY, ECANCELED, ECOMM, EDEADLOCK, EIO, EL3RST, ...

Anyhow, I guess that ship has sailed. But just mentioning EAGAIN in a
kernel-doc is way to vague for user-space to figure out they should
loop on it.

Thanks
David
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Chris Wilson]

On Wed, Mar 23, 2016 at 12:30:42PM +0100, David Herrmann wrote:
> My question was rather about why we do this? Semantics for EINTR are
> well defined, and with SA_RESTART (default on linux) user-space can
> ignore it. However, looping on EAGAIN is very uncommon, and it is not
> at all clear why it is needed?
> 
> Returning an error to user-space makes sense if user-space has a
> reason to react to it. I fail to see how EAGAIN on a cache-flush/sync
> operation helps user-space at all? As someone without insight into the
> driver implementation, it is hard to tell why.. Any hints?

The reason we return EAGAIN is to workaround a deadlock we face when
blocking on the GPU holding the struct_mutex (inside the client's
process), but the GPU is dead. As our locking is very, very coarse we
cannot restart the GPU without acquiring the struct_mutex being held by
the client so we wake the client up and tell them the resource they are
waiting on (the flush of the object from the GPU into the CPU domain) is
temporarily unavailable. If they try to immediately wait upon the ioctl
again, they are blocked waiting for the reset to occur before they may
complete their flush. There are a few other possible deadlocks that are
also avoided with EAGAIN (again, the issue is more or less the lack of
fine grained locking).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[David Herrmann]

Hey

On Mon, Mar 21, 2016 at 6:14 PM, Daniel Vetter  wrote:
> On Mon, Mar 21, 2016 at 01:26:58PM +0100, David Herrmann wrote:
>> Hi
>>
>> On Mon, Mar 21, 2016 at 8:51 AM, Daniel Vetter  
>> wrote:
>> > Just a bit of wording polish plus mentioning that it can fail and must
>> > be restarted.
>> >
>> > Requested by Sumit.
>> >
>> > v2: Fix them typos (Hans).
>> >
>> > Cc: Chris Wilson 
>> > Cc: Tiago Vignatti 
>> > Cc: Stéphane Marchesin 
>> > Cc: David Herrmann 
>> > Cc: Sumit Semwal 
>> > Cc: Daniel Vetter 
>> > CC: linux-media@vger.kernel.org
>> > Cc: dri-de...@lists.freedesktop.org
>> > Cc: linaro-mm-...@lists.linaro.org
>> > Cc: intel-...@lists.freedesktop.org
>> > Cc: de...@driverdev.osuosl.org
>> > Cc: Hans Verkuil 
>> > Acked-by: Sumit Semwal 
>> > Signed-off-by: Daniel Vetter 
>> > ---
>> >  Documentation/dma-buf-sharing.txt | 11 ++-
>> >  drivers/dma-buf/dma-buf.c |  2 +-
>> >  2 files changed, 7 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/Documentation/dma-buf-sharing.txt 
>> > b/Documentation/dma-buf-sharing.txt
>> > index 32ac32e773e1..ca44c5820585 100644
>> > --- a/Documentation/dma-buf-sharing.txt
>> > +++ b/Documentation/dma-buf-sharing.txt
>> > @@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 
>> > 2 main use-cases:
>> >
>> > No special interfaces, userspace simply calls mmap on the dma-buf fd, 
>> > making
>> > sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is 
>> > *always*
>> > -   used when the access happens. This is discussed next paragraphs.
>> > +   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail 
>> > with
>> > +   -EAGAIN or -EINTR, in which case it must be restarted.
>>
>> What is "restart on EAGAIN" supposed to mean? Or more generally, what
>> does EAGAIN tell the caller?
>
> Do what drmIoctl does essentially.
>
> while (ret == -1 && (errno == EAGAIN || errno == EINTR)
> ret = ioctl();
>
> Typed from memery, too lazy to look it up in the source ;-) I'm trying to
> sell the idea of a real dma-buf manpage to Sumit, we should clarify this
> in detail there.

My question was rather about why we do this? Semantics for EINTR are
well defined, and with SA_RESTART (default on linux) user-space can
ignore it. However, looping on EAGAIN is very uncommon, and it is not
at all clear why it is needed?

Returning an error to user-space makes sense if user-space has a
reason to react to it. I fail to see how EAGAIN on a cache-flush/sync
operation helps user-space at all? As someone without insight into the
driver implementation, it is hard to tell why.. Any hints?

Thanks
David
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Daniel Vetter]

On Mon, Mar 21, 2016 at 01:26:58PM +0100, David Herrmann wrote:
> Hi
> 
> On Mon, Mar 21, 2016 at 8:51 AM, Daniel Vetter  wrote:
> > Just a bit of wording polish plus mentioning that it can fail and must
> > be restarted.
> >
> > Requested by Sumit.
> >
> > v2: Fix them typos (Hans).
> >
> > Cc: Chris Wilson 
> > Cc: Tiago Vignatti 
> > Cc: Stéphane Marchesin 
> > Cc: David Herrmann 
> > Cc: Sumit Semwal 
> > Cc: Daniel Vetter 
> > CC: linux-media@vger.kernel.org
> > Cc: dri-de...@lists.freedesktop.org
> > Cc: linaro-mm-...@lists.linaro.org
> > Cc: intel-...@lists.freedesktop.org
> > Cc: de...@driverdev.osuosl.org
> > Cc: Hans Verkuil 
> > Acked-by: Sumit Semwal 
> > Signed-off-by: Daniel Vetter 
> > ---
> >  Documentation/dma-buf-sharing.txt | 11 ++-
> >  drivers/dma-buf/dma-buf.c |  2 +-
> >  2 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/dma-buf-sharing.txt 
> > b/Documentation/dma-buf-sharing.txt
> > index 32ac32e773e1..ca44c5820585 100644
> > --- a/Documentation/dma-buf-sharing.txt
> > +++ b/Documentation/dma-buf-sharing.txt
> > @@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 
> > 2 main use-cases:
> >
> > No special interfaces, userspace simply calls mmap on the dma-buf fd, 
> > making
> > sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is 
> > *always*
> > -   used when the access happens. This is discussed next paragraphs.
> > +   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail with
> > +   -EAGAIN or -EINTR, in which case it must be restarted.
> 
> What is "restart on EAGAIN" supposed to mean? Or more generally, what
> does EAGAIN tell the caller?

Do what drmIoctl does essentially.

while (ret == -1 && (errno == EAGAIN || errno == EINTR)
ret = ioctl();

Typed from memery, too lazy to look it up in the source ;-) I'm trying to
sell the idea of a real dma-buf manpage to Sumit, we should clarify this
in detail there.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Tiago Vignatti]

On 03/21/2016 04:51 AM, Daniel Vetter wrote:

Just a bit of wording polish plus mentioning that it can fail and must
be restarted.

Requested by Sumit.

v2: Fix them typos (Hans).

Cc: Chris Wilson 
Cc: Tiago Vignatti 
Cc: Stéphane Marchesin 
Cc: David Herrmann 
Cc: Sumit Semwal 
Cc: Daniel Vetter 
CC: linux-media@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Cc: linaro-mm-...@lists.linaro.org
Cc: intel-...@lists.freedesktop.org
Cc: de...@driverdev.osuosl.org
Cc: Hans Verkuil 
Acked-by: Sumit Semwal 
Signed-off-by: Daniel Vetter 


Reviewed-by: Tiago Vignatti 

Best regards,

Tiago



---
  Documentation/dma-buf-sharing.txt | 11 ++-
  drivers/dma-buf/dma-buf.c |  2 +-
  2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/dma-buf-sharing.txt 
b/Documentation/dma-buf-sharing.txt
index 32ac32e773e1..ca44c5820585 100644
--- a/Documentation/dma-buf-sharing.txt
+++ b/Documentation/dma-buf-sharing.txt
@@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 2 
main use-cases:

 No special interfaces, userspace simply calls mmap on the dma-buf fd, 
making
 sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always*
-   used when the access happens. This is discussed next paragraphs.
+   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail with
+   -EAGAIN or -EINTR, in which case it must be restarted.

 Some systems 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
@@ -366,10 +367,10 @@ Being able to mmap an export dma-buf buffer object has 2 
main use-cases:
 want (with the new data being consumed by the GPU or say scanout 
device)
   - munmap once you don't need the buffer any more

-Therefore, for correctness and optimal performance, systems with the memory
-cache shared by the GPU and CPU i.e. the "coherent" and also the
-"incoherent" are always required to use SYNC_START and SYNC_END before and
-after, respectively, when accessing the mapped address.
+For correctness and optimal performance, it is always required to use
+SYNC_START and SYNC_END before and after, respectively, when accessing the
+mapped address. Userspace cannot rely on coherent access, even when there
+are systems where it just works without calling these ioctls.

  2. Supporting existing mmap interfaces in importers

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 774a60f4309a..4a2c07ee6677 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -612,7 +612,7 @@ EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
   * @dmabuf:   [in]buffer to complete cpu access for.
   * @direction:[in]length of range for cpu access.
   *
- * This call must always succeed.
+ * Can return negative error values, returns 0 on success.
   */
  int dma_buf_end_cpu_access(struct dma_buf *dmabuf,
   enum dma_data_direction direction)



--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[David Herrmann]

Hi

On Mon, Mar 21, 2016 at 8:51 AM, Daniel Vetter  wrote:
> Just a bit of wording polish plus mentioning that it can fail and must
> be restarted.
>
> Requested by Sumit.
>
> v2: Fix them typos (Hans).
>
> Cc: Chris Wilson 
> Cc: Tiago Vignatti 
> Cc: Stéphane Marchesin 
> Cc: David Herrmann 
> Cc: Sumit Semwal 
> Cc: Daniel Vetter 
> CC: linux-media@vger.kernel.org
> Cc: dri-de...@lists.freedesktop.org
> Cc: linaro-mm-...@lists.linaro.org
> Cc: intel-...@lists.freedesktop.org
> Cc: de...@driverdev.osuosl.org
> Cc: Hans Verkuil 
> Acked-by: Sumit Semwal 
> Signed-off-by: Daniel Vetter 
> ---
>  Documentation/dma-buf-sharing.txt | 11 ++-
>  drivers/dma-buf/dma-buf.c |  2 +-
>  2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/dma-buf-sharing.txt 
> b/Documentation/dma-buf-sharing.txt
> index 32ac32e773e1..ca44c5820585 100644
> --- a/Documentation/dma-buf-sharing.txt
> +++ b/Documentation/dma-buf-sharing.txt
> @@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 2 
> main use-cases:
>
> No special interfaces, userspace simply calls mmap on the dma-buf fd, 
> making
> sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always*
> -   used when the access happens. This is discussed next paragraphs.
> +   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail with
> +   -EAGAIN or -EINTR, in which case it must be restarted.

What is "restart on EAGAIN" supposed to mean? Or more generally, what
does EAGAIN tell the caller?

Thanks
David

> Some systems 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
> @@ -366,10 +367,10 @@ Being able to mmap an export dma-buf buffer object has 
> 2 main use-cases:
> want (with the new data being consumed by the GPU or say scanout 
> device)
>   - munmap once you don't need the buffer any more
>
> -Therefore, for correctness and optimal performance, systems with the 
> memory
> -cache shared by the GPU and CPU i.e. the "coherent" and also the
> -"incoherent" are always required to use SYNC_START and SYNC_END before 
> and
> -after, respectively, when accessing the mapped address.
> +For correctness and optimal performance, it is always required to use
> +SYNC_START and SYNC_END before and after, respectively, when accessing 
> the
> +mapped address. Userspace cannot rely on coherent access, even when there
> +are systems where it just works without calling these ioctls.
>
>  2. Supporting existing mmap interfaces in importers
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 774a60f4309a..4a2c07ee6677 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -612,7 +612,7 @@ EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
>   * @dmabuf:[in]buffer to complete cpu access for.
>   * @direction: [in]length of range for cpu access.
>   *
> - * This call must always succeed.
> + * Can return negative error values, returns 0 on success.
>   */
>  int dma_buf_end_cpu_access(struct dma_buf *dmabuf,
>enum dma_data_direction direction)
> --
> 2.8.0.rc3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Hans Verkuil]

On 03/21/2016 08:51 AM, Daniel Vetter wrote:
> Just a bit of wording polish plus mentioning that it can fail and must
> be restarted.
> 
> Requested by Sumit.
> 
> v2: Fix them typos (Hans).
> 
> Cc: Chris Wilson 
> Cc: Tiago Vignatti 
> Cc: Stéphane Marchesin 
> Cc: David Herrmann 
> Cc: Sumit Semwal 
> Cc: Daniel Vetter 
> CC: linux-media@vger.kernel.org
> Cc: dri-de...@lists.freedesktop.org
> Cc: linaro-mm-...@lists.linaro.org
> Cc: intel-...@lists.freedesktop.org
> Cc: de...@driverdev.osuosl.org
> Cc: Hans Verkuil 
> Acked-by: Sumit Semwal 
> Signed-off-by: Daniel Vetter 
> ---
>  Documentation/dma-buf-sharing.txt | 11 ++-
>  drivers/dma-buf/dma-buf.c |  2 +-
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/dma-buf-sharing.txt 
> b/Documentation/dma-buf-sharing.txt
> index 32ac32e773e1..ca44c5820585 100644
> --- a/Documentation/dma-buf-sharing.txt
> +++ b/Documentation/dma-buf-sharing.txt
> @@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 2 
> main use-cases:
>  
> No special interfaces, userspace simply calls mmap on the dma-buf fd, 
> making
> sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always*
> -   used when the access happens. This is discussed next paragraphs.
> +   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail with
> +   -EAGAIN or -EINTR, in which case it must be restarted.
>  
> Some systems 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
> @@ -366,10 +367,10 @@ Being able to mmap an export dma-buf buffer object has 
> 2 main use-cases:
> want (with the new data being consumed by the GPU or say scanout 
> device)
>   - munmap once you don't need the buffer any more
>  
> -Therefore, for correctness and optimal performance, systems with the 
> memory
> -cache shared by the GPU and CPU i.e. the "coherent" and also the
> -"incoherent" are always required to use SYNC_START and SYNC_END before 
> and
> -after, respectively, when accessing the mapped address.
> +For correctness and optimal performance, it is always required to use
> +SYNC_START and SYNC_END before and after, respectively, when accessing 
> the
> +mapped address. Userspace cannot rely on coherent access, even when there
> +are systems where it just works without calling these ioctls.
>  
>  2. Supporting existing mmap interfaces in importers
>  
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 774a60f4309a..4a2c07ee6677 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -612,7 +612,7 @@ EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
>   * @dmabuf:  [in]buffer to complete cpu access for.
>   * @direction:   [in]length of range for cpu access.
>   *
> - * This call must always succeed.
> + * Can return negative error values, returns 0 on success.
>   */
>  int dma_buf_end_cpu_access(struct dma_buf *dmabuf,
>  enum dma_data_direction direction)
> 

Much better :-)

Acked-by: Hans Verkuil 

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Daniel Vetter]

On Mon, Mar 21, 2016 at 8:40 AM, Hans Verkuil  wrote:
>> +For correctness and optimal performance, it is always required to use
>> +SYNC_START and SYNC_END before and after, respectively, when accessing 
>> the
>> +mapped address. Userspace cannot on coherent access, even when there are
>
> "Userspace cannot on coherent access"? Do you mean "cannot do"? Sorry, the
> meaning isn't clear to me.

"cannot rely on". I'll send out v2 asap (and let's hope the coffee
works this time around).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Hans Verkuil]

Hi Daniel,

Two small comments:

On 03/21/2016 08:30 AM, Daniel Vetter wrote:
> Just a bit of wording polish plus mentioning that it can fail and must
> be restarted.
> 
> Requested by Sumit.
> 
> Cc: Chris Wilson 
> Cc: Tiago Vignatti 
> Cc: Stéphane Marchesin 
> Cc: David Herrmann 
> Cc: Sumit Semwal 
> Cc: Daniel Vetter 
> CC: linux-media@vger.kernel.org
> Cc: dri-de...@lists.freedesktop.org
> Cc: linaro-mm-...@lists.linaro.org
> Cc: intel-...@lists.freedesktop.org
> Cc: de...@driverdev.osuosl.org
> Signed-off-by: Daniel Vetter 
> ---
>  Documentation/dma-buf-sharing.txt | 11 ++-
>  drivers/dma-buf/dma-buf.c |  2 +-
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/dma-buf-sharing.txt 
> b/Documentation/dma-buf-sharing.txt
> index 32ac32e773e1..5c4e3e586ec8 100644
> --- a/Documentation/dma-buf-sharing.txt
> +++ b/Documentation/dma-buf-sharing.txt
> @@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 2 
> main use-cases:
>  
> No special interfaces, userspace simply calls mmap on the dma-buf fd, 
> making
> sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always*
> -   used when the access happens. This is discussed next paragraphs.
> +   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail with
> +   -EGAIN or -EINTR, in which case it must be restarted.

EGAIN -> EAGAIN

>  
> Some systems 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
> @@ -366,10 +367,10 @@ Being able to mmap an export dma-buf buffer object has 
> 2 main use-cases:
> want (with the new data being consumed by the GPU or say scanout 
> device)
>   - munmap once you don't need the buffer any more
>  
> -Therefore, for correctness and optimal performance, systems with the 
> memory
> -cache shared by the GPU and CPU i.e. the "coherent" and also the
> -"incoherent" are always required to use SYNC_START and SYNC_END before 
> and
> -after, respectively, when accessing the mapped address.
> +For correctness and optimal performance, it is always required to use
> +SYNC_START and SYNC_END before and after, respectively, when accessing 
> the
> +mapped address. Userspace cannot on coherent access, even when there are

"Userspace cannot on coherent access"? Do you mean "cannot do"? Sorry, the
meaning isn't clear to me.

Regards,

Hans

> +systems where it just works without calling these ioctls.
>  
>  2. Supporting existing mmap interfaces in importers
>  
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 774a60f4309a..4a2c07ee6677 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -612,7 +612,7 @@ EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
>   * @dmabuf:  [in]buffer to complete cpu access for.
>   * @direction:   [in]length of range for cpu access.
>   *
> - * This call must always succeed.
> + * Can return negative error values, returns 0 on success.
>   */
>  int dma_buf_end_cpu_access(struct dma_buf *dmabuf,
>  enum dma_data_direction direction)
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Sumit Semwal]

On 21 March 2016 at 13:00, Daniel Vetter  wrote:
> Just a bit of wording polish plus mentioning that it can fail and must
> be restarted.
>
> Requested by Sumit.
>
> Cc: Chris Wilson 
> Cc: Tiago Vignatti 
> Cc: Stéphane Marchesin 
> Cc: David Herrmann 
> Cc: Sumit Semwal 
> Cc: Daniel Vetter 
> CC: linux-media@vger.kernel.org
> Cc: dri-de...@lists.freedesktop.org
> Cc: linaro-mm-...@lists.linaro.org
> Cc: intel-...@lists.freedesktop.org
> Cc: de...@driverdev.osuosl.org
> Signed-off-by: Daniel Vetter 
Acked-by: Sumit Semwal 

> ---
>  Documentation/dma-buf-sharing.txt | 11 ++-
>  drivers/dma-buf/dma-buf.c |  2 +-
>  2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/dma-buf-sharing.txt 
> b/Documentation/dma-buf-sharing.txt
> index 32ac32e773e1..5c4e3e586ec8 100644
> --- a/Documentation/dma-buf-sharing.txt
> +++ b/Documentation/dma-buf-sharing.txt
> @@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 2 
> main use-cases:
>
> No special interfaces, userspace simply calls mmap on the dma-buf fd, 
> making
> sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always*
> -   used when the access happens. This is discussed next paragraphs.
> +   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail with
> +   -EGAIN or -EINTR, in which case it must be restarted.
>
> Some systems 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
> @@ -366,10 +367,10 @@ Being able to mmap an export dma-buf buffer object has 
> 2 main use-cases:
> want (with the new data being consumed by the GPU or say scanout 
> device)
>   - munmap once you don't need the buffer any more
>
> -Therefore, for correctness and optimal performance, systems with the 
> memory
> -cache shared by the GPU and CPU i.e. the "coherent" and also the
> -"incoherent" are always required to use SYNC_START and SYNC_END before 
> and
> -after, respectively, when accessing the mapped address.
> +For correctness and optimal performance, it is always required to use
> +SYNC_START and SYNC_END before and after, respectively, when accessing 
> the
> +mapped address. Userspace cannot on coherent access, even when there are
> +systems where it just works without calling these ioctls.
>
>  2. Supporting existing mmap interfaces in importers
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 774a60f4309a..4a2c07ee6677 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -612,7 +612,7 @@ EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
>   * @dmabuf:[in]buffer to complete cpu access for.
>   * @direction: [in]length of range for cpu access.
>   *
> - * This call must always succeed.
> + * Can return negative error values, returns 0 on success.
>   */
>  int dma_buf_end_cpu_access(struct dma_buf *dmabuf,
>enum dma_data_direction direction)
> --
> 2.8.0.rc3
>

Best regards,
Sumit.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html