* [PATCH 0/3] drm/i915: Module unload fixes
@ 2015-11-06 13:08 ville.syrjala
2015-11-06 13:08 ` [PATCH 1/3] drm/i915: Kill intel_runtime_pm_disable() ville.syrjala
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: ville.syrjala @ 2015-11-06 13:08 UTC (permalink / raw)
To: intel-gfx
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
This series fixes an rpm leak during driver unload, and reorganizes the fbdev
init/cleanup to make a bit more sense.
Ville Syrjälä (3):
drm/i915: Kill intel_runtime_pm_disable()
drm/i915: Do fbdev fini first during unload
drm/i915: Move the fbdev async_schedule() into intel_fbdev.c
drivers/gpu/drm/i915/i915_dma.c | 7 +++----
drivers/gpu/drm/i915/intel_drv.h | 4 ++--
drivers/gpu/drm/i915/intel_fbdev.c | 7 ++++++-
drivers/gpu/drm/i915/intel_runtime_pm.c | 17 -----------------
4 files changed, 11 insertions(+), 24 deletions(-)
--
2.4.10
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/3] drm/i915: Kill intel_runtime_pm_disable() 2015-11-06 13:08 [PATCH 0/3] drm/i915: Module unload fixes ville.syrjala @ 2015-11-06 13:08 ` ville.syrjala 2015-11-10 17:20 ` Jesse Barnes 2015-11-06 13:08 ` [PATCH 2/3] drm/i915: Do fbdev fini first during unload ville.syrjala 2015-11-06 13:08 ` [PATCH 3/3] drm/i915: Move the fbdev async_schedule() into intel_fbdev.c ville.syrjala 2 siblings, 1 reply; 14+ messages in thread From: ville.syrjala @ 2015-11-06 13:08 UTC (permalink / raw) To: intel-gfx; +Cc: Daniel Stone From: Ville Syrjälä <ville.syrjala@linux.intel.com> intel_runtime_pm_disable() takes an extra rpm reference which combined with the one we leak from intel_display_set_init_power() leaves the usage count at <original>+1 after the driver has been unloaded. The original ref is dropped explicitly in intel_runtime_pm_enable(). So the next time we load the driver we can no longer do runtime PM ever. This used to work, but commit 292b990e86ab ("drm/i915: Update power domains on readout.") broke things by not dropping the init power domain during fbdev teardown. Based on the comment in intel_power_domains_fini(), the way it used to to work wasn't intentional. As in we weren't supposed to drop the init power during driver unload. And since we no longer do, we now leak an extra rpm reference. So fix things by throwing intel_runtime_pm_disable() to the bin, so that the only leaked reference comes from the init power domain. Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Daniel Stone <daniels@collabora.com> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> Fixes: 292b990e86ab ("drm/i915: Update power domains on readout.") Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_runtime_pm.c | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 1017555..bdc9ed4 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -1847,21 +1847,6 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv) return 0; } -static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv) -{ - struct drm_device *dev = dev_priv->dev; - struct device *device = &dev->pdev->dev; - - if (!HAS_RUNTIME_PM(dev)) - return; - - if (!intel_enable_rc6(dev)) - return; - - /* Make sure we're not suspended first. */ - pm_runtime_get_sync(device); -} - /** * intel_power_domains_fini - finalizes the power domain structures * @dev_priv: i915 device instance @@ -1872,8 +1857,6 @@ static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv) */ void intel_power_domains_fini(struct drm_i915_private *dev_priv) { - intel_runtime_pm_disable(dev_priv); - /* The i915.ko module is still not prepared to be loaded when * the power well is not enabled, so just enable it in case * we're going to unload/reload. */ -- 2.4.10 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] drm/i915: Kill intel_runtime_pm_disable() 2015-11-06 13:08 ` [PATCH 1/3] drm/i915: Kill intel_runtime_pm_disable() ville.syrjala @ 2015-11-10 17:20 ` Jesse Barnes 2015-11-10 23:49 ` Rafael J. Wysocki 0 siblings, 1 reply; 14+ messages in thread From: Jesse Barnes @ 2015-11-10 17:20 UTC (permalink / raw) To: ville.syrjala, intel-gfx; +Cc: Rafael J. Wysocki, Daniel Stone On 11/06/2015 05:08 AM, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > intel_runtime_pm_disable() takes an extra rpm reference which combined > with the one we leak from intel_display_set_init_power() leaves the > usage count at <original>+1 after the driver has been unloaded. > The original ref is dropped explicitly in intel_runtime_pm_enable(). > So the next time we load the driver we can no longer do runtime PM ever. > > This used to work, but > commit 292b990e86ab ("drm/i915: Update power domains on readout.") > broke things by not dropping the init power domain during fbdev > teardown. Based on the comment in intel_power_domains_fini(), the > way it used to to work wasn't intentional. As in we weren't supposed > to drop the init power during driver unload. And since we no longer > do, we now leak an extra rpm reference. > > So fix things by throwing intel_runtime_pm_disable() to the bin, so > that the only leaked reference comes from the init power domain. > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Daniel Stone <daniels@collabora.com> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > Fixes: 292b990e86ab ("drm/i915: Update power domains on readout.") > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_runtime_pm.c | 17 ----------------- > 1 file changed, 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 1017555..bdc9ed4 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -1847,21 +1847,6 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv) > return 0; > } > > -static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv) > -{ > - struct drm_device *dev = dev_priv->dev; > - struct device *device = &dev->pdev->dev; > - > - if (!HAS_RUNTIME_PM(dev)) > - return; > - > - if (!intel_enable_rc6(dev)) > - return; > - > - /* Make sure we're not suspended first. */ > - pm_runtime_get_sync(device); > -} > - > /** > * intel_power_domains_fini - finalizes the power domain structures > * @dev_priv: i915 device instance > @@ -1872,8 +1857,6 @@ static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv) > */ > void intel_power_domains_fini(struct drm_i915_private *dev_priv) > { > - intel_runtime_pm_disable(dev_priv); > - > /* The i915.ko module is still not prepared to be loaded when > * the power well is not enabled, so just enable it in case > * we're going to unload/reload. */ > Yeah I guess this is fine. Will we still disable RPM on unload? What's the expected behavior here? Cc'ing Rafael. Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] drm/i915: Kill intel_runtime_pm_disable() 2015-11-10 17:20 ` Jesse Barnes @ 2015-11-10 23:49 ` Rafael J. Wysocki 0 siblings, 0 replies; 14+ messages in thread From: Rafael J. Wysocki @ 2015-11-10 23:49 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx, Daniel Stone On Tuesday, November 10, 2015 09:20:56 AM Jesse Barnes wrote: > On 11/06/2015 05:08 AM, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > intel_runtime_pm_disable() takes an extra rpm reference which combined > > with the one we leak from intel_display_set_init_power() leaves the > > usage count at <original>+1 after the driver has been unloaded. > > The original ref is dropped explicitly in intel_runtime_pm_enable(). > > So the next time we load the driver we can no longer do runtime PM ever. > > > > This used to work, but > > commit 292b990e86ab ("drm/i915: Update power domains on readout.") > > broke things by not dropping the init power domain during fbdev > > teardown. Based on the comment in intel_power_domains_fini(), the > > way it used to to work wasn't intentional. As in we weren't supposed > > to drop the init power during driver unload. And since we no longer > > do, we now leak an extra rpm reference. > > > > So fix things by throwing intel_runtime_pm_disable() to the bin, so > > that the only leaked reference comes from the init power domain. > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Daniel Stone <daniels@collabora.com> > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > > Fixes: 292b990e86ab ("drm/i915: Update power domains on readout.") > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_runtime_pm.c | 17 ----------------- > > 1 file changed, 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > > index 1017555..bdc9ed4 100644 > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > @@ -1847,21 +1847,6 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv) > > return 0; > > } > > > > -static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv) > > -{ > > - struct drm_device *dev = dev_priv->dev; > > - struct device *device = &dev->pdev->dev; > > - > > - if (!HAS_RUNTIME_PM(dev)) > > - return; > > - > > - if (!intel_enable_rc6(dev)) > > - return; > > - > > - /* Make sure we're not suspended first. */ > > - pm_runtime_get_sync(device); > > -} > > - > > /** > > * intel_power_domains_fini - finalizes the power domain structures > > * @dev_priv: i915 device instance > > @@ -1872,8 +1857,6 @@ static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv) > > */ > > void intel_power_domains_fini(struct drm_i915_private *dev_priv) > > { > > - intel_runtime_pm_disable(dev_priv); > > - > > /* The i915.ko module is still not prepared to be loaded when > > * the power well is not enabled, so just enable it in case > > * we're going to unload/reload. */ > > > > Yeah I guess this is fine. Will we still disable RPM on unload? What's > the expected behavior here? Cc'ing Rafael. If that's a PCI device, you don't have to do anything with it. In fact, you aren't expected to do anything with it even. Thanks, Rafael _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] drm/i915: Do fbdev fini first during unload 2015-11-06 13:08 [PATCH 0/3] drm/i915: Module unload fixes ville.syrjala 2015-11-06 13:08 ` [PATCH 1/3] drm/i915: Kill intel_runtime_pm_disable() ville.syrjala @ 2015-11-06 13:08 ` ville.syrjala 2015-11-06 13:33 ` Chris Wilson 2015-11-06 14:16 ` Ville Syrjälä 2015-11-06 13:08 ` [PATCH 3/3] drm/i915: Move the fbdev async_schedule() into intel_fbdev.c ville.syrjala 2 siblings, 2 replies; 14+ messages in thread From: ville.syrjala @ 2015-11-06 13:08 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> We set up fbdev last during load, so doing the fbdev cleanup should be first. We weren't supposed to drop the init power during driver load, but since the fbdev teardown happened after intel_power_domains_fini() that could have happened due in one of two ways. First it could have happened during the modeset caused by normal fbdev cleanup. But in addition it could have happened already via the intel_fbdev_initial_config() since that is executed asynhronously, and the async_synchronize_full() was done during fbdev cleanup, after intel_power_domains_fini(). All of that got eliminated by commit 292b990e86abc ("drm/i915: Update power domains on readout.") since we now drop the init power synchronously during driver load. So there is no real bug wrt. the init power anymore, but still it seems better to do the fbdev cleanup first, before we've potentially cleaned up something else important. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/i915_dma.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index ffcb9c6..c58048f 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1132,6 +1132,8 @@ int i915_driver_unload(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; int ret; + intel_fbdev_fini(dev); + i915_audio_component_cleanup(dev_priv); ret = i915_gem_suspend(dev); @@ -1154,8 +1156,6 @@ int i915_driver_unload(struct drm_device *dev) acpi_video_unregister(); - intel_fbdev_fini(dev); - drm_vblank_cleanup(dev); intel_modeset_cleanup(dev); -- 2.4.10 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] drm/i915: Do fbdev fini first during unload 2015-11-06 13:08 ` [PATCH 2/3] drm/i915: Do fbdev fini first during unload ville.syrjala @ 2015-11-06 13:33 ` Chris Wilson 2015-11-06 14:16 ` Ville Syrjälä 1 sibling, 0 replies; 14+ messages in thread From: Chris Wilson @ 2015-11-06 13:33 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx On Fri, Nov 06, 2015 at 03:08:32PM +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > We set up fbdev last during load, so doing the fbdev cleanup should be > first. > > We weren't supposed to drop the init power during driver load, but since > the fbdev teardown happened after intel_power_domains_fini() that could > have happened due in one of two ways. First it could have happened > during the modeset caused by normal fbdev cleanup. But in addition it > could have happened already via the intel_fbdev_initial_config() since > that is executed asynhronously, and the async_synchronize_full() was > done during fbdev cleanup, after intel_power_domains_fini(). All of > that got eliminated by > commit 292b990e86abc ("drm/i915: Update power domains on readout.") > since we now drop the init power synchronously during driver load. > > So there is no real bug wrt. the init power anymore, but still it seems > better to do the fbdev cleanup first, before we've potentially cleaned > up something else important. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Hmm, a good onion and some nice cheese make a fine sandwich. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] drm/i915: Do fbdev fini first during unload 2015-11-06 13:08 ` [PATCH 2/3] drm/i915: Do fbdev fini first during unload ville.syrjala 2015-11-06 13:33 ` Chris Wilson @ 2015-11-06 14:16 ` Ville Syrjälä 1 sibling, 0 replies; 14+ messages in thread From: Ville Syrjälä @ 2015-11-06 14:16 UTC (permalink / raw) To: intel-gfx On Fri, Nov 06, 2015 at 03:08:32PM +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > We set up fbdev last during load, so doing the fbdev cleanup should be > first. > > We weren't supposed to drop the init power during driver load, but since ^^^^ That should have said "unload". > the fbdev teardown happened after intel_power_domains_fini() that could > have happened due in one of two ways. First it could have happened > during the modeset caused by normal fbdev cleanup. But in addition it > could have happened already via the intel_fbdev_initial_config() since > that is executed asynhronously, and the async_synchronize_full() was > done during fbdev cleanup, after intel_power_domains_fini(). All of > that got eliminated by > commit 292b990e86abc ("drm/i915: Update power domains on readout.") > since we now drop the init power synchronously during driver load. > > So there is no real bug wrt. the init power anymore, but still it seems > better to do the fbdev cleanup first, before we've potentially cleaned > up something else important. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_dma.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index ffcb9c6..c58048f 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1132,6 +1132,8 @@ int i915_driver_unload(struct drm_device *dev) > struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > > + intel_fbdev_fini(dev); > + > i915_audio_component_cleanup(dev_priv); > > ret = i915_gem_suspend(dev); > @@ -1154,8 +1156,6 @@ int i915_driver_unload(struct drm_device *dev) > > acpi_video_unregister(); > > - intel_fbdev_fini(dev); > - > drm_vblank_cleanup(dev); > > intel_modeset_cleanup(dev); > -- > 2.4.10 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] drm/i915: Move the fbdev async_schedule() into intel_fbdev.c 2015-11-06 13:08 [PATCH 0/3] drm/i915: Module unload fixes ville.syrjala 2015-11-06 13:08 ` [PATCH 1/3] drm/i915: Kill intel_runtime_pm_disable() ville.syrjala 2015-11-06 13:08 ` [PATCH 2/3] drm/i915: Do fbdev fini first during unload ville.syrjala @ 2015-11-06 13:08 ` ville.syrjala 2015-11-06 13:30 ` Chris Wilson ` (2 more replies) 2 siblings, 3 replies; 14+ messages in thread From: ville.syrjala @ 2015-11-06 13:08 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> 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. 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@linux.intel.com> --- 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] drm/i915: Move the fbdev async_schedule() into intel_fbdev.c 2015-11-06 13:08 ` [PATCH 3/3] drm/i915: Move the fbdev async_schedule() into intel_fbdev.c ville.syrjala @ 2015-11-06 13:30 ` Chris Wilson 2015-11-06 17:04 ` Jesse Barnes 2015-11-08 16:44 ` Lukas Wunner 2 siblings, 0 replies; 14+ messages in thread From: Chris Wilson @ 2015-11-06 13:30 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx On Fri, Nov 06, 2015 at 03:08:33PM +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > 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. > > 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@linux.intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] drm/i915: Move the fbdev async_schedule() into intel_fbdev.c 2015-11-06 13:08 ` [PATCH 3/3] drm/i915: Move the fbdev async_schedule() into intel_fbdev.c ville.syrjala 2015-11-06 13:30 ` Chris Wilson @ 2015-11-06 17:04 ` Jesse Barnes 2015-11-08 16:44 ` Lukas Wunner 2 siblings, 0 replies; 14+ messages in thread From: Jesse Barnes @ 2015-11-06 17:04 UTC (permalink / raw) To: ville.syrjala, intel-gfx On 11/06/2015 05:08 AM, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > 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. > > 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@linux.intel.com> > --- > 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; > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] drm/i915: Move the fbdev async_schedule() into intel_fbdev.c 2015-11-06 13:08 ` [PATCH 3/3] drm/i915: Move the fbdev async_schedule() into intel_fbdev.c ville.syrjala 2015-11-06 13:30 ` Chris Wilson 2015-11-06 17:04 ` Jesse Barnes @ 2015-11-08 16:44 ` Lukas Wunner 2015-11-09 11:00 ` Ville Syrjälä 2 siblings, 1 reply; 14+ messages in thread From: Lukas Wunner @ 2015-11-08 16:44 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx Hi Ville, On Fri, Nov 06, 2015 at 03:08:33PM +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > 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@wunner.de> 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@linux.intel.com> > --- > 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] drm/i915: Move the fbdev async_schedule() into intel_fbdev.c 2015-11-08 16:44 ` Lukas Wunner @ 2015-11-09 11:00 ` Ville Syrjälä 2015-11-10 16:27 ` Lukas Wunner 0 siblings, 1 reply; 14+ messages in thread From: Ville Syrjälä @ 2015-11-09 11:00 UTC (permalink / raw) To: Lukas Wunner; +Cc: intel-gfx 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@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > 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@wunner.de> > 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. > 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@linux.intel.com> > > --- > > 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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] drm/i915: Move the fbdev async_schedule() into intel_fbdev.c 2015-11-09 11:00 ` Ville Syrjälä @ 2015-11-10 16:27 ` Lukas Wunner 2015-11-11 11:46 ` Ville Syrjälä 0 siblings, 1 reply; 14+ messages in thread From: Lukas Wunner @ 2015-11-10 16:27 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx 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@linux.intel.com wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > 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@wunner.de> > > > > 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@linux.intel.com> > > > --- > > > 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@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] drm/i915: Move the fbdev async_schedule() into intel_fbdev.c 2015-11-10 16:27 ` Lukas Wunner @ 2015-11-11 11:46 ` Ville Syrjälä 0 siblings, 0 replies; 14+ messages in thread From: Ville Syrjälä @ 2015-11-11 11:46 UTC (permalink / raw) To: Lukas Wunner; +Cc: intel-gfx On Tue, Nov 10, 2015 at 05:27:55PM +0100, Lukas Wunner wrote: > 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@linux.intel.com wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > 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@wunner.de> > > > > > > > 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. OK. Series pushed to dinq. Thanks for the reviews. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-11-11 11:46 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-11-06 13:08 [PATCH 0/3] drm/i915: Module unload fixes ville.syrjala 2015-11-06 13:08 ` [PATCH 1/3] drm/i915: Kill intel_runtime_pm_disable() ville.syrjala 2015-11-10 17:20 ` Jesse Barnes 2015-11-10 23:49 ` Rafael J. Wysocki 2015-11-06 13:08 ` [PATCH 2/3] drm/i915: Do fbdev fini first during unload ville.syrjala 2015-11-06 13:33 ` Chris Wilson 2015-11-06 14:16 ` Ville Syrjälä 2015-11-06 13:08 ` [PATCH 3/3] drm/i915: Move the fbdev async_schedule() into intel_fbdev.c ville.syrjala 2015-11-06 13:30 ` Chris Wilson 2015-11-06 17:04 ` Jesse Barnes 2015-11-08 16:44 ` Lukas Wunner 2015-11-09 11:00 ` Ville Syrjälä 2015-11-10 16:27 ` Lukas Wunner 2015-11-11 11:46 ` Ville Syrjälä
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox