Re: [PATCH v4 1/2] drm: move i915_kick_out_vgacon to vgaarb

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Feb 22, 2019 at 08:16:03AM +0100, Gerd Hoffmann wrote:
> Also rename it and call it automatically from
> drm_fb_helper_remove_conflicting_pci_framebuffers()

Yeah this looks neat.

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_fb_helper.h     | 14 +++++++++++---
>  include/linux/vgaarb.h          |  2 ++
>  drivers/gpu/drm/i915/i915_drv.c | 43 -----------------------------------------
>  drivers/gpu/vga/vgaarb.c        | 38 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 51 insertions(+), 46 deletions(-)
> 
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index bb9acea61369..286d58efed5d 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -36,6 +36,7 @@ struct drm_fb_helper;
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_device.h>
>  #include <linux/kgdb.h>
> +#include <linux/vgaarb.h>
>  
>  enum mode_set_atomic {
>  	LEAVE_ATOMIC_MODE_SET,
> @@ -642,11 +643,18 @@ drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
>  						  int resource_id,
>  						  const char *name)
>  {
> +	int ret = 0;
> +
> +	/*
> +	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
> +	 * otherwise the vga fbdev driver falls over.
> +	 */
>  #if IS_REACHABLE(CONFIG_FB)
> -	return remove_conflicting_pci_framebuffers(pdev, resource_id, name);
> -#else
> -	return 0;
> +	ret = remove_conflicting_pci_framebuffers(pdev, resource_id, name);
>  #endif
> +	if (ret == 0)
> +		ret = vga_remove_vgacon(pdev);
> +	return ret;
>  }
>  
>  #endif
> diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
> index ee162e3e879b..553b34c8b5f7 100644
> --- a/include/linux/vgaarb.h
> +++ b/include/linux/vgaarb.h
> @@ -125,9 +125,11 @@ extern void vga_put(struct pci_dev *pdev, unsigned int rsrc);
>  #ifdef CONFIG_VGA_ARB
>  extern struct pci_dev *vga_default_device(void);
>  extern void vga_set_default_device(struct pci_dev *pdev);
> +extern int vga_remove_vgacon(struct pci_dev *pdev);
>  #else
>  static inline struct pci_dev *vga_default_device(void) { return NULL; };
>  static inline void vga_set_default_device(struct pci_dev *pdev) { };
> +static inline int vga_remove_vgacon(struct pci_dev *pdev) { return 0; };
>  #endif
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6630212f2faf..0e87eef542da 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)
>  {
>  	/*
> @@ -1410,22 +1377,12 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
>  	if (ret)
>  		goto err_perf;
>  
> -	/*
> -	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
> -	 * otherwise the vga fbdev driver falls over.
> -	 */
>  	ret = i915_kick_out_firmware_fb(dev_priv);

This needs to be replaced with a call to
drm_fb_helper_remove_conflicting_pci_framebuffers, because the above
wrapper hasn't been converted yet. Otherwise you end up removing the
vgacon unbind from i915.
>  	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;
> -	}
> -
>  	ret = i915_ggtt_init_hw(dev_priv);
>  	if (ret)
>  		goto err_ggtt;
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index dc8e039bfab5..524092a43de6 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -48,6 +48,8 @@
>  #include <linux/miscdevice.h>
>  #include <linux/slab.h>
>  #include <linux/screen_info.h>
> +#include <linux/vt.h>
> +#include <linux/console.h>
>  
>  #include <linux/uaccess.h>
>  
> @@ -168,6 +170,42 @@ void vga_set_default_device(struct pci_dev *pdev)
>  	vga_default = pci_dev_get(pdev);
>  }
>  

kerneldoc would be nice here.

> +#if !defined(CONFIG_VGA_CONSOLE)
> +int vga_remove_vgacon(struct pci_dev *pdev)
> +{
> +        return 0;
> +}
> +#elif !defined(CONFIG_DUMMY_CONSOLE)
> +int vga_remove_vgacon(struct pci_dev *pdev)
> +{
> +        return -ENODEV;
> +}
> +#else
> +int vga_remove_vgacon(struct pci_dev *pdev)
> +{
> +        int ret = 0;
> +
> +	if (pdev != vga_default)
> +		return 0;
> +	vgaarb_info(&pdev->dev, "deactivate vga console\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(vga_remove_vgacon);
> +

Asides from the comments, lgtm. Of course we'll need intel-gfx-ci to
approve too :-) Please cc intel-gfx on the next version for the entire
patcheset (our CI doesn't pick up incomplete patchesets).

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  static inline void vga_irq_set_state(struct vga_device *vgadev, bool state)
>  {
>  	if (vgadev->irq_set_state)
> -- 
> 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