Hi Ville, On Mon, Nov 09, 2015 at 01:00:50PM +0200, Ville Syrjälä wrote: > On Sun, Nov 08, 2015 at 05:44:37PM +0100, Lukas Wunner wrote: > > Hi Ville, > > > > On Fri, Nov 06, 2015 at 03:08:33PM +0200, ville.syrjala@xxxxxxxxxxxxxxx wrote: > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > Reading the driver load/unload code leaves one confused as there's > > > an async_schedule() in the load, but not async_synchronize_full() > > > in sight. In fact it's hidden inside intel_fbdev.c. So let's move the > > > async_schedule() into intel_fbdev.c as well so that it's next to the > > > async_synchronize_full(), which should make the relationship easier > > > to see. > > > > Hm, what do you think about solving it the other way round, i.e. moving > > the async_synchronize_full() to i915_driver_unload()? Incidentally I was > > working on this same part of the code and that's how I solved it. This way > > it's possible to call intel_fbdev_fini() from intel_fbdev_initial_config(). > > With your solution this would deadlock. > > > > Link: https://github.com/l1k/linux/commit/aa12badac846 > > Message-Id: <aa12badac846f24b49d83768146b62e2ac159eb3.1446987413.git.lukas@xxxxxxxxx> > > > > I think I'd still like to hide it all in intel_fbdev.c. You could just > split the fbdev_fini() into two parts; one doing the real work, and the > second one just doing async_synchronize + call the first one. Looking at this with a fresh pair of eyeballs I realized I could simply call async_synchronize_full() conditionally if (!current_is_async()), thereby differentiating between fbdev_fini() being called from i915_driver_unload() versus intel_fbdev_initial_config(). If your patch gets pushed, I think I'll rebase and solve it like that. Thanks for your input, Lukas > > > Thanks, > > > > Lukas > > > > > > > Plus this way we won't schedule a nop function call when fbdev is > > > disabled. And we were passing a pointer to a static inline > > > function to async_schedule(), which seems rather dubious to me. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/i915_dma.c | 3 +-- > > > drivers/gpu/drm/i915/intel_drv.h | 4 ++-- > > > drivers/gpu/drm/i915/intel_fbdev.c | 7 ++++++- > > > 3 files changed, 9 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > > index c58048f..cae3d78 100644 > > > --- a/drivers/gpu/drm/i915/i915_dma.c > > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > > @@ -28,7 +28,6 @@ > > > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > > > -#include <linux/async.h> > > > #include <drm/drmP.h> > > > #include <drm/drm_crtc_helper.h> > > > #include <drm/drm_fb_helper.h> > > > @@ -437,7 +436,7 @@ static int i915_load_modeset_init(struct drm_device *dev) > > > * scanning against hotplug events. Hence do this first and ignore the > > > * tiny window where we will loose hotplug notifactions. > > > */ > > > - async_schedule(intel_fbdev_initial_config, dev_priv); > > > + intel_fbdev_initial_config_async(dev); > > > > > > drm_kms_helper_poll_init(dev); > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > > index 00d9882..50c78b6 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -1298,7 +1298,7 @@ void intel_dvo_init(struct drm_device *dev); > > > /* legacy fbdev emulation in intel_fbdev.c */ > > > #ifdef CONFIG_DRM_FBDEV_EMULATION > > > extern int intel_fbdev_init(struct drm_device *dev); > > > -extern void intel_fbdev_initial_config(void *data, async_cookie_t cookie); > > > +extern void intel_fbdev_initial_config_async(struct drm_device *dev); > > > extern void intel_fbdev_fini(struct drm_device *dev); > > > 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); > > > @@ -1309,7 +1309,7 @@ static inline int intel_fbdev_init(struct drm_device *dev) > > > return 0; > > > } > > > > > > -static inline void intel_fbdev_initial_config(void *data, async_cookie_t cookie) > > > +static inline void intel_fbdev_initial_config_async(struct drm_device *dev) > > > { > > > } > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > > > index 840d6bf..fe1fdb6 100644 > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > > @@ -702,7 +702,7 @@ int intel_fbdev_init(struct drm_device *dev) > > > return 0; > > > } > > > > > > -void intel_fbdev_initial_config(void *data, async_cookie_t cookie) > > > +static void intel_fbdev_initial_config(void *data, async_cookie_t cookie) > > > { > > > struct drm_i915_private *dev_priv = data; > > > struct intel_fbdev *ifbdev = dev_priv->fbdev; > > > @@ -711,6 +711,11 @@ void intel_fbdev_initial_config(void *data, async_cookie_t cookie) > > > drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp); > > > } > > > > > > +void intel_fbdev_initial_config_async(struct drm_device *dev) > > > +{ > > > + async_schedule(intel_fbdev_initial_config, to_i915(dev)); > > > +} > > > + > > > void intel_fbdev_fini(struct drm_device *dev) > > > { > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > -- > > > 2.4.10 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
