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

[Matthew Brost]

On Thu, Dec 10, 2020 at 09:00:53PM +0000, Chris Wilson wrote:
> Quoting Matthew Brost (2020-12-10 19:28:06)
> > On Thu, Dec 10, 2020 at 08:02:37AM +0000, 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 <matthew.brost at intel.com>

> -Chris