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

[Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>]

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.

Thanks,
Daniele

> Eventually we will have the phases annotated.
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx