<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, May 15, 2017 at 11:21 PM, Pandiyan, Dhinakaran <span dir="ltr"><<a href="mailto:dhinakaran.pandiyan@intel.com" target="_blank" class="gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed cremed">dhinakaran.pandiyan@intel.com</a>></span> wrote:<br><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 Mon, 2017-05-15 at 17:43 -0700, Puthikorn Voravootivat wrote:<br>
><br>
><br>
> On Mon, May 15, 2017 at 4:07 PM, Pandiyan, Dhinakaran<br>
> <<a href="mailto:dhinakaran.pandiyan@intel.com" class="gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed cremed">dhinakaran.pandiyan@intel.com</a><wbr>> wrote:<br>
> On Fri, 2017-05-12 at 17:31 -0700, Puthikorn Voravootivat<br>
> wrote:<br>
> ><br>
> ><br>
> ><br>
> > On Fri, May 12, 2017 at 5:12 PM, Pandiyan, Dhinakaran<br>
> > <<a href="mailto:dhinakaran.pandiyan@intel.com" class="gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed cremed">dhinakaran.pandiyan@intel.com</a><wbr>> wrote:<br>
> > On Thu, 2017-05-11 at 16:02 -0700, Puthikorn<br>
> Voravootivat<br>
> > wrote:<br>
> > > Read desired PWM frequency from panel vbt and<br>
> calculate the<br>
> > > value for divider in DPCD address 0x724 and 0x728<br>
> to have<br>
> > > as many bits as possible for PWM duty cyle for<br>
> granularity<br>
> > of<br>
> > > brightness adjustment while the frequency is still<br>
> within<br>
> > 25%<br>
> > > of the desired frequency.<br>
> ><br>
> > I read a few eDP panel data sheets, the PWM<br>
> frequencies all<br>
> > start from<br>
> > ~200Hz. If the VBT chooses this lowest value to<br>
> allow for more<br>
> > brightness control, and then this patch lowers the<br>
> value by<br>
> > another 25%,<br>
> > we'll end up below the panel allowed PWM frequency.<br>
> ><br>
> > In fact, one of the systems I checked had PWM<br>
> frequency as<br>
> > 200Hz in VBT<br>
> > and the panel datasheet also had PWM frequency range<br>
> starting<br>
> > from<br>
> > 200Hz. Have you considered this case?<br>
> ><br>
> > The spec said "A given LCD panel typically has a limited<br>
> range of<br>
> > backlight frequency capability.<br>
> > To limit the programmable frequency range, limitations are<br>
> placed on<br>
> > the allowable total divider ratio with the Sink device"<br>
> > So I think it should be auto cap to 200Hz in this case.<br>
> ><br>
> > -DK<br>
> > ><br>
> > > Signed-off-by: Puthikorn Voravootivat<br>
> <<a href="mailto:puthik@chromium.org" class="gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed cremed">puthik@chromium.org</a>><br>
> > > ---<br>
> > > drivers/gpu/drm/i915/intel_dp_<wbr>aux_backlight.c |<br>
> 81<br>
> > +++++++++++++++++++++++++++<br>
> > > 1 file changed, 81 insertions(+)<br>
> > ><br>
> > > diff --git<br>
> a/drivers/gpu/drm/i915/intel_<wbr>dp_aux_backlight.c<br>
> > b/drivers/gpu/drm/i915/intel_<wbr>dp_aux_backlight.c<br>
> > > index 0b48851013cc..6f10a2f1ab76 100644<br>
> > > ---<br>
> a/drivers/gpu/drm/i915/intel_<wbr>dp_aux_backlight.c<br>
> > > +++<br>
> b/drivers/gpu/drm/i915/intel_<wbr>dp_aux_backlight.c<br>
> > > @@ -113,12 +113,86 @@<br>
> > intel_dp_aux_set_dynamic_<wbr>backlight_percent(struct<br>
> intel_dp<br>
> > *intel_dp,<br>
> > > }<br>
> > > }<br>
> > ><br>
> > > +/*<br>
> > > + * Set PWM Frequency divider to match desired<br>
> frequency in<br>
> > vbt.<br>
> > > + * The PWM Frequency is calculated as 27Mhz / (F<br>
> x P).<br>
> > > + * - Where F = PWM Frequency Pre-Divider value<br>
> programmed<br>
> > by field 7:0 of the<br>
> > > + * EDP_BACKLIGHT_FREQ_SET register<br>
> (DPCD<br>
> > Address 00728h)<br>
> > > + * - Where P = 2^Pn, where Pn is the value<br>
> programmed by<br>
> > field 4:0 of the<br>
> > > + * EDP_PWMGEN_BIT_COUNT register<br>
> (DPCD Address<br>
> > 00724h)<br>
> > > + */<br>
> > > +static void intel_dp_aux_set_pwm_freq(<wbr>struct<br>
> > intel_connector *connector)<br>
> > > +{<br>
> > > + struct drm_i915_private *dev_priv =<br>
> > to_i915(connector->base.dev);<br>
> > > + struct intel_dp *intel_dp =<br>
> > enc_to_intel_dp(&connector-><wbr>encoder->base);<br>
> > > + int freq, fxp, fxp_min, fxp_max, fxp_actual,<br>
> f = 1;<br>
> > > + u8 pn, pn_min, pn_max;<br>
> > > +<br>
> > > + /* Find desired value of (F x P)<br>
> > > + * Note that, if F x P is out of supported<br>
> range, the<br>
> > maximum value or<br>
> > > + * minimum value will applied automatically.<br>
> So no<br>
> > need to check that.<br>
> > > + */<br>
> > > + freq = dev_priv->vbt.backlight.pwm_<wbr>freq_hz;<br>
> > > + DRM_DEBUG_KMS("VBT defined backlight<br>
> frequency %u Hz<br>
> > \n", freq);<br>
> > > + if (!freq) {<br>
> > > + DRM_DEBUG_KMS("Use panel default<br>
> backlight<br>
> > frequency\n");<br>
> > > + return;<br>
> > > + }<br>
> > > +<br>
> > > + fxp = KHz(DP_EDP_BACKLIGHT_FREQ_<wbr>BASE_KHZ) /<br>
> freq;<br>
> > > +<br>
> > > + /* Use highest possible value of Pn for more<br>
> > granularity of brightness<br>
> > > + * adjustment while satifying the conditions<br>
> below.<br>
> > > + * - Pn is in the range of Pn_min and Pn_max<br>
> > > + * - F is in the range of 1 and 255<br>
> > > + * - Effective frequency is within 25% of<br>
> desired<br>
> > frequency.<br>
> > > + */<br>
> > > + if (drm_dp_dpcd_readb(&intel_dp-><wbr>aux,<br>
> > > +<br>
> > DP_EDP_PWMGEN_BIT_COUNT_CAP_<wbr>MIN, &pn_min) != 1) {<br>
> > > + DRM_DEBUG_KMS("Failed to read pwmgen<br>
> bit count<br>
> > cap min\n");<br>
> > > + return;<br>
> > > + }<br>
> > > + if (drm_dp_dpcd_readb(&intel_dp-><wbr>aux,<br>
> > > +<br>
> > DP_EDP_PWMGEN_BIT_COUNT_CAP_<wbr>MAX, &pn_max) != 1) {<br>
> > > + DRM_DEBUG_KMS("Failed to read pwmgen<br>
> bit count<br>
> > cap max\n");<br>
> > > + return;<br>
> > > + }<br>
> > > + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;<br>
> > > + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;<br>
> > > +<br>
> > > + fxp_min = fxp * 3 / 4;<br>
> > > + fxp_max = fxp * 5 / 4;<br>
> ><br>
> ><br>
> > You are allowing fxp between +/- 25% of the actual.<br>
> This isn't<br>
> > same as<br>
> > the "Effective frequency is within 25% of desired<br>
> frequency."<br>
> > right? The<br>
> > frequency can vary between -20% and +33%.<br>
> ><br>
> ><br>
> > You are right.<br>
> > You want me to change commit message to reflect this or<br>
> change the<br>
> > code to<br>
> > match the commit message?<br>
><br>
><br>
> I am okay with fixing the comment and commit message. Is the<br>
> 25%<br>
> arbitrary or based on the distances between the possible<br>
> values? Please<br>
> make a note in the comment if it's the former.<br>
><br>
><br>
> > > + if (fxp_min < (1 << pn_min) || (255 <<<br>
> pn_max) <<br>
> > fxp_max) {<br>
><br>
><br>
><br>
> > > + DRM_DEBUG_KMS("VBT defined backlight<br>
> frequency<br>
> > out of range\n");<br>
> > > + return;<br>
> > > + }<br>
> > > +<br>
> > > + for (pn = pn_max; pn > pn_min; pn--) {<br>
><br>
> Is there a reason this is not pn >= pn_min?<br>
> This is bug because f value will be incorrect in the case that pn ==<br>
> pn_min.<br>
> Thanks for catching this.<br>
><br>
<br>
</div></div>Isn't that a side-effect using the right shift operation for division?<br></blockquote><div>The bug is just for loop that exit too soon. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Would DIV_ROUND_CLOSEST allow you to use pn_min?<br></blockquote><div>It does not related to the point above, but DIV_ROUND_CLOSEST still</div><div>better than just using right shift. I will change to that in next version.</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><div class="gmail-h5"><br>
><br>
> > > + f = clamp(fxp >> pn, 1, 255);<br>
> > > + fxp_actual = f << pn;<br>
> > > + if (fxp_min <= fxp_actual &&<br>
> fxp_actual <=<br>
> > fxp_max)<br>
> > > + break;<br>
> > > + }<br>
> > > +<br>
> > > + if (drm_dp_dpcd_writeb(&intel_dp-<wbr>>aux,<br>
> > > +<br>
> DP_EDP_PWMGEN_BIT_COUNT, pn) <<br>
> > 0) {<br>
> > > + DRM_DEBUG_KMS("Failed to write aux<br>
> pwmgen bit<br>
> > count\n");<br>
><br>
><br>
> If the number of brightness control bits are changing, the<br>
> max.<br>
> brightness value changes too.<br>
><br>
> Please see intel_dp_aux_setup_backlight()<wbr>.<br>
><br>
><br>
> I think this is fine because<br>
> - intel_dp_aux_setup_backlight() will<br>
> called intel_dp_aux_enable_backlight(<wbr>) which<br>
> called intel_dp_aux_set_pwm_freq() first before determine the max<br>
> brightness value.<br>
> - Also, the panel I tested does not change<br>
> BACKLIGHT_BRIGHTNESS_BYTE_<wbr>COUNT<br>
><br>
> when changing the frequency.<br>
<br>
</div></div>The only values I see being set for max brightness are 0xFFFF and 0xFF<br>
<br>
if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_<wbr>BYTE_COUNT)<br>
panel->backlight.max = 0xFFFF;<br>
else<br>
panel->backlight.max = 0xFF;</blockquote><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I can't see where you are setting this based on Pn. Can you please point<br>
out? From what I understand, max should be 2^Pn.<br>
<br></blockquote><div><div><br class="gmail-Apple-interchange-newline">It is suppose to be 0xFF or 0xFFFF only.</div><div><br></div><div>From eDP spec:</div><div><div>0x702 bit 2 BACKLIGHT_BRIGHTNESS_BYTE_COUNT</div><div>0 = Indicates that the Sink device supports a 1-byte setting for backlight brightness</div><div>1 = Indicates that the Sink device supports a 2-byte setting for the backlight brightness</div></div></div><div><br></div><div>0x722 EDP_BACKLIGHT_BRIGHTNESS_MSB</div><div>The actual number of assigned bits for the backlight brightness PWM generator
is set by</div><div>field 4:0 of the EDP_PWMGEN_BIT_COUNT register (DPCD
Address 00724h).</div><div>Assigned bits are allocated to the MSB of the enabled register combination<br></div><div><br></div><div>This means that if PWM_GEN_BIT_COUNT is less than 16 then the panel will ignore</div><div>the least significant bit in EDP_BACKLIGHT_BRIGHTNESS register.</div><div><br></div><div>For example, </div><div>if BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and PWM_GEN_BIT_COUNT = 16</div><div>and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd</div><div>In this case, all bits in EDP_BACKLIGHT_BRIGHTNESS will be used.<br></div><div>0xabcd means 0xabcd / 0xffff or (43981 / 65535) or 67.111%</div><div><br></div>if BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and PWM_GEN_BIT_COUNT = 12</div><div class="gmail_quote">and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd<br></div><div class="gmail_quote">In this case, the last 4 bits will be discarded.<br><div>0xabcd means 0xabc / 0xfff or 2748 / 4095 or 67.106%</div><div><br></div><div>I think it should be fine to just have 8 or 16 bits of the max brightness and let panel drop</div><div>the unneed bit to simplify the driver code.</div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Also, won't this function be called every time _enable_backlight() is<br>
called? Isn't doing this from setup sufficient? I guess, fixing it is an<br>
optimization that'd be nice to have but not necessary.<br></blockquote><div> </div><div>Lets get this patch set done first. I can send in another patch after this is go in to optimize this.</div><div>Probably need to add new member in struct drm_i915_private</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">><br>
><br>
> > > + return;<br>
> > > + }<br>
> > > + if (drm_dp_dpcd_writeb(&intel_dp-<wbr>>aux,<br>
> > > +<br>
> DP_EDP_BACKLIGHT_FREQ_SET, (u8)<br>
> > f) < 0) {<br>
> > > + DRM_DEBUG_KMS("Failed to write aux<br>
> backlight<br>
> > freq\n");<br>
> > > + return;<br>
> > > + }<br>
> > > +}<br>
> > > +<br>
> > > static void intel_dp_aux_enable_backlight(<wbr>struct<br>
> > intel_connector *connector)<br>
> > > {<br>
> > > struct intel_dp *intel_dp =<br>
> > enc_to_intel_dp(&connector-><wbr>encoder->base);<br>
> > > uint8_t dpcd_buf = 0;<br>
> > > uint8_t new_dpcd_buf = 0;<br>
> > > uint8_t edp_backlight_mode = 0;<br>
> > > + bool freq_cap;<br>
> > ><br>
> > > if (drm_dp_dpcd_readb(&intel_dp-><wbr>aux,<br>
> > ><br>
> DP_EDP_BACKLIGHT_MODE_SET_<wbr>REGISTER,<br>
> > &dpcd_buf) != 1) {<br>
> > > @@ -151,6 +225,10 @@ static void<br>
> > intel_dp_aux_enable_backlight(<wbr>struct intel_connector<br>
> > *connector)<br>
> > > DRM_DEBUG_KMS("Enable dynamic<br>
> brightness.\n");<br>
> > > }<br>
> > ><br>
> > > + freq_cap = intel_dp->edp_dpcd[2] &<br>
> > DP_EDP_BACKLIGHT_FREQ_AUX_SET_<wbr>CAP;<br>
> > > + if (freq_cap)<br>
> > > + new_dpcd_buf |=<br>
> > DP_EDP_BACKLIGHT_FREQ_AUX_SET_<wbr>ENABLE;<br>
> > > +<br>
> > > if (new_dpcd_buf != dpcd_buf) {<br>
> > > if<br>
> (drm_dp_dpcd_writeb(&intel_dp-<wbr>>aux,<br>
> > ><br>
> DP_EDP_BACKLIGHT_MODE_SET_<wbr>REGISTER,<br>
> > new_dpcd_buf) < 0) {<br>
> > > @@ -158,6 +236,9 @@ static void<br>
> > intel_dp_aux_enable_backlight(<wbr>struct intel_connector<br>
> > *connector)<br>
> > > }<br>
> > > }<br>
> > ><br>
> > > + if (freq_cap)<br>
> > > +<br>
> intel_dp_aux_set_pwm_freq(<wbr>connector);<br>
> > > +<br>
> > > set_aux_backlight_enable(<wbr>intel_dp, true);<br>
> > > intel_dp_aux_set_backlight(<wbr>connector,<br>
> > connector->panel.backlight.<wbr>level);<br>
> > > }<br>
> ><br>
> ><br>
> ><br>
> ><br>
><br>
> > ______________________________<wbr>_________________<br>
> > Intel-gfx mailing list<br>
> > <a href="mailto:Intel-gfx@lists.freedesktop.org" class="gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed cremed">Intel-gfx@lists.freedesktop.<wbr>org</a><br>
> > <a href="https://lists.freedesktop.org/mailman/listinfo/intel-gfx" rel="noreferrer" target="_blank" class="gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed cremed">https://lists.freedesktop.org/<wbr>mailman/listinfo/intel-gfx</a><br>
><br>
><br>
><br>
> ______________________________<wbr>_________________<br>
> Intel-gfx mailing list<br>
> <a href="mailto:Intel-gfx@lists.freedesktop.org" class="gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed cremed">Intel-gfx@lists.freedesktop.<wbr>org</a><br>
> <a href="https://lists.freedesktop.org/mailman/listinfo/intel-gfx" rel="noreferrer" target="_blank" class="gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed gmail-cremed cremed">https://lists.freedesktop.org/<wbr>mailman/listinfo/intel-gfx</a><br>
<br>
</div></div></blockquote></div><br></div></div>