[Intel-gfx] [PATCH 4/4] drm/i915: Implement fbdev emulation as in-kernel client
Thomas Zimmermann
tzimmermann at suse.de
Mon Apr 3 12:50:56 UTC 2023
Hi
Am 28.03.23 um 17:31 schrieb Jani Nikula:
> On Tue, 28 Mar 2023, Thomas Zimmermann <tzimmermann at suse.de> 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
>> DRM device. Just like in most drivers, i915 fbdev emulation now
>> acts like 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.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>
> I like the direction in the series, but I'm afraid I don't have the time
> for proper detailed review of this right now. Just a couple of nits I
> stumbled on inline.
Thanks a look for looking at it. I keep this patchset up for a little
longer and maybe someone sends in further comments. Otherwise I'll send
out an update and ask around on irc.
Best regards
Thomas
>
> Anyone else have a chance to get this reviewed?
>
> BR,
> Jani.
>
>
>> ---
>> drivers/gpu/drm/i915/display/intel_display.c | 30 ---
>> drivers/gpu/drm/i915/display/intel_fbdev.c | 184 +++++++++----------
>> drivers/gpu/drm/i915/display/intel_fbdev.h | 20 +-
>> drivers/gpu/drm/i915/i915_driver.c | 2 +
>> 4 files changed, 87 insertions(+), 149 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 430a1016e013..1bbeb7a061e3 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"
>> @@ -8588,11 +8587,6 @@ int intel_modeset_init(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);
>>
>> @@ -8796,9 +8790,6 @@ void intel_modeset_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 */
>> @@ -8864,21 +8855,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
>> - * fbdev->async_cookie.
>> - */
>> drm_kms_helper_poll_init(&i915->drm);
>> }
>>
>> @@ -8887,14 +8863,8 @@ void intel_display_driver_unregister(struct drm_i915_private *i915)
>> if (!HAS_DISPLAY(i915))
>> return;
>>
>> - intel_fbdev_unregister(i915);
>> intel_audio_deinit(i915);
>>
>> - /*
>> - * After flushing the fbdev (incl. a late async config which
>> - * will have delayed queuing of a hotplug event), then flush
>> - * the hotplug events.
>> - */
>> drm_kms_helper_poll_fini(&i915->drm);
>> drm_atomic_helper_shutdown(&i915->drm);
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> index bdb9e6f43602..8b618db30ee5 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>
>> @@ -38,6 +37,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>
>>
>> @@ -55,7 +55,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 */
>> @@ -120,6 +119,26 @@ static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,
>> return ret;
>> }
>>
>> +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,
>> DRM_FB_HELPER_DEFAULT_OPS,
>> @@ -131,6 +150,7 @@ static const struct fb_ops intelfb_ops = {
>> .fb_imageblit = drm_fb_helper_cfb_imageblit,
>> .fb_pan_display = intel_fbdev_pan_display,
>> .fb_blank = intel_fbdev_blank,
>> + .fb_destroy = intel_fbdev_fb_destroy,
>> };
>>
>> static int intelfb_alloc(struct drm_fb_helper *helper,
>> @@ -198,7 +218,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,
>> @@ -323,7 +342,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:
>> @@ -349,26 +368,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,
>> @@ -532,16 +531,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.
>> @@ -631,8 +620,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;
>> @@ -649,7 +636,6 @@ static void intel_fbdev_restore_mode(struct drm_i915_private *dev_priv)
>> if (!ifbdev)
>> return;
>>
>> - intel_fbdev_sync(ifbdev);
>> if (!ifbdev->vma)
>> return;
>>
>> @@ -662,7 +648,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)
>> {
>> @@ -676,9 +675,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 radeon fbdev emulation (ret=%d)\n", ret);
> ^^^^^^
>
> Hrrmmh. ;)
>
>> + return ret;
>> }
>>
>> static const struct drm_client_funcs intel_fbdev_client_funcs = {
>> @@ -688,22 +714,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;
>> -
>> - ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
>> - if (ifbdev == NULL)
>> - return -ENOMEM;
>> + return;
>>
>> - mutex_init(&ifbdev->hpd_lock);
>> + ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
>> + if (!ifbdev)
>> + 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
>> @@ -711,68 +738,23 @@ 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);
>> + ret = intel_fbdev_client_hotplug(&ifbdev->helper.client);
>> if (ret)
>> - goto err_drm_client_release;
>> + drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
>>
>> - 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_client_release(&ifbdev->helper.client);
>> + 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 8c953f102ba2..8123c813caaf 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)
>> +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 be804fd4f79f..0b6722bcfbb2 100644
>> --- a/drivers/gpu/drm/i915/i915_driver.c
>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>> @@ -820,6 +820,8 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>
>> i915->do_release = true;
>>
>> + intel_fbdev_setup(i915);
>> +
>
> Maybe we could call this from intel_display_driver_register()?
>
> We're trying to separate display related stuff from the rest, and call
> as few display functions from top level driver code as
> possible. (Clearly, we're nowhere near this goal yet.)
>
>
>> return 0;
>>
>> out_cleanup_gem:
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20230403/b7681fec/attachment.sig>
More information about the Intel-gfx
mailing list