Re: [Intel-gfx] [PATCH 8/9] drm/i915/gtt: Make swapping the pd entry generic

[Chris Wilson]

Quoting Chris Wilson (2019-06-11 20:50:09)
> Quoting Mika Kuoppala (2019-06-11 18:27:30)
> > Swapping a pd entry is same across the page directories, if
> > we succeed we need to increment the count and write the phys page
> > entry. Make a common function for it.
> > 
> > Signed-off-by: Mika Kuoppala 
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 41 +++--
> >  1 file changed, 27 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index f1d7874834e2..9b0d0a077e31 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -752,6 +752,27 @@ static void __set_pd_entry(struct i915_page_directory 
> > * const pd,
> >gen8_pde_encode(px_dma(to), I915_CACHE_LLC));\
> >  })
> >  
> > +static void *__swap_pd_entry(struct i915_page_directory * const pd,
> > +const unsigned short pde,
> > +void * const old_val,
> > +void * const new_val,
> > +const u64 encoded_entry)
> 
> Mark this as inline and pass in the encode function, the compiler should
> do the rest.
> 
> > +{
> > +   void * const old = cmpxchg(&pd->entry[pde], old_val, new_val);
> > +
> > +   if (likely(old == old_val)) {
> > +   atomic_inc(&pd->used);
> 
> Hmm, looking at this again, this would be safer if atomic_inc was before
> the cmpxchg, with an atomic_dec on the fail path.

Actually already taken care of, the parent pins the child page directory
before entering.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 8/9] drm/i915/gtt: Make swapping the pd entry generic

[Chris Wilson]

Quoting Mika Kuoppala (2019-06-11 18:27:30)
> Swapping a pd entry is same across the page directories, if
> we succeed we need to increment the count and write the phys page
> entry. Make a common function for it.
> 
> Signed-off-by: Mika Kuoppala 
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 41 +++--
>  1 file changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index f1d7874834e2..9b0d0a077e31 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -752,6 +752,27 @@ static void __set_pd_entry(struct i915_page_directory * 
> const pd,
>gen8_pde_encode(px_dma(to), I915_CACHE_LLC));\
>  })
>  
> +static void *__swap_pd_entry(struct i915_page_directory * const pd,
> +const unsigned short pde,
> +void * const old_val,
> +void * const new_val,
> +const u64 encoded_entry)

Mark this as inline and pass in the encode function, the compiler should
do the rest.

> +{
> +   void * const old = cmpxchg(&pd->entry[pde], old_val, new_val);
> +
> +   if (likely(old == old_val)) {
> +   atomic_inc(&pd->used);

Hmm, looking at this again, this would be safer if atomic_inc was before
the cmpxchg, with an atomic_dec on the fail path.

> +   if (likely(pd_has_phys_page(pd)))
> +   __set_pd_entry(pd, pde, encoded_entry);
> +   }
> +
> +   return old;
> +}
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 8/9] drm/i915/gtt: Make swapping the pd entry generic

[Mika Kuoppala]

Swapping a pd entry is same across the page directories, if
we succeed we need to increment the count and write the phys page
entry. Make a common function for it.

Signed-off-by: Mika Kuoppala 
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 41 +++--
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index f1d7874834e2..9b0d0a077e31 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -752,6 +752,27 @@ static void __set_pd_entry(struct i915_page_directory * 
const pd,
   gen8_pde_encode(px_dma(to), I915_CACHE_LLC));\
 })
 
+static void *__swap_pd_entry(struct i915_page_directory * const pd,
+const unsigned short pde,
+void * const old_val,
+void * const new_val,
+const u64 encoded_entry)
+{
+   void * const old = cmpxchg(&pd->entry[pde], old_val, new_val);
+
+   if (likely(old == old_val)) {
+   atomic_inc(&pd->used);
+   if (likely(pd_has_phys_page(pd)))
+   __set_pd_entry(pd, pde, encoded_entry);
+   }
+
+   return old;
+}
+
+#define swap_pd_entry(pd, pde, old, to) \
+   __swap_pd_entry((pd), (pde), (old), (to), \
+   gen8_pde_encode(px_dma(to), I915_CACHE_LLC))
+
 /*
  * PDE TLBs are a pain to invalidate on GEN8+. When we modify
  * the page table structures, we mark them dirty so that
@@ -1326,11 +1347,8 @@ static int gen8_ppgtt_alloc_pd(struct i915_address_space 
*vm,
if (count < GEN8_PTES || intel_vgpu_active(vm->i915))
gen8_initialize_pt(vm, pt);
 
-   old = cmpxchg(&pd->entry[pde], vm->scratch_pt, pt);
-   if (old == vm->scratch_pt) {
-   set_pd_entry(pd, pde, pt);
-   atomic_inc(&pd->used);
-   } else {
+   old = swap_pd_entry(pd, pde, vm->scratch_pt, pt);
+   if (unlikely(old != vm->scratch_pt)) {
free_pt(vm, pt);
pt = old;
}
@@ -1371,11 +1389,8 @@ static int gen8_ppgtt_alloc_pdp(struct 
i915_address_space *vm,
 
init_pd(vm, pd, vm->scratch_pt);
 
-   old = cmpxchg(&pdp->entry[pdpe], vm->scratch_pd, pd);
-   if (old == vm->scratch_pd) {
-   set_pd_entry(pdp, pdpe, pd);
-   atomic_inc(&pdp->used);
-   } else {
+   old = swap_pd_entry(pdp, pdpe, vm->scratch_pd, pd);
+   if (unlikely(old != vm->scratch_pd)) {
free_pd(vm, pd);
pd = old;
}
@@ -1440,10 +1455,8 @@ static int gen8_ppgtt_alloc_4lvl(struct 
i915_address_space *vm,
 
init_pd(vm, pdp, vm->scratch_pd);
 
-   old = cmpxchg(&pml4->entry[pml4e], vm->scratch_pdp, 
pdp);
-   if (old == vm->scratch_pdp) {
-   set_pd_entry(pml4, pml4e, pdp);
-   } else {
+   old = swap_pd_entry(pml4, pml4e, vm->scratch_pdp, pdp);
+   if (unlikely(old != vm->scratch_pdp)) {
free_pd(vm, pdp);
pdp = old;
}
-- 
2.17.1

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