From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Kuogee Hsieh <quic_khsieh@quicinc.com>,
robdclark@gmail.com, sean@poorly.run, swboyd@chromium.org,
dianders@chromium.org, vkoul@kernel.org, daniel@ffwll.ch,
airlied@linux.ie, agross@kernel.org, bjorn.andersson@linaro.org
Cc: quic_abhinavk@quicinc.com, quic_aravindh@quicinc.com,
quic_sbillaka@quicinc.com, freedreno@lists.freedesktop.org,
dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6] drm/msm/dp: remove fail safe mode related code
Date: Wed, 27 Apr 2022 10:52:50 +0300 [thread overview]
Message-ID: <c9ba0a63-0bc4-ed31-d36a-9b2fe1e09cd6@linaro.org> (raw)
In-Reply-To: <1651007534-31842-1-git-send-email-quic_khsieh@quicinc.com>
On 27/04/2022 00:12, Kuogee Hsieh wrote:
> Current DP driver implementation has adding safe mode done at
> dp_hpd_plug_handle() which is expected to be executed under event
> thread context.
>
> However there is possible circular locking happen (see blow stack trace)
> after edp driver call dp_hpd_plug_handle() from dp_bridge_enable() which
> is executed under drm_thread context.
>
> After review all possibilities methods and as discussed on
> https://patchwork.freedesktop.org/patch/483155/, supporting EDID
> compliance tests in the driver is quite hacky. As seen with other
> vendor drivers, supporting these will be much easier with IGT. Hence
> removing all the related fail safe code for it so that no possibility
> of circular lock will happen.
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.15.35-lockdep #6 Tainted: G W
> ------------------------------------------------------
> frecon/429 is trying to acquire lock:
> ffffff808dc3c4e8 (&dev->mode_config.mutex){+.+.}-{3:3}, at:
> dp_panel_add_fail_safe_mode+0x4c/0xa0
>
> but task is already holding lock:
> ffffff808dc441e0 (&kms->commit_lock[i]){+.+.}-{3:3}, at: lock_crtcs+0xb4/0x124
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #3 (&kms->commit_lock[i]){+.+.}-{3:3}:
> __mutex_lock_common+0x174/0x1a64
> mutex_lock_nested+0x98/0xac
> lock_crtcs+0xb4/0x124
> msm_atomic_commit_tail+0x330/0x748
> commit_tail+0x19c/0x278
> drm_atomic_helper_commit+0x1dc/0x1f0
> drm_atomic_commit+0xc0/0xd8
> drm_atomic_helper_set_config+0xb4/0x134
> drm_mode_setcrtc+0x688/0x1248
> drm_ioctl_kernel+0x1e4/0x338
> drm_ioctl+0x3a4/0x684
> __arm64_sys_ioctl+0x118/0x154
> invoke_syscall+0x78/0x224
> el0_svc_common+0x178/0x200
> do_el0_svc+0x94/0x13c
> el0_svc+0x5c/0xec
> el0t_64_sync_handler+0x78/0x108
> el0t_64_sync+0x1a4/0x1a8
>
> -> #2 (crtc_ww_class_mutex){+.+.}-{3:3}:
> __mutex_lock_common+0x174/0x1a64
> ww_mutex_lock+0xb8/0x278
> modeset_lock+0x304/0x4ac
> drm_modeset_lock+0x4c/0x7c
> drmm_mode_config_init+0x4a8/0xc50
> msm_drm_init+0x274/0xac0
> msm_drm_bind+0x20/0x2c
> try_to_bring_up_master+0x3dc/0x470
> __component_add+0x18c/0x3c0
> component_add+0x1c/0x28
> dp_display_probe+0x954/0xa98
> platform_probe+0x124/0x15c
> really_probe+0x1b0/0x5f8
> __driver_probe_device+0x174/0x20c
> driver_probe_device+0x70/0x134
> __device_attach_driver+0x130/0x1d0
> bus_for_each_drv+0xfc/0x14c
> __device_attach+0x1bc/0x2bc
> device_initial_probe+0x1c/0x28
> bus_probe_device+0x94/0x178
> deferred_probe_work_func+0x1a4/0x1f0
> process_one_work+0x5d4/0x9dc
> worker_thread+0x898/0xccc
> kthread+0x2d4/0x3d4
> ret_from_fork+0x10/0x20
>
> -> #1 (crtc_ww_class_acquire){+.+.}-{0:0}:
> ww_acquire_init+0x1c4/0x2c8
> drm_modeset_acquire_init+0x44/0xc8
> drm_helper_probe_single_connector_modes+0xb0/0x12dc
> drm_mode_getconnector+0x5dc/0xfe8
> drm_ioctl_kernel+0x1e4/0x338
> drm_ioctl+0x3a4/0x684
> __arm64_sys_ioctl+0x118/0x154
> invoke_syscall+0x78/0x224
> el0_svc_common+0x178/0x200
> do_el0_svc+0x94/0x13c
> el0_svc+0x5c/0xec
> el0t_64_sync_handler+0x78/0x108
> el0t_64_sync+0x1a4/0x1a8
>
> -> #0 (&dev->mode_config.mutex){+.+.}-{3:3}:
> __lock_acquire+0x2650/0x672c
> lock_acquire+0x1b4/0x4ac
> __mutex_lock_common+0x174/0x1a64
> mutex_lock_nested+0x98/0xac
> dp_panel_add_fail_safe_mode+0x4c/0xa0
> dp_hpd_plug_handle+0x1f0/0x280
> dp_bridge_enable+0x94/0x2b8
> drm_atomic_bridge_chain_enable+0x11c/0x168
> drm_atomic_helper_commit_modeset_enables+0x500/0x740
> msm_atomic_commit_tail+0x3e4/0x748
> commit_tail+0x19c/0x278
> drm_atomic_helper_commit+0x1dc/0x1f0
> drm_atomic_commit+0xc0/0xd8
> drm_atomic_helper_set_config+0xb4/0x134
> drm_mode_setcrtc+0x688/0x1248
> drm_ioctl_kernel+0x1e4/0x338
> drm_ioctl+0x3a4/0x684
> __arm64_sys_ioctl+0x118/0x154
> invoke_syscall+0x78/0x224
> el0_svc_common+0x178/0x200
> do_el0_svc+0x94/0x13c
> el0_svc+0x5c/0xec
> el0t_64_sync_handler+0x78/0x108
> el0t_64_sync+0x1a4/0x1a8
>
> Changes in v2:
> -- re text commit title
> -- remove all fail safe mode
>
> Changes in v3:
> -- remove dp_panel_add_fail_safe_mode() from dp_panel.h
> -- add Fixes
>
> Changes in v5:
> -- to=dianders@chromium.org
>
> Changes in v6:
> -- fix Fixes commit ID
>
> Fixes: 8b2c181e3dcf ("drm/msm/dp: add fail safe mode outside of event_mutex context")
> Reported-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 6 ------
> drivers/gpu/drm/msm/dp/dp_panel.c | 11 -----------
> drivers/gpu/drm/msm/dp/dp_panel.h | 1 -
> 3 files changed, 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 92cd50f..01453db 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -555,12 +555,6 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
>
> mutex_unlock(&dp->event_mutex);
>
> - /*
> - * add fail safe mode outside event_mutex scope
> - * to avoid potiential circular lock with drm thread
> - */
> - dp_panel_add_fail_safe_mode(dp->dp_display.connector);
> -
> /* uevent will complete connection part */
> return 0;
> };
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
> index 1aa9aa8c..8ff174a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -151,15 +151,6 @@ static int dp_panel_update_modes(struct drm_connector *connector,
> return rc;
> }
>
> -void dp_panel_add_fail_safe_mode(struct drm_connector *connector)
> -{
> - /* fail safe edid */
> - mutex_lock(&connector->dev->mode_config.mutex);
> - if (drm_add_modes_noedid(connector, 640, 480))
> - drm_set_preferred_mode(connector, 640, 480);
> - mutex_unlock(&connector->dev->mode_config.mutex);
> -}
> -
> int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
> struct drm_connector *connector)
> {
> @@ -215,8 +206,6 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
> rc = -ETIMEDOUT;
> goto end;
> }
> -
> - dp_panel_add_fail_safe_mode(connector);
> }
>
> if (panel->aux_cfg_update_done) {
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
> index 99739ea..9023e5b 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
> @@ -59,7 +59,6 @@ int dp_panel_init_panel_info(struct dp_panel *dp_panel);
> int dp_panel_deinit(struct dp_panel *dp_panel);
> int dp_panel_timing_cfg(struct dp_panel *dp_panel);
> void dp_panel_dump_regs(struct dp_panel *dp_panel);
> -void dp_panel_add_fail_safe_mode(struct drm_connector *connector);
> int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
> struct drm_connector *connector);
> u32 dp_panel_get_mode_bpp(struct dp_panel *dp_panel, u32 mode_max_bpp,
--
With best wishes
Dmitry
WARNING: multiple messages have this Message-ID (diff)
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Kuogee Hsieh <quic_khsieh@quicinc.com>,
robdclark@gmail.com, sean@poorly.run, swboyd@chromium.org,
dianders@chromium.org, vkoul@kernel.org, daniel@ffwll.ch,
airlied@linux.ie, agross@kernel.org, bjorn.andersson@linaro.org
Cc: quic_sbillaka@quicinc.com, linux-arm-msm@vger.kernel.org,
quic_abhinavk@quicinc.com, dri-devel@lists.freedesktop.org,
linux-kernel@vger.kernel.org, quic_aravindh@quicinc.com,
freedreno@lists.freedesktop.org
Subject: Re: [PATCH v6] drm/msm/dp: remove fail safe mode related code
Date: Wed, 27 Apr 2022 10:52:50 +0300 [thread overview]
Message-ID: <c9ba0a63-0bc4-ed31-d36a-9b2fe1e09cd6@linaro.org> (raw)
In-Reply-To: <1651007534-31842-1-git-send-email-quic_khsieh@quicinc.com>
On 27/04/2022 00:12, Kuogee Hsieh wrote:
> Current DP driver implementation has adding safe mode done at
> dp_hpd_plug_handle() which is expected to be executed under event
> thread context.
>
> However there is possible circular locking happen (see blow stack trace)
> after edp driver call dp_hpd_plug_handle() from dp_bridge_enable() which
> is executed under drm_thread context.
>
> After review all possibilities methods and as discussed on
> https://patchwork.freedesktop.org/patch/483155/, supporting EDID
> compliance tests in the driver is quite hacky. As seen with other
> vendor drivers, supporting these will be much easier with IGT. Hence
> removing all the related fail safe code for it so that no possibility
> of circular lock will happen.
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.15.35-lockdep #6 Tainted: G W
> ------------------------------------------------------
> frecon/429 is trying to acquire lock:
> ffffff808dc3c4e8 (&dev->mode_config.mutex){+.+.}-{3:3}, at:
> dp_panel_add_fail_safe_mode+0x4c/0xa0
>
> but task is already holding lock:
> ffffff808dc441e0 (&kms->commit_lock[i]){+.+.}-{3:3}, at: lock_crtcs+0xb4/0x124
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #3 (&kms->commit_lock[i]){+.+.}-{3:3}:
> __mutex_lock_common+0x174/0x1a64
> mutex_lock_nested+0x98/0xac
> lock_crtcs+0xb4/0x124
> msm_atomic_commit_tail+0x330/0x748
> commit_tail+0x19c/0x278
> drm_atomic_helper_commit+0x1dc/0x1f0
> drm_atomic_commit+0xc0/0xd8
> drm_atomic_helper_set_config+0xb4/0x134
> drm_mode_setcrtc+0x688/0x1248
> drm_ioctl_kernel+0x1e4/0x338
> drm_ioctl+0x3a4/0x684
> __arm64_sys_ioctl+0x118/0x154
> invoke_syscall+0x78/0x224
> el0_svc_common+0x178/0x200
> do_el0_svc+0x94/0x13c
> el0_svc+0x5c/0xec
> el0t_64_sync_handler+0x78/0x108
> el0t_64_sync+0x1a4/0x1a8
>
> -> #2 (crtc_ww_class_mutex){+.+.}-{3:3}:
> __mutex_lock_common+0x174/0x1a64
> ww_mutex_lock+0xb8/0x278
> modeset_lock+0x304/0x4ac
> drm_modeset_lock+0x4c/0x7c
> drmm_mode_config_init+0x4a8/0xc50
> msm_drm_init+0x274/0xac0
> msm_drm_bind+0x20/0x2c
> try_to_bring_up_master+0x3dc/0x470
> __component_add+0x18c/0x3c0
> component_add+0x1c/0x28
> dp_display_probe+0x954/0xa98
> platform_probe+0x124/0x15c
> really_probe+0x1b0/0x5f8
> __driver_probe_device+0x174/0x20c
> driver_probe_device+0x70/0x134
> __device_attach_driver+0x130/0x1d0
> bus_for_each_drv+0xfc/0x14c
> __device_attach+0x1bc/0x2bc
> device_initial_probe+0x1c/0x28
> bus_probe_device+0x94/0x178
> deferred_probe_work_func+0x1a4/0x1f0
> process_one_work+0x5d4/0x9dc
> worker_thread+0x898/0xccc
> kthread+0x2d4/0x3d4
> ret_from_fork+0x10/0x20
>
> -> #1 (crtc_ww_class_acquire){+.+.}-{0:0}:
> ww_acquire_init+0x1c4/0x2c8
> drm_modeset_acquire_init+0x44/0xc8
> drm_helper_probe_single_connector_modes+0xb0/0x12dc
> drm_mode_getconnector+0x5dc/0xfe8
> drm_ioctl_kernel+0x1e4/0x338
> drm_ioctl+0x3a4/0x684
> __arm64_sys_ioctl+0x118/0x154
> invoke_syscall+0x78/0x224
> el0_svc_common+0x178/0x200
> do_el0_svc+0x94/0x13c
> el0_svc+0x5c/0xec
> el0t_64_sync_handler+0x78/0x108
> el0t_64_sync+0x1a4/0x1a8
>
> -> #0 (&dev->mode_config.mutex){+.+.}-{3:3}:
> __lock_acquire+0x2650/0x672c
> lock_acquire+0x1b4/0x4ac
> __mutex_lock_common+0x174/0x1a64
> mutex_lock_nested+0x98/0xac
> dp_panel_add_fail_safe_mode+0x4c/0xa0
> dp_hpd_plug_handle+0x1f0/0x280
> dp_bridge_enable+0x94/0x2b8
> drm_atomic_bridge_chain_enable+0x11c/0x168
> drm_atomic_helper_commit_modeset_enables+0x500/0x740
> msm_atomic_commit_tail+0x3e4/0x748
> commit_tail+0x19c/0x278
> drm_atomic_helper_commit+0x1dc/0x1f0
> drm_atomic_commit+0xc0/0xd8
> drm_atomic_helper_set_config+0xb4/0x134
> drm_mode_setcrtc+0x688/0x1248
> drm_ioctl_kernel+0x1e4/0x338
> drm_ioctl+0x3a4/0x684
> __arm64_sys_ioctl+0x118/0x154
> invoke_syscall+0x78/0x224
> el0_svc_common+0x178/0x200
> do_el0_svc+0x94/0x13c
> el0_svc+0x5c/0xec
> el0t_64_sync_handler+0x78/0x108
> el0t_64_sync+0x1a4/0x1a8
>
> Changes in v2:
> -- re text commit title
> -- remove all fail safe mode
>
> Changes in v3:
> -- remove dp_panel_add_fail_safe_mode() from dp_panel.h
> -- add Fixes
>
> Changes in v5:
> -- to=dianders@chromium.org
>
> Changes in v6:
> -- fix Fixes commit ID
>
> Fixes: 8b2c181e3dcf ("drm/msm/dp: add fail safe mode outside of event_mutex context")
> Reported-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 6 ------
> drivers/gpu/drm/msm/dp/dp_panel.c | 11 -----------
> drivers/gpu/drm/msm/dp/dp_panel.h | 1 -
> 3 files changed, 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 92cd50f..01453db 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -555,12 +555,6 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
>
> mutex_unlock(&dp->event_mutex);
>
> - /*
> - * add fail safe mode outside event_mutex scope
> - * to avoid potiential circular lock with drm thread
> - */
> - dp_panel_add_fail_safe_mode(dp->dp_display.connector);
> -
> /* uevent will complete connection part */
> return 0;
> };
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
> index 1aa9aa8c..8ff174a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -151,15 +151,6 @@ static int dp_panel_update_modes(struct drm_connector *connector,
> return rc;
> }
>
> -void dp_panel_add_fail_safe_mode(struct drm_connector *connector)
> -{
> - /* fail safe edid */
> - mutex_lock(&connector->dev->mode_config.mutex);
> - if (drm_add_modes_noedid(connector, 640, 480))
> - drm_set_preferred_mode(connector, 640, 480);
> - mutex_unlock(&connector->dev->mode_config.mutex);
> -}
> -
> int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
> struct drm_connector *connector)
> {
> @@ -215,8 +206,6 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
> rc = -ETIMEDOUT;
> goto end;
> }
> -
> - dp_panel_add_fail_safe_mode(connector);
> }
>
> if (panel->aux_cfg_update_done) {
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
> index 99739ea..9023e5b 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
> @@ -59,7 +59,6 @@ int dp_panel_init_panel_info(struct dp_panel *dp_panel);
> int dp_panel_deinit(struct dp_panel *dp_panel);
> int dp_panel_timing_cfg(struct dp_panel *dp_panel);
> void dp_panel_dump_regs(struct dp_panel *dp_panel);
> -void dp_panel_add_fail_safe_mode(struct drm_connector *connector);
> int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
> struct drm_connector *connector);
> u32 dp_panel_get_mode_bpp(struct dp_panel *dp_panel, u32 mode_max_bpp,
--
With best wishes
Dmitry
next prev parent reply other threads:[~2022-04-27 7:52 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-26 21:12 [PATCH v6] drm/msm/dp: remove fail safe mode related code Kuogee Hsieh
2022-04-26 21:12 ` Kuogee Hsieh
2022-04-26 21:18 ` Stephen Boyd
2022-04-26 21:18 ` Stephen Boyd
2022-04-26 21:30 ` Doug Anderson
2022-04-26 21:30 ` Doug Anderson
2022-04-27 7:52 ` Dmitry Baryshkov [this message]
2022-04-27 7:52 ` Dmitry Baryshkov
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=c9ba0a63-0bc4-ed31-d36a-9b2fe1e09cd6@linaro.org \
--to=dmitry.baryshkov@linaro.org \
--cc=agross@kernel.org \
--cc=airlied@linux.ie \
--cc=bjorn.andersson@linaro.org \
--cc=daniel@ffwll.ch \
--cc=dianders@chromium.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=quic_abhinavk@quicinc.com \
--cc=quic_aravindh@quicinc.com \
--cc=quic_khsieh@quicinc.com \
--cc=quic_sbillaka@quicinc.com \
--cc=robdclark@gmail.com \
--cc=sean@poorly.run \
--cc=swboyd@chromium.org \
--cc=vkoul@kernel.org \
/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.