Warn when refcount > 0 in drm_vblank_cleanup.
Signed-off-by: PrasannaKumar Muralidharan prasannatsmkumar@gmail.com --- No change in v2.
drivers/gpu/drm/drm_vblank.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 70f2b95..3e61aeb 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -405,6 +405,8 @@ void drm_vblank_cleanup(struct drm_device *dev) for (pipe = 0; pipe < dev->num_crtcs; pipe++) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
+ WARN_ON(atomic_read(&vblank->refcount) > 0); + WARN_ON(READ_ONCE(vblank->enabled) && drm_core_check_feature(dev, DRIVER_MODESET));
In i915 driver unload drm_vblank_get is added to test whether drm_vblank_cleanup refcount validation patch is working.
Signed-off-by: PrasannaKumar Muralidharan prasannatsmkumar@gmail.com --- Changes in v2: Use drm_crtc_vblank_get instead of _put. In previous patch _put was wrongly used.
drivers/gpu/drm/i915/i915_drv.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9f45cfe..4aee1c0 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1373,6 +1373,13 @@ void i915_driver_unload(struct drm_device *dev) struct drm_i915_private *dev_priv = to_i915(dev); struct pci_dev *pdev = dev_priv->drm.pdev;
+ enum pipe pipe; + for_each_pipe(dev_priv, pipe) { + struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, + pipe); + drm_crtc_vblank_get(&crtc->base); + } + i915_driver_unregister(dev_priv);
if (i915_gem_suspend(dev_priv))
Hi All,
On 24 October 2017 at 22:18, PrasannaKumar Muralidharan prasannatsmkumar@gmail.com wrote:
In i915 driver unload drm_vblank_get is added to test whether drm_vblank_cleanup refcount validation patch is working.
Signed-off-by: PrasannaKumar Muralidharan prasannatsmkumar@gmail.com
Changes in v2: Use drm_crtc_vblank_get instead of _put. In previous patch _put was wrongly used.
drivers/gpu/drm/i915/i915_drv.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9f45cfe..4aee1c0 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1373,6 +1373,13 @@ void i915_driver_unload(struct drm_device *dev) struct drm_i915_private *dev_priv = to_i915(dev); struct pci_dev *pdev = dev_priv->drm.pdev;
enum pipe pipe;for_each_pipe(dev_priv, pipe) {struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,pipe);drm_crtc_vblank_get(&crtc->base);}i915_driver_unregister(dev_priv); if (i915_gem_suspend(dev_priv))-- 2.10.0
From https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6167/fi-ilk-650/igt@drv_m...
it can be seen that this patch triggers warning when vblank->refcount
0 in vblank cleanup. This tests patch 1 successfully.
I think patch 1 is good to go.
Thanks, PrasannaKumar
On Wed, Oct 25, 2017 at 08:44:45PM +0530, PrasannaKumar Muralidharan wrote:
Hi All,
On 24 October 2017 at 22:18, PrasannaKumar Muralidharan prasannatsmkumar@gmail.com wrote:
In i915 driver unload drm_vblank_get is added to test whether drm_vblank_cleanup refcount validation patch is working.
Signed-off-by: PrasannaKumar Muralidharan prasannatsmkumar@gmail.com
Changes in v2: Use drm_crtc_vblank_get instead of _put. In previous patch _put was wrongly used.
drivers/gpu/drm/i915/i915_drv.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9f45cfe..4aee1c0 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1373,6 +1373,13 @@ void i915_driver_unload(struct drm_device *dev) struct drm_i915_private *dev_priv = to_i915(dev); struct pci_dev *pdev = dev_priv->drm.pdev;
enum pipe pipe;for_each_pipe(dev_priv, pipe) {struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,pipe);drm_crtc_vblank_get(&crtc->base);}i915_driver_unregister(dev_priv); if (i915_gem_suspend(dev_priv))-- 2.10.0
From https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6167/fi-ilk-650/igt@drv_m... it can be seen that this patch triggers warning when vblank->refcount
0 in vblank cleanup. This tests patch 1 successfully.
I think patch 1 is good to go.
Yeah it works, but we don't know whether it breaks anything yet. Can you pls resubmit just the first patch? Abusing CI was just an idea to get it fully tested, before we can merge it still needs to pass full CI. We just had an issue 2 weeks ago where CI blew up because an untested patch landed that broke every test :-/ -Daniel
Hi Daniel,
On 30 October 2017 at 15:40, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Oct 25, 2017 at 08:44:45PM +0530, PrasannaKumar Muralidharan wrote:
Hi All,
On 24 October 2017 at 22:18, PrasannaKumar Muralidharan prasannatsmkumar@gmail.com wrote:
In i915 driver unload drm_vblank_get is added to test whether drm_vblank_cleanup refcount validation patch is working.
Signed-off-by: PrasannaKumar Muralidharan prasannatsmkumar@gmail.com
Changes in v2: Use drm_crtc_vblank_get instead of _put. In previous patch _put was wrongly used.
drivers/gpu/drm/i915/i915_drv.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9f45cfe..4aee1c0 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1373,6 +1373,13 @@ void i915_driver_unload(struct drm_device *dev) struct drm_i915_private *dev_priv = to_i915(dev); struct pci_dev *pdev = dev_priv->drm.pdev;
enum pipe pipe;for_each_pipe(dev_priv, pipe) {struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,pipe);drm_crtc_vblank_get(&crtc->base);}i915_driver_unregister(dev_priv); if (i915_gem_suspend(dev_priv))-- 2.10.0
From https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6167/fi-ilk-650/igt@drv_m... it can be seen that this patch triggers warning when vblank->refcount
0 in vblank cleanup. This tests patch 1 successfully.
I think patch 1 is good to go.
Yeah it works, but we don't know whether it breaks anything yet. Can you pls resubmit just the first patch? Abusing CI was just an idea to get it fully tested, before we can merge it still needs to pass full CI. We just had an issue 2 weeks ago where CI blew up because an untested patch landed that broke every test :-/
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
I have already sent that patch alone. Please look at https://patchwork.freedesktop.org/patch/184866/.
Regards, PrasannaKumar
On Mon, Oct 30, 2017 at 06:01:12PM +0530, PrasannaKumar Muralidharan wrote:
Hi Daniel,
On 30 October 2017 at 15:40, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Oct 25, 2017 at 08:44:45PM +0530, PrasannaKumar Muralidharan wrote:
Hi All,
On 24 October 2017 at 22:18, PrasannaKumar Muralidharan prasannatsmkumar@gmail.com wrote:
In i915 driver unload drm_vblank_get is added to test whether drm_vblank_cleanup refcount validation patch is working.
Signed-off-by: PrasannaKumar Muralidharan prasannatsmkumar@gmail.com
Changes in v2: Use drm_crtc_vblank_get instead of _put. In previous patch _put was wrongly used.
drivers/gpu/drm/i915/i915_drv.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9f45cfe..4aee1c0 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1373,6 +1373,13 @@ void i915_driver_unload(struct drm_device *dev) struct drm_i915_private *dev_priv = to_i915(dev); struct pci_dev *pdev = dev_priv->drm.pdev;
enum pipe pipe;for_each_pipe(dev_priv, pipe) {struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,pipe);drm_crtc_vblank_get(&crtc->base);}i915_driver_unregister(dev_priv); if (i915_gem_suspend(dev_priv))-- 2.10.0
From https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6167/fi-ilk-650/igt@drv_m... it can be seen that this patch triggers warning when vblank->refcount
0 in vblank cleanup. This tests patch 1 successfully.
I think patch 1 is good to go.
Yeah it works, but we don't know whether it breaks anything yet. Can you pls resubmit just the first patch? Abusing CI was just an idea to get it fully tested, before we can merge it still needs to pass full CI. We just had an issue 2 weeks ago where CI blew up because an untested patch landed that broke every test :-/
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
I have already sent that patch alone. Please look at https://patchwork.freedesktop.org/patch/184866/.
Seems to fail in CI:
https://patchwork.freedesktop.org/series/32648/
I guess you need to test this on a local box somewhere to figure out what's wrong. -Daniel
Hi Daniel,
On 31 October 2017 at 15:51, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Oct 30, 2017 at 06:01:12PM +0530, PrasannaKumar Muralidharan wrote:
Hi Daniel,
On 30 October 2017 at 15:40, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Oct 25, 2017 at 08:44:45PM +0530, PrasannaKumar Muralidharan wrote:
Hi All,
On 24 October 2017 at 22:18, PrasannaKumar Muralidharan prasannatsmkumar@gmail.com wrote:
In i915 driver unload drm_vblank_get is added to test whether drm_vblank_cleanup refcount validation patch is working.
Signed-off-by: PrasannaKumar Muralidharan prasannatsmkumar@gmail.com
Changes in v2: Use drm_crtc_vblank_get instead of _put. In previous patch _put was wrongly used.
drivers/gpu/drm/i915/i915_drv.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9f45cfe..4aee1c0 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1373,6 +1373,13 @@ void i915_driver_unload(struct drm_device *dev) struct drm_i915_private *dev_priv = to_i915(dev); struct pci_dev *pdev = dev_priv->drm.pdev;
enum pipe pipe;for_each_pipe(dev_priv, pipe) {struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,pipe);drm_crtc_vblank_get(&crtc->base);}i915_driver_unregister(dev_priv); if (i915_gem_suspend(dev_priv))-- 2.10.0
From https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6167/fi-ilk-650/igt@drv_m... it can be seen that this patch triggers warning when vblank->refcount
0 in vblank cleanup. This tests patch 1 successfully.
I think patch 1 is good to go.
Yeah it works, but we don't know whether it breaks anything yet. Can you pls resubmit just the first patch? Abusing CI was just an idea to get it fully tested, before we can merge it still needs to pass full CI. We just had an issue 2 weeks ago where CI blew up because an untested patch landed that broke every test :-/
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
I have already sent that patch alone. Please look at https://patchwork.freedesktop.org/patch/184866/.
Seems to fail in CI:
https://patchwork.freedesktop.org/series/32648/
I guess you need to test this on a local box somewhere to figure out what's wrong.
My patch is supposed to catch problem with drivers. It warns when vblank refcount is non-zero in drm_vblank_cleanup call. From CI log it can be seen that warning being triggered. I feel that my patch is exposing existing issue.
If I misinterpreted something please let me know.
Thanks, PrasannaKumar
On Tue, Oct 31, 2017 at 08:37:21PM +0530, PrasannaKumar Muralidharan wrote:
Hi Daniel,
On 31 October 2017 at 15:51, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Oct 30, 2017 at 06:01:12PM +0530, PrasannaKumar Muralidharan wrote:
Hi Daniel,
On 30 October 2017 at 15:40, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Oct 25, 2017 at 08:44:45PM +0530, PrasannaKumar Muralidharan wrote:
Hi All,
On 24 October 2017 at 22:18, PrasannaKumar Muralidharan prasannatsmkumar@gmail.com wrote:
In i915 driver unload drm_vblank_get is added to test whether drm_vblank_cleanup refcount validation patch is working.
Signed-off-by: PrasannaKumar Muralidharan prasannatsmkumar@gmail.com
Changes in v2: Use drm_crtc_vblank_get instead of _put. In previous patch _put was wrongly used.
drivers/gpu/drm/i915/i915_drv.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9f45cfe..4aee1c0 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1373,6 +1373,13 @@ void i915_driver_unload(struct drm_device *dev) struct drm_i915_private *dev_priv = to_i915(dev); struct pci_dev *pdev = dev_priv->drm.pdev;
enum pipe pipe;for_each_pipe(dev_priv, pipe) {struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,pipe);drm_crtc_vblank_get(&crtc->base);}i915_driver_unregister(dev_priv); if (i915_gem_suspend(dev_priv))-- 2.10.0
From https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6167/fi-ilk-650/igt@drv_m... it can be seen that this patch triggers warning when vblank->refcount
0 in vblank cleanup. This tests patch 1 successfully.
I think patch 1 is good to go.
Yeah it works, but we don't know whether it breaks anything yet. Can you pls resubmit just the first patch? Abusing CI was just an idea to get it fully tested, before we can merge it still needs to pass full CI. We just had an issue 2 weeks ago where CI blew up because an untested patch landed that broke every test :-/
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
I have already sent that patch alone. Please look at https://patchwork.freedesktop.org/patch/184866/.
Seems to fail in CI:
https://patchwork.freedesktop.org/series/32648/
I guess you need to test this on a local box somewhere to figure out what's wrong.
My patch is supposed to catch problem with drivers. It warns when vblank refcount is non-zero in drm_vblank_cleanup call. From CI log it can be seen that warning being triggered. I feel that my patch is exposing existing issue.
If I misinterpreted something please let me know.
This is probably what's happening, but I can't merge a patch that breaks CI. So we need to track down that issue before merging. -Daniel
Hi Daniel,
On 31 October 2017 at 21:57, Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Oct 31, 2017 at 08:37:21PM +0530, PrasannaKumar Muralidharan wrote:
My patch is supposed to catch problem with drivers. It warns when vblank refcount is non-zero in drm_vblank_cleanup call. From CI log it can be seen that warning being triggered. I feel that my patch is exposing existing issue.
If I misinterpreted something please let me know.
This is probably what's happening, but I can't merge a patch that breaks CI. So we need to track down that issue before merging.
I understand. Being new to DRM subsystem I don't know if I can contribute in finding the issue. I would be able to help if I could get some guidance.
Thanks, PrasannaKumar
On Wed, Nov 01, 2017 at 09:48:28AM +0530, PrasannaKumar Muralidharan wrote:
Hi Daniel,
On 31 October 2017 at 21:57, Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Oct 31, 2017 at 08:37:21PM +0530, PrasannaKumar Muralidharan wrote:
My patch is supposed to catch problem with drivers. It warns when vblank refcount is non-zero in drm_vblank_cleanup call. From CI log it can be seen that warning being triggered. I feel that my patch is exposing existing issue.
If I misinterpreted something please let me know.
This is probably what's happening, but I can't merge a patch that breaks CI. So we need to track down that issue before merging.
I understand. Being new to DRM subsystem I don't know if I can contribute in finding the issue. I would be able to help if I could get some guidance.
If you have an intel laptop anywhere at hand that you could use to reproduce the issue, that would be a good start.
Then go through the setup for the kms/drm validation suite:
https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#testing-and-validatio...
That should allow you to locally reproduce the issue (it seems to affect many machines, so I'd assume it doesn't matter which one you have).
Once you can reproduce it should be doable to figure out where we leak this reference (but usually it's a bit of hard work).
Btw I discussed your patch a bit on irc, a first step would be to also print the refcount when it's leaked, not just warn about it. Knowing how many references are leaked is sometimes a good hint about what's going on. -Daniel
Hi Daniel,
On 1 November 2017 at 14:23, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Nov 01, 2017 at 09:48:28AM +0530, PrasannaKumar Muralidharan wrote:
Hi Daniel,
On 31 October 2017 at 21:57, Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Oct 31, 2017 at 08:37:21PM +0530, PrasannaKumar Muralidharan wrote:
My patch is supposed to catch problem with drivers. It warns when vblank refcount is non-zero in drm_vblank_cleanup call. From CI log it can be seen that warning being triggered. I feel that my patch is exposing existing issue.
If I misinterpreted something please let me know.
This is probably what's happening, but I can't merge a patch that breaks CI. So we need to track down that issue before merging.
I understand. Being new to DRM subsystem I don't know if I can contribute in finding the issue. I would be able to help if I could get some guidance.
If you have an intel laptop anywhere at hand that you could use to reproduce the issue, that would be a good start.
I do have one but it is my primary machine.
Then go through the setup for the kms/drm validation suite:
https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#testing-and-validatio...
That should allow you to locally reproduce the issue (it seems to affect many machines, so I'd assume it doesn't matter which one you have).
If this setup is going to affect my normal workflow setup I would prefer not to use it.
Once you can reproduce it should be doable to figure out where we leak this reference (but usually it's a bit of hard work).
Anyway I will give it a try and see how far I can go.
Btw I discussed your patch a bit on irc, a first step would be to also print the refcount when it's leaked, not just warn about it. Knowing how many references are leaked is sometimes a good hint about what's going on.
I will send a v4.
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Regards, PrasannaKumar
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
PrasannaKumar Muralidharan