On Thu, Oct 8, 2020 at 11:22 AM Thomas Zimmermann tzimmermann@suse.de wrote:

Hi

Am 07.10.20 um 15:30 schrieb Daniel Vetter:

...

We didn't take the kernel_fb_helper_lock mutex, which protects that code. While at it, simplify the code

• inline the function (originally shared with kgdb I think)
• drop the error tracking and all the complications
• drop the pointless early out, it served nothing

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch

---

drivers/gpu/drm/drm_fb_helper.c | 26 +++++--------------------- 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 8697554ccd41..c2f72bb6afb1 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -281,18 +281,12 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked);

#ifdef CONFIG_MAGIC_SYSRQ -/*

• • restore fbcon display for all kms driver's using this helper, used for sysrq
• • and panic handling.
• */

-static bool drm_fb_helper_force_kernel_mode(void) +/* emergency restore, don't bother with error reporting */ +static void drm_fb_helper_restore_work_fn(struct work_struct *ignored) {

• bool ret, error = false;
  struct drm_fb_helper *helper;
  
  

• if (list_empty(&kernel_fb_helper_list))
  

•         return false;
  

• mutex_lock(&kernel_fb_helper_lock);
  list_for_each_entry(helper, &kernel_fb_helper_list, kernel_fb_list) {
          struct drm_device *dev = helper->dev;
  
  

@@ -300,22 +294,12 @@ static bool drm_fb_helper_force_kernel_mode(void) continue;

          mutex_lock(&helper->lock);

•         ret = drm_client_modeset_commit_locked(&helper->client);
  

•         if (ret)
  

•                 error = true;
  

•         drm_client_modeset_commit_locked(&helper->client);
          mutex_unlock(&helper->lock);
  }
  

• return error;
  

• mutex_unlock(&kernel_fb_helper_lock);
  

}

-static void drm_fb_helper_restore_work_fn(struct work_struct *ignored) -{

• bool ret;
  

• ret = drm_fb_helper_force_kernel_mode();
  

• if (ret == true)
  

•         DRM_ERROR("Failed to restore crtc configuration\n");
  

Is there a specific reason for removing that warning? Even if it doesn't show up on screen, is it not helpful in the kernel's log?

I just found it really unhelpful, if you're trying to force-show fbcon, what's the point if it doesn't work out? The user will notice. Also, if we're really in a dire situation where you want this, in my experience there's a bunch of random other reasons why this can fail, mostly when the work we have to schedule for locking reasons never runs. So it's also unreliable.

In any case, the rest looks good.

Acked-by: Thomas Zimmermann tzimmermann@suse.de

Thanks for taking a look, I'll apply it. -Daniel

Best regards Thomas

...

-} static DECLARE_WORK(drm_fb_helper_restore_work, drm_fb_helper_restore_work_fn);

static void drm_fb_helper_sysrq(int dummy1)

-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer