I noticed that the backlight on the ASUS Chromebook Flip C101 (bob) is
flickering.
We're sending it a high frequency pwm signal, but the EDP panel decided to
"parse" the signal, read the duty cycle, then make its own signal that
it sends to the LEDs.
So even though we send a nice high refresh rate at 1200Hz, the panel backlight
flickers at 200Hz (which is not even divisible by the 60Hz refresh rate).
The fix for that is to enable the EDP_BACKLIGHT_FREQ_PWM_PIN_PASSTHRU bit from
the DPCD EDP registers. This makes the panel actually follow the signal
we're giving it.
This series includes the optional dt binding to enable this fix
(backlight-pwm-passthru) and the corresponding code in the analogix
drm/bridge driver.
Alexandru M Stan (2):
dt-bindings: analogix-dp: Add backlight-pwm-passthru
drm/bridge: analogix: Enable EDP_BACKLIGHT_FREQ_PWM_PIN_PASSTHRU
.../bindings/display/bridge/analogix_dp.txt | 4 ++
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 48 ++++++++++++++++++++++
drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 1 +
3 files changed, 53 insertions(+)
--
2.13.5
Configure the DPCD registers for the backlight to respect the pwm frequency
of the input. We sometimes don't want it to generate its own.
Signed-off-by: Alexandru M Stan <[email protected]>
---
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 48 ++++++++++++++++++++++
drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 1 +
2 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 5c52307146c7..b830403be8eb 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -916,6 +916,46 @@ static irqreturn_t analogix_dp_irq_thread(int irq, void *arg)
return IRQ_HANDLED;
}
+static int analogix_dp_backlight_pwm_passthru(struct analogix_dp_device *dp)
+{
+ u8 value;
+ const u8 expected_cap = DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP |
+ DP_EDP_BACKLIGHT_FREQ_PWM_PIN_PASSTHRU_CAP;
+ int ret = 0;
+
+ ret = drm_dp_dpcd_readb(&dp->aux, DP_EDP_BACKLIGHT_ADJUSTMENT_CAP,
+ &value);
+ if (ret != 1) {
+ DRM_DEV_ERROR(dp->dev,
+ "backlight PWM passthru: Can't read BACKLIGHT_ADJUSTMENT_CAP\n");
+ return ret;
+ }
+
+ if ((value & expected_cap) != expected_cap) {
+ DRM_DEV_ERROR(dp->dev,
+ "panel doesn't support backlight PWM passthru, BACKLIGHT_ADJUSTMENT_CAP=0x%02x\n",
+ value);
+ return -1;
+ }
+
+ ret = drm_dp_dpcd_readb(&dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
+ &value);
+ if (ret != 1) {
+ DRM_DEV_ERROR(dp->dev,
+ "backlight PWM passthru: Can't read BACKLIGHT_MODE_SET_REGISTER\n");
+ return ret;
+ }
+
+ value |= DP_EDP_BACKLIGHT_FREQ_PWM_PIN_PASSTHRU_ENABLE;
+ ret = drm_dp_dpcd_writeb(&dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
+ value);
+ if (ret != 1) {
+ DRM_DEV_ERROR(dp->dev,
+ "backlight PWM passthru: Can't write BACKLIGHT_MODE_SET_REGISTER\n");
+ }
+ return ret;
+}
+
static void analogix_dp_commit(struct analogix_dp_device *dp)
{
int ret;
@@ -954,6 +994,12 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
dp->psr_enable = analogix_dp_detect_sink_psr(dp);
if (dp->psr_enable)
analogix_dp_enable_sink_psr(dp);
+
+ if (dp->backlight_pwm_passthru) {
+ if (analogix_dp_backlight_pwm_passthru(dp) != 1)
+ DRM_DEV_ERROR(dp->dev,
+ "Could not enable backlight pwm pin passthru.\n");
+ }
}
/*
@@ -1424,6 +1470,8 @@ analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
if (IS_ERR(dp->reg_base))
return ERR_CAST(dp->reg_base);
+ dp->backlight_pwm_passthru =
+ of_property_read_bool(dev->of_node, "backlight-pwm-passthru");
dp->force_hpd = of_property_read_bool(dev->of_node, "force-hpd");
dp->hpd_gpio = of_get_named_gpio(dev->of_node, "hpd-gpios", 0);
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index 6a96ef7e6934..aea51413e78e 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -171,6 +171,7 @@ struct analogix_dp_device {
struct phy *phy;
int dpms_mode;
int hpd_gpio;
+ bool backlight_pwm_passthru;
bool force_hpd;
bool psr_enable;
bool fast_train_support;
--
2.13.5
Documentation for the optional backlight-pwm-passthru property.
Tells the EDP panel to folow the input pwm frequency instead
of generating its own.
Signed-off-by: Alexandru M Stan <[email protected]>
---
Documentation/devicetree/bindings/display/bridge/analogix_dp.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
index 0c7473dd0e51..3c15242f6ce3 100644
--- a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
+++ b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
@@ -23,6 +23,10 @@ Required properties for dp-controller:
from general PHY binding: Should be "dp".
Optional properties for dp-controller:
+ -backlight-pwm-passthru:
+ Directly pass the PWM frequency applied to the BL_PWM_DIM
+ pin to the backlight current source. Done via
+ EDP_BACKLIGHT_MODE_SET_REGISTER on DPCD.
-force-hpd:
Indicate driver need force hpd when hpd detect failed, this
is used for some eDP screen which don't have hpd signal.
--
2.13.5
On Thu, Mar 15, 2018 at 07:56:59PM -0700, Alexandru M Stan wrote:
> Configure the DPCD registers for the backlight to respect the pwm frequency
> of the input. We sometimes don't want it to generate its own.
>
> Signed-off-by: Alexandru M Stan <[email protected]>
I wonder a bit whether we should have some generic infrastructure in the
dp helpers itself to apply quirks like this. It's more-or-less what we
have for EDID quirking.
But then I guess for panels a lot of it is specific to the
bridge/encoder/panel combo and given platform, so probably not much use
for reuse across boards/platforms. External displays would be a different
story. I guess you can count that as an
Acked-by: Daniel Vetter <[email protected]>
on the overall thing, but pls get bridge folks to review this too.
-Daniel
> ---
>
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 48 ++++++++++++++++++++++
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 1 +
> 2 files changed, 49 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 5c52307146c7..b830403be8eb 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -916,6 +916,46 @@ static irqreturn_t analogix_dp_irq_thread(int irq, void *arg)
> return IRQ_HANDLED;
> }
>
> +static int analogix_dp_backlight_pwm_passthru(struct analogix_dp_device *dp)
> +{
> + u8 value;
> + const u8 expected_cap = DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP |
> + DP_EDP_BACKLIGHT_FREQ_PWM_PIN_PASSTHRU_CAP;
> + int ret = 0;
> +
> + ret = drm_dp_dpcd_readb(&dp->aux, DP_EDP_BACKLIGHT_ADJUSTMENT_CAP,
> + &value);
> + if (ret != 1) {
> + DRM_DEV_ERROR(dp->dev,
> + "backlight PWM passthru: Can't read BACKLIGHT_ADJUSTMENT_CAP\n");
> + return ret;
> + }
> +
> + if ((value & expected_cap) != expected_cap) {
> + DRM_DEV_ERROR(dp->dev,
> + "panel doesn't support backlight PWM passthru, BACKLIGHT_ADJUSTMENT_CAP=0x%02x\n",
> + value);
> + return -1;
> + }
> +
> + ret = drm_dp_dpcd_readb(&dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> + &value);
> + if (ret != 1) {
> + DRM_DEV_ERROR(dp->dev,
> + "backlight PWM passthru: Can't read BACKLIGHT_MODE_SET_REGISTER\n");
> + return ret;
> + }
> +
> + value |= DP_EDP_BACKLIGHT_FREQ_PWM_PIN_PASSTHRU_ENABLE;
> + ret = drm_dp_dpcd_writeb(&dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> + value);
> + if (ret != 1) {
> + DRM_DEV_ERROR(dp->dev,
> + "backlight PWM passthru: Can't write BACKLIGHT_MODE_SET_REGISTER\n");
> + }
> + return ret;
> +}
> +
> static void analogix_dp_commit(struct analogix_dp_device *dp)
> {
> int ret;
> @@ -954,6 +994,12 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
> dp->psr_enable = analogix_dp_detect_sink_psr(dp);
> if (dp->psr_enable)
> analogix_dp_enable_sink_psr(dp);
> +
> + if (dp->backlight_pwm_passthru) {
> + if (analogix_dp_backlight_pwm_passthru(dp) != 1)
> + DRM_DEV_ERROR(dp->dev,
> + "Could not enable backlight pwm pin passthru.\n");
> + }
> }
>
> /*
> @@ -1424,6 +1470,8 @@ analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
> if (IS_ERR(dp->reg_base))
> return ERR_CAST(dp->reg_base);
>
> + dp->backlight_pwm_passthru =
> + of_property_read_bool(dev->of_node, "backlight-pwm-passthru");
> dp->force_hpd = of_property_read_bool(dev->of_node, "force-hpd");
>
> dp->hpd_gpio = of_get_named_gpio(dev->of_node, "hpd-gpios", 0);
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> index 6a96ef7e6934..aea51413e78e 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> @@ -171,6 +171,7 @@ struct analogix_dp_device {
> struct phy *phy;
> int dpms_mode;
> int hpd_gpio;
> + bool backlight_pwm_passthru;
> bool force_hpd;
> bool psr_enable;
> bool fast_train_support;
> --
> 2.13.5
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Friday 16 March 2018 08:26 AM, Alexandru M Stan wrote:
> Documentation for the optional backlight-pwm-passthru property.
> Tells the EDP panel to folow the input pwm frequency instead
s/folow/follow
It would be nice if we could add the details you mentioned in
patch #0 in either this or the next patch.
> of generating its own.
This is one of those bindings which is more a knob than a HW property,
but I can't think of any easy way to figure this out in SW. So, I guess
it's okay to have.
One thing I was wondering about was whether this prop should belong to
the eDP controller or the eDP panel. I don't have any strong opinion
about it, though.
Reviewed-by: Archit Taneja <[email protected]>
Thanks,
Archit
>
> Signed-off-by: Alexandru M Stan <[email protected]>
> ---
>
> Documentation/devicetree/bindings/display/bridge/analogix_dp.txt | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
> index 0c7473dd0e51..3c15242f6ce3 100644
> --- a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
> @@ -23,6 +23,10 @@ Required properties for dp-controller:
> from general PHY binding: Should be "dp".
>
> Optional properties for dp-controller:
> + -backlight-pwm-passthru:
> + Directly pass the PWM frequency applied to the BL_PWM_DIM
> + pin to the backlight current source. Done via
> + EDP_BACKLIGHT_MODE_SET_REGISTER on DPCD.
> -force-hpd:
> Indicate driver need force hpd when hpd detect failed, this
> is used for some eDP screen which don't have hpd signal.
>
On Thu, Mar 15, 2018 at 07:56:57PM -0700, Alexandru M Stan wrote:
> I noticed that the backlight on the ASUS Chromebook Flip C101 (bob) is
> flickering.
>
> We're sending it a high frequency pwm signal, but the EDP panel decided to
> "parse" the signal, read the duty cycle, then make its own signal that
> it sends to the LEDs.
>
> So even though we send a nice high refresh rate at 1200Hz, the panel backlight
> flickers at 200Hz (which is not even divisible by the 60Hz refresh rate).
>
> The fix for that is to enable the EDP_BACKLIGHT_FREQ_PWM_PIN_PASSTHRU bit from
> the DPCD EDP registers. This makes the panel actually follow the signal
> we're giving it.
>
> This series includes the optional dt binding to enable this fix
> (backlight-pwm-passthru) and the corresponding code in the analogix
> drm/bridge driver.
>
Thanks for sending these patches!
With Archit and Daniel's comments addressed, feel free to add my
Reviewed-by: Sean Paul <[email protected]>
>
> Alexandru M Stan (2):
> dt-bindings: analogix-dp: Add backlight-pwm-passthru
> drm/bridge: analogix: Enable EDP_BACKLIGHT_FREQ_PWM_PIN_PASSTHRU
>
> .../bindings/display/bridge/analogix_dp.txt | 4 ++
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 48 ++++++++++++++++++++++
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 1 +
> 3 files changed, 53 insertions(+)
>
> --
> 2.13.5
>
--
Sean Paul, Software Engineer, Google / Chromium OS
On Fri, Mar 16, 2018 at 02:56:09PM +0530, Archit Taneja wrote:
>
>
> On Friday 16 March 2018 08:26 AM, Alexandru M Stan wrote:
> > Documentation for the optional backlight-pwm-passthru property.
> > Tells the EDP panel to folow the input pwm frequency instead
>
> s/folow/follow
>
> It would be nice if we could add the details you mentioned in
> patch #0 in either this or the next patch.
>
> > of generating its own.
>
> This is one of those bindings which is more a knob than a HW property,
> but I can't think of any easy way to figure this out in SW. So, I guess
> it's okay to have.
>
> One thing I was wondering about was whether this prop should belong to
> the eDP controller or the eDP panel. I don't have any strong opinion
> about it, though.
Seems to me, it should be the panel. It's a setting in the panel, right?
Is this generic to DP panels or something specific to a certain panel?
The naming (vendor prefix or not) and doc location should match
whatever the answer to that is.
>
> Reviewed-by: Archit Taneja <[email protected]>
>
> Thanks,
> Archit
>
>
> >
> > Signed-off-by: Alexandru M Stan <[email protected]>
> > ---
> >
> > Documentation/devicetree/bindings/display/bridge/analogix_dp.txt | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
> > index 0c7473dd0e51..3c15242f6ce3 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
> > +++ b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
> > @@ -23,6 +23,10 @@ Required properties for dp-controller:
> > from general PHY binding: Should be "dp".
> > Optional properties for dp-controller:
> > + -backlight-pwm-passthru:
> > + Directly pass the PWM frequency applied to the BL_PWM_DIM
> > + pin to the backlight current source. Done via
> > + EDP_BACKLIGHT_MODE_SET_REGISTER on DPCD.
> > -force-hpd:
> > Indicate driver need force hpd when hpd detect failed, this
> > is used for some eDP screen which don't have hpd signal.
> >
Hello,
On Sunday, 18 March 2018 14:52:45 EET Rob Herring wrote:
> On Fri, Mar 16, 2018 at 02:56:09PM +0530, Archit Taneja wrote:
> > On Friday 16 March 2018 08:26 AM, Alexandru M Stan wrote:
> > > Documentation for the optional backlight-pwm-passthru property.
> > > Tells the EDP panel to folow the input pwm frequency instead
> >
> > s/folow/follow
> >
> > It would be nice if we could add the details you mentioned in
> > patch #0 in either this or the next patch.
> >
> > > of generating its own.
> >
> > This is one of those bindings which is more a knob than a HW property,
> > but I can't think of any easy way to figure this out in SW. So, I guess
> > it's okay to have.
> >
> > One thing I was wondering about was whether this prop should belong to
> > the eDP controller or the eDP panel. I don't have any strong opinion
> > about it, though.
>
> Seems to me, it should be the panel. It's a setting in the panel, right?
As this aims at fixing an issue with the panel, I agree it should be a panel
property.
Stupid question (I'm not too familiar with eDP panels), couldn't we set
EDP_BACKLIGHT_FREQ_PWM_PIN_PASSTHRU unconditionally ?
> Is this generic to DP panels or something specific to a certain panel?
> The naming (vendor prefix or not) and doc location should match
> whatever the answer to that is.
>
> > Reviewed-by: Archit Taneja <[email protected]>
> >
> > Thanks,
> > Archit
> >
> > > Signed-off-by: Alexandru M Stan <[email protected]>
> > > ---
> > >
> > > Documentation/devicetree/bindings/display/bridge/analogix_dp.txt | 4
> > > ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
> > > b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
> > > index 0c7473dd0e51..3c15242f6ce3 100644
> > > --- a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
> > > +++ b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
> > >
> > > @@ -23,6 +23,10 @@ Required properties for dp-controller:
> > > from general PHY binding: Should be "dp".
> > >
> > > Optional properties for dp-controller:
> > > + -backlight-pwm-passthru:
> > > + Directly pass the PWM frequency applied to the BL_PWM_DIM
> > > + pin to the backlight current source. Done via
> > > + EDP_BACKLIGHT_MODE_SET_REGISTER on DPCD.
> > >
> > > -force-hpd:
> > > Indicate driver need force hpd when hpd detect failed, this
> > > is used for some eDP screen which don't have hpd signal.
--
Regards,
Laurent Pinchart