[PATCH 0/3] kill /dev/agpgart, v2

[PATCH 0/3] kill /dev/agpgart, v2

[Daniel Vetter]

Hi all,

Well, only for gen6+, and now with Dave's suggestion implemented to tell drm
core that we don't need agp. It's ugly and I really prefer if we'd kill the drm
agp midlayer stuff instead. But this way we can go forward with moving gen6+ gtt
stuff to drm/i915 and implementing some neat new cool things.

As usual, flames&bikesheds highly welcome.

Cheers, Daniel

Daniel Vetter (3):
  drm/i915: don't use dev->agp
  drm/i915: disable drm agp support for !gen3 with kms enabled
  agp/intel-agp: remove snb+ host bridge pciids

 drivers/char/agp/intel-agp.c    |   11 -----------
 drivers/gpu/drm/i915/i915_dma.c |    2 +-
 drivers/gpu/drm/i915/i915_drv.c |   18 ++++++++++++------
 3 files changed, 13 insertions(+), 18 deletions(-)

-- 
1.7.7.6

[PATCH 1/3] drm/i915: don't use dev->agp

[Daniel Vetter]

This single leftover use is due to a patch that went into 3.5 through
-fixes. With the fake agp stuff on demise, at least for gen6+ we can't
use this any more.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 9563ab8..2166519 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1414,7 +1414,7 @@ static void i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
 	if (!ap)
 		return;
 
-	ap->ranges[0].base = dev_priv->dev->agp->base;
+	ap->ranges[0].base = dev_priv->mm.gtt->gma_bus_addr;
 	ap->ranges[0].size =
 		dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
 	primary =
-- 
1.7.7.6

[PATCH 2/3] drm/i915: disable drm agp support for !gen3 with kms enabled

[Daniel Vetter]

This is the quick&dirty way Dave Airlie suggested to workaround the
midlayer drm agp brain-damange. Note that i915_probe is only called
when the driver has ksm enabled, so no need to check for that.

We also need to move the intel_agp_enabled check at the right place.
Note that the only thing this does is enforce the correct module load
order (by using a symbol from intel-agp.ko) to ensure that the fake
agp driver is ready before the drm core tries to set up the agp stuff.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a378c08..5223ac2 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -930,10 +930,12 @@ int i915_reset(struct drm_device *dev)
 	return 0;
 }
 
-
 static int __devinit
 i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
+	struct intel_device_info *intel_info =
+		(struct intel_device_info *) ent->driver_data;
+
 	/* Only bind to function 0 of the device. Early generations
 	 * used function 1 as a placeholder for multi-head. This causes
 	 * us confusion instead, especially on the systems where both
@@ -942,6 +944,15 @@ i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (PCI_FUNC(pdev->devfn))
 		return -ENODEV;
 
+	/* Only enable AGP support for gen 3. */
+	if (intel_info->gen != 3) {
+		driver.driver_features &=
+			~(DRIVER_USE_AGP | DRIVER_REQUIRE_AGP);
+	} else if (!intel_agp_enabled) {
+		DRM_ERROR("drm/i915 can't work without intel_agp module!\n");
+		return -ENODEV;
+	}
+
 	return drm_get_pci_dev(pdev, ent, &driver);
 }
 
@@ -1102,11 +1113,6 @@ static struct pci_driver i915_pci_driver = {
 
 static int __init i915_init(void)
 {
-	if (!intel_agp_enabled) {
-		DRM_ERROR("drm/i915 can't work without intel_agp module!\n");
-		return -ENODEV;
-	}
-
 	driver.num_ioctls = i915_max_ioctl;
 
 	/*
-- 
1.7.7.6

Re: [PATCH 2/3] drm/i915: disable drm agp support for !gen3 with kms enabled

[Eugeni Dodonov]

On 06/24/2012 03:51 PM, Daniel Vetter wrote:
> +	/* Only enable AGP support for gen 3. */
> +	if (intel_info->gen != 3) {
> +		driver.driver_features &=
> +			~(DRIVER_USE_AGP | DRIVER_REQUIRE_AGP);
> +	} else if (!intel_agp_enabled) {
> +		DRM_ERROR("drm/i915 can't work without intel_agp module!\n");
> +		return -ENODEV;
> +	}
> +

<bikeshed>
For the future generations of developers to touch gen3 (or something
older than gen3 :)), I'd suggest to expand the comment to explain why we
need it on gen3 and not on all the other gens. I haven't seen this
commented in any other agpgart killing patches, so perhaps this would be
the best place.
</bikeshed>

Eugeni

Re: [PATCH 2/3] drm/i915: disable drm agp support for !gen3 with kms enabled

[Daniel Vetter]

On Sun, Jun 24, 2012 at 07:57:26PM -0300, Eugeni Dodonov wrote:
> On 06/24/2012 03:51 PM, Daniel Vetter wrote:
> > +	/* Only enable AGP support for gen 3. */
> > +	if (intel_info->gen != 3) {
> > +		driver.driver_features &=
> > +			~(DRIVER_USE_AGP | DRIVER_REQUIRE_AGP);
> > +	} else if (!intel_agp_enabled) {
> > +		DRM_ERROR("drm/i915 can't work without intel_agp module!\n");
> > +		return -ENODEV;
> > +	}
> > +
> 
> <bikeshed>
> For the future generations of developers to touch gen3 (or something
> older than gen3 :)), I'd suggest to expand the comment to explain why we
> need it on gen3 and not on all the other gens. I haven't seen this
> commented in any other agpgart killing patches, so perhaps this would be
> the best place.
> </bikeshed>

Well, the older series that rips out the midlayer-ish drm agp
initialization had more justification - we need it to support dead-broken
XvMC on gen3, which is using the old drm add_map stuff to access bos, even
when kms is enabled (gasp!). I'll add this little blurb to the comment
when applying.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH] drm/i915: disable drm agp support for !gen3 with kms enabled

[Daniel Vetter]

This is the quick&dirty way Dave Airlie suggested to workaround the
midlayer drm agp brain-damange. Note that i915_probe is only called
when the driver has ksm enabled, so no need to check for that.

We also need to move the intel_agp_enabled check at the right place.
Note that the only thing this does is enforce the correct module load
order (by using a symbol from intel-agp.ko) to ensure that the fake
agp driver is ready before the drm core tries to set up the agp stuff.

v2: Add a comment to explain why gen3 needs all this legacy fake agp
stuff - we've shipped an XvMC library with a kms-enabled ddx that
requires it (but only on gen3).

v3: Make it clear that this is really a gen3 issue only in the
comment.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a378c08..79be879 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -930,10 +930,12 @@ int i915_reset(struct drm_device *dev)
 	return 0;
 }
 
-
 static int __devinit
 i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
+	struct intel_device_info *intel_info =
+		(struct intel_device_info *) ent->driver_data;
+
 	/* Only bind to function 0 of the device. Early generations
 	 * used function 1 as a placeholder for multi-head. This causes
 	 * us confusion instead, especially on the systems where both
@@ -942,6 +944,18 @@ i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (PCI_FUNC(pdev->devfn))
 		return -ENODEV;
 
+	/* We've managed to ship a kms-enabled ddx that shipped with an XvMC
+	 * implementation for gen3 (and only gen3) that used legacy drm maps
+	 * (gasp!) to share buffers between X and the client. Hence we need to
+	 * keep around the fake agp stuff for gen3, even when kms is enabled. */
+	if (intel_info->gen != 3) {
+		driver.driver_features &=
+			~(DRIVER_USE_AGP | DRIVER_REQUIRE_AGP);
+	} else if (!intel_agp_enabled) {
+		DRM_ERROR("drm/i915 can't work without intel_agp module!\n");
+		return -ENODEV;
+	}
+
 	return drm_get_pci_dev(pdev, ent, &driver);
 }
 
@@ -1102,11 +1116,6 @@ static struct pci_driver i915_pci_driver = {
 
 static int __init i915_init(void)
 {
-	if (!intel_agp_enabled) {
-		DRM_ERROR("drm/i915 can't work without intel_agp module!\n");
-		return -ENODEV;
-	}
-
 	driver.num_ioctls = i915_max_ioctl;
 
 	/*
-- 
1.7.7.6

Re: [PATCH] drm/i915: disable drm agp support for !gen3 with kms enabled

[Chris Wilson]

On Mon, 25 Jun 2012 15:58:49 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> This is the quick&dirty way Dave Airlie suggested to workaround the
> midlayer drm agp brain-damange. Note that i915_probe is only called
> when the driver has ksm enabled, so no need to check for that.
s/ksm/kms/
> 
> We also need to move the intel_agp_enabled check at the right place.
> Note that the only thing this does is enforce the correct module load
> order (by using a symbol from intel-agp.ko) to ensure that the fake
> agp driver is ready before the drm core tries to set up the agp stuff.
> 
> v2: Add a comment to explain why gen3 needs all this legacy fake agp
> stuff - we've shipped an XvMC library with a kms-enabled ddx that
> requires it (but only on gen3).
> 
> v3: Make it clear that this is really a gen3 issue only in the
> comment.
Make it clear that this is only a gen3 issue in the comment.
 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: disable drm agp support for !gen3 with kms enabled

[Daniel Vetter]

On Mon, Jun 25, 2012 at 04:18:32PM +0100, Chris Wilson wrote:
> On Mon, 25 Jun 2012 15:58:49 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > This is the quick&dirty way Dave Airlie suggested to workaround the
> > midlayer drm agp brain-damange. Note that i915_probe is only called
> > when the driver has ksm enabled, so no need to check for that.
> s/ksm/kms/
> > 
> > We also need to move the intel_agp_enabled check at the right place.
> > Note that the only thing this does is enforce the correct module load
> > order (by using a symbol from intel-agp.ko) to ensure that the fake
> > agp driver is ready before the drm core tries to set up the agp stuff.
> > 
> > v2: Add a comment to explain why gen3 needs all this legacy fake agp
> > stuff - we've shipped an XvMC library with a kms-enabled ddx that
> > requires it (but only on gen3).
> > 
> > v3: Make it clear that this is really a gen3 issue only in the
> > comment.
> Make it clear that this is only a gen3 issue in the comment.
>  
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

I've queued all three patches for -next (with the changelog fixed as
suggested), thanks for the review.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: disable drm agp support for !gen3 with kms enabled

[Eugeni Dodonov]

On 06/25/2012 10:58 AM, Daniel Vetter wrote:
> This is the quick&dirty way Dave Airlie suggested to workaround the
> midlayer drm agp brain-damange. Note that i915_probe is only called
> when the driver has ksm enabled, so no need to check for that.
> 
> We also need to move the intel_agp_enabled check at the right place.
> Note that the only thing this does is enforce the correct module load
> order (by using a symbol from intel-agp.ko) to ensure that the fake
> agp driver is ready before the drm core tries to set up the agp stuff.
> 
> v2: Add a comment to explain why gen3 needs all this legacy fake agp
> stuff - we've shipped an XvMC library with a kms-enabled ddx that
> requires it (but only on gen3).
> 
> v3: Make it clear that this is really a gen3 issue only in the
> comment.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

Eugeni

[PATCH 3/3] agp/intel-agp: remove snb+ host bridge pciids

[Daniel Vetter]

drm/i915 now takes care itself of setting up the gtt for
these chips.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/char/agp/intel-agp.c |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c
index 92622d4..b130df0 100644
--- a/drivers/char/agp/intel-agp.c
+++ b/drivers/char/agp/intel-agp.c
@@ -903,17 +903,6 @@ static struct pci_device_id agp_intel_pci_table[] = {
 	ID(PCI_DEVICE_ID_INTEL_IRONLAKE_M_HB),
 	ID(PCI_DEVICE_ID_INTEL_IRONLAKE_MA_HB),
 	ID(PCI_DEVICE_ID_INTEL_IRONLAKE_MC2_HB),
-	ID(PCI_DEVICE_ID_INTEL_SANDYBRIDGE_HB),
-	ID(PCI_DEVICE_ID_INTEL_SANDYBRIDGE_M_HB),
-	ID(PCI_DEVICE_ID_INTEL_SANDYBRIDGE_S_HB),
-	ID(PCI_DEVICE_ID_INTEL_IVYBRIDGE_HB),
-	ID(PCI_DEVICE_ID_INTEL_IVYBRIDGE_M_HB),
-	ID(PCI_DEVICE_ID_INTEL_IVYBRIDGE_S_HB),
-	ID(PCI_DEVICE_ID_INTEL_VALLEYVIEW_HB),
-	ID(PCI_DEVICE_ID_INTEL_HASWELL_HB),
-	ID(PCI_DEVICE_ID_INTEL_HASWELL_M_HB),
-	ID(PCI_DEVICE_ID_INTEL_HASWELL_S_HB),
-	ID(PCI_DEVICE_ID_INTEL_HASWELL_E_HB),
 	{ }
 };
 
-- 
1.7.7.6

Re: [PATCH 0/3] kill /dev/agpgart, v2

[Eugeni Dodonov]

On 06/24/2012 03:51 PM, Daniel Vetter wrote:
> Hi all,
> 
> Well, only for gen6+, and now with Dave's suggestion implemented to tell drm
> core that we don't need agp. It's ugly and I really prefer if we'd kill the drm
> agp midlayer stuff instead. But this way we can go forward with moving gen6+ gtt
> stuff to drm/i915 and implementing some neat new cool things.
> 
> As usual, flames&bikesheds highly welcome.

I cannot think on a less invasive way to do so, so, for the series:
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

With just one tiny bikeshed on patch2.

Eugeni

Re: [PATCH 0/3] kill /dev/agpgart, v2

[Chris Wilson]

On Sun, 24 Jun 2012 19:53:33 -0300, Eugeni Dodonov <eugeni.dodonov@linux.intel.com> wrote:
> On 06/24/2012 03:51 PM, Daniel Vetter wrote:
> > Hi all,
> > 
> > Well, only for gen6+, and now with Dave's suggestion implemented to tell drm
> > core that we don't need agp. It's ugly and I really prefer if we'd kill the drm
> > agp midlayer stuff instead. But this way we can go forward with moving gen6+ gtt
> > stuff to drm/i915 and implementing some neat new cool things.
> > 
> > As usual, flames&bikesheds highly welcome.
> 
> I cannot think on a less invasive way to do so, so, for the series:
> Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> 
> With just one tiny bikeshed on patch2.

Agreed, except that it is not just bikeshedding on patch too. Without
any hint as to why gen3 is special, both in the changelog and justifying
the code as a comment (remember the comments are to explain why!), the
patch is NAKed.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre