| Message ID | 20190401201411.2611-1-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: initialize uncore->lock in uncore_init | expand |
Quoting Daniele Ceraolo Spurio (2019-04-01 21:14:11) > Now that all the uncore management is hidden under the uncore struct, > move the lock initialization under the uncore_init as well for better > encapsulation. > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_drv.c | 1 - > drivers/gpu/drm/i915/intel_uncore.c | 2 ++ > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 0ca57dc5da5c..667177b7d821 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -873,7 +873,6 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) > spin_lock_init(&dev_priv->irq_lock); > spin_lock_init(&dev_priv->gpu_error.lock); > mutex_init(&dev_priv->backlight_lock); > - spin_lock_init(&dev_priv->uncore.lock); > > mutex_init(&dev_priv->sb_lock); > mutex_init(&dev_priv->av_mutex); > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 106df24f20a5..250496f792e5 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -1530,6 +1530,8 @@ int intel_uncore_init(struct intel_uncore *uncore) > struct drm_i915_private *i915 = uncore_to_i915(uncore); > int ret; > > + spin_lock_init(&uncore->lock); Be consistent and make an init_early(). And while you are here this should be called intel_uncore_init_mmio(). Eventually we will have the phases annotated. -Chris
On 4/1/19 1:24 PM, Chris Wilson wrote: > Quoting Daniele Ceraolo Spurio (2019-04-01 21:14:11) >> Now that all the uncore management is hidden under the uncore struct, >> move the lock initialization under the uncore_init as well for better >> encapsulation. >> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 1 - >> drivers/gpu/drm/i915/intel_uncore.c | 2 ++ >> 2 files changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index 0ca57dc5da5c..667177b7d821 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -873,7 +873,6 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) >> spin_lock_init(&dev_priv->irq_lock); >> spin_lock_init(&dev_priv->gpu_error.lock); >> mutex_init(&dev_priv->backlight_lock); >> - spin_lock_init(&dev_priv->uncore.lock); >> >> mutex_init(&dev_priv->sb_lock); >> mutex_init(&dev_priv->av_mutex); >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c >> index 106df24f20a5..250496f792e5 100644 >> --- a/drivers/gpu/drm/i915/intel_uncore.c >> +++ b/drivers/gpu/drm/i915/intel_uncore.c >> @@ -1530,6 +1530,8 @@ int intel_uncore_init(struct intel_uncore *uncore) >> struct drm_i915_private *i915 = uncore_to_i915(uncore); >> int ret; >> >> + spin_lock_init(&uncore->lock); > > Be consistent and make an init_early(). And while you are here this > should be called intel_uncore_init_mmio(). you mean to rename uncore_mmio_setup to intel_uncore_init_mmio, or are you suggesting to rename intel_uncore_init itself? I was planning to split out uncore_mmio_setup in the future to allow multiple uncore structures to be initialized separately from the mapping of the bar and to pull out unrelated init calls from uncore_init. Something like: static int i915_driver_init_mmio(...) { [... bridge dev init ...] ret = intel_uncore_mmio_setup(&dev_priv->uncore); if (ret < 0) goto err_bridge; i915_check_vgpu(i915); intel_uncore_init_mmio_access(&dev_priv->uncore); [... other stuff ...] } I was planning on sending this later once I had more code for the display uncore ready, but I can send it by itself to clean up the ordering. Thanks, Daniele > > Eventually we will have the phases annotated. > -Chris >
Quoting Daniele Ceraolo Spurio (2019-04-01 22:06:00) > > > On 4/1/19 1:24 PM, Chris Wilson wrote: > > Quoting Daniele Ceraolo Spurio (2019-04-01 21:14:11) > >> Now that all the uncore management is hidden under the uncore struct, > >> move the lock initialization under the uncore_init as well for better > >> encapsulation. > >> > >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> --- > >> drivers/gpu/drm/i915/i915_drv.c | 1 - > >> drivers/gpu/drm/i915/intel_uncore.c | 2 ++ > >> 2 files changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > >> index 0ca57dc5da5c..667177b7d821 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.c > >> +++ b/drivers/gpu/drm/i915/i915_drv.c > >> @@ -873,7 +873,6 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) > >> spin_lock_init(&dev_priv->irq_lock); > >> spin_lock_init(&dev_priv->gpu_error.lock); > >> mutex_init(&dev_priv->backlight_lock); > >> - spin_lock_init(&dev_priv->uncore.lock); > >> > >> mutex_init(&dev_priv->sb_lock); > >> mutex_init(&dev_priv->av_mutex); > >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > >> index 106df24f20a5..250496f792e5 100644 > >> --- a/drivers/gpu/drm/i915/intel_uncore.c > >> +++ b/drivers/gpu/drm/i915/intel_uncore.c > >> @@ -1530,6 +1530,8 @@ int intel_uncore_init(struct intel_uncore *uncore) > >> struct drm_i915_private *i915 = uncore_to_i915(uncore); > >> int ret; > >> > >> + spin_lock_init(&uncore->lock); > > > > Be consistent and make an init_early(). And while you are here this > > should be called intel_uncore_init_mmio(). > > you mean to rename uncore_mmio_setup to intel_uncore_init_mmio, or are > you suggesting to rename intel_uncore_init itself? I was planning to > split out uncore_mmio_setup in the future to allow multiple uncore > structures to be initialized separately from the mapping of the bar and > to pull out unrelated init calls from uncore_init. Something like: > > static int i915_driver_init_mmio(...) > { > [... bridge dev init ...] > > ret = intel_uncore_mmio_setup(&dev_priv->uncore); > if (ret < 0) > goto err_bridge; > > i915_check_vgpu(i915); > > intel_uncore_init_mmio_access(&dev_priv->uncore); > > [... other stuff ...] > } > > I was planning on sending this later once I had more code for the > display uncore ready, but I can send it by itself to clean up the ordering. Nothing so dramatic for now, just pointing out index 3ee7a72e664e..4bfcd927bce1 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -874,10 +874,11 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) intel_device_info_subplatform_init(dev_priv); + intel_uncore_init_early(&dev_priv->uncore); + spin_lock_init(&dev_priv->irq_lock); spin_lock_init(&dev_priv->gpu_error.lock); mutex_init(&dev_priv->backlight_lock); - spin_lock_init(&dev_priv->uncore.lock); mutex_init(&dev_priv->sb_lock); mutex_init(&dev_priv->av_mutex); @@ -960,7 +961,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) if (i915_get_bridge_dev(dev_priv)) return -EIO; - ret = intel_uncore_init(&dev_priv->uncore); + ret = intel_uncore_init_mmio(&dev_priv->uncore); if (ret < 0) goto err_bridge; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 106df24f20a5..0cfbc36aa15c 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1524,8 +1524,12 @@ static void uncore_mmio_cleanup(struct intel_uncore *uncore) pci_iounmap(pdev, uncore->regs); } +int intel_uncore_init_early(struct intel_uncore *uncore) +{ + spin_lock_init(&uncore->lock); +} -int intel_uncore_init(struct intel_uncore *uncore) +int intel_uncore_init_mmio(struct intel_uncore *uncore) { struct drm_i915_private *i915 = uncore_to_i915(uncore); int ret; would marry the function names to the init phases. -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 0ca57dc5da5c..667177b7d821 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -873,7 +873,6 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) spin_lock_init(&dev_priv->irq_lock); spin_lock_init(&dev_priv->gpu_error.lock); mutex_init(&dev_priv->backlight_lock); - spin_lock_init(&dev_priv->uncore.lock); mutex_init(&dev_priv->sb_lock); mutex_init(&dev_priv->av_mutex); diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 106df24f20a5..250496f792e5 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1530,6 +1530,8 @@ int intel_uncore_init(struct intel_uncore *uncore) struct drm_i915_private *i915 = uncore_to_i915(uncore); int ret; + spin_lock_init(&uncore->lock); + ret = uncore_mmio_setup(uncore); if (ret) return ret;
Now that all the uncore management is hidden under the uncore struct, move the lock initialization under the uncore_init as well for better encapsulation. Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.c | 1 - drivers/gpu/drm/i915/intel_uncore.c | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-)