Re: [PATCH v7 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 2017-05-16 at 13:56 -0700, Puthikorn Voravootivat wrote:
> 
> 
> On Tue, May 16, 2017 at 1:29 PM, Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@xxxxxxxxx> 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.pandiyan@xxxxxxxxx> 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.pandiyan@xxxxxxxxx> 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.pandiyan@xxxxxxxxx> 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
>         >         >         <puthik@xxxxxxxxxxxx>
>         >         >         >         > ---
>         >         >         >         >
>         >         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,
>         >         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@xxxxxxxxxxxxxxxxxxxxx
>         >         >         >
>         >
>          https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>         >         >
>         >         >
>         >         >
>         >         > _______________________________________________
>         >         > Intel-gfx mailing list
>         >         > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>         >         >
>         https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>         >
>         >
>         >
>         >
>         > _______________________________________________
>         > Intel-gfx mailing list
>         > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>         > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>         
>         
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux