[Intel-gfx] [PATCH 5/8] drm/i915: Check error return when converting pipe to connector

[Imre Deak]

On Wed, Apr 26, 2017 at 05:53:52PM +0300, Jani Nikula wrote:
> On Wed, 26 Apr 2017, Imre Deak <imre.deak at 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 at 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 at 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