Commit 1daddbc8dec5 ("staging: vboxvideo: Update driver to use drm_dev_register.") replaced the obsolere drm_get_pci_dev() with normal pci probe and remove functions.
But the new vbox_pci_probe() is missing a pci_enable_device() call, causing interrupts to not be delivered. This causes resizes of the vm window to not get seen by the drm/kms code.
This commit adds the missing pci_enable_device() call, fixing this.
Fixes: 1daddbc8dec5 ("staging: vboxvideo: Update driver to use ...") Cc: Fabio Rafael da Rosa fdr@pid42.net Cc: Nicholas Mc Guire der.herr@hofr.at Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/staging/vboxvideo/vbox_drv.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/staging/vboxvideo/vbox_drv.c b/drivers/staging/vboxvideo/vbox_drv.c index da92c493f157..69cc508af1bc 100644 --- a/drivers/staging/vboxvideo/vbox_drv.c +++ b/drivers/staging/vboxvideo/vbox_drv.c @@ -59,6 +59,11 @@ static int vbox_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) ret = PTR_ERR(dev); goto err_drv_alloc; } + + ret = pci_enable_device(pdev); + if (ret) + goto err_pci_enable; + dev->pdev = pdev; pci_set_drvdata(pdev, dev);
@@ -75,6 +80,8 @@ static int vbox_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) err_drv_dev_register: vbox_driver_unload(dev); err_vbox_driver_load: + pci_disable_device(pdev); + err_pci_enable: drm_dev_put(dev); err_drv_alloc: return ret;
Commit 2408898e3b6c ("staging: vboxvideo: Add page-flip support") only calls vbox_crtc_do_set_base() on page-flips, but despite that function's name it only pins the new fb, unpins the old fb and sets vbox_crtc->fb_offset. It does not program the hardware to scan out at the new vbox_crtc->fb_offset value.
This was causing only every other frame (assuming page-flipping between 2 buffers) to be shown since we kept scanning out of the old (now unpinned!) buffer.
This commit fixes this by adding code to vbox_crtc_page_flip() to tell the hardware to scanout from the new fb_offset.
Fixes: 2408898e3b6c ("staging: vboxvideo: Add page-flip support") Cc: Steve Longerbeam steve_longerbeam@mentor.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/staging/vboxvideo/vbox_mode.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c index a83eac8668d0..79836c8fb909 100644 --- a/drivers/staging/vboxvideo/vbox_mode.c +++ b/drivers/staging/vboxvideo/vbox_mode.c @@ -323,6 +323,11 @@ static int vbox_crtc_page_flip(struct drm_crtc *crtc, 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)
Hi Hans,
On 09/10/2018 11:30 AM, Hans de Goede wrote:
Commit 2408898e3b6c ("staging: vboxvideo: Add page-flip support") only calls vbox_crtc_do_set_base() on page-flips, but despite that function's name it only pins the new fb, unpins the old fb and sets vbox_crtc->fb_offset. It does not program the hardware to scan out at the new vbox_crtc->fb_offset value.
Has that always been the case of vbox_crtc_do_set_base()? Or was there a recent commit that changed that behavior?
I tested this patch using a Weston EGL mock navigation test app around 4.14 time-frame, that exercises page flip and it was scanning out the new fb, but maybe what I was looking at was a scan-out of an old/now stale fb from a previous page-flip.
In any case thanks for fixing.
Steve
This was causing only every other frame (assuming page-flipping between 2 buffers) to be shown since we kept scanning out of the old (now unpinned!) buffer.
This commit fixes this by adding code to vbox_crtc_page_flip() to tell the hardware to scanout from the new fb_offset.
Fixes: 2408898e3b6c ("staging: vboxvideo: Add page-flip support") Cc: Steve Longerbeam steve_longerbeam@mentor.com Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/staging/vboxvideo/vbox_mode.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c index a83eac8668d0..79836c8fb909 100644 --- a/drivers/staging/vboxvideo/vbox_mode.c +++ b/drivers/staging/vboxvideo/vbox_mode.c @@ -323,6 +323,11 @@ static int vbox_crtc_page_flip(struct drm_crtc *crtc, 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)
Hi,
On 11-09-18 01:08, Steve Longerbeam wrote:
Hi Hans,
On 09/10/2018 11:30 AM, Hans de Goede wrote:
Commit 2408898e3b6c ("staging: vboxvideo: Add page-flip support") only calls vbox_crtc_do_set_base() on page-flips, but despite that function's name it only pins the new fb, unpins the old fb and sets vbox_crtc->fb_offset. It does not program the hardware to scan out at the new vbox_crtc->fb_offset value.
Has that always been the case of vbox_crtc_do_set_base()? Or was there a recent commit that changed that behavior?
I believe that always was the case.
I tested this patch using a Weston EGL mock navigation test app around 4.14 time-frame, that exercises page flip and it was scanning out the new fb, but maybe what I was looking at was a scan-out of an old/now stale fb from a previous page-flip.
Yes I think that is what you saw, this was not easy to notice, I noticed that when typing slowly my letters would always appear 2 at a time, then I deliberately typed: 'a' waited a couple of seconds and the 'a' would not show up until I also typed 'b' and then both showed up at once.
This was with GNOME3 as Wayland compositor, which I believe stops sending new frames when nothing changes which makes this very noticeable :)
In any case thanks for fixing.
You are welcome.
Regards,
Hans
Steve
This was causing only every other frame (assuming page-flipping between 2 buffers) to be shown since we kept scanning out of the old (now unpinned!) buffer.
This commit fixes this by adding code to vbox_crtc_page_flip() to tell the hardware to scanout from the new fb_offset.
Fixes: 2408898e3b6c ("staging: vboxvideo: Add page-flip support") Cc: Steve Longerbeam steve_longerbeam@mentor.com Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/staging/vboxvideo/vbox_mode.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c index a83eac8668d0..79836c8fb909 100644 --- a/drivers/staging/vboxvideo/vbox_mode.c +++ b/drivers/staging/vboxvideo/vbox_mode.c @@ -323,6 +323,11 @@ static int vbox_crtc_page_flip(struct drm_crtc *crtc, 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)
On Mon, Sep 10, 2018 at 08:30:38PM +0200, Hans de Goede wrote:
Commit 1daddbc8dec5 ("staging: vboxvideo: Update driver to use drm_dev_register.") replaced the obsolere drm_get_pci_dev() with normal pci probe and remove functions.
But the new vbox_pci_probe() is missing a pci_enable_device() call, causing interrupts to not be delivered. This causes resizes of the vm window to not get seen by the drm/kms code.
This commit adds the missing pci_enable_device() call, fixing this.
pci_enable_device is the wrapper to pci_enable_device_flags() the later return < 0 on error - so while the check for if(ret) will do the right think here I think it would be prefereable to explicidly use if (ret < 0) as all error values pci_enable_device_flags() returns are negative.
Fixes: 1daddbc8dec5 ("staging: vboxvideo: Update driver to use ...") Cc: Fabio Rafael da Rosa fdr@pid42.net Cc: Nicholas Mc Guire der.herr@hofr.at Signed-off-by: Hans de Goede hdegoede@redhat.com
Reviewed-by: Nicholas Mc Guire der.herr@hofr.at
drivers/staging/vboxvideo/vbox_drv.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/staging/vboxvideo/vbox_drv.c b/drivers/staging/vboxvideo/vbox_drv.c index da92c493f157..69cc508af1bc 100644 --- a/drivers/staging/vboxvideo/vbox_drv.c +++ b/drivers/staging/vboxvideo/vbox_drv.c @@ -59,6 +59,11 @@ static int vbox_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) ret = PTR_ERR(dev); goto err_drv_alloc; }
- ret = pci_enable_device(pdev);
- if (ret)
goto err_pci_enable;- dev->pdev = pdev; pci_set_drvdata(pdev, dev);
@@ -75,6 +80,8 @@ static int vbox_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) err_drv_dev_register: vbox_driver_unload(dev); err_vbox_driver_load:
- pci_disable_device(pdev);
- err_pci_enable: drm_dev_put(dev); err_drv_alloc: return ret;
-- 2.19.0.rc0
Hi,
On 11-09-18 08:48, Nicholas Mc Guire wrote:
On Mon, Sep 10, 2018 at 08:30:38PM +0200, Hans de Goede wrote:
Commit 1daddbc8dec5 ("staging: vboxvideo: Update driver to use drm_dev_register.") replaced the obsolere drm_get_pci_dev() with normal pci probe and remove functions.
But the new vbox_pci_probe() is missing a pci_enable_device() call, causing interrupts to not be delivered. This causes resizes of the vm window to not get seen by the drm/kms code.
This commit adds the missing pci_enable_device() call, fixing this.
pci_enable_device is the wrapper to pci_enable_device_flags() the later return < 0 on error - so while the check for if(ret) will do the right think here I think it would be prefereable to explicidly use if (ret < 0) as all error values pci_enable_device_flags() returns are negative.
The use of "if (ret)" checks for functions which return 0 on success negative value on error is a standard pattern in the kernel, so I would prefer to keep this as is.
Fixes: 1daddbc8dec5 ("staging: vboxvideo: Update driver to use ...") Cc: Fabio Rafael da Rosa fdr@pid42.net Cc: Nicholas Mc Guire der.herr@hofr.at Signed-off-by: Hans de Goede hdegoede@redhat.com
Reviewed-by: Nicholas Mc Guire der.herr@hofr.at
Thanks.
Regards,
Hans
drivers/staging/vboxvideo/vbox_drv.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/staging/vboxvideo/vbox_drv.c b/drivers/staging/vboxvideo/vbox_drv.c index da92c493f157..69cc508af1bc 100644 --- a/drivers/staging/vboxvideo/vbox_drv.c +++ b/drivers/staging/vboxvideo/vbox_drv.c @@ -59,6 +59,11 @@ static int vbox_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) ret = PTR_ERR(dev); goto err_drv_alloc; }
- ret = pci_enable_device(pdev);
- if (ret)
goto err_pci_enable;- dev->pdev = pdev; pci_set_drvdata(pdev, dev);
@@ -75,6 +80,8 @@ static int vbox_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) err_drv_dev_register: vbox_driver_unload(dev); err_vbox_driver_load:
- pci_disable_device(pdev);
- err_pci_enable: drm_dev_put(dev); err_drv_alloc: return ret;
-- 2.19.0.rc0
On Tue, Sep 11, 2018 at 08:53:35AM +0200, Hans de Goede wrote:
Hi,
On 11-09-18 08:48, Nicholas Mc Guire wrote:
On Mon, Sep 10, 2018 at 08:30:38PM +0200, Hans de Goede wrote:
Commit 1daddbc8dec5 ("staging: vboxvideo: Update driver to use drm_dev_register.") replaced the obsolere drm_get_pci_dev() with normal pci probe and remove functions.
But the new vbox_pci_probe() is missing a pci_enable_device() call, causing interrupts to not be delivered. This causes resizes of the vm window to not get seen by the drm/kms code.
This commit adds the missing pci_enable_device() call, fixing this.
pci_enable_device is the wrapper to pci_enable_device_flags() the later return < 0 on error - so while the check for if(ret) will do the right think here I think it would be prefereable to explicidly use if (ret < 0) as all error values pci_enable_device_flags() returns are negative.
The use of "if (ret)" checks for functions which return 0 on success negative value on error is a standard pattern in the kernel, so I would prefer to keep this as is.
Well as noted it does the right thing - just checking the use of pci_enable_device() in the existing drivers it seems that it is roughly balanced between checking < 0 and !0. The rational for < 0 would be that the negative return values mandate a signed type, whilc !0 does not and if someone then uses and unsigned type the error case would return as success while the < 0 would be detected at compile time (or other static code checkers).
thx! hofrat
Fixes: 1daddbc8dec5 ("staging: vboxvideo: Update driver to use ...") Cc: Fabio Rafael da Rosa fdr@pid42.net Cc: Nicholas Mc Guire der.herr@hofr.at Signed-off-by: Hans de Goede hdegoede@redhat.com
Reviewed-by: Nicholas Mc Guire der.herr@hofr.at
Thanks.
Regards,
Hans
drivers/staging/vboxvideo/vbox_drv.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/staging/vboxvideo/vbox_drv.c b/drivers/staging/vboxvideo/vbox_drv.c index da92c493f157..69cc508af1bc 100644 --- a/drivers/staging/vboxvideo/vbox_drv.c +++ b/drivers/staging/vboxvideo/vbox_drv.c @@ -59,6 +59,11 @@ static int vbox_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) ret = PTR_ERR(dev); goto err_drv_alloc; }
- ret = pci_enable_device(pdev);
- if (ret)
goto err_pci_enable;- dev->pdev = pdev; pci_set_drvdata(pdev, dev);
@@ -75,6 +80,8 @@ static int vbox_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) err_drv_dev_register: vbox_driver_unload(dev); err_vbox_driver_load:
- pci_disable_device(pdev);
- err_pci_enable: drm_dev_put(dev); err_drv_alloc: return ret;
-- 2.19.0.rc0
On Tue, Sep 11, 2018 at 07:41:10AM +0000, Nicholas Mc Guire wrote:
On Tue, Sep 11, 2018 at 08:53:35AM +0200, Hans de Goede wrote:
Hi,
On 11-09-18 08:48, Nicholas Mc Guire wrote:
On Mon, Sep 10, 2018 at 08:30:38PM +0200, Hans de Goede wrote:
Commit 1daddbc8dec5 ("staging: vboxvideo: Update driver to use drm_dev_register.") replaced the obsolere drm_get_pci_dev() with normal pci probe and remove functions.
But the new vbox_pci_probe() is missing a pci_enable_device() call, causing interrupts to not be delivered. This causes resizes of the vm window to not get seen by the drm/kms code.
This commit adds the missing pci_enable_device() call, fixing this.
pci_enable_device is the wrapper to pci_enable_device_flags() the later return < 0 on error - so while the check for if(ret) will do the right think here I think it would be prefereable to explicidly use if (ret < 0) as all error values pci_enable_device_flags() returns are negative.
The use of "if (ret)" checks for functions which return 0 on success negative value on error is a standard pattern in the kernel, so I would prefer to keep this as is.
Well as noted it does the right thing - just checking the use of pci_enable_device() in the existing drivers it seems that it is roughly balanced between checking < 0 and !0. The rational for < 0 would be that the negative return values mandate a signed type, whilc !0 does not and if someone then uses and unsigned type the error case would return as success while the < 0 would be detected at compile time (or other static code checkers).
If the function returns int but ret is an unsigned int and we do "if (ret)", then yes we don't print a static checker warning. But the code works perfectly so it doesn't matter.
I'm going to publish some code soon which will complain if a function returns specific negatives and you save it to an unsigned type which is smaller than an int.
regards, dan carpenter
On Tue, Sep 11, 2018 at 06:48:27AM +0000, Nicholas Mc Guire wrote:
On Mon, Sep 10, 2018 at 08:30:38PM +0200, Hans de Goede wrote:
Commit 1daddbc8dec5 ("staging: vboxvideo: Update driver to use drm_dev_register.") replaced the obsolere drm_get_pci_dev() with normal pci probe and remove functions.
But the new vbox_pci_probe() is missing a pci_enable_device() call, causing interrupts to not be delivered. This causes resizes of the vm window to not get seen by the drm/kms code.
This commit adds the missing pci_enable_device() call, fixing this.
pci_enable_device is the wrapper to pci_enable_device_flags() the later return < 0 on error - so while the check for if(ret) will do the right think here I think it would be prefereable to explicidly use if (ret < 0) as all error values pci_enable_device_flags() returns are negative.
It could go either way, I think. "if (ret) " is sort of as explicit as "if (ret < 0) " when you consider the false side. When I see "if (ret)" then I know the code returns negative error codes and zero, but when I see "if (ret < 0)" then I think maybe this returns positive non-zero values as well.
As a static analysis person, the "if (ret)" style is easier for me. Sometimes Smatch doesn't know what a function returns. Maybe the error code comes from a different thread and Smatch doesn't understand threads. So then when people use "if (ret)" Smatch knows that non-zero means *param1 is not initialized. Then the caller does "if (ret < 0)" that means that positive non-zero values are not handled so let's print an uninitialized variable warning. Just to spell it out a little more, the error code won't be printed for "if (ret)" because negatives are a subset of non-zero.
Of course, if you do it consistently there won't be a warning message. I never see the consistent subsystems, so I don't know if they exist.
regards, dan carpenter
On Tue, Sep 11, 2018 at 10:20:41AM +0300, Dan Carpenter wrote:
On Tue, Sep 11, 2018 at 06:48:27AM +0000, Nicholas Mc Guire wrote:
On Mon, Sep 10, 2018 at 08:30:38PM +0200, Hans de Goede wrote:
Commit 1daddbc8dec5 ("staging: vboxvideo: Update driver to use drm_dev_register.") replaced the obsolere drm_get_pci_dev() with normal pci probe and remove functions.
But the new vbox_pci_probe() is missing a pci_enable_device() call, causing interrupts to not be delivered. This causes resizes of the vm window to not get seen by the drm/kms code.
This commit adds the missing pci_enable_device() call, fixing this.
pci_enable_device is the wrapper to pci_enable_device_flags() the later return < 0 on error - so while the check for if(ret) will do the right think here I think it would be prefereable to explicidly use if (ret < 0) as all error values pci_enable_device_flags() returns are negative.
It could go either way, I think. "if (ret) " is sort of as explicit as "if (ret < 0) " when you consider the false side. When I see "if (ret)" then I know the code returns negative error codes and zero, but when I see "if (ret < 0)" then I think maybe this returns positive non-zero values as well.
As a static analysis person, the "if (ret)" style is easier for me. Sometimes Smatch doesn't know what a function returns. Maybe the error code comes from a different thread and Smatch doesn't understand threads. So then when people use "if (ret)" Smatch knows that non-zero means *param1 is not initialized. Then the caller does "if (ret < 0)" that means that positive non-zero values are not handled so let's print an uninitialized variable warning. Just to spell it out a little more, the error code won't be printed for "if (ret)" because negatives are a subset of non-zero.
Of course, if you do it consistently there won't be a warning message. I never see the consistent subsystems, so I don't know if they exist.
Probably true - there is quite a bit of incorrect type issues in the kernel and there are a cases of comparing to e.g. <= 0 for signed types is used, so I personally prefere if the check allows type inference - if I see a "ret < 0" it can be infered that the type must be signed and an unsigned is an error while for !0 case does not allow such inference.
Anyway - as noted the patch seems correct with respect to the intent and if the general preference is for "if (ret)" then no objections.
thanks for the clarification !
hofrat
dri-devel@lists.freedesktop.org
-
Dan Carpenter -
Hans de Goede -
Nicholas Mc Guire -
Steve Longerbeam