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

[Jani Nikula <jani.nikula@linux.intel.com>]

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx