Re: kern/50060: kernel crash with i915drmksm on Intel 965Q

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

The following reply was made to PR kern/50060; it has been noted by GNATS.

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: gson%netbsd.org@localhost, nisimura%netbsd.org@localhost
Cc: gnats-bugs%NetBSD.org@localhost, gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost
Subject: Re: kern/50060: kernel crash with i915drmksm on Intel 965Q
Date: Mon, 12 Oct 2015 23:07:57 +0000

 This is a multi-part message in MIME format.
 --=_I2aphbwziqku9cDX0Sh0XLwUoEpS6aeG
 
 Here's a slightly better, if more ambitious, patch, which factors out
 the relevant logic and leaves a comment explaining what is going on
 here so it won't take me a whole day to figure this out next time it
 bites us.
 
 --=_I2aphbwziqku9cDX0Sh0XLwUoEpS6aeG
 Content-Type: text/plain; charset="ISO-8859-1"; name="agp1"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="agp1.patch"
 
 Index: agp_i810.c
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
 RCS file: /cvsroot/src/sys/dev/pci/agp_i810.c,v
 retrieving revision 1.118
 diff -p -u -r1.118 agp_i810.c
 --- agp_i810.c	5 Apr 2015 12:55:20 -0000	1.118
 +++ agp_i810.c	12 Oct 2015 23:06:42 -0000
 @@ -407,47 +407,91 @@ agp_i810_attach(device_t parent, device_
  	}
  	aprint_naive("\n");
 =20
 -	mmadr_type =3D PCI_MAPREG_TYPE_MEM;
 +	/* Discriminate on the chipset to choose the relevant BARs.  */
  	switch (isc->chiptype) {
  	case CHIP_I915:
  	case CHIP_G33:
  		apbase =3D AGP_I915_GMADR;
  		mmadr_bar =3D AGP_I915_MMADR;
 -		isc->size =3D 512*1024;
  		gtt_bar =3D AGP_I915_GTTADR;
  		gtt_off =3D ~(bus_size_t)0; /* XXXGCC */
  		break;
  	case CHIP_I965:
  		apbase =3D AGP_I965_GMADR;
  		mmadr_bar =3D AGP_I965_MMADR;
 -		mmadr_type |=3D PCI_MAPREG_MEM_TYPE_64BIT;
 -		if (pci_mapreg_info(isc->vga_pa.pa_pc, isc->vga_pa.pa_tag,
 -			AGP_I965_MMADR, mmadr_type, NULL, &isc->size, NULL))
 -			isc->size =3D 512*1024; /* XXX */
  		gtt_bar =3D 0;
  		gtt_off =3D AGP_I965_GTT;
  		break;
  	case CHIP_G4X:
  		apbase =3D AGP_I965_GMADR;
  		mmadr_bar =3D AGP_I965_MMADR;
 -		mmadr_type |=3D PCI_MAPREG_MEM_TYPE_64BIT;
 -		if (pci_mapreg_info(isc->vga_pa.pa_pc, isc->vga_pa.pa_tag,
 -			AGP_I965_MMADR, mmadr_type, NULL, &isc->size, NULL))
 -			isc->size =3D 512*1024; /* XXX */
  		gtt_bar =3D 0;
  		gtt_off =3D AGP_G4X_GTT;
  		break;
  	default:
  		apbase =3D AGP_I810_GMADR;
  		mmadr_bar =3D AGP_I810_MMADR;
 -		if (pci_mapreg_info(isc->vga_pa.pa_pc, isc->vga_pa.pa_tag,
 -			AGP_I810_MMADR, mmadr_type, NULL, &isc->size, NULL))
 -			isc->size =3D 512*1024; /* XXX */
  		gtt_bar =3D 0;
  		gtt_off =3D AGP_I810_GTT;
  		break;
  	}
 =20
 +	/*
 +	 * Ensure the MMIO BAR is, in fact, a memory BAR.
 +	 *
 +	 * XXX This is required because we use pa_memt below.  It is
 +	 * not a priori clear to me there is any other reason to
 +	 * require this.
 +	 */
 +	mmadr_type =3D pci_mapreg_type(isc->vga_pa.pa_pc, isc->vga_pa.pa_tag,
 +	    mmadr_bar);
 +	if ((mmadr_type & PCI_MAPREG_TYPE_MEM) !=3D PCI_MAPREG_TYPE_MEM) {
 +		aprint_error_dev(self, "non-memory device MMIO registers\n");
 +		error =3D ENXIO;
 +		goto fail1;
 +	}
 +
 +	/*
 +	 * Determine the size of the MMIO registers.
 +	 *
 +	 * XXX The size of the MMIO registers we use is statically
 +	 * determined, as a function of the chipset, by the driver's
 +	 * implementation.
 +	 *
 +	 * On some chipsets, the GTT is part of the MMIO register BAR.
 +	 * We would like to map the GTT separately, so that we can map
 +	 * it prefetchable, which we can't do with the MMIO registers.
 +	 * Consequently, we would especially like to map a fixed size
 +	 * of MMIO registers, not just whatever size the BAR says.
 +	 *
 +	 * However, old drm assumes that the combined GTT/MMIO register
 +	 * space is a single bus space mapping, so mapping them
 +	 * separately breaks that.  Once we rip out old drm, we can
 +	 * replace the pci_mapreg_info call by the chipset switch.
 +	 */
 +#if notyet
 +	switch (isc->chiptype) {
 +	case CHIP_I810:
 +	case CHIP_I830:
 +	case CHIP_I855:
 +	case CHIP_I915:
 +	case CHIP_G33:
 +	case CHIP_I965:
 +	case CHIP_G4X:
 +		isc->size =3D 512*1024;
 +		break;
 +	case CHIP_SANDYBRIDGE:
 +	case CHIP_IVYBRIDGE:
 +	case CHIP_HASWELL:
 +		isc->size =3D 2*1024*1024;
 +		break;
 +	}
 +#else
 +	if (pci_mapreg_info(isc->vga_pa.pa_pc, isc->vga_pa.pa_tag,
 +		mmadr_bar, mmadr_type, NULL, &isc->size, NULL))
 +		isc->size =3D 512*1024;
 +#endif	/* notyet */
 +
  	/* Map (or, rather, find the address and size of) the aperture.  */
  	if (isc->chiptype =3D=3D CHIP_I965 || isc->chiptype =3D=3D CHIP_G4X)
  		error =3D agp_i965_map_aperture(&isc->vga_pa, sc, apbase);
 
 --=_I2aphbwziqku9cDX0Sh0XLwUoEpS6aeG--