Patchwork UBUNTU: SAUCE: i915: ubreak G33 GTT size calculation

login
register
mail settings
Submitter Kees Cook
Date July 1, 2010, 8:19 p.m.
Message ID <20100701201925.GJ4837@outflux.net>
Download mbox | patch
Permalink /patch/57594/
State Rejected
Headers show

Comments

Kees Cook - July 1, 2010, 8:19 p.m.
Upstream fixed https://bugzilla.kernel.org/show_bug.cgi?id=15733
by correctly calculating the GTT size using aperture size
on non-G33 hardware.  In the process, the G33 case was
also changed, which lead to the regression documented in
https://bugzilla.kernel.org/show_bug.cgi?id=16294

This patch reverts the G33 logic without re-breaking the non-G33 logic.

Signed-off-by: Kees Cook <kees.cook@canonical.com>
---
 drivers/char/agp/intel-gtt.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
Stefan Bader - July 2, 2010, 6:50 a.m.
It is a patch. But what for? Clearly not Dapper but Lucid or Maverick? ;-)

-Stefan

On 07/01/2010 10:19 PM, Kees Cook wrote:
> Upstream fixed https://bugzilla.kernel.org/show_bug.cgi?id=15733
> by correctly calculating the GTT size using aperture size
> on non-G33 hardware.  In the process, the G33 case was
> also changed, which lead to the regression documented in
> https://bugzilla.kernel.org/show_bug.cgi?id=16294
> 
> This patch reverts the G33 logic without re-breaking the non-G33 logic.
> 
> Signed-off-by: Kees Cook <kees.cook@canonical.com>
> ---
>  drivers/char/agp/intel-gtt.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index 9344216..472d9f7 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -1229,6 +1229,13 @@ static int intel_i915_get_gtt_size(void)
>  				(gmch_ctrl & G33_PGETBL_SIZE_MASK));
>  			size = 512;
>  		}
> +		/* revert the G33 logic changes due to
> +		   https://bugzilla.kernel.org/show_bug.cgi?id=16294
> +		   without re-breaking the non-G33 case which motivated
> +		   the change from
> +		   https://bugzilla.kernel.org/show_bug.cgi?id=15733
> +		 */
> +		size = 1024;
>  	} else {
>  		/* On previous hardware, the GTT size was just what was
>  		 * required to map the aperture.
Andy Whitcroft - July 2, 2010, 9:15 a.m.
On Thu, Jul 01, 2010 at 01:19:25PM -0700, Kees Cook wrote:
> Upstream fixed https://bugzilla.kernel.org/show_bug.cgi?id=15733
> by correctly calculating the GTT size using aperture size
> on non-G33 hardware.  In the process, the G33 case was
> also changed, which lead to the regression documented in
> https://bugzilla.kernel.org/show_bug.cgi?id=16294
> 
> This patch reverts the G33 logic without re-breaking the non-G33 logic.
> 
> Signed-off-by: Kees Cook <kees.cook@canonical.com>
> ---
>  drivers/char/agp/intel-gtt.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index 9344216..472d9f7 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -1229,6 +1229,13 @@ static int intel_i915_get_gtt_size(void)
>  				(gmch_ctrl & G33_PGETBL_SIZE_MASK));
>  			size = 512;
>  		}
> +		/* revert the G33 logic changes due to
> +		   https://bugzilla.kernel.org/show_bug.cgi?id=16294
> +		   without re-breaking the non-G33 case which motivated
> +		   the change from
> +		   https://bugzilla.kernel.org/show_bug.cgi?id=15733
> +		 */
> +		size = 1024;

This seems to just slam the size to 1024 having spent some effort to
calculate it.  I assume therefore that this is not an upstream ready
patch?
>  	} else {
>  		/* On previous hardware, the GTT size was just what was
>  		 * required to map the aperture.

-apw
Kees Cook - July 2, 2010, 6:06 p.m.
Hi Stefan,

On Fri, Jul 02, 2010 at 08:50:18AM +0200, Stefan Bader wrote:
> It is a patch. But what for? Clearly not Dapper but Lucid or Maverick? ;-)

It's just for maverick.  It reverts a logical portion of
f1befe71fa7a79ab733011b045639d8d809924ad which is only in Maverick.

-Kees
Kees Cook - July 2, 2010, 6:08 p.m.
Hi Andy,

On Fri, Jul 02, 2010 at 10:15:57AM +0100, Andy Whitcroft wrote:
> On Thu, Jul 01, 2010 at 01:19:25PM -0700, Kees Cook wrote:
> > Upstream fixed https://bugzilla.kernel.org/show_bug.cgi?id=15733
> > by correctly calculating the GTT size using aperture size
> > on non-G33 hardware.  In the process, the G33 case was
> > also changed, which lead to the regression documented in
> > https://bugzilla.kernel.org/show_bug.cgi?id=16294
> > 
> > This patch reverts the G33 logic without re-breaking the non-G33 logic.
> > 
> > Signed-off-by: Kees Cook <kees.cook@canonical.com>
> > ---
> >  drivers/char/agp/intel-gtt.c |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> > index 9344216..472d9f7 100644
> > --- a/drivers/char/agp/intel-gtt.c
> > +++ b/drivers/char/agp/intel-gtt.c
> > @@ -1229,6 +1229,13 @@ static int intel_i915_get_gtt_size(void)
> >  				(gmch_ctrl & G33_PGETBL_SIZE_MASK));
> >  			size = 512;
> >  		}
> > +		/* revert the G33 logic changes due to
> > +		   https://bugzilla.kernel.org/show_bug.cgi?id=16294
> > +		   without re-breaking the non-G33 case which motivated
> > +		   the change from
> > +		   https://bugzilla.kernel.org/show_bug.cgi?id=15733
> > +		 */
> > +		size = 1024;
> 
> This seems to just slam the size to 1024 having spent some effort to
> calculate it.  I assume therefore that this is not an upstream ready
> patch?

Correct.  And my system (as well as any other G33 Intel system) is
unbootable without this correction.  Upstream basically said, "oh, yeah, I
guess that calculation isn't correct, we'll have to find someone who know
the hardware better."  In the meantime, I'm proposing this as a compromise
-- it restores the Lucid behavior (unconditionally size=1024) for G33
hardware, and retains the Maverick behavior (size=aperture) for non-G33
hardware.

-Kees

Patch

diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
index 9344216..472d9f7 100644
--- a/drivers/char/agp/intel-gtt.c
+++ b/drivers/char/agp/intel-gtt.c
@@ -1229,6 +1229,13 @@  static int intel_i915_get_gtt_size(void)
 				(gmch_ctrl & G33_PGETBL_SIZE_MASK));
 			size = 512;
 		}
+		/* revert the G33 logic changes due to
+		   https://bugzilla.kernel.org/show_bug.cgi?id=16294
+		   without re-breaking the non-G33 case which motivated
+		   the change from
+		   https://bugzilla.kernel.org/show_bug.cgi?id=15733
+		 */
+		size = 1024;
 	} else {
 		/* On previous hardware, the GTT size was just what was
 		 * required to map the aperture.