dma-buf: Update docs for SYNC ioctl

Commit Message

Daniel Vetter March 21, 2016, 7:51 a.m. UTC

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 <[e-mail]>
Cc: Tiago Vignatti <[e-mail]>
Cc: Stéphane Marchesin <[e-mail]>
Cc: David Herrmann <[e-mail]>
Cc: Sumit Semwal <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
CC: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: Hans Verkuil <[e-mail]>
Acked-by: Sumit Semwal <[e-mail]>
Signed-off-by: Daniel Vetter <[e-mail]>
---
 Documentation/dma-buf-sharing.txt | 11 ++++++-----
 drivers/dma-buf/dma-buf.c         |  2 +-
 2 files changed, 7 insertions(+), 6 deletions(-)

Comments

Hans Verkuil March 21, 2016, 7:53 a.m. UTC | #1

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 <[e-mail]>
> Cc: Tiago Vignatti <[e-mail]>
> Cc: Stéphane Marchesin <[e-mail]>
> Cc: David Herrmann <[e-mail]>
> Cc: Sumit Semwal <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> CC: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: Hans Verkuil <[e-mail]>
> Acked-by: Sumit Semwal <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>
> ---
>  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 <[e-mail]>

Regards,

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

David Herrmann March 21, 2016, 12:26 p.m. UTC | #2

Hi

On Mon, Mar 21, 2016 at 8:51 AM, Daniel Vetter <[e-mail]> 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 <[e-mail]>
> Cc: Tiago Vignatti <[e-mail]>
> Cc: Stéphane Marchesin <[e-mail]>
> Cc: David Herrmann <[e-mail]>
> Cc: Sumit Semwal <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> CC: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: Hans Verkuil <[e-mail]>
> Acked-by: Sumit Semwal <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>
> ---
>  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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tiago Vignatti March 21, 2016, 1:16 p.m. UTC | #3

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 <[e-mail]>
> Cc: Tiago Vignatti <[e-mail]>
> Cc: Stéphane Marchesin <[e-mail]>
> Cc: David Herrmann <[e-mail]>
> Cc: Sumit Semwal <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> CC: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: Hans Verkuil <[e-mail]>
> Acked-by: Sumit Semwal <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>

Reviewed-by: Tiago Vignatti <[e-mail]>

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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Daniel Vetter March 21, 2016, 5:14 p.m. UTC | #4

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 <[e-mail]> 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 <[e-mail]>
> > Cc: Tiago Vignatti <[e-mail]>
> > Cc: Stéphane Marchesin <[e-mail]>
> > Cc: David Herrmann <[e-mail]>
> > Cc: Sumit Semwal <[e-mail]>
> > Cc: Daniel Vetter <[e-mail]>
> > CC: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: Hans Verkuil <[e-mail]>
> > Acked-by: Sumit Semwal <[e-mail]>
> > Signed-off-by: Daniel Vetter <[e-mail]>
> > ---
> >  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

David Herrmann March 23, 2016, 11:30 a.m. UTC | #5

Hey

On Mon, Mar 21, 2016 at 6:14 PM, Daniel Vetter <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
>> > Cc: Tiago Vignatti <[e-mail]>
>> > Cc: Stéphane Marchesin <[e-mail]>
>> > Cc: David Herrmann <[e-mail]>
>> > Cc: Sumit Semwal <[e-mail]>
>> > Cc: Daniel Vetter <[e-mail]>
>> > CC: [e-mail]
>> > Cc: [e-mail]
>> > Cc: [e-mail]
>> > Cc: [e-mail]
>> > Cc: [e-mail]
>> > Cc: Hans Verkuil <[e-mail]>
>> > Acked-by: Sumit Semwal <[e-mail]>
>> > Signed-off-by: Daniel Vetter <[e-mail]>
>> > ---
>> >  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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chris Wilson March 23, 2016, 11:56 a.m. UTC | #6

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

David Herrmann March 23, 2016, 3:32 p.m. UTC | #7

Hi

On Wed, Mar 23, 2016 at 12:56 PM, Chris Wilson <[e-mail]> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chris Wilson March 23, 2016, 3:42 p.m. UTC | #8

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

Tiago Vignatti March 28, 2016, 7:42 p.m. UTC | #9

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 <[e-mail]> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Herrmann March 29, 2016, 9:47 a.m. UTC | #10

Hi

On Mon, Mar 28, 2016 at 9:42 PM, Tiago Vignatti
<[e-mail]> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tiago Vignatti March 29, 2016, 5:20 p.m. UTC | #11

On 03/29/2016 06:47 AM, David Herrmann wrote:
> Hi
>
> On Mon, Mar 28, 2016 at 9:42 PM, Tiago Vignatti
> <[e-mail]> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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)