Re: [PATCH] dma-buf: Update docs for SYNC ioctl
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
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
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
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 v2 2/2] dma-buf: Add user interfaces for dmabuf sync support
Hi
On Wed, Aug 21, 2013 at 12:33 PM, Inki Dae wrote:
> This patch adds lock and poll callbacks to dma buf file operations,
> and these callbacks will be called by fcntl and select system calls.
>
> fcntl and select system calls can be used to wait for the completion
> of DMA or CPU access to a shared dmabuf. The difference of them is
> fcntl system call takes a lock after the completion but select system
> call doesn't. So in case of fcntl system call, it's useful when a task
> wants to access a shared dmabuf without any broken. On the other hand,
> it's useful when a task wants to just wait for the completion.
1)
So how is that supposed to work in user-space? I don't want to block
on a buffer, but get notified once I can lock it. So I do:
select(..dmabuf..)
Once it is finished, I want to use it:
flock(..dmabuf..)
However, how can I guarantee the flock will not block? Some other
process might have locked it in between. So I do a non-blocking
flock() and if it fails I wait again? Looks ugly and un-predictable.
2)
What do I do if some user-space program holds a lock and dead-locks?
3)
How do we do modesetting in atomic-context in the kernel? There is no
way to lock the object. But this is required for panic-handlers and
more importantly the kdb debugging hooks.
Ok, I can live with that being racy, but would still be nice to be considered.
4)
Why do we need locks? Aren't fences enough? That is, in which
situation is a lock really needed?
If we assume we have two writers A and B (DMA, CPU, GPU, whatever) and
they have no synchronization on their own. What do we win by
synchronizing their writes? Ok, yeah, we end up with either A or B and
not a mixture of both. But if we cannot predict whether we get A or B,
I don't know why we care at all? It's random, so a mixture would be
fine, too, wouldn't it?
So if user-space doesn't have any synchronization on its own, I don't
see why we need an implicit sync on a dma-buf. Could you describe a
more elaborate use-case?
I think the problems we need to fix are read/write syncs. So we have a
write that issues the DMA+write plus a fence and passes the buf plus
fence to the reader. The reader waits for the fence and then issues
the read plus fence. It passes the fence back to the writer. The
writer waits for the fence again and then issues the next write if
required.
This has the following advantages:
- fences are _guaranteed_ to finish in a given time period. Locks, on
the other hand, might never be freed (of the holder dead-locks, for
instance)
- you avoid any stalls. That is, if a writer releases a buffer and
immediately locks it again, the reader side might stall if it didn't
lock it in exactly the given window. You have no control to guarantee
the reader ever gets access. You would need a synchronization in
user-space between the writer and reader to guarantee that. This makes
the whole lock useles, doesn't it?
Cheers
David
> Changelog v2:
> - Add select system call support.
> . The purpose of this feature is to wait for the completion of DMA or
> CPU access to a dmabuf without that caller locks the dmabuf again
> after the completion.
> That is useful when caller wants to be aware of the completion of
> DMA access to the dmabuf, and the caller doesn't use intefaces for
> the DMA device driver.
>
> Signed-off-by: Inki Dae
> Signed-off-by: Kyungmin Park
> ---
> drivers/base/dma-buf.c | 81
>
> 1 files changed, 81 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
> index 4aca57a..f16a396 100644
> --- a/drivers/base/dma-buf.c
> +++ b/drivers/base/dma-buf.c
> @@ -29,6 +29,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> static inline int is_dma_buf_file(struct file *);
> @@ -80,9 +81,89 @@ static int dma_buf_mmap_internal(struct file *file, struct
> vm_area_struct *vma)
> return dmabuf->ops->mmap(dmabuf, vma);
> }
>
> +static unsigned int dma_buf_poll(struct file *filp,
> + struct poll_table_struct *poll)
> +{
> + struct dma_buf *dmabuf;
> + struct dmabuf_sync_reservation *robj;
> + int ret = 0;
> +
> + if (!is_dma_buf_file(filp))
> + return POLLERR;
> +
> + dmabuf = filp->private_data;
> + if (!dmabuf || !dmabuf->sync)
> + return POLLERR;
> +
> + robj = dmabuf->sync;
> +
> + mutex_lock(&robj->lock);
> +
> + robj->polled = true;
> +
> + /*
> +* CPU or DMA access to this buffer has been completed, and
> +* the blocked task has been waked up. Return poll event
> +* so that the task can get out of select().
> +*/
> + if (robj->poll_event) {
> + robj->poll_event = false;
> + mutex_unlock(&robj->lock);
> + return POLLIN | POLLOUT;
> + }
> +
> + /*
> +* There is no anyone accessing this b
Re: dma-buf/fbdev: one-to-many support
Hi Laurent and Alan On Tue, Jul 17, 2012 at 1:24 PM, Alan Cox wrote: >> The main issue is that fbdev has been designed with the implicit assumption >> that an fbdev driver will always own the graphics memory it uses. All >> components in the stack, from drivers to applications, have been designed >> around that assumption. >> >> We could of course fix this, revamp the fbdev API and turn it into a modern >> graphics API, but I really wonder whether it would be worth it. DRM has been >> getting quite a lot of attention lately, especially from embedded developers >> and vendors, and the trend seems to me like the (Linux) world will gradually >> move from fbdev to DRM. >> >> Please feel free to disagree :-) > > I would disagree on the "main issue" bit. All the graphics cards have > their own formats and cache management rules. Simply sharing a buffer > doesn't work - which is why all of the extra gloop will be needed. This is exactly why I suggested adding an "owner" field. A driver could then check whether the buffer it is supposed to share/takeover is from a compatible (or even the same) driver/device. If it is not, it would simply reject using the buffer. Then again, if we have multiple devices that are incompatible, we are still unable to share the buffer. So this attempt would only be useful if we have tons of DisplayLink devices attached that all use the same driver, for example. Regarding DRM: In user-space I prefer DRM over fbdev. With the introduction of the dumb-buffers there isn't even the need to have mesa installed. However, fblog runs in kernel space and currently cannot use DRM as there is no in-kernel DRM API. I looked at drm-fops.c whether it is easy to create a very simple in-kernel API but then I dropped the idea as this might be too complex for a simple debugging-only driver. Another attempt would be making the drm-fb-helper more generic so we can use this layer as in-kernel DRM API. I had a deeper look into this this weekend and so as a summary I think all in-kernel graphics access is probably not worth optimizing it. fbcon is already working great and fblog is only used during boot and oopses/panics and can be restricted to a single device. I will have another look at the drivers in a few weeks but if you tell me that this is not easy to implement, I will probably have to let this idea go. 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
dma-buf/fbdev: one-to-many support
Hi I am currently working on fblog [1] (a replacement for fbcon without VT dependencies) but this questions does also apply to other fbdev users. Is there a way to share framebuffers between fbdev devices? I was thinking especially of USB devices like DisplayLink. If they share the same screen dimensions it would increase performance a lot if I could display a single buffer on all the devices instead of copying it into each framebuffer. I was told to have a look at the dma-buf framework to implement this. However, looking at the fbdev dma-buf support I think that this isn't currently possible. Each fbdev device takes the exporter-role and provides a single dma-buf object. However, if I wanted to share the buffers, I would need to be the exporter. Or there needs to be a way for the fbdev devices to import a dma-buf from other fbdev devices. I also took a short look at DRM prime support and noticed that it is capable of importing buffers (or at least it looks like it is). Therefore, I was wondering whether it does make sense to add an "import dma-buf" callback to fbdev devices and if the fbdev driver supports this, I can simply draw to a single dma-buf from one fbdev device and push it to all other fbdev devices that share the same dimensions. It would also be nice to allow multiple buffer-owners or a way to transfer ownership. That is, if the owner/exporter of the dma-buf vanishes, I would pass it to another fbdev device which would pick it up so I don't have to create a new one. I think this is only interesting for DisplayLink-devices as they are currently the only way to get a bunch of displays connected to a single machine. Anyway, if you think that this isn't worth it, I will probably drop this idea. Regards David [1] fblog kernel driver: http://lwn.net/Articles/505965/ -- 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
