| Message ID | 20171030153951.56269-9-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 30, 2017 at 04:39:44PM +0100, Noralf Trønnes wrote: > This driver can use drm_fb_helper_output_poll_changed() as its > .output_poll_changed callback. > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- > drivers/gpu/drm/i915/intel_display.c | 2 +- > drivers/gpu/drm/i915/intel_drv.h | 5 ----- > drivers/gpu/drm/i915/intel_fbdev.c | 8 -------- > 3 files changed, 1 insertion(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index f780f39e0758..b205e2c782bb 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14123,7 +14123,7 @@ static void intel_atomic_state_free(struct drm_atomic_state *state) > static const struct drm_mode_config_funcs intel_mode_funcs = { > .fb_create = intel_user_framebuffer_create, > .get_format_info = intel_get_format_info, > - .output_poll_changed = intel_fbdev_output_poll_changed, > + .output_poll_changed = drm_fb_helper_output_poll_changed, > .atomic_check = intel_atomic_check, > .atomic_commit = intel_atomic_commit, > .atomic_state_alloc = intel_atomic_state_alloc, > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 463ed152e6b1..dfcf5ba220e8 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1607,7 +1607,6 @@ extern void intel_fbdev_initial_config_async(struct drm_device *dev); > extern void intel_fbdev_unregister(struct drm_i915_private *dev_priv); > extern void intel_fbdev_fini(struct drm_i915_private *dev_priv); > extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous); > -extern void intel_fbdev_output_poll_changed(struct drm_device *dev); > extern void intel_fbdev_restore_mode(struct drm_device *dev); > #else > static inline int intel_fbdev_init(struct drm_device *dev) > @@ -1631,10 +1630,6 @@ static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state, bo > { > } > > -static inline void intel_fbdev_output_poll_changed(struct drm_device *dev) > -{ > -} > - > static inline void intel_fbdev_restore_mode(struct drm_device *dev) > { > } > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > index f2bb8116227c..35babbadfc5a 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -796,14 +796,6 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous > console_unlock(); > } > > -void intel_fbdev_output_poll_changed(struct drm_device *dev) > -{ > - struct intel_fbdev *ifbdev = to_i915(dev)->fbdev; > - > - if (ifbdev) This removes the NULL check here, and I think we still need that one for some slightly unclear but probably hilarious reasons. I guess simplest if you just drop the i915 patch as too tricky. -Daniel > - drm_fb_helper_hotplug_event(&ifbdev->helper); > -} > - > void intel_fbdev_restore_mode(struct drm_device *dev) > { > struct intel_fbdev *ifbdev = to_i915(dev)->fbdev; > -- > 2.14.2 >
Den 31.10.2017 11.27, skrev Daniel Vetter: > On Mon, Oct 30, 2017 at 04:39:44PM +0100, Noralf Trønnes wrote: >> This driver can use drm_fb_helper_output_poll_changed() as its >> .output_poll_changed callback. >> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> --- >> drivers/gpu/drm/i915/intel_display.c | 2 +- >> drivers/gpu/drm/i915/intel_drv.h | 5 ----- >> drivers/gpu/drm/i915/intel_fbdev.c | 8 -------- >> 3 files changed, 1 insertion(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index f780f39e0758..b205e2c782bb 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -14123,7 +14123,7 @@ static void intel_atomic_state_free(struct drm_atomic_state *state) >> static const struct drm_mode_config_funcs intel_mode_funcs = { >> .fb_create = intel_user_framebuffer_create, >> .get_format_info = intel_get_format_info, >> - .output_poll_changed = intel_fbdev_output_poll_changed, >> + .output_poll_changed = drm_fb_helper_output_poll_changed, >> .atomic_check = intel_atomic_check, >> .atomic_commit = intel_atomic_commit, >> .atomic_state_alloc = intel_atomic_state_alloc, >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 463ed152e6b1..dfcf5ba220e8 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -1607,7 +1607,6 @@ extern void intel_fbdev_initial_config_async(struct drm_device *dev); >> extern void intel_fbdev_unregister(struct drm_i915_private *dev_priv); >> extern void intel_fbdev_fini(struct drm_i915_private *dev_priv); >> extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous); >> -extern void intel_fbdev_output_poll_changed(struct drm_device *dev); >> extern void intel_fbdev_restore_mode(struct drm_device *dev); >> #else >> static inline int intel_fbdev_init(struct drm_device *dev) >> @@ -1631,10 +1630,6 @@ static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state, bo >> { >> } >> >> -static inline void intel_fbdev_output_poll_changed(struct drm_device *dev) >> -{ >> -} >> - >> static inline void intel_fbdev_restore_mode(struct drm_device *dev) >> { >> } >> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c >> index f2bb8116227c..35babbadfc5a 100644 >> --- a/drivers/gpu/drm/i915/intel_fbdev.c >> +++ b/drivers/gpu/drm/i915/intel_fbdev.c >> @@ -796,14 +796,6 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous >> console_unlock(); >> } >> >> -void intel_fbdev_output_poll_changed(struct drm_device *dev) >> -{ >> - struct intel_fbdev *ifbdev = to_i915(dev)->fbdev; >> - >> - if (ifbdev) > This removes the NULL check here, and I think we still need that one for > some slightly unclear but probably hilarious reasons. > > I guess simplest if you just drop the i915 patch as too tricky. Ok. Several drivers had this NULL check and I did a verification in the drivers fbdev code that conversion was OK. But I left it to the maintainer to know about any code paths that wasn't obvious to me by looking at the fbdev code. On the surface i915 looks OK and here's the rationale: dev->fb_helper is set in drm_fb_helper_init() and cleared in drm_fb_helper_fini(). All fbdev init error paths I've seen call drm_fb_helper_fini(). This means that dev->fb_helper is only set when fbdev is initialized. drm_fb_helper_output_poll_changed() can handle dev->fb_helper == NULL: void drm_fb_helper_output_poll_changed(struct drm_device *dev) { drm_fb_helper_hotplug_event(dev->fb_helper); } int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) { ... if (!drm_fbdev_emulation || !fb_helper) return 0; ... } dev->fb_helper and dev_priv->fbdev are set and cleared in sync: int intel_fbdev_init(struct drm_device *dev) { struct drm_i915_private *dev_priv = to_i915(dev); struct intel_fbdev *ifbdev; int ret; if (WARN_ON(INTEL_INFO(dev_priv)->num_pipes == 0)) return -ENODEV; ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL); if (ifbdev == NULL) return -ENOMEM; drm_fb_helper_prepare(dev, &ifbdev->helper, &intel_fb_helper_funcs); if (!intel_fbdev_init_bios(dev, ifbdev)) ifbdev->preferred_bpp = 32; ret = drm_fb_helper_init(dev, &ifbdev->helper, 4); if (ret) { kfree(ifbdev); return ret; } dev->fb_helper is now set (by drm_fb_helper_init()). dev_priv->fbdev = ifbdev; dev_priv->fbdev is now set INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker); drm_fb_helper_single_add_all_connectors(&ifbdev->helper); return 0; } void intel_fbdev_fini(struct drm_i915_private *dev_priv) { struct intel_fbdev *ifbdev = fetch_and_zero(&dev_priv->fbdev); dev_priv->fbdev is now NULL if (!ifbdev) return; intel_fbdev_destroy(ifbdev); } static void intel_fbdev_destroy(struct intel_fbdev *ifbdev) { /* We rely on the object-free to release the VMA pinning for * the info->screen_base mmaping. Leaking the VMA is simpler than * trying to rectify all the possible error paths leading here. */ drm_fb_helper_fini(&ifbdev->helper); dev->fb_helper is now NULL (cleared by drm_fb_helper_fini()) if (ifbdev->vma) { mutex_lock(&ifbdev->helper.dev->struct_mutex); intel_unpin_fb_vma(ifbdev->vma); mutex_unlock(&ifbdev->helper.dev->struct_mutex); } if (ifbdev->fb) drm_framebuffer_remove(&ifbdev->fb->base); kfree(ifbdev); } But I understand that it's better to be safe than sorry :-) Hopefully all your CI work will make checking patches like this a breeze one day. Noralf. > -Daniel > >> - drm_fb_helper_hotplug_event(&ifbdev->helper); >> -} >> - >> void intel_fbdev_restore_mode(struct drm_device *dev) >> { >> struct intel_fbdev *ifbdev = to_i915(dev)->fbdev; >> -- >> 2.14.2 >>
On Tue, Oct 31, 2017 at 03:26:50PM +0100, Noralf Trønnes wrote: > > Den 31.10.2017 11.27, skrev Daniel Vetter: > > On Mon, Oct 30, 2017 at 04:39:44PM +0100, Noralf Trønnes wrote: > > > This driver can use drm_fb_helper_output_poll_changed() as its > > > .output_poll_changed callback. > > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 2 +- > > > drivers/gpu/drm/i915/intel_drv.h | 5 ----- > > > drivers/gpu/drm/i915/intel_fbdev.c | 8 -------- > > > 3 files changed, 1 insertion(+), 14 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index f780f39e0758..b205e2c782bb 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -14123,7 +14123,7 @@ static void intel_atomic_state_free(struct drm_atomic_state *state) > > > static const struct drm_mode_config_funcs intel_mode_funcs = { > > > .fb_create = intel_user_framebuffer_create, > > > .get_format_info = intel_get_format_info, > > > - .output_poll_changed = intel_fbdev_output_poll_changed, > > > + .output_poll_changed = drm_fb_helper_output_poll_changed, > > > .atomic_check = intel_atomic_check, > > > .atomic_commit = intel_atomic_commit, > > > .atomic_state_alloc = intel_atomic_state_alloc, > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > > index 463ed152e6b1..dfcf5ba220e8 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -1607,7 +1607,6 @@ extern void intel_fbdev_initial_config_async(struct drm_device *dev); > > > extern void intel_fbdev_unregister(struct drm_i915_private *dev_priv); > > > extern void intel_fbdev_fini(struct drm_i915_private *dev_priv); > > > extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous); > > > -extern void intel_fbdev_output_poll_changed(struct drm_device *dev); > > > extern void intel_fbdev_restore_mode(struct drm_device *dev); > > > #else > > > static inline int intel_fbdev_init(struct drm_device *dev) > > > @@ -1631,10 +1630,6 @@ static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state, bo > > > { > > > } > > > -static inline void intel_fbdev_output_poll_changed(struct drm_device *dev) > > > -{ > > > -} > > > - > > > static inline void intel_fbdev_restore_mode(struct drm_device *dev) > > > { > > > } > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > > > index f2bb8116227c..35babbadfc5a 100644 > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > > @@ -796,14 +796,6 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous > > > console_unlock(); > > > } > > > -void intel_fbdev_output_poll_changed(struct drm_device *dev) > > > -{ > > > - struct intel_fbdev *ifbdev = to_i915(dev)->fbdev; > > > - > > > - if (ifbdev) > > This removes the NULL check here, and I think we still need that one for > > some slightly unclear but probably hilarious reasons. > > > > I guess simplest if you just drop the i915 patch as too tricky. > > Ok. > > Several drivers had this NULL check and I did a verification in the drivers > fbdev code that conversion was OK. But I left it to the maintainer to know > about any code paths that wasn't obvious to me by looking at the fbdev code. > > On the surface i915 looks OK and here's the rationale: > > dev->fb_helper is set in drm_fb_helper_init() and cleared in > drm_fb_helper_fini(). > All fbdev init error paths I've seen call drm_fb_helper_fini(). > This means that dev->fb_helper is only set when fbdev is initialized. > > drm_fb_helper_output_poll_changed() can handle dev->fb_helper == NULL: > > void drm_fb_helper_output_poll_changed(struct drm_device *dev) > { > drm_fb_helper_hotplug_event(dev->fb_helper); > } > > int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) > { > ... > if (!drm_fbdev_emulation || !fb_helper) > return 0; > ... > } Argh, I totally missed that your patch series is adding these checks. So looks all good, on the driver patches (except i915): Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> But probably good if you wait another week or so before pushing those. Core bits imo should land right away, that also gives more options to driver maintainers to push themselves. > dev->fb_helper and dev_priv->fbdev are set and cleared in sync: > > int intel_fbdev_init(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_fbdev *ifbdev; > int ret; > > if (WARN_ON(INTEL_INFO(dev_priv)->num_pipes == 0)) > return -ENODEV; > > ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL); > if (ifbdev == NULL) > return -ENOMEM; > > drm_fb_helper_prepare(dev, &ifbdev->helper, &intel_fb_helper_funcs); > > if (!intel_fbdev_init_bios(dev, ifbdev)) > ifbdev->preferred_bpp = 32; > > ret = drm_fb_helper_init(dev, &ifbdev->helper, 4); > if (ret) { > kfree(ifbdev); > return ret; > } > > dev->fb_helper is now set (by drm_fb_helper_init()). > > dev_priv->fbdev = ifbdev; > > dev_priv->fbdev is now set > > INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker); > > drm_fb_helper_single_add_all_connectors(&ifbdev->helper); > > return 0; > } > > void intel_fbdev_fini(struct drm_i915_private *dev_priv) > { > struct intel_fbdev *ifbdev = fetch_and_zero(&dev_priv->fbdev); > > dev_priv->fbdev is now NULL > > if (!ifbdev) > return; > > intel_fbdev_destroy(ifbdev); > } > > static void intel_fbdev_destroy(struct intel_fbdev *ifbdev) > { > /* We rely on the object-free to release the VMA pinning for > * the info->screen_base mmaping. Leaking the VMA is simpler than > * trying to rectify all the possible error paths leading here. > */ > > drm_fb_helper_fini(&ifbdev->helper); > > dev->fb_helper is now NULL (cleared by drm_fb_helper_fini()) > > if (ifbdev->vma) { > mutex_lock(&ifbdev->helper.dev->struct_mutex); > intel_unpin_fb_vma(ifbdev->vma); > mutex_unlock(&ifbdev->helper.dev->struct_mutex); > } > > if (ifbdev->fb) > drm_framebuffer_remove(&ifbdev->fb->base); > > kfree(ifbdev); > } > > > But I understand that it's better to be safe than sorry :-) > > Hopefully all your CI work will make checking patches like this a breeze one > day. Already works, just submit the i915-only patch (after you've merged the core work into drm-misc-next and drm-tip is rebuilt) to the intel-gfx mailing list, and CI will pick it up and stress test it for you. Once that's done the i915 patch is also ready for merge, for that one: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Please include that tag when resubmitting. Sorry for being a bit blind, I guess coffee didn't quite work yet :-) Thanks, Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f780f39e0758..b205e2c782bb 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14123,7 +14123,7 @@ static void intel_atomic_state_free(struct drm_atomic_state *state) static const struct drm_mode_config_funcs intel_mode_funcs = { .fb_create = intel_user_framebuffer_create, .get_format_info = intel_get_format_info, - .output_poll_changed = intel_fbdev_output_poll_changed, + .output_poll_changed = drm_fb_helper_output_poll_changed, .atomic_check = intel_atomic_check, .atomic_commit = intel_atomic_commit, .atomic_state_alloc = intel_atomic_state_alloc, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 463ed152e6b1..dfcf5ba220e8 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1607,7 +1607,6 @@ extern void intel_fbdev_initial_config_async(struct drm_device *dev); extern void intel_fbdev_unregister(struct drm_i915_private *dev_priv); extern void intel_fbdev_fini(struct drm_i915_private *dev_priv); extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous); -extern void intel_fbdev_output_poll_changed(struct drm_device *dev); extern void intel_fbdev_restore_mode(struct drm_device *dev); #else static inline int intel_fbdev_init(struct drm_device *dev) @@ -1631,10 +1630,6 @@ static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state, bo { } -static inline void intel_fbdev_output_poll_changed(struct drm_device *dev) -{ -} - static inline void intel_fbdev_restore_mode(struct drm_device *dev) { } diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index f2bb8116227c..35babbadfc5a 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -796,14 +796,6 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous console_unlock(); } -void intel_fbdev_output_poll_changed(struct drm_device *dev) -{ - struct intel_fbdev *ifbdev = to_i915(dev)->fbdev; - - if (ifbdev) - drm_fb_helper_hotplug_event(&ifbdev->helper); -} - void intel_fbdev_restore_mode(struct drm_device *dev) { struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
This driver can use drm_fb_helper_output_poll_changed() as its .output_poll_changed callback. Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- drivers/gpu/drm/i915/intel_display.c | 2 +- drivers/gpu/drm/i915/intel_drv.h | 5 ----- drivers/gpu/drm/i915/intel_fbdev.c | 8 -------- 3 files changed, 1 insertion(+), 14 deletions(-)