On Mon, May 15, 2017 at 11:21 PM, Pandiyan, Dhinakaran < dhinakaran.pandi...@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.pandi...@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.pandi...@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 > > <put...@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 > >
_______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx