<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>