Re: [PATCH] Add virtio gpu driver.
On Tue, May 31, 2016 at 8:29 AM, Gerd Hoffmann wrote: >> but it also means that >> atomic userspace can't use hotspots before we add properties to fbs. And >> doing that is a bit tricky since drm_framebuffer objects are meant to be >> invariant - this assumption is deeply in-grained into the code all over >> the place, everything just compares pointers when semantically it means to >> compare the entire fb (including backing storage pointer/offsets and >> everything). > > Hmm, the hotspot location for a given cursor image is invariant too, so > I don't think that would be a big issue. > > I'd expect userspace define a bunch of cursors, then switch between them > (instead of defining a single cursor, then constantly updating it). Agreed, conceptually it would be a really nice fit to put the hotspot into the invariant drm_framebuffer. I just meant to say that you need to add a bit of new uapi to make it happen, since we can't reuse our existing get/set_prop functions (since those only change props after the object is created). And we also can't use our existing ioctls to enumerate properties of an object (since chicken-egg: you want the list of properties before creating a new framebuffer, so can't use that framebuffer to enumerate them). But really not a big concern. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 7/7] [wip] virtio-gpu: add page flip support
On Tue, May 31, 2016 at 8:18 AM, Gerd Hoffmann wrote: >> > https://lists.freedesktop.org/archives/dri-devel/2016-May/108772.html >> >> Hm, smells more like virtio isn't too happy with the default ordering of >> the commit operation. The default is: >> >> - Disable any crtc/encoders that need to be disabled/change. >> - Bash new plane setup into hw. >> - Enable all crtcs/encoders that need to be enabled/have changed. >> >> There's two problems: >> - some hw gets real grumpy if you bash in plane state without the crtc >> state yet matching. >> - if you do runtime pm nothing is enabled and the writes get lost at best, >> or hang your interconnect at worst. >> >> That's why you can overwrite atomic_commit_tail, and use something more >> sensible. See for example what it looks like for rockchip. I have a gut >> feeling that should also take care of your troubles. Using the old crtc is >> definitely not what you want. > >> Another option is that virtio isn't happy about bashing in plane state for >> disabled crtc. Again helpers have you covered, look at the active_only >> parameter for drm_atomic_helper_commit_planes(). > > virtio-gpu is a bit simplified compared to real hardware, so there isn't > really separate plane/crtc state. > > Right now the virtual outputs are linked to drm_crtc. To apply any > changes I need to lookup the crtc to figure which virtual output should > be updated. > > So, setting active_only should make sure I have a valid crtc pointer on > plane updates, right? It probably also skips the disable + enable crtc > steps on commit? What happens when outputs are disabled? > > Maybe it makes sense to link our virtual outputs to (primary) planes not > crtcs? Nah, I just misunderstood your patch. If it's all about finding the corresponding crtc, then you're all good. I thought there was some other reason (like the virtual hw getting upset about certain things). I'll pick up your patch into my nonblocking atomic branch to make sure virtio isn't accidentally broken. btw can you pls drop an ack or r-b onto my virtio conversion? I already added your tested-by. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] Add virtio gpu driver.
Hi, > > Why attach the hotspot to the plane? Wouldn't it make more sense to > > make it a framebuffer property? > > We don't have properties on the framebuffer. I guess you /could/ just add > it internally to struct drm_framebuffer, and not bother exposing to > userspace. I guess that would be a lot simpler, Yes. I can simply stick the hotspot into drm_framebuffer in drm_mode_cursor_universal() and pick up the values in the driver's plane update function. > but it also means that > atomic userspace can't use hotspots before we add properties to fbs. And > doing that is a bit tricky since drm_framebuffer objects are meant to be > invariant - this assumption is deeply in-grained into the code all over > the place, everything just compares pointers when semantically it means to > compare the entire fb (including backing storage pointer/offsets and > everything). Hmm, the hotspot location for a given cursor image is invariant too, so I don't think that would be a big issue. I'd expect userspace define a bunch of cursors, then switch between them (instead of defining a single cursor, then constantly updating it). cheers, Gerd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 7/7] [wip] virtio-gpu: add page flip support
Hi, > > https://lists.freedesktop.org/archives/dri-devel/2016-May/108772.html > > Hm, smells more like virtio isn't too happy with the default ordering of > the commit operation. The default is: > > - Disable any crtc/encoders that need to be disabled/change. > - Bash new plane setup into hw. > - Enable all crtcs/encoders that need to be enabled/have changed. > > There's two problems: > - some hw gets real grumpy if you bash in plane state without the crtc > state yet matching. > - if you do runtime pm nothing is enabled and the writes get lost at best, > or hang your interconnect at worst. > > That's why you can overwrite atomic_commit_tail, and use something more > sensible. See for example what it looks like for rockchip. I have a gut > feeling that should also take care of your troubles. Using the old crtc is > definitely not what you want. > Another option is that virtio isn't happy about bashing in plane state for > disabled crtc. Again helpers have you covered, look at the active_only > parameter for drm_atomic_helper_commit_planes(). virtio-gpu is a bit simplified compared to real hardware, so there isn't really separate plane/crtc state. Right now the virtual outputs are linked to drm_crtc. To apply any changes I need to lookup the crtc to figure which virtual output should be updated. So, setting active_only should make sure I have a valid crtc pointer on plane updates, right? It probably also skips the disable + enable crtc steps on commit? What happens when outputs are disabled? Maybe it makes sense to link our virtual outputs to (primary) planes not crtcs? cheers, Gerd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 2/2] vhost_net: conditionally enable tx polling
On 2016年05月30日 23:55, Michael S. Tsirkin wrote:
On Mon, May 30, 2016 at 02:47:54AM -0400, Jason Wang wrote:
We always poll tx for socket, this is sub optimal since:
- it will be only used when we exceed the sndbuf of the socket.
- since we use two independent polls for tx and vq, this will slightly
increase the waitqueue traversing time and more important, vhost
could not benefit from commit
9e641bdcfa4ef4d6e2fbaa59c1be0ad5d1551fd5 ("net-tun: restructure
tun_do_read for better sleep/wakeup efficiency") even if we've
stopped rx polling during handle_rx since tx poll were still left in
the waitqueue.
Why is this an issue?
sock_def_write_space only wakes up when queue is half empty,
not on each packet.
if ((atomic_read(&sk->sk_wmem_alloc) << 1) <= sk->sk_sndbuf)
I suspect the issue is with your previous patch,
it now pokes at the spinlock on data path
where it used not to.
Is that right?
The problem is not tx wake up but still rx wake up. Patch 1 removes rx
poll, but still left tx poll. So in sock_def_readable(),
skwq_has_sleeper() returns true, we still need to traverse waitqueue and
touch spinlocks. With this patch, unless a heavy tx load, tx poll were
disabled, sock_def_readable() can return finish very soon.
Fix this by conditionally enable tx polling only when -EAGAIN were
met.
Test shows about 8% improvement on guest rx pps.
Before: ~135
After: ~146
Signed-off-by: Jason Wang
---
drivers/vhost/net.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index e91603b..5a05fa0 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -378,6 +378,7 @@ static void handle_tx(struct vhost_net *net)
goto out;
vhost_disable_notify(&net->dev, vq);
+ vhost_net_disable_vq(net, vq);
hdr_size = nvq->vhost_hlen;
zcopy = nvq->ubufs;
@@ -459,6 +460,8 @@ static void handle_tx(struct vhost_net *net)
% UIO_MAXIOV;
}
vhost_discard_vq_desc(vq, 1);
+ if (err == -EAGAIN)
+ vhost_net_enable_vq(net, vq);
break;
}
if (err != len)
--
1.8.3.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 1/2] vhost_net: stop polling socket during rx processing
On 2016年05月30日 23:47, Michael S. Tsirkin wrote:
On Mon, May 30, 2016 at 02:47:53AM -0400, Jason Wang wrote:
We don't stop rx polling socket during rx processing, this will lead
unnecessary wakeups from under layer net devices (E.g
sock_def_readable() form tun). Rx will be slowed down in this
way. This patch avoids this by stop polling socket during rx
processing. A small drawback is that this introduces some overheads in
light load case because of the extra start/stop polling, but single
netperf TCP_RR does not notice any change. In a super heavy load case,
e.g using pktgen to inject packet to guest, we get about ~8.8%
improvement on pps:
before: ~124 pkt/s
after: ~135 pkt/s
Signed-off-by: Jason Wang
---
drivers/vhost/net.c | 56 +++--
1 file changed, 29 insertions(+), 27 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 10ff494..e91603b 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -301,6 +301,32 @@ static bool vhost_can_busy_poll(struct vhost_dev *dev,
!vhost_has_work(dev);
}
+static void vhost_net_disable_vq(struct vhost_net *n,
+struct vhost_virtqueue *vq)
+{
+ struct vhost_net_virtqueue *nvq =
+ container_of(vq, struct vhost_net_virtqueue, vq);
+ struct vhost_poll *poll = n->poll + (nvq - n->vqs);
+ if (!vq->private_data)
+ return;
+ vhost_poll_stop(poll);
+}
+
+static int vhost_net_enable_vq(struct vhost_net *n,
+ struct vhost_virtqueue *vq)
+{
+ struct vhost_net_virtqueue *nvq =
+ container_of(vq, struct vhost_net_virtqueue, vq);
+ struct vhost_poll *poll = n->poll + (nvq - n->vqs);
+ struct socket *sock;
+
+ sock = vq->private_data;
+ if (!sock)
+ return 0;
+
+ return vhost_poll_start(poll, sock->file);
+}
+
static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
struct vhost_virtqueue *vq,
struct iovec iov[], unsigned int iov_size,
BTW we might want to rename these functions, name no longer
reflects function ...
Do you mean adding something reflect busy polling in the name? Then the
name may be too long or have suggestion on the name?
@@ -627,6 +653,7 @@ static void handle_rx(struct vhost_net *net)
if (!sock)
goto out;
vhost_disable_notify(&net->dev, vq);
+ vhost_net_disable_vq(net, vq);
vhost_hlen = nvq->vhost_hlen;
sock_hlen = nvq->sock_hlen;
@@ -715,9 +742,10 @@ static void handle_rx(struct vhost_net *net)
total_len += vhost_len;
if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
vhost_poll_queue(&vq->poll);
- break;
+ goto out;
}
}
+ vhost_net_enable_vq(net, vq);
OK so if sock is readable but RX VQ is empty, this will
immediately schedule another round of handle_rx and so ad
infinitum,
Looks like a bug.
Yes it is, will change the above headcount check to:
/* OK, now we need to know about added descriptors. */
if (!headcount) {
if (unlikely(vhost_enable_notify(&net->dev, vq))) {
/* They have slipped one in as we were
* doing that: check again. */
vhost_disable_notify(&net->dev, vq);
continue;
}
/* Nothing new? Wait for eventfd to tell us
* they refilled. */
goto out;
}
out:
mutex_unlock(&vq->mutex);
}
@@ -796,32 +824,6 @@ static int vhost_net_open(struct inode *inode, struct file
*f)
return 0;
}
-static void vhost_net_disable_vq(struct vhost_net *n,
-struct vhost_virtqueue *vq)
-{
- struct vhost_net_virtqueue *nvq =
- container_of(vq, struct vhost_net_virtqueue, vq);
- struct vhost_poll *poll = n->poll + (nvq - n->vqs);
- if (!vq->private_data)
- return;
- vhost_poll_stop(poll);
-}
-
-static int vhost_net_enable_vq(struct vhost_net *n,
- struct vhost_virtqueue *vq)
-{
- struct vhost_net_virtqueue *nvq =
- container_of(vq, struct vhost_net_virtqueue, vq);
- struct vhost_poll *poll = n->poll + (nvq - n->vqs);
- struct socket *sock;
-
- sock = vq->private_data;
- if (!sock)
- return 0;
-
- return vhost_poll_start(poll, sock->file);
-}
-
static struct socket *vhost_net_stop_vq(struct vhost_net *n,
struct vhost_virtqueue *vq)
{
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to maj
[PATCH v6v3 02/12] mm: migrate: support non-lru movable page migration
Per Vlastimi's review comment. Thanks for the detail review, Vlastimi! If you have another concern, feel free to say. After I resolve all thing, I will send v7 rebased on recent mmotm. >From b14aab2d3ac0702c7b2eec36409d74406d43 Mon Sep 17 00:00:00 2001 From: Minchan Kim Date: Fri, 8 Apr 2016 10:34:49 +0900 Subject: [PATCH] mm: migrate: support non-lru movable page migration We have allowed migration for only LRU pages until now and it was enough to make high-order pages. But recently, embedded system(e.g., webOS, android) uses lots of non-movable pages(e.g., zram, GPU memory) so we have seen several reports about troubles of small high-order allocation. For fixing the problem, there were several efforts (e,g,. enhance compaction algorithm, SLUB fallback to 0-order page, reserved memory, vmalloc and so on) but if there are lots of non-movable pages in system, their solutions are void in the long run. So, this patch is to support facility to change non-movable pages with movable. For the feature, this patch introduces functions related to migration to address_space_operations as well as some page flags. If a driver want to make own pages movable, it should define three functions which are function pointers of struct address_space_operations. 1. bool (*isolate_page) (struct page *page, isolate_mode_t mode); What VM expects on isolate_page function of driver is to return *true* if driver isolates page successfully. On returing true, VM marks the page as PG_isolated so concurrent isolation in several CPUs skip the page for isolation. If a driver cannot isolate the page, it should return *false*. Once page is successfully isolated, VM uses page.lru fields so driver shouldn't expect to preserve values in that fields. 2. int (*migratepage) (struct address_space *mapping, struct page *newpage, struct page *oldpage, enum migrate_mode); After isolation, VM calls migratepage of driver with isolated page. The function of migratepage is to move content of the old page to new page and set up fields of struct page newpage. Keep in mind that you should indicate to the VM the oldpage is no longer movable via __ClearPageMovable() under page_lock if you migrated the oldpage successfully and returns 0. If driver cannot migrate the page at the moment, driver can return -EAGAIN. On -EAGAIN, VM will retry page migration in a short time because VM interprets -EAGAIN as "temporal migration failure". On returning any error except -EAGAIN, VM will give up the page migration without retrying in this time. Driver shouldn't touch page.lru field VM using in the functions. 3. void (*putback_page)(struct page *); If migration fails on isolated page, VM should return the isolated page to the driver so VM calls driver's putback_page with migration failed page. In this function, driver should put the isolated page back to the own data structure. 4. non-lru movable page flags There are two page flags for supporting non-lru movable page. * PG_movable Driver should use the below function to make page movable under page_lock. void __SetPageMovable(struct page *page, struct address_space *mapping) It needs argument of address_space for registering migration family functions which will be called by VM. Exactly speaking, PG_movable is not a real flag of struct page. Rather than, VM reuses page->mapping's lower bits to represent it. #define PAGE_MAPPING_MOVABLE 0x2 page->mapping = page->mapping | PAGE_MAPPING_MOVABLE; so driver shouldn't access page->mapping directly. Instead, driver should use page_mapping which mask off the low two bits of page->mapping so it can get right struct address_space. For testing of non-lru movable page, VM supports __PageMovable function. However, it doesn't guarantee to identify non-lru movable page because page->mapping field is unified with other variables in struct page. As well, if driver releases the page after isolation by VM, page->mapping doesn't have stable value although it has PAGE_MAPPING_MOVABLE (Look at __ClearPageMovable). But __PageMovable is cheap to catch whether page is LRU or non-lru movable once the page has been isolated. Because LRU pages never can have PAGE_MAPPING_MOVABLE in page->mapping. It is also good for just peeking to test non-lru movable pages before more expensive checking with lock_page in pfn scanning to select victim. For guaranteeing non-lru movable page, VM provides PageMovable function. Unlike __PageMovable, PageMovable functions validates page->mapping and mapping->a_ops->isolate_page under lock_page. The lock_page prevents sudden destroying of page->mapping. Driver using __SetPageMovable should clear the flag via __ClearMovablePage under page_lock before the releasing the page. * PG_isolated To prevent concurrent isolation among several CPUs, VM marks isolated page as PG_isolated under lock_page. So if a CPU encounters PG_isolated non-lru movable page, it can skip it. Driver doesn't need to manipulate the fla
Re: PATCH v6v2 02/12] mm: migrate: support non-lru movable page migration
On Mon, May 30, 2016 at 11:36:07AM +0200, Vlastimil Babka wrote:
> On 05/30/2016 03:39 AM, Minchan Kim wrote:
> >After isolation, VM calls migratepage of driver with isolated page.
> >The function of migratepage is to move content of the old page to new page
> >and set up fields of struct page newpage. Keep in mind that you should
> >clear PG_movable of oldpage via __ClearPageMovable under page_lock if you
> >migrated the oldpage successfully and returns 0.
>
> This "clear PG_movable" is one of the reasons I was confused about
> what __ClearPageMovable() really does. There's no actual
> "PG_movable" page flag and the function doesn't clear even the
> actual mapping flag :) Also same thing in the Documentation/ part.
>
> Something like "... you should indicate to the VM that the oldpage
> is no longer movable via __ClearPageMovable() ..."?
It's better. I will change it.
>
> >--- a/mm/compaction.c
> >+++ b/mm/compaction.c
> >@@ -81,6 +81,39 @@ static inline bool migrate_async_suitable(int migratetype)
> >
> > #ifdef CONFIG_COMPACTION
> >
> >+int PageMovable(struct page *page)
> >+{
> >+struct address_space *mapping;
> >+
> >+VM_BUG_ON_PAGE(!PageLocked(page), page);
> >+if (!__PageMovable(page))
> >+return 0;
> >+
> >+mapping = page_mapping(page);
> >+if (mapping && mapping->a_ops && mapping->a_ops->isolate_page)
> >+return 1;
> >+
> >+return 0;
> >+}
> >+EXPORT_SYMBOL(PageMovable);
> >+
> >+void __SetPageMovable(struct page *page, struct address_space *mapping)
> >+{
> >+VM_BUG_ON_PAGE(!PageLocked(page), page);
> >+VM_BUG_ON_PAGE((unsigned long)mapping & PAGE_MAPPING_MOVABLE, page);
> >+page->mapping = (void *)((unsigned long)mapping | PAGE_MAPPING_MOVABLE);
> >+}
> >+EXPORT_SYMBOL(__SetPageMovable);
> >+
> >+void __ClearPageMovable(struct page *page)
> >+{
> >+VM_BUG_ON_PAGE(!PageLocked(page), page);
> >+VM_BUG_ON_PAGE(!PageMovable(page), page);
> >+page->mapping = (void *)((unsigned long)page->mapping &
> >+PAGE_MAPPING_MOVABLE);
> >+}
> >+EXPORT_SYMBOL(__ClearPageMovable);
>
> The second confusing thing is that the function is named
> __ClearPageMovable(), but what it really clears is the mapping
> pointer,
> which is not at all the opposite of what __SetPageMovable() does.
>
> I know it's explained in the documentation, but it also deserves a
> comment here so it doesn't confuse everyone who looks at it.
> Even better would be a less confusing name for the function, but I
> can't offer one right now.
To me, __ClearPageMovable naming is suitable for user POV.
It effectively makes the page unmovable. The confusion is just caused
by the implementation and I don't prefer exported API depends on the
implementation. So I want to add just comment.
I didn't add comment above the function because I don't want to export
internal implementation to the user. I think they don't need to know it.
index a7df2ae71f2a..d1d2063b4fd9 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -108,6 +108,11 @@ void __ClearPageMovable(struct page *page)
{
VM_BUG_ON_PAGE(!PageLocked(page), page);
VM_BUG_ON_PAGE(!PageMovable(page), page);
+ /*
+* Clear registered address_space val with keeping PAGE_MAPPING_MOVABLE
+* flag so that VM can catch up released page by driver after isolation.
+* With it, VM migration doesn't try to put it back.
+*/
page->mapping = (void *)((unsigned long)page->mapping &
PAGE_MAPPING_MOVABLE);
>
> >diff --git a/mm/util.c b/mm/util.c
> >index 917e0e3d0f8e..b756ee36f7f0 100644
> >--- a/mm/util.c
> >+++ b/mm/util.c
> >@@ -399,10 +399,12 @@ struct address_space *page_mapping(struct page *page)
> > }
> >
> > mapping = page->mapping;
>
> I'd probably use READ_ONCE() here to be safe. Not all callers are
> under page lock?
I don't understand. Yeah, all caller are not under page lock but at least,
new user of movable pages should call it under page_lock.
Yeah, I will write the rule down in document.
In this case, what kinds of problem do you see?
>
> >-if ((unsigned long)mapping & PAGE_MAPPING_FLAGS)
> >+if ((unsigned long)mapping & PAGE_MAPPING_ANON)
> > return NULL;
> >-return mapping;
> >+
> >+return (void *)((unsigned long)mapping & ~PAGE_MAPPING_FLAGS);
> > }
> >+EXPORT_SYMBOL(page_mapping);
> >
> > /* Slow path of page_mapcount() for compound pages */
> > int __page_mapcount(struct page *page)
> >
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 2/2] vhost_net: conditionally enable tx polling
On Mon, May 30, 2016 at 02:47:54AM -0400, Jason Wang wrote:
> We always poll tx for socket, this is sub optimal since:
>
> - it will be only used when we exceed the sndbuf of the socket.
> - since we use two independent polls for tx and vq, this will slightly
> increase the waitqueue traversing time and more important, vhost
> could not benefit from commit
> 9e641bdcfa4ef4d6e2fbaa59c1be0ad5d1551fd5 ("net-tun: restructure
> tun_do_read for better sleep/wakeup efficiency") even if we've
> stopped rx polling during handle_rx since tx poll were still left in
> the waitqueue.
Why is this an issue?
sock_def_write_space only wakes up when queue is half empty,
not on each packet.
if ((atomic_read(&sk->sk_wmem_alloc) << 1) <= sk->sk_sndbuf)
I suspect the issue is with your previous patch,
it now pokes at the spinlock on data path
where it used not to.
Is that right?
>
> Fix this by conditionally enable tx polling only when -EAGAIN were
> met.
>
> Test shows about 8% improvement on guest rx pps.
>
> Before: ~135
> After: ~146
>
> Signed-off-by: Jason Wang
> ---
> drivers/vhost/net.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index e91603b..5a05fa0 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -378,6 +378,7 @@ static void handle_tx(struct vhost_net *net)
> goto out;
>
> vhost_disable_notify(&net->dev, vq);
> + vhost_net_disable_vq(net, vq);
>
> hdr_size = nvq->vhost_hlen;
> zcopy = nvq->ubufs;
> @@ -459,6 +460,8 @@ static void handle_tx(struct vhost_net *net)
> % UIO_MAXIOV;
> }
> vhost_discard_vq_desc(vq, 1);
> + if (err == -EAGAIN)
> + vhost_net_enable_vq(net, vq);
> break;
> }
> if (err != len)
> --
> 1.8.3.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 1/2] vhost_net: stop polling socket during rx processing
On Mon, May 30, 2016 at 02:47:53AM -0400, Jason Wang wrote:
> We don't stop rx polling socket during rx processing, this will lead
> unnecessary wakeups from under layer net devices (E.g
> sock_def_readable() form tun). Rx will be slowed down in this
> way. This patch avoids this by stop polling socket during rx
> processing. A small drawback is that this introduces some overheads in
> light load case because of the extra start/stop polling, but single
> netperf TCP_RR does not notice any change. In a super heavy load case,
> e.g using pktgen to inject packet to guest, we get about ~8.8%
> improvement on pps:
>
> before: ~124 pkt/s
> after: ~135 pkt/s
>
> Signed-off-by: Jason Wang
> ---
> drivers/vhost/net.c | 56
> +++--
> 1 file changed, 29 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 10ff494..e91603b 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -301,6 +301,32 @@ static bool vhost_can_busy_poll(struct vhost_dev *dev,
> !vhost_has_work(dev);
> }
>
> +static void vhost_net_disable_vq(struct vhost_net *n,
> + struct vhost_virtqueue *vq)
> +{
> + struct vhost_net_virtqueue *nvq =
> + container_of(vq, struct vhost_net_virtqueue, vq);
> + struct vhost_poll *poll = n->poll + (nvq - n->vqs);
> + if (!vq->private_data)
> + return;
> + vhost_poll_stop(poll);
> +}
> +
> +static int vhost_net_enable_vq(struct vhost_net *n,
> + struct vhost_virtqueue *vq)
> +{
> + struct vhost_net_virtqueue *nvq =
> + container_of(vq, struct vhost_net_virtqueue, vq);
> + struct vhost_poll *poll = n->poll + (nvq - n->vqs);
> + struct socket *sock;
> +
> + sock = vq->private_data;
> + if (!sock)
> + return 0;
> +
> + return vhost_poll_start(poll, sock->file);
> +}
> +
> static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
> struct vhost_virtqueue *vq,
> struct iovec iov[], unsigned int iov_size,
BTW we might want to rename these functions, name no longer
reflects function ...
> @@ -627,6 +653,7 @@ static void handle_rx(struct vhost_net *net)
> if (!sock)
> goto out;
> vhost_disable_notify(&net->dev, vq);
> + vhost_net_disable_vq(net, vq);
>
> vhost_hlen = nvq->vhost_hlen;
> sock_hlen = nvq->sock_hlen;
> @@ -715,9 +742,10 @@ static void handle_rx(struct vhost_net *net)
> total_len += vhost_len;
> if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
> vhost_poll_queue(&vq->poll);
> - break;
> + goto out;
> }
> }
> + vhost_net_enable_vq(net, vq);
OK so if sock is readable but RX VQ is empty, this will
immediately schedule another round of handle_rx and so ad
infinitum,
Looks like a bug.
> out:
> mutex_unlock(&vq->mutex);
> }
> @@ -796,32 +824,6 @@ static int vhost_net_open(struct inode *inode, struct
> file *f)
> return 0;
> }
>
> -static void vhost_net_disable_vq(struct vhost_net *n,
> - struct vhost_virtqueue *vq)
> -{
> - struct vhost_net_virtqueue *nvq =
> - container_of(vq, struct vhost_net_virtqueue, vq);
> - struct vhost_poll *poll = n->poll + (nvq - n->vqs);
> - if (!vq->private_data)
> - return;
> - vhost_poll_stop(poll);
> -}
> -
> -static int vhost_net_enable_vq(struct vhost_net *n,
> - struct vhost_virtqueue *vq)
> -{
> - struct vhost_net_virtqueue *nvq =
> - container_of(vq, struct vhost_net_virtqueue, vq);
> - struct vhost_poll *poll = n->poll + (nvq - n->vqs);
> - struct socket *sock;
> -
> - sock = vq->private_data;
> - if (!sock)
> - return 0;
> -
> - return vhost_poll_start(poll, sock->file);
> -}
> -
> static struct socket *vhost_net_stop_vq(struct vhost_net *n,
> struct vhost_virtqueue *vq)
> {
> --
> 1.8.3.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-gpu: fix output lookup
On Mon, May 30, 2016 at 02:03:26PM +0200, Gerd Hoffmann wrote:
> Needed for multihead setups where we can have disabled
> outputs and therefore plane->crtc can be NULL.
>
> Signed-off-by: Gerd Hoffmann
See my reply in the other thread, but I think you have a more fundamental
issue here. I dropped two suggestions worth trying, both need your own
atomic_commit_tail (on top of my branch at least):
- set active_only to true in commit_planes()
- move commit_planes after commit_modeset_enables().
Cheers, Daniel
> ---
> drivers/gpu/drm/virtio/virtgpu_plane.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c
> b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index 70b44a2..e50674b 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -63,11 +63,17 @@ static void virtio_gpu_plane_atomic_update(struct
> drm_plane *plane,
> {
> struct drm_device *dev = plane->dev;
> struct virtio_gpu_device *vgdev = dev->dev_private;
> - struct virtio_gpu_output *output =
> drm_crtc_to_virtio_gpu_output(plane->crtc);
> + struct virtio_gpu_output *output = NULL;
> struct virtio_gpu_framebuffer *vgfb;
> struct virtio_gpu_object *bo;
> uint32_t handle;
>
> + if (plane->state->crtc)
> + output = drm_crtc_to_virtio_gpu_output(plane->state->crtc);
> + if (old_state->crtc)
> + output = drm_crtc_to_virtio_gpu_output(old_state->crtc);
> + WARN_ON(!output);
> +
> if (plane->state->fb) {
> vgfb = to_virtio_gpu_framebuffer(plane->state->fb);
> bo = gem_to_virtio_gpu_obj(vgfb->obj);
> --
> 1.8.3.1
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] Add virtio gpu driver.
On Mon, May 30, 2016 at 03:50:36PM +0200, Gerd Hoffmann wrote:
> Hi,
>
> > - add a small core function to registerr HOT_X/HOT_Y for a (cursor) plane,
> > e.g. drm_plane_register_hotspot(). That should allocate the properties
> > (if they don't exist yet) and then attach those props to the cursor. We
> > don't want those props everywhere, but only on drivers that support/need
> > them, aka virtual hw.
>
> Hmm, why is this special to virtual hw?
>
> > if (crtc->cursor) {
> > - ret = drm_mode_cursor_universal(crtc, req, file_priv);
> > + if (drm_core_check_feature(DRIVER_ATOMIC))
> > + ret = drm_mode_cursor_atomic(crtc, req, file_priv);
> > + else
> > + ret = drm_mode_cursor_universal(crtc, req, file_priv);
> > goto out;
>
> > drm_mode_cursor_atomic would simply be a fusing of
> > drm_mode_cursor_universal + drm_atomic_helper_update_plane (dump all the
> > intermediate variables and store directly in the plane state), with the
> > addition of also storing hot_x/y into the plane state.
>
> Hmm, that'll either make drm_mode_cursor_atomic a big cut+pasted
> function, or need quite some refactoring to move common code into
> functions callable from both drm_mode_cursor_atomic
> +drm_mode_cursor_universal ...
>
> Why attach the hotspot to the plane? Wouldn't it make more sense to
> make it a framebuffer property?
We don't have properties on the framebuffer. I guess you /could/ just add
it internally to struct drm_framebuffer, and not bother exposing to
userspace. I guess that would be a lot simpler, but it also means that
atomic userspace can't use hotspots before we add properties to fbs. And
doing that is a bit tricky since drm_framebuffer objects are meant to be
invariant - this assumption is deeply in-grained into the code all over
the place, everything just compares pointers when semantically it means to
compare the entire fb (including backing storage pointer/offsets and
everything).
So would be a bit more work to wire up for atomic userspace, but indeed a
lot less work to implement. I'm totally happy if you go with that tradeoff
;-)
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 7/7] [wip] virtio-gpu: add page flip support
On Mon, May 30, 2016 at 02:06:50PM +0200, Gerd Hoffmann wrote: > Hi, > > > > But I'll take you up on the implied offer to help out and test ;-) > > > > git://people.freedesktop.org/~danvet/drm stuff > > Tried that branch. > > > Would be really awesome if you could test this on virtio. Note that the > > new nonblocking helpers require that your atomic backend gets the drm > > event handling right. So if there's a bug in that logic then you'll see > > lots of dmesg noise about waits timing out (after 10s of waiting). From a > > quick inspection it should work though. > > No timeouts. Yay! > > But it seems crtcs can be (temporarely) disabled now, so we might have > to pick up the crtc from old_state in virtio_gpu_plane_atomic_update to > figure which virtual output needs to be turned off. Ran into this last > week already. Happened with multihead setups only, but the same patch > fixes this one too ;) > > https://lists.freedesktop.org/archives/dri-devel/2016-May/108772.html Hm, smells more like virtio isn't too happy with the default ordering of the commit operation. The default is: - Disable any crtc/encoders that need to be disabled/change. - Bash new plane setup into hw. - Enable all crtcs/encoders that need to be enabled/have changed. There's two problems: - some hw gets real grumpy if you bash in plane state without the crtc state yet matching. - if you do runtime pm nothing is enabled and the writes get lost at best, or hang your interconnect at worst. That's why you can overwrite atomic_commit_tail, and use something more sensible. See for example what it looks like for rockchip. I have a gut feeling that should also take care of your troubles. Using the old crtc is definitely not what you want. Another option is that virtio isn't happy about bashing in plane state for disabled crtc. Again helpers have you covered, look at the active_only parameter for drm_atomic_helper_commit_planes(). Aside, if you wonder why these defaults: They match what the crtc helpers are doing, but they are a bit surprising indeed. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] Add virtio gpu driver.
Hi,
> - add a small core function to registerr HOT_X/HOT_Y for a (cursor) plane,
> e.g. drm_plane_register_hotspot(). That should allocate the properties
> (if they don't exist yet) and then attach those props to the cursor. We
> don't want those props everywhere, but only on drivers that support/need
> them, aka virtual hw.
Hmm, why is this special to virtual hw?
> if (crtc->cursor) {
> - ret = drm_mode_cursor_universal(crtc, req, file_priv);
> + if (drm_core_check_feature(DRIVER_ATOMIC))
> + ret = drm_mode_cursor_atomic(crtc, req, file_priv);
> + else
> + ret = drm_mode_cursor_universal(crtc, req, file_priv);
> goto out;
> drm_mode_cursor_atomic would simply be a fusing of
> drm_mode_cursor_universal + drm_atomic_helper_update_plane (dump all the
> intermediate variables and store directly in the plane state), with the
> addition of also storing hot_x/y into the plane state.
Hmm, that'll either make drm_mode_cursor_atomic a big cut+pasted
function, or need quite some refactoring to move common code into
functions callable from both drm_mode_cursor_atomic
+drm_mode_cursor_universal ...
Why attach the hotspot to the plane? Wouldn't it make more sense to
make it a framebuffer property?
cheers,
Gerd
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 03/12] mm: balloon: use general non-lru movable page feature
On 05/20/2016 04:23 PM, Minchan Kim wrote:
Now, VM has a feature to migrate non-lru movable pages so
balloon doesn't need custom migration hooks in migrate.c
and compaction.c. Instead, this patch implements
page->mapping->a_ops->{isolate|migrate|putback} functions.
With that, we could remove hooks for ballooning in general
migration functions and make balloon compaction simple.
Cc: virtualization@lists.linux-foundation.org
Cc: Rafael Aquini
Cc: Konstantin Khlebnikov
Signed-off-by: Gioh Kim
Signed-off-by: Minchan Kim
Except for the inode/pseudofs stuff which I'm not familiar with,
Acked-by: Vlastimil Babka
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 7/7] [wip] virtio-gpu: add page flip support
Hi, > > But I'll take you up on the implied offer to help out and test ;-) > > git://people.freedesktop.org/~danvet/drm stuff Tried that branch. > Would be really awesome if you could test this on virtio. Note that the > new nonblocking helpers require that your atomic backend gets the drm > event handling right. So if there's a bug in that logic then you'll see > lots of dmesg noise about waits timing out (after 10s of waiting). From a > quick inspection it should work though. No timeouts. Yay! But it seems crtcs can be (temporarely) disabled now, so we might have to pick up the crtc from old_state in virtio_gpu_plane_atomic_update to figure which virtual output needs to be turned off. Ran into this last week already. Happened with multihead setups only, but the same patch fixes this one too ;) https://lists.freedesktop.org/archives/dri-devel/2016-May/108772.html cheers, Gerd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio-gpu: fix output lookup
Needed for multihead setups where we can have disabled
outputs and therefore plane->crtc can be NULL.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/virtio/virtgpu_plane.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c
b/drivers/gpu/drm/virtio/virtgpu_plane.c
index 70b44a2..e50674b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -63,11 +63,17 @@ static void virtio_gpu_plane_atomic_update(struct drm_plane
*plane,
{
struct drm_device *dev = plane->dev;
struct virtio_gpu_device *vgdev = dev->dev_private;
- struct virtio_gpu_output *output =
drm_crtc_to_virtio_gpu_output(plane->crtc);
+ struct virtio_gpu_output *output = NULL;
struct virtio_gpu_framebuffer *vgfb;
struct virtio_gpu_object *bo;
uint32_t handle;
+ if (plane->state->crtc)
+ output = drm_crtc_to_virtio_gpu_output(plane->state->crtc);
+ if (old_state->crtc)
+ output = drm_crtc_to_virtio_gpu_output(old_state->crtc);
+ WARN_ON(!output);
+
if (plane->state->fb) {
vgfb = to_virtio_gpu_framebuffer(plane->state->fb);
bo = gem_to_virtio_gpu_obj(vgfb->obj);
--
1.8.3.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: PATCH v6v2 02/12] mm: migrate: support non-lru movable page migration
On 05/30/2016 03:39 AM, Minchan Kim wrote:
After isolation, VM calls migratepage of driver with isolated page.
The function of migratepage is to move content of the old page to new page
and set up fields of struct page newpage. Keep in mind that you should
clear PG_movable of oldpage via __ClearPageMovable under page_lock if you
migrated the oldpage successfully and returns 0.
This "clear PG_movable" is one of the reasons I was confused about what
__ClearPageMovable() really does. There's no actual "PG_movable" page
flag and the function doesn't clear even the actual mapping flag :) Also
same thing in the Documentation/ part.
Something like "... you should indicate to the VM that the oldpage is no
longer movable via __ClearPageMovable() ..."?
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -81,6 +81,39 @@ static inline bool migrate_async_suitable(int migratetype)
#ifdef CONFIG_COMPACTION
+int PageMovable(struct page *page)
+{
+ struct address_space *mapping;
+
+ VM_BUG_ON_PAGE(!PageLocked(page), page);
+ if (!__PageMovable(page))
+ return 0;
+
+ mapping = page_mapping(page);
+ if (mapping && mapping->a_ops && mapping->a_ops->isolate_page)
+ return 1;
+
+ return 0;
+}
+EXPORT_SYMBOL(PageMovable);
+
+void __SetPageMovable(struct page *page, struct address_space *mapping)
+{
+ VM_BUG_ON_PAGE(!PageLocked(page), page);
+ VM_BUG_ON_PAGE((unsigned long)mapping & PAGE_MAPPING_MOVABLE, page);
+ page->mapping = (void *)((unsigned long)mapping | PAGE_MAPPING_MOVABLE);
+}
+EXPORT_SYMBOL(__SetPageMovable);
+
+void __ClearPageMovable(struct page *page)
+{
+ VM_BUG_ON_PAGE(!PageLocked(page), page);
+ VM_BUG_ON_PAGE(!PageMovable(page), page);
+ page->mapping = (void *)((unsigned long)page->mapping &
+ PAGE_MAPPING_MOVABLE);
+}
+EXPORT_SYMBOL(__ClearPageMovable);
The second confusing thing is that the function is named
__ClearPageMovable(), but what it really clears is the mapping pointer,
which is not at all the opposite of what __SetPageMovable() does.
I know it's explained in the documentation, but it also deserves a
comment here so it doesn't confuse everyone who looks at it.
Even better would be a less confusing name for the function, but I can't
offer one right now.
diff --git a/mm/util.c b/mm/util.c
index 917e0e3d0f8e..b756ee36f7f0 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -399,10 +399,12 @@ struct address_space *page_mapping(struct page *page)
}
mapping = page->mapping;
I'd probably use READ_ONCE() here to be safe. Not all callers are under
page lock?
- if ((unsigned long)mapping & PAGE_MAPPING_FLAGS)
+ if ((unsigned long)mapping & PAGE_MAPPING_ANON)
return NULL;
- return mapping;
+
+ return (void *)((unsigned long)mapping & ~PAGE_MAPPING_FLAGS);
}
+EXPORT_SYMBOL(page_mapping);
/* Slow path of page_mapcount() for compound pages */
int __page_mapcount(struct page *page)
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 02/12] mm: migrate: support non-lru movable page migration
On 05/30/2016 03:33 AM, Minchan Kim wrote:
+ page->mapping = (void *)((unsigned long)page->mapping &
+ PAGE_MAPPING_MOVABLE);
This should be negated to clear... use ~PAGE_MAPPING_MOVABLE ?
No.
The intention is to clear only mapping value but PAGE_MAPPING_MOVABLE
flag. So, any new migration trial will be failed because PageMovable
checks page's mapping value but ongoing migraion handling can catch
whether it's movable page or not with the type bit.
Oh, OK, I got that wrong. I'll point out in the reply to the v6v2 what
misled me :)
So this effectively prevents movable compound pages from being
migrated. Are you sure no users of this functionality are going to
have compound pages? I assumed that they could, and so made the code
like this, with the is_lru variable (which is redundant after your
change).
This implementation at the moment disables effectively non-lru compound
page migration but I'm not a god so I can't make sure no one doesn't want
it in future. If someone want it, we can support it then because this work
doesn't prevent it by design.
Oh well. As long as the balloon pages or zsmalloc don't already use
compound pages...
I thouht PageCompound check right before isolate_movable_page in
isolate_migratepages_block will filter it out mostly but yeah
it is racy without zone->lru_lock so it could reach to isolate_movable_page.
However, PageMovable check in there investigates mapping, mapping->a_ops,
and a_ops->isolate_page to verify whether it's movable page or not.
I thought it's sufficient to filter THP page.
I guess, yeah.
[...]
@@ -755,33 +844,69 @@ static int move_to_new_page(struct page *newpage, struct
page *page,
enum migrate_mode mode)
{
struct address_space *mapping;
- int rc;
+ int rc = -EAGAIN;
+ bool is_lru = !__PageMovable(page);
VM_BUG_ON_PAGE(!PageLocked(page), page);
VM_BUG_ON_PAGE(!PageLocked(newpage), newpage);
mapping = page_mapping(page);
- if (!mapping)
- rc = migrate_page(mapping, newpage, page, mode);
- else if (mapping->a_ops->migratepage)
- /*
-* Most pages have a mapping and most filesystems provide a
-* migratepage callback. Anonymous pages are part of swap
-* space which also has its own migratepage callback. This
-* is the most common path for page migration.
-*/
- rc = mapping->a_ops->migratepage(mapping, newpage, page, mode);
- else
- rc = fallback_migrate_page(mapping, newpage, page, mode);
+ /*
+* In case of non-lru page, it could be released after
+* isolation step. In that case, we shouldn't try
+* fallback migration which is designed for LRU pages.
+*/
Hmm but is_lru was determined from !__PageMovable() above, also well
after the isolation step. So if the driver already released it, we
wouldn't detect it? And this function is all under same page lock,
so if __PageMovable was true above, so will be PageMovable below?
You are missing what I mentioned above.
We should keep the type bit to catch what you are saying(i.e., driver
already released).
__PageMovable just checks PAGE_MAPPING_MOVABLE flag and PageMovable
checks page->mapping valid while __ClearPageMovable reset only
valid vaule of mapping, not PAGE_MAPPING_MOVABLE flag.
I wrote it down in Documentation/vm/page_migration.
"For testing of non-lru movable page, VM supports __PageMovable function.
However, it doesn't guarantee to identify non-lru movable page because
page->mapping field is unified with other variables in struct page.
As well, if driver releases the page after isolation by VM, page->mapping
doesn't have stable value although it has PAGE_MAPPING_MOVABLE
(Look at __ClearPageMovable). But __PageMovable is cheap to catch whether
page is LRU or non-lru movable once the page has been isolated. Because
LRU pages never can have PAGE_MAPPING_MOVABLE in page->mapping. It is also
good for just peeking to test non-lru movable pages before more expensive
checking with lock_page in pfn scanning to select victim.
For guaranteeing non-lru movable page, VM provides PageMovable function.
Unlike __PageMovable, PageMovable functions validates page->mapping and
mapping->a_ops->isolate_page under lock_page. The lock_page prevents sudden
destroying of page->mapping.
Driver using __SetPageMovable should clear the flag via __ClearMovablePage
under page_lock before the releasing the page."
Right, I get it now.
+ if (!((unsigned long)page->mapping & PAGE_MAPPING_FLAGS))
page->mapping = NULL;
The two lines above make little sense to me without a comment.
I folded this.
@@ -901,7 +901,12 @@ static int move_to_new_page(struct page *newpage, struct
page *page,
__ClearPageIsolated(page);
}
-
Re: [PATCH v3 7/7] [wip] virtio-gpu: add page flip support
On Fri, May 27, 2016 at 09:50:27AM +0200, Daniel Vetter wrote: > On Fri, May 27, 2016 at 09:46:03AM +0200, Gerd Hoffmann wrote: > > On Mi, 2016-05-25 at 18:37 +0200, Daniel Vetter wrote: > > > On Fri, Oct 2, 2015 at 1:58 PM, Gerd Hoffmann wrote: > > > > Signed-off-by: Gerd Hoffmann > > > > > > So I entirely missed this, but this isn't really how to implement > > > page_flip for an atomic driver. Working on some stuff and will hack up > > > a likely totally broken patch, but should be enough as guideline. > > > -Daniel > > > > Hmm, no patch in my inbox yet. Care to send it over or give some hints > > what is wrong with this? > > You should use drm_atomic_helper_page_flip as .page_flip hook instead of > rolling your own, when you have a brand-new atomic driver. If that helper > doesn't work that means you have a bug in your driver somewhere. > > And indeed virtio just plugs in drm_atomic_helper_commit, which thus far > doesnt' do nonblocking commits, which means the page flip stuff won't > work. I'm working on generic nonblocking atomic to fix this up. Currently > still wip and buggy though, hence no patches. > > But I'll take you up on the implied offer to help out and test ;-) Ok, patches are now on the list and also in my fdo repo at: https://cgit.freedesktop.org/~danvet/drm/log/ git://people.freedesktop.org/~danvet/drm stuff Would be really awesome if you could test this on virtio. Note that the new nonblocking helpers require that your atomic backend gets the drm event handling right. So if there's a bug in that logic then you'll see lots of dmesg noise about waits timing out (after 10s of waiting). From a quick inspection it should work though. Upshot is that once you get normal modeset to work with those new helpers, then nonblocking should Just Work, and there's no need anymore for additional testing. Or at least shouldn't. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
