Re: [Intel-gfx] [PATCH v5 7/7] drm/i915: Implement fbdev emulation as in-kernel client

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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