Hi Emil,
On 02/12/2014 11:18 PM, Emil Velikov wrote:
On 12/02/14 05:38, Alexandre Courbot wrote:
Upcoming mobile Kepler GPUs (such as GK20A) use the platform bus instead of PCI to which Nouveau is tightly dependent. This patch allows Nouveau to handle platform devices by:
- abstracting PCI-dependent functions that were typically used for resource querying and page mapping,
- introducing a nv_device_is_pci() function that allows to make PCI-dependent code conditional,
- providing a nouveau_drm_platform_probe() function that takes a GPU platform device to be probed.
Core code as well as engine/subdev drivers are updated wherever possible to make use of these functions. Some older drivers are too dependent on PCI to be properly updated, but all newer code on which future chips may depend should at least be runnable with platform devices.
Hi Alexandre
I've tried really hard to find something wrong with this patch but it seems that you have it polished very nicely.
Great!
There is one quite minor nit in-line, but I'm not fussed either way.
Signed-off-by: Alexandre Courbot acourbot@nvidia.com
Changes since v1:
Refactored nouveau_device_create_() to take an additional bus type argument instead of having two versions of it that duplicate code.
Fixed a typo when substituting pci_resource_* with nv_device_resource_*
Check whether devices are PCI in relevant functions instead of nouveau_drm_load().
drivers/gpu/drm/nouveau/core/engine/device/base.c | 83 ++++++++++++++++++++-- drivers/gpu/drm/nouveau/core/engine/falcon.c | 6 +- drivers/gpu/drm/nouveau/core/engine/fifo/base.c | 2 +- drivers/gpu/drm/nouveau/core/engine/graph/nv20.c | 2 +- drivers/gpu/drm/nouveau/core/engine/graph/nv40.c | 2 +- drivers/gpu/drm/nouveau/core/engine/graph/nvc0.c | 4 +- drivers/gpu/drm/nouveau/core/engine/xtensa.c | 2 +- drivers/gpu/drm/nouveau/core/include/core/device.h | 30 ++++++++ .../gpu/drm/nouveau/core/include/engine/device.h | 17 +++-- drivers/gpu/drm/nouveau/core/include/subdev/mc.h | 1 + drivers/gpu/drm/nouveau/core/os.h | 1 + drivers/gpu/drm/nouveau/core/subdev/bar/base.c | 4 +- drivers/gpu/drm/nouveau/core/subdev/bar/nv50.c | 4 +- drivers/gpu/drm/nouveau/core/subdev/bar/nvc0.c | 15 ++-- .../gpu/drm/nouveau/core/subdev/devinit/fbmem.h | 8 ++- drivers/gpu/drm/nouveau/core/subdev/devinit/nv04.c | 2 +- drivers/gpu/drm/nouveau/core/subdev/devinit/nv05.c | 2 +- drivers/gpu/drm/nouveau/core/subdev/devinit/nv10.c | 2 +- drivers/gpu/drm/nouveau/core/subdev/devinit/nv20.c | 2 +- drivers/gpu/drm/nouveau/core/subdev/fb/nv50.c | 9 +-- drivers/gpu/drm/nouveau/core/subdev/fb/nvc0.c | 9 +-- drivers/gpu/drm/nouveau/core/subdev/i2c/base.c | 2 +- drivers/gpu/drm/nouveau/core/subdev/instmem/nv40.c | 7 +- drivers/gpu/drm/nouveau/core/subdev/mc/base.c | 39 ++++++---- drivers/gpu/drm/nouveau/core/subdev/mxm/base.c | 2 +- drivers/gpu/drm/nouveau/nouveau_abi16.c | 13 +++- drivers/gpu/drm/nouveau/nouveau_agp.c | 2 +- drivers/gpu/drm/nouveau/nouveau_bios.c | 4 ++ drivers/gpu/drm/nouveau/nouveau_bo.c | 22 +++--- drivers/gpu/drm/nouveau/nouveau_chan.c | 2 +- drivers/gpu/drm/nouveau/nouveau_display.c | 3 +- drivers/gpu/drm/nouveau/nouveau_drm.c | 59 ++++++++++++--- drivers/gpu/drm/nouveau/nouveau_sysfs.c | 8 ++- drivers/gpu/drm/nouveau/nouveau_ttm.c | 31 ++++---- drivers/gpu/drm/nouveau/nouveau_vga.c | 5 ++ 35 files changed, 297 insertions(+), 109 deletions(-)
[snip]
diff --git a/drivers/gpu/drm/nouveau/core/subdev/mc/base.c b/drivers/gpu/drm/nouveau/core/subdev/mc/base.c index b4b9943773bc..572190c8363b 100644 --- a/drivers/gpu/drm/nouveau/core/subdev/mc/base.c +++ b/drivers/gpu/drm/nouveau/core/subdev/mc/base.c @@ -93,8 +93,8 @@ _nouveau_mc_dtor(struct nouveau_object *object) { struct nouveau_device *device = nv_device(object); struct nouveau_mc *pmc = (void *)object;
- free_irq(device->pdev->irq, pmc);
- if (pmc->use_msi)
- free_irq(pmc->irq, pmc);
- if (nv_device_is_pci(device) && pmc->use_msi)
You should be able to keep the conditional as is.
pci_disable_msi(device->pdev);nouveau_subdev_destroy(&pmc->base); } @@ -114,22 +114,25 @@ nouveau_mc_create_(struct nouveau_object *parent, struct nouveau_object *engine, if (ret) return ret;
- switch (device->pdev->device & 0x0ff0) {
- case 0x00f0:
- case 0x02e0:
/* BR02? NFI how these would be handled yet exactly */break;- default:
switch (device->chipset) {case 0xaa: break; /* reported broken, nv also disable it */default:pmc->use_msi = true;
if (nv_device_is_pci(device))
switch (device->pdev->device & 0x0ff0) {case 0x00f0:case 0x02e0:/* BR02? NFI how these would be handled yet exactly */ break;default:switch (device->chipset) {case 0xaa:/* reported broken, nv also disable it */break;default:pmc->use_msi = true;break;} }
pmc->use_msi = nouveau_boolopt(device->cfgopt, "NvMSI", pmc->use_msi);
As you explicitly disable msi on platform devices you can move the option parsing within the if (nv_device_is_pci()) block.
Yes, that's correct. Actually I think it would also make sense to move the next paragraph under the "if (nv_device_is_pci())" block as well, since it also deals with MSI.
This way you can drop the change in the following conditional and the similar one in _nouveau_mc_dtor.
- if (pmc->use_msi && oclass->msi_rearm) {
- if (nv_device_is_pci(device) && pmc->use_msi && oclass->msi_rearm) {
Will do that, rebase and post a v3.
Many thanks, and again, welcome to nouveau :-)
Thanks for the review and the welcome! :)
Alex.