[PATCH] drm: Put damage blob when destroy plane state

Thu Dec 27 11:57:58 UTC 2018

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 at vmware.com> wrote:
> > > > Reviewed-by: Thomas Hellstrom <thellstrom at 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 at ffwll.ch>
> > > > > Signed-off-by: Deepak Rawat <drawat at 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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch