[PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against

[PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against

[Chris Wilson]

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 <chris@chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 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 = &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(&crtc_state->base.commit->hw_done);
-	} else
-		ret = wait_for_completion_interruptible(&conn_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(&commit->hw_done);
+		if (ret)
+			return ret;
+	}
 
 	ret = mutex_lock_interruptible(&dev_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

✓ Fi.CI.BAT: success for drm/i915: Be defensive and don't assume PSR has any commit to sync against

[Patchwork]

== Series Details ==

Series: drm/i915: Be defensive and don't assume PSR has any commit to sync against
URL   : https://patchwork.freedesktop.org/series/49141/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4767 -> Patchwork_10084 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/49141/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10084 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-kbl-soraka:      NOTRUN -> INCOMPLETE (fdo#107774, fdo#107556)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s3:
      fi-cfl-8109u:       DMESG-WARN (fdo#107345) -> PASS +1

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       DMESG-WARN (fdo#105128, fdo#107139) -> PASS

    igt@kms_flip@basic-flip-vs-modeset:
      fi-cfl-8109u:       INCOMPLETE (fdo#107468) -> PASS

    
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
  fdo#107345 https://bugs.freedesktop.org/show_bug.cgi?id=107345
  fdo#107468 https://bugs.freedesktop.org/show_bug.cgi?id=107468
  fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
  fdo#107774 https://bugs.freedesktop.org/show_bug.cgi?id=107774


== Participating hosts (50 -> 46) ==

  Additional (1): fi-kbl-soraka 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4767 -> Patchwork_10084

  CI_DRM_4767: e9b69bafd3c2c13a8b9fa8e7a410f5d5ef32e328 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4626: bfce01d8c93dbd86e6ab04ca1afb844e0cbc8078 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10084: 23e6b7e3848fb3bbf675255440b077f6187a81a5 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

23e6b7e3848f drm/i915: Be defensive and don't assume PSR has any commit to sync against

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10084/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against

[Ville Syrjälä]

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 <chris@chris-wilson.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  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 = &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(&crtc_state->base.commit->hw_done);
> -	} else
> -		ret = wait_for_completion_interruptible(&conn_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(&commit->hw_done);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	ret = mutex_lock_interruptible(&dev_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

Re: [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against

[Chris Wilson]

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 <chris@chris-wilson.co.uk>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  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 = &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(&crtc_state->base.commit->hw_done);
> > -     } else
> > -             ret = wait_for_completion_interruptible(&conn_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: [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against

[Ville Syrjälä]

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 <chris@chris-wilson.co.uk>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  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 = &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(&crtc_state->base.commit->hw_done);
> > > -     } else
> > > -             ret = wait_for_completion_interruptible(&conn_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.syrjala@linux.intel.com>

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against

[Ville Syrjälä]

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 <chris@chris-wilson.co.uk>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  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 = &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(&crtc_state->base.commit->hw_done);
> > > > -     } else
> > > > -             ret = wait_for_completion_interruptible(&conn_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: [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against

[Chris Wilson]

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 <chris@chris-wilson.co.uk>
> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > ---
> > > > >  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 = &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(&crtc_state->base.commit->hw_done);
> > > > > -     } else
> > > > > -             ret = wait_for_completion_interruptible(&conn_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: [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against

[Pandiyan, Dhinakaran]

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 <chris@chris-wilson.co.uk>
> > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > ---
> > > > > >  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 = &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(&crtc_state->base.commit-
> > > > > > >hw_done);
> > > > > > -     } else
> > > > > > -             ret =
> > > > > > wait_for_completion_interruptible(&conn_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: [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against

[Ville Syrjälä]

On Tue, Sep 04, 2018 at 08:54:03PM +0000, 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 <chris@chris-wilson.co.uk>
> > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > > ---
> > > > > > >  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 = &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(&crtc_state->base.commit-
> > > > > > > >hw_done);
> > > > > > > -     } else
> > > > > > > -             ret =
> > > > > > > wait_for_completion_interruptible(&conn_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: [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against

[Maarten Lankhorst]

Op 05-09-18 om 12:22 schreef Ville Syrjälä:
> On Tue, Sep 04, 2018 at 08:54:03PM +0000, 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 <chris@chris-wilson.co.uk>
>>>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>>>>>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>>>>>>>> ---
>>>>>>>>  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 = &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(&crtc_state->base.commit-
>>>>>>>>> hw_done);
>>>>>>>> -     } else
>>>>>>>> -             ret =
>>>>>>>> wait_for_completion_interruptible(&conn_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: [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against

[Pandiyan, Dhinakaran]

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 +0000, 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 <chris@chris-wilson.co.uk
> > > > > > > > > >
> > > > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.
> > > > > > > > > com>
> > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.co
> > > > > > > > > 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 = &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(&crtc_state-
> > > > > > > > > >base.commit-
> > > > > > > > > > hw_done);
> > > > > > > > > 
> > > > > > > > > -     } else
> > > > > > > > > -             ret =
> > > > > > > > > wait_for_completion_interruptible(&conn_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 state corresponding to reading CRCs in the test, got freed
before PSR was enabled.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Be defensive and don't assume PSR has any commit to sync against

[Patchwork]

== Series Details ==

Series: drm/i915: Be defensive and don't assume PSR has any commit to sync against
URL   : https://patchwork.freedesktop.org/series/49141/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4767_full -> Patchwork_10084_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_10084_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@shrink:
      shard-hsw:          PASS -> INCOMPLETE (fdo#106886, fdo#103540)
      shard-kbl:          PASS -> FAIL (fdo#106886)

    igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy:
      shard-hsw:          PASS -> FAIL (fdo#105767) +1

    igt@kms_cursor_legacy@cursor-vs-flip-toggle:
      shard-hsw:          PASS -> FAIL (fdo#103355) +1

    
    ==== Possible fixes ====

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105363, fdo#102887) -> PASS

    igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-draw-pwrite:
      shard-glk:          FAIL (fdo#103167) -> PASS

    igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
      shard-glk:          INCOMPLETE (fdo#103359, k.org#198133) -> PASS

    igt@kms_setmode@basic:
      shard-apl:          FAIL (fdo#99912) -> PASS

    
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103355 https://bugs.freedesktop.org/show_bug.cgi?id=103355
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4767 -> Patchwork_10084

  CI_DRM_4767: e9b69bafd3c2c13a8b9fa8e7a410f5d5ef32e328 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4626: bfce01d8c93dbd86e6ab04ca1afb844e0cbc8078 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10084: 23e6b7e3848fb3bbf675255440b077f6187a81a5 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10084/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx