| Message ID | 1493214013-15580-6-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 26 Apr 2017, Imre Deak <imre.deak@intel.com> wrote: > An error from intel_get_pipe_from_connector() would mean a bug somewhere > else, but we still should check for it to prevent some other more > obscure bug later. Do check for invalid pipe, but please just limp on instead of bailing out of the functions. See notes inline. BR, Jani. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_panel.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index cb50c52..dbe05ec 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector) > struct drm_i915_private *dev_priv = to_i915(connector->base.dev); > struct intel_panel *panel = &connector->panel; > enum pipe pipe = intel_get_pipe_from_connector(connector); > - enum transcoder cpu_transcoder = > - intel_pipe_to_cpu_transcoder(dev_priv, pipe); > + enum transcoder cpu_transcoder; > u32 cpu_ctl2, pch_ctl1, pch_ctl2; > > + if (WARN_ON_ONCE(pipe == INVALID_PIPE)) > + return; Please just assign e.g. TRANSCODER_EDP to cpu_transcoder in this case, go on, and hope for the best. Do leave the warn in place though. > + > + cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe); > + > cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2); > if (cpu_ctl2 & BLM_PWM_ENABLE) { > DRM_DEBUG_KMS("cpu backlight already enabled\n"); > @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector) > enum pipe pipe = intel_get_pipe_from_connector(connector); > u32 ctl, ctl2, freq; > > + if (WARN_ON_ONCE(pipe == INVALID_PIPE)) > + return; Same here, please just assign e.g. PIPE_A here with the warning, and limp on. > + > ctl2 = I915_READ(BLC_PWM_CTL2); > if (ctl2 & BLM_PWM_ENABLE) { > DRM_DEBUG_KMS("backlight already enabled\n"); > @@ -1093,6 +1100,9 @@ void intel_panel_enable_backlight(struct intel_connector *connector) > if (!panel->backlight.present) > return; > > + if (WARN_ON_ONCE(pipe == INVALID_PIPE)) > + return; Here, pipe is only used for the debug logging, just skip that instead. > + > DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe)); > > mutex_lock(&dev_priv->backlight_lock);
On Wed, Apr 26, 2017 at 05:12:32PM +0300, Jani Nikula wrote: > On Wed, 26 Apr 2017, Imre Deak <imre.deak@intel.com> wrote: > > An error from intel_get_pipe_from_connector() would mean a bug somewhere > > else, but we still should check for it to prevent some other more > > obscure bug later. > > Do check for invalid pipe, but please just limp on instead of bailing > out of the functions. See notes inline. > > BR, > Jani. > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_panel.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > > index cb50c52..dbe05ec 100644 > > --- a/drivers/gpu/drm/i915/intel_panel.c > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector) > > struct drm_i915_private *dev_priv = to_i915(connector->base.dev); > > struct intel_panel *panel = &connector->panel; > > enum pipe pipe = intel_get_pipe_from_connector(connector); > > - enum transcoder cpu_transcoder = > > - intel_pipe_to_cpu_transcoder(dev_priv, pipe); > > + enum transcoder cpu_transcoder; > > u32 cpu_ctl2, pch_ctl1, pch_ctl2; > > > > + if (WARN_ON_ONCE(pipe == INVALID_PIPE)) > > + return; > > Please just assign e.g. TRANSCODER_EDP to cpu_transcoder in this case, > go on, and hope for the best. Do leave the warn in place though. > > > + > > + cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe); > > + > > cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2); > > if (cpu_ctl2 & BLM_PWM_ENABLE) { > > DRM_DEBUG_KMS("cpu backlight already enabled\n"); > > @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector) > > enum pipe pipe = intel_get_pipe_from_connector(connector); > > u32 ctl, ctl2, freq; > > > > + if (WARN_ON_ONCE(pipe == INVALID_PIPE)) > > + return; > > Same here, please just assign e.g. PIPE_A here with the warning, and > limp on. > > > + > > ctl2 = I915_READ(BLC_PWM_CTL2); > > if (ctl2 & BLM_PWM_ENABLE) { > > DRM_DEBUG_KMS("backlight already enabled\n"); > > @@ -1093,6 +1100,9 @@ void intel_panel_enable_backlight(struct intel_connector *connector) > > if (!panel->backlight.present) > > return; > > > > + if (WARN_ON_ONCE(pipe == INVALID_PIPE)) > > + return; > > Here, pipe is only used for the debug logging, just skip that instead. Ok, can do. This would make things inconsistent wrt. vlv_enable/disable_backlight() though, so are you ok if I change those too accordingly? --Imre > > > + > > DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe)); > > > > mutex_lock(&dev_priv->backlight_lock); > > -- > Jani Nikula, Intel Open Source Technology Center
On Wed, 26 Apr 2017, Imre Deak <imre.deak@intel.com> wrote: > On Wed, Apr 26, 2017 at 05:12:32PM +0300, Jani Nikula wrote: >> On Wed, 26 Apr 2017, Imre Deak <imre.deak@intel.com> wrote: >> > An error from intel_get_pipe_from_connector() would mean a bug somewhere >> > else, but we still should check for it to prevent some other more >> > obscure bug later. >> >> Do check for invalid pipe, but please just limp on instead of bailing >> out of the functions. See notes inline. >> >> BR, >> Jani. >> >> > >> > Signed-off-by: Imre Deak <imre.deak@intel.com> >> > --- >> > drivers/gpu/drm/i915/intel_panel.c | 14 ++++++++++++-- >> > 1 file changed, 12 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c >> > index cb50c52..dbe05ec 100644 >> > --- a/drivers/gpu/drm/i915/intel_panel.c >> > +++ b/drivers/gpu/drm/i915/intel_panel.c >> > @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector) >> > struct drm_i915_private *dev_priv = to_i915(connector->base.dev); >> > struct intel_panel *panel = &connector->panel; >> > enum pipe pipe = intel_get_pipe_from_connector(connector); >> > - enum transcoder cpu_transcoder = >> > - intel_pipe_to_cpu_transcoder(dev_priv, pipe); >> > + enum transcoder cpu_transcoder; >> > u32 cpu_ctl2, pch_ctl1, pch_ctl2; >> > >> > + if (WARN_ON_ONCE(pipe == INVALID_PIPE)) >> > + return; >> >> Please just assign e.g. TRANSCODER_EDP to cpu_transcoder in this case, >> go on, and hope for the best. Do leave the warn in place though. >> >> > + >> > + cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe); >> > + >> > cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2); >> > if (cpu_ctl2 & BLM_PWM_ENABLE) { >> > DRM_DEBUG_KMS("cpu backlight already enabled\n"); >> > @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector) >> > enum pipe pipe = intel_get_pipe_from_connector(connector); >> > u32 ctl, ctl2, freq; >> > >> > + if (WARN_ON_ONCE(pipe == INVALID_PIPE)) >> > + return; >> >> Same here, please just assign e.g. PIPE_A here with the warning, and >> limp on. >> >> > + >> > ctl2 = I915_READ(BLC_PWM_CTL2); >> > if (ctl2 & BLM_PWM_ENABLE) { >> > DRM_DEBUG_KMS("backlight already enabled\n"); >> > @@ -1093,6 +1100,9 @@ void intel_panel_enable_backlight(struct intel_connector *connector) >> > if (!panel->backlight.present) >> > return; >> > >> > + if (WARN_ON_ONCE(pipe == INVALID_PIPE)) >> > + return; >> >> Here, pipe is only used for the debug logging, just skip that instead. > > Ok, can do. This would make things inconsistent wrt. > vlv_enable/disable_backlight() though, so are you ok if I change those > too accordingly? Those are different in the sense that the *registers* are chosen based on the pipe, so it *must* be one of A or B. But in the paths changed here, pipe is only used for some bit fields of the registers being updated. IIRC they are not even crucial for fundamental operation of the backlight. So I don't want to bail out unless we really can't proceed. BR, Jani. > > --Imre > >> >> > + >> > DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe)); >> > >> > mutex_lock(&dev_priv->backlight_lock); >> >> -- >> Jani Nikula, Intel Open Source Technology Center
On Wed, Apr 26, 2017 at 05:53:52PM +0300, Jani Nikula wrote: > On Wed, 26 Apr 2017, Imre Deak <imre.deak@intel.com> wrote: > > On Wed, Apr 26, 2017 at 05:12:32PM +0300, Jani Nikula wrote: > >> On Wed, 26 Apr 2017, Imre Deak <imre.deak@intel.com> wrote: > >> > An error from intel_get_pipe_from_connector() would mean a bug somewhere > >> > else, but we still should check for it to prevent some other more > >> > obscure bug later. > >> > >> Do check for invalid pipe, but please just limp on instead of bailing > >> out of the functions. See notes inline. > >> > >> BR, > >> Jani. > >> > >> > > >> > Signed-off-by: Imre Deak <imre.deak@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/intel_panel.c | 14 ++++++++++++-- > >> > 1 file changed, 12 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > >> > index cb50c52..dbe05ec 100644 > >> > --- a/drivers/gpu/drm/i915/intel_panel.c > >> > +++ b/drivers/gpu/drm/i915/intel_panel.c > >> > @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector) > >> > struct drm_i915_private *dev_priv = to_i915(connector->base.dev); > >> > struct intel_panel *panel = &connector->panel; > >> > enum pipe pipe = intel_get_pipe_from_connector(connector); > >> > - enum transcoder cpu_transcoder = > >> > - intel_pipe_to_cpu_transcoder(dev_priv, pipe); > >> > + enum transcoder cpu_transcoder; > >> > u32 cpu_ctl2, pch_ctl1, pch_ctl2; > >> > > >> > + if (WARN_ON_ONCE(pipe == INVALID_PIPE)) > >> > + return; > >> > >> Please just assign e.g. TRANSCODER_EDP to cpu_transcoder in this case, > >> go on, and hope for the best. Do leave the warn in place though. > >> > >> > + > >> > + cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe); > >> > + > >> > cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2); > >> > if (cpu_ctl2 & BLM_PWM_ENABLE) { > >> > DRM_DEBUG_KMS("cpu backlight already enabled\n"); > >> > @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector) > >> > enum pipe pipe = intel_get_pipe_from_connector(connector); > >> > u32 ctl, ctl2, freq; > >> > > >> > + if (WARN_ON_ONCE(pipe == INVALID_PIPE)) > >> > + return; > >> > >> Same here, please just assign e.g. PIPE_A here with the warning, and > >> limp on. > >> > >> > + > >> > ctl2 = I915_READ(BLC_PWM_CTL2); > >> > if (ctl2 & BLM_PWM_ENABLE) { > >> > DRM_DEBUG_KMS("backlight already enabled\n"); > >> > @@ -1093,6 +1100,9 @@ void intel_panel_enable_backlight(struct intel_connector *connector) > >> > if (!panel->backlight.present) > >> > return; > >> > > >> > + if (WARN_ON_ONCE(pipe == INVALID_PIPE)) > >> > + return; > >> > >> Here, pipe is only used for the debug logging, just skip that instead. > > > > Ok, can do. This would make things inconsistent wrt. > > vlv_enable/disable_backlight() though, so are you ok if I change those > > too accordingly? > > Those are different in the sense that the *registers* are chosen based > on the pipe, so it *must* be one of A or B. But in the paths changed > here, pipe is only used for some bit fields of the registers being > updated. IIRC they are not even crucial for fundamental operation of the > backlight. So I don't want to bail out unless we really can't proceed. Ok, will keep those as-is. --Imre
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index cb50c52..dbe05ec 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector) struct drm_i915_private *dev_priv = to_i915(connector->base.dev); struct intel_panel *panel = &connector->panel; enum pipe pipe = intel_get_pipe_from_connector(connector); - enum transcoder cpu_transcoder = - intel_pipe_to_cpu_transcoder(dev_priv, pipe); + enum transcoder cpu_transcoder; u32 cpu_ctl2, pch_ctl1, pch_ctl2; + if (WARN_ON_ONCE(pipe == INVALID_PIPE)) + return; + + cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe); + cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2); if (cpu_ctl2 & BLM_PWM_ENABLE) { DRM_DEBUG_KMS("cpu backlight already enabled\n"); @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector) enum pipe pipe = intel_get_pipe_from_connector(connector); u32 ctl, ctl2, freq; + if (WARN_ON_ONCE(pipe == INVALID_PIPE)) + return; + ctl2 = I915_READ(BLC_PWM_CTL2); if (ctl2 & BLM_PWM_ENABLE) { DRM_DEBUG_KMS("backlight already enabled\n"); @@ -1093,6 +1100,9 @@ void intel_panel_enable_backlight(struct intel_connector *connector) if (!panel->backlight.present) return; + if (WARN_ON_ONCE(pipe == INVALID_PIPE)) + return; + DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe)); mutex_lock(&dev_priv->backlight_lock);
An error from intel_get_pipe_from_connector() would mean a bug somewhere else, but we still should check for it to prevent some other more obscure bug later. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_panel.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)