| Message ID | 20100701201925.GJ4837@outflux.net |
|---|---|
| State | Rejected |
| Headers | show |
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.
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
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
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
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.
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(-)