[PATCH v8 6/6] drm/{i915,xe}: Implement fbdev emulation as in-kernel client
Thomas Zimmermann
tzimmermann at suse.de
Tue Apr 23 11:13:23 UTC 2024
Hi
Am 22.04.24 um 16:11 schrieb Hogander, Jouni:
> On Tue, 2024-04-09 at 10:04 +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 intel_fbdev_setup() after i915 has registered its
>> DRM device. Just like similar code in other drivers, i915 fbdev
>> emulation now acts like a regular DRM client. Do the same for xe.
>>
>> 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_client_dev_unregister() releases all in-kernel clients
>> 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. Do the same for xe and remove its call to
>> intel_fbdev_fini().
>>
>> v8:
>> - setup client in intel_display_driver_register (Jouni)
>> - mention xe in commit message
>>
>> v7:
>> - update xe driver
>> - reword commit message
>>
>> v6:
>> - use 'i915' for i915 device (Jouni)
>> - remove unnecessary code for non-atomic mode setting (Jouni, Ville)
>> - fix function name in commit message (Jouni)
>>
>> 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 at suse.de>
> Reviewed-by: Jouni Högander <jouni.hogander at intel.com>
Thank you so much. All patches has R-bs. Can you add the series to the
intel tree?
Best regards
Thomas
>
>> ---
>> drivers/gpu/drm/i915/display/intel_display.c | 1 -
>> .../drm/i915/display/intel_display_driver.c | 20 +-
>> drivers/gpu/drm/i915/display/intel_fbdev.c | 177 ++++++++--------
>> --
>> drivers/gpu/drm/i915/display/intel_fbdev.h | 20 +-
>> drivers/gpu/drm/xe/display/xe_display.c | 2 -
>> 5 files changed, 80 insertions(+), 140 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
>> b/drivers/gpu/drm/i915/display/intel_display.c
>> index 614e60420a29a..161a5aabf6746 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -85,7 +85,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 e5f052d4ff1cc..ed8589fa35448 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
>> @@ -514,10 +514,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);
>>
>> @@ -544,16 +540,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
>> @@ -562,6 +548,8 @@ void intel_display_driver_register(struct
>> drm_i915_private *i915)
>> drm_kms_helper_poll_init(&i915->drm);
>> intel_hpd_poll_disable(i915);
>>
>> + intel_fbdev_setup(i915);
>> +
>> intel_display_device_info_print(DISPLAY_INFO(i915),
>> DISPLAY_RUNTIME_INFO(i915),
>> &p);
>> }
>> @@ -597,9 +585,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 */
>> @@ -640,7 +625,6 @@ void intel_display_driver_unregister(struct
>> drm_i915_private *i915)
>>
>> drm_client_dev_unregister(&i915->drm);
>>
>> - intel_fbdev_unregister(i915);
>> /*
>> * After flushing the fbdev (incl. a late async config which
>> * will have delayed queuing of a hotplug event), then flush
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
>> b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> index f783de611a7f5..bda702c2cab8e 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);
>> +}
>> +
>> __diag_push();
>> __diag_ignore_all("-Woverride-init", "Allow field initialization
>> overrides for fb ops");
>>
>> @@ -147,6 +166,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,
>> };
>>
>> __diag_pop();
>> @@ -158,7 +178,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);
>> const struct i915_gtt_view view = {
>> .type = I915_GTT_VIEW_NORMAL,
>> };
>> @@ -250,7 +269,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:
>> @@ -276,26 +295,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,
>> @@ -459,16 +458,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.
>> @@ -559,8 +548,6 @@ static int intel_fbdev_output_poll_changed(struct
>> drm_device *dev)
>> if (!ifbdev)
>> return -EINVAL;
>>
>> - intel_fbdev_sync(ifbdev);
>> -
>> mutex_lock(&ifbdev->hpd_lock);
>> send_hpd = !ifbdev->hpd_suspended;
>> ifbdev->hpd_waiting = true;
>> @@ -580,7 +567,6 @@ static int intel_fbdev_restore_mode(struct
>> drm_i915_private *dev_priv)
>> if (!ifbdev)
>> return -EINVAL;
>>
>> - intel_fbdev_sync(ifbdev);
>> if (!ifbdev->vma)
>> return -ENOMEM;
>>
>> @@ -598,7 +584,20 @@ static int 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)
>> {
>> @@ -616,7 +615,31 @@ static int intel_fbdev_client_restore(struct
>> drm_client_dev *client)
>>
>> static int intel_fbdev_client_hotplug(struct drm_client_dev *client)
>> {
>> - return 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)
>> + return intel_fbdev_output_poll_changed(dev);
>> +
>> + ret = drm_fb_helper_init(dev, fb_helper);
>> + if (ret)
>> + goto err_drm_err;
>> +
>> + 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 = {
>> @@ -626,22 +649,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 *i915)
>> {
>> - struct drm_i915_private *dev_priv = to_i915(dev);
>> + struct drm_device *dev = &i915->drm;
>> struct intel_fbdev *ifbdev;
>> int ret;
>>
>> - if (drm_WARN_ON(dev, !HAS_DISPLAY(dev_priv)))
>> - return -ENODEV;
>> + if (!HAS_DISPLAY(i915))
>> + 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);
>>
>> + i915->display.fbdev.fbdev = ifbdev;
>> + INIT_WORK(&i915->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
>> @@ -649,68 +673,19 @@ int intel_fbdev_init(struct drm_device *dev)
>>
>> ret = drm_client_init(dev, &ifbdev->helper.client, "intel-
>> 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/xe/display/xe_display.c
>> b/drivers/gpu/drm/xe/display/xe_display.c
>> index cdbc3f04c80a7..ca5cbe1d8a03b 100644
>> --- a/drivers/gpu/drm/xe/display/xe_display.c
>> +++ b/drivers/gpu/drm/xe/display/xe_display.c
>> @@ -214,9 +214,7 @@ void xe_display_fini(struct xe_device *xe)
>> if (!xe->info.enable_display)
>> return;
>>
>> - /* poll work can call into fbdev, hence clean that up
>> afterwards */
>> intel_hpd_poll_fini(xe);
>> - intel_fbdev_fini(xe);
>>
>> intel_hdcp_component_fini(xe);
>> intel_audio_deinit(xe);
--
--
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)
More information about the Intel-gfx
mailing list