[PATCH 1/7] drm/i915: Fix the offset issue for the stolen GEM objects

[PATCH 1/7] drm/i915: Fix the offset issue for the stolen GEM objects

[akash.goel]

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 first PAGE pointed to by 'scattterlist'
structure. As a result when a new GEM object allocated from 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'.

v2: Modified the commit message. (Chris)

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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;
-- 
1.8.5.2

Re: [PATCH 1/7] drm/i915: Fix the offset issue for the stolen GEM objects

[Jesse Barnes]

On Mon, 13 Jan 2014 16:24:45 +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 first PAGE pointed to by 'scattterlist'
> structure. As a result when a new GEM object allocated from 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'.
> 
> v2: Modified the commit message. (Chris)
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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;

Let's get this upstream and cc stable.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/7] drm/i915: Fix the offset issue for the stolen GEM objects

[Daniel Vetter]

On Mon, Jan 27, 2014 at 04:30:41PM -0800, Jesse Barnes wrote:
> On Mon, 13 Jan 2014 16:24:45 +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 first PAGE pointed to by 'scattterlist'
> > structure. As a result when a new GEM object allocated from 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'.
> > 
> > v2: Modified the commit message. (Chris)
> > 
> > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 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;
> 
> Let's get this upstream and cc stable.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Indeed this slipped through the cracks. Patch authors: Please poke me per
mail (cc mailing lists still) or on irc if a patch seems to linger for 1-2
weeks. Merged to -fixes, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 1/7] drm/i915: Fix the offset issue for the stolen GEM objects

[akash.goel]

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'.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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;
-- 
1.8.5.2

Re: [PATCH 1/7] drm/i915: Fix the offset issue for the stolen GEM objects

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/7] drm/i915: Fix the offset issue for the stolen GEM objects

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/7] drm/i915: Fix the offset issue for the stolen GEM objects

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre