[PATCH 1/3] acpi_video: fix leaking PCI references

[PATCH 1/3] acpi_video: fix leaking PCI references

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

Otherwise we keep a bogus pci reference to the GPU

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/acpi/video.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 9577b6f..66e8f73 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1745,6 +1745,7 @@ static int acpi_video_bus_remove(struct acpi_device *device, int type)
 
 static int __init intel_opregion_present(void)
 {
+	int i915 = 0;
 #if defined(CONFIG_DRM_I915) || defined(CONFIG_DRM_I915_MODULE)
 	struct pci_dev *dev = NULL;
 	u32 address;
@@ -1757,10 +1758,10 @@ static int __init intel_opregion_present(void)
 		pci_read_config_dword(dev, 0xfc, &address);
 		if (!address)
 			continue;
-		return 1;
+		i915 = 1;
 	}
 #endif
-	return 0;
+	return i915;
 }
 
 int acpi_video_register(void)

[PATCH 2/3] acpi_video: Intel video is not always i915

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

Handle the GMA500/600/36x0 cases. Also stop it poking at random registers on
the i740 cards that may be out there still.

This should also allow the legacy gma500 stub driver to go away as the ACPI
video layer will now do the right thing rather than assume all the world is
Gen graphics and need the gma500 stub as a workaround.

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/acpi/video.c |   54 +++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 66e8f73..b2ae7aa 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1743,25 +1743,63 @@ static int acpi_video_bus_remove(struct acpi_device *device, int type)
 	return 0;
 }
 
+static int __init is_gma_pvr(struct pci_dev *dev)
+{
+	/* Medfield */
+	if ((dev->device & 0xFFF8) == 0x0130)
+		return 1;
+	/* GMA36x0 */
+	if ((dev->device & 0xFFF8) == 0x0be0)
+		return 1;
+	/* GMA600 */
+	if ((dev->device & 0xFFF8) == 0x4100)
+		return 1;
+	/* GMA500 */
+	if ((dev->device & 0xFFFE) == 0x8108)
+		return 1;
+	/* E620 */
+	if (dev->device == 0x4108)
+		return 1;
+	return 0;
+}
+
+static int __init is_i740(struct pci_dev *dev)
+{
+	if (dev->device == 0x00D1)
+		return 1;
+	if (dev->device == 0x7000)
+		return 1;
+	return 0;
+}
+
 static int __init intel_opregion_present(void)
 {
-	int i915 = 0;
-#if defined(CONFIG_DRM_I915) || defined(CONFIG_DRM_I915_MODULE)
+	int opregion = 0;
 	struct pci_dev *dev = NULL;
-	u32 address;
 
 	for_each_pci_dev(dev) {
 		if ((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
 			continue;
 		if (dev->vendor != PCI_VENDOR_ID_INTEL)
 			continue;
-		pci_read_config_dword(dev, 0xfc, &address);
-		if (!address)
+		/* We don't want to poke around undefined i740 registers */
+		if (is_i740(dev))
 			continue;
-		i915 = 1;
-	}
+#if defined(CONFIG_DRM_GMA500) || defined(CONFIG_DRM_GMA500_MODULE)
+		if (is_gma_pvr(dev))
+			opregion = 1;
 #endif
-	return i915;
+#if defined(CONFIG_DRM_I915) || defined(CONFIG_DRM_I915_MODULE)
+		if (!is_gma_pvr(dev)) {
+			u32 address;
+			pci_read_config_dword(dev, 0xfc, &address);
+			if (!address)
+				continue;
+			opregion = 1;
+		}
+#endif
+	}
+	return opregion;
 }
 
 int acpi_video_register(void)

[PATCH 3/3] gma500: don't register the ACPI video bus

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

We are not yet ready for this and it makes a mess on some devices.

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/gpu/drm/gma500/psb_drv.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index d5a6eab..45bd0c7 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -350,7 +350,7 @@ static int psb_driver_load(struct drm_device *dev, unsigned long chipset)
 	PSB_WSGX32(0x30000000, PSB_CR_BIF_3D_REQ_BASE);
 
 /*	igd_opregion_init(&dev_priv->opregion_dev); */
-	acpi_video_register();
+/*	acpi_video_register(); */
 	if (dev_priv->lid_state)
 		psb_lid_timer_init(dev_priv);
 

Re: [PATCH 2/3] acpi_video: Intel video is not always i915

[Matthew Garrett]

On Tue, Apr 24, 2012 at 04:45:01PM +0100, Alan Cox wrote:
> From: Alan Cox <alan@linux.intel.com>
> 
> Handle the GMA500/600/36x0 cases. Also stop it poking at random registers on
> the i740 cards that may be out there still.

The PowerVR Intels I'd seen had the opregion address in the 0xfc 
register as well. Is this no longer true on the latest?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 2/3] acpi_video: Intel video is not always i915

[Matthew Garrett]

On Tue, Apr 24, 2012 at 11:31:17PM +0100, Alan Cox wrote:
> On Tue, 24 Apr 2012 22:02:18 +0100
> Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > The PowerVR Intels I'd seen had the opregion address in the 0xfc 
> > register as well. Is this no longer true on the latest?
> 
> PowerVR does - i740 never did.
> 
> The PowerVR 0xfc poking also doesn't currently work once the driver takes
> over because it isn't yet implementing the driver end of the weird ACPI
> messaging/event stuff.
> 
> Once it does the GMA500 will be able to do an ACPI video register, but we
> will still need the check to get the ifdeffery right for what drivers are
> compiled for the kernel.

Right now you seem to set opregion unconditionally on PVR, which seems 
to be equivalent to the 0xfc check that was there before - I can 
understand excluding i740, but the PVR check could be left with the gen 
hardware one?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 2/3] acpi_video: Intel video is not always i915

[Alan Cox]

On Tue, 24 Apr 2012 22:02:18 +0100
Matthew Garrett <mjg59@srcf.ucam.org> wrote:

> On Tue, Apr 24, 2012 at 04:45:01PM +0100, Alan Cox wrote:
> > From: Alan Cox <alan@linux.intel.com>
> > 
> > Handle the GMA500/600/36x0 cases. Also stop it poking at random registers on
> > the i740 cards that may be out there still.
> 
> The PowerVR Intels I'd seen had the opregion address in the 0xfc 
> register as well. Is this no longer true on the latest?

PowerVR does - i740 never did.

The PowerVR 0xfc poking also doesn't currently work once the driver takes
over because it isn't yet implementing the driver end of the weird ACPI
messaging/event stuff.

Once it does the GMA500 will be able to do an ACPI video register, but we
will still need the check to get the ifdeffery right for what drivers are
compiled for the kernel.

Alan

Re: [PATCH 2/3] acpi_video: Intel video is not always i915

[Alan Cox]

On Tue, 24 Apr 2012 23:30:22 +0100
Matthew Garrett <mjg59@srcf.ucam.org> wrote:

> On Tue, Apr 24, 2012 at 11:31:17PM +0100, Alan Cox wrote:
> > On Tue, 24 Apr 2012 22:02:18 +0100
> > Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > > The PowerVR Intels I'd seen had the opregion address in the 0xfc 
> > > register as well. Is this no longer true on the latest?
> > 
> > PowerVR does - i740 never did.
> > 
> > The PowerVR 0xfc poking also doesn't currently work once the driver takes
> > over because it isn't yet implementing the driver end of the weird ACPI
> > messaging/event stuff.
> > 
> > Once it does the GMA500 will be able to do an ACPI video register, but we
> > will still need the check to get the ifdeffery right for what drivers are
> > compiled for the kernel.
> 
> Right now you seem to set opregion unconditionally on PVR, which seems 
> to be equivalent to the 0xfc check that was there before - I can 
> understand excluding i740, but the PVR check could be left with the gen 
> hardware one?

Not really - they are two drivers. If you build with i915 and not GMA500
you need the opregion for one and the acpi fallback for the other, and
vice versa. So we have to check both CONFIG_xxx macro sets.

Right now GMA500 needs the ACPI video stuff never to be enabled on some
machines. Until we've got full opregion support in the driver that won't
change.

Alan

Re: [PATCH 2/3] acpi_video: Intel video is not always i915

[Matthew Garrett]

On Wed, Apr 25, 2012 at 11:27:11AM +0100, Alan Cox wrote:
> On Tue, 24 Apr 2012 23:30:22 +0100
> Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > Right now you seem to set opregion unconditionally on PVR, which seems 
> > to be equivalent to the 0xfc check that was there before - I can 
> > understand excluding i740, but the PVR check could be left with the gen 
> > hardware one?
> 
> Not really - they are two drivers. If you build with i915 and not GMA500
> you need the opregion for one and the acpi fallback for the other, and
> vice versa. So we have to check both CONFIG_xxx macro sets.
> 
> Right now GMA500 needs the ACPI video stuff never to be enabled on some
> machines. Until we've got full opregion support in the driver that won't
> change.

opregion always seems to be set to 1 if is_gma_pvr() is true, which 
means we'll now never bind the acpi video driver on PVR hardware. If 
that's what you want, why distinguish between PVR and GEN?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 2/3] acpi_video: Intel video is not always i915

[Alan Cox]

On Wed, 25 Apr 2012 12:31:52 +0100
Matthew Garrett <mjg59@srcf.ucam.org> wrote:

> On Wed, Apr 25, 2012 at 11:27:11AM +0100, Alan Cox wrote:
> > On Tue, 24 Apr 2012 23:30:22 +0100
> > Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > > Right now you seem to set opregion unconditionally on PVR, which seems 
> > > to be equivalent to the 0xfc check that was there before - I can 
> > > understand excluding i740, but the PVR check could be left with the gen 
> > > hardware one?
> > 
> > Not really - they are two drivers. If you build with i915 and not GMA500
> > you need the opregion for one and the acpi fallback for the other, and
> > vice versa. So we have to check both CONFIG_xxx macro sets.
> > 
> > Right now GMA500 needs the ACPI video stuff never to be enabled on some
> > machines. Until we've got full opregion support in the driver that won't
> > change.
> 
> opregion always seems to be set to 1 if is_gma_pvr() is true, which 
> means we'll now never bind the acpi video driver on PVR hardware. If 
> that's what you want, why distinguish between PVR and GEN?

Because as I said before there are two drivers involved and you may only
have one compiled into your kernel. If you have

CONFIG_DRM_GMA500=y
CONFIG_DRM_I915=n

you want the acpi stuff to autoregister even if an i915 is found, but not
if a GMA500 is found. Ditto the reverse.

Alan

Re: [PATCH 2/3] acpi_video: Intel video is not always i915

[Matthew Garrett]

On Wed, Apr 25, 2012 at 01:24:38PM +0100, Alan Cox wrote:

> Because as I said before there are two drivers involved and you may only
> have one compiled into your kernel. If you have
> 
> CONFIG_DRM_GMA500=y
> CONFIG_DRM_I915=n
> 
> you want the acpi stuff to autoregister even if an i915 is found, but not
> if a GMA500 is found. Ditto the reverse.

No you don't - there's various platforms that will hang if you do that. 
It's necessary to make sure that the DIDL fields are set up before 
calling any ACPI video functions on opregion hardware.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 2/3] acpi_video: Intel video is not always i915

[Matthew Garrett]

On Wed, Apr 25, 2012 at 01:49:40PM +0100, Alan Cox wrote:
> On Wed, 25 Apr 2012 13:40:18 +0100
> Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > No you don't - there's various platforms that will hang if you do that. 
> > It's necessary to make sure that the DIDL fields are set up before 
> > calling any ACPI video functions on opregion hardware.
> 
> Then the existing driver code is already broken, because it ifdefs out
> the check if i915 is not compiled for the kernel.

Yes, that does seem broken. I'm fine with just removing the #ifdefs and 
adding the i740 check.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 2/3] acpi_video: Intel video is not always i915

[Alan Cox]

On Wed, 25 Apr 2012 13:40:18 +0100
Matthew Garrett <mjg59@srcf.ucam.org> wrote:

> On Wed, Apr 25, 2012 at 01:24:38PM +0100, Alan Cox wrote:
> 
> > Because as I said before there are two drivers involved and you may only
> > have one compiled into your kernel. If you have
> > 
> > CONFIG_DRM_GMA500=y
> > CONFIG_DRM_I915=n
> > 
> > you want the acpi stuff to autoregister even if an i915 is found, but not
> > if a GMA500 is found. Ditto the reverse.
> 
> No you don't - there's various platforms that will hang if you do that. 
> It's necessary to make sure that the DIDL fields are set up before 
> calling any ACPI video functions on opregion hardware.

Then the existing driver code is already broken, because it ifdefs out
the check if i915 is not compiled for the kernel.

Alan

[PATCH 2/3] acpi_video: Intel video is not always i915

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

Stop it poking at random registers on the i740 cards that may be out there
still.

As per Matthew's feedback remove the conditional checks and never enable the
opregion handling unless an appropriate driver has been loaded.

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/acpi/video.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 66e8f73..609262d 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1743,10 +1743,18 @@ static int acpi_video_bus_remove(struct acpi_device *device, int type)
 	return 0;
 }
 
+static int __init is_i740(struct pci_dev *dev)
+{
+	if (dev->device == 0x00D1)
+		return 1;
+	if (dev->device == 0x7000)
+		return 1;
+	return 0;
+}
+
 static int __init intel_opregion_present(void)
 {
-	int i915 = 0;
-#if defined(CONFIG_DRM_I915) || defined(CONFIG_DRM_I915_MODULE)
+	int opregion = 0;
 	struct pci_dev *dev = NULL;
 	u32 address;
 
@@ -1755,13 +1763,15 @@ static int __init intel_opregion_present(void)
 			continue;
 		if (dev->vendor != PCI_VENDOR_ID_INTEL)
 			continue;
+		/* We don't want to poke around undefined i740 registers */
+		if (is_i740(dev))
+			continue;
 		pci_read_config_dword(dev, 0xfc, &address);
 		if (!address)
 			continue;
-		i915 = 1;
+		opregion = 1;
 	}
-#endif
-	return i915;
+	return opregion;
 }
 
 int acpi_video_register(void)