Re: [PATCH v2] i915: intel_dp_aux_backlight: Fix max backlight calculations

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Wed, 2019-07-10 at 17:43 -0700, Furquan Shaikh wrote:
> Max backlight value for the panel was being calculated using byte
> count i.e. 0xffff if 2 bytes are supported for backlight brightness
> and 0xff if 1 byte is supported. However, EDP_PWMGEN_BIT_COUNT
> determines the number of active control bits used for the brightness
> setting. Thus, even if the panel uses 2 byte setting, it might not
> use
> all the control bits. Thus, max backlight should be set based on the
> value of EDP_PWMGEN_BIT_COUNT instead of assuming 65535 or 255.
> 
> Additionally, EDP_PWMGEN_BIT_COUNT was being updated based on the VBT
> frequency which results in a different max backlight value. Thus,
> setting of EDP_PWMGEN_BIT_COUNT is moved to setup phase instead of
> enable so that max backlight can be calculated correctly. Only the
> frequency divider is set during the enable phase using the value of
> EDP_PWMGEN_BIT_COUNT.
> 
> Signed-off-by: Furquan Shaikh <furquan@google.com>
> Reviewed-by: Stéphane Marchesin <marcheu@chromium.org>

Hi,

Thank you for your patch. See comments inline.

> ---
> v2: In case of DPCD failure and pn being uninitialized, return
> max_backlight as 0.
> 
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 134 ++++++++++++--
> ----
>  1 file changed, 90 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 357136f17f85..b3678b8a5b4d 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -110,61 +110,34 @@ static bool intel_dp_aux_set_pwm_freq(struct
> intel_connector *connector)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector-
> >base.dev);
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&connector-
> >encoder->base);
> -	int freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;
> -	u8 pn, pn_min, pn_max;
> +	int freq, fxp, f, fxp_actual, fxp_min, fxp_max;
> +	u8 pn;
>  
> -	/* Find desired value of (F x P)
> -	 * Note that, if F x P is out of supported range, the maximum
> value or
> -	 * minimum value will applied automatically. So no need to
> check that.
> -	 */
>  	freq = dev_priv->vbt.backlight.pwm_freq_hz;
> -	DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz\n", freq);
>  	if (!freq) {
>  		DRM_DEBUG_KMS("Use panel default backlight
> frequency\n");
>  		return false;
>  	}
>  
> -	fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ),
> freq);
> -
> -	/* Use highest possible value of Pn for more granularity of
> brightness
> -	 * adjustment while satifying the conditions below.
> -	 * - Pn is in the range of Pn_min and Pn_max
> -	 * - F is in the range of 1 and 255
> -	 * - FxP is within 25% of desired value.
> -	 *   Note: 25% is arbitrary value and may need some tweak.
> -	 */
> -	if (drm_dp_dpcd_readb(&intel_dp->aux,
> -			       DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN,
> &pn_min) != 1) {
> -		DRM_DEBUG_KMS("Failed to read pwmgen bit count cap
> min\n");
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT,
> +			      &pn) < 0) {
> +		DRM_DEBUG_KMS("Failed to read aux pwmgen bit count\n");
>  		return false;
>  	}
> -	if (drm_dp_dpcd_readb(&intel_dp->aux,
> -			       DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX,
> &pn_max) != 1) {
> -		DRM_DEBUG_KMS("Failed to read pwmgen bit count cap
> max\n");
> -		return false;
> -	}
> -	pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> -	pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
>  
> +	fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ),
> freq);
> +	f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
> +	fxp_actual = f << pn;
> +
> +	/* Ensure frequency is within 25% of desired value */
>  	fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
>  	fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
> -	if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> -		DRM_DEBUG_KMS("VBT defined backlight frequency out of
> range\n");
> -		return false;
> -	}
>  
> -	for (pn = pn_max; pn >= pn_min; pn--) {
> -		f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
> -		fxp_actual = f << pn;
> -		if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
> -			break;
> -	}
> -
> -	if (drm_dp_dpcd_writeb(&intel_dp->aux,
> -			       DP_EDP_PWMGEN_BIT_COUNT, pn) < 0) {
> -		DRM_DEBUG_KMS("Failed to write aux pwmgen bit
> count\n");
> +	if (fxp_min > fxp_actual || fxp_actual > fxp_max) {
> +		DRM_DEBUG_KMS("Actual frequency out of range\n");
>  		return false;
>  	}
> +
>  	if (drm_dp_dpcd_writeb(&intel_dp->aux,
>  			       DP_EDP_BACKLIGHT_FREQ_SET, (u8) f) < 0)
> {
>  		DRM_DEBUG_KMS("Failed to write aux backlight freq\n");
> @@ -224,16 +197,89 @@ static void
> intel_dp_aux_disable_backlight(const struct drm_connector_state *old
>  	set_aux_backlight_enable(enc_to_intel_dp(old_conn_state-
> >best_encoder), false);
>  }
>  
> +static u32 intel_dp_aux_calc_max_backlight(struct intel_connector
> *connector)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(connector-
> >base.dev);
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&connector-
> >encoder->base);
> +	u32 max_backlight = 0;
> +	int freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;
> +	u8 pn, pn_min, pn_max;
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT,
> +			      &pn) != 1)
> +		return max_backlight;

