Re: [PATCH 4/5] drm/i915: Use highest frequency divider for PWM

From: Jani Nikula <jani.nikula@linux.intel.com>
To: intel-gfx@lists.freedesktop.org
Cc: Puthikorn Voravootivat <puthik@chromium.org>
Subject: Re: [PATCH 4/5] drm/i915: Use highest frequency divider for	PWM
Date: Thu, 09 Mar 2017 12:40:45 +0200	[thread overview]
Message-ID: <87innihgwi.fsf@intel.com> (raw)
In-Reply-To: <20170308213053.194062-5-puthik@chromium.org>

On Wed, 08 Mar 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> TCON tend to have better brightness scaling with lower
> PWM frequency. This patch set the divider to highest
> value to lower the PWM frequency.

Just setting 0xff to DP_EDP_BACKLIGHT_FREQ_SET is way too arbitrary and
panel dependent. Going to a too low frequency may lead to flicker, and
you have no idea how low you're going because you ignore
DP_EDP_PWMGEN_BIT_COUNT completely. I think it's possible to land in
audible frequencies too, depending on the TCON hardware. (*)

I think the way to go is to check the VBT for OEM specified backlight
frequency, tuned for the specific hardware, and do the math to set the
registers right. This is what we use (well, fall back to) for the PWM
pin frequency setting in intel_panel.c.

BR,
Jani.

(*) Admittedly there's a certain charm in the idea of getting a bug
report, "I can hear my backlight". ;)

>
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 643b604be2de..32b426006a6a 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -116,6 +116,7 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  	uint8_t dpcd_buf = 0;
>  	uint8_t new_dpcd_buf = 0;
>  	uint8_t edp_backlight_mode = 0;
> +	bool freq_cap = false;
>  
>  	set_aux_backlight_enable(intel_dp, true);
>  
> @@ -146,10 +147,20 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  		intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0, 100);
>  	}
>  
> +	/* Use highest frequency divider if supported */
> +	freq_cap = intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
> +	if (freq_cap)
> +		new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
> +
>  	if (new_dpcd_buf != dpcd_buf) {
>  		drm_dp_dpcd_writeb(&intel_dp->aux,
>  			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
>  	}
> +
> +	if (freq_cap) {
> +		drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_FREQ_SET,
> +			0xff);
> +	}
>  }
>  
>  static void intel_dp_aux_disable_backlight(struct intel_connector *connector)

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