<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Tue, May 16, 2017 at 2:21 PM, Pandiyan, Dhinakaran <<a href="mailto:dhinakaran.pandiyan@intel.com" target="_blank" class="gmail-cremed cremed">dhinakaran.pandiyan@intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">On Tue, 2017-05-16 at 13:56 -0700, Puthikorn Voravootivat wrote: > > > On Tue, May 16, 2017 at 1:29 PM, Pandiyan, Dhinakaran > <<a href="mailto:dhinakaran.pandiyan@intel.com" class="gmail-cremed cremed">dhinakaran.pandiyan@intel.com</a>> wrote: > On Tue, 2017-05-16 at 11:07 -0700, Puthikorn Voravootivat > wrote: > > > > > > On Mon, May 15, 2017 at 11:21 PM, Pandiyan, Dhinakaran > > <<a href="mailto:dhinakaran.pandiyan@intel.com" class="gmail-cremed cremed">dhinakaran.pandiyan@intel.com</a>> wrote: > > On Mon, 2017-05-15 at 17:43 -0700, Puthikorn > Voravootivat > > wrote: > > > > > > > > > On Mon, May 15, 2017 at 4:07 PM, Pandiyan, > Dhinakaran > > > <<a href="mailto:dhinakaran.pandiyan@intel.com" class="gmail-cremed cremed">dhinakaran.pandiyan@intel.com</a>> wrote: > > > On Fri, 2017-05-12 at 17:31 -0700, > Puthikorn > > Voravootivat > > > wrote: > > > > > > > > > > > > > > > > On Fri, May 12, 2017 at 5:12 PM, > Pandiyan, > > Dhinakaran > > > > <<a href="mailto:dhinakaran.pandiyan@intel.com" class="gmail-cremed cremed">dhinakaran.pandiyan@intel.com</a>> 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 > > > <<a href="mailto:puthik@chromium.org" class="gmail-cremed cremed">puthik@chromium.org</a>> > > > > > --- > > > > > > > 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. > </div></div>Ah! I thought you were giving me a reason to not change it to pn >= pn_min . <div><div class="gmail-h5"> > > 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. > </div></div>Great! Thanks for confirming. <div class="gmail-HOEnZb"><div class="gmail-h5"> > You can apply this patch for easy testing > <a href="https://patchwork.kernel.org/patch/6241481/" rel="noreferrer" target="_blank" class="gmail-cremed cremed">https://patchwork.kernel.org/patch/6241481/</a> > > > > > > > > > Also, won't this function be called every time > > _enable_backlight() is > > called? Isn't doing this from setup sufficient? I > guess, </div></div></blockquote><div>I looked at this a bit and look like we need to call this every time _enable() is called.</div><div>One panel that I have reset the DP_EDP_PWMGEN_BIT_COUNT (pn) to the lowest value</div><div>every time panel backlight is turned off. So doing this from setup is insufficient.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5"> > > 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 > > > > <a href="mailto:Intel-gfx@lists.freedesktop.org" class="gmail-cremed cremed">Intel-gfx@lists.freedesktop.org</a> > > > > > > > <a href="https://lists.freedesktop.org/mailman/listinfo/intel-gfx" rel="noreferrer" target="_blank" class="gmail-cremed cremed">https://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> > > > > > > > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > <a href="mailto:Intel-gfx@lists.freedesktop.org" class="gmail-cremed cremed">Intel-gfx@lists.freedesktop.org</a> > > > > <a href="https://lists.freedesktop.org/mailman/listinfo/intel-gfx" rel="noreferrer" target="_blank" class="gmail-cremed cremed">https://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> > > > > > > > > > > _______________________________________________ > > Intel-gfx mailing list > > <a href="mailto:Intel-gfx@lists.freedesktop.org" class="gmail-cremed cremed">Intel-gfx@lists.freedesktop.org</a> > > <a href="https://lists.freedesktop.org/mailman/listinfo/intel-gfx" rel="noreferrer" target="_blank" class="gmail-cremed cremed">https://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> > > > > > _______________________________________________ > Intel-gfx mailing list > <a href="mailto:Intel-gfx@lists.freedesktop.org" class="gmail-cremed cremed">Intel-gfx@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/intel-gfx" rel="noreferrer" target="_blank" class="gmail-cremed cremed">https://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> </div></div></blockquote></div> </div></div>