| Message ID | 1443787104-24243-8-git-send-email-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 2, 2015 at 1:58 PM, Gerd Hoffmann <kraxel@redhat.com> wrote: > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> 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 > --- > drivers/gpu/drm/virtio/virtgpu_display.c | 49 ++++++++++++++++++++++++++++++-- > 1 file changed, 46 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c > index c9c1427..f545913 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_display.c > +++ b/drivers/gpu/drm/virtio/virtgpu_display.c > @@ -125,6 +125,51 @@ static int virtio_gpu_crtc_cursor_move(struct drm_crtc *crtc, > return 0; > } > > +static int virtio_gpu_page_flip(struct drm_crtc *crtc, > + struct drm_framebuffer *fb, > + struct drm_pending_vblank_event *event, > + uint32_t flags) > +{ > + struct virtio_gpu_device *vgdev = crtc->dev->dev_private; > + struct virtio_gpu_output *output = > + container_of(crtc, struct virtio_gpu_output, crtc); > + struct drm_plane *plane = crtc->primary; > + struct virtio_gpu_framebuffer *vgfb; > + struct virtio_gpu_object *bo; > + unsigned long irqflags; > + uint32_t handle; > + > + plane->fb = fb; > + vgfb = to_virtio_gpu_framebuffer(plane->fb); > + bo = gem_to_virtio_gpu_obj(vgfb->obj); > + handle = bo->hw_res_handle; > + > + DRM_DEBUG("handle 0x%x%s, crtc %dx%d\n", handle, > + bo->dumb ? ", dumb" : "", > + crtc->mode.hdisplay, crtc->mode.vdisplay); > + if (bo->dumb) { > + virtio_gpu_cmd_transfer_to_host_2d > + (vgdev, handle, 0, > + cpu_to_le32(crtc->mode.hdisplay), > + cpu_to_le32(crtc->mode.vdisplay), > + 0, 0, NULL); > + } > + virtio_gpu_cmd_set_scanout(vgdev, output->index, handle, > + crtc->mode.hdisplay, > + crtc->mode.vdisplay, 0, 0); > + virtio_gpu_cmd_resource_flush(vgdev, handle, 0, 0, > + crtc->mode.hdisplay, > + crtc->mode.vdisplay); > + > + if (event) { > + spin_lock_irqsave(&crtc->dev->event_lock, irqflags); > + drm_send_vblank_event(crtc->dev, -1, event); > + spin_unlock_irqrestore(&crtc->dev->event_lock, irqflags); > + } > + > + return 0; > +} > + > static const struct drm_crtc_funcs virtio_gpu_crtc_funcs = { > .cursor_set2 = virtio_gpu_crtc_cursor_set, > .cursor_move = virtio_gpu_crtc_cursor_move, > @@ -132,9 +177,7 @@ static const struct drm_crtc_funcs virtio_gpu_crtc_funcs = { > .set_config = drm_atomic_helper_set_config, > .destroy = drm_crtc_cleanup, > > -#if 0 /* not (yet) working without vblank support according to docs */ > - .page_flip = drm_atomic_helper_page_flip, > -#endif > + .page_flip = virtio_gpu_page_flip, > .reset = drm_atomic_helper_crtc_reset, > .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > -- > 1.8.3.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mi, 2016-05-25 at 18:37 +0200, Daniel Vetter wrote: > On Fri, Oct 2, 2015 at 1:58 PM, Gerd Hoffmann <kraxel@redhat.com> wrote: > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > 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? thanks, Gerd
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 <kraxel@redhat.com> wrote: > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > > > 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 ;-) -Daniel
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 <kraxel@redhat.com> wrote: > > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > > > > > 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
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
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
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
On Tue, May 31, 2016 at 8:18 AM, Gerd Hoffmann <kraxel@redhat.com> 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
Hi, > > 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? > Nah, I just misunderstood your patch. If it's all about finding the > corresponding crtc, then you're all good. Yes, it's all about finding the crtc. > I thought there was some > other reason (like the virtual hw getting upset about certain things). virtio wouldn't be upset. It's a pointless exercise though to first disable the output, just to re-enable it the next moment with the new page-flipped framebuffer. So I guess I should look at the active_only thing nevertheless. > btw can you pls drop an ack or r-b > onto my virtio conversion? I already added your tested-by. Grr, mail is not in my dri-devel folder. Guess that is the <censored> "avoid-duplicates" mailman option at work. Feel free to just add the r-b too. Or I'll send it for the next version of the series. cheers, Gerd
On Tue, May 31, 2016 at 9:34 AM, Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > >> > 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? > >> Nah, I just misunderstood your patch. If it's all about finding the >> corresponding crtc, then you're all good. > > Yes, it's all about finding the crtc. > >> I thought there was some >> other reason (like the virtual hw getting upset about certain things). > > virtio wouldn't be upset. > > It's a pointless exercise though to first disable the output, just to > re-enable it the next moment with the new page-flipped framebuffer. So > I guess I should look at the active_only thing nevertheless. It's still possible that the plane can get disabled without it getting enabled. Userspace is allowed to do this. But since this suprises a lot of driver writers there's a special atomic_plane_disable hook you can use for the disable path. Instead of hand-rolling that check in your own code. >> btw can you pls drop an ack or r-b >> onto my virtio conversion? I already added your tested-by. > > Grr, mail is not in my dri-devel folder. Guess that is the <censored> > "avoid-duplicates" mailman option at work. > > Feel free to just add the r-b too. Or I'll send it for the next version > of the series. Added your r-b locally so it won't get lost on the next round. Thanks, Daniel
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index c9c1427..f545913 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -125,6 +125,51 @@ static int virtio_gpu_crtc_cursor_move(struct drm_crtc *crtc, return 0; } +static int virtio_gpu_page_flip(struct drm_crtc *crtc, + struct drm_framebuffer *fb, + struct drm_pending_vblank_event *event, + uint32_t flags) +{ + struct virtio_gpu_device *vgdev = crtc->dev->dev_private; + struct virtio_gpu_output *output = + container_of(crtc, struct virtio_gpu_output, crtc); + struct drm_plane *plane = crtc->primary; + struct virtio_gpu_framebuffer *vgfb; + struct virtio_gpu_object *bo; + unsigned long irqflags; + uint32_t handle; + + plane->fb = fb; + vgfb = to_virtio_gpu_framebuffer(plane->fb); + bo = gem_to_virtio_gpu_obj(vgfb->obj); + handle = bo->hw_res_handle; + + DRM_DEBUG("handle 0x%x%s, crtc %dx%d\n", handle, + bo->dumb ? ", dumb" : "", + crtc->mode.hdisplay, crtc->mode.vdisplay); + if (bo->dumb) { + virtio_gpu_cmd_transfer_to_host_2d + (vgdev, handle, 0, + cpu_to_le32(crtc->mode.hdisplay), + cpu_to_le32(crtc->mode.vdisplay), + 0, 0, NULL); + } + virtio_gpu_cmd_set_scanout(vgdev, output->index, handle, + crtc->mode.hdisplay, + crtc->mode.vdisplay, 0, 0); + virtio_gpu_cmd_resource_flush(vgdev, handle, 0, 0, + crtc->mode.hdisplay, + crtc->mode.vdisplay); + + if (event) { + spin_lock_irqsave(&crtc->dev->event_lock, irqflags); + drm_send_vblank_event(crtc->dev, -1, event); + spin_unlock_irqrestore(&crtc->dev->event_lock, irqflags); + } + + return 0; +} + static const struct drm_crtc_funcs virtio_gpu_crtc_funcs = { .cursor_set2 = virtio_gpu_crtc_cursor_set, .cursor_move = virtio_gpu_crtc_cursor_move, @@ -132,9 +177,7 @@ static const struct drm_crtc_funcs virtio_gpu_crtc_funcs = { .set_config = drm_atomic_helper_set_config, .destroy = drm_crtc_cleanup, -#if 0 /* not (yet) working without vblank support according to docs */ - .page_flip = drm_atomic_helper_page_flip, -#endif + .page_flip = virtio_gpu_page_flip, .reset = drm_atomic_helper_crtc_reset, .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- drivers/gpu/drm/virtio/virtgpu_display.c | 49 ++++++++++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 3 deletions(-)