All of lore.kernel.org
 help / color / mirror / Atom feed
From: Yakir Yang <ykk@rock-chips.com>
To: Sean Paul <seanpaul@chromium.org>
Cc: "Krzysztof Kozlowski" <k.kozlowski@samsung.com>,
	linux-samsung-soc <linux-samsung-soc@vger.kernel.org>,
	linux-rockchip@lists.infradead.org,
	"Mark Yao" <yzq@rock-chips.com>,
	"Jingoo Han" <jingoohan1@gmail.com>,
	"Emil Velikov" <emil.l.velikov@gmail.com>,
	"Douglas Anderson" <dianders@chromium.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	"Tomasz Figa" <tfiga@chromium.org>,
	"Javier Martinez Canillas" <javier@osg.samsung.com>,
	"Daniel Vetter" <daniel.vetter@ffwll.ch>,
	"Stéphane Marchesin" <marcheu@chromium.org>,
	"Thierry Reding" <treding@nvidia.com>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>
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	[thread overview]
Message-ID: <57917011.6000108@rock-chips.com> (raw)
In-Reply-To: <CAOw6vbK7CnFwH_Tyrq6ab4DBxVLa8-JCYEM-Tu+mOYsKmNULtg@mail.gmail.com>


[-- Attachment #1.1: Type: text/plain, Size: 3878 bytes --]

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

>> +        */
>> +       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
>>
>>
>
>


[-- Attachment #1.2: Type: text/html, Size: 5978 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2016-07-22  1:00 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-07-21 13:14 [PATCH v1 1/2] drm/panel: simple-panel: add the delay timing for Sharp LQ123P1JX31 Yakir Yang
2016-07-21 13:14 ` [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable Yakir Yang
2016-07-21 14:28   ` Sean Paul
2016-07-22  1:00     ` Yakir Yang [this message]
2016-07-22 15:03       ` Sean Paul
2016-07-28  3:28         ` Yakir Yang
2016-07-28 14:06           ` Sean Paul
2016-07-28 14:06             ` Sean Paul
2016-07-29 19:16             ` [PATCH] drm/analogix_dp: Ensure the panel is properly prepared/unprepared Sean Paul
2016-08-01  3:27               ` Yakir Yang
2016-08-01  3:27                 ` Yakir Yang
2016-08-01 20:20                 ` Sean Paul
2016-08-01 20:20                   ` Sean Paul
2016-08-01 20:46                   ` Sean Paul
2016-08-01 20:46                     ` Sean Paul
2016-08-01  3:28               ` Yakir Yang
2016-08-01  3:28                 ` Yakir Yang
2016-08-08 18:53               ` [PATCH v2] drm/bridge: analogix_dp: " Sean Paul
2016-08-09  2:35                 ` Yakir Yang
2016-08-09  2:35                   ` Yakir Yang
2016-08-09  6:05                   ` Daniel Vetter
2016-08-09  6:05                     ` Daniel Vetter
2016-08-17  5:08                   ` Archit Taneja
2016-07-21 14:14 ` [PATCH v1.1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable Yakir Yang
2016-07-21 14:30 ` [PATCH v1 1/2] drm/panel: simple-panel: add the delay timing for Sharp LQ123P1JX31 Sean Paul
2016-07-22  1:00   ` Yakir Yang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=57917011.6000108@rock-chips.com \
    --to=ykk@rock-chips.com \
    --cc=daniel.vetter@ffwll.ch \
    --cc=dianders@chromium.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=emil.l.velikov@gmail.com \
    --cc=javier@osg.samsung.com \
    --cc=jingoohan1@gmail.com \
    --cc=k.kozlowski@samsung.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=marcheu@chromium.org \
    --cc=seanpaul@chromium.org \
    --cc=tfiga@chromium.org \
    --cc=treding@nvidia.com \
    --cc=yzq@rock-chips.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.