| Message ID | 20190222071604.26024-2-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm & vgaarb: handle vgacon removal in vgaarb. | expand |
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 >
Hi, > > - /* > > - * 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. Ah, little but important difference I didn't notice on the first look. That wrapper calls the non-pci version. But seems it isn't that easy to switch over because the framebuffer is in stolen memory instead of a pci bar ... > > if (ret) { > > DRM_ERROR("failed to remove conflicting framebuffer drivers\n"); > > goto err_ggtt; > > } > > > > - ret = i915_kick_out_vgacon(dev_priv); ... but we can continue to just call vga_remove_vgacon() here. > 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). Ok, will do. cheers, Gerd
On Fri, Feb 22, 2019 at 12:03 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > > Hi, > > > > - /* > > > - * 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. > > Ah, little but important difference I didn't notice on the first look. > That wrapper calls the non-pci version. But seems it isn't that easy > to switch over because the framebuffer is in stolen memory instead of a > pci bar ... stolen memory is where the fb physically resides. the pci bar is how you access it (as long as you take all the pci bars). From a quick look i915 and pci version of remove_conflicting_fb matched. -Daniel > > > if (ret) { > > > DRM_ERROR("failed to remove conflicting framebuffer drivers\n"); > > > goto err_ggtt; > > > } > > > > > > - ret = i915_kick_out_vgacon(dev_priv); > > ... but we can continue to just call vga_remove_vgacon() here. > > > 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). > > Ok, will do. > > cheers, > Gerd >
On Fri, Feb 22, 2019 at 06:20:11PM +0100, Daniel Vetter wrote: > On Fri, Feb 22, 2019 at 12:03 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > Hi, > > > > > > - /* > > > > - * 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. > > > > Ah, little but important difference I didn't notice on the first look. > > That wrapper calls the non-pci version. But seems it isn't that easy > > to switch over because the framebuffer is in stolen memory instead of a > > pci bar ... > > stolen memory is where the fb physically resides. the pci bar is how > you access it (as long as you take all the pci bars). From a quick > look i915 and pci version of remove_conflicting_fb matched. Well, it is ap->ranges[0].base = ggtt->gmadr.start; ap->ranges[0].size = ggtt->mappable_end; vs. ap->ranges[0].base = pci_resource_start(pdev, res_id); ap->ranges[0].size = pci_resource_len(pdev, res_id); So not obvious that they are the same. At least to me, maybe it is a different story for someone knowing the i915 driver better. thanks, Gerd
On Mon, Feb 25, 2019 at 09:34:09AM +0100, Gerd Hoffmann wrote: > On Fri, Feb 22, 2019 at 06:20:11PM +0100, Daniel Vetter wrote: > > On Fri, Feb 22, 2019 at 12:03 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > > > Hi, > > > > > > > > - /* > > > > > - * 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. > > > > > > Ah, little but important difference I didn't notice on the first look. > > > That wrapper calls the non-pci version. But seems it isn't that easy > > > to switch over because the framebuffer is in stolen memory instead of a > > > pci bar ... > > > > stolen memory is where the fb physically resides. the pci bar is how > > you access it (as long as you take all the pci bars). From a quick > > look i915 and pci version of remove_conflicting_fb matched. > > Well, it is > > ap->ranges[0].base = ggtt->gmadr.start; > ap->ranges[0].size = ggtt->mappable_end; > > vs. > > ap->ranges[0].base = pci_resource_start(pdev, res_id); > ap->ranges[0].size = pci_resource_len(pdev, res_id); > > So not obvious that they are the same. At least to me, maybe it is a > different story for someone knowing the i915 driver better. It's one of the pci bars, so amounts to the same. But yeah not obvious if you don't know that. -Daniel
> > > stolen memory is where the fb physically resides. the pci bar is how > > > you access it (as long as you take all the pci bars). From a quick > > > look i915 and pci version of remove_conflicting_fb matched. > > > > Well, it is > > > > ap->ranges[0].base = ggtt->gmadr.start; > > ap->ranges[0].size = ggtt->mappable_end; > > > > vs. > > > > ap->ranges[0].base = pci_resource_start(pdev, res_id); > > ap->ranges[0].size = pci_resource_len(pdev, res_id); > > > > So not obvious that they are the same. At least to me, maybe it is a > > different story for someone knowing the i915 driver better. > > It's one of the pci bars, so amounts to the same. But yeah not obvious if > you don't know that. On my system (kaby lake) it happens to be pci bar 2, is that true for all igd devices? cheers, Gerd
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); 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); } +#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); + static inline void vga_irq_set_state(struct vga_device *vgadev, bool state) { if (vgadev->irq_set_state)
Also rename it and call it automatically from drm_fb_helper_remove_conflicting_pci_framebuffers() 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(-)