On Wed, Apr 26, 2017 at 05:53:52PM +0300, Jani Nikula wrote:
> On Wed, 26 Apr 2017, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> > On Wed, Apr 26, 2017 at 05:12:32PM +0300, Jani Nikula wrote:
> >> On Wed, 26 Apr 2017, Imre Deak <imre.deak@xxxxxxxxx> 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@xxxxxxxxx>
> >> > ---
> >> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
