Re: [PATCH] drm/i915: initialize uncore->lock in uncore_init

[Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxx>
> >> Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> >> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >> ---
> >>   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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx