Re: [PATCH] Add support for Intel i965G/Q GARTs.

[Dave Jones <davej@redhat.com>]

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