Re: [82/86] drm/i915: Move custom hotplug code into separate callback

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 19.08.24 um 10:52 schrieb Sui Jingfeng:
Hi, Thomas


I love your patch, yet ...


On 2024/8/16 20:23, Thomas Zimmermann wrote:
i915's fbdev contains additional code for hotplugging a display that
cannot be ported to the common fbdev client. Introduce the callback
struct drm_fb_helper.fb_hotplug and implement it for i915. The fbdev
helpers invoke the callback before handing the hotplug event.

Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tursulin@xxxxxxxxxxx>
Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
Cc: "Thomas Hellström" <thomas.hellstrom@xxxxxxxxxxxxxxx>
---
  drivers/gpu/drm/drm_fb_helper.c            |  6 +++
  drivers/gpu/drm/i915/display/intel_fbdev.c | 43 ++++++++++++----------
  include/drm/drm_fb_helper.h                | 13 +++++++
  3 files changed, 42 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d9e539b0fd1a..92926cb02dfb 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1938,6 +1938,12 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
      if (!drm_fbdev_emulation || !fb_helper)
          return 0;
  +    if (fb_helper->funcs->fb_hotplug) {

We seems need to check the existence on the 'fb_helper->funcs' here,

For example:


if (fb_helper->funcs && fb_helper->funcs->fb_hotplug) {

Otherwise, it will de-reference NULL pointer.
Can be observed on a trivial driver though,
with no monitor(display) connected.

Indeed. That needs to be fixed. Thank you for noting.

To give some context:  I was hoping to remove drm_fb_helper_funcs at some point. fb_probe is now gone with these patches and fb_dirty can certainly be replaced as well. (I once had prototype patches to do that). This leaves the new callbacks for 915, for which I don't have a good alternative solution. So it seems that drm_fb_helper_funcs will only be used by i915/xe in the long term.

Best regards
Thomas



+        err = fb_helper->funcs->fb_hotplug(fb_helper);
+        if (err)
+            return err;
+    }
+
      mutex_lock(&fb_helper->lock);
      if (fb_helper->deferred_setup) {
          err = __drm_fb_helper_initial_config_and_unlock(fb_helper);
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
index c03fb00002a4..abe77ef0bd84 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -305,10 +305,32 @@ static void intelfb_restore(struct drm_fb_helper *fb_helper)
      intel_fbdev_invalidate(ifbdev);
  }
  +static int intelfb_hotplug(struct drm_fb_helper *fb_helper)
+{
+    struct drm_device *dev = fb_helper->client.dev;
+    struct drm_i915_private *dev_priv = to_i915(dev);
+    struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
+    bool send_hpd;
+
+    if (!ifbdev)
+        return -EINVAL;
+
+    mutex_lock(&ifbdev->hpd_lock);
+    send_hpd = !ifbdev->hpd_suspended;
+    ifbdev->hpd_waiting = true;
+    mutex_unlock(&ifbdev->hpd_lock);
+
+    if (!send_hpd || !(ifbdev->vma || dev->fb_helper->deferred_setup))
+        return -EAGAIN;
+
+    return 0;
+}
+
  static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
      .fb_probe = intelfb_create,
      .fb_dirty = intelfb_dirty,
      .fb_restore = intelfb_restore,
+    .fb_hotplug = intelfb_hotplug,
  };
    /*
@@ -557,25 +579,6 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
      intel_fbdev_hpd_set_suspend(dev_priv, state);
  }
  -static int intel_fbdev_output_poll_changed(struct drm_device *dev)
-{
-    struct intel_fbdev *ifbdev = to_i915(dev)->display.fbdev.fbdev;
-    bool send_hpd;
-
-    if (!ifbdev)
-        return -EINVAL;
-
-    mutex_lock(&ifbdev->hpd_lock);
-    send_hpd = !ifbdev->hpd_suspended;
-    ifbdev->hpd_waiting = true;
-    mutex_unlock(&ifbdev->hpd_lock);
-
-    if (send_hpd && (ifbdev->vma || dev->fb_helper->deferred_setup))
-        drm_fb_helper_hotplug_event(dev->fb_helper);
-
-    return 0;
-}
-
  static int intel_fbdev_restore_mode(struct drm_i915_private *dev_priv)
  {
      struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
@@ -637,7 +640,7 @@ static int intel_fbdev_client_hotplug(struct drm_client_dev *client)
      int ret;
        if (dev->fb_helper)
-        return intel_fbdev_output_poll_changed(dev);
+        return drm_fb_helper_hotplug_event(fb_helper);
        ret = drm_fb_helper_init(dev, fb_helper);
      if (ret)
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 34eb77c18a13..3dcb9a60e408 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -112,6 +112,19 @@ struct drm_fb_helper_funcs {
       * TODO: Fix i915 to not require this callback.
       */
      void (*fb_restore)(struct drm_fb_helper *helper);
+
+    /**
+     * @fb_hotplug:
+     *
+     * Driver callback to prepare hotplug event. If set, fbdev
+     * emulation will invoke this callback before sending a hotplug
+     * event.
+     *
+     * Only for i915. Do not use in new code.
+     *
+     * TODO: Fix i915 to not require this callback.
+     */
+    int (*fb_hotplug)(struct drm_fb_helper *helper);
  };
    /**


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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux