drm: Put damage blob when destroy plane state

PATCH In review Deepak Rawat 4 participants 6 messages Dec 21, 2018 – Dec 27, 2018

Subsystems (2)

DRM drivers DRM misc

lore

Share & tools

lore

Deepak Rawat

author

2018-12-21 19:36 UTC #0 lore raw

PATCH 1/1 drm: Put damage blob when destroy plane state

Somehow the code to put the damage blob on destroy plane state and set the blob to NULL when duplicate plane state was not merged. May be because the files are refactored since the patch was written. With this fix add those.

Cc Daniel Vetter <[email protected]>

Signed-off-by Deepak Rawat <[email protected]>

---
 drivers/gpu/drm/drm_atomic_state_helper.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/drm_atomic_state_helper.c +3 -0

index 3ba996069d69..709355c6bac6 100644
@@ -241,6 +241,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 
 	state->fence = NULL;
 	state->commit = NULL;
+	state->fb_damage_clips = NULL;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);

@@ -285,6 +286,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 
 	if (state->commit)
 		drm_crtc_commit_put(state->commit);
+
+	drm_property_blob_put(state->fb_damage_clips);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);

Signature · 6 lines

2.17.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Thomas Hellstrom

Reviewed-by on v1 1/1

2018-12-21 19:56 UTC #1 lore raw

Reviewed-by Thomas Hellstrom <[email protected]>

Daniel, Dave, could you help try to get this patch in -next before the merge window. Otherwise people will start to experience random kernel crashes. I figure we don't want to wait until first -fixes pull with this.

Thanks,
Thomas

Quoted 32 lines · Deepak Rawat

Somehow the code to put the damage blob on destroy plane state and set
the blob to NULL when duplicate plane state was not merged. May be because the files are refactored since the patch was written. With this
fix add those.
Cc Daniel Vetter <[email protected]>
Signed-off-by Deepak Rawat <[email protected]>
---
 drivers/gpu/drm/drm_atomic_state_helper.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/gpu/drm/drm_atomic_state_helper.c +1 -0
index 3ba996069d69..709355c6bac6 100644
@@ -241,6 +241,7 @@ void
 __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 
 	state->fence = NULL;
 	state->commit = NULL;
+	state->fb_damage_clips = NULL;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
@@ -285,6 +286,8 @@ void
__drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)

drivers/gpu/drm/drm_atomic_state_helper.c void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) · line 292

Quoted 6 lines

drivers/gpu/drm/drm_atomic_state_helper.c +2 -0

 	if (state->commit)
 		drm_crtc_commit_put(state->commit);
+
+	drm_property_blob_put(state->fb_damage_clips);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter

2018-12-24 10:50 UTC #2 lore raw

Quoted 7 lines · Thomas Hellstrom

Reviewed-by Thomas Hellstrom <[email protected]>
Daniel, Dave, could you help try to get this patch in -next before the merge window. Otherwise people will start to experience random kernel crashes. I figure we don't want to wait until first -fixes pull with this.

Afaiui this will only blow up with new userspace on new kernels, that doesn't feel like rushing an updated -next out the door material to me. More concerning is why this fell through the cracks:
- We have an igt, do those testcases not hit this bug?
- Were the tests not run before you've sent out the pull?
- The merged version doesn't seem to match any of the versions I've found on dri-devel, I guess should have been resend when there was conflicts?
- Some other crack in the matrix?

Quoted 4 lines

Thanks,
Thomas

Quoted 10 lines

Quoted 9 lines · Deepak Rawat
Somehow the code to put the damage blob on destroy plane state and set
the blob to NULL when duplicate plane state was not merged. May be because the files are refactored since the patch was written. With this
fix add those.
Cc Daniel Vetter <[email protected]>
Signed-off-by Deepak Rawat <[email protected]>

Needs a Fixes: tag referencing the broken commit, pls remember to add these anytime you fix an issue with a commit. I'll add that and push it to drm-misc-next-fixes.

Thanks, Daniel

Quoted 22 lines

---
 drivers/gpu/drm/drm_atomic_state_helper.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/drm_atomic_state_helper.c +1 -0

index 3ba996069d69..709355c6bac6 100644
@@ -241,6 +241,7 @@ void
 __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 
      state->fence = NULL;
      state->commit = NULL;
+     state->fb_damage_clips = NULL;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);

@@ -285,6 +286,8 @@ void

__drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)

