[PATCH v2 0/2] drm/qxl: kick out vgacon

[PATCH v2 0/2] drm/qxl: kick out vgacon

[Gerd Hoffmann]

v2: move the i915 code to drm_fb_helper and use that instead of
reinventing the wheel.

Gerd Hoffmann (2):
  drm: move i915_kick_out_vgacon to drm_fb_helper
  drm/qxl: kick out vgacon

 include/drm/drm_fb_helper.h     |  2 ++
 drivers/gpu/drm/drm_fb_helper.c | 39 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.c | 35 +----------------------------------
 drivers/gpu/drm/qxl/qxl_drv.c   |  1 +
 4 files changed, 43 insertions(+), 34 deletions(-)

-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Gerd Hoffmann]

It'll be useful for other drivers too, so move it to drm_fb_helper.c
(and rename it of course).  Also add docs.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/drm/drm_fb_helper.h     |  2 ++
 drivers/gpu/drm/drm_fb_helper.c | 39 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.c | 35 +----------------------------------
 3 files changed, 42 insertions(+), 34 deletions(-)

diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index bb9acea613..a401ba47ad 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -649,4 +649,6 @@ drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
 #endif
 }
 
+int drm_fb_helper_kick_out_vgacon(void);
+
 #endif
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 0e9349ff2d..a2d7e5bc51 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -35,6 +35,7 @@
 #include <linux/sysrq.h>
 #include <linux/slab.h>
 #include <linux/module.h>
+#include <linux/vt_kern.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_fb_helper.h>
@@ -3353,3 +3354,41 @@ int __init drm_fb_helper_modinit(void)
 	return 0;
 }
 EXPORT_SYMBOL(drm_fb_helper_modinit);
