Re: [PATCH] drm/i915/guc: Add onion teardown to the GuC setup

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On pe, 2017-02-17 at 15:06 -0800, Daniele Ceraolo Spurio wrote:
> 
> On 16/02/17 06:18, Oscar Mateo wrote:
> > 
> > Starting with intel_guc_loader, down to intel_guc_submission
> > and finally to intel_guc_log.
> > 
> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

<SNIP>

> > @@ -835,14 +841,11 @@ static void guc_addon_create(struct intel_guc *guc)
> >  			sizeof(struct guc_mmio_reg_state) +
> >  			GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;
> > 
> > -	vma = guc->ads_vma;
> > -	if (!vma) {
> 
> I believe the if was here to avoid re-allocating the vma if this 
> function was called after a GPU reset. I agree that the check should be 
> outside this function (and it already is), but we might want to still 
> add here something like:
> 
> 	if (WARN_ON(guc->ads_vma))
> 		return 0;

GEM_BUG_ON(guc->ads_vma); and then make sure we have sufficient
I-G-T/selftest coverage.

> > +void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> > +{
> > +	struct intel_guc *guc = &dev_priv->guc;
> > +
> 
> if I'm not missing anything, this function is called from 
> intel_guc_fini(), which can in turn be called from the onion unwinding 
> of i915_load_modeset_init() before intel_guc_init() is ever called, so 
> we need to either fix that unwinding or add some kind of guard here.

Proper unwinding to the rescue!

<SNIP>

> > +int intel_guc_log_create(struct intel_guc *guc)
> > +{
> > +	struct i915_vma *vma;
> > +	unsigned long offset;
> > +	uint32_t size, flags;
> > +	int ret;
> > +
> 
> If we expect this to not be called if the log already exists, for safety 
> we could add something like:
> 
> 	if (WARN_ON(guc->log.vma))
> 		return 0;
> 
> To make sure we don't mess this up if we keep the object alive across 
> resets.

GEM_BUG_ON(guc->log.vma);

> > @@ -653,6 +663,7 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
> >  		return;
> > 
> >  	mutex_lock(&dev_priv->drm.struct_mutex);
> > -	guc_log_cleanup(&dev_priv->guc);
> > +	gen9_disable_guc_interrupts(dev_priv);
> > +	intel_guc_log_destroy(&dev_priv->guc);
> 
> OCD nitpick: i915_guc_log_unregister is not simmetric with 
> i915_guc_log_register after this change, because intel_guc_log_destroy() 
> is destroying the vma, which was not created by guc_log_late_setup().

Yeah, destroy should be hoisted to calling function. And not so much
OCD, this is what we get double-free errors for all the time :)

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx