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

[Deepak Singh Rawat]

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.

> 
> 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