| Message ID | 20240816125408.310253-83-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Provide client setup helper and convert drivers | expand |
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@suse.de> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Tvrtko Ursulin <tursulin@ursulin.net> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Cc: "Thomas Hellström" <thomas.hellstrom@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); > }; > > /**
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@suse.de> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Tvrtko Ursulin <tursulin@ursulin.net> >> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >> Cc: "Thomas Hellström" <thomas.hellstrom@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); >> }; >> /** >
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@suse.de> >>> Cc: Jani Nikula <jani.nikula@linux.intel.com> >>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>> Cc: Tvrtko Ursulin <tursulin@ursulin.net> >>> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >>> Cc: "Thomas Hellström" <thomas.hellstrom@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
Hi Am 20.08.24 um 12:39 schrieb Sui Jingfeng: > > > 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. The correct place to do acceleration would be in the blit helpers in drm_format_helper.c. But no one bothered so far. For fbdev-only acceleration, DRM drivers could have custom fb_ops for accelerated drawing, copying, etc. Few cared and almost all removed that code at some point. The latest consent was that the complexity of the implementation outweights the performance gains. Even the few devs who actually asked for it didn't bother in the end. Best regards Thomas > > Best regards, > Sui >
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) { + 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); }; /**
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@suse.de> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Tvrtko Ursulin <tursulin@ursulin.net> Cc: Lucas De Marchi <lucas.demarchi@intel.com> Cc: "Thomas Hellström" <thomas.hellstrom@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(-)