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@xxxxxxxxx>
<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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
