[Intel-gfx] [PATCH] drm/i915/dp: Tweak initial dpcd backlight.enabled value

[Lyude Paul]

So if I understand this correctly, it sounds like that some Pixelbooks boot up
with DP_EDP_BACKLIGHT_BRIGHTNESS_MSB set to a non-zero value, without the
panel actually having DPCD backlight controls enabled?

If I'm understanding that correctly, then this patch looks good to me:

Reviewed-by: Lyude Paul <lyude at redhat.com>

On Thu, 2020-09-17 at 20:28 -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul at chromium.org>
> 
> In commit 79946723092b ("drm/i915: Assume 100% brightness when not in
> DPCD control mode"), we fixed the brightness level when DPCD control was
> not active to max brightness. This is as good as we can guess since most
> backlights go on full when uncontrolled.
> 
> However in doing so we changed the semantics of the initial
> 'backlight.enabled' value. At least on Pixelbooks, they  were relying
> on the brightness level in DP_EDP_BACKLIGHT_BRIGHTNESS_MSB to be 0 on
> boot such that enabled would be false. This causes the device to be
> enabled when the brightness is set. Without this, brightness control
> doesn't work. So by changing brightness to max, we also flipped enabled
> to be true on boot.
> 
> To fix this, make enabled a function of brightness and backlight control
> mechanism.
> 
> Fixes: 79946723092b ("drm/i915: Assume 100% brightness when not in DPCD
> control mode")
> Cc: Lyude Paul <lyude at redhat.com>
> Cc: Jani Nikula <jani.nikula at intel.com>
> Cc: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
> Cc: "Ville Syrjälä" <ville.syrjala at linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Kevin Chowski <chowski at chromium.org>>
> Signed-off-by: Sean Paul <seanpaul at chromium.org>
> ---
>  .../drm/i915/display/intel_dp_aux_backlight.c | 31 ++++++++++++-------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> index acbd7eb66cbe..036f504ac7db 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -52,17 +52,11 @@ static void set_aux_backlight_enable(struct intel_dp
> *intel_dp, bool enable)
>  	}
>  }
>  
> -/*
> - * Read the current backlight value from DPCD register(s) based
> - * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> - */
> -static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
> +static bool intel_dp_aux_backlight_dpcd_mode(struct intel_connector
> *connector)
>  {
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> -	u8 read_val[2] = { 0x0 };
>  	u8 mode_reg;
> -	u16 level = 0;
>  
>  	if (drm_dp_dpcd_readb(&intel_dp->aux,
>  			      DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> @@ -70,15 +64,29 @@ static u32 intel_dp_aux_get_backlight(struct
> intel_connector *connector)
>  		drm_dbg_kms(&i915->drm,
>  			    "Failed to read the DPCD register 0x%x\n",
>  			    DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> -		return 0;
> +		return false;
>  	}
>  
> +	return (mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> +	       DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> +}
> +
> +/*
> + * Read the current backlight value from DPCD register(s) based
> + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
> + */
> +static u32 intel_dp_aux_get_backlight(struct intel_connector *connector)
> +{
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +	u8 read_val[2] = { 0x0 };
> +	u16 level = 0;
> +
>  	/*
>  	 * If we're not in DPCD control mode yet, the programmed brightness
>  	 * value is meaningless and we should assume max brightness
>  	 */
> -	if ((mode_reg & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
> -	    DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD)
> +	if (!intel_dp_aux_backlight_dpcd_mode(connector))
>  		return connector->panel.backlight.max;
>  
>  	if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> @@ -319,7 +327,8 @@ static int intel_dp_aux_setup_backlight(struct
> intel_connector *connector,
>  
>  	panel->backlight.min = 0;
>  	panel->backlight.level = intel_dp_aux_get_backlight(connector);
> -	panel->backlight.enabled = panel->backlight.level != 0;
> +	panel->backlight.enabled = intel_dp_aux_backlight_dpcd_mode(connector)
> &&
> +				   panel->backlight.level != 0;
>  
>  	return 0;
>  }
-- 
Cheers,
	Lyude Paul (she/her)
	Software Engineer at Red Hat