[PATCH 1/4] agp/intel-gtt: Don't leak the scratch page

[PATCH 1/4] agp/intel-gtt: Don't leak the scratch page

[Daniel Vetter]

Recently discovered by enabling CONFIG_DMA_API_DEBUG in our CI. By the
looks of it broken since forever.

v2: Don't forget to set the scratch page back to wb (Chris). Reuse
intel_gtt_teardown_scratch_page for that (and fix it up to treat
needs_dmar y/n correctly).

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93793
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/char/agp/intel-gtt.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
index 1341a94cc779..e657f989745e 100644
--- a/drivers/char/agp/intel-gtt.c
+++ b/drivers/char/agp/intel-gtt.c
@@ -555,8 +555,10 @@ static unsigned int intel_gtt_mappable_entries(void)
 static void intel_gtt_teardown_scratch_page(void)
 {
 	set_pages_wb(intel_private.scratch_page, 1);
-	pci_unmap_page(intel_private.pcidev, intel_private.scratch_page_dma,
-		       PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
+	if (intel_private.needs_dmar)
+		pci_unmap_page(intel_private.pcidev,
+			       intel_private.scratch_page_dma,
+			       PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
 	__free_page(intel_private.scratch_page);
 }
 
@@ -1430,6 +1432,8 @@ void intel_gmch_remove(void)
 	if (--intel_private.refcount)
 		return;
 
+	if (intel_private.scratch_page)
+		intel_gtt_teardown_scratch_page();
 	if (intel_private.pcidev)
 		pci_dev_put(intel_private.pcidev);
 	if (intel_private.bridge_dev)
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915: Stop depending upon CONFIG_AGP_INTEL

[Daniel Vetter]

The AGP_INTEL driver provides an interface for very old userspace to
control the GART (though the GART itself was only ever emulated on Intel
systems). The pci bridge discovery code is also used by the i915.ko
driver to set up the GTT on old systems, but it does not require the
old userspace interface. When i915.ko selects the old interface, it
binds another user to the core GTT routines, and in particular creates a
second reference to the scratch pages allocated. This hinders resource
leak debugging for when we unload i915.ko as we want to assert that all
DMA pages have been released, but we appear to leak because of the
secondary interface which persists after i915.ko unloads.

All i915.ko users do not require the old /dev/agpgart interface so stop
selecting it and simplify our debugging by dropping the historical
baggage.

Note that by selecting AGP=n it was already possible to unselect
AGP_INTEL. But since we've dropped support for any of the AGP stuff
long ago there's really no point for this any more.

Also note that we still need INTEL_GTT, which is the underlying,
share, driver for the graphics GART on gen1-5.

v2: Entirely new commit message (Chris, Ville).

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/Kconfig | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index fcd77b27514d..9aa7d2d98add 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -2,9 +2,7 @@ config DRM_I915
 	tristate "Intel 8xx/9xx/G3x/G4x/HD Graphics"
 	depends on DRM
 	depends on X86 && PCI
-	depends on (AGP || AGP=n)
 	select INTEL_GTT
-	select AGP_INTEL if AGP
 	select INTERVAL_TREE
 	# we need shmfs for the swappable backing store, and in particular
 	# the shmem_readpage() which depends upon tmpfs
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] agp/intel-gtt: Only register fake agp driver for gen1

[Daniel Vetter]

The fake agp driver for the intel graphics gart is only needed for ums
support. And we ditched that a long time ago:

commit 03dae59c72ffffd8ef6e005f48ba356c863e0587
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Wed Jul 23 16:27:25 2014 +0200

    drm/i915: Ditch UMS config option

With this there's no longer the problem that 2 drivers (fake agp
driver and the drm/i915 driver) fight over the same piece, which fixes
apparent dma leaks detected by CONFIG_DMA_API_DEBUG.

Note that the leak isn't real since intel-gtt refcounts and will tear
down eventually. But the debug code assumes that when the i915 driver
unbinds from the pci device everything should be gone. Which isn't the
case if we have intel-agp enabled - userspace might need it. But by
ditching this intel-gtt setup and teardown is completely tied to the
livetime of the "real" driver.

While at it untangle the init ordering a bit - the fake agp wouldn't
be initialized correctly if i915.ko loads first. Which isn't a problem
since when i915 loads in kms mode you won't need the fake agp support
needed by the ums driver ...

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93793
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/char/agp/intel-gtt.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
index e657f989745e..aef87fdbd187 100644
--- a/drivers/char/agp/intel-gtt.c
+++ b/drivers/char/agp/intel-gtt.c
@@ -1348,16 +1348,6 @@ int intel_gmch_probe(struct pci_dev *bridge_pdev, struct pci_dev *gpu_pdev,
 {
 	int i, mask;
 
-	/*
-	 * Can be called from the fake agp driver but also directly from
-	 * drm/i915.ko. Hence we need to check whether everything is set up
-	 * already.
-	 */
-	if (intel_private.driver) {
-		intel_private.refcount++;
-		return 1;
-	}
-
 	for (i = 0; intel_gtt_chipsets[i].name != NULL; i++) {
 		if (gpu_pdev) {
 			if (gpu_pdev->device ==
@@ -1378,16 +1368,26 @@ int intel_gmch_probe(struct pci_dev *bridge_pdev, struct pci_dev *gpu_pdev,
 	if (!intel_private.driver)
 		return 0;
 
-	intel_private.refcount++;
-
 #if IS_ENABLED(CONFIG_AGP_INTEL)
 	if (bridge) {
+		if (INTEL_GTT_GEN > 1)
+			return 0;
+
 		bridge->driver = &intel_fake_agp_driver;
 		bridge->dev_private_data = &intel_private;
 		bridge->dev = bridge_pdev;
 	}
 #endif
 
+
+	/*
+	 * Can be called from the fake agp driver but also directly from
+	 * drm/i915.ko. Hence we need to check whether everything is set up
+	 * already.
+	 */
+	if (intel_private.refcount++)
+		return 1;
+
 	intel_private.bridge_dev = pci_dev_get(bridge_pdev);
 
 	dev_info(&bridge_pdev->dev, "Intel %s Chipset\n", intel_gtt_chipsets[i].name);
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915: curb fifo underruns, somewhat

[Daniel Vetter]

So I have no real justification for this. It does seem to help
a bit though. It all kinda looks like a fifo underrun due to bad
watermarks, but the fifo underruns definitely start to happen
way before we enable the first plane.

And the 4 additional vblanks are also not really perfect in stopping
them, so it all seems fairly random.

The IIR clearing was just a side trail, assuming that somehow we
had old underruns stuck somewhere in the 2-deep queue. Didn't seem
to help.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93787
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c      | 8 ++++++++
 drivers/gpu/drm/i915/intel_display.c | 9 ++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 25a89373df63..bbc4d1b1dc67 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -236,6 +236,10 @@ void ilk_update_display_irq(struct drm_i915_private *dev_priv,
 		dev_priv->irq_mask = new_val;
 		I915_WRITE(DEIMR, dev_priv->irq_mask);
 		POSTING_READ(DEIMR);
+		I915_WRITE(DEIIR, dev_priv->irq_mask);
+		POSTING_READ(DEIIR);
+		I915_WRITE(DEIIR, dev_priv->irq_mask);
+		POSTING_READ(DEIIR);
 	}
 }
 
@@ -492,6 +496,10 @@ void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
 
 	I915_WRITE(SDEIMR, sdeimr);
 	POSTING_READ(SDEIMR);
+	I915_WRITE(SDEIIR, sdeimr);
+	POSTING_READ(SDEIIR);
+	I915_WRITE(SDEIIR, sdeimr);
+	POSTING_READ(SDEIIR);
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8104511ad302..068504011b73 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4181,7 +4181,7 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
 
 	intel_fdi_normal_train(crtc);
 
-	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
+	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, !IS_GEN5(dev));
 
 	/* For PCH DP, enable TRANS_DP_CTL */
 	if (HAS_PCH_CPT(dev) && intel_crtc->config->has_dp_encoder) {
@@ -4930,6 +4930,13 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	/* Must wait for vblank to avoid spurious PCH FIFO underruns */
 	if (intel_crtc->config->has_pch_encoder)
 		intel_wait_for_vblank(dev, pipe);
+	if (IS_GEN5(dev)) {
+		intel_wait_for_vblank(dev, pipe);
+		intel_wait_for_vblank(dev, pipe);
+		intel_wait_for_vblank(dev, pipe);
+		intel_wait_for_vblank(dev, pipe);
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
+	}
 	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
 
 	intel_fbc_enable(intel_crtc);
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Stop depending upon CONFIG_AGP_INTEL

[Chris Wilson]

On Wed, Jan 27, 2016 at 02:37:59PM +0100, Daniel Vetter wrote:
> The AGP_INTEL driver provides an interface for very old userspace to
> control the GART (though the GART itself was only ever emulated on Intel
> systems). The pci bridge discovery code is also used by the i915.ko
> driver to set up the GTT on old systems, but it does not require the
> old userspace interface. When i915.ko selects the old interface, it
> binds another user to the core GTT routines, and in particular creates a
> second reference to the scratch pages allocated. This hinders resource
> leak debugging for when we unload i915.ko as we want to assert that all
> DMA pages have been released, but we appear to leak because of the
> secondary interface which persists after i915.ko unloads.
> 
> All i915.ko users do not require the old /dev/agpgart interface so stop
> selecting it and simplify our debugging by dropping the historical
> baggage.
> 
> Note that by selecting AGP=n it was already possible to unselect
> AGP_INTEL. But since we've dropped support for any of the AGP stuff
> long ago there's really no point for this any more.
> 
> Also note that we still need INTEL_GTT, which is the underlying,
> share, driver for the graphics GART on gen1-5.
/share,/shared
 
> v2: Entirely new commit message (Chris, Ville).
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Stop depending upon CONFIG_AGP_INTEL

[Ville Syrjälä]

On Wed, Jan 27, 2016 at 02:37:59PM +0100, Daniel Vetter wrote:
> The AGP_INTEL driver provides an interface for very old userspace to
> control the GART (though the GART itself was only ever emulated on Intel
> systems). The pci bridge discovery code is also used by the i915.ko
> driver to set up the GTT on old systems, but it does not require the
> old userspace interface. When i915.ko selects the old interface, it
> binds another user to the core GTT routines, and in particular creates a
> second reference to the scratch pages allocated. This hinders resource
> leak debugging for when we unload i915.ko as we want to assert that all
> DMA pages have been released, but we appear to leak because of the
> secondary interface which persists after i915.ko unloads.
> 
> All i915.ko users do not require the old /dev/agpgart interface so stop
> selecting it and simplify our debugging by dropping the historical
> baggage.
> 
> Note that by selecting AGP=n it was already possible to unselect
> AGP_INTEL. But since we've dropped support for any of the AGP stuff
> long ago there's really no point for this any more.

Hmm. There surely was some kind of dependency between intel-agp and i915
which required the former to be builtin when i915 was, and that was the
reason for the kconfig magic IIRC. Not quite sure why I made i915 depend
on AGP and select INTEL_AGP though, instead of just
'depends on (INTEL_AGP || INTEL_AGP=n)'
but I guess there was some reason.

Anyways, after looking at git logs, I guess the dependency was
'extern int intel_agp_enabled' which got killed off in
commit 3bb6ce668663 ("drm/i915: Kill legeacy AGP for gen3 kms")
so perhaps note that in the commit message.

But yeah, with that stuff gone I think this should be fine, so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Also note that we still need INTEL_GTT, which is the underlying,
> share, driver for the graphics GART on gen1-5.
  ^^^^^

shared?

> 
> v2: Entirely new commit message (Chris, Ville).
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/Kconfig | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index fcd77b27514d..9aa7d2d98add 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -2,9 +2,7 @@ config DRM_I915
>  	tristate "Intel 8xx/9xx/G3x/G4x/HD Graphics"
>  	depends on DRM
>  	depends on X86 && PCI
> -	depends on (AGP || AGP=n)
>  	select INTEL_GTT
> -	select AGP_INTEL if AGP
>  	select INTERVAL_TREE
>  	# we need shmfs for the swappable backing store, and in particular
>  	# the shmem_readpage() which depends upon tmpfs
> -- 
> 2.5.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] agp/intel-gtt: Don't leak the scratch page

[Chris Wilson]

On Wed, Jan 27, 2016 at 02:37:58PM +0100, Daniel Vetter wrote:
> Recently discovered by enabling CONFIG_DMA_API_DEBUG in our CI. By the
> looks of it broken since forever.
> 
> v2: Don't forget to set the scratch page back to wb (Chris). Reuse
> intel_gtt_teardown_scratch_page for that (and fix it up to treat
> needs_dmar y/n correctly).
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93793
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

I wish I said that in my first review,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/4] agp/intel-gtt: Don't leak the scratch page

[Patchwork]

== Summary ==

Built on 430706bace599ea1a908b9a7c6b7ea17535fe17f drm-intel-nightly: 2016y-01m-27d-16h-33m-06s UTC integration manifest

Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b:
                dmesg-warn -> PASS       (ilk-hp8440p)

bdw-nuci7        total:141  pass:132  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:144  pass:138  dwarn:0   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:144  pass:120  dwarn:0   dfail:0   fail:0   skip:24 
byt-nuc          total:144  pass:129  dwarn:0   dfail:0   fail:0   skip:15 
hsw-brixbox      total:144  pass:137  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:144  pass:140  dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:144  pass:105  dwarn:0   dfail:0   fail:1   skip:38 
ivb-t430s        total:144  pass:138  dwarn:0   dfail:0   fail:0   skip:6  
skl-i5k-2        total:144  pass:135  dwarn:1   dfail:0   fail:0   skip:8  
snb-dellxps      total:144  pass:130  dwarn:0   dfail:0   fail:0   skip:14 
snb-x220t        total:144  pass:130  dwarn:0   dfail:0   fail:1   skip:13 

Results at /archive/results/CI_IGT_test/Patchwork_1274/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/4] agp/intel-gtt: Don't leak the scratch page

[Patchwork]

== Summary ==

Series 2876v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/2876/revisions/1/mbox/


bdw-nuci7        total:156  pass:147  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:159  pass:153  dwarn:0   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:159  pass:135  dwarn:0   dfail:0   fail:0   skip:24 
byt-nuc          total:159  pass:142  dwarn:0   dfail:0   fail:0   skip:17 
hsw-brixbox      total:159  pass:152  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:159  pass:155  dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:159  pass:114  dwarn:0   dfail:0   fail:1   skip:44 
ivb-t430s        total:159  pass:151  dwarn:0   dfail:0   fail:0   skip:8  
skl-i5k-2        total:159  pass:150  dwarn:1   dfail:0   fail:0   skip:8  
snb-dellxps      total:159  pass:141  dwarn:0   dfail:0   fail:0   skip:18 
snb-x220t        total:159  pass:141  dwarn:0   dfail:0   fail:1   skip:17 

Results at /archive/results/CI_IGT_test/Patchwork_1302/

b3f8ad64bc71f6236f05c2e9f4ad49a61745869a drm-intel-nightly: 2016y-01m-28d-10h-26m-23s UTC integration manifest
41383ed69f4953d40519e470885a320f94e88357 drm/i915: curb fifo underruns, somewhat
d6a53a565fb92bd19afdcccffc562d73ed1b61a7 agp/intel-gtt: Only register fake agp driver for gen1
e05a9063c5ad0cff2896913355798bbcd59cab1e drm/i915: Stop depending upon CONFIG_AGP_INTEL
5f503ba045a8db8d778849727b04295a0bde097e agp/intel-gtt: Don't leak the scratch page

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] agp/intel-gtt: Only register fake agp driver for gen1

[Daniel Vetter]

On Wed, Jan 27, 2016 at 02:38:00PM +0100, Daniel Vetter wrote:
> The fake agp driver for the intel graphics gart is only needed for ums
> support. And we ditched that a long time ago:
> 
> commit 03dae59c72ffffd8ef6e005f48ba356c863e0587
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Wed Jul 23 16:27:25 2014 +0200
> 
>     drm/i915: Ditch UMS config option
> 
> With this there's no longer the problem that 2 drivers (fake agp
> driver and the drm/i915 driver) fight over the same piece, which fixes
> apparent dma leaks detected by CONFIG_DMA_API_DEBUG.
> 
> Note that the leak isn't real since intel-gtt refcounts and will tear
> down eventually. But the debug code assumes that when the i915 driver
> unbinds from the pci device everything should be gone. Which isn't the
> case if we have intel-agp enabled - userspace might need it. But by
> ditching this intel-gtt setup and teardown is completely tied to the
> livetime of the "real" driver.
> 
> While at it untangle the init ordering a bit - the fake agp wouldn't
> be initialized correctly if i915.ko loads first. Which isn't a problem
> since when i915 loads in kms mode you won't need the fake agp support
> needed by the ums driver ...
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93793
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Merged patches 1&2 from this series. Anyone up for an r-b/ack on this one
here?

Thanks, Daniel

> ---
>  drivers/char/agp/intel-gtt.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index e657f989745e..aef87fdbd187 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -1348,16 +1348,6 @@ int intel_gmch_probe(struct pci_dev *bridge_pdev, struct pci_dev *gpu_pdev,
>  {
>  	int i, mask;
>  
> -	/*
> -	 * Can be called from the fake agp driver but also directly from
> -	 * drm/i915.ko. Hence we need to check whether everything is set up
> -	 * already.
> -	 */
> -	if (intel_private.driver) {
> -		intel_private.refcount++;
> -		return 1;
> -	}
> -
>  	for (i = 0; intel_gtt_chipsets[i].name != NULL; i++) {
>  		if (gpu_pdev) {
>  			if (gpu_pdev->device ==
> @@ -1378,16 +1368,26 @@ int intel_gmch_probe(struct pci_dev *bridge_pdev, struct pci_dev *gpu_pdev,
>  	if (!intel_private.driver)
>  		return 0;
>  
> -	intel_private.refcount++;
> -
>  #if IS_ENABLED(CONFIG_AGP_INTEL)
>  	if (bridge) {
> +		if (INTEL_GTT_GEN > 1)
> +			return 0;
> +
>  		bridge->driver = &intel_fake_agp_driver;
>  		bridge->dev_private_data = &intel_private;
>  		bridge->dev = bridge_pdev;
>  	}
>  #endif
>  
> +
> +	/*
> +	 * Can be called from the fake agp driver but also directly from
> +	 * drm/i915.ko. Hence we need to check whether everything is set up
> +	 * already.
> +	 */
> +	if (intel_private.refcount++)
> +		return 1;
> +
>  	intel_private.bridge_dev = pci_dev_get(bridge_pdev);
>  
>  	dev_info(&bridge_pdev->dev, "Intel %s Chipset\n", intel_gtt_chipsets[i].name);
> -- 
> 2.5.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] agp/intel-gtt: Only register fake agp driver for gen1

[Ville Syrjälä]

On Wed, Jan 27, 2016 at 02:38:00PM +0100, Daniel Vetter wrote:
> The fake agp driver for the intel graphics gart is only needed for ums
> support. And we ditched that a long time ago:
> 
> commit 03dae59c72ffffd8ef6e005f48ba356c863e0587
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Wed Jul 23 16:27:25 2014 +0200
> 
>     drm/i915: Ditch UMS config option
> 
> With this there's no longer the problem that 2 drivers (fake agp
> driver and the drm/i915 driver) fight over the same piece, which fixes
> apparent dma leaks detected by CONFIG_DMA_API_DEBUG.
> 
> Note that the leak isn't real since intel-gtt refcounts and will tear
> down eventually. But the debug code assumes that when the i915 driver
> unbinds from the pci device everything should be gone. Which isn't the
> case if we have intel-agp enabled - userspace might need it. But by
> ditching this intel-gtt setup and teardown is completely tied to the
> livetime of the "real" driver.
> 
> While at it untangle the init ordering a bit - the fake agp wouldn't
> be initialized correctly if i915.ko loads first. Which isn't a problem
> since when i915 loads in kms mode you won't need the fake agp support
> needed by the ums driver ...
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93793
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/char/agp/intel-gtt.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index e657f989745e..aef87fdbd187 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -1348,16 +1348,6 @@ int intel_gmch_probe(struct pci_dev *bridge_pdev, struct pci_dev *gpu_pdev,
>  {
>  	int i, mask;
>  
> -	/*
> -	 * Can be called from the fake agp driver but also directly from
> -	 * drm/i915.ko. Hence we need to check whether everything is set up
> -	 * already.
> -	 */
> -	if (intel_private.driver) {
> -		intel_private.refcount++;
> -		return 1;
> -	}
> -
>  	for (i = 0; intel_gtt_chipsets[i].name != NULL; i++) {
>  		if (gpu_pdev) {
>  			if (gpu_pdev->device ==
> @@ -1378,16 +1368,26 @@ int intel_gmch_probe(struct pci_dev *bridge_pdev, struct pci_dev *gpu_pdev,
>  	if (!intel_private.driver)
>  		return 0;
>  
> -	intel_private.refcount++;
> -
>  #if IS_ENABLED(CONFIG_AGP_INTEL)
>  	if (bridge) {
> +		if (INTEL_GTT_GEN > 1)
> +			return 0;

So if this happens first, we set up intel_private.driver but leave the
refcount at 0. Then i915 loads and we set up intel_private.driver again,
and bump the refcount to 0. Well, we should end up pointing
intel_privatee.driver at the same thing both times so not really a
problem I suppose.

So yeah, I think this ought to work
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I guess we could also trim the gen>=2 gmch pci ids from the agp
driver's pci id table, to avoid even probing it.

> +
>  		bridge->driver = &intel_fake_agp_driver;
>  		bridge->dev_private_data = &intel_private;
>  		bridge->dev = bridge_pdev;
>  	}
>  #endif
>  
> +
> +	/*
> +	 * Can be called from the fake agp driver but also directly from
> +	 * drm/i915.ko. Hence we need to check whether everything is set up
> +	 * already.
> +	 */
> +	if (intel_private.refcount++)
> +		return 1;
> +
>  	intel_private.bridge_dev = pci_dev_get(bridge_pdev);
>  
>  	dev_info(&bridge_pdev->dev, "Intel %s Chipset\n", intel_gtt_chipsets[i].name);
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] agp/intel-gtt: Only register fake agp driver for gen1

[Daniel Vetter]

On Thu, Feb 11, 2016 at 12:31:45PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 27, 2016 at 02:38:00PM +0100, Daniel Vetter wrote:
> > The fake agp driver for the intel graphics gart is only needed for ums
> > support. And we ditched that a long time ago:
> > 
> > commit 03dae59c72ffffd8ef6e005f48ba356c863e0587
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Wed Jul 23 16:27:25 2014 +0200
> > 
> >     drm/i915: Ditch UMS config option
> > 
> > With this there's no longer the problem that 2 drivers (fake agp
> > driver and the drm/i915 driver) fight over the same piece, which fixes
> > apparent dma leaks detected by CONFIG_DMA_API_DEBUG.
> > 
> > Note that the leak isn't real since intel-gtt refcounts and will tear
> > down eventually. But the debug code assumes that when the i915 driver
> > unbinds from the pci device everything should be gone. Which isn't the
> > case if we have intel-agp enabled - userspace might need it. But by
> > ditching this intel-gtt setup and teardown is completely tied to the
> > livetime of the "real" driver.
> > 
> > While at it untangle the init ordering a bit - the fake agp wouldn't
> > be initialized correctly if i915.ko loads first. Which isn't a problem
> > since when i915 loads in kms mode you won't need the fake agp support
> > needed by the ums driver ...
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93793
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/char/agp/intel-gtt.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> > index e657f989745e..aef87fdbd187 100644
> > --- a/drivers/char/agp/intel-gtt.c
> > +++ b/drivers/char/agp/intel-gtt.c
> > @@ -1348,16 +1348,6 @@ int intel_gmch_probe(struct pci_dev *bridge_pdev, struct pci_dev *gpu_pdev,
> >  {
> >  	int i, mask;
> >  
> > -	/*
> > -	 * Can be called from the fake agp driver but also directly from
> > -	 * drm/i915.ko. Hence we need to check whether everything is set up
> > -	 * already.
> > -	 */
> > -	if (intel_private.driver) {
> > -		intel_private.refcount++;
> > -		return 1;
> > -	}
> > -
> >  	for (i = 0; intel_gtt_chipsets[i].name != NULL; i++) {
> >  		if (gpu_pdev) {
> >  			if (gpu_pdev->device ==
> > @@ -1378,16 +1368,26 @@ int intel_gmch_probe(struct pci_dev *bridge_pdev, struct pci_dev *gpu_pdev,
> >  	if (!intel_private.driver)
> >  		return 0;
> >  
> > -	intel_private.refcount++;
> > -
> >  #if IS_ENABLED(CONFIG_AGP_INTEL)
> >  	if (bridge) {
> > +		if (INTEL_GTT_GEN > 1)
> > +			return 0;
> 
> So if this happens first, we set up intel_private.driver but leave the
> refcount at 0. Then i915 loads and we set up intel_private.driver again,
> and bump the refcount to 0. Well, we should end up pointing
> intel_privatee.driver at the same thing both times so not really a
> problem I suppose.
> 
> So yeah, I think this ought to work
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks for the review, applied.

> I guess we could also trim the gen>=2 gmch pci ids from the agp
> driver's pci id table, to avoid even probing it.

Hm yeah, forgot about that. I'll try to remember and do a follow-up patch.
-Daniel

> 
> > +
> >  		bridge->driver = &intel_fake_agp_driver;
> >  		bridge->dev_private_data = &intel_private;
> >  		bridge->dev = bridge_pdev;
> >  	}
> >  #endif
> >  
> > +
> > +	/*
> > +	 * Can be called from the fake agp driver but also directly from
> > +	 * drm/i915.ko. Hence we need to check whether everything is set up
> > +	 * already.
> > +	 */
> > +	if (intel_private.refcount++)
> > +		return 1;
> > +
> >  	intel_private.bridge_dev = pci_dev_get(bridge_pdev);
> >  
> >  	dev_info(&bridge_pdev->dev, "Intel %s Chipset\n", intel_gtt_chipsets[i].name);
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx