diff mbox series

drm: Fix fbcon blank on QEMU graphics drivers

Message ID 20210416125344.13550-1-tiwai@suse.de (mailing list archive)
State New, archived
Headers show
Series drm: Fix fbcon blank on QEMU graphics drivers | expand

Commit Message

Takashi Iwai April 16, 2021, 12:53 p.m. UTC 
Currently the DRM fbcon helper for console blank,
drm_fb_helper_blank(), simply calls drm_fb_helper_dpms() and always
returns zero, supposing the driver dealing with DPMS or atomic
crtc->active flip to handle blanking the screen.  It works on most of
devices, but broken on most of KVM/QEMU graphics: bochs, qxl and
cirrus drivers just ignore crtc->active state change as blanking (or
cirrus ignoring DPMS).  In practice, when you run like
  % setterm --blank force
on a VT console, the screen freezes without actually blanking.

A simple fix for this problem would be not to rely on DPMS but let
fbcon performs the generic blank code.  This can be achieved just by
returning an error from drm_fb_helper_blank().

In this patch, we add a flag, no_dpms_blank, to drm_fb_helper for
indicating that the driver doesn't handle blank via DPMS or
crtc->active flip.  When this flag is set, drm_fb_helper_blank()
simply returns an error, so that fbcon falls back to its generic blank
handler.  The flag is set to both bochs and qxl drivers in this patch,
while cirrus is left untouched as it's declared as to-be-deprecated.

Link: https://lore.kernel.org/dri-devel/20170726205636.19144-1-tiwai@suse.de/
BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1095700
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---

Here I whip a dead horse again, revisiting the long-standing issue
stated in the previous patch set in 2017:
  https://lore.kernel.org/dri-devel/20170726205636.19144-1-tiwai@suse.de/

I thought to refresh the previous patch set at first, but it seems
invalid for the atomic modeset case.  And for the atomic, it's even
more difficult to propagate the return from the bottom to up.
So I ended up with this approach as it's much simpler.

But if there is any better way (even simpler or more robust), I'd
happily rewrite, too.

---
 drivers/gpu/drm/bochs/bochs_drv.c | 3 +++
 drivers/gpu/drm/drm_fb_helper.c   | 5 +++++
 drivers/gpu/drm/qxl/qxl_drv.c     | 3 +++
 include/drm/drm_fb_helper.h       | 8 ++++++++
 4 files changed, 19 insertions(+)

Comments

kernel test robot April 16, 2021, 6:07 p.m. UTC | #1 
Hi Takashi,

I love your patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-tip/drm-tip drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next linus/master v5.12-rc7 next-20210416]
[cannot apply to drm/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Takashi-Iwai/drm-Fix-fbcon-blank-on-QEMU-graphics-drivers/20210416-205539
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-a003-20210416 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 6a18cc23efad410db48a3ccfc233d215de7d4cb9)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/4b1a07505589e5f12ae52f249fa93b400e35e602
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Takashi-Iwai/drm-Fix-fbcon-blank-on-QEMU-graphics-drivers/20210416-205539
        git checkout 4b1a07505589e5f12ae52f249fa93b400e35e602
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/qxl/qxl_drv.c:123:12: error: no member named 'fb_helper' in 'struct qxl_device'
           if (qdev->fb_helper)
               ~~~~  ^
   drivers/gpu/drm/qxl/qxl_drv.c:124:9: error: no member named 'fb_helper' in 'struct qxl_device'
                   qdev->fb_helper->no_dpms_blank = true;
                   ~~~~  ^
   2 errors generated.


vim +123 drivers/gpu/drm/qxl/qxl_drv.c

    71	
    72	static int
    73	qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
    74	{
    75		struct qxl_device *qdev;
    76		int ret;
    77	
    78		if (pdev->revision < 4) {
    79			DRM_ERROR("qxl too old, doesn't support client_monitors_config,"
    80				  " use xf86-video-qxl in user mode");
    81			return -EINVAL; /* TODO: ENODEV ? */
    82		}
    83	
    84		qdev = devm_drm_dev_alloc(&pdev->dev, &qxl_driver,
    85					  struct qxl_device, ddev);
    86		if (IS_ERR(qdev)) {
    87			pr_err("Unable to init drm dev");
    88			return -ENOMEM;
    89		}
    90	
    91		ret = pci_enable_device(pdev);
    92		if (ret)
    93			return ret;
    94	
    95		ret = drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, "qxl");
    96		if (ret)
    97			goto disable_pci;
    98	
    99		if (is_vga(pdev) && pdev->revision < 5) {
   100			ret = vga_get_interruptible(pdev, VGA_RSRC_LEGACY_IO);
   101			if (ret) {
   102				DRM_ERROR("can't get legacy vga ioports\n");
   103				goto disable_pci;
   104			}
   105		}
   106	
   107		ret = qxl_device_init(qdev, pdev);
   108		if (ret)
   109			goto put_vga;
   110	
   111		ret = qxl_modeset_init(qdev);
   112		if (ret)
   113			goto unload;
   114	
   115		drm_kms_helper_poll_init(&qdev->ddev);
   116	
   117		/* Complete initialization. */
   118		ret = drm_dev_register(&qdev->ddev, ent->driver_data);
   119		if (ret)
   120			goto modeset_cleanup;
   121	
   122		drm_fbdev_generic_setup(&qdev->ddev, 32);
 > 123		if (qdev->fb_helper)
   124			qdev->fb_helper->no_dpms_blank = true;
   125	
   126		return 0;
   127	
   128	modeset_cleanup:
   129		qxl_modeset_fini(qdev);
   130	unload:
   131		qxl_device_fini(qdev);
   132	put_vga:
   133		if (is_vga(pdev) && pdev->revision < 5)
   134			vga_put(pdev, VGA_RSRC_LEGACY_IO);
   135	disable_pci:
   136		pci_disable_device(pdev);
   137	
   138		return ret;
   139	}
   140	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot April 16, 2021, 6:07 p.m. UTC | #2 
Hi Takashi,

I love your patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-tip/drm-tip drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next linus/master v5.12-rc7 next-20210416]
[cannot apply to drm/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Takashi-Iwai/drm-Fix-fbcon-blank-on-QEMU-graphics-drivers/20210416-205539
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: mips-randconfig-r001-20210416 (attached as .config)
compiler: mips64el-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/4b1a07505589e5f12ae52f249fa93b400e35e602
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Takashi-Iwai/drm-Fix-fbcon-blank-on-QEMU-graphics-drivers/20210416-205539
        git checkout 4b1a07505589e5f12ae52f249fa93b400e35e602
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/gpu/drm/qxl/qxl_drv.c: In function 'qxl_pci_probe':
>> drivers/gpu/drm/qxl/qxl_drv.c:123:10: error: 'struct qxl_device' has no member named 'fb_helper'
     123 |  if (qdev->fb_helper)
         |          ^~
   drivers/gpu/drm/qxl/qxl_drv.c:124:7: error: 'struct qxl_device' has no member named 'fb_helper'
     124 |   qdev->fb_helper->no_dpms_blank = true;
         |       ^~


vim +123 drivers/gpu/drm/qxl/qxl_drv.c

    71	
    72	static int
    73	qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
    74	{
    75		struct qxl_device *qdev;
    76		int ret;
    77	
    78		if (pdev->revision < 4) {
    79			DRM_ERROR("qxl too old, doesn't support client_monitors_config,"
    80				  " use xf86-video-qxl in user mode");
    81			return -EINVAL; /* TODO: ENODEV ? */
    82		}
    83	
    84		qdev = devm_drm_dev_alloc(&pdev->dev, &qxl_driver,
    85					  struct qxl_device, ddev);
    86		if (IS_ERR(qdev)) {
    87			pr_err("Unable to init drm dev");
    88			return -ENOMEM;
    89		}
    90	
    91		ret = pci_enable_device(pdev);
    92		if (ret)
    93			return ret;
    94	
    95		ret = drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, "qxl");
    96		if (ret)
    97			goto disable_pci;
    98	
    99		if (is_vga(pdev) && pdev->revision < 5) {
   100			ret = vga_get_interruptible(pdev, VGA_RSRC_LEGACY_IO);
   101			if (ret) {
   102				DRM_ERROR("can't get legacy vga ioports\n");
   103				goto disable_pci;
   104			}
   105		}
   106	
   107		ret = qxl_device_init(qdev, pdev);
   108		if (ret)
   109			goto put_vga;
   110	
   111		ret = qxl_modeset_init(qdev);
   112		if (ret)
   113			goto unload;
   114	
   115		drm_kms_helper_poll_init(&qdev->ddev);
   116	
   117		/* Complete initialization. */
   118		ret = drm_dev_register(&qdev->ddev, ent->driver_data);
   119		if (ret)
   120			goto modeset_cleanup;
   121	
   122		drm_fbdev_generic_setup(&qdev->ddev, 32);
 > 123		if (qdev->fb_helper)
   124			qdev->fb_helper->no_dpms_blank = true;
   125	
   126		return 0;
   127	
   128	modeset_cleanup:
   129		qxl_modeset_fini(qdev);
   130	unload:
   131		qxl_device_fini(qdev);
   132	put_vga:
   133		if (is_vga(pdev) && pdev->revision < 5)
   134			vga_put(pdev, VGA_RSRC_LEGACY_IO);
   135	disable_pci:
   136		pci_disable_device(pdev);
   137	
   138		return ret;
   139	}
   140	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Daniel Vetter April 16, 2021, 6:30 p.m. UTC | #3 
On Fri, Apr 16, 2021 at 2:53 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> Currently the DRM fbcon helper for console blank,
> drm_fb_helper_blank(), simply calls drm_fb_helper_dpms() and always
> returns zero, supposing the driver dealing with DPMS or atomic
> crtc->active flip to handle blanking the screen.  It works on most of
> devices, but broken on most of KVM/QEMU graphics: bochs, qxl and
> cirrus drivers just ignore crtc->active state change as blanking (or
> cirrus ignoring DPMS).  In practice, when you run like
>   % setterm --blank force
> on a VT console, the screen freezes without actually blanking.
>
> A simple fix for this problem would be not to rely on DPMS but let
> fbcon performs the generic blank code.  This can be achieved just by
> returning an error from drm_fb_helper_blank().
>
> In this patch, we add a flag, no_dpms_blank, to drm_fb_helper for
> indicating that the driver doesn't handle blank via DPMS or
> crtc->active flip.  When this flag is set, drm_fb_helper_blank()
> simply returns an error, so that fbcon falls back to its generic blank
> handler.  The flag is set to both bochs and qxl drivers in this patch,
> while cirrus is left untouched as it's declared as to-be-deprecated.
>
> Link: https://lore.kernel.org/dri-devel/20170726205636.19144-1-tiwai@suse.de/
> BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1095700
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Uh we're supposed to fix these drivers to actually blank (scan out
black, worst case), not paper over it in higher levels. Atomic kms is
meant to be a somewhat useful abstraction. So now fbcon blanks, but
your desktop still just freezes.

> ---
>
> Here I whip a dead horse again, revisiting the long-standing issue
> stated in the previous patch set in 2017:
>   https://lore.kernel.org/dri-devel/20170726205636.19144-1-tiwai@suse.de/
>
> I thought to refresh the previous patch set at first, but it seems
> invalid for the atomic modeset case.  And for the atomic, it's even
> more difficult to propagate the return from the bottom to up.
> So I ended up with this approach as it's much simpler.

Yeah that's because atomic assume you can at least blank your screen to black.
-Daniel

> But if there is any better way (even simpler or more robust), I'd
> happily rewrite, too.
>
> ---
>  drivers/gpu/drm/bochs/bochs_drv.c | 3 +++
>  drivers/gpu/drm/drm_fb_helper.c   | 5 +++++
>  drivers/gpu/drm/qxl/qxl_drv.c     | 3 +++
>  include/drm/drm_fb_helper.h       | 8 ++++++++
>  4 files changed, 19 insertions(+)
>
> diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c
> index b469624fe40d..816899a266ff 100644
> --- a/drivers/gpu/drm/bochs/bochs_drv.c
> +++ b/drivers/gpu/drm/bochs/bochs_drv.c
> @@ -132,6 +132,9 @@ static int bochs_pci_probe(struct pci_dev *pdev,
>                 goto err_unload;
>
>         drm_fbdev_generic_setup(dev, 32);
> +       if (dev->fb_helper)
> +               dev->fb_helper->no_dpms_blank = true;
> +
>         return ret;
>
>  err_unload:
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index f6baa2046124..b892f02ff2f1 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -332,9 +332,14 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
>   */
>  int drm_fb_helper_blank(int blank, struct fb_info *info)
>  {
> +       struct drm_fb_helper *fb_helper = info->par;
> +
>         if (oops_in_progress)
>                 return -EBUSY;
>
> +       if (fb_helper->no_dpms_blank)
> +               return -EINVAL;
> +
>         switch (blank) {
>         /* Display: On; HSync: On, VSync: On */
>         case FB_BLANK_UNBLANK:
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index 1864467f1063..58ecfaeed7c1 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -120,6 +120,9 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>                 goto modeset_cleanup;
>
>         drm_fbdev_generic_setup(&qdev->ddev, 32);
> +       if (qdev->fb_helper)
> +               qdev->fb_helper->no_dpms_blank = true;
> +
>         return 0;
>
>  modeset_cleanup:
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 3b273f9ca39a..151be4219c32 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -176,6 +176,14 @@ struct drm_fb_helper {
>          */
>         bool deferred_setup;
>
> +       /**
> +        * @no_dpms_blank:
> +        *
> +        * A flag indicating that the driver doesn't support blanking.
> +        * Then fbcon core code falls back to its generic handler.
> +        */
> +       bool no_dpms_blank;
> +
>         /**
>          * @preferred_bpp:
>          *
> --
> 2.26.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Takashi Iwai April 20, 2021, 4:41 p.m. UTC | #4 
On Fri, 16 Apr 2021 20:30:05 +0200,
Daniel Vetter wrote:
> 
> On Fri, Apr 16, 2021 at 2:53 PM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > Currently the DRM fbcon helper for console blank,
> > drm_fb_helper_blank(), simply calls drm_fb_helper_dpms() and always
> > returns zero, supposing the driver dealing with DPMS or atomic
> > crtc->active flip to handle blanking the screen.  It works on most of
> > devices, but broken on most of KVM/QEMU graphics: bochs, qxl and
> > cirrus drivers just ignore crtc->active state change as blanking (or
> > cirrus ignoring DPMS).  In practice, when you run like
> >   % setterm --blank force
> > on a VT console, the screen freezes without actually blanking.
> >
> > A simple fix for this problem would be not to rely on DPMS but let
> > fbcon performs the generic blank code.  This can be achieved just by
> > returning an error from drm_fb_helper_blank().
> >
> > In this patch, we add a flag, no_dpms_blank, to drm_fb_helper for
> > indicating that the driver doesn't handle blank via DPMS or
> > crtc->active flip.  When this flag is set, drm_fb_helper_blank()
> > simply returns an error, so that fbcon falls back to its generic blank
> > handler.  The flag is set to both bochs and qxl drivers in this patch,
> > while cirrus is left untouched as it's declared as to-be-deprecated.
> >
> > Link: https://lore.kernel.org/dri-devel/20170726205636.19144-1-tiwai@suse.de/
> > BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1095700
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> Uh we're supposed to fix these drivers to actually blank (scan out
> black, worst case), not paper over it in higher levels. Atomic kms is
> meant to be a somewhat useful abstraction. So now fbcon blanks, but
> your desktop still just freezes.
> 
> > ---
> >
> > Here I whip a dead horse again, revisiting the long-standing issue
> > stated in the previous patch set in 2017:
> >   https://lore.kernel.org/dri-devel/20170726205636.19144-1-tiwai@suse.de/
> >
> > I thought to refresh the previous patch set at first, but it seems
> > invalid for the atomic modeset case.  And for the atomic, it's even
> > more difficult to propagate the return from the bottom to up.
> > So I ended up with this approach as it's much simpler.
> 
> Yeah that's because atomic assume you can at least blank your screen to black.

OK, then there is no other way than fixing those drivers to deal with
the blanking...

I cooked up an experimental patch for bochs, and will submit later.


thanks,

Takashi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c
index b469624fe40d..816899a266ff 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -132,6 +132,9 @@  static int bochs_pci_probe(struct pci_dev *pdev,
 		goto err_unload;
 
 	drm_fbdev_generic_setup(dev, 32);
+	if (dev->fb_helper)
+		dev->fb_helper->no_dpms_blank = true;
+
 	return ret;
 
 err_unload:
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index f6baa2046124..b892f02ff2f1 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -332,9 +332,14 @@  static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
  */
 int drm_fb_helper_blank(int blank, struct fb_info *info)
 {
+	struct drm_fb_helper *fb_helper = info->par;
+
 	if (oops_in_progress)
 		return -EBUSY;
 
+	if (fb_helper->no_dpms_blank)
+		return -EINVAL;
+
 	switch (blank) {
 	/* Display: On; HSync: On, VSync: On */
 	case FB_BLANK_UNBLANK:
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index 1864467f1063..58ecfaeed7c1 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -120,6 +120,9 @@  qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto modeset_cleanup;
 
 	drm_fbdev_generic_setup(&qdev->ddev, 32);
+	if (qdev->fb_helper)
+		qdev->fb_helper->no_dpms_blank = true;
+
 	return 0;
 
 modeset_cleanup:
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 3b273f9ca39a..151be4219c32 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -176,6 +176,14 @@  struct drm_fb_helper {
 	 */
 	bool deferred_setup;
 
+	/**
+	 * @no_dpms_blank:
+	 *
+	 * A flag indicating that the driver doesn't support blanking.
+	 * Then fbcon core code falls back to its generic handler.
+	 */
+	bool no_dpms_blank;
+
 	/**
 	 * @preferred_bpp:
 	 *