On Tue, May 16, 2017 at 1:29 PM, Pandiyan, Dhinakaran <dhinakaran.pandiyan@xxxxxxxxx> wrote:
Not sure I got that, what's the invalid "f" case that you see withOn 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.
>
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.
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.
^This makes it somewhat clear but gets confusing again.
> 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 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 Note:
> 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.
>
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.
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@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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
