diff mbox series

[v5,7/7] drm/i915: Implement fbdev emulation as in-kernel client

Message ID 20230927102808.18650-8-tzimmermann@suse.de (mailing list archive)
State New, archived
Headers show
Series drm/i915: Convert fbdev to DRM client | expand

Commit Message

Thomas Zimmermann Sept. 27, 2023, 10:26 a.m. UTC
Replace all code that initializes or releases fbdev emulation
throughout the driver. Instead initialize the fbdev client by a
single call to i915_fbdev_setup() after i915 has registered its
DRM device. Just like similar code in other drivers, i915 fbdev
emulation now acts as a regular DRM client.

The fbdev client setup consists of the initial preparation and the
hot-plugging of the display. The latter creates the fbdev device
and sets up the fbdev framebuffer. The setup performs display
hot-plugging once. If no display can be detected, DRM probe helpers
re-run the detection on each hotplug event.

A call to drm_dev_unregister() releases the client automatically.
No further action is required within i915. If the fbdev framebuffer
has been fully set up, struct fb_ops.fb_destroy implements the
release. For partially initialized emulation, the fbdev client
reverts the initial setup.

v3:
	* as before, silently ignore devices without displays
v2:
	* let drm_client_register() handle initial hotplug
	* fix driver name in error message (Jani)
	* fix non-fbdev build (kernel test robot)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/i915/display/intel_display.c  |   1 -
 .../drm/i915/display/intel_display_driver.c   |  18 --
 drivers/gpu/drm/i915/display/intel_fbdev.c    | 180 ++++++++----------
 drivers/gpu/drm/i915/display/intel_fbdev.h    |  20 +-
 drivers/gpu/drm/i915/i915_driver.c            |   2 +
 5 files changed, 83 insertions(+), 138 deletions(-)

Comments

Hogander, Jouni Oct. 25, 2023, 11:36 a.m. UTC | #1
Hi Thomas, couple of minor comments and a question below.

On Wed, 2023-09-27 at 12:26 +0200, Thomas Zimmermann wrote:
> Replace all code that initializes or releases fbdev emulation
> throughout the driver. Instead initialize the fbdev client by a
> single call to i915_fbdev_setup() after i915 has registered its

intel_fbdev_setup

> DRM device. Just like similar code in other drivers, i915 fbdev
> emulation now acts as a regular DRM client.
> 
> The fbdev client setup consists of the initial preparation and the
> hot-plugging of the display. The latter creates the fbdev device
> and sets up the fbdev framebuffer. The setup performs display
> hot-plugging once. If no display can be detected, DRM probe helpers
> re-run the detection on each hotplug event.
> 
> A call to drm_dev_unregister() releases the client automatically.
> No further action is required within i915. If the fbdev framebuffer
> has been fully set up, struct fb_ops.fb_destroy implements the
> release. For partially initialized emulation, the fbdev client
> reverts the initial setup.
> 
> v3:
>         * as before, silently ignore devices without displays
> v2:
>         * let drm_client_register() handle initial hotplug
>         * fix driver name in error message (Jani)
>         * fix non-fbdev build (kernel test robot)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  |   1 -
>  .../drm/i915/display/intel_display_driver.c   |  18 --
>  drivers/gpu/drm/i915/display/intel_fbdev.c    | 180 ++++++++--------
> --
>  drivers/gpu/drm/i915/display/intel_fbdev.h    |  20 +-
>  drivers/gpu/drm/i915/i915_driver.c            |   2 +
>  5 files changed, 83 insertions(+), 138 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index edbcf5968804d..7efa8d2787c39 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -81,7 +81,6 @@
>  #include "intel_dvo.h"
>  #include "intel_fb.h"
>  #include "intel_fbc.h"
> -#include "intel_fbdev.h"
>  #include "intel_fdi.h"
>  #include "intel_fifo_underrun.h"
>  #include "intel_frontbuffer.h"
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c
> b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index ffdcddd1943e0..213a4ee93ffc2 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -364,10 +364,6 @@ int intel_display_driver_probe(struct
> drm_i915_private *i915)
>  
>         intel_overlay_setup(i915);
>  
> -       ret = intel_fbdev_init(&i915->drm);
> -       if (ret)
> -               return ret;
> -
>         /* Only enable hotplug handling once the fbdev is fully set
> up. */
>         intel_hpd_init(i915);
>         intel_hpd_poll_disable(i915);
> @@ -392,16 +388,6 @@ void intel_display_driver_register(struct
> drm_i915_private *i915)
>  
>         intel_display_debugfs_register(i915);
>  
> -       /*
> -        * Some ports require correctly set-up hpd registers for
> -        * detection to work properly (leading to ghost connected
> -        * connector status), e.g. VGA on gm45.  Hence we can only
> set
> -        * up the initial fbdev config after hpd irqs are fully
> -        * enabled. We do it last so that the async config cannot run
> -        * before the connectors are registered.
> -        */
> -       intel_fbdev_initial_config_async(i915);
> -
>         /*
>          * We need to coordinate the hotplugs with the asynchronous
>          * fbdev configuration, for which we use the
> @@ -445,9 +431,6 @@ void intel_display_driver_remove_noirq(struct
> drm_i915_private *i915)
>          */
>         intel_hpd_poll_fini(i915);
>  
> -       /* poll work can call into fbdev, hence clean that up
> afterwards */
> -       intel_fbdev_fini(i915);
> -
>         intel_unregister_dsm_handler();
>  
>         /* flush any delayed tasks or pending work */
> @@ -484,7 +467,6 @@ void intel_display_driver_unregister(struct
> drm_i915_private *i915)
>         if (!HAS_DISPLAY(i915))
>                 return;
>  
> -       intel_fbdev_unregister(i915);
>         intel_audio_deinit(i915);
>  
>         /*
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
> b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 39de61d4e7906..100a4aaf1b7e4 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -24,7 +24,6 @@
>   *     David Airlie
>   */
>  
> -#include <linux/async.h>
>  #include <linux/console.h>
>  #include <linux/delay.h>
>  #include <linux/errno.h>
> @@ -39,6 +38,7 @@
>  #include <linux/vga_switcheroo.h>
>  
>  #include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
> @@ -58,7 +58,6 @@ struct intel_fbdev {
>         struct intel_framebuffer *fb;
>         struct i915_vma *vma;
>         unsigned long vma_flags;
> -       async_cookie_t cookie;
>         int preferred_bpp;
>  
>         /* Whether or not fbdev hpd processing is temporarily
> suspended */
> @@ -135,6 +134,26 @@ static int intel_fbdev_mmap(struct fb_info
> *info, struct vm_area_struct *vma)
>         return i915_gem_fb_mmap(obj, vma);
>  }
>  
> +static void intel_fbdev_fb_destroy(struct fb_info *info)
> +{
> +       struct drm_fb_helper *fb_helper = info->par;
> +       struct intel_fbdev *ifbdev = container_of(fb_helper, struct
> intel_fbdev, helper);
> +
> +       drm_fb_helper_fini(&ifbdev->helper);
> +
> +       /*
> +        * We rely on the object-free to release the VMA pinning for
> +        * the info->screen_base mmaping. Leaking the VMA is simpler
> than
> +        * trying to rectify all the possible error paths leading
> here.
> +        */
> +       intel_unpin_fb_vma(ifbdev->vma, ifbdev->vma_flags);
> +       drm_framebuffer_remove(&ifbdev->fb->base);
> +
> +       drm_client_release(&fb_helper->client);
> +       drm_fb_helper_unprepare(&ifbdev->helper);
> +       kfree(ifbdev);
> +}
> +
>  static const struct fb_ops intelfb_ops = {
>         .owner = THIS_MODULE,
>         __FB_DEFAULT_DEFERRED_OPS_RDWR(intel_fbdev),
> @@ -144,6 +163,7 @@ static const struct fb_ops intelfb_ops = {
>         .fb_pan_display = intel_fbdev_pan_display,
>         __FB_DEFAULT_DEFERRED_OPS_DRAW(intel_fbdev),
>         .fb_mmap = intel_fbdev_mmap,
> +       .fb_destroy = intel_fbdev_fb_destroy,
>  };
>  
>  static int intelfb_alloc(struct drm_fb_helper *helper,
> @@ -212,7 +232,6 @@ static int intelfb_create(struct drm_fb_helper
> *helper,
>         struct intel_framebuffer *intel_fb = ifbdev->fb;
>         struct drm_device *dev = helper->dev;
>         struct drm_i915_private *dev_priv = to_i915(dev);
> -       struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
>         struct i915_ggtt *ggtt = to_gt(dev_priv)->ggtt;
>         const struct i915_gtt_view view = {
>                 .type = I915_GTT_VIEW_NORMAL,
> @@ -337,7 +356,7 @@ static int intelfb_create(struct drm_fb_helper
> *helper,
>         ifbdev->vma_flags = flags;
>  
>         intel_runtime_pm_put(&dev_priv->runtime_pm, wakeref);
> -       vga_switcheroo_client_fb_set(pdev, info);
> +
>         return 0;
>  
>  out_unpin:
> @@ -363,26 +382,6 @@ static const struct drm_fb_helper_funcs
> intel_fb_helper_funcs = {
>         .fb_dirty = intelfb_dirty,
>  };
>  
> -static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
> -{
> -       /* We rely on the object-free to release the VMA pinning for
> -        * the info->screen_base mmaping. Leaking the VMA is simpler
> than
> -        * trying to rectify all the possible error paths leading
> here.
> -        */
> -
> -       drm_fb_helper_fini(&ifbdev->helper);
> -
> -       if (ifbdev->vma)
> -               intel_unpin_fb_vma(ifbdev->vma, ifbdev->vma_flags);
> -
> -       if (ifbdev->fb)
> -               drm_framebuffer_remove(&ifbdev->fb->base);
> -
> -       drm_client_release(&ifbdev->helper.client);
> -       drm_fb_helper_unprepare(&ifbdev->helper);
> -       kfree(ifbdev);
> -}
> -
>  /*
>   * Build an intel_fbdev struct using a BIOS allocated framebuffer,
> if possible.
>   * The core display code will have read out the current plane
> configuration,
> @@ -546,16 +545,6 @@ static void intel_fbdev_suspend_worker(struct
> work_struct *work)
>                                 true);
>  }
>  
> -static void intel_fbdev_sync(struct intel_fbdev *ifbdev)
> -{
> -       if (!ifbdev->cookie)
> -               return;
> -
> -       /* Only serialises with all preceding async calls, hence +1
> */
> -       async_synchronize_cookie(ifbdev->cookie + 1);
> -       ifbdev->cookie = 0;
> -}
> -
>  /* Suspends/resumes fbdev processing of incoming HPD events. When
> resuming HPD
>   * processing, fbdev will perform a full connector reprobe if a
> hotplug event
>   * was received while HPD was suspended.
> @@ -646,8 +635,6 @@ static void
> intel_fbdev_output_poll_changed(struct drm_device *dev)
>         if (!ifbdev)
>                 return;
>  
> -       intel_fbdev_sync(ifbdev);
> -
>         mutex_lock(&ifbdev->hpd_lock);
>         send_hpd = !ifbdev->hpd_suspended;
>         ifbdev->hpd_waiting = true;
> @@ -664,7 +651,6 @@ static void intel_fbdev_restore_mode(struct
> drm_i915_private *dev_priv)
>         if (!ifbdev)
>                 return;
>  
> -       intel_fbdev_sync(ifbdev);
>         if (!ifbdev->vma)
>                 return;
>  
> @@ -677,7 +663,20 @@ static void intel_fbdev_restore_mode(struct
> drm_i915_private *dev_priv)
>   */
>  
>  static void intel_fbdev_client_unregister(struct drm_client_dev
> *client)
> -{ }
> +{
> +       struct drm_fb_helper *fb_helper =
> drm_fb_helper_from_client(client);
> +       struct drm_device *dev = fb_helper->dev;
> +       struct pci_dev *pdev = to_pci_dev(dev->dev);
> +
> +       if (fb_helper->info) {
> +               vga_switcheroo_client_fb_set(pdev, NULL);
> +               drm_fb_helper_unregister_info(fb_helper);
> +       } else {
> +               drm_fb_helper_unprepare(fb_helper);
> +               drm_client_release(&fb_helper->client);
> +               kfree(fb_helper);
> +       }
> +}
>  
>  static int intel_fbdev_client_restore(struct drm_client_dev *client)
>  {
> @@ -691,9 +690,36 @@ static int intel_fbdev_client_restore(struct
> drm_client_dev *client)
>  
>  static int intel_fbdev_client_hotplug(struct drm_client_dev *client)
>  {
> -       intel_fbdev_output_poll_changed(client->dev);
> +       struct drm_fb_helper *fb_helper =
> drm_fb_helper_from_client(client);
> +       struct drm_device *dev = client->dev;
> +       struct pci_dev *pdev = to_pci_dev(dev->dev);
> +       int ret;
> +
> +       if (dev->fb_helper) {
> +               intel_fbdev_output_poll_changed(dev);
> +               return 0;
> +       }
> +
> +       ret = drm_fb_helper_init(dev, fb_helper);
> +       if (ret)
> +               goto err_drm_err;
> +
> +       if (!drm_drv_uses_atomic_modeset(dev))
> +               drm_helper_disable_unused_functions(dev);

Can you please explain why this is needed here?

> +
> +       ret = drm_fb_helper_initial_config(fb_helper);
> +       if (ret)
> +               goto err_drm_fb_helper_fini;
> +
> +       vga_switcheroo_client_fb_set(pdev, fb_helper->info);
>  
>         return 0;
> +
> +err_drm_fb_helper_fini:
> +       drm_fb_helper_fini(fb_helper);
> +err_drm_err:
> +       drm_err(dev, "Failed to setup i915 fbdev emulation
> (ret=%d)\n", ret);
> +       return ret;
>  }
>  
>  static const struct drm_client_funcs intel_fbdev_client_funcs = {
> @@ -703,22 +729,23 @@ static const struct drm_client_funcs
> intel_fbdev_client_funcs = {
>         .hotplug        = intel_fbdev_client_hotplug,
>  };
>  
> -int intel_fbdev_init(struct drm_device *dev)
> +void intel_fbdev_setup(struct drm_i915_private *dev_priv)

Use i915 rather than dev_priv.

BR,

Jouni Högander

>  {
> -       struct drm_i915_private *dev_priv = to_i915(dev);
> +       struct drm_device *dev = &dev_priv->drm;
>         struct intel_fbdev *ifbdev;
>         int ret;
>  
> -       if (drm_WARN_ON(dev, !HAS_DISPLAY(dev_priv)))
> -               return -ENODEV;
> +       if (!HAS_DISPLAY(dev_priv))
> +               return;
>  
>         ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
>         if (!ifbdev)
> -               return -ENOMEM;
> -
> -       mutex_init(&ifbdev->hpd_lock);
> +               return;
>         drm_fb_helper_prepare(dev, &ifbdev->helper, 32,
> &intel_fb_helper_funcs);
>  
> +       dev_priv->display.fbdev.fbdev = ifbdev;
> +       INIT_WORK(&dev_priv->display.fbdev.suspend_work,
> intel_fbdev_suspend_worker);
> +       mutex_init(&ifbdev->hpd_lock);
>         if (intel_fbdev_init_bios(dev, ifbdev))
>                 ifbdev->helper.preferred_bpp = ifbdev->preferred_bpp;
>         else
> @@ -726,68 +753,19 @@ int intel_fbdev_init(struct drm_device *dev)
>  
>         ret = drm_client_init(dev, &ifbdev->helper.client, "i915-
> fbdev",
>                               &intel_fbdev_client_funcs);
> -       if (ret)
> +       if (ret) {
> +               drm_err(dev, "Failed to register client: %d\n", ret);
>                 goto err_drm_fb_helper_unprepare;
> +       }
>  
> -       ret = drm_fb_helper_init(dev, &ifbdev->helper);
> -       if (ret)
> -               goto err_drm_client_release;
> -
> -       dev_priv->display.fbdev.fbdev = ifbdev;
> -       INIT_WORK(&dev_priv->display.fbdev.suspend_work,
> intel_fbdev_suspend_worker);
> +       drm_client_register(&ifbdev->helper.client);
>  
> -       return 0;
> +       return;
>  
> -err_drm_client_release:
> -       drm_client_release(&ifbdev->helper.client);
>  err_drm_fb_helper_unprepare:
>         drm_fb_helper_unprepare(&ifbdev->helper);
> +       mutex_destroy(&ifbdev->hpd_lock);
>         kfree(ifbdev);
> -       return ret;
> -}
> -
> -static void intel_fbdev_initial_config(void *data, async_cookie_t
> cookie)
> -{
> -       struct intel_fbdev *ifbdev = data;
> -
> -       /* Due to peculiar init order wrt to hpd handling this is
> separate. */
> -       if (drm_fb_helper_initial_config(&ifbdev->helper))
> -               intel_fbdev_unregister(to_i915(ifbdev->helper.dev));
> -}
> -
> -void intel_fbdev_initial_config_async(struct drm_i915_private
> *dev_priv)
> -{
> -       struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
> -
> -       if (!ifbdev)
> -               return;
> -
> -       ifbdev->cookie = async_schedule(intel_fbdev_initial_config,
> ifbdev);
> -}
> -
> -void intel_fbdev_unregister(struct drm_i915_private *dev_priv)
> -{
> -       struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
> -
> -       if (!ifbdev)
> -               return;
> -
> -       intel_fbdev_set_suspend(&dev_priv->drm,
> FBINFO_STATE_SUSPENDED, true);
> -
> -       if (!current_is_async())
> -               intel_fbdev_sync(ifbdev);
> -
> -       drm_fb_helper_unregister_info(&ifbdev->helper);
> -}
> -
> -void intel_fbdev_fini(struct drm_i915_private *dev_priv)
> -{
> -       struct intel_fbdev *ifbdev = fetch_and_zero(&dev_priv-
> >display.fbdev.fbdev);
> -
> -       if (!ifbdev)
> -               return;
> -
> -       intel_fbdev_destroy(ifbdev);
>  }
>  
>  struct intel_framebuffer *intel_fbdev_framebuffer(struct intel_fbdev
> *fbdev)
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.h
> b/drivers/gpu/drm/i915/display/intel_fbdev.h
> index 8c953f102ba22..08de2d5b34338 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.h
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.h
> @@ -14,27 +14,11 @@ struct intel_fbdev;
>  struct intel_framebuffer;
>  
>  #ifdef CONFIG_DRM_FBDEV_EMULATION
> -int intel_fbdev_init(struct drm_device *dev);
> -void intel_fbdev_initial_config_async(struct drm_i915_private
> *dev_priv);
> -void intel_fbdev_unregister(struct drm_i915_private *dev_priv);
> -void intel_fbdev_fini(struct drm_i915_private *dev_priv);
> +void intel_fbdev_setup(struct drm_i915_private *dev_priv);
>  void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool
> synchronous);
>  struct intel_framebuffer *intel_fbdev_framebuffer(struct intel_fbdev
> *fbdev);
>  #else
> -static inline int intel_fbdev_init(struct drm_device *dev)
> -{
> -       return 0;
> -}
> -
> -static inline void intel_fbdev_initial_config_async(struct
> drm_i915_private *dev_priv)
> -{
> -}
> -
> -static inline void intel_fbdev_unregister(struct drm_i915_private
> *dev_priv)
> -{
> -}
> -
> -static inline void intel_fbdev_fini(struct drm_i915_private
> *dev_priv)
> +static inline void intel_fbdev_setup(struct drm_i915_private
> *dev_priv)
>  {
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> index 86460cd8167d1..53663c0cc3be4 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -817,6 +817,8 @@ int i915_driver_probe(struct pci_dev *pdev, const
> struct pci_device_id *ent)
>  
>         i915->do_release = true;
>  
> +       intel_fbdev_setup(i915);
> +
>         return 0;
>  
>  out_cleanup_gem:
Thomas Zimmermann Nov. 1, 2023, 8:33 a.m. UTC | #2
Hi

Am 25.10.23 um 13:36 schrieb Hogander, Jouni:
[...]
>> +
>> +       if (!drm_drv_uses_atomic_modeset(dev))
>> +               drm_helper_disable_unused_functions(dev);
> 
> Can you please explain why this is needed here?

This disables some parts of the mode-setting pipeline and is required 
for drivers with non-atomic commits.  AFAICT atomic mode setting is not 
supported on some very old Intel chips. [1]  I'll leave a short comment 
on the code.

[1] 
https://elixir.bootlin.com/linux/v6.6/source/drivers/gpu/drm/i915/intel_device_info.c#L399

Best regard
Thomas

> 
>> +
>> +       ret = drm_fb_helper_initial_config(fb_helper);
>> +       if (ret)
>> +               goto err_drm_fb_helper_fini;
>> +
>> +       vga_switcheroo_client_fb_set(pdev, fb_helper->info);
>>   
>>          return 0;
>> +
>> +err_drm_fb_helper_fini:
>> +       drm_fb_helper_fini(fb_helper);
>> +err_drm_err:
>> +       drm_err(dev, "Failed to setup i915 fbdev emulation
>> (ret=%d)\n", ret);
>> +       return ret;
>>   }
>>   
>>   static const struct drm_client_funcs intel_fbdev_client_funcs = {
>> @@ -703,22 +729,23 @@ static const struct drm_client_funcs
>> intel_fbdev_client_funcs = {
>>          .hotplug        = intel_fbdev_client_hotplug,
>>   };
>>   
>> -int intel_fbdev_init(struct drm_device *dev)
>> +void intel_fbdev_setup(struct drm_i915_private *dev_priv)
> 
> Use i915 rather than dev_priv.
> 
> BR,
> 
> Jouni Högander
> 
>>   {
>> -       struct drm_i915_private *dev_priv = to_i915(dev);
>> +       struct drm_device *dev = &dev_priv->drm;
>>          struct intel_fbdev *ifbdev;
>>          int ret;
>>   
>> -       if (drm_WARN_ON(dev, !HAS_DISPLAY(dev_priv)))
>> -               return -ENODEV;
>> +       if (!HAS_DISPLAY(dev_priv))
>> +               return;
>>   
>>          ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
>>          if (!ifbdev)
>> -               return -ENOMEM;
>> -
>> -       mutex_init(&ifbdev->hpd_lock);
>> +               return;
>>          drm_fb_helper_prepare(dev, &ifbdev->helper, 32,
>> &intel_fb_helper_funcs);
>>   
>> +       dev_priv->display.fbdev.fbdev = ifbdev;
>> +       INIT_WORK(&dev_priv->display.fbdev.suspend_work,
>> intel_fbdev_suspend_worker);
>> +       mutex_init(&ifbdev->hpd_lock);
>>          if (intel_fbdev_init_bios(dev, ifbdev))
>>                  ifbdev->helper.preferred_bpp = ifbdev->preferred_bpp;
>>          else
>> @@ -726,68 +753,19 @@ int intel_fbdev_init(struct drm_device *dev)
>>   
>>          ret = drm_client_init(dev, &ifbdev->helper.client, "i915-
>> fbdev",
>>                                &intel_fbdev_client_funcs);
>> -       if (ret)
>> +       if (ret) {
>> +               drm_err(dev, "Failed to register client: %d\n", ret);
>>                  goto err_drm_fb_helper_unprepare;
>> +       }
>>   
>> -       ret = drm_fb_helper_init(dev, &ifbdev->helper);
>> -       if (ret)
>> -               goto err_drm_client_release;
>> -
>> -       dev_priv->display.fbdev.fbdev = ifbdev;
>> -       INIT_WORK(&dev_priv->display.fbdev.suspend_work,
>> intel_fbdev_suspend_worker);
>> +       drm_client_register(&ifbdev->helper.client);
>>   
>> -       return 0;
>> +       return;
>>   
>> -err_drm_client_release:
>> -       drm_client_release(&ifbdev->helper.client);
>>   err_drm_fb_helper_unprepare:
>>          drm_fb_helper_unprepare(&ifbdev->helper);
>> +       mutex_destroy(&ifbdev->hpd_lock);
>>          kfree(ifbdev);
>> -       return ret;
>> -}
>> -
>> -static void intel_fbdev_initial_config(void *data, async_cookie_t
>> cookie)
>> -{
>> -       struct intel_fbdev *ifbdev = data;
>> -
>> -       /* Due to peculiar init order wrt to hpd handling this is
>> separate. */
>> -       if (drm_fb_helper_initial_config(&ifbdev->helper))
>> -               intel_fbdev_unregister(to_i915(ifbdev->helper.dev));
>> -}
>> -
>> -void intel_fbdev_initial_config_async(struct drm_i915_private
>> *dev_priv)
>> -{
>> -       struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
>> -
>> -       if (!ifbdev)
>> -               return;
>> -
>> -       ifbdev->cookie = async_schedule(intel_fbdev_initial_config,
>> ifbdev);
>> -}
>> -
>> -void intel_fbdev_unregister(struct drm_i915_private *dev_priv)
>> -{
>> -       struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
>> -
>> -       if (!ifbdev)
>> -               return;
>> -
>> -       intel_fbdev_set_suspend(&dev_priv->drm,
>> FBINFO_STATE_SUSPENDED, true);
>> -
>> -       if (!current_is_async())
>> -               intel_fbdev_sync(ifbdev);
>> -
>> -       drm_fb_helper_unregister_info(&ifbdev->helper);
>> -}
>> -
>> -void intel_fbdev_fini(struct drm_i915_private *dev_priv)
>> -{
>> -       struct intel_fbdev *ifbdev = fetch_and_zero(&dev_priv-
>>> display.fbdev.fbdev);
>> -
>> -       if (!ifbdev)
>> -               return;
>> -
>> -       intel_fbdev_destroy(ifbdev);
>>   }
>>   
>>   struct intel_framebuffer *intel_fbdev_framebuffer(struct intel_fbdev
>> *fbdev)
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.h
>> b/drivers/gpu/drm/i915/display/intel_fbdev.h
>> index 8c953f102ba22..08de2d5b34338 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fbdev.h
>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.h
>> @@ -14,27 +14,11 @@ struct intel_fbdev;
>>   struct intel_framebuffer;
>>   
>>   #ifdef CONFIG_DRM_FBDEV_EMULATION
>> -int intel_fbdev_init(struct drm_device *dev);
>> -void intel_fbdev_initial_config_async(struct drm_i915_private
>> *dev_priv);
>> -void intel_fbdev_unregister(struct drm_i915_private *dev_priv);
>> -void intel_fbdev_fini(struct drm_i915_private *dev_priv);
>> +void intel_fbdev_setup(struct drm_i915_private *dev_priv);
>>   void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool
>> synchronous);
>>   struct intel_framebuffer *intel_fbdev_framebuffer(struct intel_fbdev
>> *fbdev);
>>   #else
>> -static inline int intel_fbdev_init(struct drm_device *dev)
>> -{
>> -       return 0;
>> -}
>> -
>> -static inline void intel_fbdev_initial_config_async(struct
>> drm_i915_private *dev_priv)
>> -{
>> -}
>> -
>> -static inline void intel_fbdev_unregister(struct drm_i915_private
>> *dev_priv)
>> -{
>> -}
>> -
>> -static inline void intel_fbdev_fini(struct drm_i915_private
>> *dev_priv)
>> +static inline void intel_fbdev_setup(struct drm_i915_private
>> *dev_priv)
>>   {
>>   }
>>   
>> diff --git a/drivers/gpu/drm/i915/i915_driver.c
>> b/drivers/gpu/drm/i915/i915_driver.c
>> index 86460cd8167d1..53663c0cc3be4 100644
>> --- a/drivers/gpu/drm/i915/i915_driver.c
>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>> @@ -817,6 +817,8 @@ int i915_driver_probe(struct pci_dev *pdev, const
>> struct pci_device_id *ent)
>>   
>>          i915->do_release = true;
>>   
>> +       intel_fbdev_setup(i915);
>> +
>>          return 0;
>>   
>>   out_cleanup_gem:
>
Ville Syrjälä Nov. 1, 2023, 9:25 a.m. UTC | #3
On Wed, Nov 01, 2023 at 09:33:41AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 25.10.23 um 13:36 schrieb Hogander, Jouni:
> [...]
> >> +
> >> +       if (!drm_drv_uses_atomic_modeset(dev))
> >> +               drm_helper_disable_unused_functions(dev);
> > 
> > Can you please explain why this is needed here?
> 
> This disables some parts of the mode-setting pipeline and is required 
> for drivers with non-atomic commits.  AFAICT atomic mode setting is not 
> supported on some very old Intel chips. [1]  I'll leave a short comment 
> on the code.

We don't expose the atomic uapi because the watermark code is
kinda sketchy for the old chips, but internally i915 is 100% atomic.

> 
> [1] 
> https://elixir.bootlin.com/linux/v6.6/source/drivers/gpu/drm/i915/intel_device_info.c#L399
> 
> Best regard
> Thomas
> 
> > 
> >> +
> >> +       ret = drm_fb_helper_initial_config(fb_helper);
> >> +       if (ret)
> >> +               goto err_drm_fb_helper_fini;
> >> +
> >> +       vga_switcheroo_client_fb_set(pdev, fb_helper->info);
> >>   
> >>          return 0;
> >> +
> >> +err_drm_fb_helper_fini:
> >> +       drm_fb_helper_fini(fb_helper);
> >> +err_drm_err:
> >> +       drm_err(dev, "Failed to setup i915 fbdev emulation
> >> (ret=%d)\n", ret);
> >> +       return ret;
> >>   }
> >>   
> >>   static const struct drm_client_funcs intel_fbdev_client_funcs = {
> >> @@ -703,22 +729,23 @@ static const struct drm_client_funcs
> >> intel_fbdev_client_funcs = {
> >>          .hotplug        = intel_fbdev_client_hotplug,
> >>   };
> >>   
> >> -int intel_fbdev_init(struct drm_device *dev)
> >> +void intel_fbdev_setup(struct drm_i915_private *dev_priv)
> > 
> > Use i915 rather than dev_priv.
> > 
> > BR,
> > 
> > Jouni Högander
> > 
> >>   {
> >> -       struct drm_i915_private *dev_priv = to_i915(dev);
> >> +       struct drm_device *dev = &dev_priv->drm;
> >>          struct intel_fbdev *ifbdev;
> >>          int ret;
> >>   
> >> -       if (drm_WARN_ON(dev, !HAS_DISPLAY(dev_priv)))
> >> -               return -ENODEV;
> >> +       if (!HAS_DISPLAY(dev_priv))
> >> +               return;
> >>   
> >>          ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
> >>          if (!ifbdev)
> >> -               return -ENOMEM;
> >> -
> >> -       mutex_init(&ifbdev->hpd_lock);
> >> +               return;
> >>          drm_fb_helper_prepare(dev, &ifbdev->helper, 32,
> >> &intel_fb_helper_funcs);
> >>   
> >> +       dev_priv->display.fbdev.fbdev = ifbdev;
> >> +       INIT_WORK(&dev_priv->display.fbdev.suspend_work,
> >> intel_fbdev_suspend_worker);
> >> +       mutex_init(&ifbdev->hpd_lock);
> >>          if (intel_fbdev_init_bios(dev, ifbdev))
> >>                  ifbdev->helper.preferred_bpp = ifbdev->preferred_bpp;
> >>          else
> >> @@ -726,68 +753,19 @@ int intel_fbdev_init(struct drm_device *dev)
> >>   
> >>          ret = drm_client_init(dev, &ifbdev->helper.client, "i915-
> >> fbdev",
> >>                                &intel_fbdev_client_funcs);
> >> -       if (ret)
> >> +       if (ret) {
> >> +               drm_err(dev, "Failed to register client: %d\n", ret);
> >>                  goto err_drm_fb_helper_unprepare;
> >> +       }
> >>   
> >> -       ret = drm_fb_helper_init(dev, &ifbdev->helper);
> >> -       if (ret)
> >> -               goto err_drm_client_release;
> >> -
> >> -       dev_priv->display.fbdev.fbdev = ifbdev;
> >> -       INIT_WORK(&dev_priv->display.fbdev.suspend_work,
> >> intel_fbdev_suspend_worker);
> >> +       drm_client_register(&ifbdev->helper.client);
> >>   
> >> -       return 0;
> >> +       return;
> >>   
> >> -err_drm_client_release:
> >> -       drm_client_release(&ifbdev->helper.client);
> >>   err_drm_fb_helper_unprepare:
> >>          drm_fb_helper_unprepare(&ifbdev->helper);
> >> +       mutex_destroy(&ifbdev->hpd_lock);
> >>          kfree(ifbdev);
> >> -       return ret;
> >> -}
> >> -
> >> -static void intel_fbdev_initial_config(void *data, async_cookie_t
> >> cookie)
> >> -{
> >> -       struct intel_fbdev *ifbdev = data;
> >> -
> >> -       /* Due to peculiar init order wrt to hpd handling this is
> >> separate. */
> >> -       if (drm_fb_helper_initial_config(&ifbdev->helper))
> >> -               intel_fbdev_unregister(to_i915(ifbdev->helper.dev));
> >> -}
> >> -
> >> -void intel_fbdev_initial_config_async(struct drm_i915_private
> >> *dev_priv)
> >> -{
> >> -       struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
> >> -
> >> -       if (!ifbdev)
> >> -               return;
> >> -
> >> -       ifbdev->cookie = async_schedule(intel_fbdev_initial_config,
> >> ifbdev);
> >> -}
> >> -
> >> -void intel_fbdev_unregister(struct drm_i915_private *dev_priv)
> >> -{
> >> -       struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
> >> -
> >> -       if (!ifbdev)
> >> -               return;
> >> -
> >> -       intel_fbdev_set_suspend(&dev_priv->drm,
> >> FBINFO_STATE_SUSPENDED, true);
> >> -
> >> -       if (!current_is_async())
> >> -               intel_fbdev_sync(ifbdev);
> >> -
> >> -       drm_fb_helper_unregister_info(&ifbdev->helper);
> >> -}
> >> -
> >> -void intel_fbdev_fini(struct drm_i915_private *dev_priv)
> >> -{
> >> -       struct intel_fbdev *ifbdev = fetch_and_zero(&dev_priv-
> >>> display.fbdev.fbdev);
> >> -
> >> -       if (!ifbdev)
> >> -               return;
> >> -
> >> -       intel_fbdev_destroy(ifbdev);
> >>   }
> >>   
> >>   struct intel_framebuffer *intel_fbdev_framebuffer(struct intel_fbdev
> >> *fbdev)
> >> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.h
> >> b/drivers/gpu/drm/i915/display/intel_fbdev.h
> >> index 8c953f102ba22..08de2d5b34338 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_fbdev.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.h
> >> @@ -14,27 +14,11 @@ struct intel_fbdev;
> >>   struct intel_framebuffer;
> >>   
> >>   #ifdef CONFIG_DRM_FBDEV_EMULATION
> >> -int intel_fbdev_init(struct drm_device *dev);
> >> -void intel_fbdev_initial_config_async(struct drm_i915_private
> >> *dev_priv);
> >> -void intel_fbdev_unregister(struct drm_i915_private *dev_priv);
> >> -void intel_fbdev_fini(struct drm_i915_private *dev_priv);
> >> +void intel_fbdev_setup(struct drm_i915_private *dev_priv);
> >>   void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool
> >> synchronous);
> >>   struct intel_framebuffer *intel_fbdev_framebuffer(struct intel_fbdev
> >> *fbdev);
> >>   #else
> >> -static inline int intel_fbdev_init(struct drm_device *dev)
> >> -{
> >> -       return 0;
> >> -}
> >> -
> >> -static inline void intel_fbdev_initial_config_async(struct
> >> drm_i915_private *dev_priv)
> >> -{
> >> -}
> >> -
> >> -static inline void intel_fbdev_unregister(struct drm_i915_private
> >> *dev_priv)
> >> -{
> >> -}
> >> -
> >> -static inline void intel_fbdev_fini(struct drm_i915_private
> >> *dev_priv)
> >> +static inline void intel_fbdev_setup(struct drm_i915_private
> >> *dev_priv)
> >>   {
> >>   }
> >>   
> >> diff --git a/drivers/gpu/drm/i915/i915_driver.c
> >> b/drivers/gpu/drm/i915/i915_driver.c
> >> index 86460cd8167d1..53663c0cc3be4 100644
> >> --- a/drivers/gpu/drm/i915/i915_driver.c
> >> +++ b/drivers/gpu/drm/i915/i915_driver.c
> >> @@ -817,6 +817,8 @@ int i915_driver_probe(struct pci_dev *pdev, const
> >> struct pci_device_id *ent)
> >>   
> >>          i915->do_release = true;
> >>   
> >> +       intel_fbdev_setup(i915);
> >> +
> >>          return 0;
> >>   
> >>   out_cleanup_gem:
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
Thomas Zimmermann Nov. 1, 2023, 9:35 a.m. UTC | #4
Hi

Am 01.11.23 um 10:25 schrieb Ville Syrjälä:
> On Wed, Nov 01, 2023 at 09:33:41AM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 25.10.23 um 13:36 schrieb Hogander, Jouni:
>> [...]
>>>> +
>>>> +       if (!drm_drv_uses_atomic_modeset(dev))
>>>> +               drm_helper_disable_unused_functions(dev);
>>>
>>> Can you please explain why this is needed here?
>>
>> This disables some parts of the mode-setting pipeline and is required
>> for drivers with non-atomic commits.  AFAICT atomic mode setting is not
>> supported on some very old Intel chips. [1]  I'll leave a short comment
>> on the code.
> 
> We don't expose the atomic uapi because the watermark code is
> kinda sketchy for the old chips, but internally i915 is 100% atomic.

Great, thanks. I'll drop that code then.

Best regards
Thomas

> 
>>
>> [1]
>> https://elixir.bootlin.com/linux/v6.6/source/drivers/gpu/drm/i915/intel_device_info.c#L399
>>
>> Best regard
>> Thomas
>>
>>>
>>>> +
>>>> +       ret = drm_fb_helper_initial_config(fb_helper);
>>>> +       if (ret)
>>>> +               goto err_drm_fb_helper_fini;
>>>> +
>>>> +       vga_switcheroo_client_fb_set(pdev, fb_helper->info);
>>>>    
>>>>           return 0;
>>>> +
>>>> +err_drm_fb_helper_fini:
>>>> +       drm_fb_helper_fini(fb_helper);
>>>> +err_drm_err:
>>>> +       drm_err(dev, "Failed to setup i915 fbdev emulation
>>>> (ret=%d)\n", ret);
>>>> +       return ret;
>>>>    }
>>>>    
>>>>    static const struct drm_client_funcs intel_fbdev_client_funcs = {
>>>> @@ -703,22 +729,23 @@ static const struct drm_client_funcs
>>>> intel_fbdev_client_funcs = {
>>>>           .hotplug        = intel_fbdev_client_hotplug,
>>>>    };
>>>>    
>>>> -int intel_fbdev_init(struct drm_device *dev)
>>>> +void intel_fbdev_setup(struct drm_i915_private *dev_priv)
>>>
>>> Use i915 rather than dev_priv.
>>>
>>> BR,
>>>
>>> Jouni Högander
>>>
>>>>    {
>>>> -       struct drm_i915_private *dev_priv = to_i915(dev);
>>>> +       struct drm_device *dev = &dev_priv->drm;
>>>>           struct intel_fbdev *ifbdev;
>>>>           int ret;
>>>>    
>>>> -       if (drm_WARN_ON(dev, !HAS_DISPLAY(dev_priv)))
>>>> -               return -ENODEV;
>>>> +       if (!HAS_DISPLAY(dev_priv))
>>>> +               return;
>>>>    
>>>>           ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
>>>>           if (!ifbdev)
>>>> -               return -ENOMEM;
>>>> -
>>>> -       mutex_init(&ifbdev->hpd_lock);
>>>> +               return;
>>>>           drm_fb_helper_prepare(dev, &ifbdev->helper, 32,
>>>> &intel_fb_helper_funcs);
>>>>    
>>>> +       dev_priv->display.fbdev.fbdev = ifbdev;
>>>> +       INIT_WORK(&dev_priv->display.fbdev.suspend_work,
>>>> intel_fbdev_suspend_worker);
>>>> +       mutex_init(&ifbdev->hpd_lock);
>>>>           if (intel_fbdev_init_bios(dev, ifbdev))
>>>>                   ifbdev->helper.preferred_bpp = ifbdev->preferred_bpp;
>>>>           else
>>>> @@ -726,68 +753,19 @@ int intel_fbdev_init(struct drm_device *dev)
>>>>    
>>>>           ret = drm_client_init(dev, &ifbdev->helper.client, "i915-
>>>> fbdev",
>>>>                                 &intel_fbdev_client_funcs);
>>>> -       if (ret)
>>>> +       if (ret) {
>>>> +               drm_err(dev, "Failed to register client: %d\n", ret);
>>>>                   goto err_drm_fb_helper_unprepare;
>>>> +       }
>>>>    
>>>> -       ret = drm_fb_helper_init(dev, &ifbdev->helper);
>>>> -       if (ret)
>>>> -               goto err_drm_client_release;
>>>> -
>>>> -       dev_priv->display.fbdev.fbdev = ifbdev;
>>>> -       INIT_WORK(&dev_priv->display.fbdev.suspend_work,
>>>> intel_fbdev_suspend_worker);
>>>> +       drm_client_register(&ifbdev->helper.client);
>>>>    
>>>> -       return 0;
>>>> +       return;
>>>>    
>>>> -err_drm_client_release:
>>>> -       drm_client_release(&ifbdev->helper.client);
>>>>    err_drm_fb_helper_unprepare:
>>>>           drm_fb_helper_unprepare(&ifbdev->helper);
>>>> +       mutex_destroy(&ifbdev->hpd_lock);
>>>>           kfree(ifbdev);
>>>> -       return ret;
>>>> -}
>>>> -
>>>> -static void intel_fbdev_initial_config(void *data, async_cookie_t
>>>> cookie)
>>>> -{
>>>> -       struct intel_fbdev *ifbdev = data;
>>>> -
>>>> -       /* Due to peculiar init order wrt to hpd handling this is
>>>> separate. */
>>>> -       if (drm_fb_helper_initial_config(&ifbdev->helper))
>>>> -               intel_fbdev_unregister(to_i915(ifbdev->helper.dev));
>>>> -}
>>>> -
>>>> -void intel_fbdev_initial_config_async(struct drm_i915_private
>>>> *dev_priv)
>>>> -{
>>>> -       struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
>>>> -
>>>> -       if (!ifbdev)
>>>> -               return;
>>>> -
>>>> -       ifbdev->cookie = async_schedule(intel_fbdev_initial_config,
>>>> ifbdev);
>>>> -}
>>>> -
>>>> -void intel_fbdev_unregister(struct drm_i915_private *dev_priv)
>>>> -{
>>>> -       struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
>>>> -
>>>> -       if (!ifbdev)
>>>> -               return;
>>>> -
>>>> -       intel_fbdev_set_suspend(&dev_priv->drm,
>>>> FBINFO_STATE_SUSPENDED, true);
>>>> -
>>>> -       if (!current_is_async())
>>>> -               intel_fbdev_sync(ifbdev);
>>>> -
>>>> -       drm_fb_helper_unregister_info(&ifbdev->helper);
>>>> -}
>>>> -
>>>> -void intel_fbdev_fini(struct drm_i915_private *dev_priv)
>>>> -{
>>>> -       struct intel_fbdev *ifbdev = fetch_and_zero(&dev_priv-
>>>>> display.fbdev.fbdev);
>>>> -
>>>> -       if (!ifbdev)
>>>> -               return;
>>>> -
>>>> -       intel_fbdev_destroy(ifbdev);
>>>>    }
>>>>    
>>>>    struct intel_framebuffer *intel_fbdev_framebuffer(struct intel_fbdev
>>>> *fbdev)
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.h
>>>> b/drivers/gpu/drm/i915/display/intel_fbdev.h
>>>> index 8c953f102ba22..08de2d5b34338 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_fbdev.h
>>>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.h
>>>> @@ -14,27 +14,11 @@ struct intel_fbdev;
>>>>    struct intel_framebuffer;
>>>>    
>>>>    #ifdef CONFIG_DRM_FBDEV_EMULATION
>>>> -int intel_fbdev_init(struct drm_device *dev);
>>>> -void intel_fbdev_initial_config_async(struct drm_i915_private
>>>> *dev_priv);
>>>> -void intel_fbdev_unregister(struct drm_i915_private *dev_priv);
>>>> -void intel_fbdev_fini(struct drm_i915_private *dev_priv);
>>>> +void intel_fbdev_setup(struct drm_i915_private *dev_priv);
>>>>    void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool
>>>> synchronous);
>>>>    struct intel_framebuffer *intel_fbdev_framebuffer(struct intel_fbdev
>>>> *fbdev);
>>>>    #else
>>>> -static inline int intel_fbdev_init(struct drm_device *dev)
>>>> -{
>>>> -       return 0;
>>>> -}
>>>> -
>>>> -static inline void intel_fbdev_initial_config_async(struct
>>>> drm_i915_private *dev_priv)
>>>> -{
>>>> -}
>>>> -
>>>> -static inline void intel_fbdev_unregister(struct drm_i915_private
>>>> *dev_priv)
>>>> -{
>>>> -}
>>>> -
>>>> -static inline void intel_fbdev_fini(struct drm_i915_private
>>>> *dev_priv)
>>>> +static inline void intel_fbdev_setup(struct drm_i915_private
>>>> *dev_priv)
>>>>    {
>>>>    }
>>>>    
>>>> diff --git a/drivers/gpu/drm/i915/i915_driver.c
>>>> b/drivers/gpu/drm/i915/i915_driver.c
>>>> index 86460cd8167d1..53663c0cc3be4 100644
>>>> --- a/drivers/gpu/drm/i915/i915_driver.c
>>>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>>>> @@ -817,6 +817,8 @@ int i915_driver_probe(struct pci_dev *pdev, const
>>>> struct pci_device_id *ent)
>>>>    
>>>>           i915->do_release = true;
>>>>    
>>>> +       intel_fbdev_setup(i915);
>>>> +
>>>>           return 0;
>>>>    
>>>>    out_cleanup_gem:
>>>
>>
>> -- 
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Frankenstrasse 146, 90461 Nuernberg, Germany
>> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>> HRB 36809 (AG Nuernberg)
> 
> 
> 
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index edbcf5968804d..7efa8d2787c39 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -81,7 +81,6 @@ 
 #include "intel_dvo.h"
 #include "intel_fb.h"
 #include "intel_fbc.h"
-#include "intel_fbdev.h"
 #include "intel_fdi.h"
 #include "intel_fifo_underrun.h"
 #include "intel_frontbuffer.h"
diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
index ffdcddd1943e0..213a4ee93ffc2 100644
--- a/drivers/gpu/drm/i915/display/intel_display_driver.c
+++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
@@ -364,10 +364,6 @@  int intel_display_driver_probe(struct drm_i915_private *i915)
 
 	intel_overlay_setup(i915);
 
-	ret = intel_fbdev_init(&i915->drm);
-	if (ret)
-		return ret;
-
 	/* Only enable hotplug handling once the fbdev is fully set up. */
 	intel_hpd_init(i915);
 	intel_hpd_poll_disable(i915);
@@ -392,16 +388,6 @@  void intel_display_driver_register(struct drm_i915_private *i915)
 
 	intel_display_debugfs_register(i915);
 
-	/*
-	 * Some ports require correctly set-up hpd registers for
-	 * detection to work properly (leading to ghost connected
-	 * connector status), e.g. VGA on gm45.  Hence we can only set
-	 * up the initial fbdev config after hpd irqs are fully
-	 * enabled. We do it last so that the async config cannot run
-	 * before the connectors are registered.
-	 */
-	intel_fbdev_initial_config_async(i915);
-
 	/*
 	 * We need to coordinate the hotplugs with the asynchronous
 	 * fbdev configuration, for which we use the
@@ -445,9 +431,6 @@  void intel_display_driver_remove_noirq(struct drm_i915_private *i915)
 	 */
 	intel_hpd_poll_fini(i915);
 
-	/* poll work can call into fbdev, hence clean that up afterwards */
-	intel_fbdev_fini(i915);
-
 	intel_unregister_dsm_handler();
 
 	/* flush any delayed tasks or pending work */
@@ -484,7 +467,6 @@  void intel_display_driver_unregister(struct drm_i915_private *i915)
 	if (!HAS_DISPLAY(i915))
 		return;
 
-	intel_fbdev_unregister(i915);
 	intel_audio_deinit(i915);
 
 	/*
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
index 39de61d4e7906..100a4aaf1b7e4 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -24,7 +24,6 @@ 
  *     David Airlie
  */
 
-#include <linux/async.h>
 #include <linux/console.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
@@ -39,6 +38,7 @@ 
 #include <linux/vga_switcheroo.h>
 
 #include <drm/drm_crtc.h>
+#include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_framebuffer_helper.h>
@@ -58,7 +58,6 @@  struct intel_fbdev {
 	struct intel_framebuffer *fb;
 	struct i915_vma *vma;
 	unsigned long vma_flags;
-	async_cookie_t cookie;
 	int preferred_bpp;
 
 	/* Whether or not fbdev hpd processing is temporarily suspended */
@@ -135,6 +134,26 @@  static int intel_fbdev_mmap(struct fb_info *info, struct vm_area_struct *vma)
 	return i915_gem_fb_mmap(obj, vma);
 }
 
+static void intel_fbdev_fb_destroy(struct fb_info *info)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+	struct intel_fbdev *ifbdev = container_of(fb_helper, struct intel_fbdev, helper);
+
+	drm_fb_helper_fini(&ifbdev->helper);
+
+	/*
+	 * We rely on the object-free to release the VMA pinning for
+	 * the info->screen_base mmaping. Leaking the VMA is simpler than
+	 * trying to rectify all the possible error paths leading here.
+	 */
+	intel_unpin_fb_vma(ifbdev->vma, ifbdev->vma_flags);
+	drm_framebuffer_remove(&ifbdev->fb->base);
+
+	drm_client_release(&fb_helper->client);
+	drm_fb_helper_unprepare(&ifbdev->helper);
+	kfree(ifbdev);
+}
+
 static const struct fb_ops intelfb_ops = {
 	.owner = THIS_MODULE,
 	__FB_DEFAULT_DEFERRED_OPS_RDWR(intel_fbdev),
@@ -144,6 +163,7 @@  static const struct fb_ops intelfb_ops = {
 	.fb_pan_display = intel_fbdev_pan_display,
 	__FB_DEFAULT_DEFERRED_OPS_DRAW(intel_fbdev),
 	.fb_mmap = intel_fbdev_mmap,
+	.fb_destroy = intel_fbdev_fb_destroy,
 };
 
 static int intelfb_alloc(struct drm_fb_helper *helper,
@@ -212,7 +232,6 @@  static int intelfb_create(struct drm_fb_helper *helper,
 	struct intel_framebuffer *intel_fb = ifbdev->fb;
 	struct drm_device *dev = helper->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
 	struct i915_ggtt *ggtt = to_gt(dev_priv)->ggtt;
 	const struct i915_gtt_view view = {
 		.type = I915_GTT_VIEW_NORMAL,
@@ -337,7 +356,7 @@  static int intelfb_create(struct drm_fb_helper *helper,
 	ifbdev->vma_flags = flags;
 
 	intel_runtime_pm_put(&dev_priv->runtime_pm, wakeref);
-	vga_switcheroo_client_fb_set(pdev, info);
+
 	return 0;
 
 out_unpin:
@@ -363,26 +382,6 @@  static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
 	.fb_dirty = intelfb_dirty,
 };
 
-static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
-{
-	/* We rely on the object-free to release the VMA pinning for
-	 * the info->screen_base mmaping. Leaking the VMA is simpler than
-	 * trying to rectify all the possible error paths leading here.
-	 */
-
-	drm_fb_helper_fini(&ifbdev->helper);
-
-	if (ifbdev->vma)
-		intel_unpin_fb_vma(ifbdev->vma, ifbdev->vma_flags);
-
-	if (ifbdev->fb)
-		drm_framebuffer_remove(&ifbdev->fb->base);
-
-	drm_client_release(&ifbdev->helper.client);
-	drm_fb_helper_unprepare(&ifbdev->helper);
-	kfree(ifbdev);
-}
-
 /*
  * Build an intel_fbdev struct using a BIOS allocated framebuffer, if possible.
  * The core display code will have read out the current plane configuration,
@@ -546,16 +545,6 @@  static void intel_fbdev_suspend_worker(struct work_struct *work)
 				true);
 }
 
-static void intel_fbdev_sync(struct intel_fbdev *ifbdev)
-{
-	if (!ifbdev->cookie)
-		return;
-
-	/* Only serialises with all preceding async calls, hence +1 */
-	async_synchronize_cookie(ifbdev->cookie + 1);
-	ifbdev->cookie = 0;
-}
-
 /* Suspends/resumes fbdev processing of incoming HPD events. When resuming HPD
  * processing, fbdev will perform a full connector reprobe if a hotplug event
  * was received while HPD was suspended.
@@ -646,8 +635,6 @@  static void intel_fbdev_output_poll_changed(struct drm_device *dev)
 	if (!ifbdev)
 		return;
 
-	intel_fbdev_sync(ifbdev);
-
 	mutex_lock(&ifbdev->hpd_lock);
 	send_hpd = !ifbdev->hpd_suspended;
 	ifbdev->hpd_waiting = true;
@@ -664,7 +651,6 @@  static void intel_fbdev_restore_mode(struct drm_i915_private *dev_priv)
 	if (!ifbdev)
 		return;
 
-	intel_fbdev_sync(ifbdev);
 	if (!ifbdev->vma)
 		return;
 
@@ -677,7 +663,20 @@  static void intel_fbdev_restore_mode(struct drm_i915_private *dev_priv)
  */
 
 static void intel_fbdev_client_unregister(struct drm_client_dev *client)
-{ }
+{
+	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
+	struct drm_device *dev = fb_helper->dev;
+	struct pci_dev *pdev = to_pci_dev(dev->dev);
+
+	if (fb_helper->info) {
+		vga_switcheroo_client_fb_set(pdev, NULL);
+		drm_fb_helper_unregister_info(fb_helper);
+	} else {
+		drm_fb_helper_unprepare(fb_helper);
+		drm_client_release(&fb_helper->client);
+		kfree(fb_helper);
+	}
+}
 
 static int intel_fbdev_client_restore(struct drm_client_dev *client)
 {
@@ -691,9 +690,36 @@  static int intel_fbdev_client_restore(struct drm_client_dev *client)
 
 static int intel_fbdev_client_hotplug(struct drm_client_dev *client)
 {
-	intel_fbdev_output_poll_changed(client->dev);
+	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
+	struct drm_device *dev = client->dev;
+	struct pci_dev *pdev = to_pci_dev(dev->dev);
+	int ret;
+
+	if (dev->fb_helper) {
+		intel_fbdev_output_poll_changed(dev);
+		return 0;
+	}
+
+	ret = drm_fb_helper_init(dev, fb_helper);
+	if (ret)
+		goto err_drm_err;
+
+	if (!drm_drv_uses_atomic_modeset(dev))
+		drm_helper_disable_unused_functions(dev);
+
+	ret = drm_fb_helper_initial_config(fb_helper);
+	if (ret)
+		goto err_drm_fb_helper_fini;
+
+	vga_switcheroo_client_fb_set(pdev, fb_helper->info);
 
 	return 0;
+
+err_drm_fb_helper_fini:
+	drm_fb_helper_fini(fb_helper);
+err_drm_err:
+	drm_err(dev, "Failed to setup i915 fbdev emulation (ret=%d)\n", ret);
+	return ret;
 }
 
 static const struct drm_client_funcs intel_fbdev_client_funcs = {
@@ -703,22 +729,23 @@  static const struct drm_client_funcs intel_fbdev_client_funcs = {
 	.hotplug	= intel_fbdev_client_hotplug,
 };
 
-int intel_fbdev_init(struct drm_device *dev)
+void intel_fbdev_setup(struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_device *dev = &dev_priv->drm;
 	struct intel_fbdev *ifbdev;
 	int ret;
 
-	if (drm_WARN_ON(dev, !HAS_DISPLAY(dev_priv)))
-		return -ENODEV;
+	if (!HAS_DISPLAY(dev_priv))
+		return;
 
 	ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
 	if (!ifbdev)
-		return -ENOMEM;
-
-	mutex_init(&ifbdev->hpd_lock);
+		return;
 	drm_fb_helper_prepare(dev, &ifbdev->helper, 32, &intel_fb_helper_funcs);
 
+	dev_priv->display.fbdev.fbdev = ifbdev;
+	INIT_WORK(&dev_priv->display.fbdev.suspend_work, intel_fbdev_suspend_worker);
+	mutex_init(&ifbdev->hpd_lock);
 	if (intel_fbdev_init_bios(dev, ifbdev))
 		ifbdev->helper.preferred_bpp = ifbdev->preferred_bpp;
 	else
@@ -726,68 +753,19 @@  int intel_fbdev_init(struct drm_device *dev)
 
 	ret = drm_client_init(dev, &ifbdev->helper.client, "i915-fbdev",
 			      &intel_fbdev_client_funcs);
-	if (ret)
+	if (ret) {
+		drm_err(dev, "Failed to register client: %d\n", ret);
 		goto err_drm_fb_helper_unprepare;
+	}
 
-	ret = drm_fb_helper_init(dev, &ifbdev->helper);
-	if (ret)
-		goto err_drm_client_release;
-
-	dev_priv->display.fbdev.fbdev = ifbdev;
-	INIT_WORK(&dev_priv->display.fbdev.suspend_work, intel_fbdev_suspend_worker);
+	drm_client_register(&ifbdev->helper.client);
 
-	return 0;
+	return;
 
-err_drm_client_release:
-	drm_client_release(&ifbdev->helper.client);
 err_drm_fb_helper_unprepare:
 	drm_fb_helper_unprepare(&ifbdev->helper);
+	mutex_destroy(&ifbdev->hpd_lock);
 	kfree(ifbdev);
-	return ret;
-}
-
-static void intel_fbdev_initial_config(void *data, async_cookie_t cookie)
-{
-	struct intel_fbdev *ifbdev = data;
-
-	/* Due to peculiar init order wrt to hpd handling this is separate. */
-	if (drm_fb_helper_initial_config(&ifbdev->helper))
-		intel_fbdev_unregister(to_i915(ifbdev->helper.dev));
-}
-
-void intel_fbdev_initial_config_async(struct drm_i915_private *dev_priv)
-{
-	struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
-
-	if (!ifbdev)
-		return;
-
-	ifbdev->cookie = async_schedule(intel_fbdev_initial_config, ifbdev);
-}
-
-void intel_fbdev_unregister(struct drm_i915_private *dev_priv)
-{
-	struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
-
-	if (!ifbdev)
-		return;
-
-	intel_fbdev_set_suspend(&dev_priv->drm, FBINFO_STATE_SUSPENDED, true);
-
-	if (!current_is_async())
-		intel_fbdev_sync(ifbdev);
-
-	drm_fb_helper_unregister_info(&ifbdev->helper);
-}
-
-void intel_fbdev_fini(struct drm_i915_private *dev_priv)
-{
-	struct intel_fbdev *ifbdev = fetch_and_zero(&dev_priv->display.fbdev.fbdev);
-
-	if (!ifbdev)
-		return;
-
-	intel_fbdev_destroy(ifbdev);
 }
 
 struct intel_framebuffer *intel_fbdev_framebuffer(struct intel_fbdev *fbdev)
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.h b/drivers/gpu/drm/i915/display/intel_fbdev.h
index 8c953f102ba22..08de2d5b34338 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.h
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.h
@@ -14,27 +14,11 @@  struct intel_fbdev;
 struct intel_framebuffer;
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
-int intel_fbdev_init(struct drm_device *dev);
-void intel_fbdev_initial_config_async(struct drm_i915_private *dev_priv);
-void intel_fbdev_unregister(struct drm_i915_private *dev_priv);
-void intel_fbdev_fini(struct drm_i915_private *dev_priv);
+void intel_fbdev_setup(struct drm_i915_private *dev_priv);
 void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous);
 struct intel_framebuffer *intel_fbdev_framebuffer(struct intel_fbdev *fbdev);
 #else
-static inline int intel_fbdev_init(struct drm_device *dev)
-{
-	return 0;
-}
-
-static inline void intel_fbdev_initial_config_async(struct drm_i915_private *dev_priv)
-{
-}
-
-static inline void intel_fbdev_unregister(struct drm_i915_private *dev_priv)
-{
-}
-
-static inline void intel_fbdev_fini(struct drm_i915_private *dev_priv)
+static inline void intel_fbdev_setup(struct drm_i915_private *dev_priv)
 {
 }
 
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index 86460cd8167d1..53663c0cc3be4 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -817,6 +817,8 @@  int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	i915->do_release = true;
 
+	intel_fbdev_setup(i915);
+
 	return 0;
 
 out_cleanup_gem: