Re: [PATCH] drm/i915: BIOS and power context stolen mem handling for VLV v6

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue, 7 May 2013 23:14:40 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Tue, May 07, 2013 at 03:08:42PM -0700, Jesse Barnes wrote:
> > On Tue, 7 May 2013 22:57:37 +0100
> > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 
> > > On Tue, May 07, 2013 at 02:34:31PM -0700, Jesse Barnes wrote:
> > > > +static void valleyview_setup_pctx(struct drm_device *dev)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +	struct drm_i915_gem_object *pctx;
> > > > +	unsigned long pctx_paddr;
> > > > +	u32 pcbr;
> > > > +	int pctx_size = 24*1024;
> > > > +
> > > > +	pcbr = I915_READ(VLV_PCBR);
> > > > +	if (pcbr) {
> > > > +		/* BIOS set it up already, grab the pre-alloc'd space */
> > > > +		int pcbr_offset;
> > > > +
> > > > +		pcbr_offset = (pcbr & (~4095)) - dev_priv->mm.stolen_base;
> > > > +		pctx = i915_gem_object_create_stolen_for_preallocated(dev_priv->dev,
> > > > +								      pcbr_offset,
> > > > +								      pcbr_offset,
> > > > +								      pctx_size);
> > > 
> > > We're reserving global GTT space here for someting that just looks like
> > > it requires contiguous physical memory. This may cause issues if
> > > something else is already bound to that GTT range. I think we want to
> > > extend the interface to not reserve GTT space if gtt_offset == -1.
> > 
> > I should have added a comment for this too.  The physical range for the
> > power context must reside in stolen space.  So while it needs
> > contiguous physical memory, it also has a relationship to the stolen
> > handling.  I'd prefer to see a failure here than to try to do something
> > fancy with funky GTT vs stolen setups...  (Still waiting on the BIOS
> > guys to promise the identity map going forward.)
> 
> Hmm, I am not understanding exactly what you need here. The register you
> are reading looks to be a physical address - so why do we need to also
> reserve a virtual address with the same offset? And if that is required,
> shouldn't the else branch also reserve the corresponding virtual
> address for its stolen allocation?

I thought the stolen create code would do that for me; on looking I see
I'm wrong.

I'm conflating the stolen physically contiguous range here with the
first stolen_size portion of the GTT space.  I don't know if anything
depends on that or not, but given the way things are set up at boot
time I thought it would be best not to break that assumption.

-- 
Jesse Barnes, Intel Open Source Technology Center