[PATCH 0/2] Add backlight-pwm-passthru in analogix DP driver

[PATCH 0/2] Add backlight-pwm-passthru in analogix DP driver

[Alexandru M Stan]

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

[PATCH 1/2] dt-bindings: analogix-dp: Add backlight-pwm-passthru

[Alexandru M Stan]

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 <amstan@chromium.org>
---

 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

Re: [PATCH 1/2] dt-bindings: analogix-dp: Add backlight-pwm-passthru

[Archit Taneja]

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 <architt@codeaurora.org>

Thanks,
Archit


> 
> Signed-off-by: Alexandru M Stan <amstan@chromium.org>
> ---
> 
>   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.
> 

Re: [PATCH 1/2] dt-bindings: analogix-dp: Add backlight-pwm-passthru

[Rob Herring]

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 <architt@codeaurora.org>
> 
> Thanks,
> Archit
> 
> 
> > 
> > Signed-off-by: Alexandru M Stan <amstan@chromium.org>
> > ---
> > 
> >   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.
> > 

Re: [PATCH 1/2] dt-bindings: analogix-dp: Add backlight-pwm-passthru

[Laurent Pinchart]

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 <architt@codeaurora.org>
> > 
> > Thanks,
> > Archit
> > 
> > > Signed-off-by: Alexandru M Stan <amstan@chromium.org>
> > > ---
> > > 
> > >   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

[PATCH 2/2] drm/bridge: analogix: Enable EDP_BACKLIGHT_FREQ_PWM_PIN_PASSTHRU

[Alexandru M Stan]

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 <amstan@chromium.org>
---

 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

Re: [PATCH 2/2] drm/bridge: analogix: Enable EDP_BACKLIGHT_FREQ_PWM_PIN_PASSTHRU

[Daniel Vetter]

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 <amstan@chromium.org>

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 <daniel.vetter@ffwll.ch>

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

Re: [PATCH 0/2] Add backlight-pwm-passthru in analogix DP driver

[Sean Paul]

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 <seanpaul@chromium.org>

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