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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 7951 bytes --]

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:
> 

-- 
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)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]