Sui Jingfeng
2024-Aug-19 08:52 UTC
[82/86] drm/i915: Move custom hotplug code into separate callback
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 at suse.de> > Cc: Jani Nikula <jani.nikula at linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com> > Cc: Tvrtko Ursulin <tursulin at ursulin.net> > Cc: Lucas De Marchi <lucas.demarchi at intel.com> > Cc: "Thomas Hellstr?m" <thomas.hellstrom at linux.intel.com> > --- > 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.> + 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); > }; > > /**-- Best regards, Sui
Thomas Zimmermann
2024-Aug-20 07:39 UTC
[82/86] drm/i915: Move custom hotplug code into separate callback
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 at suse.de> >> Cc: Jani Nikula <jani.nikula at linux.intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com> >> Cc: Tvrtko Ursulin <tursulin at ursulin.net> >> Cc: Lucas De Marchi <lucas.demarchi at intel.com> >> Cc: "Thomas Hellstr?m" <thomas.hellstrom at linux.intel.com> >> --- >> ? 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)