From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yakir Yang Subject: Re: [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable Date: Fri, 22 Jul 2016 09:00:01 +0800 Message-ID: <57917011.6000108@rock-chips.com> References: <1469106874-4912-1-git-send-email-ykk@rock-chips.com> <1469106886-4979-1-git-send-email-ykk@rock-chips.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1366488861==" Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Sean Paul Cc: Krzysztof Kozlowski , linux-samsung-soc , linux-rockchip@lists.infradead.org, Mark Yao , Jingoo Han , Emil Velikov , Douglas Anderson , dri-devel , Tomasz Figa , Javier Martinez Canillas , Daniel Vetter , =?UTF-8?Q?St=c3=a9phane_Marchesin?= , Thierry Reding , Linux Kernel Mailing List List-Id: linux-rockchip.vger.kernel.org This is a multi-part message in MIME format. --===============1366488861== Content-Type: multipart/alternative; boundary="------------030503050201010402000606" This is a multi-part message in MIME format. --------------030503050201010402000606 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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 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 >> --- >> 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 ** >> + */ >> + 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 >> >> > > --------------030503050201010402000606 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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:
=C2=A0=C2=A0=C2=A0 [PATCH v1.1 2/2] drm/bridge: analogix_dp: turn off= the panel when eDP need to disable
=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 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.=C2= =A0

On 07/21/2016 10:28 PM, Sean Paul wrote:
On Thu, Jul 21, 2016 at 9:14 AM, Yakir Yang <ykk@roc=
k-chips.com> wrote:

Some panels (like Sharp LQ123P1JX31) need to be tu=
rn 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 =3D 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 f=
irst 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

    =C2=A0 --> analogix_dp_bind=C2=A0 # we still haven't prepared the =
panel
    here, that means panel have been powered up

    =C2=A0=C2=A0=C2=A0 --> analogix_dp_detect=C2=A0 # Oops, losing pan=
el valid hotplug
    signal, cause panel have been powered up

    =C2=A0=C2=A0=C2=A0 --> ** Keep detecting **

    

    

      

      

        
+        */
+       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 =3D drm_panel_prepare(dp->plat_data->panel);
if (ret)
  DRM_ERROR("failed to setup the panel ret=3D%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_b=
ridge *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 disab=
le the panel\n");
                        return;
                }
@@ -1333,13 +1342,6 @@ int analogix_dp_bind(struct device *dev, struct dr=
m_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 =3D devm_request_threaded_irq(&pdev->dev, dp->irq,
--
1.9.1




      

      




    

    

  


--------------030503050201010402000606--


--===============1366488861==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline

X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs
IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz
dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg==

--===============1366488861==--