[v1,2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable
diff mbox

Message ID 1469106886-4979-1-git-send-email-ykk@rock-chips.com
State New
Headers show

Commit Message

Yakir Yang July 21, 2016, 1:14 p.m. UTC
Some panels (like Sharp LQ123P1JX31) need to be turn off when eDP
controller stop to send valid video signal, otherwhise panel would
go burn in, and keep flicker and flicker.

So it's better to turn off the panel when eDP need to disable, and
we need to turn on the panel in connector->detect() callback, so
that driver would detect panel status rightly.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

Sean Paul July 21, 2016, 2:28 p.m. UTC | #1
On Thu, Jul 21, 2016 at 9:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> Some panels (like Sharp LQ123P1JX31) need to be turn off when eDP
> controller stop to send valid video signal, otherwhise panel would
> go burn in, and keep flicker and flicker.
>
> So it's better to turn off the panel when eDP need to disable, and
> we need to turn on the panel in connector->detect() callback, so
> that driver would detect panel status rightly.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 32715da..ea059b3 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -961,6 +961,14 @@ analogix_dp_detect(struct drm_connector *connector, bool force)
>  {
>         struct analogix_dp_device *dp = to_dp(connector);
>
> +       /*
> +        * Panle would prepare for several times here, but don't worry it

s/Panle/Panel/

> +        * would only enable the hardware at the first prepare time.


Errr, this shouldn't go in detect. How about putting this in
bridge_enable instead?

> +        */
> +       if (dp->plat_data->panel)
> +               if (drm_panel_prepare(dp->plat_data->panel))

Personally, I don't like doing work in a conditional since you're
throwing the return code away. Could you break this out into:

ret = drm_panel_prepare(dp->plat_data->panel);
if (ret)
  DRM_ERROR("failed to setup the panel ret=%d\n", ret);

> +                       DRM_ERROR("failed to setup the panel\n");
> +
>         if (analogix_dp_detect_hpd(dp))
>                 return connector_status_disconnected;
>
> @@ -1063,7 +1071,8 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>                 return;
>
>         if (dp->plat_data->panel) {
> -               if (drm_panel_disable(dp->plat_data->panel)) {
> +               if (drm_panel_disable(dp->plat_data->panel) ||
> +                   drm_panel_unprepare(dp->plat_data->panel)) {

Same comment here, please break this out into separate statements for
better readability/logging.

>                         DRM_ERROR("failed to disable the panel\n");
>                         return;
>                 }
> @@ -1333,13 +1342,6 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
>
>         phy_power_on(dp->phy);
>
> -       if (dp->plat_data->panel) {
> -               if (drm_panel_prepare(dp->plat_data->panel)) {
> -                       DRM_ERROR("failed to setup the panel\n");
> -                       return -EBUSY;
> -               }
> -       }
> -
>         analogix_dp_init_dp(dp);
>
>         ret = devm_request_threaded_irq(&pdev->dev, dp->irq,
> --
> 1.9.1
>
>
Sean Paul July 22, 2016, 3:03 p.m. UTC | #2
On Thu, Jul 21, 2016 at 9:00 PM, Yakir Yang <ykk@rock-chips.com> wrote:
> Sean,
>
> Thanks for your fast respond :-)
>
> But this patch is not the latest one, I have upgraded this to "v1.1" version
> to fix the eDP can't be disabled problem:
>     [PATCH v1.1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP
> need to disable
>         Changes in v1.1: - unprepare the panel at the end of
> bridge->disable() function
>
> In spite of this, I would take your comments with my "v1.1" patch.
>
> On 07/21/2016 10:28 PM, Sean Paul wrote:
>
> On Thu, Jul 21, 2016 at 9:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>
> Some panels (like Sharp LQ123P1JX31) need to be turn off when eDP
> controller stop to send valid video signal, otherwhise panel would
> go burn in, and keep flicker and flicker.
>
> So it's better to turn off the panel when eDP need to disable, and
> we need to turn on the panel in connector->detect() callback, so
> that driver would detect panel status rightly.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 32715da..ea059b3 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -961,6 +961,14 @@ analogix_dp_detect(struct drm_connector *connector,
> bool force)
>  {
>         struct analogix_dp_device *dp = to_dp(connector);
>
> +       /*
> +        * Panle would prepare for several times here, but don't worry it
>
> s/Panle/Panel/
>
>
> Done
>
> +        * would only enable the hardware at the first prepare time.
>
> Errr, this shouldn't go in detect. How about putting this in
> bridge_enable instead?
>
>
> Nope, if we put this in bridge_enable, then eDP would never be enabled.
> Here're the calling flow.
>
> --> analogix_dp_probe
>   --> analogix_dp_bind  # we still haven't prepared the panel here, that
> means panel have been powered up
>     --> analogix_dp_detect  # Oops, losing panel valid hotplug signal, cause
> panel have been powered up
>     --> ** Keep detecting **


Yeah, after playing around with the patch yesterday I discovered this
for myself. I still don't think detect() should be changing hardware
state. Perhaps you could add a matching unprepare() after you attempt
to detect the panel?

Sean

>
> +        */
> +       if (dp->plat_data->panel)
> +               if (drm_panel_prepare(dp->plat_data->panel))
>
> Personally, I don't like doing work in a conditional since you're
> throwing the return code away. Could you break this out into:
>
> ret = drm_panel_prepare(dp->plat_data->panel);
> if (ret)
>   DRM_ERROR("failed to setup the panel ret=%d\n", ret);
>
>
> Okay
>
>
> +                       DRM_ERROR("failed to setup the panel\n");
> +
>         if (analogix_dp_detect_hpd(dp))
>                 return connector_status_disconnected;
>
> @@ -1063,7 +1071,8 @@ static void analogix_dp_bridge_disable(struct
> drm_bridge *bridge)
>                 return;
>
>         if (dp->plat_data->panel) {
> -               if (drm_panel_disable(dp->plat_data->panel)) {
> +               if (drm_panel_disable(dp->plat_data->panel) ||
> +                   drm_panel_unprepare(dp->plat_data->panel)) {
>
> Same comment here, please break this out into separate statements for
> better readability/logging.
>
>
> Okay,
>
>
> Thanks,
> - Yakir
>
>
>                         DRM_ERROR("failed to disable the panel\n");
>                         return;
>                 }
> @@ -1333,13 +1342,6 @@ int analogix_dp_bind(struct device *dev, struct
> drm_device *drm_dev,
>
>         phy_power_on(dp->phy);
>
> -       if (dp->plat_data->panel) {
> -               if (drm_panel_prepare(dp->plat_data->panel)) {
> -                       DRM_ERROR("failed to setup the panel\n");
> -                       return -EBUSY;
> -               }
> -       }
> -
>         analogix_dp_init_dp(dp);
>
>         ret = devm_request_threaded_irq(&pdev->dev, dp->irq,
> --
> 1.9.1
>
>
>
>
>
Yakir Yang July 28, 2016, 3:28 a.m. UTC | #3
Sean,

Oops, sorry about miss your suggest :(

On 07/22/2016 11:03 PM, Sean Paul wrote:
> On Thu, Jul 21, 2016 at 9:00 PM, Yakir Yang <ykk@rock-chips.com> wrote:
>> Sean,
>>
>> Thanks for your fast respond :-)
>>
>> But this patch is not the latest one, I have upgraded this to "v1.1" version
>> to fix the eDP can't be disabled problem:
>>      [PATCH v1.1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP
>> need to disable
>>          Changes in v1.1: - unprepare the panel at the end of
>> bridge->disable() function
>>
>> In spite of this, I would take your comments with my "v1.1" patch.
>>
>> On 07/21/2016 10:28 PM, Sean Paul wrote:
>>
>> On Thu, Jul 21, 2016 at 9:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>>
>> Some panels (like Sharp LQ123P1JX31) need to be turn off when eDP
>> controller stop to send valid video signal, otherwhise panel would
>> go burn in, and keep flicker and flicker.
>>
>> So it's better to turn off the panel when eDP need to disable, and
>> we need to turn on the panel in connector->detect() callback, so
>> that driver would detect panel status rightly.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> ---
>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++++++++++--------
>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> index 32715da..ea059b3 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> @@ -961,6 +961,14 @@ analogix_dp_detect(struct drm_connector *connector,
>> bool force)
>>   {
>>          struct analogix_dp_device *dp = to_dp(connector);
>>
>> +       /*
>> +        * Panle would prepare for several times here, but don't worry it
>>
>> s/Panle/Panel/
>>
>>
>> Done
>>
>> +        * would only enable the hardware at the first prepare time.
>>
>> Errr, this shouldn't go in detect. How about putting this in
>> bridge_enable instead?
>>
>>
>> Nope, if we put this in bridge_enable, then eDP would never be enabled.
>> Here're the calling flow.
>>
>> --> analogix_dp_probe
>>    --> analogix_dp_bind  # we still haven't prepared the panel here, that
>> means panel have been powered up
>>      --> analogix_dp_detect  # Oops, losing panel valid hotplug signal, cause
>> panel have been powered up
>>      --> ** Keep detecting **
>
> Yeah, after playing around with the patch yesterday I discovered this
> for myself. I still don't think detect() should be changing hardware
> state. Perhaps you could add a matching unprepare() after you attempt
> to detect the panel?

Hmm, I don't understand the meaning of "add a matching unprepare()".

I didn't see there is an unprepare() callback in drm_crtc_helper.h, would
you like to share some simple code :-D

Thanks,
- Yakir

> Sean
>
>> +        */
>> +       if (dp->plat_data->panel)
>> +               if (drm_panel_prepare(dp->plat_data->panel))
>>
>> Personally, I don't like doing work in a conditional since you're
>> throwing the return code away. Could you break this out into:
>>
>> ret = drm_panel_prepare(dp->plat_data->panel);
>> if (ret)
>>    DRM_ERROR("failed to setup the panel ret=%d\n", ret);
>>
>>
>> Okay
>>
>>
>> +                       DRM_ERROR("failed to setup the panel\n");
>> +
>>          if (analogix_dp_detect_hpd(dp))
>>                  return connector_status_disconnected;
>>
>> @@ -1063,7 +1071,8 @@ static void analogix_dp_bridge_disable(struct
>> drm_bridge *bridge)
>>                  return;
>>
>>          if (dp->plat_data->panel) {
>> -               if (drm_panel_disable(dp->plat_data->panel)) {
>> +               if (drm_panel_disable(dp->plat_data->panel) ||
>> +                   drm_panel_unprepare(dp->plat_data->panel)) {
>>
>> Same comment here, please break this out into separate statements for
>> better readability/logging.
>>
>>
>> Okay,
>>
>>
>> Thanks,
>> - Yakir
>>
>>
>>                          DRM_ERROR("failed to disable the panel\n");
>>                          return;
>>                  }
>> @@ -1333,13 +1342,6 @@ int analogix_dp_bind(struct device *dev, struct
>> drm_device *drm_dev,
>>
>>          phy_power_on(dp->phy);
>>
>> -       if (dp->plat_data->panel) {
>> -               if (drm_panel_prepare(dp->plat_data->panel)) {
>> -                       DRM_ERROR("failed to setup the panel\n");
>> -                       return -EBUSY;
>> -               }
>> -       }
>> -
>>          analogix_dp_init_dp(dp);
>>
>>          ret = devm_request_threaded_irq(&pdev->dev, dp->irq,
>> --
>> 1.9.1
>>
>>
>>
>>
>>
>
>
Sean Paul July 28, 2016, 2:06 p.m. UTC | #4
On Wed, Jul 27, 2016 at 11:28 PM, Yakir Yang <ykk@rock-chips.com> wrote:
> Sean,
>
> Oops, sorry about miss your suggest :(
>
>
> On 07/22/2016 11:03 PM, Sean Paul wrote:
>>
>> On Thu, Jul 21, 2016 at 9:00 PM, Yakir Yang <ykk@rock-chips.com> wrote:
>>>
>>> Sean,
>>>
>>> Thanks for your fast respond :-)
>>>
>>> But this patch is not the latest one, I have upgraded this to "v1.1"
>>> version
>>> to fix the eDP can't be disabled problem:
>>>      [PATCH v1.1 2/2] drm/bridge: analogix_dp: turn off the panel when
>>> eDP
>>> need to disable
>>>          Changes in v1.1: - unprepare the panel at the end of
>>> bridge->disable() function
>>>
>>> In spite of this, I would take your comments with my "v1.1" patch.
>>>
>>> On 07/21/2016 10:28 PM, Sean Paul wrote:
>>>
>>> On Thu, Jul 21, 2016 at 9:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>>>
>>> Some panels (like Sharp LQ123P1JX31) need to be turn off when eDP
>>> controller stop to send valid video signal, otherwhise panel would
>>> go burn in, and keep flicker and flicker.
>>>
>>> So it's better to turn off the panel when eDP need to disable, and
>>> we need to turn on the panel in connector->detect() callback, so
>>> that driver would detect panel status rightly.
>>>
>>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>>> ---
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18
>>> ++++++++++--------
>>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> index 32715da..ea059b3 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> @@ -961,6 +961,14 @@ analogix_dp_detect(struct drm_connector *connector,
>>> bool force)
>>>   {
>>>          struct analogix_dp_device *dp = to_dp(connector);
>>>
>>> +       /*
>>> +        * Panle would prepare for several times here, but don't worry it
>>>
>>> s/Panle/Panel/
>>>
>>>
>>> Done
>>>
>>> +        * would only enable the hardware at the first prepare time.
>>>
>>> Errr, this shouldn't go in detect. How about putting this in
>>> bridge_enable instead?
>>>
>>>
>>> Nope, if we put this in bridge_enable, then eDP would never be enabled.
>>> Here're the calling flow.
>>>
>>> --> analogix_dp_probe
>>>    --> analogix_dp_bind  # we still haven't prepared the panel here, that
>>> means panel have been powered up
>>>      --> analogix_dp_detect  # Oops, losing panel valid hotplug signal,
>>> cause
>>> panel have been powered up
>>>      --> ** Keep detecting **
>>
>>
>> Yeah, after playing around with the patch yesterday I discovered this
>> for myself. I still don't think detect() should be changing hardware
>> state. Perhaps you could add a matching unprepare() after you attempt
>> to detect the panel?
>
>
> Hmm, I don't understand the meaning of "add a matching unprepare()".
>
> I didn't see there is an unprepare() callback in drm_crtc_helper.h, would
> you like to share some simple code :-D
>

In other words, make sure the hardware is in the same state at the
beginning and end of detect(). You shouldn't have the panel in a
different state when you exit detect().

Sean



> Thanks,
> - Yakir
>
>
>> Sean
>>
>>> +        */
>>> +       if (dp->plat_data->panel)
>>> +               if (drm_panel_prepare(dp->plat_data->panel))
>>>
>>> Personally, I don't like doing work in a conditional since you're
>>> throwing the return code away. Could you break this out into:
>>>
>>> ret = drm_panel_prepare(dp->plat_data->panel);
>>> if (ret)
>>>    DRM_ERROR("failed to setup the panel ret=%d\n", ret);
>>>
>>>
>>> Okay
>>>
>>>
>>> +                       DRM_ERROR("failed to setup the panel\n");
>>> +
>>>          if (analogix_dp_detect_hpd(dp))
>>>                  return connector_status_disconnected;
>>>
>>> @@ -1063,7 +1071,8 @@ static void analogix_dp_bridge_disable(struct
>>> drm_bridge *bridge)
>>>                  return;
>>>
>>>          if (dp->plat_data->panel) {
>>> -               if (drm_panel_disable(dp->plat_data->panel)) {
>>> +               if (drm_panel_disable(dp->plat_data->panel) ||
>>> +                   drm_panel_unprepare(dp->plat_data->panel)) {
>>>
>>> Same comment here, please break this out into separate statements for
>>> better readability/logging.
>>>
>>>
>>> Okay,
>>>
>>>
>>> Thanks,
>>> - Yakir
>>>
>>>
>>>                          DRM_ERROR("failed to disable the panel\n");
>>>                          return;
>>>                  }
>>> @@ -1333,13 +1342,6 @@ int analogix_dp_bind(struct device *dev, struct
>>> drm_device *drm_dev,
>>>
>>>          phy_power_on(dp->phy);
>>>
>>> -       if (dp->plat_data->panel) {
>>> -               if (drm_panel_prepare(dp->plat_data->panel)) {
>>> -                       DRM_ERROR("failed to setup the panel\n");
>>> -                       return -EBUSY;
>>> -               }
>>> -       }
>>> -
>>>          analogix_dp_init_dp(dp);
>>>
>>>          ret = devm_request_threaded_irq(&pdev->dev, dp->irq,
>>> --
>>> 1.9.1
>>>
>>>
>>>
>>>
>>>
>>
>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 32715da..ea059b3 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -961,6 +961,14 @@  analogix_dp_detect(struct drm_connector *connector, bool force)
 {
 	struct analogix_dp_device *dp = to_dp(connector);
 
+	/*
+	 * Panle would prepare for several times here, but don't worry it
+	 * would only enable the hardware at the first prepare time.
+	 */
+	if (dp->plat_data->panel)
+		if (drm_panel_prepare(dp->plat_data->panel))
+			DRM_ERROR("failed to setup the panel\n");
+
 	if (analogix_dp_detect_hpd(dp))
 		return connector_status_disconnected;
 
@@ -1063,7 +1071,8 @@  static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
 		return;
 
 	if (dp->plat_data->panel) {
-		if (drm_panel_disable(dp->plat_data->panel)) {
+		if (drm_panel_disable(dp->plat_data->panel) ||
+		    drm_panel_unprepare(dp->plat_data->panel)) {
 			DRM_ERROR("failed to disable the panel\n");
 			return;
 		}
@@ -1333,13 +1342,6 @@  int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 
 	phy_power_on(dp->phy);
 
-	if (dp->plat_data->panel) {
-		if (drm_panel_prepare(dp->plat_data->panel)) {
-			DRM_ERROR("failed to setup the panel\n");
-			return -EBUSY;
-		}
-	}
-
 	analogix_dp_init_dp(dp);
 
 	ret = devm_request_threaded_irq(&pdev->dev, dp->irq,