| Message ID | 20190403233539.31828-3-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] drm/i915/psr: Update PSR2 SU corruption workaround comment | expand |
On Wed, Apr 03, 2019 at 04:35:35PM -0700, José Roberto de Souza wrote: > Even when driver is reloaded and hits this scenario the PSR mutex > should be initialized, otherwise reading PSR debugfs status will > execute mutex_lock() over a mutex that was not initialized. > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index c80bb3003a7d..a84da931c3be 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -1227,7 +1227,6 @@ void intel_psr_init(struct drm_i915_private *dev_priv) > if (val) { > DRM_DEBUG_KMS("PSR interruption error set\n"); > dev_priv->psr.sink_not_reliable = true; > - return; There are other returns above and if debugfs hits this case maybe it is worth to move the mutex initialization up instead? > } > > /* Set link_standby x link_off defaults */ > -- > 2.21.0 >
On Wed, 2019-04-03 at 17:27 -0700, Rodrigo Vivi wrote: > On Wed, Apr 03, 2019 at 04:35:35PM -0700, José Roberto de Souza > wrote: > > Even when driver is reloaded and hits this scenario the PSR mutex > > should be initialized, otherwise reading PSR debugfs status will > > execute mutex_lock() over a mutex that was not initialized. > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index c80bb3003a7d..a84da931c3be 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -1227,7 +1227,6 @@ void intel_psr_init(struct drm_i915_private > > *dev_priv) > > if (val) { > > DRM_DEBUG_KMS("PSR interruption error set\n"); > > dev_priv->psr.sink_not_reliable = true; > > - return; > > There are other returns above and if debugfs hits this case maybe it > is worth to move the mutex initialization up instead? We have those two returns in PSR debugfs, !HAS_PSR(dev_priv) and !psr- >sink_support and in this cases we don't have any PSR functionality so not worthy to initialize anything PSR related. > > > } > > > > /* Set link_standby x link_off defaults */ > > -- > > 2.21.0 > >
On Thu, Apr 04, 2019 at 12:25:56PM -0700, Souza, Jose wrote: > On Wed, 2019-04-03 at 17:27 -0700, Rodrigo Vivi wrote: > > On Wed, Apr 03, 2019 at 04:35:35PM -0700, José Roberto de Souza > > wrote: > > > Even when driver is reloaded and hits this scenario the PSR mutex > > > should be initialized, otherwise reading PSR debugfs status will > > > execute mutex_lock() over a mutex that was not initialized. > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_psr.c | 1 - > > > 1 file changed, 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index c80bb3003a7d..a84da931c3be 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -1227,7 +1227,6 @@ void intel_psr_init(struct drm_i915_private > > > *dev_priv) > > > if (val) { > > > DRM_DEBUG_KMS("PSR interruption error set\n"); > > > dev_priv->psr.sink_not_reliable = true; > > > - return; > > > > There are other returns above and if debugfs hits this case maybe it > > is worth to move the mutex initialization up instead? > > > We have those two returns in PSR debugfs, !HAS_PSR(dev_priv) and !psr- > >sink_support and in this cases we don't have any PSR functionality so > not worthy to initialize anything PSR related. oh, indeed. Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > } > > > > > > /* Set link_standby x link_off defaults */ > > > -- > > > 2.21.0 > > >
On Wed, 2019-04-03 at 16:35 -0700, José Roberto de Souza wrote: > Even when driver is reloaded and hits this scenario the PSR mutex > should be initialized, otherwise reading PSR debugfs status will > execute mutex_lock() over a mutex that was not initialized. > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > b/drivers/gpu/drm/i915/intel_psr.c > index c80bb3003a7d..a84da931c3be 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -1227,7 +1227,6 @@ void intel_psr_init(struct drm_i915_private *dev_priv) > if (val) { > DRM_DEBUG_KMS("PSR interruption error set\n"); > dev_priv->psr.sink_not_reliable = true; Should we just sink_support = false and keep the return? IOW is there any use for sink_not_reliable? > - return; > } > > /* Set link_standby x link_off defaults */
On Thu, 2019-04-04 at 17:22 -0700, Dhinakaran Pandiyan wrote: > On Wed, 2019-04-03 at 16:35 -0700, José Roberto de Souza wrote: > > Even when driver is reloaded and hits this scenario the PSR mutex > > should be initialized, otherwise reading PSR debugfs status will > > execute mutex_lock() over a mutex that was not initialized. > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index c80bb3003a7d..a84da931c3be 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -1227,7 +1227,6 @@ void intel_psr_init(struct drm_i915_private > > *dev_priv) > > if (val) { > > DRM_DEBUG_KMS("PSR interruption error set\n"); > > dev_priv->psr.sink_not_reliable = true; > Should we just sink_support = false and keep the return? IOW is there > any use > for sink_not_reliable? I guess it could cause confusion as user had PSR support before the module reload and after the load PSR debugfs will say that sink do not support PSR. > > > - return; > > } > > > > /* Set link_standby x link_off defaults */
On Thu, 2019-04-04 at 17:32 -0700, Souza, Jose wrote: > On Thu, 2019-04-04 at 17:22 -0700, Dhinakaran Pandiyan wrote: > > On Wed, 2019-04-03 at 16:35 -0700, José Roberto de Souza wrote: > > > Even when driver is reloaded and hits this scenario the PSR mutex > > > should be initialized, otherwise reading PSR debugfs status will > > > execute mutex_lock() over a mutex that was not initialized. > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_psr.c | 1 - > > > 1 file changed, 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index c80bb3003a7d..a84da931c3be 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -1227,7 +1227,6 @@ void intel_psr_init(struct drm_i915_private > > > *dev_priv) > > > if (val) { > > > DRM_DEBUG_KMS("PSR interruption error set\n"); > > > dev_priv->psr.sink_not_reliable = true; > > > > Should we just sink_support = false and keep the return? IOW is there > > any use > > for sink_not_reliable? > > I guess it could cause confusion as user had PSR support before the > module reload and after the load PSR debugfs will say that sink do not > support PSR. I don't think it is any more confusing than saying sink supports PSR and not enabling it. Might as well make it clear that we are blaming the sink for not enabling PSR. > > > > > > - return; > > > } > > > > > > /* Set link_standby x link_off defaults */
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index c80bb3003a7d..a84da931c3be 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -1227,7 +1227,6 @@ void intel_psr_init(struct drm_i915_private *dev_priv) if (val) { DRM_DEBUG_KMS("PSR interruption error set\n"); dev_priv->psr.sink_not_reliable = true; - return; } /* Set link_standby x link_off defaults */
Even when driver is reloaded and hits this scenario the PSR mutex should be initialized, otherwise reading PSR debugfs status will execute mutex_lock() over a mutex that was not initialized. Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 1 - 1 file changed, 1 deletion(-)