Re: [PATCH v2] drm/i915: Add interface to reserve fence registers for vGPU

["Du, Changbin" <changbin.du@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3060 bytes --]

On Fri, Sep 01, 2017 at 11:59:53AM +0100, Chris Wilson wrote:
> Quoting changbin.du@intel.com (2017-09-01 08:08:26)
> > diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> > index 5fe2cd8..429ce5f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> > @@ -360,6 +360,50 @@ i915_vma_get_fence(struct i915_vma *vma)
> >  }
> >  
> >  /**
> > + * i915_reserve_fence - Reserve a fence for vGPU
> > + * @dev_priv: i915 device private
> > + *
> > + * This function walks the fence regs looking for a free one and remove
> > + * it from the fence_list. It is used to reserve fence for vGPU to use.
> > + */
> > +struct drm_i915_fence_reg *
> > +i915_reserve_fence(struct drm_i915_private *dev_priv)
> > +{
> > +       struct drm_i915_fence_reg *fence;
> > +       int ret;
> 
> lockdep_assert_held(&dev_priv->drm.struct_mutex);
> 
> A reminder for when we move fences to a seperate mutex.
>
ok.

> > +
> > +       /* Host at least need one fence available for display engine. */
> > +       if (unlikely(dev_priv->mm.fence_list.prev ==
> > +                    dev_priv->mm.fence_list.next))
> > +               return ERR_PTR(-ENOSPC);
> 
> I would prefer one free fence.
> 
> int count;
> 
> /* Keep at least one fence available for the display engine */
> count = 0;
> list_for_each_entry(fence, &dev_priv->mm.fence_list, link)
> 	count += !fence->pin_count;
> if (count <= 1)
> 	return ERR_PTR(-ENOSPC);
>
Got your idea, will update.

> > +
> > +       fence = fence_find(dev_priv);
> > +       if (IS_ERR(fence))
> > +               return fence;
> > +
> > +       if (fence->vma) {
> > +               /* Force-remove fence from VMA */
> > +               ret = fence_update(fence, NULL);
> > +               if (ret)
> > +                       return ERR_PTR(ret);
> > +       }
> > +
> > +       list_del(&fence->link);
> > +       return fence;
> > +}
> > +
> > +/**
> > + * i915_unreserve_fence - Reclaim a reserved fence
> > + * @fence: the fence reg
> > + *
> > + * This function add a reserved fence register from vGPU to the fence_list.
> > + */
> > +void i915_unreserve_fence(struct drm_i915_fence_reg *fence)
> > +{
> 
> lockdep_assert_held(&dev_priv->drm.struct_mutex);
> 
> If you agree to "one free fence",
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 

> Otherwise, we need to look carefully at what happens when the search
> fails. And that may suggest we need to keep one free per crtc, or that
> we can survive without (but that usually means giving up on fbc and
> whatnot, thinking of fbc is why I want to keep one spare).
> 
> (Time to write some unittests for find-a-fence.)
> -Chris

I agree to "one free fence", which is simpler. Currently GVTg will at most use
4x7=28 fence registers (4 per VM, and up to 7 VMs), so at least 4 will remained
for host. I agree to your concern, it is significative.

-- 
Thanks,
Changbin Du

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx