* [PATCH] drm/fb-helper: Fix restore_fbdev when there are pending delayed hotplug
@ 2021-11-04 16:32 Jocelyn Falempe
2021-11-05 15:50 ` Michel Dänzer
0 siblings, 1 reply; 3+ messages in thread
From: Jocelyn Falempe @ 2021-11-04 16:32 UTC (permalink / raw)
To: dri-devel; +Cc: Jocelyn Falempe
When using Xorg/Logind and an external monitor connected with an MST dock.
After disconnecting the external monitor, switching to VT may not work,
the (internal) monitor sill display Xorg, and you can't see what you are
typing in the VT.
This is related to commit e2809c7db818df6bbd0edf843e1beb2fbc9d8541 which
introduced delayed hotplug for MST, and commit
dc5bdb68b5b369d5bc7d1de96fa64cc1737a6320 which introduced a workaround for
Xorg and logind, and add a force parameter to
__drm_fb_helper_restore_fbdev_mode_unlocked() in this case.
When switching to VT, with Xorg and logind, if there
are pending hotplug event (like MST unplugged), the hotplug path
may not be fulfilled, because logind may drop the master a bit later.
It leads to the console not showing up on the monitor.
So in this case, forward the "force" parameter to the hotplug event,
and ignore if there is a drm master in this case.
Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
drivers/gpu/drm/drm_fb_helper.c | 66 ++++++++++++++++++---------------
1 file changed, 36 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 8e7a124d6c5a..c07080f661b1 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -82,6 +82,8 @@ MODULE_PARM_DESC(drm_leak_fbdev_smem,
static LIST_HEAD(kernel_fb_helper_list);
static DEFINE_MUTEX(kernel_fb_helper_lock);
+static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper, bool force);
+
/**
* DOC: fbdev helpers
*
@@ -258,7 +260,7 @@ __drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper,
mutex_unlock(&fb_helper->lock);
if (do_delayed)
- drm_fb_helper_hotplug_event(fb_helper);
+ __drm_fb_helper_hotplug_event(fb_helper, force);
return ret;
}
@@ -1930,6 +1932,38 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
}
EXPORT_SYMBOL(drm_fb_helper_initial_config);
+static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper, bool force)
+{
+ int err = 0;
+
+ if (!drm_fbdev_emulation || !fb_helper)
+ return 0;
+
+ mutex_lock(&fb_helper->lock);
+ if (fb_helper->deferred_setup) {
+ err = __drm_fb_helper_initial_config_and_unlock(fb_helper,
+ fb_helper->preferred_bpp);
+ return err;
+ }
+ if (!force) {
+ if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) {
+ fb_helper->delayed_hotplug = true;
+ mutex_unlock(&fb_helper->lock);
+ return err;
+ }
+ drm_master_internal_release(fb_helper->dev);
+ }
+ drm_dbg_kms(fb_helper->dev, "\n");
+
+ drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width, fb_helper->fb->height);
+ drm_setup_crtcs_fb(fb_helper);
+ mutex_unlock(&fb_helper->lock);
+
+ drm_fb_helper_set_par(fb_helper->fbdev);
+
+ return 0;
+}
+
/**
* drm_fb_helper_hotplug_event - respond to a hotplug notification by
* probing all the outputs attached to the fb
@@ -1953,35 +1987,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
*/
int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
{
- int err = 0;
-
- if (!drm_fbdev_emulation || !fb_helper)
- return 0;
-
- mutex_lock(&fb_helper->lock);
- if (fb_helper->deferred_setup) {
- err = __drm_fb_helper_initial_config_and_unlock(fb_helper,
- fb_helper->preferred_bpp);
- return err;
- }
-
- if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) {
- fb_helper->delayed_hotplug = true;
- mutex_unlock(&fb_helper->lock);
- return err;
- }
-
- drm_master_internal_release(fb_helper->dev);
-
- drm_dbg_kms(fb_helper->dev, "\n");
-
- drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width, fb_helper->fb->height);
- drm_setup_crtcs_fb(fb_helper);
- mutex_unlock(&fb_helper->lock);
-
- drm_fb_helper_set_par(fb_helper->fbdev);
-
- return 0;
+ return __drm_fb_helper_hotplug_event(fb_helper, false);
}
EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
--
2.33.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/fb-helper: Fix restore_fbdev when there are pending delayed hotplug
2021-11-04 16:32 [PATCH] drm/fb-helper: Fix restore_fbdev when there are pending delayed hotplug Jocelyn Falempe
@ 2021-11-05 15:50 ` Michel Dänzer
2021-11-05 17:50 ` Jocelyn Falempe
0 siblings, 1 reply; 3+ messages in thread
From: Michel Dänzer @ 2021-11-05 15:50 UTC (permalink / raw)
To: Jocelyn Falempe; +Cc: dri-devel
On 2021-11-04 17:32, Jocelyn Falempe wrote:
> When using Xorg/Logind and an external monitor connected with an MST dock.
> After disconnecting the external monitor, switching to VT may not work,
> the (internal) monitor sill display Xorg, and you can't see what you are
> typing in the VT.
>
> This is related to commit e2809c7db818df6bbd0edf843e1beb2fbc9d8541 which
> introduced delayed hotplug for MST, and commit
> dc5bdb68b5b369d5bc7d1de96fa64cc1737a6320 which introduced a workaround for
> Xorg and logind, and add a force parameter to
> __drm_fb_helper_restore_fbdev_mode_unlocked() in this case.
>
> When switching to VT, with Xorg and logind, if there
> are pending hotplug event (like MST unplugged), the hotplug path
> may not be fulfilled, because logind may drop the master a bit later.
> It leads to the console not showing up on the monitor.
>
> So in this case, forward the "force" parameter to the hotplug event,
> and ignore if there is a drm master in this case.
I'm worried that this leaves a race condition which allows the still-master (which causes drm_fb_helper_hotplug_event to bail without this patch) to modify the state set by __drm_fb_helper_hotplug_event, which could still result in similar symptoms.
I wonder if something like calling drm_fb_helper_hotplug_event when master is dropped and fb_helper->delayed_hotplug == true could work instead.
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 8e7a124d6c5a..c07080f661b1 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -82,6 +82,8 @@ MODULE_PARM_DESC(drm_leak_fbdev_smem,
> static LIST_HEAD(kernel_fb_helper_list);
> static DEFINE_MUTEX(kernel_fb_helper_lock);
>
> +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper, bool force);
> +
> /**
> * DOC: fbdev helpers
> *
> @@ -258,7 +260,7 @@ __drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper,
> mutex_unlock(&fb_helper->lock);
>
> if (do_delayed)
> - drm_fb_helper_hotplug_event(fb_helper);
> + __drm_fb_helper_hotplug_event(fb_helper, force);
>
> return ret;
> }
> @@ -1930,6 +1932,38 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
> }
> EXPORT_SYMBOL(drm_fb_helper_initial_config);
>
> +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper, bool force)
> +{
> + int err = 0;
> +
> + if (!drm_fbdev_emulation || !fb_helper)
> + return 0;
> +
> + mutex_lock(&fb_helper->lock);
> + if (fb_helper->deferred_setup) {
> + err = __drm_fb_helper_initial_config_and_unlock(fb_helper,
> + fb_helper->preferred_bpp);
> + return err;
> + }
> + if (!force) {
> + if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) {
> + fb_helper->delayed_hotplug = true;
> + mutex_unlock(&fb_helper->lock);
> + return err;
> + }
> + drm_master_internal_release(fb_helper->dev);
> + }
> + drm_dbg_kms(fb_helper->dev, "\n");
> +
> + drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width, fb_helper->fb->height);
> + drm_setup_crtcs_fb(fb_helper);
> + mutex_unlock(&fb_helper->lock);
> +
> + drm_fb_helper_set_par(fb_helper->fbdev);
> +
> + return 0;
> +}
> +
> /**
> * drm_fb_helper_hotplug_event - respond to a hotplug notification by
> * probing all the outputs attached to the fb
> @@ -1953,35 +1987,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
> */
> int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
> {
> - int err = 0;
> -
> - if (!drm_fbdev_emulation || !fb_helper)
> - return 0;
> -
> - mutex_lock(&fb_helper->lock);
> - if (fb_helper->deferred_setup) {
> - err = __drm_fb_helper_initial_config_and_unlock(fb_helper,
> - fb_helper->preferred_bpp);
> - return err;
> - }
> -
> - if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) {
> - fb_helper->delayed_hotplug = true;
> - mutex_unlock(&fb_helper->lock);
> - return err;
> - }
> -
> - drm_master_internal_release(fb_helper->dev);
> -
> - drm_dbg_kms(fb_helper->dev, "\n");
> -
> - drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width, fb_helper->fb->height);
> - drm_setup_crtcs_fb(fb_helper);
> - mutex_unlock(&fb_helper->lock);
> -
> - drm_fb_helper_set_par(fb_helper->fbdev);
> -
> - return 0;
> + return __drm_fb_helper_hotplug_event(fb_helper, false);
> }
> EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>
>
--
Earthling Michel Dänzer | https://redhat.com
Libre software enthusiast | Mesa and Xwayland developer
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/fb-helper: Fix restore_fbdev when there are pending delayed hotplug
2021-11-05 15:50 ` Michel Dänzer
@ 2021-11-05 17:50 ` Jocelyn Falempe
0 siblings, 0 replies; 3+ messages in thread
From: Jocelyn Falempe @ 2021-11-05 17:50 UTC (permalink / raw)
To: dri-devel; +Cc: michel
On 05/11/2021 16:50, Michel Dänzer wrote:
> On 2021-11-04 17:32, Jocelyn Falempe wrote:
>> When using Xorg/Logind and an external monitor connected with an MST dock.
>> After disconnecting the external monitor, switching to VT may not work,
>> the (internal) monitor sill display Xorg, and you can't see what you are
>> typing in the VT.
>>
>> This is related to commit e2809c7db818df6bbd0edf843e1beb2fbc9d8541 which
>> introduced delayed hotplug for MST, and commit
>> dc5bdb68b5b369d5bc7d1de96fa64cc1737a6320 which introduced a workaround for
>> Xorg and logind, and add a force parameter to
>> __drm_fb_helper_restore_fbdev_mode_unlocked() in this case.
>>
>> When switching to VT, with Xorg and logind, if there
>> are pending hotplug event (like MST unplugged), the hotplug path
>> may not be fulfilled, because logind may drop the master a bit later.
>> It leads to the console not showing up on the monitor.
>>
>> So in this case, forward the "force" parameter to the hotplug event,
>> and ignore if there is a drm master in this case.
>
> I'm worried that this leaves a race condition which allows the still-master (which causes drm_fb_helper_hotplug_event to bail without this patch) to modify the state set by __drm_fb_helper_hotplug_event, which could still result in similar symptoms.
>
> I wonder if something like calling drm_fb_helper_hotplug_event when master is dropped and fb_helper->delayed_hotplug == true could work instead.
>
Ok, I will try to make a new patch and call
drm_fb_helper_hotplug_event() from drm_drop_master() instead.
>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 8e7a124d6c5a..c07080f661b1 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -82,6 +82,8 @@ MODULE_PARM_DESC(drm_leak_fbdev_smem,
>> static LIST_HEAD(kernel_fb_helper_list);
>> static DEFINE_MUTEX(kernel_fb_helper_lock);
>>
>> +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper, bool force);
>> +
>> /**
>> * DOC: fbdev helpers
>> *
>> @@ -258,7 +260,7 @@ __drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper,
>> mutex_unlock(&fb_helper->lock);
>>
>> if (do_delayed)
>> - drm_fb_helper_hotplug_event(fb_helper);
>> + __drm_fb_helper_hotplug_event(fb_helper, force);
>>
>> return ret;
>> }
>> @@ -1930,6 +1932,38 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
>> }
>> EXPORT_SYMBOL(drm_fb_helper_initial_config);
>>
>> +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper, bool force)
>> +{
>> + int err = 0;
>> +
>> + if (!drm_fbdev_emulation || !fb_helper)
>> + return 0;
>> +
>> + mutex_lock(&fb_helper->lock);
>> + if (fb_helper->deferred_setup) {
>> + err = __drm_fb_helper_initial_config_and_unlock(fb_helper,
>> + fb_helper->preferred_bpp);
>> + return err;
>> + }
>> + if (!force) {
>> + if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) {
>> + fb_helper->delayed_hotplug = true;
>> + mutex_unlock(&fb_helper->lock);
>> + return err;
>> + }
>> + drm_master_internal_release(fb_helper->dev);
>> + }
>> + drm_dbg_kms(fb_helper->dev, "\n");
>> +
>> + drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width, fb_helper->fb->height);
>> + drm_setup_crtcs_fb(fb_helper);
>> + mutex_unlock(&fb_helper->lock);
>> +
>> + drm_fb_helper_set_par(fb_helper->fbdev);
>> +
>> + return 0;
>> +}
>> +
>> /**
>> * drm_fb_helper_hotplug_event - respond to a hotplug notification by
>> * probing all the outputs attached to the fb
>> @@ -1953,35 +1987,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
>> */
>> int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>> {
>> - int err = 0;
>> -
>> - if (!drm_fbdev_emulation || !fb_helper)
>> - return 0;
>> -
>> - mutex_lock(&fb_helper->lock);
>> - if (fb_helper->deferred_setup) {
>> - err = __drm_fb_helper_initial_config_and_unlock(fb_helper,
>> - fb_helper->preferred_bpp);
>> - return err;
>> - }
>> -
>> - if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) {
>> - fb_helper->delayed_hotplug = true;
>> - mutex_unlock(&fb_helper->lock);
>> - return err;
>> - }
>> -
>> - drm_master_internal_release(fb_helper->dev);
>> -
>> - drm_dbg_kms(fb_helper->dev, "\n");
>> -
>> - drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width, fb_helper->fb->height);
>> - drm_setup_crtcs_fb(fb_helper);
>> - mutex_unlock(&fb_helper->lock);
>> -
>> - drm_fb_helper_set_par(fb_helper->fbdev);
>> -
>> - return 0;
>> + return __drm_fb_helper_hotplug_event(fb_helper, false);
>> }
>> EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>>
>>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-11-05 17:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-04 16:32 [PATCH] drm/fb-helper: Fix restore_fbdev when there are pending delayed hotplug Jocelyn Falempe
2021-11-05 15:50 ` Michel Dänzer
2021-11-05 17:50 ` Jocelyn Falempe
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.