Re: [PATCH] drm/i915: Kick out vga console

[David Herrmann <dh.herrmann@gmail.com>]

Hi

On Wed, Jun 4, 2014 at 2:20 PM, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 04 Jun 2014, David Herrmann <dh.herrmann@gmail.com> wrote:
>> You rely on compiler-optimizations here. "dummy_con" is not available
>> if !CONFIG_DUMMY_CONSOLE, but you use it. This causes linker-failure
>> if dead-code elimination is not done (-O0).
>
> Nested #ifs too. How about
>
> #if !defined(CONFIG_VGA_CONSOLE)
> static inline int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> {
>         return 0;
> }
> #elif !defined(CONFIG_DUMMY_CONSOLE)
> static inline 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)
> {
>         /* ... */
> }
> #endif
>
> in proper kernel style?

Or even shorter:

#if defined(CONFIG_VGA_CONSOLE) && defined(CONFIG_DUMMY_CONSOLE)
static inline int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
{
        ...
}
#else
static inline int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
{
        return IS_ENABLED(CONFIG_VGA_CONSOLE) ? -ENODEV : 0;
}
#endif

Thanks
David

On Wed, Jun 4, 2014 at 2:20 PM, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 04 Jun 2014, David Herrmann <dh.herrmann@gmail.com> wrote:
>> Hi
>>
>> On Wed, Jun 4, 2014 at 12:57 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>>
>>> Touching the VGA resources on an IVB EFI machine causes hard hangs when
>>> we then kick out the efifb. Ouch.
>>>
>>> Apparently this also prevents unclaimed register errors on hsw and
>>> hard machine hangs on my i855gm when trying to unbind fbcon.
>>>
>>> Also, we want this to make I915_FBDEV=n safe.
>>>
>>> v2: Rebase and pimp commit message.
>>>
>>> v3: We also need to unregister the vga console, otherwise the unbind
>>> of the fb console before module unload might resurrect it again.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?idg813
>>> Cc: David Herrmann <dh.herrmann@gmail.com>
>>> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
>>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> Cc: linux-fbdev@vger.kernel.org
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> ---
>>>  drivers/gpu/drm/i915/i915_dma.c  | 34 +++++++++++++++++++++++++++++++++-
>>>  drivers/video/console/dummycon.c |  1 +
>>>  drivers/video/console/vgacon.c   |  1 +
>>>  3 files changed, 35 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>>> index b9159ade5e85..a4df80740b37 100644
>>> --- a/drivers/gpu/drm/i915/i915_dma.c
>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>>> @@ -36,6 +36,8 @@
>>>  #include "i915_drv.h"
>>>  #include "i915_trace.h"
>>>  #include <linux/pci.h>
>>> +#include <linux/console.h>
>>> +#include <linux/vt.h>
>>>  #include <linux/vgaarb.h>
>>>  #include <linux/acpi.h>
>>>  #include <linux/pnp.h>
>>> @@ -1450,6 +1452,29 @@ static void i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
>>>  }
>>>  #endif
>>>
>>> +static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
>>> +{
>>> +#if !defined(CONFIG_VGA_CONSOLE)
>>> +       return 0;
>>> +#else
>>> +       int ret;
>>> +
>>> +#if !defined(CONFIG_DUMMY_CONSOLE)
>>> +       return -ENODEV;
>>> +#endif
>>> +
>>> +       DRM_INFO("Replacing VGA console driver\n");
>>> +
>>> +       console_lock();
>>> +       ret = do_take_over_console(&dummy_con, 0, MAX_NR_CONSOLES - 1, 1);
>>
>> You rely on compiler-optimizations here. "dummy_con" is not available
>> if !CONFIG_DUMMY_CONSOLE, but you use it. This causes linker-failure
>> if dead-code elimination is not done (-O0).
>
> Nested #ifs too. How about
>
> #if !defined(CONFIG_VGA_CONSOLE)
> static inline int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> {
>         return 0;
> }
> #elif !defined(CONFIG_DUMMY_CONSOLE)
> static inline 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)
> {
>         /* ... */
> }
> #endif
>
> in proper kernel style?
>
> BR,
> Jani.
>
>
>>
>> Thanks
>> David
>>
>>> +       if (ret = 0)
>>> +               ret = do_unregister_con_driver(&vga_con);
>>> +       console_unlock();
>>> +
>>> +       return ret;
>>> +#endif
>>> +}
>>> +
>>>  static void i915_dump_device_info(struct drm_i915_private *dev_priv)
>>>  {
>>>         const struct intel_device_info *info = &dev_priv->info;
>>> @@ -1623,8 +1648,15 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>>         if (ret)
>>>                 goto out_regs;
>>>
>>> -       if (drm_core_check_feature(dev, DRIVER_MODESET))
>>> +       if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>>> +               ret = i915_kick_out_vgacon(dev_priv);
>>> +               if (ret) {
>>> +                       DRM_ERROR("failed to remove conflicting VGA console\n");
>>> +                       goto out_gtt;
>>> +               }
>>> +
>>>                 i915_kick_out_firmware_fb(dev_priv);
>>> +       }
>>>
>>>         pci_set_master(dev->pdev);
>>>
>>> diff --git a/drivers/video/console/dummycon.c b/drivers/video/console/dummycon.c
>>> index b63860f7beab..40bec8d64b0a 100644
>>> --- a/drivers/video/console/dummycon.c
>>> +++ b/drivers/video/console/dummycon.c
>>> @@ -77,3 +77,4 @@ const struct consw dummy_con = {
>>>      .con_set_palette = DUMMY,
>>>      .con_scrolldelta = DUMMY,
>>>  };
>>> +EXPORT_SYMBOL_GPL(dummy_con);
>>> diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
>>> index 9d8feac67637..84acd6223dc5 100644
>>> --- a/drivers/video/console/vgacon.c
>>> +++ b/drivers/video/console/vgacon.c
>>> @@ -1440,5 +1440,6 @@ const struct consw vga_con = {
>>>         .con_build_attr = vgacon_build_attr,
>>>         .con_invert_region = vgacon_invert_region,
>>>  };
>>> +EXPORT_SYMBOL(vga_con);
>>>
>>>  MODULE_LICENSE("GPL");
>>> --
>>> 1.8.1.4
>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Jani Nikula, Intel Open Source Technology Center