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?

Ok ok I'm sold, at least until we find otherwise or Alan's team reports
back that the GTT space is needed.

Extending the interface to not reserve GTT space makes sense; I can do
that as a follow up, if you're ok with the existing code.

-- 
Jesse Barnes, Intel Open Source Technology Center