[Intel-gfx] [PATCH] drm/i915: Put all permanent stolen allocations together

[Chris Wilson]

Quoting Ville Syrjälä (2018-09-11 17:13:18)
> On Tue, Sep 11, 2018 at 04:14:39PM +0100, Chris Wilson wrote:
> > @@ -513,7 +515,8 @@ static int intel_fbc_alloc_cfb(struct intel_crtc *crtc)
> >                       goto err_fb;
> >  
> >               ret = i915_gem_stolen_insert_node(dev_priv, compressed_llb,
> > -                                               4096, 4096);
> > +                                               4096, 4096,
> > +                                               DRM_MM_INSERT_LOW);
> 
> We seem to alloc/free the line length buffer alongside the cfb.
> So should this use best instead?

Ok, a quick glance suggested that this might have been alloc once.

> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index d99e5fabe93c..5d18301ba079 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -7382,7 +7382,8 @@ static void valleyview_setup_pctx(struct drm_i915_private *dev_priv)
> >        * overlap with other ranges, such as the frame buffer, protected
> >        * memory, or any other relevant ranges.
> >        */
> > -     pctx = i915_gem_object_create_stolen(dev_priv, pctx_size);
> > +     pctx = i915_gem_object_create_stolen(dev_priv,
> > +                                          pctx_size, DRM_MM_INSERT_LOW);
> 
> I guess there was no special requirement for the placement of this.
> AFAIK the BIOS always allocates it just below the wopcm, but I suppose
> it doesn't matter if we take a different approach.

HIGH or LOW doesn't make much difference. Could be HIGH just for fun ;)

> >       if (!pctx) {
> >               DRM_DEBUG("not enough stolen space for PCTX, disabling\n");
> >               goto out;
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 472939f5c18f..e6a23a241cf3 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -1104,7 +1104,7 @@ intel_ring_create_vma(struct drm_i915_private *dev_priv, int size)
> >       struct drm_i915_gem_object *obj;
> >       struct i915_vma *vma;
> >  
> > -     obj = i915_gem_object_create_stolen(dev_priv, size);
> > +     obj = i915_gem_object_create_stolen(dev_priv, size, DRM_MM_INSERT_BEST);
> 
> Should these go low? We never reallocate them, right?

There's one per context per engine in execlists, i.e. they are
transient. I didn't feel it was worth differentiating
execlists/legacy as a few allocations once upon startup are more than
likely to be allocated sequentially from the bottom.

Hmm. That does get trickier then as it means that depending on order if
there are any transient allocations amongst the startup, we still end up
with holes and fragmentation layer. Needs a little more thought.
-Chris