Quoted 7 lines

drivers/gpu/drm/drm_atomic_state_helper.c void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) · line 292
Quoted 7 lines
      if (state->commit)
              drm_crtc_commit_put(state->commit);
drivers/gpu/drm/drm_atomic_state_helper.c +2 -0
+
+     drm_property_blob_put(state->fb_damage_clips);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Thomas Hellstrom

2018-12-25 15:01 UTC #3 lore raw

Quoted 12 lines · Daniel Vetter

Quoted 9 lines · Thomas Hellstrom
Reviewed-by Thomas Hellstrom <[email protected]>
Daniel, Dave, could you help try to get this patch in -next before the
merge window. Otherwise people will start to experience random kernel
crashes. I figure we don't want to wait until first -fixes pull with
this.

Quoted 3 lines

Afaiui this will only blow up with new userspace on new kernels, that doesn't feel like rushing an updated -next out the door material to me.

Hmm, this was discovered when the drm-read igt test hit an OOPS, I guess when damage_clips is unintentionally Non-Null.

Quoted 1 line

More concerning is why this fell through the cracks:

I agree. Sinc I'm on vacation a very quick answer. Will investigate more thogroughly when we get back.

Quoted 1 line

- We have an igt, do those testcases not hit this bug?

The drm-read igt intermittently hit the bug.

Quoted 1 line

- Were the tests not run before you've sent out the pull?

No, and I guess that't the main culprit. The igt doesn't work well with vmwgfx, mostly because many tests are assuming gem functionality, there fore we doesn't run them regularly, but mostly a pre-commit testsuite that focuses more on rendering correctness than modesetting functionality. That said, Deepak has over time put quite an effort into making at least part of that test suite run well with vmwgfx, but I didn't consider it for this pull request. Will make sure we do in the future for core drm changes.

Quoted 3 lines

- The merged version doesn't seem to match any of the versions I've found on dri-devel, I guess should have been resend when there was conflicts?

Yes, I think there was at least one trivial merge conflict and a couple of minor checkpatch.pl fixups in the pull request. Typically for vmwgfx we don't resend the resulting patches if there are minor changes like that and they are reviewed internally. Need to get back to you about exactly where in the process we failed here.

Thanks,
Thomas

Quoted 49 lines

- Some other crack in the matrix?
Quoted 4 lines
Thanks,
Thomas
Quoted 13 lines
Quoted 12 lines · Deepak Rawat
Somehow the code to put the damage blob on destroy plane state and
set
the blob to NULL when duplicate plane state was not merged. May be
because the files are refactored since the patch was written. With
this
fix add those.
Cc Daniel Vetter <[email protected]>
Signed-off-by Deepak Rawat <[email protected]>
Needs a Fixes: tag referencing the broken commit, pls remember to add these anytime you fix an issue with a commit. I'll add that and push it to drm-misc-next-fixes.
Thanks, Daniel
Quoted 23 lines
Quoted 23 lines
---
 drivers/gpu/drm/drm_atomic_state_helper.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/gpu/drm/drm_atomic_state_helper.c +0 -0
index 3ba996069d69..709355c6bac6 100644
@@ -241,6 +241,7 @@ void
__drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
      state->fence = NULL;
      state->commit = NULL;
drivers/gpu/drm/drm_atomic_state_helper.c +1 -0
+     state->fb_damage_clips = NULL;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
@@ -285,6 +286,8 @@ void
__drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)

Quoted 14 lines

Quoted 7 lines
drivers/gpu/drm/drm_atomic_state_helper.c void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) · line 292
Quoted 7 lines
      if (state->commit)
              drm_crtc_commit_put(state->commit);
drivers/gpu/drm/drm_atomic_state_helper.c +2 -0
+
+     drm_property_blob_put(state->fb_damage_clips);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 -
https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch&data=02%7C01%7Cthellstrom%40vmware.com%7Ca62b8426087f410a689b08d6698d9110%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636812454080233380&sdata=6CZm4TU0A7wLpd1sXOUA3URmMQJqndy1WHxhtjnU8P8%3D&reserved=0

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Deepak Singh Rawat

author

2018-12-26 17:50 UTC #4 lore raw

Quoted 51 lines · Thomas Hellstrom

