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