Re: [PATCH 6/6] drm/i915: Remove most INVALID_PIPE checks from VLV backlight code

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

On Fri, 07 Nov 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Now that the backlight device no longer gets registered too early we
> should be able to drop most of the INVALID_PIPE checks form the VLV/CHV
> backlight code.

The subject and this paragraph refer to VLV/CHV but this isn't really
specific to those platforms.

> If we still manage to get here with INVALID_PIPE we will now get a WARN
> from the lower level functions and can then actually investigate further.
>
> vlv_get_backlight() still needs the check since that gets called in
> response to userspace actual_brightness reads.

IIUC this bit won't be true if you add the backlight.enabled check as I
suggested earlier.

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 2bc3309..0e2cb12 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -634,10 +634,9 @@ static void intel_panel_set_backlight(struct intel_connector *connector,
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_panel *panel = &connector->panel;
> -	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	u32 hw_level;
>  
> -	if (!panel->backlight.present || pipe == INVALID_PIPE)
> +	if (!panel->backlight.present)
>  		return;
>  
>  	mutex_lock(&dev_priv->backlight_lock);
> @@ -662,10 +661,9 @@ void intel_panel_set_backlight_acpi(struct intel_connector *connector,
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_panel *panel = &connector->panel;
> -	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	u32 hw_level;
>  
> -	if (!panel->backlight.present || pipe == INVALID_PIPE)
> +	if (!panel->backlight.present)
>  		return;

I have a feeling we may get these requests from the BIOS whenever. In
theory we should use the opregion ARDY field or somesuch to communicate
whether we're ready or not (we always say we're ready like a scout) but
even so we can't trust the BIOS to listen to what we say. Long story
short we should probably leave this check in.

With those fixed this LGTM.

BR,
Jani.


>  
>  	mutex_lock(&dev_priv->backlight_lock);
> @@ -740,9 +738,8 @@ void intel_panel_disable_backlight(struct intel_connector *connector)
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_panel *panel = &connector->panel;
> -	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  
> -	if (!panel->backlight.present || pipe == INVALID_PIPE)
> +	if (!panel->backlight.present)
>  		return;
>  
>  	/*
> @@ -949,7 +946,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
>  	struct intel_panel *panel = &connector->panel;
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  
> -	if (!panel->backlight.present || pipe == INVALID_PIPE)
> +	if (!panel->backlight.present)
>  		return;
>  
>  	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
> -- 
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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