| Message ID | 1389245417-10813-1-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 09, 2014 at 11:00:17AM +0530, akash.goel@intel.com wrote: > From: Akash Goel <akash.goel@intel.com> > > The 'offset' field of the 'scatterlist' structure was wrongly > programmed with the offset value from the base of stolen area, > whereas this field indicates the offset from where the interested > data starts within the PAGE pointed to by the 'page-link' field. > As a result when a new GEM object allocated from the stolen > area is mapped to GTT, it could lead to an overwrite of GTT entries > as the page count calculation will go wrong, refer the function > 'sg_page_count'. This statement is incorrect since my use of sg here predates sg_page_iter. The stolen sg has no page_link, the meaning of offset/length here are relative to the base of the stolen area. However, if you wish to rephrase the above... -Chris
On Thu, Jan 09, 2014 at 09:45:21AM +0000, Chris Wilson wrote: > On Thu, Jan 09, 2014 at 11:00:17AM +0530, akash.goel@intel.com wrote: > > From: Akash Goel <akash.goel@intel.com> > > > > The 'offset' field of the 'scatterlist' structure was wrongly > > programmed with the offset value from the base of stolen area, > > whereas this field indicates the offset from where the interested > > data starts within the PAGE pointed to by the 'page-link' field. > > As a result when a new GEM object allocated from the stolen > > area is mapped to GTT, it could lead to an overwrite of GTT entries > > as the page count calculation will go wrong, refer the function > > 'sg_page_count'. > > This statement is incorrect since my use of sg here predates > sg_page_iter. > > The stolen sg has no page_link, the meaning of offset/length here are > relative to the base of the stolen area. > > However, if you wish to rephrase the above... Actually we add offset both to sg->offset and adjust the dma_bus_addr since this has been introduced in commit 0104fdbb84d7adb0e377ed05bf75eba97b007544 Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Thu Nov 15 11:32:26 2012 +0000 drm/i915: Introduce i915_gem_object_create_stolen() But only Imre's conversion to the sg_page_iter started to pay any attention to sg->offset in commit 6e995e231a90ce7c5ce2a9eae23c8e22f4388db1 Author: Imre Deak <imre.deak@intel.com> Date: Mon Feb 18 19:28:04 2013 +0200 drm/i915: use for_each_sg_page for setting up the gtt ptes So with a bit of commit message rewording and these references and cc: stable this looks good. Cheers, Daniel
On Thu, Jan 09, 2014 at 10:58:35AM +0100, Daniel Vetter wrote: > On Thu, Jan 09, 2014 at 09:45:21AM +0000, Chris Wilson wrote: > > On Thu, Jan 09, 2014 at 11:00:17AM +0530, akash.goel@intel.com wrote: > > > From: Akash Goel <akash.goel@intel.com> > > > > > > The 'offset' field of the 'scatterlist' structure was wrongly > > > programmed with the offset value from the base of stolen area, > > > whereas this field indicates the offset from where the interested > > > data starts within the PAGE pointed to by the 'page-link' field. > > > As a result when a new GEM object allocated from the stolen > > > area is mapped to GTT, it could lead to an overwrite of GTT entries > > > as the page count calculation will go wrong, refer the function > > > 'sg_page_count'. > > > > This statement is incorrect since my use of sg here predates > > sg_page_iter. > > > > The stolen sg has no page_link, the meaning of offset/length here are > > relative to the base of the stolen area. > > > > However, if you wish to rephrase the above... > > Actually we add offset both to sg->offset and adjust the dma_bus_addr > since this has been introduced in > > commit 0104fdbb84d7adb0e377ed05bf75eba97b007544 > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Thu Nov 15 11:32:26 2012 +0000 > > drm/i915: Introduce i915_gem_object_create_stolen() > > But only Imre's conversion to the sg_page_iter started to pay any > attention to sg->offset in > > commit 6e995e231a90ce7c5ce2a9eae23c8e22f4388db1 > Author: Imre Deak <imre.deak@intel.com> > Date: Mon Feb 18 19:28:04 2013 +0200 > > drm/i915: use for_each_sg_page for setting up the gtt ptes > > So with a bit of commit message rewording and these references and cc: > stable this looks good. And Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=69104 Tested-by: Chris Wilson <chris@chris-wilson.co.uk> Maybe a reviewed-by when I see a good comment/changelog. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index fed87ec..1a24e84 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -250,7 +250,7 @@ i915_pages_create_for_stolen(struct drm_device *dev, } sg = st->sgl; - sg->offset = offset; + sg->offset = 0; sg->length = size; sg_dma_address(sg) = (dma_addr_t)dev_priv->mm.stolen_base + offset;