On Thu, Jul 21, 2016 at 9:00 PM, Yakir Yang <ykk at 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 at 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 at 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
>
>
>
>
>
