| Message ID | 20181221193559.4346-1-drawat@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Put damage blob when destroy plane state | expand |
Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com> 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 On Fri, 2018-12-21 at 11:35 -0800, Deepak Rawat wrote: > 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 <daniel.vetter@ffwll.ch> > Signed-off-by: Deepak Rawat <drawat@vmware.com> > --- > drivers/gpu/drm/drm_atomic_state_helper.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c > b/drivers/gpu/drm/drm_atomic_state_helper.c > index 3ba996069d69..709355c6bac6 100644 > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > @@ -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); >
On Fri, Dec 21, 2018 at 8:56 PM Thomas Hellstrom <thellstrom@vmware.com> wrote: > > Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com> > > 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? > Thanks, > Thomas > > > On Fri, 2018-12-21 at 11:35 -0800, Deepak Rawat wrote: > > 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 <daniel.vetter@ffwll.ch> > > Signed-off-by: Deepak Rawat <drawat@vmware.com> 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 > > --- > > drivers/gpu/drm/drm_atomic_state_helper.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c > > b/drivers/gpu/drm/drm_atomic_state_helper.c > > index 3ba996069d69..709355c6bac6 100644 > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > > @@ -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); > > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, 2018-12-24 at 11:49 +0100, Daniel Vetter wrote: > On Fri, Dec 21, 2018 at 8:56 PM Thomas Hellstrom < > thellstrom@vmware.com> wrote: > > Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com> > > > > 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. Hmm, this was discovered when the drm-read igt test hit an OOPS, I guess when damage_clips is unintentionally Non-Null. > 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. > - We have an igt, do those testcases not hit this bug? The drm-read igt intermittently hit the bug. > - 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. > - 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 > - Some other crack in the matrix? > > > Thanks, > > Thomas > > > > > > On Fri, 2018-12-21 at 11:35 -0800, Deepak Rawat wrote: > > > 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 <daniel.vetter@ffwll.ch> > > > Signed-off-by: Deepak Rawat <drawat@vmware.com> > > 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 > > > > --- > > > drivers/gpu/drm/drm_atomic_state_helper.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c > > > b/drivers/gpu/drm/drm_atomic_state_helper.c > > > index 3ba996069d69..709355c6bac6 100644 > > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > > > @@ -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); > > > > > > -- > 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
On Tue, 2018-12-25 at 07:01 -0800, Thomas Hellstrom wrote: > On Mon, 2018-12-24 at 11:49 +0100, Daniel Vetter wrote: > > On Fri, Dec 21, 2018 at 8:56 PM Thomas Hellstrom < > > thellstrom@vmware.com> wrote: > > > Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com> > > > > > > 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. > > Hmm, this was discovered when the drm-read igt test hit an OOPS, > I guess when damage_clips is unintentionally Non-Null. > > > > 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. > > > - We have an igt, do those testcases not hit this bug? > > The drm-read igt intermittently hit the bug. > > > > - 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. > > > - 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. 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. > > Thanks, > Thomas > > > > > - Some other crack in the matrix? > > > > > Thanks, > > > Thomas > > > > > > > > > On Fri, 2018-12-21 at 11:35 -0800, Deepak Rawat wrote: > > > > 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 <daniel.vetter@ffwll.ch> > > > > Signed-off-by: Deepak Rawat <drawat@vmware.com> > > > > 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. > > > > Thanks, Daniel > > > > > > --- > > > > drivers/gpu/drm/drm_atomic_state_helper.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c > > > > b/drivers/gpu/drm/drm_atomic_state_helper.c > > > > index 3ba996069d69..709355c6bac6 100644 > > > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > > > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > > > > @@ -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); > > > > > > > > > > -- > > 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
On Wed, Dec 26, 2018 at 09:50:44AM -0800, Deepak Singh Rawat wrote: > On Tue, 2018-12-25 at 07:01 -0800, Thomas Hellstrom wrote: > > On Mon, 2018-12-24 at 11:49 +0100, Daniel Vetter wrote: > > > On Fri, Dec 21, 2018 at 8:56 PM Thomas Hellstrom < > > > thellstrom@vmware.com> wrote: > > > > Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com> > > > > > > > > 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. > > > > Hmm, this was discovered when the drm-read igt test hit an OOPS, > > I guess when damage_clips is unintentionally Non-Null. > > > > > > > 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. > > > > > - We have an igt, do those testcases not hit this bug? > > > > The drm-read igt intermittently hit the bug. > > > > > > > - 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. > > > > > - 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. > > 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. 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 > > > > > Thanks, > > Thomas > > > > > > > > > - Some other crack in the matrix? > > > > > > > Thanks, > > > > Thomas > > > > > > > > > > > > On Fri, 2018-12-21 at 11:35 -0800, Deepak Rawat wrote: > > > > > 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 <daniel.vetter@ffwll.ch> > > > > > Signed-off-by: Deepak Rawat <drawat@vmware.com> > > > > > > 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. > > > > > > > Thanks, Daniel > > > > > > > > --- > > > > > drivers/gpu/drm/drm_atomic_state_helper.c | 3 +++ > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c > > > > > b/drivers/gpu/drm/drm_atomic_state_helper.c > > > > > index 3ba996069d69..709355c6bac6 100644 > > > > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > > > > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > > > > > @@ -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); > > > > > > > > > > > > > > -- > > > 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 >
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index 3ba996069d69..709355c6bac6 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -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);
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 <daniel.vetter@ffwll.ch> Signed-off-by: Deepak Rawat <drawat@vmware.com> --- drivers/gpu/drm/drm_atomic_state_helper.c | 3 +++ 1 file changed, 3 insertions(+)