Quoted 13 lines · Daniel Vetter
Quoted 10 lines · Thomas Hellstrom
Reviewed-by Thomas Hellstrom <[email protected]>
Daniel, Dave, could you help try to get this patch in -next before
the
merge window. Otherwise people will start to experience random kernel
crashes. I figure we don't want to wait until first -fixes pull with
this.
Quoted 4 lines
Afaiui this will only blow up with new userspace on new kernels, that
doesn't feel like rushing an updated -next out the door material to me.
Hmm, this was discovered when the drm-read igt test hit an OOPS, I guess when damage_clips is unintentionally Non-Null.
Quoted 1 line
More concerning is why this fell through the cracks:
I agree. Sinc I'm on vacation a very quick answer. Will investigate more thogroughly when we get back.
Quoted 1 line
- We have an igt, do those testcases not hit this bug?
The drm-read igt intermittently hit the bug.
Quoted 1 line
- Were the tests not run before you've sent out the pull?
No, and I guess that't the main culprit. The igt doesn't work well with
vmwgfx, mostly because many tests are assuming gem functionality, there
fore we doesn't run them regularly, but mostly a pre-commit testsuite that focuses more on rendering correctness than modesetting functionality. That said, Deepak has over time put quite an effort into
making at least part of that test suite run well with vmwgfx, but I didn't consider it for this pull request. Will make sure we do in the future for core drm changes.
Quoted 3 lines
- The merged version doesn't seem to match any of the versions I've found on dri-devel, I guess should have been resend when there was conflicts?

Quoted 7 lines

Yes, I think there was at least one trivial merge conflict and a couple
of minor checkpatch.pl fixups in the pull request. Typically for vmwgfx
we don't resend the resulting patches if there are minor changes like that and they are reviewed internally. Need to get back to you about exactly where in the process we failed here.

I did ran igt after rebasing with upstream drm-next but only kms tests and didn't saw any problem. We have glretrace running continuously with rebooting VM's every time it runs and it too didn't failed so didn't suspected anything wrong with what I sent. Unfortunately we don't run igt continuously so I guess that is what we should work on.

I am yet to investigate why only drm_read failed. Although from stack trace I can see it is failing when set_config is called from vmwgfx fb_dev driver.

Quoted 31 lines

Thanks,
Thomas
Quoted 25 lines
- Some other crack in the matrix?
Quoted 4 lines
Thanks,
Thomas
Quoted 13 lines
Quoted 12 lines · Deepak Rawat
Somehow the code to put the damage blob on destroy plane state and
set
the blob to NULL when duplicate plane state was not merged. May be
because the files are refactored since the patch was written. With
this
fix add those.
Cc Daniel Vetter <[email protected]>
Signed-off-by Deepak Rawat <[email protected]>
Needs a Fixes: tag referencing the broken commit, pls remember to add
these anytime you fix an issue with a commit. I'll add that and push
it to drm-misc-next-fixes.

Thanks Daniel for this. Will make sure in future.

Quoted 26 lines

Quoted 26 lines
Thanks, Daniel
Quoted 23 lines
Quoted 23 lines
---
 drivers/gpu/drm/drm_atomic_state_helper.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/gpu/drm/drm_atomic_state_helper.c +0 -0
index 3ba996069d69..709355c6bac6 100644
@@ -241,6 +241,7 @@ void
__drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
      state->fence = NULL;
      state->commit = NULL;
drivers/gpu/drm/drm_atomic_state_helper.c +1 -0
+     state->fb_damage_clips = NULL;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
@@ -285,6 +286,8 @@ void
__drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)

Quoted 14 lines

Quoted 9 lines
Quoted 7 lines
drivers/gpu/drm/drm_atomic_state_helper.c void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) · line 292
Quoted 7 lines
      if (state->commit)
              drm_crtc_commit_put(state->commit);
drivers/gpu/drm/drm_atomic_state_helper.c +2 -0
+
+     drm_property_blob_put(state->fb_damage_clips);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
Quoted 5 lines
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 -

https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch&data=02%7C01%7Cthellstrom%40vmware.com%7Ca62b8426087f410a689b08d6698d9110%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636812454080233380&sdata=6CZm4TU0A7wLpd1sXOUA3URmMQJqndy1WHxhtjnU8P8%3D&reserved=0

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter

2018-12-27 11:58 UTC #5 lore raw

Quoted 66 lines · Deepak Singh Rawat

Quoted 51 lines · Thomas Hellstrom
Quoted 13 lines · Daniel Vetter
Quoted 10 lines · Thomas Hellstrom
Reviewed-by Thomas Hellstrom <[email protected]>
Daniel, Dave, could you help try to get this patch in -next before
the
merge window. Otherwise people will start to experience random kernel
crashes. I figure we don't want to wait until first -fixes pull with
this.
Quoted 4 lines
Afaiui this will only blow up with new userspace on new kernels, that
doesn't feel like rushing an updated -next out the door material to me.
Hmm, this was discovered when the drm-read igt test hit an OOPS, I guess when damage_clips is unintentionally Non-Null.
Quoted 1 line
More concerning is why this fell through the cracks:
I agree. Sinc I'm on vacation a very quick answer. Will investigate more thogroughly when we get back.
Quoted 1 line
- We have an igt, do those testcases not hit this bug?
The drm-read igt intermittently hit the bug.
Quoted 1 line
- Were the tests not run before you've sent out the pull?
No, and I guess that't the main culprit. The igt doesn't work well with
vmwgfx, mostly because many tests are assuming gem functionality, there
fore we doesn't run them regularly, but mostly a pre-commit testsuite that focuses more on rendering correctness than modesetting functionality. That said, Deepak has over time put quite an effort into
making at least part of that test suite run well with vmwgfx, but I didn't consider it for this pull request. Will make sure we do in the future for core drm changes.
Quoted 3 lines
- The merged version doesn't seem to match any of the versions I've found on dri-devel, I guess should have been resend when there was conflicts?
Quoted 7 lines
Yes, I think there was at least one trivial merge conflict and a couple
of minor checkpatch.pl fixups in the pull request. Typically for vmwgfx
we don't resend the resulting patches if there are minor changes like that and they are reviewed internally. Need to get back to you about exactly where in the process we failed here.
I did ran igt after rebasing with upstream drm-next but only kms tests and didn't saw any problem. We have glretrace running continuously with rebooting VM's every time it runs and it too didn't failed so didn't suspected anything wrong with what I sent. Unfortunately we don't run igt continuously so I guess that is what we should work on.

Quoted 3 lines

I am yet to investigate why only drm_read failed. Although from stack trace I can see it is failing when set_config is called from vmwgfx fb_dev driver.

Yeah, drm_read failing isn't really the testcase I expected to blow up at all here. Sounds like an intriguing story at least on what exactly is going wrong here ...
-Daniel

Quoted 77 lines

Quoted 32 lines
Thanks,
Thomas
Quoted 25 lines
- Some other crack in the matrix?
Quoted 4 lines
Thanks,
Thomas
Quoted 13 lines
Quoted 12 lines · Deepak Rawat
Somehow the code to put the damage blob on destroy plane state and
set
the blob to NULL when duplicate plane state was not merged. May be
because the files are refactored since the patch was written. With
this
fix add those.
Cc Daniel Vetter <[email protected]>
Signed-off-by Deepak Rawat <[email protected]>
Needs a Fixes: tag referencing the broken commit, pls remember to add
these anytime you fix an issue with a commit. I'll add that and push
it to drm-misc-next-fixes.
Thanks Daniel for this. Will make sure in future.
Quoted 26 lines
Quoted 26 lines
Thanks, Daniel
Quoted 23 lines
Quoted 23 lines
---
 drivers/gpu/drm/drm_atomic_state_helper.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/gpu/drm/drm_atomic_state_helper.c +0 -0
index 3ba996069d69..709355c6bac6 100644
@@ -241,6 +241,7 @@ void
__drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
      state->fence = NULL;
      state->commit = NULL;
drivers/gpu/drm/drm_atomic_state_helper.c +1 -0
+     state->fb_damage_clips = NULL;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
@@ -285,6 +286,8 @@ void
__drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
Quoted 14 lines
Quoted 9 lines
Quoted 7 lines
drivers/gpu/drm/drm_atomic_state_helper.c void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) · line 292
Quoted 7 lines
      if (state->commit)
              drm_crtc_commit_put(state->commit);
drivers/gpu/drm/drm_atomic_state_helper.c +2 -0
+
+     drm_property_blob_put(state->fb_damage_clips);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
Quoted 5 lines
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 -
https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch&data=02%7C01%7Cthellstrom%40vmware.com%7Ca62b8426087f410a689b08d6698d9110%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636812454080233380&sdata=6CZm4TU0A7wLpd1sXOUA3URmMQJqndy1WHxhtjnU8P8%3D&reserved=0

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel