Re: [Intel-gfx] [PATCH v5 7/7] drm/i915: Implement fbdev emulation as in-kernel client
Hi
Am 01.11.23 um 10:25 schrieb Ville Syrjälä:
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.
Great, thanks. I'll drop that code then.
Best regards
Thomas
[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 = _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(>hpd_lock);
+ return;
drm_fb_helper_prepare(dev, >helper, 32,
_fb_helper_funcs);
+ dev_priv->display.fbdev.fbdev = ifbdev;
+ INIT_WORK(_priv->display.fbdev.suspend_work,
intel_fbdev_suspend_worker);
+ mutex_init(>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, >helper.client, "i915-
fbdev",
_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, >helper);
- if (ret)
- goto err_drm_client_release;
-
- dev_priv->display.fbdev.fbdev = ifbdev;
- INIT_WORK(_priv->display.fbdev.suspend_work,
intel_fbdev_suspend_worker);
+ drm_client_register(>helper.client);
- return 0;
+ return;
-err_drm_client_release:
- drm_client_release(>helper.client);
err_drm_fb_helper_unprepare:
drm_fb_helper_unprepare(>helper);
+ mutex_destroy(>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(>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(_priv->drm,
FBINFO_STATE_SUSPENDED, true);
-
- if (!current_is_async())
- intel_fbdev_sync(ifbdev);
-
- drm_fb_helper_unregister_info(>helper);
-}
-
-void intel_fbdev_fini(struct drm_i915_private *dev_priv)
-{
- struct intel_fbdev *ifbdev = fetch_and_zero(_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
+++
Re: [Intel-gfx] [PATCH v5 7/7] drm/i915: Implement fbdev emulation as in-kernel client
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 = _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(>hpd_lock);
> >> + return;
> >> drm_fb_helper_prepare(dev, >helper, 32,
> >> _fb_helper_funcs);
> >>
> >> + dev_priv->display.fbdev.fbdev = ifbdev;
> >> + INIT_WORK(_priv->display.fbdev.suspend_work,
> >> intel_fbdev_suspend_worker);
> >> + mutex_init(>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, >helper.client, "i915-
> >> fbdev",
> >> _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, >helper);
> >> - if (ret)
> >> - goto err_drm_client_release;
> >> -
> >> - dev_priv->display.fbdev.fbdev = ifbdev;
> >> - INIT_WORK(_priv->display.fbdev.suspend_work,
> >> intel_fbdev_suspend_worker);
> >> + drm_client_register(>helper.client);
> >>
> >> - return 0;
> >> + return;
> >>
> >> -err_drm_client_release:
> >> - drm_client_release(>helper.client);
> >> err_drm_fb_helper_unprepare:
> >> drm_fb_helper_unprepare(>helper);
> >> + mutex_destroy(>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(>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(_priv->drm,
> >> FBINFO_STATE_SUSPENDED, true);
> >> -
> >> - if (!current_is_async())
> >> - intel_fbdev_sync(ifbdev);
> >> -
> >> - drm_fb_helper_unregister_info(>helper);
> >> -}
> >> -
> >> -void
Re: [Intel-gfx] [PATCH v5 7/7] drm/i915: Implement fbdev emulation as in-kernel client
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 = _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(>hpd_lock);
+ return;
drm_fb_helper_prepare(dev, >helper, 32,
_fb_helper_funcs);
+ dev_priv->display.fbdev.fbdev = ifbdev;
+ INIT_WORK(_priv->display.fbdev.suspend_work,
intel_fbdev_suspend_worker);
+ mutex_init(>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, >helper.client, "i915-
fbdev",
_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, >helper);
- if (ret)
- goto err_drm_client_release;
-
- dev_priv->display.fbdev.fbdev = ifbdev;
- INIT_WORK(_priv->display.fbdev.suspend_work,
intel_fbdev_suspend_worker);
+ drm_client_register(>helper.client);
- return 0;
+ return;
-err_drm_client_release:
- drm_client_release(>helper.client);
err_drm_fb_helper_unprepare:
drm_fb_helper_unprepare(>helper);
+ mutex_destroy(>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(>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(_priv->drm,
FBINFO_STATE_SUSPENDED, true);
-
- if (!current_is_async())
- intel_fbdev_sync(ifbdev);
-
- drm_fb_helper_unregister_info(>helper);
-}
-
-void intel_fbdev_fini(struct drm_i915_private *dev_priv)
-{
- struct intel_fbdev *ifbdev = fetch_and_zero(_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
Re: [Intel-gfx] [PATCH v5 7/7] drm/i915: Implement fbdev emulation as in-kernel client
Hi Thomas, couple of minor comments and a question below.
On Wed, 2023-09-27 at 12:26 +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 i915_fbdev_setup() after i915 has registered its
intel_fbdev_setup
> DRM device. Just like similar code in other drivers, i915 fbdev
> emulation now acts as 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.
>
> 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
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 1 -
> .../drm/i915/display/intel_display_driver.c | 18 --
> drivers/gpu/drm/i915/display/intel_fbdev.c | 180
> --
> drivers/gpu/drm/i915/display/intel_fbdev.h | 20 +-
> drivers/gpu/drm/i915/i915_driver.c | 2 +
> 5 files changed, 83 insertions(+), 138 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index edbcf5968804d..7efa8d2787c39 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -81,7 +81,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 ffdcddd1943e0..213a4ee93ffc2 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -364,10 +364,6 @@ int intel_display_driver_probe(struct
> drm_i915_private *i915)
>
> intel_overlay_setup(i915);
>
> - ret = intel_fbdev_init(>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);
> @@ -392,16 +388,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
> @@ -445,9 +431,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 */
> @@ -484,7 +467,6 @@ void intel_display_driver_unregister(struct
> drm_i915_private *i915)
> if (!HAS_DISPLAY(i915))
> return;
>
> - intel_fbdev_unregister(i915);
> intel_audio_deinit(i915);
>
> /*
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
> b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 39de61d4e7906..100a4aaf1b7e4 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
> #include
> #include
> #include
> @@ -39,6 +38,7 @@
> #include
>
> #include
> +#include
> #include
> #include
> #include
> @@ -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 */
