subject:"\[Intel\-gfx\] \[PATCH 18\/21\] drm\/i915\/gt\: Add timeline \"mode\""

Re: [Intel-gfx] [PATCH 18/21] drm/i915/gt: Add timeline "mode"

2020-12-10 Thread Matthew Brost

On Thu, Dec 10, 2020 at 09:00:53PM +, Chris Wilson wrote:
> Quoting Matthew Brost (2020-12-10 19:28:06)
> > On Thu, Dec 10, 2020 at 08:02:37AM +, Chris Wilson wrote:
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_timeline_types.h 
> > > b/drivers/gpu/drm/i915/gt/intel_timeline_types.h
> > > index f187c5aac11c..32c51425a0c4 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_timeline_types.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_timeline_types.h
> > > @@ -20,6 +20,12 @@ struct i915_syncmap;
> > >  struct intel_gt;
> > >  struct intel_timeline_hwsp;
> > >  
> > > +enum intel_timeline_mode {
> > > + INTEL_TIMELINE_ABSOLUTE = 0,
> > > + INTEL_TIMELINE_CONTEXT = BIT(0),
> > > + INTEL_TIMELINE_GLOBAL = BIT(1),
> > > +};
> > > +
> > 
> > Not sure I like these names.
> > 
> > How about:
> > INTEL_TIMELINE_ABSOLUTE_GGTT
> > INTEL_TIMELINE_RELATIVE_PPGTT
> > INTEL_TIMELINE_RELATIVE_GGTT
> 
> They are all in the GGTT, including the ppHWSP.
>

Ah, got it. The 'MI_FLUSH_DW_USE_GTT' in a later patch threw me off. I
see now that it is picking between global status page and per-process
page in that case.

> One is relative to the context, the other relative to the engine.
> 
>   INTEL_TIMELINE_ABSOLUTE
>   INTEL_TIMELINE_RELATIVE_CONTEXT
>   INTEL_TIMELINE_RELATIVE_ENGINE
>

I like these names better.

> > Also not convinced we need the 'RELATIVE' modes. See my comments in 'Use
> > indices for writing into relative'.
> 
> It saves extra allocations for when we don't (e.g. gen8, and other
> contexts where we know we will never require disposable slots), and
> there's a strong incentive to not use absolute addressing with GVT

Understand using the status page to save on allocations.

I could see relative addressing helping with GVT.

With the name nits:
Reviewed-by: Matthew Brost 

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

Re: [Intel-gfx] [PATCH 18/21] drm/i915/gt: Add timeline "mode"

2020-12-10 Thread Chris Wilson

Quoting Matthew Brost (2020-12-10 19:28:06)
> On Thu, Dec 10, 2020 at 08:02:37AM +, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/gt/intel_timeline_types.h 
> > b/drivers/gpu/drm/i915/gt/intel_timeline_types.h
> > index f187c5aac11c..32c51425a0c4 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_timeline_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_timeline_types.h
> > @@ -20,6 +20,12 @@ struct i915_syncmap;
> >  struct intel_gt;
> >  struct intel_timeline_hwsp;
> >  
> > +enum intel_timeline_mode {
> > + INTEL_TIMELINE_ABSOLUTE = 0,
> > + INTEL_TIMELINE_CONTEXT = BIT(0),
> > + INTEL_TIMELINE_GLOBAL = BIT(1),
> > +};
> > +
> 
> Not sure I like these names.
> 
> How about:
> INTEL_TIMELINE_ABSOLUTE_GGTT
> INTEL_TIMELINE_RELATIVE_PPGTT
> INTEL_TIMELINE_RELATIVE_GGTT

They are all in the GGTT, including the ppHWSP.

One is relative to the context, the other relative to the engine.

  INTEL_TIMELINE_ABSOLUTE
  INTEL_TIMELINE_RELATIVE_CONTEXT
  INTEL_TIMELINE_RELATIVE_ENGINE

> Also not convinced we need the 'RELATIVE' modes. See my comments in 'Use
> indices for writing into relative'.

It saves extra allocations for when we don't (e.g. gen8, and other
contexts where we know we will never require disposable slots), and
there's a strong incentive to not use absolute addressing with GVT.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 18/21] drm/i915/gt: Add timeline "mode"

2020-12-10 Thread Matthew Brost

On Thu, Dec 10, 2020 at 08:02:37AM +, Chris Wilson wrote:
> Explicitly differentiate between the absolute and relative timelines,
> and the global HWSP and ppHWSP relative offsets. When using a timeline
> that is relative to a known status page, we can replace the absolute
> addressing in the commands with indexed variants.
> 
> Signed-off-by: Chris Wilson 
> ---
>  drivers/gpu/drm/i915/gt/intel_timeline.c  | 21 ---
>  drivers/gpu/drm/i915/gt/intel_timeline.h  |  2 +-
>  .../gpu/drm/i915/gt/intel_timeline_types.h| 10 +++--
>  3 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.c 
> b/drivers/gpu/drm/i915/gt/intel_timeline.c
> index cb20fcbb326b..da0a9659557a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_timeline.c
> +++ b/drivers/gpu/drm/i915/gt/intel_timeline.c
> @@ -229,7 +229,6 @@ static int intel_timeline_init(struct intel_timeline 
> *timeline,
>  
>   timeline->gt = gt;
>  
> - timeline->has_initial_breadcrumb = !hwsp;
>   timeline->hwsp_cacheline = NULL;
>  
>   if (!hwsp) {
> @@ -246,13 +245,29 @@ static int intel_timeline_init(struct intel_timeline 
> *timeline,
>   return PTR_ERR(cl);
>   }
>  
> + timeline->mode = INTEL_TIMELINE_ABSOLUTE;
>   timeline->hwsp_cacheline = cl;
>   timeline->hwsp_offset = cacheline * CACHELINE_BYTES;
>  
>   vaddr = page_mask_bits(cl->vaddr);
>   } else {
> - timeline->hwsp_offset = offset;
> - vaddr = i915_gem_object_pin_map(hwsp->obj, I915_MAP_WB);
> + int preferred;
> +
> + if (offset & INTEL_TIMELINE_CONTEXT) {
> + timeline->mode = INTEL_TIMELINE_CONTEXT;
> + timeline->hwsp_offset =
> + offset & ~INTEL_TIMELINE_CONTEXT;
> + preferred = i915_coherent_map_type(gt->i915);
> + } else {
> + timeline->mode = INTEL_TIMELINE_GLOBAL;
> + timeline->hwsp_offset = offset;
> + preferred = I915_MAP_WB;
> + }
> +
> + vaddr = i915_gem_object_pin_map(hwsp->obj,
> + preferred | I915_MAP_OVERRIDE);
> + if (IS_ERR(vaddr))
> + vaddr = i915_gem_object_pin_map(hwsp->obj, I915_MAP_WC);
>   if (IS_ERR(vaddr))
>   return PTR_ERR(vaddr);
>   }
> diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.h 
> b/drivers/gpu/drm/i915/gt/intel_timeline.h
> index deb71a8dbd43..69250de3a814 100644
> --- a/drivers/gpu/drm/i915/gt/intel_timeline.h
> +++ b/drivers/gpu/drm/i915/gt/intel_timeline.h
> @@ -76,7 +76,7 @@ static inline void intel_timeline_put(struct intel_timeline 
> *timeline)
>  static inline bool
>  intel_timeline_has_initial_breadcrumb(const struct intel_timeline *tl)
>  {
> - return tl->has_initial_breadcrumb;
> + return tl->mode == INTEL_TIMELINE_ABSOLUTE;
>  }
>  
>  static inline int __intel_timeline_sync_set(struct intel_timeline *tl,
> diff --git a/drivers/gpu/drm/i915/gt/intel_timeline_types.h 
> b/drivers/gpu/drm/i915/gt/intel_timeline_types.h
> index f187c5aac11c..32c51425a0c4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_timeline_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_timeline_types.h
> @@ -20,6 +20,12 @@ struct i915_syncmap;
>  struct intel_gt;
>  struct intel_timeline_hwsp;
>  
> +enum intel_timeline_mode {
> + INTEL_TIMELINE_ABSOLUTE = 0,
> + INTEL_TIMELINE_CONTEXT = BIT(0),
> + INTEL_TIMELINE_GLOBAL = BIT(1),
> +};
> +

Not sure I like these names.

How about:
INTEL_TIMELINE_ABSOLUTE_GGTT
INTEL_TIMELINE_RELATIVE_PPGTT
INTEL_TIMELINE_RELATIVE_GGTT

Also not convinced we need the 'RELATIVE' modes. See my comments in 'Use
indices for writing into relative'.

Matt

>  struct intel_timeline {
>   u64 fence_context;
>   u32 seqno;
> @@ -45,6 +51,8 @@ struct intel_timeline {
>   atomic_t pin_count;
>   atomic_t active_count;
>  
> + enum intel_timeline_mode mode;
> +
>   const u32 *hwsp_seqno;
>   struct i915_vma *hwsp_ggtt;
>   u32 hwsp_offset;
> @@ -52,8 +60,6 @@ struct intel_timeline {
>  
>   struct intel_timeline_cacheline *hwsp_cacheline;
>  
> - bool has_initial_breadcrumb;
> -
>   /**
>* List of breadcrumbs associated with GPU requests currently
>* outstanding.
> -- 
> 2.20.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 18/21] drm/i915/gt: Add timeline "mode"

2020-12-10 Thread Chris Wilson

Explicitly differentiate between the absolute and relative timelines,
and the global HWSP and ppHWSP relative offsets. When using a timeline
that is relative to a known status page, we can replace the absolute
addressing in the commands with indexed variants.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/gt/intel_timeline.c  | 21 ---
 drivers/gpu/drm/i915/gt/intel_timeline.h  |  2 +-
 .../gpu/drm/i915/gt/intel_timeline_types.h| 10 +++--
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.c 
b/drivers/gpu/drm/i915/gt/intel_timeline.c
index cb20fcbb326b..da0a9659557a 100644
--- a/drivers/gpu/drm/i915/gt/intel_timeline.c
+++ b/drivers/gpu/drm/i915/gt/intel_timeline.c
@@ -229,7 +229,6 @@ static int intel_timeline_init(struct intel_timeline 
*timeline,
 
timeline->gt = gt;
 
-   timeline->has_initial_breadcrumb = !hwsp;
timeline->hwsp_cacheline = NULL;
 
if (!hwsp) {
@@ -246,13 +245,29 @@ static int intel_timeline_init(struct intel_timeline 
*timeline,
return PTR_ERR(cl);
}
 
+   timeline->mode = INTEL_TIMELINE_ABSOLUTE;
timeline->hwsp_cacheline = cl;
timeline->hwsp_offset = cacheline * CACHELINE_BYTES;
 
vaddr = page_mask_bits(cl->vaddr);
} else {
-   timeline->hwsp_offset = offset;
-   vaddr = i915_gem_object_pin_map(hwsp->obj, I915_MAP_WB);
+   int preferred;
+
+   if (offset & INTEL_TIMELINE_CONTEXT) {
+   timeline->mode = INTEL_TIMELINE_CONTEXT;
+   timeline->hwsp_offset =
+   offset & ~INTEL_TIMELINE_CONTEXT;
+   preferred = i915_coherent_map_type(gt->i915);
+   } else {
+   timeline->mode = INTEL_TIMELINE_GLOBAL;
+   timeline->hwsp_offset = offset;
+   preferred = I915_MAP_WB;
+   }
+
+   vaddr = i915_gem_object_pin_map(hwsp->obj,
+   preferred | I915_MAP_OVERRIDE);
+   if (IS_ERR(vaddr))
+   vaddr = i915_gem_object_pin_map(hwsp->obj, I915_MAP_WC);
if (IS_ERR(vaddr))
return PTR_ERR(vaddr);
}
diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.h 
b/drivers/gpu/drm/i915/gt/intel_timeline.h
index deb71a8dbd43..69250de3a814 100644
--- a/drivers/gpu/drm/i915/gt/intel_timeline.h
+++ b/drivers/gpu/drm/i915/gt/intel_timeline.h
@@ -76,7 +76,7 @@ static inline void intel_timeline_put(struct intel_timeline 
*timeline)
 static inline bool
 intel_timeline_has_initial_breadcrumb(const struct intel_timeline *tl)
 {
-   return tl->has_initial_breadcrumb;
+   return tl->mode == INTEL_TIMELINE_ABSOLUTE;
 }
 
 static inline int __intel_timeline_sync_set(struct intel_timeline *tl,
diff --git a/drivers/gpu/drm/i915/gt/intel_timeline_types.h 
b/drivers/gpu/drm/i915/gt/intel_timeline_types.h
index f187c5aac11c..32c51425a0c4 100644
--- a/drivers/gpu/drm/i915/gt/intel_timeline_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_timeline_types.h
@@ -20,6 +20,12 @@ struct i915_syncmap;
 struct intel_gt;
 struct intel_timeline_hwsp;
 
+enum intel_timeline_mode {
+   INTEL_TIMELINE_ABSOLUTE = 0,
+   INTEL_TIMELINE_CONTEXT = BIT(0),
+   INTEL_TIMELINE_GLOBAL = BIT(1),
+};
+
 struct intel_timeline {
u64 fence_context;
u32 seqno;
@@ -45,6 +51,8 @@ struct intel_timeline {
atomic_t pin_count;
atomic_t active_count;
 
+   enum intel_timeline_mode mode;
+
const u32 *hwsp_seqno;
struct i915_vma *hwsp_ggtt;
u32 hwsp_offset;
@@ -52,8 +60,6 @@ struct intel_timeline {
 
struct intel_timeline_cacheline *hwsp_cacheline;
 
-   bool has_initial_breadcrumb;
-
/**
 * List of breadcrumbs associated with GPU requests currently
 * outstanding.
-- 
2.20.1

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

Re: [Intel-gfx] [PATCH 18/21] drm/i915/gt: Add timeline "mode"

Re: [Intel-gfx] [PATCH 18/21] drm/i915/gt: Add timeline "mode"

Re: [Intel-gfx] [PATCH 18/21] drm/i915/gt: Add timeline "mode"

[Intel-gfx] [PATCH 18/21] drm/i915/gt: Add timeline "mode"

4 matches

Site Navigation

Mail list logo

Footer information