On Thu, 21 Feb 2019, Gerd Hoffmann <[email protected]> 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 <[email protected]>
> ---
> 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
On Thu, Feb 21, 2019 at 03:08:39PM +0200, Jani Nikula wrote:
> On Thu, 21 Feb 2019, Gerd Hoffmann <[email protected]> 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 <[email protected]>
> > ---
> > 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
On Thu, 21 Feb 2019, Gerd Hoffmann <[email protected]> wrote:
> On Thu, Feb 21, 2019 at 03:08:39PM +0200, Jani Nikula wrote:
>> On Thu, 21 Feb 2019, Gerd Hoffmann <[email protected]> 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 <[email protected]>
>> > ---
>> > 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