| Message ID | 1453901881-26425-3-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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
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);
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(-)