| Message ID | 20170511230225.142870-10-puthik@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2017-05-11 at 16:02 -0700, Puthikorn Voravootivat wrote: > Read desired PWM frequency from panel vbt and calculate the > value for divider in DPCD address 0x724 and 0x728 to have > as many bits as possible for PWM duty cyle for granularity of > brightness adjustment while the frequency is still within 25% > of the desired frequency. I read a few eDP panel data sheets, the PWM frequencies all start from ~200Hz. If the VBT chooses this lowest value to allow for more brightness control, and then this patch lowers the value by another 25%, we'll end up below the panel allowed PWM frequency. In fact, one of the systems I checked had PWM frequency as 200Hz in VBT and the panel datasheet also had PWM frequency range starting from 200Hz. Have you considered this case? -DK > > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org> > --- > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 81 +++++++++++++++++++++++++++ > 1 file changed, 81 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > index 0b48851013cc..6f10a2f1ab76 100644 > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > @@ -113,12 +113,86 @@ intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp, > } > } > > +/* > + * Set PWM Frequency divider to match desired frequency in vbt. > + * The PWM Frequency is calculated as 27Mhz / (F x P). > + * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of the > + * EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h) > + * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the > + * EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h) > + */ > +static void 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; > + > + /* 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; > + } > + > + fxp = 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 > + * - Effective frequency is within 25% of desired frequency. > + */ > + 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; > + } > + 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; > + } > + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > + > + fxp_min = fxp * 3 / 4; > + fxp_max = fxp * 5 / 4; You are allowing fxp between +/- 25% of the actual. This isn't same as the "Effective frequency is within 25% of desired frequency." right? The frequency can vary between -20% and +33%. > + if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) { > + DRM_DEBUG_KMS("VBT defined backlight frequency out of range\n"); > + return; > + } > + > + for (pn = pn_max; pn > pn_min; pn--) { > + f = clamp(fxp >> 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; > + } > + 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"); > + return; > + } > +} > + > static void intel_dp_aux_enable_backlight(struct intel_connector *connector) > { > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > uint8_t dpcd_buf = 0; > uint8_t new_dpcd_buf = 0; > uint8_t edp_backlight_mode = 0; > + bool freq_cap; > > if (drm_dp_dpcd_readb(&intel_dp->aux, > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) { > @@ -151,6 +225,10 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector) > DRM_DEBUG_KMS("Enable dynamic brightness.\n"); > } > > + 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) { > if (drm_dp_dpcd_writeb(&intel_dp->aux, > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf) < 0) { > @@ -158,6 +236,9 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector) > } > } > > + if (freq_cap) > + intel_dp_aux_set_pwm_freq(connector); > + > set_aux_backlight_enable(intel_dp, true); > intel_dp_aux_set_backlight(connector, connector->panel.backlight.level); > }
On Fri, May 12, 2017 at 5:12 PM, Pandiyan, Dhinakaran < dhinakaran.pandiyan@intel.com> wrote: > On Thu, 2017-05-11 at 16:02 -0700, Puthikorn Voravootivat wrote: > > Read desired PWM frequency from panel vbt and calculate the > > value for divider in DPCD address 0x724 and 0x728 to have > > as many bits as possible for PWM duty cyle for granularity of > > brightness adjustment while the frequency is still within 25% > > of the desired frequency. > > I read a few eDP panel data sheets, the PWM frequencies all start from > ~200Hz. If the VBT chooses this lowest value to allow for more > brightness control, and then this patch lowers the value by another 25%, > we'll end up below the panel allowed PWM frequency. > > In fact, one of the systems I checked had PWM frequency as 200Hz in VBT > and the panel datasheet also had PWM frequency range starting from > 200Hz. Have you considered this case? > > The spec said "A given LCD panel typically has a limited range of backlight frequency capability. To limit the programmable frequency range, limitations are placed on the allowable total divider ratio with the Sink device" So I think it should be auto cap to 200Hz in this case. > -DK > > > > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org> > > --- > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 81 > +++++++++++++++++++++++++++ > > 1 file changed, 81 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > index 0b48851013cc..6f10a2f1ab76 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > @@ -113,12 +113,86 @@ intel_dp_aux_set_dynamic_backlight_percent(struct > intel_dp *intel_dp, > > } > > } > > > > +/* > > + * Set PWM Frequency divider to match desired frequency in vbt. > > + * The PWM Frequency is calculated as 27Mhz / (F x P). > > + * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 > of the > > + * EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h) > > + * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of > the > > + * EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h) > > + */ > > +static void 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; > > + > > + /* 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; > > + } > > + > > + fxp = 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 > > + * - Effective frequency is within 25% of desired frequency. > > + */ > > + 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; > > + } > > + 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; > > + } > > + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > > + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > > + > > + fxp_min = fxp * 3 / 4; > > + fxp_max = fxp * 5 / 4; > > You are allowing fxp between +/- 25% of the actual. This isn't same as > the "Effective frequency is within 25% of desired frequency." right? The > frequency can vary between -20% and +33%. > > You are right. You want me to change commit message to reflect this or change the code to match the commit message? > > > + if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) { > > + DRM_DEBUG_KMS("VBT defined backlight frequency out of > range\n"); > > + return; > > + } > > + > > + for (pn = pn_max; pn > pn_min; pn--) { > > + f = clamp(fxp >> 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; > > + } > > + 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"); > > + return; > > + } > > +} > > + > > static void intel_dp_aux_enable_backlight(struct intel_connector > *connector) > > { > > struct intel_dp *intel_dp = enc_to_intel_dp(&connector-> > encoder->base); > > uint8_t dpcd_buf = 0; > > uint8_t new_dpcd_buf = 0; > > uint8_t edp_backlight_mode = 0; > > + bool freq_cap; > > > > if (drm_dp_dpcd_readb(&intel_dp->aux, > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != > 1) { > > @@ -151,6 +225,10 @@ static void intel_dp_aux_enable_backlight(struct > intel_connector *connector) > > DRM_DEBUG_KMS("Enable dynamic brightness.\n"); > > } > > > > + 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) { > > if (drm_dp_dpcd_writeb(&intel_dp->aux, > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf) > < 0) { > > @@ -158,6 +236,9 @@ static void intel_dp_aux_enable_backlight(struct > intel_connector *connector) > > } > > } > > > > + if (freq_cap) > > + intel_dp_aux_set_pwm_freq(connector); > > + > > set_aux_backlight_enable(intel_dp, true); > > intel_dp_aux_set_backlight(connector, connector->panel.backlight. > level); > > } > >
On Fri, 2017-05-12 at 17:31 -0700, Puthikorn Voravootivat wrote: > > > > On Fri, May 12, 2017 at 5:12 PM, Pandiyan, Dhinakaran > <dhinakaran.pandiyan@intel.com> wrote: > On Thu, 2017-05-11 at 16:02 -0700, Puthikorn Voravootivat > wrote: > > Read desired PWM frequency from panel vbt and calculate the > > value for divider in DPCD address 0x724 and 0x728 to have > > as many bits as possible for PWM duty cyle for granularity > of > > brightness adjustment while the frequency is still within > 25% > > of the desired frequency. > > I read a few eDP panel data sheets, the PWM frequencies all > start from > ~200Hz. If the VBT chooses this lowest value to allow for more > brightness control, and then this patch lowers the value by > another 25%, > we'll end up below the panel allowed PWM frequency. > > In fact, one of the systems I checked had PWM frequency as > 200Hz in VBT > and the panel datasheet also had PWM frequency range starting > from > 200Hz. Have you considered this case? > > The spec said "A given LCD panel typically has a limited range of > backlight frequency capability. > To limit the programmable frequency range, limitations are placed on > the allowable total divider ratio with the Sink device" > So I think it should be auto cap to 200Hz in this case. > > -DK > > > > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org> > > --- > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 81 > +++++++++++++++++++++++++++ > > 1 file changed, 81 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > index 0b48851013cc..6f10a2f1ab76 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > @@ -113,12 +113,86 @@ > intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp > *intel_dp, > > } > > } > > > > +/* > > + * Set PWM Frequency divider to match desired frequency in > vbt. > > + * The PWM Frequency is calculated as 27Mhz / (F x P). > > + * - Where F = PWM Frequency Pre-Divider value programmed > by field 7:0 of the > > + * EDP_BACKLIGHT_FREQ_SET register (DPCD > Address 00728h) > > + * - Where P = 2^Pn, where Pn is the value programmed by > field 4:0 of the > > + * EDP_PWMGEN_BIT_COUNT register (DPCD Address > 00724h) > > + */ > > +static void 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; > > + > > + /* 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; > > + } > > + > > + fxp = 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 > > + * - Effective frequency is within 25% of desired > frequency. > > + */ > > + 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; > > + } > > + 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; > > + } > > + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > > + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > > + > > + fxp_min = fxp * 3 / 4; > > + fxp_max = fxp * 5 / 4; > > > You are allowing fxp between +/- 25% of the actual. This isn't > same as > the "Effective frequency is within 25% of desired frequency." > right? The > frequency can vary between -20% and +33%. > > > You are right. > You want me to change commit message to reflect this or change the > code to > match the commit message? I am okay with fixing the comment and commit message. Is the 25% arbitrary or based on the distances between the possible values? Please make a note in the comment if it's the former. > > + if (fxp_min < (1 << pn_min) || (255 << pn_max) < > fxp_max) { > > + DRM_DEBUG_KMS("VBT defined backlight frequency > out of range\n"); > > + return; > > + } > > + > > + for (pn = pn_max; pn > pn_min; pn--) { Is there a reason this is not pn >= pn_min? > > + f = clamp(fxp >> 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 the number of brightness control bits are changing, the max. brightness value changes too. Please see intel_dp_aux_setup_backlight(). > > + return; > > + } > > + 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"); > > + return; > > + } > > +} > > + > > static void intel_dp_aux_enable_backlight(struct > intel_connector *connector) > > { > > struct intel_dp *intel_dp = > enc_to_intel_dp(&connector->encoder->base); > > uint8_t dpcd_buf = 0; > > uint8_t new_dpcd_buf = 0; > > uint8_t edp_backlight_mode = 0; > > + bool freq_cap; > > > > if (drm_dp_dpcd_readb(&intel_dp->aux, > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > &dpcd_buf) != 1) { > > @@ -151,6 +225,10 @@ static void > intel_dp_aux_enable_backlight(struct intel_connector > *connector) > > DRM_DEBUG_KMS("Enable dynamic brightness.\n"); > > } > > > > + 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) { > > if (drm_dp_dpcd_writeb(&intel_dp->aux, > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > new_dpcd_buf) < 0) { > > @@ -158,6 +236,9 @@ static void > intel_dp_aux_enable_backlight(struct intel_connector > *connector) > > } > > } > > > > + if (freq_cap) > > + intel_dp_aux_set_pwm_freq(connector); > > + > > set_aux_backlight_enable(intel_dp, true); > > intel_dp_aux_set_backlight(connector, > connector->panel.backlight.level); > > } > > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, May 15, 2017 at 4:07 PM, Pandiyan, Dhinakaran < dhinakaran.pandiyan@intel.com> wrote: > On Fri, 2017-05-12 at 17:31 -0700, Puthikorn Voravootivat wrote: > > > > > > > > On Fri, May 12, 2017 at 5:12 PM, Pandiyan, Dhinakaran > > <dhinakaran.pandiyan@intel.com> wrote: > > On Thu, 2017-05-11 at 16:02 -0700, Puthikorn Voravootivat > > wrote: > > > Read desired PWM frequency from panel vbt and calculate the > > > value for divider in DPCD address 0x724 and 0x728 to have > > > as many bits as possible for PWM duty cyle for granularity > > of > > > brightness adjustment while the frequency is still within > > 25% > > > of the desired frequency. > > > > I read a few eDP panel data sheets, the PWM frequencies all > > start from > > ~200Hz. If the VBT chooses this lowest value to allow for more > > brightness control, and then this patch lowers the value by > > another 25%, > > we'll end up below the panel allowed PWM frequency. > > > > In fact, one of the systems I checked had PWM frequency as > > 200Hz in VBT > > and the panel datasheet also had PWM frequency range starting > > from > > 200Hz. Have you considered this case? > > > > The spec said "A given LCD panel typically has a limited range of > > backlight frequency capability. > > To limit the programmable frequency range, limitations are placed on > > the allowable total divider ratio with the Sink device" > > So I think it should be auto cap to 200Hz in this case. > > > > -DK > > > > > > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org> > > > --- > > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 81 > > +++++++++++++++++++++++++++ > > > 1 file changed, 81 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > index 0b48851013cc..6f10a2f1ab76 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > @@ -113,12 +113,86 @@ > > intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp > > *intel_dp, > > > } > > > } > > > > > > +/* > > > + * Set PWM Frequency divider to match desired frequency in > > vbt. > > > + * The PWM Frequency is calculated as 27Mhz / (F x P). > > > + * - Where F = PWM Frequency Pre-Divider value programmed > > by field 7:0 of the > > > + * EDP_BACKLIGHT_FREQ_SET register (DPCD > > Address 00728h) > > > + * - Where P = 2^Pn, where Pn is the value programmed by > > field 4:0 of the > > > + * EDP_PWMGEN_BIT_COUNT register (DPCD Address > > 00724h) > > > + */ > > > +static void 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; > > > + > > > + /* 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; > > > + } > > > + > > > + fxp = 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 > > > + * - Effective frequency is within 25% of desired > > frequency. > > > + */ > > > + 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; > > > + } > > > + 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; > > > + } > > > + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > > > + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > > > + > > > + fxp_min = fxp * 3 / 4; > > > + fxp_max = fxp * 5 / 4; > > > > > > You are allowing fxp between +/- 25% of the actual. This isn't > > same as > > the "Effective frequency is within 25% of desired frequency." > > right? The > > frequency can vary between -20% and +33%. > > > > > > You are right. > > You want me to change commit message to reflect this or change the > > code to > > match the commit message? > > I am okay with fixing the comment and commit message. Is the 25% > arbitrary or based on the distances between the possible values? Please > make a note in the comment if it's the former. > > > > > + if (fxp_min < (1 << pn_min) || (255 << pn_max) < > > fxp_max) { > > > > > > + DRM_DEBUG_KMS("VBT defined backlight frequency > > out of range\n"); > > > + return; > > > + } > > > + > > > + for (pn = pn_max; pn > pn_min; pn--) { > > Is there a reason this is not pn >= pn_min? > This is bug because f value will be incorrect in the case that pn == pn_min. Thanks for catching this. > > > > + f = clamp(fxp >> 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 the number of brightness control bits are changing, the max. > brightness value changes too. > > Please see intel_dp_aux_setup_backlight(). > > I think this is fine because - intel_dp_aux_setup_backlight() will called intel_dp_aux_enable_backlight() which called intel_dp_aux_set_pwm_freq() first before determine the max brightness value. - Also, the panel I tested does not change BACKLIGHT_BRIGHTNESS_BYTE_COUNT when changing the frequency. > > > > > + return; > > > + } > > > + 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"); > > > + return; > > > + } > > > +} > > > + > > > static void intel_dp_aux_enable_backlight(struct > > intel_connector *connector) > > > { > > > struct intel_dp *intel_dp = > > enc_to_intel_dp(&connector->encoder->base); > > > uint8_t dpcd_buf = 0; > > > uint8_t new_dpcd_buf = 0; > > > uint8_t edp_backlight_mode = 0; > > > + bool freq_cap; > > > > > > if (drm_dp_dpcd_readb(&intel_dp->aux, > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > &dpcd_buf) != 1) { > > > @@ -151,6 +225,10 @@ static void > > intel_dp_aux_enable_backlight(struct intel_connector > > *connector) > > > DRM_DEBUG_KMS("Enable dynamic brightness.\n"); > > > } > > > > > > + 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) { > > > if (drm_dp_dpcd_writeb(&intel_dp->aux, > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > new_dpcd_buf) < 0) { > > > @@ -158,6 +236,9 @@ static void > > intel_dp_aux_enable_backlight(struct intel_connector > > *connector) > > > } > > > } > > > > > > + if (freq_cap) > > > + intel_dp_aux_set_pwm_freq(connector); > > > + > > > set_aux_backlight_enable(intel_dp, true); > > > intel_dp_aux_set_backlight(connector, > > connector->panel.backlight.level); > > > } > > > > > > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On Mon, 2017-05-15 at 17:43 -0700, Puthikorn Voravootivat wrote: > > > On Mon, May 15, 2017 at 4:07 PM, Pandiyan, Dhinakaran > <dhinakaran.pandiyan@intel.com> wrote: > On Fri, 2017-05-12 at 17:31 -0700, Puthikorn Voravootivat > wrote: > > > > > > > > On Fri, May 12, 2017 at 5:12 PM, Pandiyan, Dhinakaran > > <dhinakaran.pandiyan@intel.com> wrote: > > On Thu, 2017-05-11 at 16:02 -0700, Puthikorn > Voravootivat > > wrote: > > > Read desired PWM frequency from panel vbt and > calculate the > > > value for divider in DPCD address 0x724 and 0x728 > to have > > > as many bits as possible for PWM duty cyle for > granularity > > of > > > brightness adjustment while the frequency is still > within > > 25% > > > of the desired frequency. > > > > I read a few eDP panel data sheets, the PWM > frequencies all > > start from > > ~200Hz. If the VBT chooses this lowest value to > allow for more > > brightness control, and then this patch lowers the > value by > > another 25%, > > we'll end up below the panel allowed PWM frequency. > > > > In fact, one of the systems I checked had PWM > frequency as > > 200Hz in VBT > > and the panel datasheet also had PWM frequency range > starting > > from > > 200Hz. Have you considered this case? > > > > The spec said "A given LCD panel typically has a limited > range of > > backlight frequency capability. > > To limit the programmable frequency range, limitations are > placed on > > the allowable total divider ratio with the Sink device" > > So I think it should be auto cap to 200Hz in this case. > > > > -DK > > > > > > Signed-off-by: Puthikorn Voravootivat > <puthik@chromium.org> > > > --- > > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | > 81 > > +++++++++++++++++++++++++++ > > > 1 file changed, 81 insertions(+) > > > > > > diff --git > a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > index 0b48851013cc..6f10a2f1ab76 100644 > > > --- > a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > +++ > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > @@ -113,12 +113,86 @@ > > intel_dp_aux_set_dynamic_backlight_percent(struct > intel_dp > > *intel_dp, > > > } > > > } > > > > > > +/* > > > + * Set PWM Frequency divider to match desired > frequency in > > vbt. > > > + * The PWM Frequency is calculated as 27Mhz / (F > x P). > > > + * - Where F = PWM Frequency Pre-Divider value > programmed > > by field 7:0 of the > > > + * EDP_BACKLIGHT_FREQ_SET register > (DPCD > > Address 00728h) > > > + * - Where P = 2^Pn, where Pn is the value > programmed by > > field 4:0 of the > > > + * EDP_PWMGEN_BIT_COUNT register > (DPCD Address > > 00724h) > > > + */ > > > +static void 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; > > > + > > > + /* 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; > > > + } > > > + > > > + fxp = 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 > > > + * - Effective frequency is within 25% of > desired > > frequency. > > > + */ > > > + 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; > > > + } > > > + 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; > > > + } > > > + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > > > + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > > > + > > > + fxp_min = fxp * 3 / 4; > > > + fxp_max = fxp * 5 / 4; > > > > > > You are allowing fxp between +/- 25% of the actual. > This isn't > > same as > > the "Effective frequency is within 25% of desired > frequency." > > right? The > > frequency can vary between -20% and +33%. > > > > > > You are right. > > You want me to change commit message to reflect this or > change the > > code to > > match the commit message? > > > I am okay with fixing the comment and commit message. Is the > 25% > arbitrary or based on the distances between the possible > values? Please > make a note in the comment if it's the former. > > > > > + if (fxp_min < (1 << pn_min) || (255 << > pn_max) < > > fxp_max) { > > > > > > + DRM_DEBUG_KMS("VBT defined backlight > frequency > > out of range\n"); > > > + return; > > > + } > > > + > > > + for (pn = pn_max; pn > pn_min; pn--) { > > Is there a reason this is not pn >= pn_min? > This is bug because f value will be incorrect in the case that pn == > pn_min. > Thanks for catching this. > Isn't that a side-effect using the right shift operation for division? Would DIV_ROUND_CLOSEST allow you to use pn_min? > > > > + f = clamp(fxp >> 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 the number of brightness control bits are changing, the > max. > brightness value changes too. > > Please see intel_dp_aux_setup_backlight(). > > > I think this is fine because > - intel_dp_aux_setup_backlight() will > called intel_dp_aux_enable_backlight() which > called intel_dp_aux_set_pwm_freq() first before determine the max > brightness value. > - Also, the panel I tested does not change > BACKLIGHT_BRIGHTNESS_BYTE_COUNT > > when changing the frequency. The only values I see being set for max brightness are 0xFFFF and 0xFF if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) panel->backlight.max = 0xFFFF; else panel->backlight.max = 0xFF; I can't see where you are setting this based on Pn. Can you please point out? From what I understand, max should be 2^Pn. Also, won't this function be called every time _enable_backlight() is called? Isn't doing this from setup sufficient? I guess, fixing it is an optimization that'd be nice to have but not necessary. > > > > > + return; > > > + } > > > + 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"); > > > + return; > > > + } > > > +} > > > + > > > static void intel_dp_aux_enable_backlight(struct > > intel_connector *connector) > > > { > > > struct intel_dp *intel_dp = > > enc_to_intel_dp(&connector->encoder->base); > > > uint8_t dpcd_buf = 0; > > > uint8_t new_dpcd_buf = 0; > > > uint8_t edp_backlight_mode = 0; > > > + bool freq_cap; > > > > > > if (drm_dp_dpcd_readb(&intel_dp->aux, > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > &dpcd_buf) != 1) { > > > @@ -151,6 +225,10 @@ static void > > intel_dp_aux_enable_backlight(struct intel_connector > > *connector) > > > DRM_DEBUG_KMS("Enable dynamic > brightness.\n"); > > > } > > > > > > + 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) { > > > if > (drm_dp_dpcd_writeb(&intel_dp->aux, > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > new_dpcd_buf) < 0) { > > > @@ -158,6 +236,9 @@ static void > > intel_dp_aux_enable_backlight(struct intel_connector > > *connector) > > > } > > > } > > > > > > + if (freq_cap) > > > + > intel_dp_aux_set_pwm_freq(connector); > > > + > > > set_aux_backlight_enable(intel_dp, true); > > > intel_dp_aux_set_backlight(connector, > > connector->panel.backlight.level); > > > } > > > > > > > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, May 15, 2017 at 11:21 PM, Pandiyan, Dhinakaran < dhinakaran.pandiyan@intel.com> wrote: > On Mon, 2017-05-15 at 17:43 -0700, Puthikorn Voravootivat wrote: > > > > > > On Mon, May 15, 2017 at 4:07 PM, Pandiyan, Dhinakaran > > <dhinakaran.pandiyan@intel.com> wrote: > > On Fri, 2017-05-12 at 17:31 -0700, Puthikorn Voravootivat > > wrote: > > > > > > > > > > > > On Fri, May 12, 2017 at 5:12 PM, Pandiyan, Dhinakaran > > > <dhinakaran.pandiyan@intel.com> wrote: > > > On Thu, 2017-05-11 at 16:02 -0700, Puthikorn > > Voravootivat > > > wrote: > > > > Read desired PWM frequency from panel vbt and > > calculate the > > > > value for divider in DPCD address 0x724 and 0x728 > > to have > > > > as many bits as possible for PWM duty cyle for > > granularity > > > of > > > > brightness adjustment while the frequency is still > > within > > > 25% > > > > of the desired frequency. > > > > > > I read a few eDP panel data sheets, the PWM > > frequencies all > > > start from > > > ~200Hz. If the VBT chooses this lowest value to > > allow for more > > > brightness control, and then this patch lowers the > > value by > > > another 25%, > > > we'll end up below the panel allowed PWM frequency. > > > > > > In fact, one of the systems I checked had PWM > > frequency as > > > 200Hz in VBT > > > and the panel datasheet also had PWM frequency range > > starting > > > from > > > 200Hz. Have you considered this case? > > > > > > The spec said "A given LCD panel typically has a limited > > range of > > > backlight frequency capability. > > > To limit the programmable frequency range, limitations are > > placed on > > > the allowable total divider ratio with the Sink device" > > > So I think it should be auto cap to 200Hz in this case. > > > > > > -DK > > > > > > > > Signed-off-by: Puthikorn Voravootivat > > <puthik@chromium.org> > > > > --- > > > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | > > 81 > > > +++++++++++++++++++++++++++ > > > > 1 file changed, 81 insertions(+) > > > > > > > > diff --git > > a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > index 0b48851013cc..6f10a2f1ab76 100644 > > > > --- > > a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > +++ > > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > @@ -113,12 +113,86 @@ > > > intel_dp_aux_set_dynamic_backlight_percent(struct > > intel_dp > > > *intel_dp, > > > > } > > > > } > > > > > > > > +/* > > > > + * Set PWM Frequency divider to match desired > > frequency in > > > vbt. > > > > + * The PWM Frequency is calculated as 27Mhz / (F > > x P). > > > > + * - Where F = PWM Frequency Pre-Divider value > > programmed > > > by field 7:0 of the > > > > + * EDP_BACKLIGHT_FREQ_SET register > > (DPCD > > > Address 00728h) > > > > + * - Where P = 2^Pn, where Pn is the value > > programmed by > > > field 4:0 of the > > > > + * EDP_PWMGEN_BIT_COUNT register > > (DPCD Address > > > 00724h) > > > > + */ > > > > +static void 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; > > > > + > > > > + /* 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; > > > > + } > > > > + > > > > + fxp = 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 > > > > + * - Effective frequency is within 25% of > > desired > > > frequency. > > > > + */ > > > > + 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; > > > > + } > > > > + 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; > > > > + } > > > > + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > > > > + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > > > > + > > > > + fxp_min = fxp * 3 / 4; > > > > + fxp_max = fxp * 5 / 4; > > > > > > > > > You are allowing fxp between +/- 25% of the actual. > > This isn't > > > same as > > > the "Effective frequency is within 25% of desired > > frequency." > > > right? The > > > frequency can vary between -20% and +33%. > > > > > > > > > You are right. > > > You want me to change commit message to reflect this or > > change the > > > code to > > > match the commit message? > > > > > > I am okay with fixing the comment and commit message. Is the > > 25% > > arbitrary or based on the distances between the possible > > values? Please > > make a note in the comment if it's the former. > > > > > > > > + if (fxp_min < (1 << pn_min) || (255 << > > pn_max) < > > > fxp_max) { > > > > > > > > > > + DRM_DEBUG_KMS("VBT defined backlight > > frequency > > > out of range\n"); > > > > + return; > > > > + } > > > > + > > > > + for (pn = pn_max; pn > pn_min; pn--) { > > > > Is there a reason this is not pn >= pn_min? > > This is bug because f value will be incorrect in the case that pn == > > pn_min. > > Thanks for catching this. > > > > Isn't that a side-effect using the right shift operation for division? > The bug is just for loop that exit too soon. Would DIV_ROUND_CLOSEST allow you to use pn_min? > It does not related to the point above, but DIV_ROUND_CLOSEST still better than just using right shift. I will change to that in next version. > > > > > > > + f = clamp(fxp >> 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 the number of brightness control bits are changing, the > > max. > > brightness value changes too. > > > > Please see intel_dp_aux_setup_backlight(). > > > > > > I think this is fine because > > - intel_dp_aux_setup_backlight() will > > called intel_dp_aux_enable_backlight() which > > called intel_dp_aux_set_pwm_freq() first before determine the max > > brightness value. > > - Also, the panel I tested does not change > > BACKLIGHT_BRIGHTNESS_BYTE_COUNT > > > > when changing the frequency. > > The only values I see being set for max brightness are 0xFFFF and 0xFF > > if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) > panel->backlight.max = 0xFFFF; > else > panel->backlight.max = 0xFF; > I can't see where you are setting this based on Pn. Can you please point > out? From what I understand, max should be 2^Pn. > > It is suppose to be 0xFF or 0xFFFF only. From eDP spec: 0x702 bit 2 BACKLIGHT_BRIGHTNESS_BYTE_COUNT 0 = Indicates that the Sink device supports a 1-byte setting for backlight brightness 1 = Indicates that the Sink device supports a 2-byte setting for the backlight brightness 0x722 EDP_BACKLIGHT_BRIGHTNESS_MSB The actual number of assigned bits for the backlight brightness PWM generator is set by field 4:0 of the EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h). Assigned bits are allocated to the MSB of the enabled register combination This means that if PWM_GEN_BIT_COUNT is less than 16 then the panel will ignore the least significant bit in EDP_BACKLIGHT_BRIGHTNESS register. For example, if BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and PWM_GEN_BIT_COUNT = 16 and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd In this case, all bits in EDP_BACKLIGHT_BRIGHTNESS will be used. 0xabcd means 0xabcd / 0xffff or (43981 / 65535) or 67.111% if BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and PWM_GEN_BIT_COUNT = 12 and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd In this case, the last 4 bits will be discarded. 0xabcd means 0xabc / 0xfff or 2748 / 4095 or 67.106% I think it should be fine to just have 8 or 16 bits of the max brightness and let panel drop the unneed bit to simplify the driver code. > Also, won't this function be called every time _enable_backlight() is > called? Isn't doing this from setup sufficient? I guess, fixing it is an > optimization that'd be nice to have but not necessary. > Lets get this patch set done first. I can send in another patch after this is go in to optimize this. Probably need to add new member in struct drm_i915_private > > > > > > > > + return; > > > > + } > > > > + 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"); > > > > + return; > > > > + } > > > > +} > > > > + > > > > static void intel_dp_aux_enable_backlight(struct > > > intel_connector *connector) > > > > { > > > > struct intel_dp *intel_dp = > > > enc_to_intel_dp(&connector->encoder->base); > > > > uint8_t dpcd_buf = 0; > > > > uint8_t new_dpcd_buf = 0; > > > > uint8_t edp_backlight_mode = 0; > > > > + bool freq_cap; > > > > > > > > if (drm_dp_dpcd_readb(&intel_dp->aux, > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > &dpcd_buf) != 1) { > > > > @@ -151,6 +225,10 @@ static void > > > intel_dp_aux_enable_backlight(struct intel_connector > > > *connector) > > > > DRM_DEBUG_KMS("Enable dynamic > > brightness.\n"); > > > > } > > > > > > > > + 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) { > > > > if > > (drm_dp_dpcd_writeb(&intel_dp->aux, > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > new_dpcd_buf) < 0) { > > > > @@ -158,6 +236,9 @@ static void > > > intel_dp_aux_enable_backlight(struct intel_connector > > > *connector) > > > > } > > > > } > > > > > > > > + if (freq_cap) > > > > + > > intel_dp_aux_set_pwm_freq(connector); > > > > + > > > > set_aux_backlight_enable(intel_dp, true); > > > > intel_dp_aux_set_backlight(connector, > > > connector->panel.backlight.level); > > > > } > > > > > > > > > > > > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On Tue, 2017-05-16 at 11:07 -0700, Puthikorn Voravootivat wrote: > > > On Mon, May 15, 2017 at 11:21 PM, Pandiyan, Dhinakaran > <dhinakaran.pandiyan@intel.com> wrote: > On Mon, 2017-05-15 at 17:43 -0700, Puthikorn Voravootivat > wrote: > > > > > > On Mon, May 15, 2017 at 4:07 PM, Pandiyan, Dhinakaran > > <dhinakaran.pandiyan@intel.com> wrote: > > On Fri, 2017-05-12 at 17:31 -0700, Puthikorn > Voravootivat > > wrote: > > > > > > > > > > > > On Fri, May 12, 2017 at 5:12 PM, Pandiyan, > Dhinakaran > > > <dhinakaran.pandiyan@intel.com> wrote: > > > On Thu, 2017-05-11 at 16:02 -0700, > Puthikorn > > Voravootivat > > > wrote: > > > > Read desired PWM frequency from panel > vbt and > > calculate the > > > > value for divider in DPCD address 0x724 > and 0x728 > > to have > > > > as many bits as possible for PWM duty > cyle for > > granularity > > > of > > > > brightness adjustment while the > frequency is still > > within > > > 25% > > > > of the desired frequency. > > > > > > I read a few eDP panel data sheets, the > PWM > > frequencies all > > > start from > > > ~200Hz. If the VBT chooses this lowest > value to > > allow for more > > > brightness control, and then this patch > lowers the > > value by > > > another 25%, > > > we'll end up below the panel allowed PWM > frequency. > > > > > > In fact, one of the systems I checked had > PWM > > frequency as > > > 200Hz in VBT > > > and the panel datasheet also had PWM > frequency range > > starting > > > from > > > 200Hz. Have you considered this case? > > > > > > The spec said "A given LCD panel typically has a > limited > > range of > > > backlight frequency capability. > > > To limit the programmable frequency range, > limitations are > > placed on > > > the allowable total divider ratio with the Sink > device" > > > So I think it should be auto cap to 200Hz in this > case. > > > > > > -DK > > > > > > > > Signed-off-by: Puthikorn Voravootivat > > <puthik@chromium.org> > > > > --- > > > > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | > > 81 > > > +++++++++++++++++++++++++++ > > > > 1 file changed, 81 insertions(+) > > > > > > > > diff --git > > a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > index 0b48851013cc..6f10a2f1ab76 100644 > > > > --- > > a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > +++ > > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > @@ -113,12 +113,86 @@ > > > > intel_dp_aux_set_dynamic_backlight_percent(struct > > intel_dp > > > *intel_dp, > > > > } > > > > } > > > > > > > > +/* > > > > + * Set PWM Frequency divider to match > desired > > frequency in > > > vbt. > > > > + * The PWM Frequency is calculated as > 27Mhz / (F > > x P). > > > > + * - Where F = PWM Frequency > Pre-Divider value > > programmed > > > by field 7:0 of the > > > > + * EDP_BACKLIGHT_FREQ_SET > register > > (DPCD > > > Address 00728h) > > > > + * - Where P = 2^Pn, where Pn is the > value > > programmed by > > > field 4:0 of the > > > > + * EDP_PWMGEN_BIT_COUNT > register > > (DPCD Address > > > 00724h) > > > > + */ > > > > +static void > 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; > > > > + > > > > + /* 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; > > > > + } > > > > + > > > > + fxp = > 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 > > > > + * - Effective frequency is within > 25% of > > desired > > > frequency. > > > > + */ > > > > + 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; > > > > + } > > > > + 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; > > > > + } > > > > + pn_min &= > DP_EDP_PWMGEN_BIT_COUNT_MASK; > > > > + pn_max &= > DP_EDP_PWMGEN_BIT_COUNT_MASK; > > > > + > > > > + fxp_min = fxp * 3 / 4; > > > > + fxp_max = fxp * 5 / 4; > > > > > > > > > You are allowing fxp between +/- 25% of > the actual. > > This isn't > > > same as > > > the "Effective frequency is within 25% of > desired > > frequency." > > > right? The > > > frequency can vary between -20% and +33%. > > > > > > > > > You are right. > > > You want me to change commit message to reflect > this or > > change the > > > code to > > > match the commit message? > > > > > > I am okay with fixing the comment and commit > message. Is the > > 25% > > arbitrary or based on the distances between the > possible > > values? Please > > make a note in the comment if it's the former. > > > > > > > > + if (fxp_min < (1 << pn_min) || > (255 << > > pn_max) < > > > fxp_max) { > > > > > > > > > > + DRM_DEBUG_KMS("VBT defined > backlight > > frequency > > > out of range\n"); > > > > + return; > > > > + } > > > > + > > > > + for (pn = pn_max; pn > pn_min; > pn--) { > > > > Is there a reason this is not pn >= pn_min? > > This is bug because f value will be incorrect in the case > that pn == > > pn_min. > > Thanks for catching this. > > > > > Isn't that a side-effect using the right shift operation for > division? > The bug is just for loop that exit too soon. > Not sure I got that, what's the invalid "f" case that you see with pn==pn_min? > Would DIV_ROUND_CLOSEST allow you to use pn_min? > It does not related to the point above, but DIV_ROUND_CLOSEST still > better than just using right shift. I will change to that in next > version. > > > > > > > > + f = clamp(fxp >> 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 the number of brightness control bits are > changing, the > > max. > > brightness value changes too. > > > > Please see intel_dp_aux_setup_backlight(). > > > > > > I think this is fine because > > - intel_dp_aux_setup_backlight() will > > called intel_dp_aux_enable_backlight() which > > called intel_dp_aux_set_pwm_freq() first before determine > the max > > brightness value. > > - Also, the panel I tested does not change > > BACKLIGHT_BRIGHTNESS_BYTE_COUNT > > > > when changing the frequency. > > > The only values I see being set for max brightness are 0xFFFF > and 0xFF > > if (intel_dp->edp_dpcd[2] & > DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) > panel->backlight.max = 0xFFFF; > else > panel->backlight.max = 0xFF; > > I can't see where you are setting this based on Pn. Can you > please point > out? From what I understand, max should be 2^Pn. > > > It is suppose to be 0xFF or 0xFFFF only. > > > From eDP spec: > 0x702 bit 2 BACKLIGHT_BRIGHTNESS_BYTE_COUNT > 0 = Indicates that the Sink device supports a 1-byte setting for > backlight brightness > 1 = Indicates that the Sink device supports a 2-byte setting for the > backlight brightness > > > 0x722 EDP_BACKLIGHT_BRIGHTNESS_MSB > The actual number of assigned bits for the backlight brightness PWM > generator is set by > field 4:0 of the EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h). > Assigned bits are allocated to the MSB of the enabled register > combination > ^This makes it somewhat clear but gets confusing again. > > This means that if PWM_GEN_BIT_COUNT is less than 16 then the panel > will ignore > the least significant bit in EDP_BACKLIGHT_BRIGHTNESS register. > > > For example, > if BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and PWM_GEN_BIT_COUNT = 16 > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd > In this case, all bits in EDP_BACKLIGHT_BRIGHTNESS will be used. > > 0xabcd means 0xabcd / 0xffff or (43981 / 65535) or 67.111% > > > if BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and PWM_GEN_BIT_COUNT = 12 > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd > > In this case, the last 4 bits will be discarded. > 0xabcd means 0xabc / 0xfff or 2748 / 4095 or 67.106% > > > I think it should be fine to just have 8 or 16 bits of the max > brightness and let panel drop > the unneed bit to simplify the driver code. > Note: The number of assigned or controllable bits will have the ability to provide full brightness control. Examples: • For 10-bit operation, 000h = 0% brightness, and 3FFh = 100% brightness. Going by the logic you stated, for a 10-bit control, the MSB and LSB registers have to be written FFFFh to set 100% brightness with the last 6 bits dropped off by the hardware. I don't like the way the spec leaves this to interpretation. Have you experimented with both 0x0ABC and 0xABCD for the 12-bit, BYTE_COUNT==1 example you gave? > > > Also, won't this function be called every time > _enable_backlight() is > called? Isn't doing this from setup sufficient? I guess, > fixing it is an > optimization that'd be nice to have but not necessary. > > Lets get this patch set done first. I can send in another patch after > this is go in to optimize this. Sure. This patch should be good to go with answers to my questions. The only thing remaining would be getting an ACK from Jani for adding a new parameter for dynamic backlight control. -DK > Probably need to add new member in struct drm_i915_private > > > > > > > > > + return; > > > > + } > > > > + 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"); > > > > + return; > > > > + } > > > > +} > > > > + > > > > static void > intel_dp_aux_enable_backlight(struct > > > intel_connector *connector) > > > > { > > > > struct intel_dp *intel_dp = > > > > enc_to_intel_dp(&connector->encoder->base); > > > > uint8_t dpcd_buf = 0; > > > > uint8_t new_dpcd_buf = 0; > > > > uint8_t edp_backlight_mode = 0; > > > > + bool freq_cap; > > > > > > > > if > (drm_dp_dpcd_readb(&intel_dp->aux, > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > &dpcd_buf) != 1) { > > > > @@ -151,6 +225,10 @@ static void > > > intel_dp_aux_enable_backlight(struct > intel_connector > > > *connector) > > > > DRM_DEBUG_KMS("Enable > dynamic > > brightness.\n"); > > > > } > > > > > > > > + 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) { > > > > if > > (drm_dp_dpcd_writeb(&intel_dp->aux, > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > new_dpcd_buf) < 0) { > > > > @@ -158,6 +236,9 @@ static void > > > intel_dp_aux_enable_backlight(struct > intel_connector > > > *connector) > > > > } > > > > } > > > > > > > > + if (freq_cap) > > > > + > > intel_dp_aux_set_pwm_freq(connector); > > > > + > > > > set_aux_backlight_enable(intel_dp, > true); > > > > > intel_dp_aux_set_backlight(connector, > > > connector->panel.backlight.level); > > > > } > > > > > > > > > > > > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, May 16, 2017 at 1:29 PM, Pandiyan, Dhinakaran < dhinakaran.pandiyan@intel.com> wrote: > On Tue, 2017-05-16 at 11:07 -0700, Puthikorn Voravootivat wrote: > > > > > > On Mon, May 15, 2017 at 11:21 PM, Pandiyan, Dhinakaran > > <dhinakaran.pandiyan@intel.com> wrote: > > On Mon, 2017-05-15 at 17:43 -0700, Puthikorn Voravootivat > > wrote: > > > > > > > > > On Mon, May 15, 2017 at 4:07 PM, Pandiyan, Dhinakaran > > > <dhinakaran.pandiyan@intel.com> wrote: > > > On Fri, 2017-05-12 at 17:31 -0700, Puthikorn > > Voravootivat > > > wrote: > > > > > > > > > > > > > > > > On Fri, May 12, 2017 at 5:12 PM, Pandiyan, > > Dhinakaran > > > > <dhinakaran.pandiyan@intel.com> wrote: > > > > On Thu, 2017-05-11 at 16:02 -0700, > > Puthikorn > > > Voravootivat > > > > wrote: > > > > > Read desired PWM frequency from panel > > vbt and > > > calculate the > > > > > value for divider in DPCD address 0x724 > > and 0x728 > > > to have > > > > > as many bits as possible for PWM duty > > cyle for > > > granularity > > > > of > > > > > brightness adjustment while the > > frequency is still > > > within > > > > 25% > > > > > of the desired frequency. > > > > > > > > I read a few eDP panel data sheets, the > > PWM > > > frequencies all > > > > start from > > > > ~200Hz. If the VBT chooses this lowest > > value to > > > allow for more > > > > brightness control, and then this patch > > lowers the > > > value by > > > > another 25%, > > > > we'll end up below the panel allowed PWM > > frequency. > > > > > > > > In fact, one of the systems I checked had > > PWM > > > frequency as > > > > 200Hz in VBT > > > > and the panel datasheet also had PWM > > frequency range > > > starting > > > > from > > > > 200Hz. Have you considered this case? > > > > > > > > The spec said "A given LCD panel typically has a > > limited > > > range of > > > > backlight frequency capability. > > > > To limit the programmable frequency range, > > limitations are > > > placed on > > > > the allowable total divider ratio with the Sink > > device" > > > > So I think it should be auto cap to 200Hz in this > > case. > > > > > > > > -DK > > > > > > > > > > Signed-off-by: Puthikorn Voravootivat > > > <puthik@chromium.org> > > > > > --- > > > > > > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | > > > 81 > > > > +++++++++++++++++++++++++++ > > > > > 1 file changed, 81 insertions(+) > > > > > > > > > > diff --git > > > a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > > > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > > index 0b48851013cc..6f10a2f1ab76 100644 > > > > > --- > > > a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > > +++ > > > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > > @@ -113,12 +113,86 @@ > > > > > > intel_dp_aux_set_dynamic_backlight_percent(struct > > > intel_dp > > > > *intel_dp, > > > > > } > > > > > } > > > > > > > > > > +/* > > > > > + * Set PWM Frequency divider to match > > desired > > > frequency in > > > > vbt. > > > > > + * The PWM Frequency is calculated as > > 27Mhz / (F > > > x P). > > > > > + * - Where F = PWM Frequency > > Pre-Divider value > > > programmed > > > > by field 7:0 of the > > > > > + * EDP_BACKLIGHT_FREQ_SET > > register > > > (DPCD > > > > Address 00728h) > > > > > + * - Where P = 2^Pn, where Pn is the > > value > > > programmed by > > > > field 4:0 of the > > > > > + * EDP_PWMGEN_BIT_COUNT > > register > > > (DPCD Address > > > > 00724h) > > > > > + */ > > > > > +static void > > 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; > > > > > + > > > > > + /* 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; > > > > > + } > > > > > + > > > > > + fxp = > > 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 > > > > > + * - Effective frequency is within > > 25% of > > > desired > > > > frequency. > > > > > + */ > > > > > + 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; > > > > > + } > > > > > + 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; > > > > > + } > > > > > + pn_min &= > > DP_EDP_PWMGEN_BIT_COUNT_MASK; > > > > > + pn_max &= > > DP_EDP_PWMGEN_BIT_COUNT_MASK; > > > > > + > > > > > + fxp_min = fxp * 3 / 4; > > > > > + fxp_max = fxp * 5 / 4; > > > > > > > > > > > > You are allowing fxp between +/- 25% of > > the actual. > > > This isn't > > > > same as > > > > the "Effective frequency is within 25% of > > desired > > > frequency." > > > > right? The > > > > frequency can vary between -20% and +33%. > > > > > > > > > > > > You are right. > > > > You want me to change commit message to reflect > > this or > > > change the > > > > code to > > > > match the commit message? > > > > > > > > > I am okay with fixing the comment and commit > > message. Is the > > > 25% > > > arbitrary or based on the distances between the > > possible > > > values? Please > > > make a note in the comment if it's the former. > > > > > > > > > > > + if (fxp_min < (1 << pn_min) || > > (255 << > > > pn_max) < > > > > fxp_max) { > > > > > > > > > > > > > > + DRM_DEBUG_KMS("VBT defined > > backlight > > > frequency > > > > out of range\n"); > > > > > + return; > > > > > + } > > > > > + > > > > > + for (pn = pn_max; pn > pn_min; > > pn--) { > > > > > > Is there a reason this is not pn >= pn_min? > > > This is bug because f value will be incorrect in the case > > that pn == > > > pn_min. > > > Thanks for catching this. > > > > > > > > > Isn't that a side-effect using the right shift operation for > > division? > > The bug is just for loop that exit too soon. > > > Not sure I got that, what's the invalid "f" case that you see with > pn==pn_min? > > > if the for loop has pn > pn_min, when pn == pn_min we will exit the loop first without running the f = clamp(fxp >> pn, 1, 255); in the next line. This would fix with pn >= pn_min in for loop. We guarantee that the break statement will be called and pn won't be pn_min - 1 because we check (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) before entering this for loop. > > Would DIV_ROUND_CLOSEST allow you to use pn_min? > > It does not related to the point above, but DIV_ROUND_CLOSEST still > > better than just using right shift. I will change to that in next > > version. > > > > > > > > > > > > + f = clamp(fxp >> 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 the number of brightness control bits are > > changing, the > > > max. > > > brightness value changes too. > > > > > > Please see intel_dp_aux_setup_backlight(). > > > > > > > > > I think this is fine because > > > - intel_dp_aux_setup_backlight() will > > > called intel_dp_aux_enable_backlight() which > > > called intel_dp_aux_set_pwm_freq() first before determine > > the max > > > brightness value. > > > - Also, the panel I tested does not change > > > BACKLIGHT_BRIGHTNESS_BYTE_COUNT > > > > > > when changing the frequency. > > > > > > The only values I see being set for max brightness are 0xFFFF > > and 0xFF > > > > if (intel_dp->edp_dpcd[2] & > > DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) > > panel->backlight.max = 0xFFFF; > > else > > panel->backlight.max = 0xFF; > > > > I can't see where you are setting this based on Pn. Can you > > please point > > out? From what I understand, max should be 2^Pn. > > > > > > It is suppose to be 0xFF or 0xFFFF only. > > > > > > From eDP spec: > > 0x702 bit 2 BACKLIGHT_BRIGHTNESS_BYTE_COUNT > > 0 = Indicates that the Sink device supports a 1-byte setting for > > backlight brightness > > 1 = Indicates that the Sink device supports a 2-byte setting for the > > backlight brightness > > > > > > 0x722 EDP_BACKLIGHT_BRIGHTNESS_MSB > > The actual number of assigned bits for the backlight brightness PWM > > generator is set by > > field 4:0 of the EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h). > > Assigned bits are allocated to the MSB of the enabled register > > combination > > > ^This makes it somewhat clear but gets confusing again. > > > > This means that if PWM_GEN_BIT_COUNT is less than 16 then the panel > > will ignore > > the least significant bit in EDP_BACKLIGHT_BRIGHTNESS register. > > > > > > For example, > > if BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and PWM_GEN_BIT_COUNT = 16 > > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd > > In this case, all bits in EDP_BACKLIGHT_BRIGHTNESS will be used. > > > > 0xabcd means 0xabcd / 0xffff or (43981 / 65535) or 67.111% > > > > > > if BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and PWM_GEN_BIT_COUNT = 12 > > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd > > > > In this case, the last 4 bits will be discarded. > > 0xabcd means 0xabc / 0xfff or 2748 / 4095 or 67.106% > > > > > > I think it should be fine to just have 8 or 16 bits of the max > > brightness and let panel drop > > the unneed bit to simplify the driver code. > > > > Note: > The number of assigned or controllable bits will have the ability to > provide full brightness control. Examples: > • > For 10-bit operation, 000h = 0% brightness, and 3FFh = 100% brightness. > > Going by the logic you stated, for a 10-bit control, the MSB and LSB > registers have to be written FFFFh to set 100% brightness with the last > 6 bits dropped off by the hardware. > > I don't like the way the spec leaves this to interpretation. Have you > experimented with both 0x0ABC and 0xABCD for the 12-bit, BYTE_COUNT==1 > example you gave? > > Yes, already test with the real panel. Here is better test case than 0xABCD 0xFFFF with 6, 12, 16 shows very bright screen with same brightness in all case. (All 100%) 0x0FFF with 6, 12, 16 bits shows dim screen. 6 bit case is slightly dimmer than other case. Note that the brightness are 4.76% / 6.23% / 6.25% in 6 / 12 / 16 bits case. You can apply this patch for easy testing https://patchwork.kernel.org/patch/6241481/ > > > > > > > Also, won't this function be called every time > > _enable_backlight() is > > called? Isn't doing this from setup sufficient? I guess, > > fixing it is an > > optimization that'd be nice to have but not necessary. > > > > Lets get this patch set done first. I can send in another patch after > > this is go in to optimize this. > > Sure. This patch should be good to go with answers to my questions. The > only thing remaining would be getting an ACK from Jani for adding a new > parameter for dynamic backlight control. > > > -DK > > > Probably need to add new member in struct drm_i915_private > > > > > > > > > > > > > + return; > > > > > + } > > > > > + 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"); > > > > > + return; > > > > > + } > > > > > +} > > > > > + > > > > > static void > > intel_dp_aux_enable_backlight(struct > > > > intel_connector *connector) > > > > > { > > > > > struct intel_dp *intel_dp = > > > > > > enc_to_intel_dp(&connector->encoder->base); > > > > > uint8_t dpcd_buf = 0; > > > > > uint8_t new_dpcd_buf = 0; > > > > > uint8_t edp_backlight_mode = 0; > > > > > + bool freq_cap; > > > > > > > > > > if > > (drm_dp_dpcd_readb(&intel_dp->aux, > > > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > > &dpcd_buf) != 1) { > > > > > @@ -151,6 +225,10 @@ static void > > > > intel_dp_aux_enable_backlight(struct > > intel_connector > > > > *connector) > > > > > DRM_DEBUG_KMS("Enable > > dynamic > > > brightness.\n"); > > > > > } > > > > > > > > > > + 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) { > > > > > if > > > (drm_dp_dpcd_writeb(&intel_dp->aux, > > > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > > new_dpcd_buf) < 0) { > > > > > @@ -158,6 +236,9 @@ static void > > > > intel_dp_aux_enable_backlight(struct > > intel_connector > > > > *connector) > > > > > } > > > > > } > > > > > > > > > > + if (freq_cap) > > > > > + > > > intel_dp_aux_set_pwm_freq(connector); > > > > > + > > > > > set_aux_backlight_enable(intel_dp, > > true); > > > > > > > intel_dp_aux_set_backlight(connector, > > > > connector->panel.backlight.level); > > > > > } > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On Tue, 2017-05-16 at 13:56 -0700, Puthikorn Voravootivat wrote: > > > On Tue, May 16, 2017 at 1:29 PM, Pandiyan, Dhinakaran > <dhinakaran.pandiyan@intel.com> wrote: > On Tue, 2017-05-16 at 11:07 -0700, Puthikorn Voravootivat > wrote: > > > > > > On Mon, May 15, 2017 at 11:21 PM, Pandiyan, Dhinakaran > > <dhinakaran.pandiyan@intel.com> wrote: > > On Mon, 2017-05-15 at 17:43 -0700, Puthikorn > Voravootivat > > wrote: > > > > > > > > > On Mon, May 15, 2017 at 4:07 PM, Pandiyan, > Dhinakaran > > > <dhinakaran.pandiyan@intel.com> wrote: > > > On Fri, 2017-05-12 at 17:31 -0700, > Puthikorn > > Voravootivat > > > wrote: > > > > > > > > > > > > > > > > On Fri, May 12, 2017 at 5:12 PM, > Pandiyan, > > Dhinakaran > > > > <dhinakaran.pandiyan@intel.com> wrote: > > > > On Thu, 2017-05-11 at 16:02 > -0700, > > Puthikorn > > > Voravootivat > > > > wrote: > > > > > Read desired PWM frequency > from panel > > vbt and > > > calculate the > > > > > value for divider in DPCD > address 0x724 > > and 0x728 > > > to have > > > > > as many bits as possible for > PWM duty > > cyle for > > > granularity > > > > of > > > > > brightness adjustment while > the > > frequency is still > > > within > > > > 25% > > > > > of the desired frequency. > > > > > > > > I read a few eDP panel data > sheets, the > > PWM > > > frequencies all > > > > start from > > > > ~200Hz. If the VBT chooses this > lowest > > value to > > > allow for more > > > > brightness control, and then > this patch > > lowers the > > > value by > > > > another 25%, > > > > we'll end up below the panel > allowed PWM > > frequency. > > > > > > > > In fact, one of the systems I > checked had > > PWM > > > frequency as > > > > 200Hz in VBT > > > > and the panel datasheet also had > PWM > > frequency range > > > starting > > > > from > > > > 200Hz. Have you considered this > case? > > > > > > > > The spec said "A given LCD panel > typically has a > > limited > > > range of > > > > backlight frequency capability. > > > > To limit the programmable frequency > range, > > limitations are > > > placed on > > > > the allowable total divider ratio with > the Sink > > device" > > > > So I think it should be auto cap to > 200Hz in this > > case. > > > > > > > > -DK > > > > > > > > > > Signed-off-by: Puthikorn > Voravootivat > > > <puthik@chromium.org> > > > > > --- > > > > > > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | > > > 81 > > > > +++++++++++++++++++++++++++ > > > > > 1 file changed, 81 > insertions(+) > > > > > > > > > > diff --git > > > > a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > > > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > > index > 0b48851013cc..6f10a2f1ab76 100644 > > > > > --- > > > > a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > > +++ > > > > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > > @@ -113,12 +113,86 @@ > > > > > > intel_dp_aux_set_dynamic_backlight_percent(struct > > > intel_dp > > > > *intel_dp, > > > > > } > > > > > } > > > > > > > > > > +/* > > > > > + * Set PWM Frequency divider > to match > > desired > > > frequency in > > > > vbt. > > > > > + * The PWM Frequency is > calculated as > > 27Mhz / (F > > > x P). > > > > > + * - Where F = PWM Frequency > > Pre-Divider value > > > programmed > > > > by field 7:0 of the > > > > > + * > EDP_BACKLIGHT_FREQ_SET > > register > > > (DPCD > > > > Address 00728h) > > > > > + * - Where P = 2^Pn, where Pn > is the > > value > > > programmed by > > > > field 4:0 of the > > > > > + * > EDP_PWMGEN_BIT_COUNT > > register > > > (DPCD Address > > > > 00724h) > > > > > + */ > > > > > +static void > > 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; > > > > > + > > > > > + /* 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; > > > > > + } > > > > > + > > > > > + fxp = > > 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 > > > > > + * - Effective frequency > is within > > 25% of > > > desired > > > > frequency. > > > > > + */ > > > > > + 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; > > > > > + } > > > > > + 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; > > > > > + } > > > > > + pn_min &= > > DP_EDP_PWMGEN_BIT_COUNT_MASK; > > > > > + pn_max &= > > DP_EDP_PWMGEN_BIT_COUNT_MASK; > > > > > + > > > > > + fxp_min = fxp * 3 / 4; > > > > > + fxp_max = fxp * 5 / 4; > > > > > > > > > > > > You are allowing fxp between +/- > 25% of > > the actual. > > > This isn't > > > > same as > > > > the "Effective frequency is > within 25% of > > desired > > > frequency." > > > > right? The > > > > frequency can vary between -20% > and +33%. > > > > > > > > > > > > You are right. > > > > You want me to change commit message to > reflect > > this or > > > change the > > > > code to > > > > match the commit message? > > > > > > > > > I am okay with fixing the comment and > commit > > message. Is the > > > 25% > > > arbitrary or based on the distances > between the > > possible > > > values? Please > > > make a note in the comment if it's the > former. > > > > > > > > > > > + if (fxp_min < (1 << > pn_min) || > > (255 << > > > pn_max) < > > > > fxp_max) { > > > > > > > > > > > > > > + > DRM_DEBUG_KMS("VBT defined > > backlight > > > frequency > > > > out of range\n"); > > > > > + return; > > > > > + } > > > > > + > > > > > + for (pn = pn_max; pn > > pn_min; > > pn--) { > > > > > > Is there a reason this is not pn >= > pn_min? > > > This is bug because f value will be incorrect in > the case > > that pn == > > > pn_min. > > > Thanks for catching this. > > > > > > > > > Isn't that a side-effect using the right shift > operation for > > division? > > The bug is just for loop that exit too soon. > > > > Not sure I got that, what's the invalid "f" case that you see > with > pn==pn_min? > > > > if the for loop has pn > pn_min, when pn == pn_min we will exit the > loop first > without running the f = clamp(fxp >> pn, 1, 255); in the next line. > > > This would fix with pn >= pn_min in for loop. > Ah! I thought you were giving me a reason to not change it to pn >= pn_min . > > We guarantee that the break statement will be called and pn won't be > pn_min - 1 > because we check (fxp_min < (1 << pn_min) || (255 << pn_max) < > fxp_max) before > entering this for loop. > > > > > Would DIV_ROUND_CLOSEST allow you to use pn_min? > > It does not related to the point above, but > DIV_ROUND_CLOSEST still > > better than just using right shift. I will change to that in > next > > version. > > > > > > > > > > > > + f = clamp(fxp >> > 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 the number of brightness control bits > are > > changing, the > > > max. > > > brightness value changes too. > > > > > > Please see intel_dp_aux_setup_backlight(). > > > > > > > > > I think this is fine because > > > - intel_dp_aux_setup_backlight() will > > > called intel_dp_aux_enable_backlight() which > > > called intel_dp_aux_set_pwm_freq() first before > determine > > the max > > > brightness value. > > > - Also, the panel I tested does not change > > > BACKLIGHT_BRIGHTNESS_BYTE_COUNT > > > > > > when changing the frequency. > > > > > > The only values I see being set for max brightness > are 0xFFFF > > and 0xFF > > > > if (intel_dp->edp_dpcd[2] & > > DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) > > panel->backlight.max = 0xFFFF; > > else > > panel->backlight.max = 0xFF; > > > > I can't see where you are setting this based on Pn. > Can you > > please point > > out? From what I understand, max should be 2^Pn. > > > > > > It is suppose to be 0xFF or 0xFFFF only. > > > > > > From eDP spec: > > 0x702 bit 2 BACKLIGHT_BRIGHTNESS_BYTE_COUNT > > 0 = Indicates that the Sink device supports a 1-byte setting > for > > backlight brightness > > 1 = Indicates that the Sink device supports a 2-byte setting > for the > > backlight brightness > > > > > > 0x722 EDP_BACKLIGHT_BRIGHTNESS_MSB > > The actual number of assigned bits for the backlight > brightness PWM > > generator is set by > > field 4:0 of the EDP_PWMGEN_BIT_COUNT register (DPCD Address > 00724h). > > Assigned bits are allocated to the MSB of the enabled > register > > combination > > > > ^This makes it somewhat clear but gets confusing again. > > > > This means that if PWM_GEN_BIT_COUNT is less than 16 then > the panel > > will ignore > > the least significant bit in EDP_BACKLIGHT_BRIGHTNESS > register. > > > > > > For example, > > if BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and > PWM_GEN_BIT_COUNT = 16 > > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd > > In this case, all bits in EDP_BACKLIGHT_BRIGHTNESS will be > used. > > > > 0xabcd means 0xabcd / 0xffff or (43981 / 65535) or 67.111% > > > > > > if BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and > PWM_GEN_BIT_COUNT = 12 > > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd > > > > In this case, the last 4 bits will be discarded. > > 0xabcd means 0xabc / 0xfff or 2748 / 4095 or 67.106% > > > > > > I think it should be fine to just have 8 or 16 bits of the > max > > brightness and let panel drop > > the unneed bit to simplify the driver code. > > > > Note: > The number of assigned or controllable bits will have the > ability to > provide full brightness control. Examples: > • > For 10-bit operation, 000h = 0% brightness, and 3FFh = 100% > brightness. > > Going by the logic you stated, for a 10-bit control, the MSB > and LSB > registers have to be written FFFFh to set 100% brightness with > the last > 6 bits dropped off by the hardware. > > I don't like the way the spec leaves this to interpretation. > Have you > experimented with both 0x0ABC and 0xABCD for the 12-bit, > BYTE_COUNT==1 > example you gave? > > Yes, already test with the real panel. > > > Here is better test case than 0xABCD > 0xFFFF with 6, 12, 16 shows very bright screen with same brightness in > all case. (All 100%) > 0x0FFF with 6, 12, 16 bits shows dim screen. 6 bit case is slightly > dimmer than other case. > Note that the brightness are 4.76% / 6.23% / 6.25% in 6 / 12 / 16 bits > case. > Great! Thanks for confirming. > You can apply this patch for easy testing > https://patchwork.kernel.org/patch/6241481/ > > > > > > > > > Also, won't this function be called every time > > _enable_backlight() is > > called? Isn't doing this from setup sufficient? I > guess, > > fixing it is an > > optimization that'd be nice to have but not > necessary. > > > > Lets get this patch set done first. I can send in another > patch after > > this is go in to optimize this. > > Sure. This patch should be good to go with answers to my > questions. The > only thing remaining would be getting an ACK from Jani for > adding a new > parameter for dynamic backlight control. > > > -DK > > > Probably need to add new member in struct drm_i915_private > > > > > > > > > > > > > + return; > > > > > + } > > > > > + 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"); > > > > > + return; > > > > > + } > > > > > +} > > > > > + > > > > > static void > > intel_dp_aux_enable_backlight(struct > > > > intel_connector *connector) > > > > > { > > > > > struct intel_dp > *intel_dp = > > > > > > enc_to_intel_dp(&connector->encoder->base); > > > > > uint8_t dpcd_buf = 0; > > > > > uint8_t new_dpcd_buf = > 0; > > > > > uint8_t > edp_backlight_mode = 0; > > > > > + bool freq_cap; > > > > > > > > > > if > > (drm_dp_dpcd_readb(&intel_dp->aux, > > > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > > &dpcd_buf) != 1) { > > > > > @@ -151,6 +225,10 @@ static > void > > > > > intel_dp_aux_enable_backlight(struct > > intel_connector > > > > *connector) > > > > > > DRM_DEBUG_KMS("Enable > > dynamic > > > brightness.\n"); > > > > > } > > > > > > > > > > + 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) { > > > > > if > > > (drm_dp_dpcd_writeb(&intel_dp->aux, > > > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > > new_dpcd_buf) < 0) { > > > > > @@ -158,6 +236,9 @@ static > void > > > > > intel_dp_aux_enable_backlight(struct > > intel_connector > > > > *connector) > > > > > } > > > > > } > > > > > > > > > > + if (freq_cap) > > > > > + > > > intel_dp_aux_set_pwm_freq(connector); > > > > > + > > > > > > set_aux_backlight_enable(intel_dp, > > true); > > > > > > > intel_dp_aux_set_backlight(connector, > > > > > connector->panel.backlight.level); > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, May 16, 2017 at 2:21 PM, Pandiyan, Dhinakaran < dhinakaran.pandiyan@intel.com> wrote: > On Tue, 2017-05-16 at 13:56 -0700, Puthikorn Voravootivat wrote: > > > > > > On Tue, May 16, 2017 at 1:29 PM, Pandiyan, Dhinakaran > > <dhinakaran.pandiyan@intel.com> wrote: > > On Tue, 2017-05-16 at 11:07 -0700, Puthikorn Voravootivat > > wrote: > > > > > > > > > On Mon, May 15, 2017 at 11:21 PM, Pandiyan, Dhinakaran > > > <dhinakaran.pandiyan@intel.com> wrote: > > > On Mon, 2017-05-15 at 17:43 -0700, Puthikorn > > Voravootivat > > > wrote: > > > > > > > > > > > > On Mon, May 15, 2017 at 4:07 PM, Pandiyan, > > Dhinakaran > > > > <dhinakaran.pandiyan@intel.com> wrote: > > > > On Fri, 2017-05-12 at 17:31 -0700, > > Puthikorn > > > Voravootivat > > > > wrote: > > > > > > > > > > > > > > > > > > > > On Fri, May 12, 2017 at 5:12 PM, > > Pandiyan, > > > Dhinakaran > > > > > <dhinakaran.pandiyan@intel.com> wrote: > > > > > On Thu, 2017-05-11 at 16:02 > > -0700, > > > Puthikorn > > > > Voravootivat > > > > > wrote: > > > > > > Read desired PWM frequency > > from panel > > > vbt and > > > > calculate the > > > > > > value for divider in DPCD > > address 0x724 > > > and 0x728 > > > > to have > > > > > > as many bits as possible for > > PWM duty > > > cyle for > > > > granularity > > > > > of > > > > > > brightness adjustment while > > the > > > frequency is still > > > > within > > > > > 25% > > > > > > of the desired frequency. > > > > > > > > > > I read a few eDP panel data > > sheets, the > > > PWM > > > > frequencies all > > > > > start from > > > > > ~200Hz. If the VBT chooses this > > lowest > > > value to > > > > allow for more > > > > > brightness control, and then > > this patch > > > lowers the > > > > value by > > > > > another 25%, > > > > > we'll end up below the panel > > allowed PWM > > > frequency. > > > > > > > > > > In fact, one of the systems I > > checked had > > > PWM > > > > frequency as > > > > > 200Hz in VBT > > > > > and the panel datasheet also had > > PWM > > > frequency range > > > > starting > > > > > from > > > > > 200Hz. Have you considered this > > case? > > > > > > > > > > The spec said "A given LCD panel > > typically has a > > > limited > > > > range of > > > > > backlight frequency capability. > > > > > To limit the programmable frequency > > range, > > > limitations are > > > > placed on > > > > > the allowable total divider ratio with > > the Sink > > > device" > > > > > So I think it should be auto cap to > > 200Hz in this > > > case. > > > > > > > > > > -DK > > > > > > > > > > > > Signed-off-by: Puthikorn > > Voravootivat > > > > <puthik@chromium.org> > > > > > > --- > > > > > > > > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | > > > > 81 > > > > > +++++++++++++++++++++++++++ > > > > > > 1 file changed, 81 > > insertions(+) > > > > > > > > > > > > diff --git > > > > > > a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > > > > > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > > > index > > 0b48851013cc..6f10a2f1ab76 100644 > > > > > > --- > > > > > > a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > > > +++ > > > > > > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > > > @@ -113,12 +113,86 @@ > > > > > > > > intel_dp_aux_set_dynamic_backlight_percent(struct > > > > intel_dp > > > > > *intel_dp, > > > > > > } > > > > > > } > > > > > > > > > > > > +/* > > > > > > + * Set PWM Frequency divider > > to match > > > desired > > > > frequency in > > > > > vbt. > > > > > > + * The PWM Frequency is > > calculated as > > > 27Mhz / (F > > > > x P). > > > > > > + * - Where F = PWM Frequency > > > Pre-Divider value > > > > programmed > > > > > by field 7:0 of the > > > > > > + * > > EDP_BACKLIGHT_FREQ_SET > > > register > > > > (DPCD > > > > > Address 00728h) > > > > > > + * - Where P = 2^Pn, where Pn > > is the > > > value > > > > programmed by > > > > > field 4:0 of the > > > > > > + * > > EDP_PWMGEN_BIT_COUNT > > > register > > > > (DPCD Address > > > > > 00724h) > > > > > > + */ > > > > > > +static void > > > 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; > > > > > > + > > > > > > + /* 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; > > > > > > + } > > > > > > + > > > > > > + fxp = > > > 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 > > > > > > + * - Effective frequency > > is within > > > 25% of > > > > desired > > > > > frequency. > > > > > > + */ > > > > > > + 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; > > > > > > + } > > > > > > + 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; > > > > > > + } > > > > > > + pn_min &= > > > DP_EDP_PWMGEN_BIT_COUNT_MASK; > > > > > > + pn_max &= > > > DP_EDP_PWMGEN_BIT_COUNT_MASK; > > > > > > + > > > > > > + fxp_min = fxp * 3 / 4; > > > > > > + fxp_max = fxp * 5 / 4; > > > > > > > > > > > > > > > You are allowing fxp between +/- > > 25% of > > > the actual. > > > > This isn't > > > > > same as > > > > > the "Effective frequency is > > within 25% of > > > desired > > > > frequency." > > > > > right? The > > > > > frequency can vary between -20% > > and +33%. > > > > > > > > > > > > > > > You are right. > > > > > You want me to change commit message to > > reflect > > > this or > > > > change the > > > > > code to > > > > > match the commit message? > > > > > > > > > > > > I am okay with fixing the comment and > > commit > > > message. Is the > > > > 25% > > > > arbitrary or based on the distances > > between the > > > possible > > > > values? Please > > > > make a note in the comment if it's the > > former. > > > > > > > > > > > > > > + if (fxp_min < (1 << > > pn_min) || > > > (255 << > > > > pn_max) < > > > > > fxp_max) { > > > > > > > > > > > > > > > > > > + > > DRM_DEBUG_KMS("VBT defined > > > backlight > > > > frequency > > > > > out of range\n"); > > > > > > + return; > > > > > > + } > > > > > > + > > > > > > + for (pn = pn_max; pn > > > pn_min; > > > pn--) { > > > > > > > > Is there a reason this is not pn >= > > pn_min? > > > > This is bug because f value will be incorrect in > > the case > > > that pn == > > > > pn_min. > > > > Thanks for catching this. > > > > > > > > > > > > > Isn't that a side-effect using the right shift > > operation for > > > division? > > > The bug is just for loop that exit too soon. > > > > > > > Not sure I got that, what's the invalid "f" case that you see > > with > > pn==pn_min? > > > > > > > > if the for loop has pn > pn_min, when pn == pn_min we will exit the > > loop first > > without running the f = clamp(fxp >> pn, 1, 255); in the next line. > > > > > > This would fix with pn >= pn_min in for loop. > > > > Ah! I thought you were giving me a reason to not change it to pn >= > pn_min . > > > > > > We guarantee that the break statement will be called and pn won't be > > pn_min - 1 > > because we check (fxp_min < (1 << pn_min) || (255 << pn_max) < > > fxp_max) before > > entering this for loop. > > > > > > > > > Would DIV_ROUND_CLOSEST allow you to use pn_min? > > > It does not related to the point above, but > > DIV_ROUND_CLOSEST still > > > better than just using right shift. I will change to that in > > next > > > version. > > > > > > > > > > > > > > > > + f = clamp(fxp >> > > 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 the number of brightness control bits > > are > > > changing, the > > > > max. > > > > brightness value changes too. > > > > > > > > Please see intel_dp_aux_setup_backlight(). > > > > > > > > > > > > I think this is fine because > > > > - intel_dp_aux_setup_backlight() will > > > > called intel_dp_aux_enable_backlight() which > > > > called intel_dp_aux_set_pwm_freq() first before > > determine > > > the max > > > > brightness value. > > > > - Also, the panel I tested does not change > > > > BACKLIGHT_BRIGHTNESS_BYTE_COUNT > > > > > > > > when changing the frequency. > > > > > > > > > The only values I see being set for max brightness > > are 0xFFFF > > > and 0xFF > > > > > > if (intel_dp->edp_dpcd[2] & > > > DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) > > > panel->backlight.max = 0xFFFF; > > > else > > > panel->backlight.max = 0xFF; > > > > > > I can't see where you are setting this based on Pn. > > Can you > > > please point > > > out? From what I understand, max should be 2^Pn. > > > > > > > > > It is suppose to be 0xFF or 0xFFFF only. > > > > > > > > > From eDP spec: > > > 0x702 bit 2 BACKLIGHT_BRIGHTNESS_BYTE_COUNT > > > 0 = Indicates that the Sink device supports a 1-byte setting > > for > > > backlight brightness > > > 1 = Indicates that the Sink device supports a 2-byte setting > > for the > > > backlight brightness > > > > > > > > > 0x722 EDP_BACKLIGHT_BRIGHTNESS_MSB > > > The actual number of assigned bits for the backlight > > brightness PWM > > > generator is set by > > > field 4:0 of the EDP_PWMGEN_BIT_COUNT register (DPCD Address > > 00724h). > > > Assigned bits are allocated to the MSB of the enabled > > register > > > combination > > > > > > > ^This makes it somewhat clear but gets confusing again. > > > > > > This means that if PWM_GEN_BIT_COUNT is less than 16 then > > the panel > > > will ignore > > > the least significant bit in EDP_BACKLIGHT_BRIGHTNESS > > register. > > > > > > > > > For example, > > > if BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and > > PWM_GEN_BIT_COUNT = 16 > > > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd > > > In this case, all bits in EDP_BACKLIGHT_BRIGHTNESS will be > > used. > > > > > > 0xabcd means 0xabcd / 0xffff or (43981 / 65535) or 67.111% > > > > > > > > > if BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and > > PWM_GEN_BIT_COUNT = 12 > > > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd > > > > > > In this case, the last 4 bits will be discarded. > > > 0xabcd means 0xabc / 0xfff or 2748 / 4095 or 67.106% > > > > > > > > > I think it should be fine to just have 8 or 16 bits of the > > max > > > brightness and let panel drop > > > the unneed bit to simplify the driver code. > > > > > > > Note: > > The number of assigned or controllable bits will have the > > ability to > > provide full brightness control. Examples: > > • > > For 10-bit operation, 000h = 0% brightness, and 3FFh = 100% > > brightness. > > > > Going by the logic you stated, for a 10-bit control, the MSB > > and LSB > > registers have to be written FFFFh to set 100% brightness with > > the last > > 6 bits dropped off by the hardware. > > > > I don't like the way the spec leaves this to interpretation. > > Have you > > experimented with both 0x0ABC and 0xABCD for the 12-bit, > > BYTE_COUNT==1 > > example you gave? > > > > Yes, already test with the real panel. > > > > > > Here is better test case than 0xABCD > > 0xFFFF with 6, 12, 16 shows very bright screen with same brightness in > > all case. (All 100%) > > 0x0FFF with 6, 12, 16 bits shows dim screen. 6 bit case is slightly > > dimmer than other case. > > Note that the brightness are 4.76% / 6.23% / 6.25% in 6 / 12 / 16 bits > > case. > > > > Great! Thanks for confirming. > > > You can apply this patch for easy testing > > https://patchwork.kernel.org/patch/6241481/ > > > > > > > > > > > > > > > Also, won't this function be called every time > > > _enable_backlight() is > > > called? Isn't doing this from setup sufficient? I > > guess, > I looked at this a bit and look like we need to call this every time _enable() is called. One panel that I have reset the DP_EDP_PWMGEN_BIT_COUNT (pn) to the lowest value every time panel backlight is turned off. So doing this from setup is insufficient. > > > fixing it is an > > > optimization that'd be nice to have but not > > necessary. > > > > > > Lets get this patch set done first. I can send in another > > patch after > > > this is go in to optimize this. > > > > Sure. This patch should be good to go with answers to my > > questions. The > > only thing remaining would be getting an ACK from Jani for > > adding a new > > parameter for dynamic backlight control. > > > > > > -DK > > > > > Probably need to add new member in struct drm_i915_private > > > > > > > > > > > > > > > > > + return; > > > > > > + } > > > > > > + 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"); > > > > > > + return; > > > > > > + } > > > > > > +} > > > > > > + > > > > > > static void > > > intel_dp_aux_enable_backlight(struct > > > > > intel_connector *connector) > > > > > > { > > > > > > struct intel_dp > > *intel_dp = > > > > > > > > enc_to_intel_dp(&connector->encoder->base); > > > > > > uint8_t dpcd_buf = 0; > > > > > > uint8_t new_dpcd_buf = > > 0; > > > > > > uint8_t > > edp_backlight_mode = 0; > > > > > > + bool freq_cap; > > > > > > > > > > > > if > > > (drm_dp_dpcd_readb(&intel_dp->aux, > > > > > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > > > &dpcd_buf) != 1) { > > > > > > @@ -151,6 +225,10 @@ static > > void > > > > > > > intel_dp_aux_enable_backlight(struct > > > intel_connector > > > > > *connector) > > > > > > > > DRM_DEBUG_KMS("Enable > > > dynamic > > > > brightness.\n"); > > > > > > } > > > > > > > > > > > > + 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) { > > > > > > if > > > > (drm_dp_dpcd_writeb(&intel_dp->aux, > > > > > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > > > new_dpcd_buf) < 0) { > > > > > > @@ -158,6 +236,9 @@ static > > void > > > > > > > intel_dp_aux_enable_backlight(struct > > > intel_connector > > > > > *connector) > > > > > > } > > > > > > } > > > > > > > > > > > > + if (freq_cap) > > > > > > + > > > > intel_dp_aux_set_pwm_freq(connector); > > > > > > + > > > > > > > > set_aux_backlight_enable(intel_dp, > > > true); > > > > > > > > > intel_dp_aux_set_backlight(connector, > > > > > > > connector->panel.backlight.level); > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On Tue, 2017-05-16 at 17:39 -0700, Puthikorn Voravootivat wrote: > > > On Tue, May 16, 2017 at 2:21 PM, Pandiyan, Dhinakaran > <dhinakaran.pandiyan@intel.com> wrote: > On Tue, 2017-05-16 at 13:56 -0700, Puthikorn Voravootivat > wrote: > > > > > > On Tue, May 16, 2017 at 1:29 PM, Pandiyan, Dhinakaran > > <dhinakaran.pandiyan@intel.com> wrote: > > On Tue, 2017-05-16 at 11:07 -0700, Puthikorn > Voravootivat > > wrote: > > > > > > > > > On Mon, May 15, 2017 at 11:21 PM, Pandiyan, > Dhinakaran > > > <dhinakaran.pandiyan@intel.com> wrote: > > > On Mon, 2017-05-15 at 17:43 -0700, > Puthikorn > > Voravootivat > > > wrote: > > > > > > > > > > > > On Mon, May 15, 2017 at 4:07 PM, > Pandiyan, > > Dhinakaran > > > > <dhinakaran.pandiyan@intel.com> wrote: > > > > On Fri, 2017-05-12 at 17:31 > -0700, > > Puthikorn > > > Voravootivat > > > > wrote: > > > > > > > > > > > > > > > > > > > > On Fri, May 12, 2017 at 5:12 > PM, > > Pandiyan, > > > Dhinakaran > > > > > > <dhinakaran.pandiyan@intel.com> wrote: > > > > > On Thu, 2017-05-11 at > 16:02 > > -0700, > > > Puthikorn > > > > Voravootivat > > > > > wrote: > > > > > > Read desired PWM > frequency > > from panel > > > vbt and > > > > calculate the > > > > > > value for divider in > DPCD > > address 0x724 > > > and 0x728 > > > > to have > > > > > > as many bits as > possible for > > PWM duty > > > cyle for > > > > granularity > > > > > of > > > > > > brightness > adjustment while > > the > > > frequency is still > > > > within > > > > > 25% > > > > > > of the desired > frequency. > > > > > > > > > > I read a few eDP panel > data > > sheets, the > > > PWM > > > > frequencies all > > > > > start from > > > > > ~200Hz. If the VBT > chooses this > > lowest > > > value to > > > > allow for more > > > > > brightness control, > and then > > this patch > > > lowers the > > > > value by > > > > > another 25%, > > > > > we'll end up below the > panel > > allowed PWM > > > frequency. > > > > > > > > > > In fact, one of the > systems I > > checked had > > > PWM > > > > frequency as > > > > > 200Hz in VBT > > > > > and the panel > datasheet also had > > PWM > > > frequency range > > > > starting > > > > > from > > > > > 200Hz. Have you > considered this > > case? > > > > > > > > > > The spec said "A given LCD > panel > > typically has a > > > limited > > > > range of > > > > > backlight frequency > capability. > > > > > To limit the programmable > frequency > > range, > > > limitations are > > > > placed on > > > > > the allowable total divider > ratio with > > the Sink > > > device" > > > > > So I think it should be auto > cap to > > 200Hz in this > > > case. > > > > > > > > > > -DK > > > > > > > > > > > > Signed-off-by: > Puthikorn > > Voravootivat > > > > <puthik@chromium.org> > > > > > > --- > > > > > > > > > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | > > > > 81 > > > > > > +++++++++++++++++++++++++++ > > > > > > 1 file changed, 81 > > insertions(+) > > > > > > > > > > > > diff --git > > > > > > a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > > > > > > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > > > index > > 0b48851013cc..6f10a2f1ab76 100644 > > > > > > --- > > > > > > a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > > > +++ > > > > > > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > > > > > @@ -113,12 +113,86 > @@ > > > > > > > > > intel_dp_aux_set_dynamic_backlight_percent(struct > > > > intel_dp > > > > > *intel_dp, > > > > > > } > > > > > > } > > > > > > > > > > > > +/* > > > > > > + * Set PWM > Frequency divider > > to match > > > desired > > > > frequency in > > > > > vbt. > > > > > > + * The PWM > Frequency is > > calculated as > > > 27Mhz / (F > > > > x P). > > > > > > + * - Where F = PWM > Frequency > > > Pre-Divider value > > > > programmed > > > > > by field 7:0 of the > > > > > > + * > > EDP_BACKLIGHT_FREQ_SET > > > register > > > > (DPCD > > > > > Address 00728h) > > > > > > + * - Where P = > 2^Pn, where Pn > > is the > > > value > > > > programmed by > > > > > field 4:0 of the > > > > > > + * > > EDP_PWMGEN_BIT_COUNT > > > register > > > > (DPCD Address > > > > > 00724h) > > > > > > + */ > > > > > > +static void > > > 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; > > > > > > + > > > > > > + /* 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; > > > > > > + } > > > > > > + > > > > > > + fxp = > > > 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 > > > > > > + * - Effective > frequency > > is within > > > 25% of > > > > desired > > > > > frequency. > > > > > > + */ > > > > > > + 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; > > > > > > + } > > > > > > + 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; > > > > > > + } > > > > > > + pn_min &= > > > DP_EDP_PWMGEN_BIT_COUNT_MASK; > > > > > > + pn_max &= > > > DP_EDP_PWMGEN_BIT_COUNT_MASK; > > > > > > + > > > > > > + fxp_min = fxp > * 3 / 4; > > > > > > + fxp_max = fxp > * 5 / 4; > > > > > > > > > > > > > > > You are allowing fxp > between +/- > > 25% of > > > the actual. > > > > This isn't > > > > > same as > > > > > the "Effective > frequency is > > within 25% of > > > desired > > > > frequency." > > > > > right? The > > > > > frequency can vary > between -20% > > and +33%. > > > > > > > > > > > > > > > You are right. > > > > > You want me to change commit > message to > > reflect > > > this or > > > > change the > > > > > code to > > > > > match the commit message? > > > > > > > > > > > > I am okay with fixing the > comment and > > commit > > > message. Is the > > > > 25% > > > > arbitrary or based on the > distances > > between the > > > possible > > > > values? Please > > > > make a note in the comment if > it's the > > former. > > > > > > > > > > > > > > + if (fxp_min < > (1 << > > pn_min) || > > > (255 << > > > > pn_max) < > > > > > fxp_max) { > > > > > > > > > > > > > > > > > > + > > DRM_DEBUG_KMS("VBT defined > > > backlight > > > > frequency > > > > > out of range\n"); > > > > > > + > return; > > > > > > + } > > > > > > + > > > > > > + for (pn = > pn_max; pn > > > pn_min; > > > pn--) { > > > > > > > > Is there a reason this is not pn > >= > > pn_min? > > > > This is bug because f value will be > incorrect in > > the case > > > that pn == > > > > pn_min. > > > > Thanks for catching this. > > > > > > > > > > > > > Isn't that a side-effect using the right > shift > > operation for > > > division? > > > The bug is just for loop that exit too soon. > > > > > > > Not sure I got that, what's the invalid "f" case > that you see > > with > > pn==pn_min? > > > > > > > > if the for loop has pn > pn_min, when pn == pn_min we will > exit the > > loop first > > without running the f = clamp(fxp >> pn, 1, 255); in the > next line. > > > > > > This would fix with pn >= pn_min in for loop. > > > > > Ah! I thought you were giving me a reason to not change it to > pn >= > pn_min . > > > > > > We guarantee that the break statement will be called and pn > won't be > > pn_min - 1 > > because we check (fxp_min < (1 << pn_min) || (255 << pn_max) > < > > fxp_max) before > > entering this for loop. > > > > > > > > > Would DIV_ROUND_CLOSEST allow you to use > pn_min? > > > It does not related to the point above, but > > DIV_ROUND_CLOSEST still > > > better than just using right shift. I will change > to that in > > next > > > version. > > > > > > > > > > > > > > > > + f = > clamp(fxp >> > > 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 the number of brightness > control bits > > are > > > changing, the > > > > max. > > > > brightness value changes too. > > > > > > > > Please see > intel_dp_aux_setup_backlight(). > > > > > > > > > > > > I think this is fine because > > > > - intel_dp_aux_setup_backlight() will > > > > called intel_dp_aux_enable_backlight() > which > > > > called intel_dp_aux_set_pwm_freq() first > before > > determine > > > the max > > > > brightness value. > > > > - Also, the panel I tested does not > change > > > > BACKLIGHT_BRIGHTNESS_BYTE_COUNT > > > > > > > > when changing the frequency. > > > > > > > > > The only values I see being set for max > brightness > > are 0xFFFF > > > and 0xFF > > > > > > if (intel_dp->edp_dpcd[2] & > > > DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) > > > panel->backlight.max = 0xFFFF; > > > else > > > panel->backlight.max = 0xFF; > > > > > > I can't see where you are setting this > based on Pn. > > Can you > > > please point > > > out? From what I understand, max should be > 2^Pn. > > > > > > > > > It is suppose to be 0xFF or 0xFFFF only. > > > > > > > > > From eDP spec: > > > 0x702 bit 2 BACKLIGHT_BRIGHTNESS_BYTE_COUNT > > > 0 = Indicates that the Sink device supports a > 1-byte setting > > for > > > backlight brightness > > > 1 = Indicates that the Sink device supports a > 2-byte setting > > for the > > > backlight brightness > > > > > > > > > 0x722 EDP_BACKLIGHT_BRIGHTNESS_MSB > > > The actual number of assigned bits for the > backlight > > brightness PWM > > > generator is set by > > > field 4:0 of the EDP_PWMGEN_BIT_COUNT register > (DPCD Address > > 00724h). > > > Assigned bits are allocated to the MSB of the > enabled > > register > > > combination > > > > > > > ^This makes it somewhat clear but gets confusing > again. > > > > > > This means that if PWM_GEN_BIT_COUNT is less than > 16 then > > the panel > > > will ignore > > > the least significant bit in > EDP_BACKLIGHT_BRIGHTNESS > > register. > > > > > > > > > For example, > > > if BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and > > PWM_GEN_BIT_COUNT = 16 > > > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd > > > In this case, all bits in EDP_BACKLIGHT_BRIGHTNESS > will be > > used. > > > > > > 0xabcd means 0xabcd / 0xffff or (43981 / 65535) or > 67.111% > > > > > > > > > if BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and > > PWM_GEN_BIT_COUNT = 12 > > > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd > > > > > > In this case, the last 4 bits will be discarded. > > > 0xabcd means 0xabc / 0xfff or 2748 / 4095 or > 67.106% > > > > > > > > > I think it should be fine to just have 8 or 16 > bits of the > > max > > > brightness and let panel drop > > > the unneed bit to simplify the driver code. > > > > > > > Note: > > The number of assigned or controllable bits will > have the > > ability to > > provide full brightness control. Examples: > > • > > For 10-bit operation, 000h = 0% brightness, and > 3FFh = 100% > > brightness. > > > > Going by the logic you stated, for a 10-bit control, > the MSB > > and LSB > > registers have to be written FFFFh to set 100% > brightness with > > the last > > 6 bits dropped off by the hardware. > > > > I don't like the way the spec leaves this to > interpretation. > > Have you > > experimented with both 0x0ABC and 0xABCD for the > 12-bit, > > BYTE_COUNT==1 > > example you gave? > > > > Yes, already test with the real panel. > > > > > > Here is better test case than 0xABCD > > 0xFFFF with 6, 12, 16 shows very bright screen with same > brightness in > > all case. (All 100%) > > 0x0FFF with 6, 12, 16 bits shows dim screen. 6 bit case is > slightly > > dimmer than other case. > > Note that the brightness are 4.76% / 6.23% / 6.25% in 6 / > 12 / 16 bits > > case. > > > > > Great! Thanks for confirming. > > > You can apply this patch for easy testing > > https://patchwork.kernel.org/patch/6241481/ > > > > > > > > > > > > > > > Also, won't this function be called every > time > > > _enable_backlight() is > > > called? Isn't doing this from setup > sufficient? I > > guess, > > I looked at this a bit and look like we need to call this every time > _enable() is called. > One panel that I have reset the DP_EDP_PWMGEN_BIT_COUNT (pn) to the > lowest value > every time panel backlight is turned off. So doing this from setup is > insufficient. Thanks for checking. I think, we can still avoid doing these every time a) DPCD reads for PWM_BIT_COUNT_CAP_{MAX,MIN} b) recomputing pn and f -DK > > > > fixing it is an > > > optimization that'd be nice to have but > not > > necessary. > > > > > > Lets get this patch set done first. I can send in > another > > patch after > > > this is go in to optimize this. > > > > Sure. This patch should be good to go with answers > to my > > questions. The > > only thing remaining would be getting an ACK from > Jani for > > adding a new > > parameter for dynamic backlight control. > > > > > > -DK > > > > > Probably need to add new member in struct > drm_i915_private > > > > > > > > > > > > > > > > > + > return; > > > > > > + } > > > > > > + 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"); > > > > > > + > return; > > > > > > + } > > > > > > +} > > > > > > + > > > > > > static void > > > intel_dp_aux_enable_backlight(struct > > > > > intel_connector > *connector) > > > > > > { > > > > > > struct > intel_dp > > *intel_dp = > > > > > > > > > enc_to_intel_dp(&connector->encoder->base); > > > > > > uint8_t > dpcd_buf = 0; > > > > > > uint8_t > new_dpcd_buf = > > 0; > > > > > > uint8_t > > edp_backlight_mode = 0; > > > > > > + bool freq_cap; > > > > > > > > > > > > if > > > (drm_dp_dpcd_readb(&intel_dp->aux, > > > > > > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > > > &dpcd_buf) != 1) { > > > > > > @@ -151,6 +225,10 @@ > static > > void > > > > > > > intel_dp_aux_enable_backlight(struct > > > intel_connector > > > > > *connector) > > > > > > > > DRM_DEBUG_KMS("Enable > > > dynamic > > > > brightness.\n"); > > > > > > } > > > > > > > > > > > > + 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) { > > > > > > if > > > > > (drm_dp_dpcd_writeb(&intel_dp->aux, > > > > > > > > > > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > > > > > new_dpcd_buf) < 0) { > > > > > > @@ -158,6 +236,9 @@ > static > > void > > > > > > > intel_dp_aux_enable_backlight(struct > > > intel_connector > > > > > *connector) > > > > > > } > > > > > > } > > > > > > > > > > > > + if (freq_cap) > > > > > > + > > > > > intel_dp_aux_set_pwm_freq(connector); > > > > > > + > > > > > > > > set_aux_backlight_enable(intel_dp, > > > true); > > > > > > > > > intel_dp_aux_set_backlight(connector, > > > > > > > connector->panel.backlight.level); > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > > Intel-gfx@lists.freedesktop.org > > > > > > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c index 0b48851013cc..6f10a2f1ab76 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -113,12 +113,86 @@ intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp, } } +/* + * Set PWM Frequency divider to match desired frequency in vbt. + * The PWM Frequency is calculated as 27Mhz / (F x P). + * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of the + * EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h) + * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the + * EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h) + */ +static void 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; + + /* 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; + } + + fxp = 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 + * - Effective frequency is within 25% of desired frequency. + */ + 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; + } + 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; + } + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK; + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK; + + fxp_min = fxp * 3 / 4; + fxp_max = 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; + } + + for (pn = pn_max; pn > pn_min; pn--) { + f = clamp(fxp >> 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; + } + 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"); + return; + } +} + static void intel_dp_aux_enable_backlight(struct intel_connector *connector) { struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); uint8_t dpcd_buf = 0; uint8_t new_dpcd_buf = 0; uint8_t edp_backlight_mode = 0; + bool freq_cap; if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) { @@ -151,6 +225,10 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector) DRM_DEBUG_KMS("Enable dynamic brightness.\n"); } + 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) { if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf) < 0) { @@ -158,6 +236,9 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector) } } + if (freq_cap) + intel_dp_aux_set_pwm_freq(connector); + set_aux_backlight_enable(intel_dp, true); intel_dp_aux_set_backlight(connector, connector->panel.backlight.level); }
Read desired PWM frequency from panel vbt and calculate the value for divider in DPCD address 0x724 and 0x728 to have as many bits as possible for PWM duty cyle for granularity of brightness adjustment while the frequency is still within 25% of the desired frequency. Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org> --- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 81 +++++++++++++++++++++++++++ 1 file changed, 81 insertions(+)