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)
Sui Jingfeng
2024-Aug-20 10:39 UTC
[82/86] drm/i915: Move custom hotplug code into separate callback
Hi, On 2024/8/20 15:39, Thomas Zimmermann wrote:> 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. >Thanks for you efforts then.> To give some context:? I was hoping to remove drm_fb_helper_funcs at > some point.Yeah, too many helper functions may make peoples daze.> 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).Well, the grammar of?"ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes);" looks strange, It's lengthy and I observed you have cleaned it up at the last patch. Which also eliminates one pair "if and else" clause, the codes looks more fluent now.> 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. >Well, since it is a DRM client now, maybe we could try to drop it?into struct drm_driver. Just like the '.fbdev_probe' callback, this may help to achieve a 100% DRM-based console/logger IMO. Besides, a lot of DRM driver instances has the DMA/2D acceleration hardware, promote it into drm_driver structure may has the potential to utilize hardware acceleration. Drivers will more easily to have custom implementation. I'm not 100% sure if it will only be used by i915 in the future. Best regards, Sui