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) -- Ville Syrjälä Intel
