Re: [PATCH 2/7] drm/i915: Resolving the memory region conflict for Stolen area

From: Jesse Barnes <jbarnes@virtuousgeek.org>
To: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: akash.goel@intel.com, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/7] drm/i915: Resolving the memory region conflict for Stolen area
Date: Mon, 3 Mar 2014 14:33:09 -0800	[thread overview]
Message-ID: <20140303143309.7a1f5a90@jbarnes-desktop> (raw)
In-Reply-To: <20140303111409.76401ce2@jbarnes-desktop>

On Mon, 3 Mar 2014 11:14:09 -0800
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Thu, 27 Feb 2014 11:01:08 +0200
> Jani Nikula <jani.nikula@linux.intel.com> wrote:
> 
> > On Thu, 27 Feb 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > > On Wed, 26 Feb 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > >> On Mon, 13 Jan 2014 16:25:21 +0530
> > >> akash.goel@intel.com wrote:
> > >>
> > >>> From: Akash Goel <akash.goel@intel.com>
> > >>> 
> > >>> There is a conflict seen when requesting the kernel to reserve
> > >>> the physical space used for the stolen area. This is because
> > >>> some BIOS are wrapping the stolen area in the root PCI bus, but have
> > >>> an off-by-one error. As a workaround we retry the reservation with an
> > >>> offset of 1 instead of 0.
> > >>> 
> > >>> v2: updated commit message & the comment in source file (Daniel)
> > >>> 
> > >>> Signed-off-by: Akash Goel <akash.goel@intel.com>
> > >>> ---
> > >>>  drivers/gpu/drm/i915/i915_gem_stolen.c | 20 +++++++++++++++++---
> > >>>  1 file changed, 17 insertions(+), 3 deletions(-)
> > >>> 
> > >>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > >>> index 1a24e84..114a806 100644
> > >>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > >>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > >>> @@ -82,9 +82,23 @@ static unsigned long i915_stolen_to_physical(struct drm_device *dev)
> > >>>  	r = devm_request_mem_region(dev->dev, base, dev_priv->gtt.stolen_size,
> > >>>  				    "Graphics Stolen Memory");
> > >>>  	if (r == NULL) {
> > >>> -		DRM_ERROR("conflict detected with stolen region: [0x%08x - 0x%08x]\n",
> > >>> -			  base, base + (uint32_t)dev_priv->gtt.stolen_size);
> > >>> -		base = 0;
> > >>> +		/*
> > >>> +		 * One more attempt but this time requesting region from
> > >>> +		 * base + 1, as we have seen that this resolves the region
> > >>> +		 * conflict with the PCI Bus.
> > >>> +		 * This is a BIOS w/a: Some BIOS wrap stolen in the root
> > >>> +		 * PCI bus, but have an off-by-one error. Hence retry the
> > >>> +		 * reservation starting from 1 instead of 0.
> > >>> +		 */
> > >>> +		r = devm_request_mem_region(dev->dev, base + 1,
> > >>> +					    dev_priv->gtt.stolen_size - 1,
> > >>> +					    "Graphics Stolen Memory");
> > >>> +		if (r == NULL) {
> > >>> +			DRM_ERROR("conflict detected with stolen region:"\
> > >>> +				  "[0x%08x - 0x%08x]\n",
> > >>> +				  base, base + (uint32_t)dev_priv->gtt.stolen_size);
> > >>> +			base = 0;
> > >>> +		}
> > >>>  	}
> > >>>  
> > >>>  	return base;
> > >>
> > >> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > >> Tested-by: Arjan van de Ven <arjan@linux.intel.com>
> > >
> > > Pushed to -fixes, thanks for the patch, review, and testing.
> > 
> > Oh, forgot to add that I fixed the DRM_ERROR string split up that
> > checkpatch complained about while applying.
> 
> Note I think we should do this at the x86 quirk level as well since
> otherwise the kernel may put MMIO space on top of our stolen range.

Nevermind, I think the stolen core code will do the right thing and
mark this region reserved... but it's worth checking on an affected
system.

-- 
Jesse Barnes, Intel Open Source Technology Center