Re: [Intel-gfx] [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against
On Wed, 2018-09-05 at 12:51 +0200, Maarten Lankhorst wrote:
> Op 05-09-18 om 12:22 schreef Ville Syrjälä:
> > On Tue, Sep 04, 2018 at 08:54:03PM +, Pandiyan, Dhinakaran
> > wrote:
> > > On Tue, 2018-09-04 at 19:12 +0100, Chris Wilson wrote:
> > > > Quoting Ville Syrjälä (2018-09-04 19:06:29)
> > > > > On Tue, Sep 04, 2018 at 08:59:32PM +0300, Ville Syrjälä
> > > > > wrote:
> > > > > > On Tue, Sep 04, 2018 at 06:44:14PM +0100, Chris Wilson
> > > > > > wrote:
> > > > > > > Quoting Ville Syrjälä (2018-09-04 18:39:53)
> > > > > > > > On Tue, Sep 04, 2018 at 05:29:02PM +0100, Chris Wilson
> > > > > > > > wrote:
> > > > > > > > > If the previous modeset commit has completed and is
> > > > > > > > > no
> > > > > > > > > longer part of
> > > > > > > > > the crtc state, skip waiting for it.
> > > > > > > > >
> > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?i
> > > > > > > > > d=1077
> > > > > > > > > 92
> > > > > > > > > Fixes: c44301fce614 ("drm/i915: Allow control of PSR
> > > > > > > > > at
> > > > > > > > > runtime through debugfs, v6")
> > > > > > > > > Signed-off-by: Chris Wilson > > > > > > > > >
> > > > > > > > > Cc: Maarten Lankhorst > > > > > > > > com>
> > > > > > > > > Cc: Rodrigo Vivi
> > > > > > > > > Cc: Dhinakaran Pandiyan > > > > > > > > m>
> > > > > > > > > ---
> > > > > > > > > drivers/gpu/drm/i915/intel_psr.c | 16 ++--
> > > > > > > > >
> > > > > > > > > 1 file changed, 10 insertions(+), 6 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > > > index 21984d4c08ed..bddc9c7c681e 100644
> > > > > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > > > @@ -834,6 +834,7 @@ int
> > > > > > > > > intel_psr_set_debugfs_mode(struct
> > > > > > > > > drm_i915_private *dev_priv,
> > > > > > > > > struct drm_device *dev = _priv->drm;
> > > > > > > > > struct drm_connector_state *conn_state;
> > > > > > > > > struct intel_crtc_state *crtc_state = NULL;
> > > > > > > > > + struct drm_crtc_commit *commit = NULL;
> > > > > > > > > struct drm_crtc *crtc;
> > > > > > > > > struct intel_dp *dp;
> > > > > > > > > int ret;
> > > > > > > > > @@ -860,12 +861,15 @@ int
> > > > > > > > > intel_psr_set_debugfs_mode(struct
> > > > > > > > > drm_i915_private *dev_priv,
> > > > > > > > > return ret;
> > > > > > > > >
> > > > > > > > > crtc_state = to_intel_crtc_state(crtc-
> > > > > > > > > > state);
> > > > > > > > >
> > > > > > > > > - ret =
> > > > > > > > > wait_for_completion_interruptible(_state-
> > > > > > > > > >base.commit-
> > > > > > > > > > hw_done);
> > > > > > > > >
> > > > > > > > > - } else
> > > > > > > > > - ret =
> > > > > > > > > wait_for_completion_interruptible(_state-
> > > > > > > > > >commit-
> > > > > > > > > > hw_done);
> > > > > > > > >
> > > > > > > > > -
> > > > > > > > > - if (ret)
> > > > > > > > > - return ret;
> > > > > > > > > + commit = crtc_state->base.commit;
> > > > > > > > > + } else {
> > > > > > > > > + commit = conn_state->commit;
> > > > > > > >
> > > > > > > > I can't even find where we clear state->commit after
> > > > > > > > its
> > > > > > > > done.
> > > > > > > > Do we just leave it pointing at freed memory or
> > > > > > > > something?
> > > > > > > > Also I
> > > > > > > > can't figure out why drm_atomic_helper_commit_hw_done()
> > > > > > > > copies
> > > > > > > > the commit also to the old state.
> > > > > > >
> > > > > > > Let me be the messenger then ;) commit is NULL at this
> > > > > > > point, I
> > > > > > > just
> > > > > > > presumed it was intentional.
> > > > > >
> > > > > > My expectation would be that it gets cleared somewhere, but
> > > > > > I
> > > > > > simply
> > > > > > can't find any such code.
> > > > >
> > > > > Actually it looks like there is no such code. The event based
> > > > > release_crtc_commit() thing gets its own reference so
> > > > > presumably
> > > > > the
> > > > > original reference stays with the state until the state
> > > > > itself gets
> > > > > destroyed.
> > > >
> > > > Happy with the it never had a commit theory, or is this a
> > > > deeper
> > > > problem
> > > > that needs root causing?
> > >
> > > Just so that I understand this correctly, even if there was a
> > > prior
> > > commit, state->commit would have been freed when completion was
> > > signaled. Is that right?
> >
> > Nah, looks like it's going to hang on to the commit as long as the
> > state exists. At least that's my reading of the atomic helper.
> > Or maybe I'm missing something clever? Daniel/Maarten?
> >
>
> The commit is refcounted, and the original state has a reference. As
> long as that's not freed, the commit will hang around. So that can't
> cause a hang..
>
Okay, the
Re: [Intel-gfx] [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against
Op 05-09-18 om 12:22 schreef Ville Syrjälä:
> On Tue, Sep 04, 2018 at 08:54:03PM +, Pandiyan, Dhinakaran wrote:
>> On Tue, 2018-09-04 at 19:12 +0100, Chris Wilson wrote:
>>> Quoting Ville Syrjälä (2018-09-04 19:06:29)
On Tue, Sep 04, 2018 at 08:59:32PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 04, 2018 at 06:44:14PM +0100, Chris Wilson wrote:
>> Quoting Ville Syrjälä (2018-09-04 18:39:53)
>>> On Tue, Sep 04, 2018 at 05:29:02PM +0100, Chris Wilson wrote:
If the previous modeset commit has completed and is no
longer part of
the crtc state, skip waiting for it.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=1077
92
Fixes: c44301fce614 ("drm/i915: Allow control of PSR at
runtime through debugfs, v6")
Signed-off-by: Chris Wilson
Cc: Maarten Lankhorst
Cc: Rodrigo Vivi
Cc: Dhinakaran Pandiyan
---
drivers/gpu/drm/i915/intel_psr.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_psr.c
b/drivers/gpu/drm/i915/intel_psr.c
index 21984d4c08ed..bddc9c7c681e 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -834,6 +834,7 @@ int intel_psr_set_debugfs_mode(struct
drm_i915_private *dev_priv,
struct drm_device *dev = _priv->drm;
struct drm_connector_state *conn_state;
struct intel_crtc_state *crtc_state = NULL;
+ struct drm_crtc_commit *commit = NULL;
struct drm_crtc *crtc;
struct intel_dp *dp;
int ret;
@@ -860,12 +861,15 @@ int intel_psr_set_debugfs_mode(struct
drm_i915_private *dev_priv,
return ret;
crtc_state = to_intel_crtc_state(crtc-
> state);
- ret =
wait_for_completion_interruptible(_state->base.commit-
> hw_done);
- } else
- ret =
wait_for_completion_interruptible(_state->commit-
> hw_done);
-
- if (ret)
- return ret;
+ commit = crtc_state->base.commit;
+ } else {
+ commit = conn_state->commit;
>>> I can't even find where we clear state->commit after its
>>> done.
>>> Do we just leave it pointing at freed memory or something?
>>> Also I
>>> can't figure out why drm_atomic_helper_commit_hw_done()
>>> copies
>>> the commit also to the old state.
>> Let me be the messenger then ;) commit is NULL at this point, I
>> just
>> presumed it was intentional.
> My expectation would be that it gets cleared somewhere, but I
> simply
> can't find any such code.
Actually it looks like there is no such code. The event based
release_crtc_commit() thing gets its own reference so presumably
the
original reference stays with the state until the state itself gets
destroyed.
>>> Happy with the it never had a commit theory, or is this a deeper
>>> problem
>>> that needs root causing?
>> Just so that I understand this correctly, even if there was a prior
>> commit, state->commit would have been freed when completion was
>> signaled. Is that right?
> Nah, looks like it's going to hang on to the commit as long as the
> state exists. At least that's my reading of the atomic helper.
> Or maybe I'm missing something clever? Daniel/Maarten?
>
The commit is refcounted, and the original state has a reference. As long as
that's not freed, the commit will hang around. So that can't cause a hang..
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against
On Tue, Sep 04, 2018 at 08:54:03PM +, Pandiyan, Dhinakaran wrote:
> On Tue, 2018-09-04 at 19:12 +0100, Chris Wilson wrote:
> > Quoting Ville Syrjälä (2018-09-04 19:06:29)
> > > On Tue, Sep 04, 2018 at 08:59:32PM +0300, Ville Syrjälä wrote:
> > > > On Tue, Sep 04, 2018 at 06:44:14PM +0100, Chris Wilson wrote:
> > > > > Quoting Ville Syrjälä (2018-09-04 18:39:53)
> > > > > > On Tue, Sep 04, 2018 at 05:29:02PM +0100, Chris Wilson wrote:
> > > > > > > If the previous modeset commit has completed and is no
> > > > > > > longer part of
> > > > > > > the crtc state, skip waiting for it.
> > > > > > >
> > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=1077
> > > > > > > 92
> > > > > > > Fixes: c44301fce614 ("drm/i915: Allow control of PSR at
> > > > > > > runtime through debugfs, v6")
> > > > > > > Signed-off-by: Chris Wilson
> > > > > > > Cc: Maarten Lankhorst
> > > > > > > Cc: Rodrigo Vivi
> > > > > > > Cc: Dhinakaran Pandiyan
> > > > > > > ---
> > > > > > > drivers/gpu/drm/i915/intel_psr.c | 16 ++--
> > > > > > > 1 file changed, 10 insertions(+), 6 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > index 21984d4c08ed..bddc9c7c681e 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > @@ -834,6 +834,7 @@ int intel_psr_set_debugfs_mode(struct
> > > > > > > drm_i915_private *dev_priv,
> > > > > > > struct drm_device *dev = _priv->drm;
> > > > > > > struct drm_connector_state *conn_state;
> > > > > > > struct intel_crtc_state *crtc_state = NULL;
> > > > > > > + struct drm_crtc_commit *commit = NULL;
> > > > > > > struct drm_crtc *crtc;
> > > > > > > struct intel_dp *dp;
> > > > > > > int ret;
> > > > > > > @@ -860,12 +861,15 @@ int intel_psr_set_debugfs_mode(struct
> > > > > > > drm_i915_private *dev_priv,
> > > > > > > return ret;
> > > > > > >
> > > > > > > crtc_state = to_intel_crtc_state(crtc-
> > > > > > > >state);
> > > > > > > - ret =
> > > > > > > wait_for_completion_interruptible(_state->base.commit-
> > > > > > > >hw_done);
> > > > > > > - } else
> > > > > > > - ret =
> > > > > > > wait_for_completion_interruptible(_state->commit-
> > > > > > > >hw_done);
> > > > > > > -
> > > > > > > - if (ret)
> > > > > > > - return ret;
> > > > > > > + commit = crtc_state->base.commit;
> > > > > > > + } else {
> > > > > > > + commit = conn_state->commit;
> > > > > >
> > > > > > I can't even find where we clear state->commit after its
> > > > > > done.
> > > > > > Do we just leave it pointing at freed memory or something?
> > > > > > Also I
> > > > > > can't figure out why drm_atomic_helper_commit_hw_done()
> > > > > > copies
> > > > > > the commit also to the old state.
> > > > >
> > > > > Let me be the messenger then ;) commit is NULL at this point, I
> > > > > just
> > > > > presumed it was intentional.
> > > >
> > > > My expectation would be that it gets cleared somewhere, but I
> > > > simply
> > > > can't find any such code.
> > >
> > > Actually it looks like there is no such code. The event based
> > > release_crtc_commit() thing gets its own reference so presumably
> > > the
> > > original reference stays with the state until the state itself gets
> > > destroyed.
> >
> > Happy with the it never had a commit theory, or is this a deeper
> > problem
> > that needs root causing?
>
> Just so that I understand this correctly, even if there was a prior
> commit, state->commit would have been freed when completion was
> signaled. Is that right?
Nah, looks like it's going to hang on to the commit as long as the
state exists. At least that's my reading of the atomic helper.
Or maybe I'm missing something clever? Daniel/Maarten?
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against
On Tue, 2018-09-04 at 19:12 +0100, Chris Wilson wrote:
> Quoting Ville Syrjälä (2018-09-04 19:06:29)
> > On Tue, Sep 04, 2018 at 08:59:32PM +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 04, 2018 at 06:44:14PM +0100, Chris Wilson wrote:
> > > > Quoting Ville Syrjälä (2018-09-04 18:39:53)
> > > > > On Tue, Sep 04, 2018 at 05:29:02PM +0100, Chris Wilson wrote:
> > > > > > If the previous modeset commit has completed and is no
> > > > > > longer part of
> > > > > > the crtc state, skip waiting for it.
> > > > > >
> > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=1077
> > > > > > 92
> > > > > > Fixes: c44301fce614 ("drm/i915: Allow control of PSR at
> > > > > > runtime through debugfs, v6")
> > > > > > Signed-off-by: Chris Wilson
> > > > > > Cc: Maarten Lankhorst
> > > > > > Cc: Rodrigo Vivi
> > > > > > Cc: Dhinakaran Pandiyan
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/intel_psr.c | 16 ++--
> > > > > > 1 file changed, 10 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > index 21984d4c08ed..bddc9c7c681e 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > @@ -834,6 +834,7 @@ int intel_psr_set_debugfs_mode(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > > struct drm_device *dev = _priv->drm;
> > > > > > struct drm_connector_state *conn_state;
> > > > > > struct intel_crtc_state *crtc_state = NULL;
> > > > > > + struct drm_crtc_commit *commit = NULL;
> > > > > > struct drm_crtc *crtc;
> > > > > > struct intel_dp *dp;
> > > > > > int ret;
> > > > > > @@ -860,12 +861,15 @@ int intel_psr_set_debugfs_mode(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > > return ret;
> > > > > >
> > > > > > crtc_state = to_intel_crtc_state(crtc-
> > > > > > >state);
> > > > > > - ret =
> > > > > > wait_for_completion_interruptible(_state->base.commit-
> > > > > > >hw_done);
> > > > > > - } else
> > > > > > - ret =
> > > > > > wait_for_completion_interruptible(_state->commit-
> > > > > > >hw_done);
> > > > > > -
> > > > > > - if (ret)
> > > > > > - return ret;
> > > > > > + commit = crtc_state->base.commit;
> > > > > > + } else {
> > > > > > + commit = conn_state->commit;
> > > > >
> > > > > I can't even find where we clear state->commit after its
> > > > > done.
> > > > > Do we just leave it pointing at freed memory or something?
> > > > > Also I
> > > > > can't figure out why drm_atomic_helper_commit_hw_done()
> > > > > copies
> > > > > the commit also to the old state.
> > > >
> > > > Let me be the messenger then ;) commit is NULL at this point, I
> > > > just
> > > > presumed it was intentional.
> > >
> > > My expectation would be that it gets cleared somewhere, but I
> > > simply
> > > can't find any such code.
> >
> > Actually it looks like there is no such code. The event based
> > release_crtc_commit() thing gets its own reference so presumably
> > the
> > original reference stays with the state until the state itself gets
> > destroyed.
>
> Happy with the it never had a commit theory, or is this a deeper
> problem
> that needs root causing?
Just so that I understand this correctly, even if there was a prior
commit, state->commit would have been freed when completion was
signaled. Is that right?
> -Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against
Quoting Ville Syrjälä (2018-09-04 19:06:29)
> On Tue, Sep 04, 2018 at 08:59:32PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 04, 2018 at 06:44:14PM +0100, Chris Wilson wrote:
> > > Quoting Ville Syrjälä (2018-09-04 18:39:53)
> > > > On Tue, Sep 04, 2018 at 05:29:02PM +0100, Chris Wilson wrote:
> > > > > If the previous modeset commit has completed and is no longer part of
> > > > > the crtc state, skip waiting for it.
> > > > >
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107792
> > > > > Fixes: c44301fce614 ("drm/i915: Allow control of PSR at runtime
> > > > > through debugfs, v6")
> > > > > Signed-off-by: Chris Wilson
> > > > > Cc: Maarten Lankhorst
> > > > > Cc: Rodrigo Vivi
> > > > > Cc: Dhinakaran Pandiyan
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_psr.c | 16 ++--
> > > > > 1 file changed, 10 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > > index 21984d4c08ed..bddc9c7c681e 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > @@ -834,6 +834,7 @@ int intel_psr_set_debugfs_mode(struct
> > > > > drm_i915_private *dev_priv,
> > > > > struct drm_device *dev = _priv->drm;
> > > > > struct drm_connector_state *conn_state;
> > > > > struct intel_crtc_state *crtc_state = NULL;
> > > > > + struct drm_crtc_commit *commit = NULL;
> > > > > struct drm_crtc *crtc;
> > > > > struct intel_dp *dp;
> > > > > int ret;
> > > > > @@ -860,12 +861,15 @@ int intel_psr_set_debugfs_mode(struct
> > > > > drm_i915_private *dev_priv,
> > > > > return ret;
> > > > >
> > > > > crtc_state = to_intel_crtc_state(crtc->state);
> > > > > - ret =
> > > > > wait_for_completion_interruptible(_state->base.commit->hw_done);
> > > > > - } else
> > > > > - ret =
> > > > > wait_for_completion_interruptible(_state->commit->hw_done);
> > > > > -
> > > > > - if (ret)
> > > > > - return ret;
> > > > > + commit = crtc_state->base.commit;
> > > > > + } else {
> > > > > + commit = conn_state->commit;
> > > >
> > > > I can't even find where we clear state->commit after its done.
> > > > Do we just leave it pointing at freed memory or something? Also I
> > > > can't figure out why drm_atomic_helper_commit_hw_done() copies
> > > > the commit also to the old state.
> > >
> > > Let me be the messenger then ;) commit is NULL at this point, I just
> > > presumed it was intentional.
> >
> > My expectation would be that it gets cleared somewhere, but I simply
> > can't find any such code.
>
> Actually it looks like there is no such code. The event based
> release_crtc_commit() thing gets its own reference so presumably the
> original reference stays with the state until the state itself gets
> destroyed.
Happy with the it never had a commit theory, or is this a deeper problem
that needs root causing?
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against
On Tue, Sep 04, 2018 at 08:59:32PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 04, 2018 at 06:44:14PM +0100, Chris Wilson wrote:
> > Quoting Ville Syrjälä (2018-09-04 18:39:53)
> > > On Tue, Sep 04, 2018 at 05:29:02PM +0100, Chris Wilson wrote:
> > > > If the previous modeset commit has completed and is no longer part of
> > > > the crtc state, skip waiting for it.
> > > >
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107792
> > > > Fixes: c44301fce614 ("drm/i915: Allow control of PSR at runtime through
> > > > debugfs, v6")
> > > > Signed-off-by: Chris Wilson
> > > > Cc: Maarten Lankhorst
> > > > Cc: Rodrigo Vivi
> > > > Cc: Dhinakaran Pandiyan
> > > > ---
> > > > drivers/gpu/drm/i915/intel_psr.c | 16 ++--
> > > > 1 file changed, 10 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 21984d4c08ed..bddc9c7c681e 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -834,6 +834,7 @@ int intel_psr_set_debugfs_mode(struct
> > > > drm_i915_private *dev_priv,
> > > > struct drm_device *dev = _priv->drm;
> > > > struct drm_connector_state *conn_state;
> > > > struct intel_crtc_state *crtc_state = NULL;
> > > > + struct drm_crtc_commit *commit = NULL;
> > > > struct drm_crtc *crtc;
> > > > struct intel_dp *dp;
> > > > int ret;
> > > > @@ -860,12 +861,15 @@ int intel_psr_set_debugfs_mode(struct
> > > > drm_i915_private *dev_priv,
> > > > return ret;
> > > >
> > > > crtc_state = to_intel_crtc_state(crtc->state);
> > > > - ret =
> > > > wait_for_completion_interruptible(_state->base.commit->hw_done);
> > > > - } else
> > > > - ret =
> > > > wait_for_completion_interruptible(_state->commit->hw_done);
> > > > -
> > > > - if (ret)
> > > > - return ret;
> > > > + commit = crtc_state->base.commit;
> > > > + } else {
> > > > + commit = conn_state->commit;
> > >
> > > I can't even find where we clear state->commit after its done.
> > > Do we just leave it pointing at freed memory or something? Also I
> > > can't figure out why drm_atomic_helper_commit_hw_done() copies
> > > the commit also to the old state.
> >
> > Let me be the messenger then ;) commit is NULL at this point, I just
> > presumed it was intentional.
>
> My expectation would be that it gets cleared somewhere, but I simply
> can't find any such code.
Actually it looks like there is no such code. The event based
release_crtc_commit() thing gets its own reference so presumably the
original reference stays with the state until the state itself gets
destroyed.
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against
On Tue, Sep 04, 2018 at 06:44:14PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjälä (2018-09-04 18:39:53)
> > On Tue, Sep 04, 2018 at 05:29:02PM +0100, Chris Wilson wrote:
> > > If the previous modeset commit has completed and is no longer part of
> > > the crtc state, skip waiting for it.
> > >
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107792
> > > Fixes: c44301fce614 ("drm/i915: Allow control of PSR at runtime through
> > > debugfs, v6")
> > > Signed-off-by: Chris Wilson
> > > Cc: Maarten Lankhorst
> > > Cc: Rodrigo Vivi
> > > Cc: Dhinakaran Pandiyan
> > > ---
> > > drivers/gpu/drm/i915/intel_psr.c | 16 ++--
> > > 1 file changed, 10 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 21984d4c08ed..bddc9c7c681e 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -834,6 +834,7 @@ int intel_psr_set_debugfs_mode(struct
> > > drm_i915_private *dev_priv,
> > > struct drm_device *dev = _priv->drm;
> > > struct drm_connector_state *conn_state;
> > > struct intel_crtc_state *crtc_state = NULL;
> > > + struct drm_crtc_commit *commit = NULL;
> > > struct drm_crtc *crtc;
> > > struct intel_dp *dp;
> > > int ret;
> > > @@ -860,12 +861,15 @@ int intel_psr_set_debugfs_mode(struct
> > > drm_i915_private *dev_priv,
> > > return ret;
> > >
> > > crtc_state = to_intel_crtc_state(crtc->state);
> > > - ret =
> > > wait_for_completion_interruptible(_state->base.commit->hw_done);
> > > - } else
> > > - ret =
> > > wait_for_completion_interruptible(_state->commit->hw_done);
> > > -
> > > - if (ret)
> > > - return ret;
> > > + commit = crtc_state->base.commit;
> > > + } else {
> > > + commit = conn_state->commit;
> >
> > I can't even find where we clear state->commit after its done.
> > Do we just leave it pointing at freed memory or something? Also I
> > can't figure out why drm_atomic_helper_commit_hw_done() copies
> > the commit also to the old state.
>
> Let me be the messenger then ;) commit is NULL at this point, I just
> presumed it was intentional.
My expectation would be that it gets cleared somewhere, but I simply
can't find any such code. So the only explanation I have now is that
there has never been any commits on that crtc/connector before and
that's why it's NULL.
Based on that theory this does seem sane, so
Reviewed-by: Ville Syrjälä
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against
Quoting Ville Syrjälä (2018-09-04 18:39:53)
> On Tue, Sep 04, 2018 at 05:29:02PM +0100, Chris Wilson wrote:
> > If the previous modeset commit has completed and is no longer part of
> > the crtc state, skip waiting for it.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107792
> > Fixes: c44301fce614 ("drm/i915: Allow control of PSR at runtime through
> > debugfs, v6")
> > Signed-off-by: Chris Wilson
> > Cc: Maarten Lankhorst
> > Cc: Rodrigo Vivi
> > Cc: Dhinakaran Pandiyan
> > ---
> > drivers/gpu/drm/i915/intel_psr.c | 16 ++--
> > 1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 21984d4c08ed..bddc9c7c681e 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -834,6 +834,7 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private
> > *dev_priv,
> > struct drm_device *dev = _priv->drm;
> > struct drm_connector_state *conn_state;
> > struct intel_crtc_state *crtc_state = NULL;
> > + struct drm_crtc_commit *commit = NULL;
> > struct drm_crtc *crtc;
> > struct intel_dp *dp;
> > int ret;
> > @@ -860,12 +861,15 @@ int intel_psr_set_debugfs_mode(struct
> > drm_i915_private *dev_priv,
> > return ret;
> >
> > crtc_state = to_intel_crtc_state(crtc->state);
> > - ret =
> > wait_for_completion_interruptible(_state->base.commit->hw_done);
> > - } else
> > - ret =
> > wait_for_completion_interruptible(_state->commit->hw_done);
> > -
> > - if (ret)
> > - return ret;
> > + commit = crtc_state->base.commit;
> > + } else {
> > + commit = conn_state->commit;
>
> I can't even find where we clear state->commit after its done.
> Do we just leave it pointing at freed memory or something? Also I
> can't figure out why drm_atomic_helper_commit_hw_done() copies
> the commit also to the old state.
Let me be the messenger then ;) commit is NULL at this point, I just
presumed it was intentional.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against
On Tue, Sep 04, 2018 at 05:29:02PM +0100, Chris Wilson wrote:
> If the previous modeset commit has completed and is no longer part of
> the crtc state, skip waiting for it.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107792
> Fixes: c44301fce614 ("drm/i915: Allow control of PSR at runtime through
> debugfs, v6")
> Signed-off-by: Chris Wilson
> Cc: Maarten Lankhorst
> Cc: Rodrigo Vivi
> Cc: Dhinakaran Pandiyan
> ---
> drivers/gpu/drm/i915/intel_psr.c | 16 ++--
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 21984d4c08ed..bddc9c7c681e 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -834,6 +834,7 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private
> *dev_priv,
> struct drm_device *dev = _priv->drm;
> struct drm_connector_state *conn_state;
> struct intel_crtc_state *crtc_state = NULL;
> + struct drm_crtc_commit *commit = NULL;
> struct drm_crtc *crtc;
> struct intel_dp *dp;
> int ret;
> @@ -860,12 +861,15 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private
> *dev_priv,
> return ret;
>
> crtc_state = to_intel_crtc_state(crtc->state);
> - ret =
> wait_for_completion_interruptible(_state->base.commit->hw_done);
> - } else
> - ret =
> wait_for_completion_interruptible(_state->commit->hw_done);
> -
> - if (ret)
> - return ret;
> + commit = crtc_state->base.commit;
> + } else {
> + commit = conn_state->commit;
I can't even find where we clear state->commit after its done.
Do we just leave it pointing at freed memory or something? Also I
can't figure out why drm_atomic_helper_commit_hw_done() copies
the commit also to the old state.
> + }
> + if (commit) {
> + ret = wait_for_completion_interruptible(>hw_done);
> + if (ret)
> + return ret;
> + }
>
> ret = mutex_lock_interruptible(_priv->psr.lock);
> if (ret)
> --
> 2.19.0.rc1
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against
If the previous modeset commit has completed and is no longer part of
the crtc state, skip waiting for it.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107792
Fixes: c44301fce614 ("drm/i915: Allow control of PSR at runtime through
debugfs, v6")
Signed-off-by: Chris Wilson
Cc: Maarten Lankhorst
Cc: Rodrigo Vivi
Cc: Dhinakaran Pandiyan
---
drivers/gpu/drm/i915/intel_psr.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 21984d4c08ed..bddc9c7c681e 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -834,6 +834,7 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private
*dev_priv,
struct drm_device *dev = _priv->drm;
struct drm_connector_state *conn_state;
struct intel_crtc_state *crtc_state = NULL;
+ struct drm_crtc_commit *commit = NULL;
struct drm_crtc *crtc;
struct intel_dp *dp;
int ret;
@@ -860,12 +861,15 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private
*dev_priv,
return ret;
crtc_state = to_intel_crtc_state(crtc->state);
- ret =
wait_for_completion_interruptible(_state->base.commit->hw_done);
- } else
- ret =
wait_for_completion_interruptible(_state->commit->hw_done);
-
- if (ret)
- return ret;
+ commit = crtc_state->base.commit;
+ } else {
+ commit = conn_state->commit;
+ }
+ if (commit) {
+ ret = wait_for_completion_interruptible(>hw_done);
+ if (ret)
+ return ret;
+ }
ret = mutex_lock_interruptible(_priv->psr.lock);
if (ret)
--
2.19.0.rc1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
