| Message ID | 20180911071544.17190-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] staging: vboxvideo: Fix modeset / page_flip error handling | expand |
On Tue, Sep 11, 2018 at 09:15:41AM +0200, Hans de Goede wrote: > The default settings for Linux vms created in VirtualBox allocate only > 16M of videomem. When running fullscreen on a 1920x1080 (or bigger) monitor > this is not a lot. > > When using GNOME3 on Wayland we have already been seeing out of video > memory errors for a while now. After commit 2408898e3b6c ("staging: > vboxvideo: Add page-flip support") this has become much worse as now > multiple buffers are used. > > There is nothing we can do about there not being enough video-mem, but > we should handle running out of video-mem properly, currently there are > 2 problems with this: > > 1) vbox_crtc_mode_set() does not check if vbox_crtc_mode_set_base() fails > at all and does not properly propagate the oom error. > > 2) vbox_crtc_do_set_base() unpins the old fb too soon: > > 2.1) It unpins it before pinning the new fb, so if the pinning of the new > fb fails (which it will when we run out of video-mem), then we also cannot > fall back to the old-fb as it has been already unpinned. We could try to > re-pin it but there is no guarantee that will succeed. > > 2.2) It unpins it before reprogramming the hardware to scan out from the > new-fb, which could lead to some ugliness where the hw is scanning out the > oldfb while it is being replaced with something else. > > Fixing this requires to do things in this order: > 1) Pin the new fb > 2) Program the hw > 3) Unpin the oldfb > > This needs to be done for both a mode_set and for a page_flip so this > commit re-writes vbox_crtc_do_set_base() into vbox_crtc_set_base_and_mode() > which does this in the correct order, putting the hardware programming > which was duplicated between the mode_set and page_flip code inside the > new function. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/staging/vboxvideo/vbox_mode.c | 121 +++++++++++++------------- > 1 file changed, 62 insertions(+), 59 deletions(-) This doesn't apply to my tree. Does it need to go on top of the previous patches you sent for 4.19-final? thanks, greg k-h
Hi, On 11-09-18 20:46, Greg Kroah-Hartman wrote: > On Tue, Sep 11, 2018 at 09:15:41AM +0200, Hans de Goede wrote: >> The default settings for Linux vms created in VirtualBox allocate only >> 16M of videomem. When running fullscreen on a 1920x1080 (or bigger) monitor >> this is not a lot. >> >> When using GNOME3 on Wayland we have already been seeing out of video >> memory errors for a while now. After commit 2408898e3b6c ("staging: >> vboxvideo: Add page-flip support") this has become much worse as now >> multiple buffers are used. >> >> There is nothing we can do about there not being enough video-mem, but >> we should handle running out of video-mem properly, currently there are >> 2 problems with this: >> >> 1) vbox_crtc_mode_set() does not check if vbox_crtc_mode_set_base() fails >> at all and does not properly propagate the oom error. >> >> 2) vbox_crtc_do_set_base() unpins the old fb too soon: >> >> 2.1) It unpins it before pinning the new fb, so if the pinning of the new >> fb fails (which it will when we run out of video-mem), then we also cannot >> fall back to the old-fb as it has been already unpinned. We could try to >> re-pin it but there is no guarantee that will succeed. >> >> 2.2) It unpins it before reprogramming the hardware to scan out from the >> new-fb, which could lead to some ugliness where the hw is scanning out the >> oldfb while it is being replaced with something else. >> >> Fixing this requires to do things in this order: >> 1) Pin the new fb >> 2) Program the hw >> 3) Unpin the oldfb >> >> This needs to be done for both a mode_set and for a page_flip so this >> commit re-writes vbox_crtc_do_set_base() into vbox_crtc_set_base_and_mode() >> which does this in the correct order, putting the hardware programming >> which was duplicated between the mode_set and page_flip code inside the >> new function. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/staging/vboxvideo/vbox_mode.c | 121 +++++++++++++------------- >> 1 file changed, 62 insertions(+), 59 deletions(-) > > This doesn't apply to my tree. Does it need to go on top of the > previous patches you sent for 4.19-final? Yes this goes on top of the fixes for 4.19-final. Regards, Hans
On Wed, Sep 12, 2018 at 09:54:51AM +0200, Hans de Goede wrote: > Hi, > > On 11-09-18 20:46, Greg Kroah-Hartman wrote: > > On Tue, Sep 11, 2018 at 09:15:41AM +0200, Hans de Goede wrote: > > > The default settings for Linux vms created in VirtualBox allocate only > > > 16M of videomem. When running fullscreen on a 1920x1080 (or bigger) monitor > > > this is not a lot. > > > > > > When using GNOME3 on Wayland we have already been seeing out of video > > > memory errors for a while now. After commit 2408898e3b6c ("staging: > > > vboxvideo: Add page-flip support") this has become much worse as now > > > multiple buffers are used. > > > > > > There is nothing we can do about there not being enough video-mem, but > > > we should handle running out of video-mem properly, currently there are > > > 2 problems with this: > > > > > > 1) vbox_crtc_mode_set() does not check if vbox_crtc_mode_set_base() fails > > > at all and does not properly propagate the oom error. > > > > > > 2) vbox_crtc_do_set_base() unpins the old fb too soon: > > > > > > 2.1) It unpins it before pinning the new fb, so if the pinning of the new > > > fb fails (which it will when we run out of video-mem), then we also cannot > > > fall back to the old-fb as it has been already unpinned. We could try to > > > re-pin it but there is no guarantee that will succeed. > > > > > > 2.2) It unpins it before reprogramming the hardware to scan out from the > > > new-fb, which could lead to some ugliness where the hw is scanning out the > > > oldfb while it is being replaced with something else. > > > > > > Fixing this requires to do things in this order: > > > 1) Pin the new fb > > > 2) Program the hw > > > 3) Unpin the oldfb > > > > > > This needs to be done for both a mode_set and for a page_flip so this > > > commit re-writes vbox_crtc_do_set_base() into vbox_crtc_set_base_and_mode() > > > which does this in the correct order, putting the hardware programming > > > which was duplicated between the mode_set and page_flip code inside the > > > new function. > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > --- > > > drivers/staging/vboxvideo/vbox_mode.c | 121 +++++++++++++------------- > > > 1 file changed, 62 insertions(+), 59 deletions(-) > > > > This doesn't apply to my tree. Does it need to go on top of the > > previous patches you sent for 4.19-final? > > Yes this goes on top of the fixes for 4.19-final. Ok, thanks for confirming. greg k-h
diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c index 79836c8fb909..47de1364ec4d 100644 --- a/drivers/staging/vboxvideo/vbox_mode.c +++ b/drivers/staging/vboxvideo/vbox_mode.c @@ -221,53 +221,68 @@ static bool vbox_set_up_input_mapping(struct vbox_private *vbox) return old_single_framebuffer != vbox->single_framebuffer; } -static int vbox_crtc_do_set_base(struct drm_crtc *crtc, - struct drm_framebuffer *old_fb, - struct drm_framebuffer *new_fb, - int x, int y) +static int vbox_fb_pin(struct drm_framebuffer *fb, u64 *addr) +{ + struct vbox_bo *bo = gem_to_vbox_bo(to_vbox_framebuffer(fb)->obj); + int ret; + + ret = vbox_bo_reserve(bo, false); + if (ret) + return ret; + + ret = vbox_bo_pin(bo, TTM_PL_FLAG_VRAM, addr); + vbox_bo_unreserve(bo); + return ret; +} + +static void vbox_fb_unpin(struct drm_framebuffer *fb) { - struct vbox_private *vbox = crtc->dev->dev_private; - struct vbox_crtc *vbox_crtc = to_vbox_crtc(crtc); - struct drm_gem_object *obj; - struct vbox_framebuffer *vbox_fb; struct vbox_bo *bo; int ret; - u64 gpu_addr; - /* Unpin the previous fb. */ - if (old_fb) { - vbox_fb = to_vbox_framebuffer(old_fb); - obj = vbox_fb->obj; - bo = gem_to_vbox_bo(obj); - ret = vbox_bo_reserve(bo, false); - if (ret) - return ret; + if (!fb) + return; - vbox_bo_unpin(bo); - vbox_bo_unreserve(bo); + bo = gem_to_vbox_bo(to_vbox_framebuffer(fb)->obj); + + ret = vbox_bo_reserve(bo, false); + if (ret) { + DRM_ERROR("Error %d reserving fb bo, leaving it pinned\n", ret); + return; } - vbox_fb = to_vbox_framebuffer(new_fb); - obj = vbox_fb->obj; - bo = gem_to_vbox_bo(obj); + vbox_bo_unpin(bo); + vbox_bo_unreserve(bo); +} - ret = vbox_bo_reserve(bo, false); - if (ret) - return ret; +static int vbox_crtc_set_base_and_mode(struct drm_crtc *crtc, + struct drm_framebuffer *old_fb, + struct drm_framebuffer *new_fb, + struct drm_display_mode *mode, + int x, int y) +{ + struct vbox_private *vbox = crtc->dev->dev_private; + struct vbox_crtc *vbox_crtc = to_vbox_crtc(crtc); + u64 gpu_addr; + int ret; - ret = vbox_bo_pin(bo, TTM_PL_FLAG_VRAM, &gpu_addr); + /* Prepare: pin the new framebuffer bo */ + ret = vbox_fb_pin(new_fb, &gpu_addr); if (ret) { - vbox_bo_unreserve(bo); + DRM_WARN("Error %d pinning new fb, out of video mem?\n", ret); return ret; } - if (&vbox->fbdev->afb == vbox_fb) + /* Commit: Update hardware to use the new fb */ + mutex_lock(&vbox->hw_mutex); + + if (&vbox->fbdev->afb == to_vbox_framebuffer(new_fb)) vbox_fbdev_set_base(vbox, gpu_addr); - vbox_bo_unreserve(bo); - /* vbox_set_start_address_crt1(crtc, (u32)gpu_addr); */ vbox_crtc->fb_offset = gpu_addr; - if (vbox_set_up_input_mapping(vbox)) { + + /* vbox_do_modeset() checks vbox->single_framebuffer so update it now */ + if (mode && vbox_set_up_input_mapping(vbox)) { struct drm_crtc *crtci; list_for_each_entry(crtci, &vbox->dev->mode_config.crtc_list, @@ -277,13 +292,20 @@ static int vbox_crtc_do_set_base(struct drm_crtc *crtc, } } - return 0; -} + vbox_set_view(crtc); + vbox_do_modeset(crtc, mode ? mode : &crtc->mode); -static int vbox_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, - struct drm_framebuffer *old_fb) -{ - return vbox_crtc_do_set_base(crtc, old_fb, CRTC_FB(crtc), x, y); + if (mode) + hgsmi_update_input_mapping(vbox->guest_pool, 0, 0, + vbox->input_mapping_width, + vbox->input_mapping_height); + + mutex_unlock(&vbox->hw_mutex); + + /* Cleanup: unpin the old fb */ + vbox_fb_unpin(old_fb); + + return 0; } static int vbox_crtc_mode_set(struct drm_crtc *crtc, @@ -291,21 +313,8 @@ static int vbox_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *adjusted_mode, int x, int y, struct drm_framebuffer *old_fb) { - struct vbox_private *vbox = crtc->dev->dev_private; - int ret; - - vbox_crtc_mode_set_base(crtc, x, y, old_fb); - - mutex_lock(&vbox->hw_mutex); - ret = vbox_set_view(crtc); - if (!ret) - vbox_do_modeset(crtc, mode); - hgsmi_update_input_mapping(vbox->guest_pool, 0, 0, - vbox->input_mapping_width, - vbox->input_mapping_height); - mutex_unlock(&vbox->hw_mutex); - - return ret; + return vbox_crtc_set_base_and_mode(crtc, old_fb, CRTC_FB(crtc), + mode, x, y); } static int vbox_crtc_page_flip(struct drm_crtc *crtc, @@ -319,15 +328,10 @@ static int vbox_crtc_page_flip(struct drm_crtc *crtc, unsigned long flags; int rc; - rc = vbox_crtc_do_set_base(crtc, CRTC_FB(crtc), fb, 0, 0); + rc = vbox_crtc_set_base_and_mode(crtc, CRTC_FB(crtc), fb, NULL, 0, 0); if (rc) return rc; - mutex_lock(&vbox->hw_mutex); - vbox_set_view(crtc); - vbox_do_modeset(crtc, &crtc->mode); - mutex_unlock(&vbox->hw_mutex); - spin_lock_irqsave(&drm->event_lock, flags); if (event) @@ -354,7 +358,6 @@ static const struct drm_crtc_helper_funcs vbox_crtc_helper_funcs = { .dpms = vbox_crtc_dpms, .mode_fixup = vbox_crtc_mode_fixup, .mode_set = vbox_crtc_mode_set, - /* .mode_set_base = vbox_crtc_mode_set_base, */ .disable = vbox_crtc_disable, .prepare = vbox_crtc_prepare, .commit = vbox_crtc_commit,
The default settings for Linux vms created in VirtualBox allocate only 16M of videomem. When running fullscreen on a 1920x1080 (or bigger) monitor this is not a lot. When using GNOME3 on Wayland we have already been seeing out of video memory errors for a while now. After commit 2408898e3b6c ("staging: vboxvideo: Add page-flip support") this has become much worse as now multiple buffers are used. There is nothing we can do about there not being enough video-mem, but we should handle running out of video-mem properly, currently there are 2 problems with this: 1) vbox_crtc_mode_set() does not check if vbox_crtc_mode_set_base() fails at all and does not properly propagate the oom error. 2) vbox_crtc_do_set_base() unpins the old fb too soon: 2.1) It unpins it before pinning the new fb, so if the pinning of the new fb fails (which it will when we run out of video-mem), then we also cannot fall back to the old-fb as it has been already unpinned. We could try to re-pin it but there is no guarantee that will succeed. 2.2) It unpins it before reprogramming the hardware to scan out from the new-fb, which could lead to some ugliness where the hw is scanning out the oldfb while it is being replaced with something else. Fixing this requires to do things in this order: 1) Pin the new fb 2) Program the hw 3) Unpin the oldfb This needs to be done for both a mode_set and for a page_flip so this commit re-writes vbox_crtc_do_set_base() into vbox_crtc_set_base_and_mode() which does this in the correct order, putting the hardware programming which was duplicated between the mode_set and page_flip code inside the new function. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/staging/vboxvideo/vbox_mode.c | 121 +++++++++++++------------- 1 file changed, 62 insertions(+), 59 deletions(-)