Should we really fail here and cause -ENODEV to be returned? We could
simply try to go back to old fashion, i.e calculate pn using pn_min to 
pn_max algorithm as it is done currently, if lets say we couldn't read
it or pn was garbage.

> +
> +	/* Use highest possible value of Pn for more granularity of
> brightness
> +	 * adjustment while satifying the conditions below.
> +	 * - Pn is in the range of Pn_min and Pn_max
> +	 * - F is in the range of 1 and 255
> +	 * - FxP is within 25% of desired value.
> +	 *   Note: 25% is arbitrary value and may need some tweak.
> +	 */
> +	if (drm_dp_dpcd_readb(&intel_dp->aux,
> +			       DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN,
> &pn_min) != 1) {
> +		DRM_DEBUG_KMS("Failed to read pwmgen bit count cap
> min\n");
> +		return max_backlight;
> +	}
> +	if (drm_dp_dpcd_readb(&intel_dp->aux,
> +			       DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX,
> &pn_max) != 1) {
> +		DRM_DEBUG_KMS("Failed to read pwmgen bit count cap
> max\n");
> +		return max_backlight;
> +	}
> +	pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +	pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +
> +	fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
> +	fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
> +	if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> +		DRM_DEBUG_KMS("VBT defined backlight frequency out of
> range\n");
> +		return max_backlight;
> +	}

If 25% of fxp is anyway an arbitrary value, can we just try to narrow
this [fxp_min, fxp_max] to [1 << pn_min, 255 << pn_max] if it happens
to be outside of the range? If fxp is within [1 << pn_min, 255 <<
pn_max] I guess we still can proceed.

> +
> +	for (pn = pn_max; pn >= pn_min; pn--) {
> +		f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
> +		fxp_actual = f << pn;
> +		if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
> +			break;
> +	}
> +
> +	if (drm_dp_dpcd_writeb(&intel_dp->aux,
> +			       DP_EDP_PWMGEN_BIT_COUNT, pn) < 0) {
> +		DRM_DEBUG_KMS("Failed to write aux pwmgen bit
> count\n");
> +		return max_backlight;
> +	}
> +
> +	max_backlight = (1 << pn) - 1;
> +
> +	return max_backlight;
> +}
> +
>  static int intel_dp_aux_setup_backlight(struct intel_connector
> *connector,
>  					enum pipe pipe)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&connector-
> >encoder->base);
>  	struct intel_panel *panel = &connector->panel;
>  
> -	if (intel_dp->edp_dpcd[2] &
> DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
> -		panel->backlight.max = 0xFFFF;
> -	else
> -		panel->backlight.max = 0xFF;
> +	panel->backlight.max =
> intel_dp_aux_calc_max_backlight(connector);
> +
> +	if (!panel->backlight.max)
> +		return -ENODEV;

Can we just fallback to old way here, instead of failure?
I see previsouly intel_dp_aux_set_pwm_freq was called only
if we had DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP set and it's 
failure didn't affect program flow that much. My concern is
that we have quite a lot of different hardware, so bailing
out that way could make our CI life a bit worse.

See intel_dp_aux_enable_backlight:

	if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
		if (intel_dp_aux_set_pwm_freq(connector))
			new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;

>  
>  	panel->backlight.min = 0;
>  	panel->backlight.level = intel_dp_aux_get_backlight(connector);

Best Regards,

Lisovskiy Stanislav

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel