| 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
| ||
|
Message-ID: <579EC1CF.9000302@rock-chips.com>
Date: Mon, 1 Aug 2016 11:28:15 +0800
From: Yakir Yang <ykk@...k-chips.com>
To: Sean Paul <seanpaul@...omium.org>
Cc: yzq@...k-chips.com, inki.dae@...sung.com, treding@...dia.com,
heiko@...ech.de, jingoohan1@...il.com, javier@....samsung.com,
marcheu@...omium.org, tfiga@...omium.org, dianders@...omium.org,
airlied@...ux.ie, daniel.vetter@...ll.ch, k.kozlowski@...sung.com,
emil.l.velikov@...il.com, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH] drm/analogix_dp: Ensure the panel is properly
prepared/unprepared
Sean,
On 07/30/2016 03:16 AM, Sean Paul wrote:
> Instead of just preparing the panel on bind, actually prepare/unprepare
> during modeset/disable. The panel must be prepared in order to read hpd
> status, so we need to refcount the prepares in order to ensure we don't
> accidentally turn the panel off at the wrong time.
>
> Signed-off-by: Sean Paul <seanpaul@...omium.org>
> ---
>
>
> Hi Yakir,
> This is what I was talking about upthread. I've tested it and it seems to be working.
>
> What do you think?
Thanks for your patch, and it works. But I have introduced two
questions, and I haven't found a way to fixed them.
> Sean
>
>
>
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 48 +++++++++++++++++-----
> 1 file changed, 37 insertions(+), 11 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..7b764a4 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -960,11 +960,27 @@ enum drm_connector_status
> analogix_dp_detect(struct drm_connector *connector, bool force)
> {
> struct analogix_dp_device *dp = to_dp(connector);
> + enum drm_connector_status status = connector_status_disconnected;
> + int ret;
>
> - if (analogix_dp_detect_hpd(dp))
> - return connector_status_disconnected;
> + if (dp->plat_data->panel && dp->dpms_mode != DRM_MODE_DPMS_ON) {
> + ret = drm_panel_prepare(dp->plat_data->panel);
> + if (ret) {
> + DRM_ERROR("failed to setup panel (%d)\n", ret);
> + return connector_status_disconnected;
> + }
> + }
> +
> + if (!analogix_dp_detect_hpd(dp))
> + status = connector_status_connected;
> +
> + if (dp->plat_data->panel && dp->dpms_mode != DRM_MODE_DPMS_ON) {
> + ret = drm_panel_unprepare(dp->plat_data->panel);
> + if (ret)
> + DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> + }
>
> - return connector_status_connected;
> + return status;
1. Panel would flicker at system boot time. Your patch would flash the
panel power in connector->detect() function when dp->dpms_mode isn't
DRM_MODE_DPMS_OFF. So when userspace keep detect the connector status in
boot time, we could see panel would flicker (light up for a while, and
turn off again, and keep loop for several time). I have copied some
kernel logs:
[ 11.065267] YKK --------- analogix_dp_detect:1052
[ 11.729596] YKK --------- analogix_dp_get_modes:1016
[ 11.737608] rockchip-dp ff970000.edp:
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[ 11.749229] rockchip-dp ff970000.edp:
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[ 11.760799] rockchip-dp ff970000.edp:
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[ 13.315962] YKK --------- analogix_dp_detect:1052
[ 13.984702] YKK --------- analogix_dp_get_modes:1016
[ 13.992977] rockchip-dp ff970000.edp:
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[ 14.004414] rockchip-dp ff970000.edp:
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[ 14.015842] rockchip-dp ff970000.edp:
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[ 14.183109] YKK --------- analogix_dp_bridge_pre_enable:1147
[ 14.306301] rockchip-dp ff970000.edp: Link Training Clock Recovery
success
[ 14.319130] rockchip-dp ff970000.edp: Link Training success!
[ 14.326388] rockchip-dp ff970000.edp: wait SYS_CTL_2.
[ 14.437247] rockchip-dp ff970000.edp: Timeout of video streamclk ok
[ 14.443585] rockchip-dp ff970000.edp: unable to config video
> }
>
> static void analogix_dp_connector_destroy(struct drm_connector *connector)
> @@ -1035,6 +1051,18 @@ static int analogix_dp_bridge_attach(struct drm_bridge *bridge)
> return 0;
> }
>
> +static void analogix_dp_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> + struct analogix_dp_device *dp = bridge->driver_private;
> + int ret;
> +
> + if (dp->plat_data->panel) {
> + ret = drm_panel_prepare(dp->plat_data->panel);
> + if (ret)
> + DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> + }
2. Driver would failed to read EDID in some case. Panel would only be
powered up in bridge->pre_enable() function which later than
connector->get_modes() function, and this would caused DPCD transfer
failed in analogix_dp_handle_edid(). This seem won't caused too big
issue, cause userspace would read EDID again after bridge/encoder is
enabled. But it's better to avoid this potential bug.
[ 11.065267] YKK --------- analogix_dp_detect:1052
[ 11.729596] YKK --------- analogix_dp_get_modes:1016
[ 11.737608] rockchip-dp ff970000.edp:
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[ 11.749229] rockchip-dp ff970000.edp:
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[ 11.760799] rockchip-dp ff970000.edp:
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[ 13.315962] YKK --------- analogix_dp_detect:1052
[ 13.984702] YKK --------- analogix_dp_get_modes:1016
[ 13.992977] rockchip-dp ff970000.edp:
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[ 14.004414] rockchip-dp ff970000.edp:
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[ 14.015842] rockchip-dp ff970000.edp:
analogix_dp_read_bytes_from_dpcd: Aux Transaction fail!
[ 14.183109] YKK --------- analogix_dp_bridge_pre_enable:1147
[ 14.306301] rockchip-dp ff970000.edp: Link Training Clock Recovery
success
[ 14.319130] rockchip-dp ff970000.edp: Link Training success!
[ 14.326388] rockchip-dp ff970000.edp: wait SYS_CTL_2.
[ 14.437247] rockchip-dp ff970000.edp: Timeout of video streamclk ok
[ 14.443585] rockchip-dp ff970000.edp: unable to config video
[ 14.520565] YKK --------- analogix_dp_detect:1052
[ 14.525339] YKK --------- analogix_dp_get_modes:1016
[ 14.531233] rockchip-dp ff970000.edp: EDID data does not include any
extensions.
[ 15.595724] YKK --------- analogix_dp_detect:1052
[ 15.600488] YKK --------- analogix_dp_get_modes:1016
[ 15.606451] rockchip-dp ff970000.edp: EDID data does not include any
extensions.
[ 17.327354] YKK --------- analogix_dp_detect:1052
[ 17.332105] YKK --------- analogix_dp_get_modes:1016
[ 17.338171] rockchip-dp ff970000.edp: EDID data does not include any
extensions.
[ 56.236024] YKK --------- analogix_dp_detect:1052
[ 56.240820] YKK --------- analogix_dp_get_modes:1016
[ 56.246855] rockchip-dp ff970000.edp: EDID data does not include any
extensions.
[ 64.092477] YKK --------- analogix_dp_detect:1052
[ 64.763712] YKK --------- analogix_dp_get_modes:1016
BR,
- Yakir
> +}
> +
> static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
> {
> struct analogix_dp_device *dp = bridge->driver_private;
> @@ -1058,6 +1086,7 @@ static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
> static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
> {
> struct analogix_dp_device *dp = bridge->driver_private;
> + int ret;
>
> if (dp->dpms_mode != DRM_MODE_DPMS_ON)
> return;
> @@ -1077,6 +1106,10 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>
> pm_runtime_put_sync(dp->dev);
>
> + ret = drm_panel_unprepare(dp->plat_data->panel);
> + if (ret)
> + DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> +
> dp->dpms_mode = DRM_MODE_DPMS_OFF;
> }
>
> @@ -1165,9 +1198,9 @@ static void analogix_dp_bridge_nop(struct drm_bridge *bridge)
> }
>
> static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
> + .pre_enable = analogix_dp_bridge_pre_enable,
> .enable = analogix_dp_bridge_enable,
> .disable = analogix_dp_bridge_disable,
> - .pre_enable = analogix_dp_bridge_nop,
> .post_disable = analogix_dp_bridge_nop,
> .mode_set = analogix_dp_bridge_mode_set,
> .attach = analogix_dp_bridge_attach,
> @@ -1333,13 +1366,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,
Powered by blists - more mailing lists