Messages in this thread | | | Date | Wed, 9 Aug 2006 15:21:21 -0400 | From | Dave Jones <> | Subject | Re: [PATCH] Add support for Intel i965G/Q GARTs. |
| |
On Wed, Aug 09, 2006 at 12:04:27PM -0700, Eric Anholt wrote:
> 0bc75aab93ee69dcf547ca55a8afcd1464dbfc95 > diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c > index 61ac380..c51b365 100644 > --- a/drivers/char/agp/intel-agp.c > +++ b/drivers/char/agp/intel-agp.c > @@ -8,8 +8,15 @@ > * > * Intel(R) 915G/915GM support added by Alan Hourihane > * <alanh@tungstengraphics.com>. > + * > + * Intel(R) 945G/945GM support added by Alan Hourihane > + * <alanh@tungstengraphics.com>. > + * > + * Intel(R) 946GZ/965Q/965G support added by Alan Hourihane > + * <alanh@tungstengraphics.com>. > */
I think we should just strip out this whole credit section. The attributions are stored in the SCM anyway, and this just seems to grow and grow.
> +#include <linux/version.h>
Unnecessary.
> +/* Should be moved to include/linux/pci_ids.h */ > +#define PCI_DEVICE_ID_INTEL_82946GZ_HB 0x2970 > +#define PCI_DEVICE_ID_INTEL_82946GZ_IG 0x2972 > +#define PCI_DEVICE_ID_INTEL_82965G_1_HB 0x2980 > +#define PCI_DEVICE_ID_INTEL_82965G_1_IG 0x2982 > +#define PCI_DEVICE_ID_INTEL_82965Q_HB 0x2990 > +#define PCI_DEVICE_ID_INTEL_82965Q_IG 0x2992 > +#define PCI_DEVICE_ID_INTEL_82965G_HB 0x29A0 > +#define PCI_DEVICE_ID_INTEL_82965G_IG 0x29A2
Actually if this is the only place they're used, this is the right place for them. Stuff should only go into linux/pci_ids.h if theres another driver that uses the same ID.
> +#define IS_I965 (agp_bridge->dev->device == PCI_DEVICE_ID_INTEL_82946GZ_HB || \ > + agp_bridge->dev->device == PCI_DEVICE_ID_INTEL_82965G_1_HB || \ > + agp_bridge->dev->device == PCI_DEVICE_ID_INTEL_82965Q_HB || \ > + agp_bridge->dev->device == PCI_DEVICE_ID_INTEL_82965G_HB) > +
How about just doing this once during module init, and setting a global 'is_i965' variable ?
> @@ -354,6 +379,7 @@ static struct aper_size_info_fixed intel > /* The 64M mode still requires a 128k gatt */ > {64, 16384, 5}, > {256, 65536, 6}, > + {512, 131072, 7}, > };
Broken indentation
> static struct _intel_i830_private { > @@ -377,7 +403,11 @@ static void intel_i830_init_gtt_entries( > /* We obtain the size of the GTT, which is also stored (for some > * reason) at the top of stolen memory. Then we add 4KB to that > * for the video BIOS popup, which is also stored in there. */ > - size = agp_bridge->driver->fetch_size() + 4; > + > + if (IS_I965) > + size = 512 + 4; > + else > + size = agp_bridge->driver->fetch_size() + 4;
ditto. (Use tabs, not spaces).
> @@ -736,7 +766,7 @@ static int intel_i915_remove_entries(str > static int intel_i915_fetch_size(void) > { > struct aper_size_info_fixed *values; > - u32 temp, offset; > + u32 temp, offset = 0; > > #define I915_256MB_ADDRESS_MASK (1<<27)
Unnecessary. We never read this before we write to it.
> +/* The intel i965 automatically initializes the agp aperture during POST. > ++ * Use the memory already set aside for in the GTT. > ++ */ > +static int intel_i965_create_gatt_table(struct agp_bridge_data *bridge) > +{ > + int page_order; > + struct aper_size_info_fixed *size; > + int num_entries; > + u32 temp; > + > + size = agp_bridge->current_size; > + page_order = size->page_order; > + num_entries = size->num_entries; > + agp_bridge->gatt_table_real = NULL; > + > + pci_read_config_dword(intel_i830_private.i830_dev, I915_MMADDR, &temp); > + > + temp &= 0xfff00000; > + intel_i830_private.gtt = ioremap((temp + (512 * 1024)) , 512 * 1024); > + > + if (!intel_i830_private.gtt) > + return -ENOMEM; > + > + > + intel_i830_private.registers = ioremap(temp,128 * 4096); > + if (!intel_i830_private.registers) > + return -ENOMEM; > + > + temp = readl(intel_i830_private.registers+I810_PGETBL_CTL) & 0xfffff000; > + global_cache_flush(); /* FIXME: ? */
After we spent quite a while cleaning up all the uses of this a few months back, it'd be a shame to add more fixme's related to this.
> static int intel_fetch_size(void) > { > @@ -1469,7 +1570,7 @@ static struct agp_bridge_driver intel_91 > .owner = THIS_MODULE, > .aperture_sizes = intel_i830_sizes, > .size_type = FIXED_APER_SIZE, > - .num_aperture_sizes = 3, > + .num_aperture_sizes = 4, > .needs_scratch_page = TRUE, > .configure = intel_i915_configure, > .fetch_size = intel_i915_fetch_size,
This is ok with all the other chipsets that use intel_915_driver ?
> @@ -1684,6 +1808,35 @@ static int __devinit agp_intel_probe(str > bridge->driver = &intel_845_driver; > name = "945GM"; > break; > + case PCI_DEVICE_ID_INTEL_82946GZ_HB: > + if (find_i830(PCI_DEVICE_ID_INTEL_82946GZ_IG)) > + bridge->driver = &intel_i965_driver; > + else > + bridge->driver = &intel_845_driver; > + name = "946GZ"; > + break; > + case PCI_DEVICE_ID_INTEL_82965G_1_HB: > + if (find_i830(PCI_DEVICE_ID_INTEL_82965G_1_IG)) > + bridge->driver = &intel_i965_driver; > + else > + bridge->driver = &intel_845_driver; > + name = "965G"; > + break; > + case PCI_DEVICE_ID_INTEL_82965Q_HB: > + if (find_i830(PCI_DEVICE_ID_INTEL_82965Q_IG)) > + bridge->driver = &intel_i965_driver; > + else > + bridge->driver = &intel_845_driver; > + name = "965Q"; > + break; > + case PCI_DEVICE_ID_INTEL_82965G_HB: > + if (find_i830(PCI_DEVICE_ID_INTEL_82965G_IG)) > + bridge->driver = &intel_i965_driver; > + else > + bridge->driver = &intel_845_driver; > + name = "965G"; > + break;
This switch just keeps getting more and more horrific. It'd be nice to have this converted to be somehow table-driven at some point.
> @@ -1825,6 +1978,10 @@ #define ID(x) \ > ID(PCI_DEVICE_ID_INTEL_82915GM_HB), > ID(PCI_DEVICE_ID_INTEL_82945G_HB), > ID(PCI_DEVICE_ID_INTEL_82945GM_HB), > + ID(PCI_DEVICE_ID_INTEL_82946GZ_HB), > + ID(PCI_DEVICE_ID_INTEL_82965G_1_HB), > + ID(PCI_DEVICE_ID_INTEL_82965Q_HB), > + ID(PCI_DEVICE_ID_INTEL_82965G_HB), > { }
Indendation again.
Dave
-- http://www.codemonkey.org.uk - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |