linux-kernel - Re: [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOw6vbK7CnFwH_Tyrq6ab4DBxVLa8-JCYEM-Tu+mOYsKmNULtg@mail.gmail.com>
Date:	Thu, 21 Jul 2016 10:28:06 -0400
From:	Sean Paul <seanpaul@...omium.org>
To:	Yakir Yang <ykk@...k-chips.com>
Cc:	Mark Yao <yzq@...k-chips.com>, Inki Dae <inki.dae@...sung.com>,
	Thierry Reding <treding@...dia.com>,
	Heiko Stuebner <heiko@...ech.de>,
	Jingoo Han <jingoohan1@...il.com>,
	Javier Martinez Canillas <javier@....samsung.com>,
	Stéphane Marchesin <marcheu@...omium.org>,
	Tomasz Figa <tfiga@...omium.org>,
	Douglas Anderson <dianders@...omium.org>,
	David Airlie <airlied@...ux.ie>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Emil Velikov <emil.l.velikov@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	dri-devel <dri-devel@...ts.freedesktop.org>,
	linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
	linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when
 eDP need to disable

On Thu, Jul 21, 2016 at 9:14 AM, Yakir Yang <ykk@...k-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@...k-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
>
>