+
+/**
+ * drm_fb_helper_kick_out_vgacon - deactivate vgacon driver.
+ *
+ * Deactivate vgacon driver so it stops accessing vga io ports.
+ * Should be called after
+ * drm_fb_helper_remove_conflicting_pci_framebuffers().
+ *
+ * Returns:
+ * Zero on success or negative error code on failure.
+ */
+int drm_fb_helper_kick_out_vgacon(void)
+{
+#if !defined(CONFIG_VGA_CONSOLE)
+        return 0;
+#elif !defined(CONFIG_DUMMY_CONSOLE)
+        return -ENODEV;
+#else
+        int ret = 0;
+
+        DRM_INFO("Replacing VGA console driver\n");
+
+        console_lock();
+        if (con_is_bound(&vga_con))
+                ret = do_take_over_console(&dummy_con, 0, MAX_NR_CONSOLES - 1, 1);
+        if (ret == 0) {
+                ret = do_unregister_con_driver(&vga_con);
+
+                /* Ignore "already unregistered". */
+                if (ret == -ENODEV)
+                        ret = 0;
+        }
+        console_unlock();
+
+        return ret;
+#endif
+}
+EXPORT_SYMBOL(drm_fb_helper_kick_out_vgacon);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6630212f2f..3edd4d7d55 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -757,39 +757,6 @@ static int i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-#if !defined(CONFIG_VGA_CONSOLE)
-static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
-{
-	return 0;
-}
-#elif !defined(CONFIG_DUMMY_CONSOLE)
-static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
-{
-	return -ENODEV;
-}
-#else
-static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
-{
-	int ret = 0;
-
-	DRM_INFO("Replacing VGA console driver\n");
-
-	console_lock();
-	if (con_is_bound(&vga_con))
-		ret = do_take_over_console(&dummy_con, 0, MAX_NR_CONSOLES - 1, 1);
-	if (ret == 0) {
-		ret = do_unregister_con_driver(&vga_con);
-
-		/* Ignore "already unregistered". */
-		if (ret == -ENODEV)
-			ret = 0;
-	}
-	console_unlock();
-
-	return ret;
-}
-#endif
-
 static void intel_init_dpio(struct drm_i915_private *dev_priv)
 {
 	/*
@@ -1420,7 +1387,7 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
 		goto err_ggtt;
 	}
 
-	ret = i915_kick_out_vgacon(dev_priv);
+	ret = drm_fb_helper_kick_out_vgacon();
 	if (ret) {
 		DRM_ERROR("failed to remove conflicting VGA console\n");
 		goto err_ggtt;
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/2] drm/qxl: kick out vgacon

[Gerd Hoffmann]

Problem:  qxl switches from native mode back into vga compatibility mode
when it notices someone is accessing vga registers.  And vgacon does
exactly that before fbcon takes over.  So make sure we kick out vgacon
early enough that it wouldn't disturb us.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index bb81e310eb..08446561aa 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -95,6 +95,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto modeset_cleanup;
 
 	drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
+	drm_fb_helper_kick_out_vgacon();
 	drm_fbdev_generic_setup(&qdev->ddev, 32);
 	return 0;
 
-- 
2.9.3

[PATCH v2 2/2] drm/qxl: kick out vgacon

[Gerd Hoffmann]

Problem:  qxl switches from native mode back into vga compatibility mode
when it notices someone is accessing vga registers.  And vgacon does
exactly that before fbcon takes over.  So make sure we kick out vgacon
early enough that it wouldn't disturb us.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index bb81e310eb..08446561aa 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -95,6 +95,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto modeset_cleanup;
 
 	drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
+	drm_fb_helper_kick_out_vgacon();
 	drm_fbdev_generic_setup(&qdev->ddev, 32);
 	return 0;
 
-- 
2.9.3

[PATCH v2 2/2] drm/qxl: kick out vgacon

[Gerd Hoffmann]

Problem:  qxl switches from native mode back into vga compatibility mode
when it notices someone is accessing vga registers.  And vgacon does
exactly that before fbcon takes over.  So make sure we kick out vgacon
early enough that it wouldn't disturb us.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index bb81e310eb..08446561aa 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -95,6 +95,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto modeset_cleanup;
 
 	drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
+	drm_fb_helper_kick_out_vgacon();
 	drm_fbdev_generic_setup(&qdev->ddev, 32);
 	return 0;
 
-- 
2.9.3

Re: [PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Daniel Vetter]

On Thu, Feb 21, 2019 at 12:35:33PM +0100, Gerd Hoffmann wrote:
> It'll be useful for other drivers too, so move it to drm_fb_helper.c
> (and rename it of course).  Also add docs.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_fb_helper.h     |  2 ++
>  drivers/gpu/drm/drm_fb_helper.c | 39 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.c | 35 +----------------------------------
>  3 files changed, 42 insertions(+), 34 deletions(-)
> 
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index bb9acea613..a401ba47ad 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -649,4 +649,6 @@ drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
>  #endif
>  }
>  
> +int drm_fb_helper_kick_out_vgacon(void);
> +
>  #endif
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0e9349ff2d..a2d7e5bc51 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -35,6 +35,7 @@
>  #include <linux/sysrq.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> +#include <linux/vt_kern.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fb_helper.h>
> @@ -3353,3 +3354,41 @@ int __init drm_fb_helper_modinit(void)
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_modinit);
> +
> +/**
> + * drm_fb_helper_kick_out_vgacon - deactivate vgacon driver.
> + *
> + * Deactivate vgacon driver so it stops accessing vga io ports.
> + * Should be called after
> + * drm_fb_helper_remove_conflicting_pci_framebuffers().

Why after? i915 calls this before kicking out the fbdev drivers ...

Otherwise lgtm.
-Daniel

> + *
> + * Returns:
> + * Zero on success or negative error code on failure.
> + */
> +int drm_fb_helper_kick_out_vgacon(void)
> +{
> +#if !defined(CONFIG_VGA_CONSOLE)
> +        return 0;
> +#elif !defined(CONFIG_DUMMY_CONSOLE)
> +        return -ENODEV;
> +#else
> +        int ret = 0;
> +
> +        DRM_INFO("Replacing VGA console driver\n");
> +
> +        console_lock();
> +        if (con_is_bound(&vga_con))
> +                ret = do_take_over_console(&dummy_con, 0, MAX_NR_CONSOLES - 1, 1);
> +        if (ret == 0) {
> +                ret = do_unregister_con_driver(&vga_con);
> +
> +                /* Ignore "already unregistered". */
> +                if (ret == -ENODEV)
> +                        ret = 0;
> +        }
> +        console_unlock();
> +
> +        return ret;
> +#endif
> +}
> +EXPORT_SYMBOL(drm_fb_helper_kick_out_vgacon);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6630212f2f..3edd4d7d55 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -757,39 +757,6 @@ static int i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> -#if !defined(CONFIG_VGA_CONSOLE)
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> -	return 0;
> -}
> -#elif !defined(CONFIG_DUMMY_CONSOLE)
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> -	return -ENODEV;
> -}
> -#else
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> -	int ret = 0;
> -
> -	DRM_INFO("Replacing VGA console driver\n");
> -
> -	console_lock();
> -	if (con_is_bound(&vga_con))
> -		ret = do_take_over_console(&dummy_con, 0, MAX_NR_CONSOLES - 1, 1);
> -	if (ret == 0) {
> -		ret = do_unregister_con_driver(&vga_con);
> -
> -		/* Ignore "already unregistered". */
> -		if (ret == -ENODEV)
> -			ret = 0;
> -	}
> -	console_unlock();
> -
> -	return ret;
> -}
> -#endif
> -
>  static void intel_init_dpio(struct drm_i915_private *dev_priv)
>  {
>  	/*
> @@ -1420,7 +1387,7 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
>  		goto err_ggtt;
>  	}
>  
> -	ret = i915_kick_out_vgacon(dev_priv);
> +	ret = drm_fb_helper_kick_out_vgacon();
>  	if (ret) {
>  		DRM_ERROR("failed to remove conflicting VGA console\n");
>  		goto err_ggtt;
> -- 
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Daniel Vetter]

On Thu, Feb 21, 2019 at 12:35:34PM +0100, Gerd Hoffmann wrote:
> Problem:  qxl switches from native mode back into vga compatibility mode
> when it notices someone is accessing vga registers.  And vgacon does
> exactly that before fbcon takes over.  So make sure we kick out vgacon
> early enough that it wouldn't disturb us.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/gpu/drm/qxl/qxl_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index bb81e310eb..08446561aa 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -95,6 +95,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		goto modeset_cleanup;
>  
>  	drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
> +	drm_fb_helper_kick_out_vgacon();

I was thinking of checking whether pdev is a VGA class device and whether
it decodes vga access, and in that case automatically calling
kick_out_vgacon from remove_conflicting_pci_framebuffer. Since that's what
drivers want anyway, and those who don't can open code it.

Or is there an issue with that?
-Daniel

>  	drm_fbdev_generic_setup(&qdev->ddev, 32);
>  	return 0;
>  
> -- 
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Daniel Vetter]

On Thu, Feb 21, 2019 at 12:35:34PM +0100, Gerd Hoffmann wrote:
> Problem:  qxl switches from native mode back into vga compatibility mode
> when it notices someone is accessing vga registers.  And vgacon does
> exactly that before fbcon takes over.  So make sure we kick out vgacon
> early enough that it wouldn't disturb us.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/gpu/drm/qxl/qxl_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index bb81e310eb..08446561aa 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -95,6 +95,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		goto modeset_cleanup;
>  
>  	drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
> +	drm_fb_helper_kick_out_vgacon();

I was thinking of checking whether pdev is a VGA class device and whether
it decodes vga access, and in that case automatically calling
kick_out_vgacon from remove_conflicting_pci_framebuffer. Since that's what
drivers want anyway, and those who don't can open code it.

Or is there an issue with that?
-Daniel

>  	drm_fbdev_generic_setup(&qdev->ddev, 32);
>  	return 0;
>  
> -- 
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Daniel Vetter]

On Thu, Feb 21, 2019 at 12:35:34PM +0100, Gerd Hoffmann wrote:
> Problem:  qxl switches from native mode back into vga compatibility mode
> when it notices someone is accessing vga registers.  And vgacon does
> exactly that before fbcon takes over.  So make sure we kick out vgacon
> early enough that it wouldn't disturb us.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/gpu/drm/qxl/qxl_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index bb81e310eb..08446561aa 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -95,6 +95,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		goto modeset_cleanup;
>  
>  	drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
> +	drm_fb_helper_kick_out_vgacon();

I was thinking of checking whether pdev is a VGA class device and whether
it decodes vga access, and in that case automatically calling
kick_out_vgacon from remove_conflicting_pci_framebuffer. Since that's what
drivers want anyway, and those who don't can open code it.

Or is there an issue with that?
-Daniel

>  	drm_fbdev_generic_setup(&qdev->ddev, 32);
>  	return 0;
>  
> -- 
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Gerd Hoffmann]

> > +/**
> > + * drm_fb_helper_kick_out_vgacon - deactivate vgacon driver.
> > + *
> > + * Deactivate vgacon driver so it stops accessing vga io ports.
> > + * Should be called after
> > + * drm_fb_helper_remove_conflicting_pci_framebuffers().
> 
> Why after? i915 calls this before kicking out the fbdev drivers ...

No, it doesn't:

<quote>
	/*
	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
	 * otherwise the vga fbdev driver falls over.
	 */
	ret = i915_kick_out_firmware_fb(dev_priv);
	if (ret) {
		DRM_ERROR("failed to remove conflicting framebuffer drivers\n");
		goto err_ggtt;
	}

	ret = i915_kick_out_vgacon(dev_priv);
	if (ret) {
		DRM_ERROR("failed to remove conflicting VGA console\n");
		goto err_ggtt;
	}
</quote>

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Gerd Hoffmann]

On Thu, Feb 21, 2019 at 01:20:11PM +0100, Daniel Vetter wrote:
> On Thu, Feb 21, 2019 at 12:35:34PM +0100, Gerd Hoffmann wrote:
> > Problem:  qxl switches from native mode back into vga compatibility mode
> > when it notices someone is accessing vga registers.  And vgacon does
> > exactly that before fbcon takes over.  So make sure we kick out vgacon
> > early enough that it wouldn't disturb us.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  drivers/gpu/drm/qxl/qxl_drv.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> > index bb81e310eb..08446561aa 100644
> > --- a/drivers/gpu/drm/qxl/qxl_drv.c
> > +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> > @@ -95,6 +95,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  		goto modeset_cleanup;
> >  
> >  	drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
> > +	drm_fb_helper_kick_out_vgacon();
> 
> I was thinking of checking whether pdev is a VGA class device and whether
> it decodes vga access, and in that case automatically calling
> kick_out_vgacon from remove_conflicting_pci_framebuffer. Since that's what
> drivers want anyway, and those who don't can open code it.
> 
> Or is there an issue with that?

It'll need more careful testing to make sure we don't have unwanted side
effects when just doing it for everyone.  And I guess most drivers don't
care much because their hardware ignores vga port writes once they are
switched out of vga text mode.

Dunno why i915 needs this.

In case of qxl it is more a historical leftover.  The very first qxl
device revision had no explicit qxl command to switch from qxl native
mode back to vga compatibility mode, vga port access was used for that
instead.  It's long fixed, but the qxl device still has that quirk for
compatibility with very old guest drivers.

cheers,
  Gerd

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Gerd Hoffmann]

On Thu, Feb 21, 2019 at 01:20:11PM +0100, Daniel Vetter wrote:
> On Thu, Feb 21, 2019 at 12:35:34PM +0100, Gerd Hoffmann wrote:
> > Problem:  qxl switches from native mode back into vga compatibility mode
> > when it notices someone is accessing vga registers.  And vgacon does
> > exactly that before fbcon takes over.  So make sure we kick out vgacon
> > early enough that it wouldn't disturb us.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  drivers/gpu/drm/qxl/qxl_drv.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> > index bb81e310eb..08446561aa 100644
> > --- a/drivers/gpu/drm/qxl/qxl_drv.c
> > +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> > @@ -95,6 +95,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  		goto modeset_cleanup;
> >  
> >  	drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
> > +	drm_fb_helper_kick_out_vgacon();
> 
> I was thinking of checking whether pdev is a VGA class device and whether
> it decodes vga access, and in that case automatically calling
> kick_out_vgacon from remove_conflicting_pci_framebuffer. Since that's what
> drivers want anyway, and those who don't can open code it.
> 
> Or is there an issue with that?

It'll need more careful testing to make sure we don't have unwanted side
effects when just doing it for everyone.  And I guess most drivers don't
care much because their hardware ignores vga port writes once they are
switched out of vga text mode.

Dunno why i915 needs this.

In case of qxl it is more a historical leftover.  The very first qxl
device revision had no explicit qxl command to switch from qxl native
mode back to vga compatibility mode, vga port access was used for that
instead.  It's long fixed, but the qxl device still has that quirk for
compatibility with very old guest drivers.

cheers,
  Gerd

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Gerd Hoffmann]

On Thu, Feb 21, 2019 at 01:20:11PM +0100, Daniel Vetter wrote:
> On Thu, Feb 21, 2019 at 12:35:34PM +0100, Gerd Hoffmann wrote:
> > Problem:  qxl switches from native mode back into vga compatibility mode
> > when it notices someone is accessing vga registers.  And vgacon does
> > exactly that before fbcon takes over.  So make sure we kick out vgacon
> > early enough that it wouldn't disturb us.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  drivers/gpu/drm/qxl/qxl_drv.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> > index bb81e310eb..08446561aa 100644
> > --- a/drivers/gpu/drm/qxl/qxl_drv.c
> > +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> > @@ -95,6 +95,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  		goto modeset_cleanup;
> >  
> >  	drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
> > +	drm_fb_helper_kick_out_vgacon();
> 
> I was thinking of checking whether pdev is a VGA class device and whether
> it decodes vga access, and in that case automatically calling
> kick_out_vgacon from remove_conflicting_pci_framebuffer. Since that's what
> drivers want anyway, and those who don't can open code it.
> 
> Or is there an issue with that?

It'll need more careful testing to make sure we don't have unwanted side
effects when just doing it for everyone.  And I guess most drivers don't
care much because their hardware ignores vga port writes once they are
switched out of vga text mode.

Dunno why i915 needs this.

In case of qxl it is more a historical leftover.  The very first qxl
device revision had no explicit qxl command to switch from qxl native
mode back to vga compatibility mode, vga port access was used for that
instead.  It's long fixed, but the qxl device still has that quirk for
compatibility with very old guest drivers.

cheers,
  Gerd

_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Jani Nikula]

On Thu, 21 Feb 2019, Gerd Hoffmann <kraxel@redhat.com> wrote:
> It'll be useful for other drivers too, so move it to drm_fb_helper.c
> (and rename it of course).  Also add docs.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_fb_helper.h     |  2 ++
>  drivers/gpu/drm/drm_fb_helper.c | 39 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.c | 35 +----------------------------------
>  3 files changed, 42 insertions(+), 34 deletions(-)
>
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index bb9acea613..a401ba47ad 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -649,4 +649,6 @@ drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
>  #endif
>  }
>  
> +int drm_fb_helper_kick_out_vgacon(void);
> +
>  #endif
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0e9349ff2d..a2d7e5bc51 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -35,6 +35,7 @@
>  #include <linux/sysrq.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> +#include <linux/vt_kern.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fb_helper.h>
> @@ -3353,3 +3354,41 @@ int __init drm_fb_helper_modinit(void)
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_modinit);
> +
> +/**
> + * drm_fb_helper_kick_out_vgacon - deactivate vgacon driver.
> + *
> + * Deactivate vgacon driver so it stops accessing vga io ports.
> + * Should be called after
> + * drm_fb_helper_remove_conflicting_pci_framebuffers().
> + *
> + * Returns:
> + * Zero on success or negative error code on failure.
> + */
> +int drm_fb_helper_kick_out_vgacon(void)
> +{
> +#if !defined(CONFIG_VGA_CONSOLE)
> +        return 0;
> +#elif !defined(CONFIG_DUMMY_CONSOLE)
> +        return -ENODEV;
> +#else

Please retain the original way of keeping conditional compilation
outside of functions.

BR,
Jani.

> +        int ret = 0;
> +
> +        DRM_INFO("Replacing VGA console driver\n");
> +
> +        console_lock();
> +        if (con_is_bound(&vga_con))
> +                ret = do_take_over_console(&dummy_con, 0, MAX_NR_CONSOLES - 1, 1);
> +        if (ret == 0) {
> +                ret = do_unregister_con_driver(&vga_con);
> +
> +                /* Ignore "already unregistered". */
> +                if (ret == -ENODEV)
> +                        ret = 0;
> +        }
> +        console_unlock();
> +
> +        return ret;
> +#endif
> +}
> +EXPORT_SYMBOL(drm_fb_helper_kick_out_vgacon);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6630212f2f..3edd4d7d55 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -757,39 +757,6 @@ static int i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> -#if !defined(CONFIG_VGA_CONSOLE)
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> -	return 0;
> -}
> -#elif !defined(CONFIG_DUMMY_CONSOLE)
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> -	return -ENODEV;
> -}
> -#else
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> -	int ret = 0;
> -
> -	DRM_INFO("Replacing VGA console driver\n");
> -
> -	console_lock();
> -	if (con_is_bound(&vga_con))
> -		ret = do_take_over_console(&dummy_con, 0, MAX_NR_CONSOLES - 1, 1);
> -	if (ret == 0) {
> -		ret = do_unregister_con_driver(&vga_con);
> -
> -		/* Ignore "already unregistered". */
> -		if (ret == -ENODEV)
> -			ret = 0;
> -	}
> -	console_unlock();
> -
> -	return ret;
> -}
> -#endif
> -
>  static void intel_init_dpio(struct drm_i915_private *dev_priv)
>  {
>  	/*
> @@ -1420,7 +1387,7 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
>  		goto err_ggtt;
>  	}
>  
> -	ret = i915_kick_out_vgacon(dev_priv);
> +	ret = drm_fb_helper_kick_out_vgacon();
>  	if (ret) {
>  		DRM_ERROR("failed to remove conflicting VGA console\n");
>  		goto err_ggtt;

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Jani Nikula]

On Thu, 21 Feb 2019, Gerd Hoffmann <kraxel@redhat.com> wrote:
> It'll be useful for other drivers too, so move it to drm_fb_helper.c
> (and rename it of course).  Also add docs.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_fb_helper.h     |  2 ++
>  drivers/gpu/drm/drm_fb_helper.c | 39 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.c | 35 +----------------------------------
>  3 files changed, 42 insertions(+), 34 deletions(-)
>
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index bb9acea613..a401ba47ad 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -649,4 +649,6 @@ drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
>  #endif
>  }
>  
> +int drm_fb_helper_kick_out_vgacon(void);
> +
>  #endif
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0e9349ff2d..a2d7e5bc51 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -35,6 +35,7 @@
>  #include <linux/sysrq.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> +#include <linux/vt_kern.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fb_helper.h>
> @@ -3353,3 +3354,41 @@ int __init drm_fb_helper_modinit(void)
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_modinit);
> +
> +/**
> + * drm_fb_helper_kick_out_vgacon - deactivate vgacon driver.
> + *
> + * Deactivate vgacon driver so it stops accessing vga io ports.
> + * Should be called after
> + * drm_fb_helper_remove_conflicting_pci_framebuffers().
> + *
> + * Returns:
> + * Zero on success or negative error code on failure.
> + */
> +int drm_fb_helper_kick_out_vgacon(void)
> +{
> +#if !defined(CONFIG_VGA_CONSOLE)
> +        return 0;
> +#elif !defined(CONFIG_DUMMY_CONSOLE)
> +        return -ENODEV;
> +#else

Please retain the original way of keeping conditional compilation
outside of functions.

BR,
Jani.

> +        int ret = 0;
> +
> +        DRM_INFO("Replacing VGA console driver\n");
> +
> +        console_lock();
> +        if (con_is_bound(&vga_con))
> +                ret = do_take_over_console(&dummy_con, 0, MAX_NR_CONSOLES - 1, 1);
> +        if (ret == 0) {
> +                ret = do_unregister_con_driver(&vga_con);
> +
> +                /* Ignore "already unregistered". */
> +                if (ret == -ENODEV)
> +                        ret = 0;
> +        }
> +        console_unlock();
> +
> +        return ret;
> +#endif
> +}
> +EXPORT_SYMBOL(drm_fb_helper_kick_out_vgacon);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6630212f2f..3edd4d7d55 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -757,39 +757,6 @@ static int i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> -#if !defined(CONFIG_VGA_CONSOLE)
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> -	return 0;
> -}
> -#elif !defined(CONFIG_DUMMY_CONSOLE)
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> -	return -ENODEV;
> -}
> -#else
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> -	int ret = 0;
> -
> -	DRM_INFO("Replacing VGA console driver\n");
> -
> -	console_lock();
> -	if (con_is_bound(&vga_con))
> -		ret = do_take_over_console(&dummy_con, 0, MAX_NR_CONSOLES - 1, 1);
> -	if (ret == 0) {
> -		ret = do_unregister_con_driver(&vga_con);
> -
> -		/* Ignore "already unregistered". */
> -		if (ret == -ENODEV)
> -			ret = 0;
> -	}
> -	console_unlock();
> -
> -	return ret;
> -}
> -#endif
> -
>  static void intel_init_dpio(struct drm_i915_private *dev_priv)
>  {
>  	/*
> @@ -1420,7 +1387,7 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
>  		goto err_ggtt;
>  	}
>  
> -	ret = i915_kick_out_vgacon(dev_priv);
> +	ret = drm_fb_helper_kick_out_vgacon();
>  	if (ret) {
>  		DRM_ERROR("failed to remove conflicting VGA console\n");
>  		goto err_ggtt;

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Gerd Hoffmann]

On Thu, Feb 21, 2019 at 03:08:39PM +0200, Jani Nikula wrote:
> On Thu, 21 Feb 2019, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > It'll be useful for other drivers too, so move it to drm_fb_helper.c
> > (and rename it of course).  Also add docs.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  include/drm/drm_fb_helper.h     |  2 ++
> >  drivers/gpu/drm/drm_fb_helper.c | 39 +++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_drv.c | 35 +----------------------------------
> >  3 files changed, 42 insertions(+), 34 deletions(-)
> >
> > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> > index bb9acea613..a401ba47ad 100644
> > --- a/include/drm/drm_fb_helper.h
> > +++ b/include/drm/drm_fb_helper.h
> > @@ -649,4 +649,6 @@ drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
> >  #endif
> >  }
> >  
> > +int drm_fb_helper_kick_out_vgacon(void);
> > +
> >  #endif
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 0e9349ff2d..a2d7e5bc51 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -35,6 +35,7 @@
> >  #include <linux/sysrq.h>
> >  #include <linux/slab.h>
> >  #include <linux/module.h>
> > +#include <linux/vt_kern.h>
> >  #include <drm/drmP.h>
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_fb_helper.h>
> > @@ -3353,3 +3354,41 @@ int __init drm_fb_helper_modinit(void)
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL(drm_fb_helper_modinit);
> > +
> > +/**
> > + * drm_fb_helper_kick_out_vgacon - deactivate vgacon driver.
> > + *
> > + * Deactivate vgacon driver so it stops accessing vga io ports.
> > + * Should be called after
> > + * drm_fb_helper_remove_conflicting_pci_framebuffers().
> > + *
> > + * Returns:
> > + * Zero on success or negative error code on failure.
> > + */
> > +int drm_fb_helper_kick_out_vgacon(void)
> > +{
> > +#if !defined(CONFIG_VGA_CONSOLE)
> > +        return 0;
> > +#elif !defined(CONFIG_DUMMY_CONSOLE)
> > +        return -ENODEV;
> > +#else
> 
> Please retain the original way of keeping conditional compilation
> outside of functions.

Care to explain why that is better?

thanks,
  Gerd

Re: [PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Gerd Hoffmann]

On Thu, Feb 21, 2019 at 03:08:39PM +0200, Jani Nikula wrote:
> On Thu, 21 Feb 2019, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > It'll be useful for other drivers too, so move it to drm_fb_helper.c
> > (and rename it of course).  Also add docs.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  include/drm/drm_fb_helper.h     |  2 ++
> >  drivers/gpu/drm/drm_fb_helper.c | 39 +++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_drv.c | 35 +----------------------------------
> >  3 files changed, 42 insertions(+), 34 deletions(-)
> >
> > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> > index bb9acea613..a401ba47ad 100644
> > --- a/include/drm/drm_fb_helper.h
> > +++ b/include/drm/drm_fb_helper.h
> > @@ -649,4 +649,6 @@ drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
> >  #endif
> >  }
> >  
> > +int drm_fb_helper_kick_out_vgacon(void);
> > +
> >  #endif
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 0e9349ff2d..a2d7e5bc51 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -35,6 +35,7 @@
> >  #include <linux/sysrq.h>
> >  #include <linux/slab.h>
> >  #include <linux/module.h>
> > +#include <linux/vt_kern.h>
> >  #include <drm/drmP.h>
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_fb_helper.h>
> > @@ -3353,3 +3354,41 @@ int __init drm_fb_helper_modinit(void)
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL(drm_fb_helper_modinit);
> > +
> > +/**
> > + * drm_fb_helper_kick_out_vgacon - deactivate vgacon driver.
> > + *
> > + * Deactivate vgacon driver so it stops accessing vga io ports.
> > + * Should be called after
> > + * drm_fb_helper_remove_conflicting_pci_framebuffers().
> > + *
> > + * Returns:
> > + * Zero on success or negative error code on failure.
> > + */
> > +int drm_fb_helper_kick_out_vgacon(void)
> > +{
> > +#if !defined(CONFIG_VGA_CONSOLE)
> > +        return 0;
> > +#elif !defined(CONFIG_DUMMY_CONSOLE)
> > +        return -ENODEV;
> > +#else
> 
> Please retain the original way of keeping conditional compilation
> outside of functions.

Care to explain why that is better?

thanks,
  Gerd

Re: [PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Noralf Trønnes]

Den 21.02.2019 12.35, skrev Gerd Hoffmann:
> It'll be useful for other drivers too, so move it to drm_fb_helper.c
> (and rename it of course).  Also add docs.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_fb_helper.h     |  2 ++
>  drivers/gpu/drm/drm_fb_helper.c | 39 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.c | 35 +----------------------------------
>  3 files changed, 42 insertions(+), 34 deletions(-)
> 
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index bb9acea613..a401ba47ad 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -649,4 +649,6 @@ drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
>  #endif
>  }
>  
> +int drm_fb_helper_kick_out_vgacon(void);
> +

Don't you need a dummy version as well for this one, like how it's done
for the other functions, to cover the case when DRM_FBDEV_EMULATION is
not selected?

Noralf.

>  #endif
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0e9349ff2d..a2d7e5bc51 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -35,6 +35,7 @@
>  #include <linux/sysrq.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> +#include <linux/vt_kern.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fb_helper.h>
> @@ -3353,3 +3354,41 @@ int __init drm_fb_helper_modinit(void)
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_modinit);
> +
> +/**
> + * drm_fb_helper_kick_out_vgacon - deactivate vgacon driver.
> + *
> + * Deactivate vgacon driver so it stops accessing vga io ports.
> + * Should be called after
> + * drm_fb_helper_remove_conflicting_pci_framebuffers().
> + *
> + * Returns:
> + * Zero on success or negative error code on failure.
> + */
> +int drm_fb_helper_kick_out_vgacon(void)
> +{
> +#if !defined(CONFIG_VGA_CONSOLE)
> +        return 0;
> +#elif !defined(CONFIG_DUMMY_CONSOLE)
> +        return -ENODEV;
> +#else
> +        int ret = 0;
> +
> +        DRM_INFO("Replacing VGA console driver\n");
> +
> +        console_lock();
> +        if (con_is_bound(&vga_con))
> +                ret = do_take_over_console(&dummy_con, 0, MAX_NR_CONSOLES - 1, 1);
> +        if (ret == 0) {
> +                ret = do_unregister_con_driver(&vga_con);
> +
> +                /* Ignore "already unregistered". */
> +                if (ret == -ENODEV)
> +                        ret = 0;
> +        }
> +        console_unlock();
> +
> +        return ret;
> +#endif
> +}
> +EXPORT_SYMBOL(drm_fb_helper_kick_out_vgacon);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6630212f2f..3edd4d7d55 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -757,39 +757,6 @@ static int i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> -#if !defined(CONFIG_VGA_CONSOLE)
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> -	return 0;
> -}
> -#elif !defined(CONFIG_DUMMY_CONSOLE)
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> -	return -ENODEV;
> -}
> -#else
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> -	int ret = 0;
> -
> -	DRM_INFO("Replacing VGA console driver\n");
> -
> -	console_lock();
> -	if (con_is_bound(&vga_con))
> -		ret = do_take_over_console(&dummy_con, 0, MAX_NR_CONSOLES - 1, 1);
> -	if (ret == 0) {
> -		ret = do_unregister_con_driver(&vga_con);
> -
> -		/* Ignore "already unregistered". */
> -		if (ret == -ENODEV)
> -			ret = 0;
> -	}
> -	console_unlock();
> -
> -	return ret;
> -}
> -#endif
> -
>  static void intel_init_dpio(struct drm_i915_private *dev_priv)
>  {
>  	/*
> @@ -1420,7 +1387,7 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
>  		goto err_ggtt;
>  	}
>  
> -	ret = i915_kick_out_vgacon(dev_priv);
> +	ret = drm_fb_helper_kick_out_vgacon();
>  	if (ret) {
>  		DRM_ERROR("failed to remove conflicting VGA console\n");
>  		goto err_ggtt;
> 

Re: [PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Noralf Trønnes]

Den 21.02.2019 12.35, skrev Gerd Hoffmann:
> It'll be useful for other drivers too, so move it to drm_fb_helper.c
> (and rename it of course).  Also add docs.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_fb_helper.h     |  2 ++
>  drivers/gpu/drm/drm_fb_helper.c | 39 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.c | 35 +----------------------------------
>  3 files changed, 42 insertions(+), 34 deletions(-)
> 
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index bb9acea613..a401ba47ad 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -649,4 +649,6 @@ drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
>  #endif
>  }
>  
> +int drm_fb_helper_kick_out_vgacon(void);
> +

Don't you need a dummy version as well for this one, like how it's done
for the other functions, to cover the case when DRM_FBDEV_EMULATION is
not selected?

Noralf.

>  #endif
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0e9349ff2d..a2d7e5bc51 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -35,6 +35,7 @@
>  #include <linux/sysrq.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> +#include <linux/vt_kern.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fb_helper.h>
> @@ -3353,3 +3354,41 @@ int __init drm_fb_helper_modinit(void)
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_modinit);
> +
> +/**
> + * drm_fb_helper_kick_out_vgacon - deactivate vgacon driver.
> + *
> + * Deactivate vgacon driver so it stops accessing vga io ports.
> + * Should be called after
> + * drm_fb_helper_remove_conflicting_pci_framebuffers().
> + *
> + * Returns:
> + * Zero on success or negative error code on failure.
> + */
> +int drm_fb_helper_kick_out_vgacon(void)
> +{
> +#if !defined(CONFIG_VGA_CONSOLE)
> +        return 0;
> +#elif !defined(CONFIG_DUMMY_CONSOLE)
> +        return -ENODEV;
> +#else
> +        int ret = 0;
> +
> +        DRM_INFO("Replacing VGA console driver\n");
> +
> +        console_lock();
> +        if (con_is_bound(&vga_con))
> +                ret = do_take_over_console(&dummy_con, 0, MAX_NR_CONSOLES - 1, 1);
> +        if (ret == 0) {
> +                ret = do_unregister_con_driver(&vga_con);
> +
> +                /* Ignore "already unregistered". */
> +                if (ret == -ENODEV)
> +                        ret = 0;
> +        }
> +        console_unlock();
> +
> +        return ret;
> +#endif
> +}
> +EXPORT_SYMBOL(drm_fb_helper_kick_out_vgacon);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6630212f2f..3edd4d7d55 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -757,39 +757,6 @@ static int i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> -#if !defined(CONFIG_VGA_CONSOLE)
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> -	return 0;
> -}
> -#elif !defined(CONFIG_DUMMY_CONSOLE)
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> -	return -ENODEV;
> -}
> -#else
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> -	int ret = 0;
> -
> -	DRM_INFO("Replacing VGA console driver\n");
> -
> -	console_lock();
> -	if (con_is_bound(&vga_con))
> -		ret = do_take_over_console(&dummy_con, 0, MAX_NR_CONSOLES - 1, 1);
> -	if (ret == 0) {
> -		ret = do_unregister_con_driver(&vga_con);
> -
> -		/* Ignore "already unregistered". */
> -		if (ret == -ENODEV)
> -			ret = 0;
> -	}
> -	console_unlock();
> -
> -	return ret;
> -}
> -#endif
> -
>  static void intel_init_dpio(struct drm_i915_private *dev_priv)
>  {
>  	/*
> @@ -1420,7 +1387,7 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
>  		goto err_ggtt;
>  	}
>  
> -	ret = i915_kick_out_vgacon(dev_priv);
> +	ret = drm_fb_helper_kick_out_vgacon();
>  	if (ret) {
>  		DRM_ERROR("failed to remove conflicting VGA console\n");
>  		goto err_ggtt;
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Daniel Vetter]

On Thu, Feb 21, 2019 at 01:39:33PM +0100, Gerd Hoffmann wrote:
> > > +/**
> > > + * drm_fb_helper_kick_out_vgacon - deactivate vgacon driver.
> > > + *
> > > + * Deactivate vgacon driver so it stops accessing vga io ports.
> > > + * Should be called after
> > > + * drm_fb_helper_remove_conflicting_pci_framebuffers().
> > 
> > Why after? i915 calls this before kicking out the fbdev drivers ...
> 
> No, it doesn't:
> 
> <quote>
> 	/*
> 	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
> 	 * otherwise the vga fbdev driver falls over.
> 	 */
> 	ret = i915_kick_out_firmware_fb(dev_priv);
> 	if (ret) {
> 		DRM_ERROR("failed to remove conflicting framebuffer drivers\n");
> 		goto err_ggtt;
> 	}
> 
> 	ret = i915_kick_out_vgacon(dev_priv);
> 	if (ret) {
> 		DRM_ERROR("failed to remove conflicting VGA console\n");
> 		goto err_ggtt;
> 	}
> </quote>

/me collects special prize for failing to remove blinders before replying

And indeed this has reasons:

commit 0485c9dc24ec0939b42ca5104c0373297506b555 (tag: drm-intel-fixes-2014-11-19)
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Fri Nov 14 10:09:49 2014 +0100

    drm/i915: Kick fbdev before vgacon
    
    It's magic, but it seems to work.
    
    This fixes a regression introduced in
    
    commit 1bb9e632a0aeee1121e652ee4dc80e5e6f14bcd2
    Author: Daniel Vetter <daniel.vetter@ffwll.ch>
    Date:   Tue Jul 8 10:02:43 2014 +0200
    
        drm/i915: Only unbind vgacon, not other console drivers
    
    My best guess is that the vga fbdev driver falls over if we rip out
    parts of vgacon. Hooray.
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82439
    Cc: stable@vger.kernel.org (v3.16+)
    Reported-and-tested-by: Lv Zheng <lv.zheng@intel.com>
    Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
    Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
    Signed-off-by: Jani Nikula <jani.nikula@intel.com>

I think given that even more reasons that the pci helper should
automatically kick out all the things, in the right order. And perhaps
cite the above commit in the commit message somewhere.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Daniel Vetter]

On Thu, Feb 21, 2019 at 02:06:23PM +0100, Gerd Hoffmann wrote:
> On Thu, Feb 21, 2019 at 01:20:11PM +0100, Daniel Vetter wrote:
> > On Thu, Feb 21, 2019 at 12:35:34PM +0100, Gerd Hoffmann wrote:
> > > Problem:  qxl switches from native mode back into vga compatibility mode
> > > when it notices someone is accessing vga registers.  And vgacon does
> > > exactly that before fbcon takes over.  So make sure we kick out vgacon
> > > early enough that it wouldn't disturb us.
> > > 
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > ---
> > >  drivers/gpu/drm/qxl/qxl_drv.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> > > index bb81e310eb..08446561aa 100644
> > > --- a/drivers/gpu/drm/qxl/qxl_drv.c
> > > +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> > > @@ -95,6 +95,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > >  		goto modeset_cleanup;
> > >  
> > >  	drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
> > > +	drm_fb_helper_kick_out_vgacon();
> > 
> > I was thinking of checking whether pdev is a VGA class device and whether
> > it decodes vga access, and in that case automatically calling
> > kick_out_vgacon from remove_conflicting_pci_framebuffer. Since that's what
> > drivers want anyway, and those who don't can open code it.
> > 
> > Or is there an issue with that?
> 
> It'll need more careful testing to make sure we don't have unwanted side
> effects when just doing it for everyone.  And I guess most drivers don't
> care much because their hardware ignores vga port writes once they are
> switched out of vga text mode.
> 
> Dunno why i915 needs this.

The problem isn't loading, it's unloading again. If you boot with vgacon,
but no fbdev driver (which iirc also has magic code to throw out the vga
console), then when you unload your kms driver vgacon kicks back in. And a
pile of things go really sideways when that happens.

I have no idea whether it's just intel hw or maybe pci decoding or
something else, but seems like good practice to kick out all existing
drivers, to make sure they can never get at the hw again.

So don't think it'll hurt to do this for everyone. But yeah maybe we can
do that as a follow-up (and convert i915 over), dunno.
-Daniel


> In case of qxl it is more a historical leftover.  The very first qxl
> device revision had no explicit qxl command to switch from qxl native
> mode back to vga compatibility mode, vga port access was used for that
> instead.  It's long fixed, but the qxl device still has that quirk for
> compatibility with very old guest drivers.
> 
> cheers,
>   Gerd
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Daniel Vetter]

On Thu, Feb 21, 2019 at 02:06:23PM +0100, Gerd Hoffmann wrote:
> On Thu, Feb 21, 2019 at 01:20:11PM +0100, Daniel Vetter wrote:
> > On Thu, Feb 21, 2019 at 12:35:34PM +0100, Gerd Hoffmann wrote:
> > > Problem:  qxl switches from native mode back into vga compatibility mode
> > > when it notices someone is accessing vga registers.  And vgacon does
> > > exactly that before fbcon takes over.  So make sure we kick out vgacon
> > > early enough that it wouldn't disturb us.
> > > 
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > ---
> > >  drivers/gpu/drm/qxl/qxl_drv.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> > > index bb81e310eb..08446561aa 100644
> > > --- a/drivers/gpu/drm/qxl/qxl_drv.c
> > > +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> > > @@ -95,6 +95,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > >  		goto modeset_cleanup;
> > >  
> > >  	drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
> > > +	drm_fb_helper_kick_out_vgacon();
> > 
> > I was thinking of checking whether pdev is a VGA class device and whether
> > it decodes vga access, and in that case automatically calling
> > kick_out_vgacon from remove_conflicting_pci_framebuffer. Since that's what
> > drivers want anyway, and those who don't can open code it.
> > 
> > Or is there an issue with that?
> 
> It'll need more careful testing to make sure we don't have unwanted side
> effects when just doing it for everyone.  And I guess most drivers don't
> care much because their hardware ignores vga port writes once they are
> switched out of vga text mode.
> 
> Dunno why i915 needs this.

The problem isn't loading, it's unloading again. If you boot with vgacon,
but no fbdev driver (which iirc also has magic code to throw out the vga
console), then when you unload your kms driver vgacon kicks back in. And a
pile of things go really sideways when that happens.

I have no idea whether it's just intel hw or maybe pci decoding or
something else, but seems like good practice to kick out all existing
drivers, to make sure they can never get at the hw again.

So don't think it'll hurt to do this for everyone. But yeah maybe we can
do that as a follow-up (and convert i915 over), dunno.
-Daniel


> In case of qxl it is more a historical leftover.  The very first qxl
> device revision had no explicit qxl command to switch from qxl native
> mode back to vga compatibility mode, vga port access was used for that
> instead.  It's long fixed, but the qxl device still has that quirk for
> compatibility with very old guest drivers.
> 
> cheers,
>   Gerd
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Daniel Vetter]

On Thu, Feb 21, 2019 at 02:06:23PM +0100, Gerd Hoffmann wrote:
> On Thu, Feb 21, 2019 at 01:20:11PM +0100, Daniel Vetter wrote:
> > On Thu, Feb 21, 2019 at 12:35:34PM +0100, Gerd Hoffmann wrote:
> > > Problem:  qxl switches from native mode back into vga compatibility mode
> > > when it notices someone is accessing vga registers.  And vgacon does
> > > exactly that before fbcon takes over.  So make sure we kick out vgacon
> > > early enough that it wouldn't disturb us.
> > > 
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > ---
> > >  drivers/gpu/drm/qxl/qxl_drv.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> > > index bb81e310eb..08446561aa 100644
> > > --- a/drivers/gpu/drm/qxl/qxl_drv.c
> > > +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> > > @@ -95,6 +95,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > >  		goto modeset_cleanup;
> > >  
> > >  	drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
> > > +	drm_fb_helper_kick_out_vgacon();
> > 
> > I was thinking of checking whether pdev is a VGA class device and whether
> > it decodes vga access, and in that case automatically calling
> > kick_out_vgacon from remove_conflicting_pci_framebuffer. Since that's what
> > drivers want anyway, and those who don't can open code it.
> > 
> > Or is there an issue with that?
> 
> It'll need more careful testing to make sure we don't have unwanted side
> effects when just doing it for everyone.  And I guess most drivers don't
> care much because their hardware ignores vga port writes once they are
> switched out of vga text mode.
> 
> Dunno why i915 needs this.

The problem isn't loading, it's unloading again. If you boot with vgacon,
but no fbdev driver (which iirc also has magic code to throw out the vga
console), then when you unload your kms driver vgacon kicks back in. And a
pile of things go really sideways when that happens.

I have no idea whether it's just intel hw or maybe pci decoding or
something else, but seems like good practice to kick out all existing
drivers, to make sure they can never get at the hw again.

So don't think it'll hurt to do this for everyone. But yeah maybe we can
do that as a follow-up (and convert i915 over), dunno.
-Daniel


> In case of qxl it is more a historical leftover.  The very first qxl
> device revision had no explicit qxl command to switch from qxl native
> mode back to vga compatibility mode, vga port access was used for that
> instead.  It's long fixed, but the qxl device still has that quirk for
> compatibility with very old guest drivers.
> 
> cheers,
>   Gerd
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Jani Nikula]

On Thu, 21 Feb 2019, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Thu, Feb 21, 2019 at 03:08:39PM +0200, Jani Nikula wrote:
>> On Thu, 21 Feb 2019, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> > It'll be useful for other drivers too, so move it to drm_fb_helper.c
>> > (and rename it of course).  Also add docs.
>> >
>> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> > ---
>> >  include/drm/drm_fb_helper.h     |  2 ++
>> >  drivers/gpu/drm/drm_fb_helper.c | 39 +++++++++++++++++++++++++++++++++++++++
>> >  drivers/gpu/drm/i915/i915_drv.c | 35 +----------------------------------
>> >  3 files changed, 42 insertions(+), 34 deletions(-)
>> >
>> > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>> > index bb9acea613..a401ba47ad 100644
>> > --- a/include/drm/drm_fb_helper.h
>> > +++ b/include/drm/drm_fb_helper.h
>> > @@ -649,4 +649,6 @@ drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
>> >  #endif
>> >  }
>> >  
>> > +int drm_fb_helper_kick_out_vgacon(void);
>> > +
>> >  #endif
>> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> > index 0e9349ff2d..a2d7e5bc51 100644
>> > --- a/drivers/gpu/drm/drm_fb_helper.c
>> > +++ b/drivers/gpu/drm/drm_fb_helper.c
>> > @@ -35,6 +35,7 @@
>> >  #include <linux/sysrq.h>
>> >  #include <linux/slab.h>
>> >  #include <linux/module.h>
>> > +#include <linux/vt_kern.h>
>> >  #include <drm/drmP.h>
>> >  #include <drm/drm_crtc.h>
>> >  #include <drm/drm_fb_helper.h>
>> > @@ -3353,3 +3354,41 @@ int __init drm_fb_helper_modinit(void)
>> >  	return 0;
>> >  }
>> >  EXPORT_SYMBOL(drm_fb_helper_modinit);
>> > +
>> > +/**
>> > + * drm_fb_helper_kick_out_vgacon - deactivate vgacon driver.
>> > + *
>> > + * Deactivate vgacon driver so it stops accessing vga io ports.
>> > + * Should be called after
>> > + * drm_fb_helper_remove_conflicting_pci_framebuffers().
>> > + *
>> > + * Returns:
>> > + * Zero on success or negative error code on failure.
>> > + */
>> > +int drm_fb_helper_kick_out_vgacon(void)
>> > +{
>> > +#if !defined(CONFIG_VGA_CONSOLE)
>> > +        return 0;
>> > +#elif !defined(CONFIG_DUMMY_CONSOLE)
>> > +        return -ENODEV;
>> > +#else
>> 
>> Please retain the original way of keeping conditional compilation
>> outside of functions.
>
> Care to explain why that is better?

Prevalent and documented [1] kernel coding style. It's easier to see
what happens in each branch, and the compiler throws the alternatives
away anyway.

Patches that do code movement should focus on code movement. Any
additional changes should be separate, and justified separately. The
function rename and documentation are of course okay, and they're
mentioned in the commit message as they should.

BR,
Jani.


[1] Documentation/process/coding-style.rst

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Jani Nikula]

On Thu, 21 Feb 2019, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Thu, Feb 21, 2019 at 03:08:39PM +0200, Jani Nikula wrote:
>> On Thu, 21 Feb 2019, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> > It'll be useful for other drivers too, so move it to drm_fb_helper.c
>> > (and rename it of course).  Also add docs.
>> >
>> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> > ---
>> >  include/drm/drm_fb_helper.h     |  2 ++
>> >  drivers/gpu/drm/drm_fb_helper.c | 39 +++++++++++++++++++++++++++++++++++++++
>> >  drivers/gpu/drm/i915/i915_drv.c | 35 +----------------------------------
>> >  3 files changed, 42 insertions(+), 34 deletions(-)
>> >
>> > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>> > index bb9acea613..a401ba47ad 100644
>> > --- a/include/drm/drm_fb_helper.h
>> > +++ b/include/drm/drm_fb_helper.h
>> > @@ -649,4 +649,6 @@ drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
>> >  #endif
>> >  }
>> >  
>> > +int drm_fb_helper_kick_out_vgacon(void);
>> > +
>> >  #endif
>> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> > index 0e9349ff2d..a2d7e5bc51 100644
>> > --- a/drivers/gpu/drm/drm_fb_helper.c
>> > +++ b/drivers/gpu/drm/drm_fb_helper.c
>> > @@ -35,6 +35,7 @@
>> >  #include <linux/sysrq.h>
>> >  #include <linux/slab.h>
>> >  #include <linux/module.h>
>> > +#include <linux/vt_kern.h>
>> >  #include <drm/drmP.h>
>> >  #include <drm/drm_crtc.h>
>> >  #include <drm/drm_fb_helper.h>
>> > @@ -3353,3 +3354,41 @@ int __init drm_fb_helper_modinit(void)
>> >  	return 0;
>> >  }
>> >  EXPORT_SYMBOL(drm_fb_helper_modinit);
>> > +
>> > +/**
>> > + * drm_fb_helper_kick_out_vgacon - deactivate vgacon driver.
>> > + *
>> > + * Deactivate vgacon driver so it stops accessing vga io ports.
>> > + * Should be called after
>> > + * drm_fb_helper_remove_conflicting_pci_framebuffers().
>> > + *
>> > + * Returns:
>> > + * Zero on success or negative error code on failure.
>> > + */
>> > +int drm_fb_helper_kick_out_vgacon(void)
>> > +{
>> > +#if !defined(CONFIG_VGA_CONSOLE)
>> > +        return 0;
>> > +#elif !defined(CONFIG_DUMMY_CONSOLE)
>> > +        return -ENODEV;
>> > +#else
>> 
>> Please retain the original way of keeping conditional compilation
>> outside of functions.
>
> Care to explain why that is better?

Prevalent and documented [1] kernel coding style. It's easier to see
what happens in each branch, and the compiler throws the alternatives
away anyway.

Patches that do code movement should focus on code movement. Any
additional changes should be separate, and justified separately. The
function rename and documentation are of course okay, and they're
mentioned in the commit message as they should.

BR,
Jani.


[1] Documentation/process/coding-style.rst

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Gerd Hoffmann]

> > +int drm_fb_helper_kick_out_vgacon(void);
> > +
> 
> Don't you need a dummy version as well for this one, like how it's done
> for the other functions, to cover the case when DRM_FBDEV_EMULATION is
> not selected?

Good question.

I guess it makes sense to kick out vgacon even with CONFIG_FB=n.

But when integrating this into
drm_fb_helper_remove_conflicting_pci_framebuffers() as suggested by
Daniel this isn't going to fly ...

cheers,
  Gerd

Re: [PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Gerd Hoffmann]

> > +int drm_fb_helper_kick_out_vgacon(void);
> > +
> 
> Don't you need a dummy version as well for this one, like how it's done
> for the other functions, to cover the case when DRM_FBDEV_EMULATION is
> not selected?

Good question.

I guess it makes sense to kick out vgacon even with CONFIG_FB=n.

But when integrating this into
drm_fb_helper_remove_conflicting_pci_framebuffers() as suggested by
Daniel this isn't going to fly ...

cheers,
  Gerd

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Gerd Hoffmann]

  Hi,

> I was thinking of checking whether pdev is a VGA class device and whether
> it decodes vga access, and in that case automatically calling

How can I figure that?  Ok, class is easy, but decode?  pci.h offers
functions to set vga decode but not to get that info ...

thanks,
  Gerd

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Gerd Hoffmann]

  Hi,

> I was thinking of checking whether pdev is a VGA class device and whether
> it decodes vga access, and in that case automatically calling

How can I figure that?  Ok, class is easy, but decode?  pci.h offers
functions to set vga decode but not to get that info ...

thanks,
  Gerd

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Gerd Hoffmann]

  Hi,

> I was thinking of checking whether pdev is a VGA class device and whether
> it decodes vga access, and in that case automatically calling

How can I figure that?  Ok, class is easy, but decode?  pci.h offers
functions to set vga decode but not to get that info ...

thanks,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Daniel Vetter]

On Thu, Feb 21, 2019 at 4:11 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > I was thinking of checking whether pdev is a VGA class device and whether
> > it decodes vga access, and in that case automatically calling
>
> How can I figure that?  Ok, class is easy, but decode?  pci.h offers
> functions to set vga decode but not to get that info ...

PCI_COMMAND_MEM and PCI_COMMAND_IO. There doesn't seem to be any
separate bits really. That's at least what I've gleaned from vgaarb.c.
The magic legacy vga decode bits only seem to exist on bridges, maybe
we can extract that logic from vgaarb.c (yes this is all a bit
spiralling out of control).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Daniel Vetter]

On Thu, Feb 21, 2019 at 4:11 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > I was thinking of checking whether pdev is a VGA class device and whether
> > it decodes vga access, and in that case automatically calling
>
> How can I figure that?  Ok, class is easy, but decode?  pci.h offers
> functions to set vga decode but not to get that info ...

PCI_COMMAND_MEM and PCI_COMMAND_IO. There doesn't seem to be any
separate bits really. That's at least what I've gleaned from vgaarb.c.
The magic legacy vga decode bits only seem to exist on bridges, maybe
we can extract that logic from vgaarb.c (yes this is all a bit
spiralling out of control).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Daniel Vetter]

On Thu, Feb 21, 2019 at 4:11 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > I was thinking of checking whether pdev is a VGA class device and whether
> > it decodes vga access, and in that case automatically calling
>
> How can I figure that?  Ok, class is easy, but decode?  pci.h offers
> functions to set vga decode but not to get that info ...

PCI_COMMAND_MEM and PCI_COMMAND_IO. There doesn't seem to be any
separate bits really. That's at least what I've gleaned from vgaarb.c.
The magic legacy vga decode bits only seem to exist on bridges, maybe
we can extract that logic from vgaarb.c (yes this is all a bit
spiralling out of control).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Daniel Vetter]

On Thu, Feb 21, 2019 at 04:09:12PM +0100, Gerd Hoffmann wrote:
> > > +int drm_fb_helper_kick_out_vgacon(void);
> > > +
> > 
> > Don't you need a dummy version as well for this one, like how it's done
> > for the other functions, to cover the case when DRM_FBDEV_EMULATION is
> > not selected?
> 
> Good question.
> 
> I guess it makes sense to kick out vgacon even with CONFIG_FB=n.
> 
> But when integrating this into
> drm_fb_helper_remove_conflicting_pci_framebuffers() as suggested by
> Daniel this isn't going to fly ...

We need to put it into both versions of that function, or pull that
function out of the #ifdef.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Daniel Vetter]

On Thu, Feb 21, 2019 at 04:09:12PM +0100, Gerd Hoffmann wrote:
> > > +int drm_fb_helper_kick_out_vgacon(void);
> > > +
> > 
> > Don't you need a dummy version as well for this one, like how it's done
> > for the other functions, to cover the case when DRM_FBDEV_EMULATION is
> > not selected?
> 
> Good question.
> 
> I guess it makes sense to kick out vgacon even with CONFIG_FB=n.
> 
> But when integrating this into
> drm_fb_helper_remove_conflicting_pci_framebuffers() as suggested by
> Daniel this isn't going to fly ...

We need to put it into both versions of that function, or pull that
function out of the #ifdef.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Gerd Hoffmann]

  Hi,

> PCI_COMMAND_MEM and PCI_COMMAND_IO. There doesn't seem to be any
> separate bits really. That's at least what I've gleaned from vgaarb.c.
> The magic legacy vga decode bits only seem to exist on bridges, maybe
> we can extract that logic from vgaarb.c (yes this is all a bit
> spiralling out of control).

Also makes me wonder whenever vgaarb is the better place (compared to
drm_fb_helper).  Tried that, doesn't look too bad, should continue
working with CONFIG_FB=n.  Will send patches in a moment.

cheers,
  Gerd

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Gerd Hoffmann]

  Hi,

> PCI_COMMAND_MEM and PCI_COMMAND_IO. There doesn't seem to be any
> separate bits really. That's at least what I've gleaned from vgaarb.c.
> The magic legacy vga decode bits only seem to exist on bridges, maybe
> we can extract that logic from vgaarb.c (yes this is all a bit
> spiralling out of control).

Also makes me wonder whenever vgaarb is the better place (compared to
drm_fb_helper).  Tried that, doesn't look too bad, should continue
working with CONFIG_FB=n.  Will send patches in a moment.

cheers,
  Gerd

Re: [PATCH v2 2/2] drm/qxl: kick out vgacon

[Gerd Hoffmann]

  Hi,

> PCI_COMMAND_MEM and PCI_COMMAND_IO. There doesn't seem to be any
> separate bits really. That's at least what I've gleaned from vgaarb.c.
> The magic legacy vga decode bits only seem to exist on bridges, maybe
> we can extract that logic from vgaarb.c (yes this is all a bit
> spiralling out of control).

Also makes me wonder whenever vgaarb is the better place (compared to
drm_fb_helper).  Tried that, doesn't look too bad, should continue
working with CONFIG_FB=n.  Will send patches in a moment.

cheers,
  Gerd