[BrownBag] drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup

[BrownBag] drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup

[Chris Wilson]

Having deferred the vma destruction to a worker where we can acquire the
struct_mutex, we have to avoid chasing back into the now destroyed
ppgtt. The pd_vma is special in having a custom unbind function to scan
for unused pages despite the VMA itself being notionally part of the
GGTT. As such, we need to disable that callback to avoid a
use-after-free.

This unfortunately blew up so early during boot that CI declared the
machine unreachable as opposed to being the major failure it was. Oops.

Fixes: d3622099c76f ("drm/i915/gtt: Always acquire struct_mutex for gen6_ppgtt_cleanup")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 8d8a4b0ad4d9..266baa11df64 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1847,6 +1847,33 @@ static void gen6_ppgtt_cleanup_work(struct work_struct *wrk)
 	kfree(work);
 }
 
+static int nop_set_pages(struct i915_vma *vma)
+{
+	return -ENODEV;
+}
+
+static void nop_clear_pages(struct i915_vma *vma)
+{
+}
+
+static int nop_bind(struct i915_vma *vma,
+		    enum i915_cache_level cache_level,
+		    u32 unused)
+{
+	return -ENODEV;
+}
+
+static void nop_unbind(struct i915_vma *vma)
+{
+}
+
+static const struct i915_vma_ops nop_vma_ops = {
+	.set_pages = nop_set_pages,
+	.clear_pages = nop_clear_pages,
+	.bind_vma = nop_bind,
+	.unbind_vma = nop_unbind,
+};
+
 static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
 {
 	struct gen6_hw_ppgtt *ppgtt = to_gen6_ppgtt(i915_vm_to_ppgtt(vm));
@@ -1855,6 +1882,7 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
 	/* FIXME remove the struct_mutex to bring the locking under control */
 	INIT_WORK(&work->base, gen6_ppgtt_cleanup_work);
 	work->vma = ppgtt->vma;
+	work->vma->ops = &nop_vma_ops;
 	schedule_work(&work->base);
 
 	gen6_ppgtt_free_pd(ppgtt);
-- 
2.20.1

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

Re: [BrownBag] drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup

[Tvrtko Ursulin]

On 24/05/2019 07:45, Chris Wilson wrote:
> Having deferred the vma destruction to a worker where we can acquire the
> struct_mutex, we have to avoid chasing back into the now destroyed
> ppgtt. The pd_vma is special in having a custom unbind function to scan
> for unused pages despite the VMA itself being notionally part of the
> GGTT. As such, we need to disable that callback to avoid a
> use-after-free.
> 
> This unfortunately blew up so early during boot that CI declared the
> machine unreachable as opposed to being the major failure it was. Oops.
> 
> Fixes: d3622099c76f ("drm/i915/gtt: Always acquire struct_mutex for gen6_ppgtt_cleanup")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 8d8a4b0ad4d9..266baa11df64 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1847,6 +1847,33 @@ static void gen6_ppgtt_cleanup_work(struct work_struct *wrk)
>   	kfree(work);
>   }
>   
> +static int nop_set_pages(struct i915_vma *vma)
> +{
> +	return -ENODEV;
> +}
> +
> +static void nop_clear_pages(struct i915_vma *vma)
> +{
> +}
> +
> +static int nop_bind(struct i915_vma *vma,
> +		    enum i915_cache_level cache_level,
> +		    u32 unused)
> +{
> +	return -ENODEV;
> +}
> +
> +static void nop_unbind(struct i915_vma *vma)
> +{
> +}
> +
> +static const struct i915_vma_ops nop_vma_ops = {
> +	.set_pages = nop_set_pages,
> +	.clear_pages = nop_clear_pages,
> +	.bind_vma = nop_bind,
> +	.unbind_vma = nop_unbind,
> +};
> +
>   static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>   {
>   	struct gen6_hw_ppgtt *ppgtt = to_gen6_ppgtt(i915_vm_to_ppgtt(vm));
> @@ -1855,6 +1882,7 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>   	/* FIXME remove the struct_mutex to bring the locking under control */
>   	INIT_WORK(&work->base, gen6_ppgtt_cleanup_work);
>   	work->vma = ppgtt->vma;
> +	work->vma->ops = &nop_vma_ops;

Could we use some asserts before overriding the vma ops? Like 
GEM_BUG_ON(vma->pages)? And something for still bound?

>   	schedule_work(&work->base);
>   
>   	gen6_ppgtt_free_pd(ppgtt);
> 

Regards,

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

Re: [BrownBag] drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-05-24 09:13:14)
> 
> On 24/05/2019 07:45, Chris Wilson wrote:
> > Having deferred the vma destruction to a worker where we can acquire the
> > struct_mutex, we have to avoid chasing back into the now destroyed
> > ppgtt. The pd_vma is special in having a custom unbind function to scan
> > for unused pages despite the VMA itself being notionally part of the
> > GGTT. As such, we need to disable that callback to avoid a
> > use-after-free.
> > 
> > This unfortunately blew up so early during boot that CI declared the
> > machine unreachable as opposed to being the major failure it was. Oops.
> > 
> > Fixes: d3622099c76f ("drm/i915/gtt: Always acquire struct_mutex for gen6_ppgtt_cleanup")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_gem_gtt.c | 28 ++++++++++++++++++++++++++++
> >   1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 8d8a4b0ad4d9..266baa11df64 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -1847,6 +1847,33 @@ static void gen6_ppgtt_cleanup_work(struct work_struct *wrk)
> >       kfree(work);
> >   }
> >   
> > +static int nop_set_pages(struct i915_vma *vma)
> > +{
> > +     return -ENODEV;
> > +}
> > +
> > +static void nop_clear_pages(struct i915_vma *vma)
> > +{
> > +}
> > +
> > +static int nop_bind(struct i915_vma *vma,
> > +                 enum i915_cache_level cache_level,
> > +                 u32 unused)
> > +{
> > +     return -ENODEV;
> > +}
> > +
> > +static void nop_unbind(struct i915_vma *vma)
> > +{
> > +}
> > +
> > +static const struct i915_vma_ops nop_vma_ops = {
> > +     .set_pages = nop_set_pages,
> > +     .clear_pages = nop_clear_pages,
> > +     .bind_vma = nop_bind,
> > +     .unbind_vma = nop_unbind,
> > +};
> > +
> >   static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> >   {
> >       struct gen6_hw_ppgtt *ppgtt = to_gen6_ppgtt(i915_vm_to_ppgtt(vm));
> > @@ -1855,6 +1882,7 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> >       /* FIXME remove the struct_mutex to bring the locking under control */
> >       INIT_WORK(&work->base, gen6_ppgtt_cleanup_work);
> >       work->vma = ppgtt->vma;
> > +     work->vma->ops = &nop_vma_ops;
> 
> Could we use some asserts before overriding the vma ops? Like 
> GEM_BUG_ON(vma->pages)? And something for still bound?

It technically still is bound as it is in the GGTT but currently
unpinned -- that will be checked on destroy, it's just we also get an
unbind callback. vma->pages doesn't exist for this (set to ERR_PTR).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [BrownBag] drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup

[Tvrtko Ursulin]

On 24/05/2019 09:17, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-05-24 09:13:14)
>>
>> On 24/05/2019 07:45, Chris Wilson wrote:
>>> Having deferred the vma destruction to a worker where we can acquire the
>>> struct_mutex, we have to avoid chasing back into the now destroyed
>>> ppgtt. The pd_vma is special in having a custom unbind function to scan
>>> for unused pages despite the VMA itself being notionally part of the
>>> GGTT. As such, we need to disable that callback to avoid a
>>> use-after-free.
>>>
>>> This unfortunately blew up so early during boot that CI declared the
>>> machine unreachable as opposed to being the major failure it was. Oops.
>>>
>>> Fixes: d3622099c76f ("drm/i915/gtt: Always acquire struct_mutex for gen6_ppgtt_cleanup")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_gem_gtt.c | 28 ++++++++++++++++++++++++++++
>>>    1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> index 8d8a4b0ad4d9..266baa11df64 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> @@ -1847,6 +1847,33 @@ static void gen6_ppgtt_cleanup_work(struct work_struct *wrk)
>>>        kfree(work);
>>>    }
>>>    
>>> +static int nop_set_pages(struct i915_vma *vma)
>>> +{
>>> +     return -ENODEV;
>>> +}
>>> +
>>> +static void nop_clear_pages(struct i915_vma *vma)
>>> +{
>>> +}
>>> +
>>> +static int nop_bind(struct i915_vma *vma,
>>> +                 enum i915_cache_level cache_level,
>>> +                 u32 unused)
>>> +{
>>> +     return -ENODEV;
>>> +}
>>> +
>>> +static void nop_unbind(struct i915_vma *vma)
>>> +{
>>> +}
>>> +
>>> +static const struct i915_vma_ops nop_vma_ops = {
>>> +     .set_pages = nop_set_pages,
>>> +     .clear_pages = nop_clear_pages,
>>> +     .bind_vma = nop_bind,
>>> +     .unbind_vma = nop_unbind,
>>> +};
>>> +
>>>    static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>>>    {
>>>        struct gen6_hw_ppgtt *ppgtt = to_gen6_ppgtt(i915_vm_to_ppgtt(vm));
>>> @@ -1855,6 +1882,7 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>>>        /* FIXME remove the struct_mutex to bring the locking under control */
>>>        INIT_WORK(&work->base, gen6_ppgtt_cleanup_work);
>>>        work->vma = ppgtt->vma;
>>> +     work->vma->ops = &nop_vma_ops;
>>
>> Could we use some asserts before overriding the vma ops? Like
>> GEM_BUG_ON(vma->pages)? And something for still bound?
> 
> It technically still is bound as it is in the GGTT but currently
> unpinned -- that will be checked on destroy, it's just we also get an
> unbind callback. vma->pages doesn't exist for this (set to ERR_PTR).

If we are getting the unbind callback and we nop-ed it, who will 
actually do it's job?

Regards,

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

Re: [BrownBag] drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-05-24 09:23:40)
> 
> On 24/05/2019 09:17, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-05-24 09:13:14)
> >>
> >> On 24/05/2019 07:45, Chris Wilson wrote:
> >>> Having deferred the vma destruction to a worker where we can acquire the
> >>> struct_mutex, we have to avoid chasing back into the now destroyed
> >>> ppgtt. The pd_vma is special in having a custom unbind function to scan
> >>> for unused pages despite the VMA itself being notionally part of the
> >>> GGTT. As such, we need to disable that callback to avoid a
> >>> use-after-free.
> >>>
> >>> This unfortunately blew up so early during boot that CI declared the
> >>> machine unreachable as opposed to being the major failure it was. Oops.
> >>>
> >>> Fixes: d3622099c76f ("drm/i915/gtt: Always acquire struct_mutex for gen6_ppgtt_cleanup")
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_gem_gtt.c | 28 ++++++++++++++++++++++++++++
> >>>    1 file changed, 28 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>> index 8d8a4b0ad4d9..266baa11df64 100644
> >>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>> @@ -1847,6 +1847,33 @@ static void gen6_ppgtt_cleanup_work(struct work_struct *wrk)
> >>>        kfree(work);
> >>>    }
> >>>    
> >>> +static int nop_set_pages(struct i915_vma *vma)
> >>> +{
> >>> +     return -ENODEV;
> >>> +}
> >>> +
> >>> +static void nop_clear_pages(struct i915_vma *vma)
> >>> +{
> >>> +}
> >>> +
> >>> +static int nop_bind(struct i915_vma *vma,
> >>> +                 enum i915_cache_level cache_level,
> >>> +                 u32 unused)
> >>> +{
> >>> +     return -ENODEV;
> >>> +}
> >>> +
> >>> +static void nop_unbind(struct i915_vma *vma)
> >>> +{
> >>> +}
> >>> +
> >>> +static const struct i915_vma_ops nop_vma_ops = {
> >>> +     .set_pages = nop_set_pages,
> >>> +     .clear_pages = nop_clear_pages,
> >>> +     .bind_vma = nop_bind,
> >>> +     .unbind_vma = nop_unbind,
> >>> +};
> >>> +
> >>>    static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> >>>    {
> >>>        struct gen6_hw_ppgtt *ppgtt = to_gen6_ppgtt(i915_vm_to_ppgtt(vm));
> >>> @@ -1855,6 +1882,7 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> >>>        /* FIXME remove the struct_mutex to bring the locking under control */
> >>>        INIT_WORK(&work->base, gen6_ppgtt_cleanup_work);
> >>>        work->vma = ppgtt->vma;
> >>> +     work->vma->ops = &nop_vma_ops;
> >>
> >> Could we use some asserts before overriding the vma ops? Like
> >> GEM_BUG_ON(vma->pages)? And something for still bound?
> > 
> > It technically still is bound as it is in the GGTT but currently
> > unpinned -- that will be checked on destroy, it's just we also get an
> > unbind callback. vma->pages doesn't exist for this (set to ERR_PTR).
> 
> If we are getting the unbind callback and we nop-ed it, who will 
> actually do it's job?

The callback is just a hook for us to prune within the ppgtt.
It still is removed from GGTT by i915_vma_unbind().
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [BrownBag] drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup

[Tvrtko Ursulin]

On 24/05/2019 09:29, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-05-24 09:23:40)
>>
>> On 24/05/2019 09:17, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-05-24 09:13:14)
>>>>
>>>> On 24/05/2019 07:45, Chris Wilson wrote:
>>>>> Having deferred the vma destruction to a worker where we can acquire the
>>>>> struct_mutex, we have to avoid chasing back into the now destroyed
>>>>> ppgtt. The pd_vma is special in having a custom unbind function to scan
>>>>> for unused pages despite the VMA itself being notionally part of the
>>>>> GGTT. As such, we need to disable that callback to avoid a
>>>>> use-after-free.
>>>>>
>>>>> This unfortunately blew up so early during boot that CI declared the
>>>>> machine unreachable as opposed to being the major failure it was. Oops.
>>>>>
>>>>> Fixes: d3622099c76f ("drm/i915/gtt: Always acquire struct_mutex for gen6_ppgtt_cleanup")
>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/i915/i915_gem_gtt.c | 28 ++++++++++++++++++++++++++++
>>>>>     1 file changed, 28 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>> index 8d8a4b0ad4d9..266baa11df64 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>> @@ -1847,6 +1847,33 @@ static void gen6_ppgtt_cleanup_work(struct work_struct *wrk)
>>>>>         kfree(work);
>>>>>     }
>>>>>     
>>>>> +static int nop_set_pages(struct i915_vma *vma)
>>>>> +{
>>>>> +     return -ENODEV;
>>>>> +}
>>>>> +
>>>>> +static void nop_clear_pages(struct i915_vma *vma)
>>>>> +{
>>>>> +}
>>>>> +
>>>>> +static int nop_bind(struct i915_vma *vma,
>>>>> +                 enum i915_cache_level cache_level,
>>>>> +                 u32 unused)
>>>>> +{
>>>>> +     return -ENODEV;
>>>>> +}
>>>>> +
>>>>> +static void nop_unbind(struct i915_vma *vma)
>>>>> +{
>>>>> +}
>>>>> +
>>>>> +static const struct i915_vma_ops nop_vma_ops = {
>>>>> +     .set_pages = nop_set_pages,
>>>>> +     .clear_pages = nop_clear_pages,
>>>>> +     .bind_vma = nop_bind,
>>>>> +     .unbind_vma = nop_unbind,
>>>>> +};
>>>>> +
>>>>>     static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>>>>>     {
>>>>>         struct gen6_hw_ppgtt *ppgtt = to_gen6_ppgtt(i915_vm_to_ppgtt(vm));
>>>>> @@ -1855,6 +1882,7 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>>>>>         /* FIXME remove the struct_mutex to bring the locking under control */
>>>>>         INIT_WORK(&work->base, gen6_ppgtt_cleanup_work);
>>>>>         work->vma = ppgtt->vma;
>>>>> +     work->vma->ops = &nop_vma_ops;
>>>>
>>>> Could we use some asserts before overriding the vma ops? Like
>>>> GEM_BUG_ON(vma->pages)? And something for still bound?
>>>
>>> It technically still is bound as it is in the GGTT but currently
>>> unpinned -- that will be checked on destroy, it's just we also get an
>>> unbind callback. vma->pages doesn't exist for this (set to ERR_PTR).
>>
>> If we are getting the unbind callback and we nop-ed it, who will
>> actually do it's job?
> 
> The callback is just a hook for us to prune within the ppgtt.
> It still is removed from GGTT by i915_vma_unbind().

So it needs GEM_BUG_ON(ppgtt->scan_for_unused_pt) before overriding the 
unbind?

Regards,

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

Re: [BrownBag] drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-05-24 09:31:45)
> 
> On 24/05/2019 09:29, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-05-24 09:23:40)
> >>
> >> On 24/05/2019 09:17, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2019-05-24 09:13:14)
> >>>>
> >>>> On 24/05/2019 07:45, Chris Wilson wrote:
> >>>>> Having deferred the vma destruction to a worker where we can acquire the
> >>>>> struct_mutex, we have to avoid chasing back into the now destroyed
> >>>>> ppgtt. The pd_vma is special in having a custom unbind function to scan
> >>>>> for unused pages despite the VMA itself being notionally part of the
> >>>>> GGTT. As such, we need to disable that callback to avoid a
> >>>>> use-after-free.
> >>>>>
> >>>>> This unfortunately blew up so early during boot that CI declared the
> >>>>> machine unreachable as opposed to being the major failure it was. Oops.
> >>>>>
> >>>>> Fixes: d3622099c76f ("drm/i915/gtt: Always acquire struct_mutex for gen6_ppgtt_cleanup")
> >>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> >>>>> ---
> >>>>>     drivers/gpu/drm/i915/i915_gem_gtt.c | 28 ++++++++++++++++++++++++++++
> >>>>>     1 file changed, 28 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>>>> index 8d8a4b0ad4d9..266baa11df64 100644
> >>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>>>> @@ -1847,6 +1847,33 @@ static void gen6_ppgtt_cleanup_work(struct work_struct *wrk)
> >>>>>         kfree(work);
> >>>>>     }
> >>>>>     
> >>>>> +static int nop_set_pages(struct i915_vma *vma)
> >>>>> +{
> >>>>> +     return -ENODEV;
> >>>>> +}
> >>>>> +
> >>>>> +static void nop_clear_pages(struct i915_vma *vma)
> >>>>> +{
> >>>>> +}
> >>>>> +
> >>>>> +static int nop_bind(struct i915_vma *vma,
> >>>>> +                 enum i915_cache_level cache_level,
> >>>>> +                 u32 unused)
> >>>>> +{
> >>>>> +     return -ENODEV;
> >>>>> +}
> >>>>> +
> >>>>> +static void nop_unbind(struct i915_vma *vma)
> >>>>> +{
> >>>>> +}
> >>>>> +
> >>>>> +static const struct i915_vma_ops nop_vma_ops = {
> >>>>> +     .set_pages = nop_set_pages,
> >>>>> +     .clear_pages = nop_clear_pages,
> >>>>> +     .bind_vma = nop_bind,
> >>>>> +     .unbind_vma = nop_unbind,
> >>>>> +};
> >>>>> +
> >>>>>     static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> >>>>>     {
> >>>>>         struct gen6_hw_ppgtt *ppgtt = to_gen6_ppgtt(i915_vm_to_ppgtt(vm));
> >>>>> @@ -1855,6 +1882,7 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> >>>>>         /* FIXME remove the struct_mutex to bring the locking under control */
> >>>>>         INIT_WORK(&work->base, gen6_ppgtt_cleanup_work);
> >>>>>         work->vma = ppgtt->vma;
> >>>>> +     work->vma->ops = &nop_vma_ops;
> >>>>
> >>>> Could we use some asserts before overriding the vma ops? Like
> >>>> GEM_BUG_ON(vma->pages)? And something for still bound?
> >>>
> >>> It technically still is bound as it is in the GGTT but currently
> >>> unpinned -- that will be checked on destroy, it's just we also get an
> >>> unbind callback. vma->pages doesn't exist for this (set to ERR_PTR).
> >>
> >> If we are getting the unbind callback and we nop-ed it, who will
> >> actually do it's job?
> > 
> > The callback is just a hook for us to prune within the ppgtt.
> > It still is removed from GGTT by i915_vma_unbind().
> 
> So it needs GEM_BUG_ON(ppgtt->scan_for_unused_pt) before overriding the 
> unbind?

No. They get freed by the cleanup itself. The scan is just an
opportunistic prune if either the context/mm is evicted but still alive.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [BrownBag] drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup

[Tvrtko Ursulin]

On 24/05/2019 09:36, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-05-24 09:31:45)
>>
>> On 24/05/2019 09:29, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-05-24 09:23:40)
>>>>
>>>> On 24/05/2019 09:17, Chris Wilson wrote:
>>>>> Quoting Tvrtko Ursulin (2019-05-24 09:13:14)
>>>>>>
>>>>>> On 24/05/2019 07:45, Chris Wilson wrote:
>>>>>>> Having deferred the vma destruction to a worker where we can acquire the
>>>>>>> struct_mutex, we have to avoid chasing back into the now destroyed
>>>>>>> ppgtt. The pd_vma is special in having a custom unbind function to scan
>>>>>>> for unused pages despite the VMA itself being notionally part of the
>>>>>>> GGTT. As such, we need to disable that callback to avoid a
>>>>>>> use-after-free.
>>>>>>>
>>>>>>> This unfortunately blew up so early during boot that CI declared the
>>>>>>> machine unreachable as opposed to being the major failure it was. Oops.
>>>>>>>
>>>>>>> Fixes: d3622099c76f ("drm/i915/gtt: Always acquire struct_mutex for gen6_ppgtt_cleanup")
>>>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
>>>>>>> ---
>>>>>>>      drivers/gpu/drm/i915/i915_gem_gtt.c | 28 ++++++++++++++++++++++++++++
>>>>>>>      1 file changed, 28 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>>>> index 8d8a4b0ad4d9..266baa11df64 100644
>>>>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>>>> @@ -1847,6 +1847,33 @@ static void gen6_ppgtt_cleanup_work(struct work_struct *wrk)
>>>>>>>          kfree(work);
>>>>>>>      }
>>>>>>>      
>>>>>>> +static int nop_set_pages(struct i915_vma *vma)
>>>>>>> +{
>>>>>>> +     return -ENODEV;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void nop_clear_pages(struct i915_vma *vma)
>>>>>>> +{
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int nop_bind(struct i915_vma *vma,
>>>>>>> +                 enum i915_cache_level cache_level,
>>>>>>> +                 u32 unused)
>>>>>>> +{
>>>>>>> +     return -ENODEV;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void nop_unbind(struct i915_vma *vma)
>>>>>>> +{
>>>>>>> +}
>>>>>>> +
>>>>>>> +static const struct i915_vma_ops nop_vma_ops = {
>>>>>>> +     .set_pages = nop_set_pages,
>>>>>>> +     .clear_pages = nop_clear_pages,
>>>>>>> +     .bind_vma = nop_bind,
>>>>>>> +     .unbind_vma = nop_unbind,
>>>>>>> +};
>>>>>>> +
>>>>>>>      static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>>>>>>>      {
>>>>>>>          struct gen6_hw_ppgtt *ppgtt = to_gen6_ppgtt(i915_vm_to_ppgtt(vm));
>>>>>>> @@ -1855,6 +1882,7 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>>>>>>>          /* FIXME remove the struct_mutex to bring the locking under control */
>>>>>>>          INIT_WORK(&work->base, gen6_ppgtt_cleanup_work);
>>>>>>>          work->vma = ppgtt->vma;
>>>>>>> +     work->vma->ops = &nop_vma_ops;
>>>>>>
>>>>>> Could we use some asserts before overriding the vma ops? Like
>>>>>> GEM_BUG_ON(vma->pages)? And something for still bound?
>>>>>
>>>>> It technically still is bound as it is in the GGTT but currently
>>>>> unpinned -- that will be checked on destroy, it's just we also get an
>>>>> unbind callback. vma->pages doesn't exist for this (set to ERR_PTR).
>>>>
>>>> If we are getting the unbind callback and we nop-ed it, who will
>>>> actually do it's job?
>>>
>>> The callback is just a hook for us to prune within the ppgtt.
>>> It still is removed from GGTT by i915_vma_unbind().
>>
>> So it needs GEM_BUG_ON(ppgtt->scan_for_unused_pt) before overriding the
>> unbind?
> 
> No. They get freed by the cleanup itself. The scan is just an
> opportunistic prune if either the context/mm is evicted but still alive.

Then the same assert in gen6_ppgtt_cleanup_work? :)

Regards,

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

Re: [BrownBag] drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-05-24 09:51:46)
> 
> On 24/05/2019 09:36, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-05-24 09:31:45)
> >>
> >> On 24/05/2019 09:29, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2019-05-24 09:23:40)
> >>>>
> >>>> On 24/05/2019 09:17, Chris Wilson wrote:
> >>>>> Quoting Tvrtko Ursulin (2019-05-24 09:13:14)
> >>>>>>
> >>>>>> On 24/05/2019 07:45, Chris Wilson wrote:
> >>>>>>> Having deferred the vma destruction to a worker where we can acquire the
> >>>>>>> struct_mutex, we have to avoid chasing back into the now destroyed
> >>>>>>> ppgtt. The pd_vma is special in having a custom unbind function to scan
> >>>>>>> for unused pages despite the VMA itself being notionally part of the
> >>>>>>> GGTT. As such, we need to disable that callback to avoid a
> >>>>>>> use-after-free.
> >>>>>>>
> >>>>>>> This unfortunately blew up so early during boot that CI declared the
> >>>>>>> machine unreachable as opposed to being the major failure it was. Oops.
> >>>>>>>
> >>>>>>> Fixes: d3622099c76f ("drm/i915/gtt: Always acquire struct_mutex for gen6_ppgtt_cleanup")
> >>>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>>>> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> >>>>>>> ---
> >>>>>>>      drivers/gpu/drm/i915/i915_gem_gtt.c | 28 ++++++++++++++++++++++++++++
> >>>>>>>      1 file changed, 28 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>>>>>> index 8d8a4b0ad4d9..266baa11df64 100644
> >>>>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>>>>>> @@ -1847,6 +1847,33 @@ static void gen6_ppgtt_cleanup_work(struct work_struct *wrk)
> >>>>>>>          kfree(work);
> >>>>>>>      }
> >>>>>>>      
> >>>>>>> +static int nop_set_pages(struct i915_vma *vma)
> >>>>>>> +{
> >>>>>>> +     return -ENODEV;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static void nop_clear_pages(struct i915_vma *vma)
> >>>>>>> +{
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static int nop_bind(struct i915_vma *vma,
> >>>>>>> +                 enum i915_cache_level cache_level,
> >>>>>>> +                 u32 unused)
> >>>>>>> +{
> >>>>>>> +     return -ENODEV;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static void nop_unbind(struct i915_vma *vma)
> >>>>>>> +{
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static const struct i915_vma_ops nop_vma_ops = {
> >>>>>>> +     .set_pages = nop_set_pages,
> >>>>>>> +     .clear_pages = nop_clear_pages,
> >>>>>>> +     .bind_vma = nop_bind,
> >>>>>>> +     .unbind_vma = nop_unbind,
> >>>>>>> +};
> >>>>>>> +
> >>>>>>>      static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> >>>>>>>      {
> >>>>>>>          struct gen6_hw_ppgtt *ppgtt = to_gen6_ppgtt(i915_vm_to_ppgtt(vm));
> >>>>>>> @@ -1855,6 +1882,7 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> >>>>>>>          /* FIXME remove the struct_mutex to bring the locking under control */
> >>>>>>>          INIT_WORK(&work->base, gen6_ppgtt_cleanup_work);
> >>>>>>>          work->vma = ppgtt->vma;
> >>>>>>> +     work->vma->ops = &nop_vma_ops;
> >>>>>>
> >>>>>> Could we use some asserts before overriding the vma ops? Like
> >>>>>> GEM_BUG_ON(vma->pages)? And something for still bound?
> >>>>>
> >>>>> It technically still is bound as it is in the GGTT but currently
> >>>>> unpinned -- that will be checked on destroy, it's just we also get an
> >>>>> unbind callback. vma->pages doesn't exist for this (set to ERR_PTR).
> >>>>
> >>>> If we are getting the unbind callback and we nop-ed it, who will
> >>>> actually do it's job?
> >>>
> >>> The callback is just a hook for us to prune within the ppgtt.
> >>> It still is removed from GGTT by i915_vma_unbind().
> >>
> >> So it needs GEM_BUG_ON(ppgtt->scan_for_unused_pt) before overriding the
> >> unbind?
> > 
> > No. They get freed by the cleanup itself. The scan is just an
> > opportunistic prune if either the context/mm is evicted but still alive.
> 
> Then the same assert in gen6_ppgtt_cleanup_work? :)

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

Re: [BrownBag] drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup

[Tvrtko Ursulin]

On 24/05/2019 09:51, Tvrtko Ursulin wrote:
> 
> On 24/05/2019 09:36, Chris Wilson wrote:
>> Quoting Tvrtko Ursulin (2019-05-24 09:31:45)
>>>
>>> On 24/05/2019 09:29, Chris Wilson wrote:
>>>> Quoting Tvrtko Ursulin (2019-05-24 09:23:40)
>>>>>
>>>>> On 24/05/2019 09:17, Chris Wilson wrote:
>>>>>> Quoting Tvrtko Ursulin (2019-05-24 09:13:14)
>>>>>>>
>>>>>>> On 24/05/2019 07:45, Chris Wilson wrote:
>>>>>>>> Having deferred the vma destruction to a worker where we can 
>>>>>>>> acquire the
>>>>>>>> struct_mutex, we have to avoid chasing back into the now destroyed
>>>>>>>> ppgtt. The pd_vma is special in having a custom unbind function 
>>>>>>>> to scan
>>>>>>>> for unused pages despite the VMA itself being notionally part of 
>>>>>>>> the
>>>>>>>> GGTT. As such, we need to disable that callback to avoid a
>>>>>>>> use-after-free.
>>>>>>>>
>>>>>>>> This unfortunately blew up so early during boot that CI declared 
>>>>>>>> the
>>>>>>>> machine unreachable as opposed to being the major failure it 
>>>>>>>> was. Oops.
>>>>>>>>
>>>>>>>> Fixes: d3622099c76f ("drm/i915/gtt: Always acquire struct_mutex 
>>>>>>>> for gen6_ppgtt_cleanup")
>>>>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>>> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
>>>>>>>> ---
>>>>>>>>      drivers/gpu/drm/i915/i915_gem_gtt.c | 28 
>>>>>>>> ++++++++++++++++++++++++++++
>>>>>>>>      1 file changed, 28 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
>>>>>>>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>>>>> index 8d8a4b0ad4d9..266baa11df64 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>>>>> @@ -1847,6 +1847,33 @@ static void 
>>>>>>>> gen6_ppgtt_cleanup_work(struct work_struct *wrk)
>>>>>>>>          kfree(work);
>>>>>>>>      }
>>>>>>>> +static int nop_set_pages(struct i915_vma *vma)
>>>>>>>> +{
>>>>>>>> +     return -ENODEV;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void nop_clear_pages(struct i915_vma *vma)
>>>>>>>> +{
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int nop_bind(struct i915_vma *vma,
>>>>>>>> +                 enum i915_cache_level cache_level,
>>>>>>>> +                 u32 unused)
>>>>>>>> +{
>>>>>>>> +     return -ENODEV;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void nop_unbind(struct i915_vma *vma)
>>>>>>>> +{
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static const struct i915_vma_ops nop_vma_ops = {
>>>>>>>> +     .set_pages = nop_set_pages,
>>>>>>>> +     .clear_pages = nop_clear_pages,
>>>>>>>> +     .bind_vma = nop_bind,
>>>>>>>> +     .unbind_vma = nop_unbind,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>      static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>>>>>>>>      {
>>>>>>>>          struct gen6_hw_ppgtt *ppgtt = 
>>>>>>>> to_gen6_ppgtt(i915_vm_to_ppgtt(vm));
>>>>>>>> @@ -1855,6 +1882,7 @@ static void gen6_ppgtt_cleanup(struct 
>>>>>>>> i915_address_space *vm)
>>>>>>>>          /* FIXME remove the struct_mutex to bring the locking 
>>>>>>>> under control */
>>>>>>>>          INIT_WORK(&work->base, gen6_ppgtt_cleanup_work);
>>>>>>>>          work->vma = ppgtt->vma;
>>>>>>>> +     work->vma->ops = &nop_vma_ops;
>>>>>>>
>>>>>>> Could we use some asserts before overriding the vma ops? Like
>>>>>>> GEM_BUG_ON(vma->pages)? And something for still bound?
>>>>>>
>>>>>> It technically still is bound as it is in the GGTT but currently
>>>>>> unpinned -- that will be checked on destroy, it's just we also get an
>>>>>> unbind callback. vma->pages doesn't exist for this (set to ERR_PTR).
>>>>>
>>>>> If we are getting the unbind callback and we nop-ed it, who will
>>>>> actually do it's job?
>>>>
>>>> The callback is just a hook for us to prune within the ppgtt.
>>>> It still is removed from GGTT by i915_vma_unbind().
>>>
>>> So it needs GEM_BUG_ON(ppgtt->scan_for_unused_pt) before overriding the
>>> unbind?
>>
>> No. They get freed by the cleanup itself. The scan is just an
>> opportunistic prune if either the context/mm is evicted but still alive.
> 
> Then the same assert in gen6_ppgtt_cleanup_work? :)

Okay ppgtt is gone so can't do it.. annoying.. Cleanup seems to support 
your claims but I think we need a BFC (big fat comment) above the vma 
ops override to explains this. With that:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [BrownBag] drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-05-24 09:57:42)
> 
> On 24/05/2019 09:51, Tvrtko Ursulin wrote:
> > 
> > On 24/05/2019 09:36, Chris Wilson wrote:
> >> Quoting Tvrtko Ursulin (2019-05-24 09:31:45)
> >>>
> >>> On 24/05/2019 09:29, Chris Wilson wrote:
> >>>> Quoting Tvrtko Ursulin (2019-05-24 09:23:40)
> >>>>>
> >>>>> On 24/05/2019 09:17, Chris Wilson wrote:
> >>>>>> Quoting Tvrtko Ursulin (2019-05-24 09:13:14)
> >>>>>>>
> >>>>>>> On 24/05/2019 07:45, Chris Wilson wrote:
> >>>>>>>> Having deferred the vma destruction to a worker where we can 
> >>>>>>>> acquire the
> >>>>>>>> struct_mutex, we have to avoid chasing back into the now destroyed
> >>>>>>>> ppgtt. The pd_vma is special in having a custom unbind function 
> >>>>>>>> to scan
> >>>>>>>> for unused pages despite the VMA itself being notionally part of 
> >>>>>>>> the
> >>>>>>>> GGTT. As such, we need to disable that callback to avoid a
> >>>>>>>> use-after-free.
> >>>>>>>>
> >>>>>>>> This unfortunately blew up so early during boot that CI declared 
> >>>>>>>> the
> >>>>>>>> machine unreachable as opposed to being the major failure it 
> >>>>>>>> was. Oops.
> >>>>>>>>
> >>>>>>>> Fixes: d3622099c76f ("drm/i915/gtt: Always acquire struct_mutex 
> >>>>>>>> for gen6_ppgtt_cleanup")
> >>>>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>>>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>>>>> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> >>>>>>>> ---
> >>>>>>>>      drivers/gpu/drm/i915/i915_gem_gtt.c | 28 
> >>>>>>>> ++++++++++++++++++++++++++++
> >>>>>>>>      1 file changed, 28 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> >>>>>>>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>>>>>>> index 8d8a4b0ad4d9..266baa11df64 100644
> >>>>>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>>>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>>>>>>> @@ -1847,6 +1847,33 @@ static void 
> >>>>>>>> gen6_ppgtt_cleanup_work(struct work_struct *wrk)
> >>>>>>>>          kfree(work);
> >>>>>>>>      }
> >>>>>>>> +static int nop_set_pages(struct i915_vma *vma)
> >>>>>>>> +{
> >>>>>>>> +     return -ENODEV;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static void nop_clear_pages(struct i915_vma *vma)
> >>>>>>>> +{
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static int nop_bind(struct i915_vma *vma,
> >>>>>>>> +                 enum i915_cache_level cache_level,
> >>>>>>>> +                 u32 unused)
> >>>>>>>> +{
> >>>>>>>> +     return -ENODEV;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static void nop_unbind(struct i915_vma *vma)
> >>>>>>>> +{
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static const struct i915_vma_ops nop_vma_ops = {
> >>>>>>>> +     .set_pages = nop_set_pages,
> >>>>>>>> +     .clear_pages = nop_clear_pages,
> >>>>>>>> +     .bind_vma = nop_bind,
> >>>>>>>> +     .unbind_vma = nop_unbind,
> >>>>>>>> +};
> >>>>>>>> +
> >>>>>>>>      static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> >>>>>>>>      {
> >>>>>>>>          struct gen6_hw_ppgtt *ppgtt = 
> >>>>>>>> to_gen6_ppgtt(i915_vm_to_ppgtt(vm));
> >>>>>>>> @@ -1855,6 +1882,7 @@ static void gen6_ppgtt_cleanup(struct 
> >>>>>>>> i915_address_space *vm)
> >>>>>>>>          /* FIXME remove the struct_mutex to bring the locking 
> >>>>>>>> under control */
> >>>>>>>>          INIT_WORK(&work->base, gen6_ppgtt_cleanup_work);
> >>>>>>>>          work->vma = ppgtt->vma;
> >>>>>>>> +     work->vma->ops = &nop_vma_ops;
> >>>>>>>
> >>>>>>> Could we use some asserts before overriding the vma ops? Like
> >>>>>>> GEM_BUG_ON(vma->pages)? And something for still bound?
> >>>>>>
> >>>>>> It technically still is bound as it is in the GGTT but currently
> >>>>>> unpinned -- that will be checked on destroy, it's just we also get an
> >>>>>> unbind callback. vma->pages doesn't exist for this (set to ERR_PTR).
> >>>>>
> >>>>> If we are getting the unbind callback and we nop-ed it, who will
> >>>>> actually do it's job?
> >>>>
> >>>> The callback is just a hook for us to prune within the ppgtt.
> >>>> It still is removed from GGTT by i915_vma_unbind().
> >>>
> >>> So it needs GEM_BUG_ON(ppgtt->scan_for_unused_pt) before overriding the
> >>> unbind?
> >>
> >> No. They get freed by the cleanup itself. The scan is just an
> >> opportunistic prune if either the context/mm is evicted but still alive.
> > 
> > Then the same assert in gen6_ppgtt_cleanup_work? :)
> 
> Okay ppgtt is gone so can't do it.. annoying.. Cleanup seems to support 
> your claims but I think we need a BFC (big fat comment) above the vma 
> ops override to explains this. With that:

It has FIXME! I really do hope this is short term...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [BrownBag] drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup

[Tvrtko Ursulin]

On 24/05/2019 10:01, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-05-24 09:57:42)
>>
>> On 24/05/2019 09:51, Tvrtko Ursulin wrote:
>>>
>>> On 24/05/2019 09:36, Chris Wilson wrote:
>>>> Quoting Tvrtko Ursulin (2019-05-24 09:31:45)
>>>>>
>>>>> On 24/05/2019 09:29, Chris Wilson wrote:
>>>>>> Quoting Tvrtko Ursulin (2019-05-24 09:23:40)
>>>>>>>
>>>>>>> On 24/05/2019 09:17, Chris Wilson wrote:
>>>>>>>> Quoting Tvrtko Ursulin (2019-05-24 09:13:14)
>>>>>>>>>
>>>>>>>>> On 24/05/2019 07:45, Chris Wilson wrote:
>>>>>>>>>> Having deferred the vma destruction to a worker where we can
>>>>>>>>>> acquire the
>>>>>>>>>> struct_mutex, we have to avoid chasing back into the now destroyed
>>>>>>>>>> ppgtt. The pd_vma is special in having a custom unbind function
>>>>>>>>>> to scan
>>>>>>>>>> for unused pages despite the VMA itself being notionally part of
>>>>>>>>>> the
>>>>>>>>>> GGTT. As such, we need to disable that callback to avoid a
>>>>>>>>>> use-after-free.
>>>>>>>>>>
>>>>>>>>>> This unfortunately blew up so early during boot that CI declared
>>>>>>>>>> the
>>>>>>>>>> machine unreachable as opposed to being the major failure it
>>>>>>>>>> was. Oops.
>>>>>>>>>>
>>>>>>>>>> Fixes: d3622099c76f ("drm/i915/gtt: Always acquire struct_mutex
>>>>>>>>>> for gen6_ppgtt_cleanup")
>>>>>>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>>>>> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
>>>>>>>>>> ---
>>>>>>>>>>       drivers/gpu/drm/i915/i915_gem_gtt.c | 28
>>>>>>>>>> ++++++++++++++++++++++++++++
>>>>>>>>>>       1 file changed, 28 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>>>>>>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>>>>>>> index 8d8a4b0ad4d9..266baa11df64 100644
>>>>>>>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>>>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>>>>>>> @@ -1847,6 +1847,33 @@ static void
>>>>>>>>>> gen6_ppgtt_cleanup_work(struct work_struct *wrk)
>>>>>>>>>>           kfree(work);
>>>>>>>>>>       }
>>>>>>>>>> +static int nop_set_pages(struct i915_vma *vma)
>>>>>>>>>> +{
>>>>>>>>>> +     return -ENODEV;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static void nop_clear_pages(struct i915_vma *vma)
>>>>>>>>>> +{
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static int nop_bind(struct i915_vma *vma,
>>>>>>>>>> +                 enum i915_cache_level cache_level,
>>>>>>>>>> +                 u32 unused)
>>>>>>>>>> +{
>>>>>>>>>> +     return -ENODEV;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static void nop_unbind(struct i915_vma *vma)
>>>>>>>>>> +{
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static const struct i915_vma_ops nop_vma_ops = {
>>>>>>>>>> +     .set_pages = nop_set_pages,
>>>>>>>>>> +     .clear_pages = nop_clear_pages,
>>>>>>>>>> +     .bind_vma = nop_bind,
>>>>>>>>>> +     .unbind_vma = nop_unbind,
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>>       static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>>>>>>>>>>       {
>>>>>>>>>>           struct gen6_hw_ppgtt *ppgtt =
>>>>>>>>>> to_gen6_ppgtt(i915_vm_to_ppgtt(vm));
>>>>>>>>>> @@ -1855,6 +1882,7 @@ static void gen6_ppgtt_cleanup(struct
>>>>>>>>>> i915_address_space *vm)
>>>>>>>>>>           /* FIXME remove the struct_mutex to bring the locking
>>>>>>>>>> under control */
>>>>>>>>>>           INIT_WORK(&work->base, gen6_ppgtt_cleanup_work);
>>>>>>>>>>           work->vma = ppgtt->vma;
>>>>>>>>>> +     work->vma->ops = &nop_vma_ops;
>>>>>>>>>
>>>>>>>>> Could we use some asserts before overriding the vma ops? Like
>>>>>>>>> GEM_BUG_ON(vma->pages)? And something for still bound?
>>>>>>>>
>>>>>>>> It technically still is bound as it is in the GGTT but currently
>>>>>>>> unpinned -- that will be checked on destroy, it's just we also get an
>>>>>>>> unbind callback. vma->pages doesn't exist for this (set to ERR_PTR).
>>>>>>>
>>>>>>> If we are getting the unbind callback and we nop-ed it, who will
>>>>>>> actually do it's job?
>>>>>>
>>>>>> The callback is just a hook for us to prune within the ppgtt.
>>>>>> It still is removed from GGTT by i915_vma_unbind().
>>>>>
>>>>> So it needs GEM_BUG_ON(ppgtt->scan_for_unused_pt) before overriding the
>>>>> unbind?
>>>>
>>>> No. They get freed by the cleanup itself. The scan is just an
>>>> opportunistic prune if either the context/mm is evicted but still alive.
>>>
>>> Then the same assert in gen6_ppgtt_cleanup_work? :)
>>
>> Okay ppgtt is gone so can't do it.. annoying.. Cleanup seems to support
>> your claims but I think we need a BFC (big fat comment) above the vma
>> ops override to explains this. With that:
> 
> It has FIXME! I really do hope this is short term...

Ok, that's good enough.

Regards,

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

✗ Fi.CI.BAT: failure for drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup

[Patchwork]

== Series Details ==

Series: drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup
URL   : https://patchwork.freedesktop.org/series/61084/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6138 -> Patchwork_13087
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_13087 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_13087, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13087/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_13087:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@reload:
    - fi-apl-guc:         NOTRUN -> [DMESG-WARN][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13087/fi-apl-guc/igt@i915_module_load@reload.html

  * igt@runner@aborted:
    - fi-apl-guc:         NOTRUN -> [FAIL][2]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13087/fi-apl-guc/igt@runner@aborted.html

  
Known issues
------------

  Here are the changes found in Patchwork_13087 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       [PASS][3] -> [INCOMPLETE][4] ([fdo#107718])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6138/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13087/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-6770hq:      [PASS][5] -> [FAIL][6] ([fdo#108511])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6138/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13087/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html

  * igt@kms_flip@basic-flip-vs-dpms:
    - fi-bxt-dsi:         [PASS][7] -> [INCOMPLETE][8] ([fdo#103927])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6138/fi-bxt-dsi/igt@kms_flip@basic-flip-vs-dpms.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13087/fi-bxt-dsi/igt@kms_flip@basic-flip-vs-dpms.html

  
#### Possible fixes ####

  * {igt@gem_busy@busy-all}:
    - {fi-icl-y}:         [INCOMPLETE][9] ([fdo#107713]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6138/fi-icl-y/igt@gem_busy@busy-all.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13087/fi-icl-y/igt@gem_busy@busy-all.html

  * igt@gem_ctx_create@basic-files:
    - {fi-icl-u2}:        [INCOMPLETE][11] ([fdo#107713] / [fdo#109100]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6138/fi-icl-u2/igt@gem_ctx_create@basic-files.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13087/fi-icl-u2/igt@gem_ctx_create@basic-files.html

  * igt@i915_selftest@live_contexts:
    - fi-skl-gvtdvm:      [DMESG-FAIL][13] ([fdo#110235]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6138/fi-skl-gvtdvm/igt@i915_selftest@live_contexts.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13087/fi-skl-gvtdvm/igt@i915_selftest@live_contexts.html

  * igt@i915_selftest@live_hangcheck:
    - {fi-icl-u3}:        [INCOMPLETE][15] ([fdo#107713] / [fdo#108569]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6138/fi-icl-u3/igt@i915_selftest@live_hangcheck.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13087/fi-icl-u3/igt@i915_selftest@live_hangcheck.html

  * {igt@i915_selftest@live_reset}:
    - fi-skl-iommu:       [INCOMPLETE][17] -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6138/fi-skl-iommu/igt@i915_selftest@live_reset.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13087/fi-skl-iommu/igt@i915_selftest@live_reset.html

  * igt@kms_frontbuffer_tracking@basic:
    - {fi-icl-u3}:        [FAIL][19] ([fdo#103167]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6138/fi-icl-u3/igt@kms_frontbuffer_tracking@basic.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13087/fi-icl-u3/igt@kms_frontbuffer_tracking@basic.html

  * igt@prime_vgem@basic-sync-default:
    - fi-pnv-d510:        [INCOMPLETE][21] ([fdo#110740]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6138/fi-pnv-d510/igt@prime_vgem@basic-sync-default.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13087/fi-pnv-d510/igt@prime_vgem@basic-sync-default.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108511]: https://bugs.freedesktop.org/show_bug.cgi?id=108511
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#110235]: https://bugs.freedesktop.org/show_bug.cgi?id=110235
  [fdo#110580 ]: https://bugs.freedesktop.org/show_bug.cgi?id=110580 
  [fdo#110718]: https://bugs.freedesktop.org/show_bug.cgi?id=110718
  [fdo#110740]: https://bugs.freedesktop.org/show_bug.cgi?id=110740
  [fdo#110753]: https://bugs.freedesktop.org/show_bug.cgi?id=110753


Participating hosts (42 -> 45)
------------------------------

  Additional (7): fi-hsw-4770r fi-byt-j1900 fi-hsw-peppy fi-apl-guc fi-hsw-4770 fi-ivb-3770 fi-byt-n2820 
  Missing    (4): fi-skl-guc fi-ilk-m540 fi-bsw-cyan fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_6138 -> Patchwork_13087

  CI_DRM_6138: a17cee540f7ffd493c7944dea5ea6e2d32fdfdb4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5011: 7f120c5f1bff2727d50f3c392d81c0f6878b61d6 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13087: 25610cdceea315bd495e3fe7533db997fa64c6f5 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

25610cdceea3 drm/i915/gtt: Neuter the deferred unbind callback from gen6_ppgtt_cleanup

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13087/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx