* [PATCH] PCI: create revision file in sysfs @ 2016-11-01 15:42 Emil Velikov 2016-11-01 15:47 ` Alex Deucher ` (2 more replies) 0 siblings, 3 replies; 41+ messages in thread From: Emil Velikov @ 2016-11-01 15:42 UTC (permalink / raw) To: dri-devel Cc: emil.l.velikov, Jammy Zhou, Michel Dänzer, Bjorn Helgaas, linux-pci, linux-kernel From: Emil Velikov <emil.velikov@collabora.com> Currently the revision isn't available via sysfs/libudev thus if one wants to know the value they need to read through the config file. This in itself wakes/powers up the device, causing unwanted delay since it can be quite costly. Expose the revision as a separate file, just like we do for the device, vendor, their subsystem version and class. Cc: Jammy Zhou <Jammy.Zhou@amd.com> Cc: Michel Dänzer <michel.daenzer@amd.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: linux-pci@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Emil Velikov <emil.velikov@collabora.com> --- Gents, I'm not subscribed to the mailing list so please keep me in the CC chain. Thanks Emil --- drivers/pci/pci-sysfs.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index bcd10c7..0666287 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -50,6 +50,7 @@ pci_config_attr(vendor, "0x%04x\n"); pci_config_attr(device, "0x%04x\n"); pci_config_attr(subsystem_vendor, "0x%04x\n"); pci_config_attr(subsystem_device, "0x%04x\n"); +pci_config_attr(revision, "0x%02x\n"); pci_config_attr(class, "0x%06x\n"); pci_config_attr(irq, "%u\n"); @@ -568,6 +569,7 @@ static struct attribute *pci_dev_attrs[] = { &dev_attr_device.attr, &dev_attr_subsystem_vendor.attr, &dev_attr_subsystem_device.attr, + &dev_attr_revision.attr, &dev_attr_class.attr, &dev_attr_irq.attr, &dev_attr_local_cpus.attr, -- 2.9.3 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH] PCI: create revision file in sysfs 2016-11-01 15:42 [PATCH] PCI: create revision file in sysfs Emil Velikov @ 2016-11-01 15:47 ` Alex Deucher 2016-11-09 16:56 ` Emil Velikov 2016-11-11 14:37 ` [PATCH v3] " Emil Velikov 2 siblings, 0 replies; 41+ messages in thread From: Alex Deucher @ 2016-11-01 15:47 UTC (permalink / raw) To: Emil Velikov Cc: Maling list - DRI developers, Jammy Zhou, Michel Dänzer, LKML, Linux PCI, Bjorn Helgaas On Tue, Nov 1, 2016 at 11:42 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > From: Emil Velikov <emil.velikov@collabora.com> > > Currently the revision isn't available via sysfs/libudev thus if one > wants to know the value they need to read through the config file. > > This in itself wakes/powers up the device, causing unwanted delay > since it can be quite costly. > > Expose the revision as a separate file, just like we do for the device, > vendor, their subsystem version and class. > > Cc: Jammy Zhou <Jammy.Zhou@amd.com> > Cc: Michel Dänzer <michel.daenzer@amd.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: linux-pci@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > --- > Gents, I'm not subscribed to the mailing list so please keep me in the > CC chain. > > Thanks > Emil > --- > drivers/pci/pci-sysfs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index bcd10c7..0666287 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -50,6 +50,7 @@ pci_config_attr(vendor, "0x%04x\n"); > pci_config_attr(device, "0x%04x\n"); > pci_config_attr(subsystem_vendor, "0x%04x\n"); > pci_config_attr(subsystem_device, "0x%04x\n"); > +pci_config_attr(revision, "0x%02x\n"); > pci_config_attr(class, "0x%06x\n"); > pci_config_attr(irq, "%u\n"); > > @@ -568,6 +569,7 @@ static struct attribute *pci_dev_attrs[] = { > &dev_attr_device.attr, > &dev_attr_subsystem_vendor.attr, > &dev_attr_subsystem_device.attr, > + &dev_attr_revision.attr, > &dev_attr_class.attr, > &dev_attr_irq.attr, > &dev_attr_local_cpus.attr, > -- > 2.9.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] PCI: create revision file in sysfs @ 2016-11-01 15:47 ` Alex Deucher 0 siblings, 0 replies; 41+ messages in thread From: Alex Deucher @ 2016-11-01 15:47 UTC (permalink / raw) To: Emil Velikov Cc: Maling list - DRI developers, Jammy Zhou, Michel Dänzer, LKML, Linux PCI, Bjorn Helgaas On Tue, Nov 1, 2016 at 11:42 AM, Emil Velikov <emil.l.velikov@gmail.com> wr= ote: > From: Emil Velikov <emil.velikov@collabora.com> > > Currently the revision isn't available via sysfs/libudev thus if one > wants to know the value they need to read through the config file. > > This in itself wakes/powers up the device, causing unwanted delay > since it can be quite costly. > > Expose the revision as a separate file, just like we do for the device, > vendor, their subsystem version and class. > > Cc: Jammy Zhou <Jammy.Zhou@amd.com> > Cc: Michel D=C3=A4nzer <michel.daenzer@amd.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: linux-pci@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > --- > Gents, I'm not subscribed to the mailing list so please keep me in the > CC chain. > > Thanks > Emil > --- > drivers/pci/pci-sysfs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index bcd10c7..0666287 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -50,6 +50,7 @@ pci_config_attr(vendor, "0x%04x\n"); > pci_config_attr(device, "0x%04x\n"); > pci_config_attr(subsystem_vendor, "0x%04x\n"); > pci_config_attr(subsystem_device, "0x%04x\n"); > +pci_config_attr(revision, "0x%02x\n"); > pci_config_attr(class, "0x%06x\n"); > pci_config_attr(irq, "%u\n"); > > @@ -568,6 +569,7 @@ static struct attribute *pci_dev_attrs[] =3D { > &dev_attr_device.attr, > &dev_attr_subsystem_vendor.attr, > &dev_attr_subsystem_device.attr, > + &dev_attr_revision.attr, > &dev_attr_class.attr, > &dev_attr_irq.attr, > &dev_attr_local_cpus.attr, > -- > 2.9.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] PCI: create revision file in sysfs 2016-11-01 15:47 ` Alex Deucher (?) @ 2016-11-08 11:27 ` Emil Velikov -1 siblings, 0 replies; 41+ messages in thread From: Emil Velikov @ 2016-11-08 11:27 UTC (permalink / raw) To: Alex Deucher Cc: Maling list - DRI developers, Michel Dänzer, LKML, Linux PCI, Bjorn Helgaas [Dropping Jammy since his email bounces] On 1 November 2016 at 15:47, Alex Deucher <alexdeucher@gmail.com> wrote: > On Tue, Nov 1, 2016 at 11:42 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >> From: Emil Velikov <emil.velikov@collabora.com> >> >> Currently the revision isn't available via sysfs/libudev thus if one >> wants to know the value they need to read through the config file. >> >> This in itself wakes/powers up the device, causing unwanted delay >> since it can be quite costly. >> >> Expose the revision as a separate file, just like we do for the device, >> vendor, their subsystem version and class. >> >> Cc: Jammy Zhou <Jammy.Zhou@amd.com> >> Cc: Michel Dänzer <michel.daenzer@amd.com> >> Cc: Bjorn Helgaas <bhelgaas@google.com> >> Cc: linux-pci@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > Thanks Alex. Gents, to elaborate a bit: When using the Mesa drivers alongside firefox [1] (since Mesa 13.0), glxinfo (Mesa 10.0) and others, all the GPUs* will be awaken, causing unwanted delays and increased power usage. >From the userspace POV we have two distinct users who require the revision file - libdrm and libpciaccess. * The latter would even wake up _all_ the devices located on a PCI bus ! Let me know if you'd like the above in the patch summary, meanwhile I'll poke and collect a few more ack/r-b/t-b. Thanks Emil [1] https://bugs.freedesktop.org/show_bug.cgi?id=98502 ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] PCI: create revision file in sysfs @ 2016-11-08 11:27 ` Emil Velikov 0 siblings, 0 replies; 41+ messages in thread From: Emil Velikov @ 2016-11-08 11:27 UTC (permalink / raw) To: Alex Deucher Cc: Bjorn Helgaas, Linux PCI, Michel Dänzer, LKML, Maling list - DRI developers [Dropping Jammy since his email bounces] On 1 November 2016 at 15:47, Alex Deucher <alexdeucher@gmail.com> wrote: > On Tue, Nov 1, 2016 at 11:42 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >> From: Emil Velikov <emil.velikov@collabora.com> >> >> Currently the revision isn't available via sysfs/libudev thus if one >> wants to know the value they need to read through the config file. >> >> This in itself wakes/powers up the device, causing unwanted delay >> since it can be quite costly. >> >> Expose the revision as a separate file, just like we do for the device, >> vendor, their subsystem version and class. >> >> Cc: Jammy Zhou <Jammy.Zhou@amd.com> >> Cc: Michel Dänzer <michel.daenzer@amd.com> >> Cc: Bjorn Helgaas <bhelgaas@google.com> >> Cc: linux-pci@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > Thanks Alex. Gents, to elaborate a bit: When using the Mesa drivers alongside firefox [1] (since Mesa 13.0), glxinfo (Mesa 10.0) and others, all the GPUs* will be awaken, causing unwanted delays and increased power usage. From the userspace POV we have two distinct users who require the revision file - libdrm and libpciaccess. * The latter would even wake up _all_ the devices located on a PCI bus ! Let me know if you'd like the above in the patch summary, meanwhile I'll poke and collect a few more ack/r-b/t-b. Thanks Emil [1] https://bugs.freedesktop.org/show_bug.cgi?id=98502 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] PCI: create revision file in sysfs @ 2016-11-08 11:27 ` Emil Velikov 0 siblings, 0 replies; 41+ messages in thread From: Emil Velikov @ 2016-11-08 11:27 UTC (permalink / raw) To: Alex Deucher Cc: Maling list - DRI developers, Michel Dänzer, LKML, Linux PCI, Bjorn Helgaas [Dropping Jammy since his email bounces] On 1 November 2016 at 15:47, Alex Deucher <alexdeucher@gmail.com> wrote: > On Tue, Nov 1, 2016 at 11:42 AM, Emil Velikov <emil.l.velikov@gmail.com> = wrote: >> From: Emil Velikov <emil.velikov@collabora.com> >> >> Currently the revision isn't available via sysfs/libudev thus if one >> wants to know the value they need to read through the config file. >> >> This in itself wakes/powers up the device, causing unwanted delay >> since it can be quite costly. >> >> Expose the revision as a separate file, just like we do for the device, >> vendor, their subsystem version and class. >> >> Cc: Jammy Zhou <Jammy.Zhou@amd.com> >> Cc: Michel D=C3=A4nzer <michel.daenzer@amd.com> >> Cc: Bjorn Helgaas <bhelgaas@google.com> >> Cc: linux-pci@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > Thanks Alex. Gents, to elaborate a bit: When using the Mesa drivers alongside firefox [1] (since Mesa 13.0), glxinfo (Mesa 10.0) and others, all the GPUs* will be awaken, causing unwanted delays and increased power usage. >From the userspace POV we have two distinct users who require the revision file - libdrm and libpciaccess. * The latter would even wake up _all_ the devices located on a PCI bus ! Let me know if you'd like the above in the patch summary, meanwhile I'll poke and collect a few more ack/r-b/t-b. Thanks Emil [1] https://bugs.freedesktop.org/show_bug.cgi?id=3D98502 ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2] PCI: create revision file in sysfs 2016-11-01 15:42 [PATCH] PCI: create revision file in sysfs Emil Velikov @ 2016-11-09 16:56 ` Emil Velikov 2016-11-09 16:56 ` Emil Velikov 2016-11-11 14:37 ` [PATCH v3] " Emil Velikov 2 siblings, 0 replies; 41+ messages in thread From: Emil Velikov @ 2016-11-09 16:56 UTC (permalink / raw) To: dri-devel; +Cc: emil.l.velikov, Bjorn Helgaas, linux-pci From: Emil Velikov <emil.velikov@collabora.com> Currently the revision isn't available via sysfs/libudev thus if one wants to know the value they need to read through the config file. This in itself wakes/powers up the device, causing unwanted delays. There are at least two userspace components which could make use the new file - libpciaccess and libdrm. At the moment the former will wake up _every_ PCI device for simple invocation of glxinfo [when using Mesa 10.0+ drivers]. While the latter [in association with Mesa 13.0] can lead to 2-3 second delays while starting firefox, thunderbird or chromium. Expose the revision as a separate file, just like we do for the device, vendor, their subsystem version and class. Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: linux-pci@vger.kernel.org Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 Tested-by: Mauro Santos <registo.mailling@gmail.com> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> Signed-off-by: Emil Velikov <emil.velikov@collabora.com> --- v2: - Add r-b/t-b tags - Slim down CC list - Add note about userspace. As before, please keep me in the CC list. Additionally if there's anything else I can do to get things going please let me know. Thanks Emil --- drivers/pci/pci-sysfs.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index bcd10c7..0666287 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -50,6 +50,7 @@ pci_config_attr(vendor, "0x%04x\n"); pci_config_attr(device, "0x%04x\n"); pci_config_attr(subsystem_vendor, "0x%04x\n"); pci_config_attr(subsystem_device, "0x%04x\n"); +pci_config_attr(revision, "0x%02x\n"); pci_config_attr(class, "0x%06x\n"); pci_config_attr(irq, "%u\n"); @@ -568,6 +569,7 @@ static struct attribute *pci_dev_attrs[] = { &dev_attr_device.attr, &dev_attr_subsystem_vendor.attr, &dev_attr_subsystem_device.attr, + &dev_attr_revision.attr, &dev_attr_class.attr, &dev_attr_irq.attr, &dev_attr_local_cpus.attr, -- 2.9.3 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v2] PCI: create revision file in sysfs @ 2016-11-09 16:56 ` Emil Velikov 0 siblings, 0 replies; 41+ messages in thread From: Emil Velikov @ 2016-11-09 16:56 UTC (permalink / raw) To: dri-devel; +Cc: Bjorn Helgaas, linux-pci, emil.l.velikov From: Emil Velikov <emil.velikov@collabora.com> Currently the revision isn't available via sysfs/libudev thus if one wants to know the value they need to read through the config file. This in itself wakes/powers up the device, causing unwanted delays. There are at least two userspace components which could make use the new file - libpciaccess and libdrm. At the moment the former will wake up _every_ PCI device for simple invocation of glxinfo [when using Mesa 10.0+ drivers]. While the latter [in association with Mesa 13.0] can lead to 2-3 second delays while starting firefox, thunderbird or chromium. Expose the revision as a separate file, just like we do for the device, vendor, their subsystem version and class. Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: linux-pci@vger.kernel.org Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 Tested-by: Mauro Santos <registo.mailling@gmail.com> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> Signed-off-by: Emil Velikov <emil.velikov@collabora.com> --- v2: - Add r-b/t-b tags - Slim down CC list - Add note about userspace. As before, please keep me in the CC list. Additionally if there's anything else I can do to get things going please let me know. Thanks Emil --- drivers/pci/pci-sysfs.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index bcd10c7..0666287 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -50,6 +50,7 @@ pci_config_attr(vendor, "0x%04x\n"); pci_config_attr(device, "0x%04x\n"); pci_config_attr(subsystem_vendor, "0x%04x\n"); pci_config_attr(subsystem_device, "0x%04x\n"); +pci_config_attr(revision, "0x%02x\n"); pci_config_attr(class, "0x%06x\n"); pci_config_attr(irq, "%u\n"); @@ -568,6 +569,7 @@ static struct attribute *pci_dev_attrs[] = { &dev_attr_device.attr, &dev_attr_subsystem_vendor.attr, &dev_attr_subsystem_device.attr, + &dev_attr_revision.attr, &dev_attr_class.attr, &dev_attr_irq.attr, &dev_attr_local_cpus.attr, -- 2.9.3 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v2] PCI: create revision file in sysfs 2016-11-09 16:56 ` Emil Velikov @ 2016-11-10 7:13 ` Greg KH -1 siblings, 0 replies; 41+ messages in thread From: Greg KH @ 2016-11-10 7:13 UTC (permalink / raw) To: Emil Velikov; +Cc: dri-devel, Bjorn Helgaas, linux-pci On Wed, Nov 09, 2016 at 04:56:07PM +0000, Emil Velikov wrote: > From: Emil Velikov <emil.velikov@collabora.com> > > Currently the revision isn't available via sysfs/libudev thus if one > wants to know the value they need to read through the config file. > > This in itself wakes/powers up the device, causing unwanted delays. > > There are at least two userspace components which could make use the new > file - libpciaccess and libdrm. At the moment the former will wake up > _every_ PCI device for simple invocation of glxinfo [when using Mesa > 10.0+ drivers]. While the latter [in association with Mesa 13.0] can > lead to 2-3 second delays while starting firefox, thunderbird or > chromium. > > Expose the revision as a separate file, just like we do for the device, > vendor, their subsystem version and class. > > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: linux-pci@vger.kernel.org > Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 > Tested-by: Mauro Santos <registo.mailling@gmail.com> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > --- > v2: > - Add r-b/t-b tags > - Slim down CC list > - Add note about userspace. > > As before, please keep me in the CC list. Additionally if there's > anything else I can do to get things going please let me know. > > Thanks > Emil > --- > drivers/pci/pci-sysfs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index bcd10c7..0666287 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -50,6 +50,7 @@ pci_config_attr(vendor, "0x%04x\n"); > pci_config_attr(device, "0x%04x\n"); > pci_config_attr(subsystem_vendor, "0x%04x\n"); > pci_config_attr(subsystem_device, "0x%04x\n"); > +pci_config_attr(revision, "0x%02x\n"); > pci_config_attr(class, "0x%06x\n"); > pci_config_attr(irq, "%u\n"); Shouldn't we get a Documentation/ABI/ update for this as well? thanks, greg k-h ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2] PCI: create revision file in sysfs @ 2016-11-10 7:13 ` Greg KH 0 siblings, 0 replies; 41+ messages in thread From: Greg KH @ 2016-11-10 7:13 UTC (permalink / raw) To: Emil Velikov; +Cc: Bjorn Helgaas, linux-pci, dri-devel On Wed, Nov 09, 2016 at 04:56:07PM +0000, Emil Velikov wrote: > From: Emil Velikov <emil.velikov@collabora.com> > > Currently the revision isn't available via sysfs/libudev thus if one > wants to know the value they need to read through the config file. > > This in itself wakes/powers up the device, causing unwanted delays. > > There are at least two userspace components which could make use the new > file - libpciaccess and libdrm. At the moment the former will wake up > _every_ PCI device for simple invocation of glxinfo [when using Mesa > 10.0+ drivers]. While the latter [in association with Mesa 13.0] can > lead to 2-3 second delays while starting firefox, thunderbird or > chromium. > > Expose the revision as a separate file, just like we do for the device, > vendor, their subsystem version and class. > > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: linux-pci@vger.kernel.org > Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 > Tested-by: Mauro Santos <registo.mailling@gmail.com> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > --- > v2: > - Add r-b/t-b tags > - Slim down CC list > - Add note about userspace. > > As before, please keep me in the CC list. Additionally if there's > anything else I can do to get things going please let me know. > > Thanks > Emil > --- > drivers/pci/pci-sysfs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index bcd10c7..0666287 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -50,6 +50,7 @@ pci_config_attr(vendor, "0x%04x\n"); > pci_config_attr(device, "0x%04x\n"); > pci_config_attr(subsystem_vendor, "0x%04x\n"); > pci_config_attr(subsystem_device, "0x%04x\n"); > +pci_config_attr(revision, "0x%02x\n"); > pci_config_attr(class, "0x%06x\n"); > pci_config_attr(irq, "%u\n"); Shouldn't we get a Documentation/ABI/ update for this as well? thanks, greg k-h _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2] PCI: create revision file in sysfs 2016-11-10 7:13 ` Greg KH (?) @ 2016-11-10 13:14 ` Emil Velikov 2016-11-10 23:59 ` Bjorn Helgaas -1 siblings, 1 reply; 41+ messages in thread From: Emil Velikov @ 2016-11-10 13:14 UTC (permalink / raw) To: Greg KH; +Cc: ML dri-devel, Bjorn Helgaas, Linux PCI On 10 November 2016 at 07:13, Greg KH <gregkh@linuxfoundation.org> wrote: > On Wed, Nov 09, 2016 at 04:56:07PM +0000, Emil Velikov wrote: >> From: Emil Velikov <emil.velikov@collabora.com> >> >> Currently the revision isn't available via sysfs/libudev thus if one >> wants to know the value they need to read through the config file. >> >> This in itself wakes/powers up the device, causing unwanted delays. >> >> There are at least two userspace components which could make use the new >> file - libpciaccess and libdrm. At the moment the former will wake up >> _every_ PCI device for simple invocation of glxinfo [when using Mesa >> 10.0+ drivers]. While the latter [in association with Mesa 13.0] can >> lead to 2-3 second delays while starting firefox, thunderbird or >> chromium. >> >> Expose the revision as a separate file, just like we do for the device, >> vendor, their subsystem version and class. >> >> Cc: Bjorn Helgaas <bhelgaas@google.com> >> Cc: linux-pci@vger.kernel.org >> Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 >> Tested-by: Mauro Santos <registo.mailling@gmail.com> >> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> >> Signed-off-by: Emil Velikov <emil.velikov@collabora.com> >> --- >> v2: >> - Add r-b/t-b tags >> - Slim down CC list >> - Add note about userspace. >> >> As before, please keep me in the CC list. Additionally if there's >> anything else I can do to get things going please let me know. >> >> Thanks >> Emil >> --- >> drivers/pci/pci-sysfs.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c >> index bcd10c7..0666287 100644 >> --- a/drivers/pci/pci-sysfs.c >> +++ b/drivers/pci/pci-sysfs.c >> @@ -50,6 +50,7 @@ pci_config_attr(vendor, "0x%04x\n"); >> pci_config_attr(device, "0x%04x\n"); >> pci_config_attr(subsystem_vendor, "0x%04x\n"); >> pci_config_attr(subsystem_device, "0x%04x\n"); >> +pci_config_attr(revision, "0x%02x\n"); >> pci_config_attr(class, "0x%06x\n"); >> pci_config_attr(irq, "%u\n"); > > Shouldn't we get a Documentation/ABI/ update for this as well? > Definitely, we should. I've updated Documentation/filesystems/sysfs-pci.txt [locally] yet looking through ABI/ there is only a 'testing' one - Documentation/ABI/testing/sysfs-bus-pci. Feels a bit strange there is no stable one, guess I should/could start one ? Thanks Emil ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2] PCI: create revision file in sysfs 2016-11-10 13:14 ` Emil Velikov @ 2016-11-10 23:59 ` Bjorn Helgaas 2016-11-11 0:31 ` Emil Velikov 2016-11-14 3:35 ` Michel Dänzer 0 siblings, 2 replies; 41+ messages in thread From: Bjorn Helgaas @ 2016-11-10 23:59 UTC (permalink / raw) To: Emil Velikov; +Cc: Greg KH, ML dri-devel, Bjorn Helgaas, Linux PCI Hi Emil, On Thu, Nov 10, 2016 at 01:14:35PM +0000, Emil Velikov wrote: > On 10 November 2016 at 07:13, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Wed, Nov 09, 2016 at 04:56:07PM +0000, Emil Velikov wrote: > >> From: Emil Velikov <emil.velikov@collabora.com> > >> > >> Currently the revision isn't available via sysfs/libudev thus if one > >> wants to know the value they need to read through the config file. > >> > >> This in itself wakes/powers up the device, causing unwanted delays. > >> > >> There are at least two userspace components which could make use the new > >> file - libpciaccess and libdrm. At the moment the former will wake up > >> _every_ PCI device for simple invocation of glxinfo [when using Mesa > >> 10.0+ drivers]. While the latter [in association with Mesa 13.0] can > >> lead to 2-3 second delays while starting firefox, thunderbird or > >> chromium. I agree, these unwanted delays are completely unacceptable. My question is whether we should fix them by exporting more information from the kernel, or by changing the way the userspace components work. It should not take anywhere near 2 seconds to wake up a PCI device. That makes me think there's a more serious problem than just a lack of caching for the revision field, e.g., maybe we're looking at far more PCI devices than we need to, or we're doing it many times to the same device, or ... If I understand correctly, the delay was bisected to https://cgit.freedesktop.org/mesa/mesa/commit/?id=be239326aa4f, which removed a bunch of code that looked up the vendor and device IDs, and replaced it with drmGetDevice(). And apparently drmGetDevice(), in this path: drmGetDevice drmProcessPciDevice drmParsePciDeviceInfo is a little more thorough in that it looks up the *revision* in addition to the vendor and device IDs. So we pay the cost for the revision even though in this instance we don't care about the revision at all. drmParsePciDeviceInfo() currently reads the whole config header from sysfs (https://cgit.freedesktop.org/drm/libdrm/tree/xf86drm.c#n2949), but I think you're extending that to try the vendor, device, subsystem_vendor, subsystem_device, and (if present) revision sysfs files first (http://www.spinics.net/lists/dri-devel/msg122319.html). Bottom line, I guess I'm not super opposed to this, but I do feel like we're making a kernel change to cover up a userspace problem, and I think it would be better to push on that userspace problem a little more. > >> Expose the revision as a separate file, just like we do for the device, > >> vendor, their subsystem version and class. > >> > >> Cc: Bjorn Helgaas <bhelgaas@google.com> > >> Cc: linux-pci@vger.kernel.org > >> Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 > >> Tested-by: Mauro Santos <registo.mailling@gmail.com> > >> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > >> Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > >> --- > >> v2: > >> - Add r-b/t-b tags > >> - Slim down CC list > >> - Add note about userspace. > >> > >> As before, please keep me in the CC list. Additionally if there's > >> anything else I can do to get things going please let me know. > >> > >> Thanks > >> Emil > >> --- > >> drivers/pci/pci-sysfs.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > >> index bcd10c7..0666287 100644 > >> --- a/drivers/pci/pci-sysfs.c > >> +++ b/drivers/pci/pci-sysfs.c > >> @@ -50,6 +50,7 @@ pci_config_attr(vendor, "0x%04x\n"); > >> pci_config_attr(device, "0x%04x\n"); > >> pci_config_attr(subsystem_vendor, "0x%04x\n"); > >> pci_config_attr(subsystem_device, "0x%04x\n"); > >> +pci_config_attr(revision, "0x%02x\n"); > >> pci_config_attr(class, "0x%06x\n"); > >> pci_config_attr(irq, "%u\n"); > > > > Shouldn't we get a Documentation/ABI/ update for this as well? > > > Definitely, we should. > > I've updated Documentation/filesystems/sysfs-pci.txt [locally] yet > looking through ABI/ there is only a 'testing' one - > Documentation/ABI/testing/sysfs-bus-pci. > > Feels a bit strange there is no stable one, guess I should/could start one ? I wouldn't jump to the conclusion that this new ABI is "stable" when all the existing ones are only "testing". I'd just leave it in testing along with all the others. Bjorn ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2] PCI: create revision file in sysfs 2016-11-10 23:59 ` Bjorn Helgaas @ 2016-11-11 0:31 ` Emil Velikov 2016-11-11 14:49 ` Bjorn Helgaas 2016-11-14 3:35 ` Michel Dänzer 1 sibling, 1 reply; 41+ messages in thread From: Emil Velikov @ 2016-11-11 0:31 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: Greg KH, ML dri-devel, Bjorn Helgaas, Linux PCI On 10 November 2016 at 23:59, Bjorn Helgaas <helgaas@kernel.org> wrote: > Hi Emil, > > On Thu, Nov 10, 2016 at 01:14:35PM +0000, Emil Velikov wrote: >> On 10 November 2016 at 07:13, Greg KH <gregkh@linuxfoundation.org> wrote: >> > On Wed, Nov 09, 2016 at 04:56:07PM +0000, Emil Velikov wrote: >> >> From: Emil Velikov <emil.velikov@collabora.com> >> >> >> >> Currently the revision isn't available via sysfs/libudev thus if one >> >> wants to know the value they need to read through the config file. >> >> >> >> This in itself wakes/powers up the device, causing unwanted delays. >> >> >> >> There are at least two userspace components which could make use the new >> >> file - libpciaccess and libdrm. At the moment the former will wake up >> >> _every_ PCI device for simple invocation of glxinfo [when using Mesa >> >> 10.0+ drivers]. While the latter [in association with Mesa 13.0] can >> >> lead to 2-3 second delays while starting firefox, thunderbird or >> >> chromium. > > I agree, these unwanted delays are completely unacceptable. My > question is whether we should fix them by exporting more information > from the kernel, or by changing the way the userspace components work. > > It should not take anywhere near 2 seconds to wake up a PCI device. > That makes me think there's a more serious problem than just a lack of > caching for the revision field, e.g., maybe we're looking at far more > PCI devices than we need to, or we're doing it many times to the same > device, or ... > > If I understand correctly, the delay was bisected to > https://cgit.freedesktop.org/mesa/mesa/commit/?id=be239326aa4f, which > removed a bunch of code that looked up the vendor and device IDs, and > replaced it with drmGetDevice(). And apparently drmGetDevice(), in > this path: > > drmGetDevice > drmProcessPciDevice > drmParsePciDeviceInfo > > is a little more thorough in that it looks up the *revision* in > addition to the vendor and device IDs. So we pay the cost for the > revision even though in this instance we don't care about the revision > at all. > Above all, apologies for all the "lovely" code that you had to go through for these. And yes, you've got it spot on. > drmParsePciDeviceInfo() currently reads the whole config header from > sysfs (https://cgit.freedesktop.org/drm/libdrm/tree/xf86drm.c#n2949), > but I think you're extending that to try the vendor, device, > subsystem_vendor, subsystem_device, and (if present) revision sysfs > files first (http://www.spinics.net/lists/dri-devel/msg122319.html). > Yes, making the revision file optional and "faking it" was my first thought, esp. since we don't have any users of it (yet). Although people are not too keen on it, so we'll likely opt for revision-less API. > Bottom line, I guess I'm not super opposed to this, but I do feel like > we're making a kernel change to cover up a userspace problem, and I > think it would be better to push on that userspace problem a little > more. > Yes, definitely we can beat some sense into userspace. Yet that shouldn't be a deterrent for exposing the revision. As hinted before the other prominent user libpciaccess wakes up probes _every_ pci device. Atm that library is used by Xorg, Spice, libvirt and a few others. Amongst which are the Intel GL drivers (via libdrm_intel.so), [only] when GLX_MESA_query_renderer is used. Or in other words - if Firefox/other GL app wants to use the extension, they'll get similar delays. We should look into that one as well, but it will be more picky to address (read "slower to reach end users"). >> I've updated Documentation/filesystems/sysfs-pci.txt [locally] yet >> looking through ABI/ there is only a 'testing' one - >> Documentation/ABI/testing/sysfs-bus-pci. >> >> Feels a bit strange there is no stable one, guess I should/could start one ? > > I wouldn't jump to the conclusion that this new ABI is "stable" when > all the existing ones are only "testing". I'd just leave it in > testing along with all the others. > Agreed. Thank you ! Emil ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2] PCI: create revision file in sysfs 2016-11-11 0:31 ` Emil Velikov @ 2016-11-11 14:49 ` Bjorn Helgaas 2016-11-11 18:56 ` Emil Velikov 0 siblings, 1 reply; 41+ messages in thread From: Bjorn Helgaas @ 2016-11-11 14:49 UTC (permalink / raw) To: Emil Velikov; +Cc: Greg KH, ML dri-devel, Bjorn Helgaas, Linux PCI On Fri, Nov 11, 2016 at 12:31:47AM +0000, Emil Velikov wrote: > On 10 November 2016 at 23:59, Bjorn Helgaas <helgaas@kernel.org> wrote: > > Hi Emil, > > > > On Thu, Nov 10, 2016 at 01:14:35PM +0000, Emil Velikov wrote: > >> On 10 November 2016 at 07:13, Greg KH <gregkh@linuxfoundation.org> wrote: > >> > On Wed, Nov 09, 2016 at 04:56:07PM +0000, Emil Velikov wrote: > >> >> From: Emil Velikov <emil.velikov@collabora.com> > >> >> > >> >> Currently the revision isn't available via sysfs/libudev thus if one > >> >> wants to know the value they need to read through the config file. > >> >> > >> >> This in itself wakes/powers up the device, causing unwanted delays. > >> >> > >> >> There are at least two userspace components which could make use the new > >> >> file - libpciaccess and libdrm. At the moment the former will wake up > >> >> _every_ PCI device for simple invocation of glxinfo [when using Mesa > >> >> 10.0+ drivers]. While the latter [in association with Mesa 13.0] can > >> >> lead to 2-3 second delays while starting firefox, thunderbird or > >> >> chromium. > > > > I agree, these unwanted delays are completely unacceptable. My > > question is whether we should fix them by exporting more information > > from the kernel, or by changing the way the userspace components work. > > > > It should not take anywhere near 2 seconds to wake up a PCI device. > > That makes me think there's a more serious problem than just a lack of > > caching for the revision field, e.g., maybe we're looking at far more > > PCI devices than we need to, or we're doing it many times to the same > > device, or ... > > > > If I understand correctly, the delay was bisected to > > https://cgit.freedesktop.org/mesa/mesa/commit/?id=be239326aa4f, which > > removed a bunch of code that looked up the vendor and device IDs, and > > replaced it with drmGetDevice(). And apparently drmGetDevice(), in > > this path: > > > > drmGetDevice > > drmProcessPciDevice > > drmParsePciDeviceInfo > > > > is a little more thorough in that it looks up the *revision* in > > addition to the vendor and device IDs. So we pay the cost for the > > revision even though in this instance we don't care about the revision > > at all. > > > Above all, apologies for all the "lovely" code that you had to go > through for these. > And yes, you've got it spot on. > > > drmParsePciDeviceInfo() currently reads the whole config header from > > sysfs (https://cgit.freedesktop.org/drm/libdrm/tree/xf86drm.c#n2949), > > but I think you're extending that to try the vendor, device, > > subsystem_vendor, subsystem_device, and (if present) revision sysfs > > files first (http://www.spinics.net/lists/dri-devel/msg122319.html). > > > Yes, making the revision file optional and "faking it" was my first > thought, esp. since we don't have any users of it (yet). > Although people are not too keen on it, so we'll likely opt for > revision-less API. > > > Bottom line, I guess I'm not super opposed to this, but I do feel like > > we're making a kernel change to cover up a userspace problem, and I > > think it would be better to push on that userspace problem a little > > more. > > > Yes, definitely we can beat some sense into userspace. Yet that > shouldn't be a deterrent for exposing the revision. Maybe. If we speed things up by extending this kernel ABI, there's much less incentive to optimize the userspace stuff. I feel a little bit like an enabler for undesirable userspace behavior :) > As hinted before the other prominent user libpciaccess wakes up probes > _every_ pci device. Is it really necessary to probe *every* PCI device? That doesn't sound like a scalable design. As you can tell, the argument that "we should add this kernel ABI to make suboptimal userspace algorithms go faster" doesn't feel very compelling to me. > Atm that library is used by Xorg, Spice, libvirt and a few others. > Amongst which are the Intel GL drivers (via libdrm_intel.so), [only] > when GLX_MESA_query_renderer is used. > > Or in other words - if Firefox/other GL app wants to use the > extension, they'll get similar delays. > We should look into that one as well, but it will be more picky to > address (read "slower to reach end users"). ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2] PCI: create revision file in sysfs 2016-11-11 14:49 ` Bjorn Helgaas @ 2016-11-11 18:56 ` Emil Velikov 0 siblings, 0 replies; 41+ messages in thread From: Emil Velikov @ 2016-11-11 18:56 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: Greg KH, ML dri-devel, Bjorn Helgaas, Linux PCI Hi Bjorn, On 11 November 2016 at 14:49, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Fri, Nov 11, 2016 at 12:31:47AM +0000, Emil Velikov wrote: >> On 10 November 2016 at 23:59, Bjorn Helgaas <helgaas@kernel.org> wrote: >> > Hi Emil, >> > >> > On Thu, Nov 10, 2016 at 01:14:35PM +0000, Emil Velikov wrote: >> >> On 10 November 2016 at 07:13, Greg KH <gregkh@linuxfoundation.org> wrote: >> >> > On Wed, Nov 09, 2016 at 04:56:07PM +0000, Emil Velikov wrote: >> >> >> From: Emil Velikov <emil.velikov@collabora.com> >> >> >> >> >> >> Currently the revision isn't available via sysfs/libudev thus if one >> >> >> wants to know the value they need to read through the config file. >> >> >> >> >> >> This in itself wakes/powers up the device, causing unwanted delays. >> >> >> >> >> >> There are at least two userspace components which could make use the new >> >> >> file - libpciaccess and libdrm. At the moment the former will wake up >> >> >> _every_ PCI device for simple invocation of glxinfo [when using Mesa >> >> >> 10.0+ drivers]. While the latter [in association with Mesa 13.0] can >> >> >> lead to 2-3 second delays while starting firefox, thunderbird or >> >> >> chromium. >> > >> > I agree, these unwanted delays are completely unacceptable. My >> > question is whether we should fix them by exporting more information >> > from the kernel, or by changing the way the userspace components work. >> > >> > It should not take anywhere near 2 seconds to wake up a PCI device. >> > That makes me think there's a more serious problem than just a lack of >> > caching for the revision field, e.g., maybe we're looking at far more >> > PCI devices than we need to, or we're doing it many times to the same >> > device, or ... >> > >> > If I understand correctly, the delay was bisected to >> > https://cgit.freedesktop.org/mesa/mesa/commit/?id=be239326aa4f, which >> > removed a bunch of code that looked up the vendor and device IDs, and >> > replaced it with drmGetDevice(). And apparently drmGetDevice(), in >> > this path: >> > >> > drmGetDevice >> > drmProcessPciDevice >> > drmParsePciDeviceInfo >> > >> > is a little more thorough in that it looks up the *revision* in >> > addition to the vendor and device IDs. So we pay the cost for the >> > revision even though in this instance we don't care about the revision >> > at all. >> > >> Above all, apologies for all the "lovely" code that you had to go >> through for these. >> And yes, you've got it spot on. >> >> > drmParsePciDeviceInfo() currently reads the whole config header from >> > sysfs (https://cgit.freedesktop.org/drm/libdrm/tree/xf86drm.c#n2949), >> > but I think you're extending that to try the vendor, device, >> > subsystem_vendor, subsystem_device, and (if present) revision sysfs >> > files first (http://www.spinics.net/lists/dri-devel/msg122319.html). >> > >> Yes, making the revision file optional and "faking it" was my first >> thought, esp. since we don't have any users of it (yet). >> Although people are not too keen on it, so we'll likely opt for >> revision-less API. >> >> > Bottom line, I guess I'm not super opposed to this, but I do feel like >> > we're making a kernel change to cover up a userspace problem, and I >> > think it would be better to push on that userspace problem a little >> > more. >> > >> Yes, definitely we can beat some sense into userspace. Yet that >> shouldn't be a deterrent for exposing the revision. > > Maybe. If we speed things up by extending this kernel ABI, there's > much less incentive to optimize the userspace stuff. I feel a little > bit like an enabler for undesirable userspace behavior :) > Yes, fixing userspace to not do silly things is the goal. But at the same time even if userspace is perfect, there is no reason to power on the device just to get the revision field, is it ? Especially since everything else is readily available. >> As hinted before the other prominent user libpciaccess wakes up probes >> _every_ pci device. > > Is it really necessary to probe *every* PCI device? That doesn't > sound like a scalable design. > > As you can tell, the argument that "we should add this kernel ABI to > make suboptimal userspace algorithms go faster" doesn't feel very > compelling to me. > "Don't shoot the messenger" comes to mind. I'm just the stupid^Wnice person who's trying to untangle unfortunate design decisions - don't force me to rewrite more than a dozen pieces of software, please ? Even then, I wonder how long it'll take for those to hit end users. Yes I see your concern - userspace does do stupid stuff. Yet it [sometimes] must know the information and the current way of retrieving it (waking up the device) is quite sub-optimal. Thanks Emil P.S. Some drivers have custom ioctls to retrieve the device info (incl. revision). Surely we won't want to continue promoting/assisting that ? ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2] PCI: create revision file in sysfs @ 2016-11-11 18:56 ` Emil Velikov 0 siblings, 0 replies; 41+ messages in thread From: Emil Velikov @ 2016-11-11 18:56 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: Bjorn Helgaas, Greg KH, ML dri-devel, Linux PCI Hi Bjorn, On 11 November 2016 at 14:49, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Fri, Nov 11, 2016 at 12:31:47AM +0000, Emil Velikov wrote: >> On 10 November 2016 at 23:59, Bjorn Helgaas <helgaas@kernel.org> wrote: >> > Hi Emil, >> > >> > On Thu, Nov 10, 2016 at 01:14:35PM +0000, Emil Velikov wrote: >> >> On 10 November 2016 at 07:13, Greg KH <gregkh@linuxfoundation.org> wrote: >> >> > On Wed, Nov 09, 2016 at 04:56:07PM +0000, Emil Velikov wrote: >> >> >> From: Emil Velikov <emil.velikov@collabora.com> >> >> >> >> >> >> Currently the revision isn't available via sysfs/libudev thus if one >> >> >> wants to know the value they need to read through the config file. >> >> >> >> >> >> This in itself wakes/powers up the device, causing unwanted delays. >> >> >> >> >> >> There are at least two userspace components which could make use the new >> >> >> file - libpciaccess and libdrm. At the moment the former will wake up >> >> >> _every_ PCI device for simple invocation of glxinfo [when using Mesa >> >> >> 10.0+ drivers]. While the latter [in association with Mesa 13.0] can >> >> >> lead to 2-3 second delays while starting firefox, thunderbird or >> >> >> chromium. >> > >> > I agree, these unwanted delays are completely unacceptable. My >> > question is whether we should fix them by exporting more information >> > from the kernel, or by changing the way the userspace components work. >> > >> > It should not take anywhere near 2 seconds to wake up a PCI device. >> > That makes me think there's a more serious problem than just a lack of >> > caching for the revision field, e.g., maybe we're looking at far more >> > PCI devices than we need to, or we're doing it many times to the same >> > device, or ... >> > >> > If I understand correctly, the delay was bisected to >> > https://cgit.freedesktop.org/mesa/mesa/commit/?id=be239326aa4f, which >> > removed a bunch of code that looked up the vendor and device IDs, and >> > replaced it with drmGetDevice(). And apparently drmGetDevice(), in >> > this path: >> > >> > drmGetDevice >> > drmProcessPciDevice >> > drmParsePciDeviceInfo >> > >> > is a little more thorough in that it looks up the *revision* in >> > addition to the vendor and device IDs. So we pay the cost for the >> > revision even though in this instance we don't care about the revision >> > at all. >> > >> Above all, apologies for all the "lovely" code that you had to go >> through for these. >> And yes, you've got it spot on. >> >> > drmParsePciDeviceInfo() currently reads the whole config header from >> > sysfs (https://cgit.freedesktop.org/drm/libdrm/tree/xf86drm.c#n2949), >> > but I think you're extending that to try the vendor, device, >> > subsystem_vendor, subsystem_device, and (if present) revision sysfs >> > files first (http://www.spinics.net/lists/dri-devel/msg122319.html). >> > >> Yes, making the revision file optional and "faking it" was my first >> thought, esp. since we don't have any users of it (yet). >> Although people are not too keen on it, so we'll likely opt for >> revision-less API. >> >> > Bottom line, I guess I'm not super opposed to this, but I do feel like >> > we're making a kernel change to cover up a userspace problem, and I >> > think it would be better to push on that userspace problem a little >> > more. >> > >> Yes, definitely we can beat some sense into userspace. Yet that >> shouldn't be a deterrent for exposing the revision. > > Maybe. If we speed things up by extending this kernel ABI, there's > much less incentive to optimize the userspace stuff. I feel a little > bit like an enabler for undesirable userspace behavior :) > Yes, fixing userspace to not do silly things is the goal. But at the same time even if userspace is perfect, there is no reason to power on the device just to get the revision field, is it ? Especially since everything else is readily available. >> As hinted before the other prominent user libpciaccess wakes up probes >> _every_ pci device. > > Is it really necessary to probe *every* PCI device? That doesn't > sound like a scalable design. > > As you can tell, the argument that "we should add this kernel ABI to > make suboptimal userspace algorithms go faster" doesn't feel very > compelling to me. > "Don't shoot the messenger" comes to mind. I'm just the stupid^Wnice person who's trying to untangle unfortunate design decisions - don't force me to rewrite more than a dozen pieces of software, please ? Even then, I wonder how long it'll take for those to hit end users. Yes I see your concern - userspace does do stupid stuff. Yet it [sometimes] must know the information and the current way of retrieving it (waking up the device) is quite sub-optimal. Thanks Emil P.S. Some drivers have custom ioctls to retrieve the device info (incl. revision). Surely we won't want to continue promoting/assisting that ? _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2] PCI: create revision file in sysfs 2016-11-11 18:56 ` Emil Velikov (?) @ 2016-11-14 17:20 ` Bjorn Helgaas -1 siblings, 0 replies; 41+ messages in thread From: Bjorn Helgaas @ 2016-11-14 17:20 UTC (permalink / raw) To: Emil Velikov; +Cc: Greg KH, ML dri-devel, Bjorn Helgaas, Linux PCI On Fri, Nov 11, 2016 at 06:56:51PM +0000, Emil Velikov wrote: > Hi Bjorn, > > On 11 November 2016 at 14:49, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Nov 11, 2016 at 12:31:47AM +0000, Emil Velikov wrote: > >> On 10 November 2016 at 23:59, Bjorn Helgaas <helgaas@kernel.org> wrote: > >> > Hi Emil, > >> > > >> > On Thu, Nov 10, 2016 at 01:14:35PM +0000, Emil Velikov wrote: > >> >> On 10 November 2016 at 07:13, Greg KH <gregkh@linuxfoundation.org> wrote: > >> >> > On Wed, Nov 09, 2016 at 04:56:07PM +0000, Emil Velikov wrote: > >> >> >> From: Emil Velikov <emil.velikov@collabora.com> > >> >> >> > >> >> >> Currently the revision isn't available via sysfs/libudev thus if one > >> >> >> wants to know the value they need to read through the config file. > >> >> >> > >> >> >> This in itself wakes/powers up the device, causing unwanted delays. > >> >> >> > >> >> >> There are at least two userspace components which could make use the new > >> >> >> file - libpciaccess and libdrm. At the moment the former will wake up > >> >> >> _every_ PCI device for simple invocation of glxinfo [when using Mesa > >> >> >> 10.0+ drivers]. While the latter [in association with Mesa 13.0] can > >> >> >> lead to 2-3 second delays while starting firefox, thunderbird or > >> >> >> chromium. > >> > > >> > I agree, these unwanted delays are completely unacceptable. My > >> > question is whether we should fix them by exporting more information > >> > from the kernel, or by changing the way the userspace components work. > >> > > >> > It should not take anywhere near 2 seconds to wake up a PCI device. > >> > That makes me think there's a more serious problem than just a lack of > >> > caching for the revision field, e.g., maybe we're looking at far more > >> > PCI devices than we need to, or we're doing it many times to the same > >> > device, or ... > >> > > >> > If I understand correctly, the delay was bisected to > >> > https://cgit.freedesktop.org/mesa/mesa/commit/?id=be239326aa4f, which > >> > removed a bunch of code that looked up the vendor and device IDs, and > >> > replaced it with drmGetDevice(). And apparently drmGetDevice(), in > >> > this path: > >> > > >> > drmGetDevice > >> > drmProcessPciDevice > >> > drmParsePciDeviceInfo > >> > > >> > is a little more thorough in that it looks up the *revision* in > >> > addition to the vendor and device IDs. So we pay the cost for the > >> > revision even though in this instance we don't care about the revision > >> > at all. > >> > > >> Above all, apologies for all the "lovely" code that you had to go > >> through for these. > >> And yes, you've got it spot on. > >> > >> > drmParsePciDeviceInfo() currently reads the whole config header from > >> > sysfs (https://cgit.freedesktop.org/drm/libdrm/tree/xf86drm.c#n2949), > >> > but I think you're extending that to try the vendor, device, > >> > subsystem_vendor, subsystem_device, and (if present) revision sysfs > >> > files first (http://www.spinics.net/lists/dri-devel/msg122319.html). > >> > > >> Yes, making the revision file optional and "faking it" was my first > >> thought, esp. since we don't have any users of it (yet). > >> Although people are not too keen on it, so we'll likely opt for > >> revision-less API. > >> > >> > Bottom line, I guess I'm not super opposed to this, but I do feel like > >> > we're making a kernel change to cover up a userspace problem, and I > >> > think it would be better to push on that userspace problem a little > >> > more. > >> > > >> Yes, definitely we can beat some sense into userspace. Yet that > >> shouldn't be a deterrent for exposing the revision. > > > > Maybe. If we speed things up by extending this kernel ABI, there's > > much less incentive to optimize the userspace stuff. I feel a little > > bit like an enabler for undesirable userspace behavior :) > > > Yes, fixing userspace to not do silly things is the goal. But at the > same time even if userspace is perfect, there is no reason to power on > the device just to get the revision field, is it ? > Especially since everything else is readily available. > > >> As hinted before the other prominent user libpciaccess wakes up probes > >> _every_ pci device. > > > > Is it really necessary to probe *every* PCI device? That doesn't > > sound like a scalable design. > > > > As you can tell, the argument that "we should add this kernel ABI to > > make suboptimal userspace algorithms go faster" doesn't feel very > > compelling to me. > > > "Don't shoot the messenger" comes to mind. I'm just the stupid^Wnice > person who's trying to untangle unfortunate design decisions - don't > force me to rewrite more than a dozen pieces of software, please ? > Even then, I wonder how long it'll take for those to hit end users. Pre-be239326aa4f, you had: int libudev_get_pci_id_for_fd(int fd, int *vendor_id, int *chip_id) int sysfs_get_pci_id_for_fd(int fd, int *vendor_id, int *chip_id) int drm_get_pci_id_for_fd(int fd, int *vendor_id, int *chip_id) None of them returned the revision. There was some duplicated code, but it was apparently functional and fast. be239326aa4f removed libudev_get_pci_id_for_fd() and sysfs_get_pci_id_for_fd(), which made the code prettier. It also changed drm_get_pci_id_for_fd() to use drmGetDevice() instead of the awful hard-coding of vendor/device IDs based on drmGetVersion()->name. But drmGetDevice() also returns the revision, which we don't need. If you applied http://www.spinics.net/lists/dri-devel/msg122319.html, you'd have code that's fast but unreliable (as you pointed out, it returns the revision on new kernels, but 0 on old kernels, with no hint to the caller about whether the revision is accurate). If the caller can say "I don't care about the revision", e.g., http://www.spinics.net/lists/dri-devel/msg123013.html, you can make drm_get_pci_id_for_fd() fast again. But it will be fast and functional even if the kernel doesn't export a "revision" sysfs file. So what's the benefit of adding it? This seems like a long circular chain of making things simpler in one area but having to add new complications in another to compensate. Bjorn ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2] PCI: create revision file in sysfs 2016-11-10 23:59 ` Bjorn Helgaas @ 2016-11-14 3:35 ` Michel Dänzer 2016-11-14 3:35 ` Michel Dänzer 1 sibling, 0 replies; 41+ messages in thread From: Michel Dänzer @ 2016-11-14 3:35 UTC (permalink / raw) To: Bjorn Helgaas, Emil Velikov Cc: Bjorn Helgaas, Greg KH, ML dri-devel, Linux PCI On 11/11/16 08:59 AM, Bjorn Helgaas wrote: > On Thu, Nov 10, 2016 at 01:14:35PM +0000, Emil Velikov wrote: >> On 10 November 2016 at 07:13, Greg KH <gregkh@linuxfoundation.org> wrote: >>> On Wed, Nov 09, 2016 at 04:56:07PM +0000, Emil Velikov wrote: >>>> From: Emil Velikov <emil.velikov@collabora.com> >>>> >>>> Currently the revision isn't available via sysfs/libudev thus if one >>>> wants to know the value they need to read through the config file. >>>> >>>> This in itself wakes/powers up the device, causing unwanted delays. >>>> >>>> There are at least two userspace components which could make use the new >>>> file - libpciaccess and libdrm. At the moment the former will wake up >>>> _every_ PCI device for simple invocation of glxinfo [when using Mesa >>>> 10.0+ drivers]. While the latter [in association with Mesa 13.0] can >>>> lead to 2-3 second delays while starting firefox, thunderbird or >>>> chromium. > > I agree, these unwanted delays are completely unacceptable. My > question is whether we should fix them by exporting more information > from the kernel, or by changing the way the userspace components work. > > It should not take anywhere near 2 seconds to wake up a PCI device. The DRM drivers for AMD/ATI GPUs can take on the order of that to initialize, so a single wakeup might be sufficient for the described symptoms. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2] PCI: create revision file in sysfs @ 2016-11-14 3:35 ` Michel Dänzer 0 siblings, 0 replies; 41+ messages in thread From: Michel Dänzer @ 2016-11-14 3:35 UTC (permalink / raw) To: Bjorn Helgaas, Emil Velikov Cc: Bjorn Helgaas, Greg KH, ML dri-devel, Linux PCI On 11/11/16 08:59 AM, Bjorn Helgaas wrote: > On Thu, Nov 10, 2016 at 01:14:35PM +0000, Emil Velikov wrote: >> On 10 November 2016 at 07:13, Greg KH <gregkh@linuxfoundation.org> wrote: >>> On Wed, Nov 09, 2016 at 04:56:07PM +0000, Emil Velikov wrote: >>>> From: Emil Velikov <emil.velikov@collabora.com> >>>> >>>> Currently the revision isn't available via sysfs/libudev thus if one >>>> wants to know the value they need to read through the config file. >>>> >>>> This in itself wakes/powers up the device, causing unwanted delays. >>>> >>>> There are at least two userspace components which could make use the new >>>> file - libpciaccess and libdrm. At the moment the former will wake up >>>> _every_ PCI device for simple invocation of glxinfo [when using Mesa >>>> 10.0+ drivers]. While the latter [in association with Mesa 13.0] can >>>> lead to 2-3 second delays while starting firefox, thunderbird or >>>> chromium. > > I agree, these unwanted delays are completely unacceptable. My > question is whether we should fix them by exporting more information > from the kernel, or by changing the way the userspace components work. > > It should not take anywhere near 2 seconds to wake up a PCI device. The DRM drivers for AMD/ATI GPUs can take on the order of that to initialize, so a single wakeup might be sufficient for the described symptoms. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v3] PCI: create revision file in sysfs 2016-11-01 15:42 [PATCH] PCI: create revision file in sysfs Emil Velikov 2016-11-01 15:47 ` Alex Deucher 2016-11-09 16:56 ` Emil Velikov @ 2016-11-11 14:37 ` Emil Velikov 2016-11-14 18:40 ` Daniel Vetter 2016-11-18 19:06 ` Bjorn Helgaas 2 siblings, 2 replies; 41+ messages in thread From: Emil Velikov @ 2016-11-11 14:37 UTC (permalink / raw) To: dri-devel; +Cc: emil.l.velikov, Bjorn Helgaas, linux-pci, Greg KH From: Emil Velikov <emil.velikov@collabora.com> Currently the revision isn't available via sysfs/libudev thus if one wants to know the value they need to read through the config file. This in itself wakes/powers up the device, causing unwanted delay since it can be quite costly. There are at least two userspace components which could make use the new file libpciaccess and libdrm. The former [used in various places] wakes up _every_ PCI device, which can be observed via glxinfo [when using Mesa 10.0+ drivers]. While the latter [in association with Mesa 13.0] can lead to 2-3 second delays while starting firefox, thunderbird or chromium. Expose the revision as a separate file, just like we do for the device, vendor, their subsystem version and class. Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: linux-pci@vger.kernel.org Cc: Greg KH <gregkh@linuxfoundation.org> Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 Tested-by: Mauro Santos <registo.mailling@gmail.com> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> Signed-off-by: Emil Velikov <emil.velikov@collabora.com> --- v2: Add r-b/t-b tags, slim down CC list, add note about userspace. v3: Add Documentation/ bits (Greg KH) Gents, please keep me in the CC list. Thanks Emil --- Documentation/ABI/testing/sysfs-bus-pci | 7 +++++++ Documentation/filesystems/sysfs-pci.txt | 2 ++ drivers/pci/pci-sysfs.c | 2 ++ 3 files changed, 11 insertions(+) diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci index b3bc50f..5a1732b 100644 --- a/Documentation/ABI/testing/sysfs-bus-pci +++ b/Documentation/ABI/testing/sysfs-bus-pci @@ -294,3 +294,10 @@ Description: a firmware bug to the system vendor. Writing to this file taints the kernel with TAINT_FIRMWARE_WORKAROUND, which reduces the supportability of your system. + +What: /sys/bus/pci/devices/.../revision +Date: November 2016 +Contact: Emil Velikov <emil.l.velikov@gmail.com> +Description: + This file contains the revision field of the the PCI device. + The value comes from device config space. The file is read only. diff --git a/Documentation/filesystems/sysfs-pci.txt b/Documentation/filesystems/sysfs-pci.txt index 74eaac2..6ea1ced 100644 --- a/Documentation/filesystems/sysfs-pci.txt +++ b/Documentation/filesystems/sysfs-pci.txt @@ -17,6 +17,7 @@ that support it. For example, a given bus might look like this: | |-- resource0 | |-- resource1 | |-- resource2 + | |-- revision | |-- rom | |-- subsystem_device | |-- subsystem_vendor @@ -41,6 +42,7 @@ files, each with their own function. resource PCI resource host addresses (ascii, ro) resource0..N PCI resource N, if present (binary, mmap, rw[1]) resource0_wc..N_wc PCI WC map resource N, if prefetchable (binary, mmap) + revision PCI revision (ascii, ro) rom PCI ROM resource, if present (binary, ro) subsystem_device PCI subsystem device (ascii, ro) subsystem_vendor PCI subsystem vendor (ascii, ro) diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index bcd10c7..0666287 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -50,6 +50,7 @@ pci_config_attr(vendor, "0x%04x\n"); pci_config_attr(device, "0x%04x\n"); pci_config_attr(subsystem_vendor, "0x%04x\n"); pci_config_attr(subsystem_device, "0x%04x\n"); +pci_config_attr(revision, "0x%02x\n"); pci_config_attr(class, "0x%06x\n"); pci_config_attr(irq, "%u\n"); @@ -568,6 +569,7 @@ static struct attribute *pci_dev_attrs[] = { &dev_attr_device.attr, &dev_attr_subsystem_vendor.attr, &dev_attr_subsystem_device.attr, + &dev_attr_revision.attr, &dev_attr_class.attr, &dev_attr_irq.attr, &dev_attr_local_cpus.attr, -- 2.9.3 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs 2016-11-11 14:37 ` [PATCH v3] " Emil Velikov @ 2016-11-14 18:40 ` Daniel Vetter 2016-11-18 19:06 ` Bjorn Helgaas 1 sibling, 0 replies; 41+ messages in thread From: Daniel Vetter @ 2016-11-14 18:40 UTC (permalink / raw) To: Emil Velikov; +Cc: dri-devel, Bjorn Helgaas, linux-pci, Greg KH On Fri, Nov 11, 2016 at 02:37:23PM +0000, Emil Velikov wrote: > From: Emil Velikov <emil.velikov@collabora.com> > > Currently the revision isn't available via sysfs/libudev thus if one > wants to know the value they need to read through the config file. > > This in itself wakes/powers up the device, causing unwanted delay > since it can be quite costly. > > There are at least two userspace components which could make use the new > file libpciaccess and libdrm. The former [used in various places] wakes > up _every_ PCI device, which can be observed via glxinfo [when using > Mesa 10.0+ drivers]. While the latter [in association with Mesa 13.0] > can lead to 2-3 second delays while starting firefox, thunderbird or > chromium. > > Expose the revision as a separate file, just like we do for the device, > vendor, their subsystem version and class. > > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: linux-pci@vger.kernel.org > Cc: Greg KH <gregkh@linuxfoundation.org> > Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 > Tested-by: Mauro Santos <registo.mailling@gmail.com> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> Given that waking a gpu can take somewhere between ages and forever, and that we read the pci revisions everytime we launch a new gl app I think this is the correct approach. Of course we could just patch libdrm and everyone to not look at the pci revision, but that just leads to every pci-based driver having a driver-private ioctl/getparam thing to expose it. Which also doesn't make much sense. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Björn, if you're all ok with this we'd like to start landing at least libdrm patches before this patch hits a released kernel, just to shorten the pain window for users waiting for upgrades. Thanks, Daniel > --- > v2: Add r-b/t-b tags, slim down CC list, add note about userspace. > > v3: Add Documentation/ bits (Greg KH) > > Gents, please keep me in the CC list. > > Thanks > Emil > --- > Documentation/ABI/testing/sysfs-bus-pci | 7 +++++++ > Documentation/filesystems/sysfs-pci.txt | 2 ++ > drivers/pci/pci-sysfs.c | 2 ++ > 3 files changed, 11 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci > index b3bc50f..5a1732b 100644 > --- a/Documentation/ABI/testing/sysfs-bus-pci > +++ b/Documentation/ABI/testing/sysfs-bus-pci > @@ -294,3 +294,10 @@ Description: > a firmware bug to the system vendor. Writing to this file > taints the kernel with TAINT_FIRMWARE_WORKAROUND, which > reduces the supportability of your system. > + > +What: /sys/bus/pci/devices/.../revision > +Date: November 2016 > +Contact: Emil Velikov <emil.l.velikov@gmail.com> > +Description: > + This file contains the revision field of the the PCI device. > + The value comes from device config space. The file is read only. > diff --git a/Documentation/filesystems/sysfs-pci.txt b/Documentation/filesystems/sysfs-pci.txt > index 74eaac2..6ea1ced 100644 > --- a/Documentation/filesystems/sysfs-pci.txt > +++ b/Documentation/filesystems/sysfs-pci.txt > @@ -17,6 +17,7 @@ that support it. For example, a given bus might look like this: > | |-- resource0 > | |-- resource1 > | |-- resource2 > + | |-- revision > | |-- rom > | |-- subsystem_device > | |-- subsystem_vendor > @@ -41,6 +42,7 @@ files, each with their own function. > resource PCI resource host addresses (ascii, ro) > resource0..N PCI resource N, if present (binary, mmap, rw[1]) > resource0_wc..N_wc PCI WC map resource N, if prefetchable (binary, mmap) > + revision PCI revision (ascii, ro) > rom PCI ROM resource, if present (binary, ro) > subsystem_device PCI subsystem device (ascii, ro) > subsystem_vendor PCI subsystem vendor (ascii, ro) > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index bcd10c7..0666287 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -50,6 +50,7 @@ pci_config_attr(vendor, "0x%04x\n"); > pci_config_attr(device, "0x%04x\n"); > pci_config_attr(subsystem_vendor, "0x%04x\n"); > pci_config_attr(subsystem_device, "0x%04x\n"); > +pci_config_attr(revision, "0x%02x\n"); > pci_config_attr(class, "0x%06x\n"); > pci_config_attr(irq, "%u\n"); > > @@ -568,6 +569,7 @@ static struct attribute *pci_dev_attrs[] = { > &dev_attr_device.attr, > &dev_attr_subsystem_vendor.attr, > &dev_attr_subsystem_device.attr, > + &dev_attr_revision.attr, > &dev_attr_class.attr, > &dev_attr_irq.attr, > &dev_attr_local_cpus.attr, > -- > 2.9.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs @ 2016-11-14 18:40 ` Daniel Vetter 0 siblings, 0 replies; 41+ messages in thread From: Daniel Vetter @ 2016-11-14 18:40 UTC (permalink / raw) To: Emil Velikov; +Cc: Bjorn Helgaas, linux-pci, dri-devel, Greg KH On Fri, Nov 11, 2016 at 02:37:23PM +0000, Emil Velikov wrote: > From: Emil Velikov <emil.velikov@collabora.com> > > Currently the revision isn't available via sysfs/libudev thus if one > wants to know the value they need to read through the config file. > > This in itself wakes/powers up the device, causing unwanted delay > since it can be quite costly. > > There are at least two userspace components which could make use the new > file libpciaccess and libdrm. The former [used in various places] wakes > up _every_ PCI device, which can be observed via glxinfo [when using > Mesa 10.0+ drivers]. While the latter [in association with Mesa 13.0] > can lead to 2-3 second delays while starting firefox, thunderbird or > chromium. > > Expose the revision as a separate file, just like we do for the device, > vendor, their subsystem version and class. > > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: linux-pci@vger.kernel.org > Cc: Greg KH <gregkh@linuxfoundation.org> > Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 > Tested-by: Mauro Santos <registo.mailling@gmail.com> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> Given that waking a gpu can take somewhere between ages and forever, and that we read the pci revisions everytime we launch a new gl app I think this is the correct approach. Of course we could just patch libdrm and everyone to not look at the pci revision, but that just leads to every pci-based driver having a driver-private ioctl/getparam thing to expose it. Which also doesn't make much sense. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Björn, if you're all ok with this we'd like to start landing at least libdrm patches before this patch hits a released kernel, just to shorten the pain window for users waiting for upgrades. Thanks, Daniel > --- > v2: Add r-b/t-b tags, slim down CC list, add note about userspace. > > v3: Add Documentation/ bits (Greg KH) > > Gents, please keep me in the CC list. > > Thanks > Emil > --- > Documentation/ABI/testing/sysfs-bus-pci | 7 +++++++ > Documentation/filesystems/sysfs-pci.txt | 2 ++ > drivers/pci/pci-sysfs.c | 2 ++ > 3 files changed, 11 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci > index b3bc50f..5a1732b 100644 > --- a/Documentation/ABI/testing/sysfs-bus-pci > +++ b/Documentation/ABI/testing/sysfs-bus-pci > @@ -294,3 +294,10 @@ Description: > a firmware bug to the system vendor. Writing to this file > taints the kernel with TAINT_FIRMWARE_WORKAROUND, which > reduces the supportability of your system. > + > +What: /sys/bus/pci/devices/.../revision > +Date: November 2016 > +Contact: Emil Velikov <emil.l.velikov@gmail.com> > +Description: > + This file contains the revision field of the the PCI device. > + The value comes from device config space. The file is read only. > diff --git a/Documentation/filesystems/sysfs-pci.txt b/Documentation/filesystems/sysfs-pci.txt > index 74eaac2..6ea1ced 100644 > --- a/Documentation/filesystems/sysfs-pci.txt > +++ b/Documentation/filesystems/sysfs-pci.txt > @@ -17,6 +17,7 @@ that support it. For example, a given bus might look like this: > | |-- resource0 > | |-- resource1 > | |-- resource2 > + | |-- revision > | |-- rom > | |-- subsystem_device > | |-- subsystem_vendor > @@ -41,6 +42,7 @@ files, each with their own function. > resource PCI resource host addresses (ascii, ro) > resource0..N PCI resource N, if present (binary, mmap, rw[1]) > resource0_wc..N_wc PCI WC map resource N, if prefetchable (binary, mmap) > + revision PCI revision (ascii, ro) > rom PCI ROM resource, if present (binary, ro) > subsystem_device PCI subsystem device (ascii, ro) > subsystem_vendor PCI subsystem vendor (ascii, ro) > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index bcd10c7..0666287 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -50,6 +50,7 @@ pci_config_attr(vendor, "0x%04x\n"); > pci_config_attr(device, "0x%04x\n"); > pci_config_attr(subsystem_vendor, "0x%04x\n"); > pci_config_attr(subsystem_device, "0x%04x\n"); > +pci_config_attr(revision, "0x%02x\n"); > pci_config_attr(class, "0x%06x\n"); > pci_config_attr(irq, "%u\n"); > > @@ -568,6 +569,7 @@ static struct attribute *pci_dev_attrs[] = { > &dev_attr_device.attr, > &dev_attr_subsystem_vendor.attr, > &dev_attr_subsystem_device.attr, > + &dev_attr_revision.attr, > &dev_attr_class.attr, > &dev_attr_irq.attr, > &dev_attr_local_cpus.attr, > -- > 2.9.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs 2016-11-14 18:40 ` Daniel Vetter (?) @ 2016-11-16 20:58 ` Bjorn Helgaas 2016-11-16 21:30 ` Sinan Kaya ` (2 more replies) -1 siblings, 3 replies; 41+ messages in thread From: Bjorn Helgaas @ 2016-11-16 20:58 UTC (permalink / raw) To: Daniel Vetter Cc: Emil Velikov, dri-devel, Bjorn Helgaas, linux-pci, Greg KH, Sinan Kaya, Lukas Wunner [+cc Sinan, Lukas] Hi Daniel, On Mon, Nov 14, 2016 at 07:40:03PM +0100, Daniel Vetter wrote: > On Fri, Nov 11, 2016 at 02:37:23PM +0000, Emil Velikov wrote: > > From: Emil Velikov <emil.velikov@collabora.com> > > > > Currently the revision isn't available via sysfs/libudev thus if one > > wants to know the value they need to read through the config file. > > > > This in itself wakes/powers up the device, causing unwanted delay > > since it can be quite costly. > > > > There are at least two userspace components which could make use the new > > file libpciaccess and libdrm. The former [used in various places] wakes > > up _every_ PCI device, which can be observed via glxinfo [when using > > Mesa 10.0+ drivers]. While the latter [in association with Mesa 13.0] > > can lead to 2-3 second delays while starting firefox, thunderbird or > > chromium. > > > > Expose the revision as a separate file, just like we do for the device, > > vendor, their subsystem version and class. > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Cc: linux-pci@vger.kernel.org > > Cc: Greg KH <gregkh@linuxfoundation.org> > > Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 > > Tested-by: Mauro Santos <registo.mailling@gmail.com> > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > Given that waking a gpu can take somewhere between ages and forever, and > that we read the pci revisions everytime we launch a new gl app I think > this is the correct approach. Of course we could just patch libdrm and > everyone to not look at the pci revision, but that just leads to every > pci-based driver having a driver-private ioctl/getparam thing to expose > it. Which also doesn't make much sense. This re-asserts what has already been said, but doesn't address any of my questions in the v2 discussion, so I'm still looking to continue that thread. I am curious about this long wakeup issue, though. Are we talking about a D3cold -> D0 transition? I assume so, since config space is generally accessible in all power states except D3cold. From the device's point of view this is basically like a power-on. I think the gist of PCIe r3.0, sec 6.6.1 is that we need to wait 100ms, e.g., PCI_PM_D3COLD_WAIT, before doing config accesses. We do support Configuration Request Retry Status Software Visibility (pci_enable_crs()), so a device *can* take longer than 100ms after power-up to respond to a config read, but I think that only applies to reads of the Vendor ID. I cc'd Sinan because we do have some issues with our CRS support, and maybe he can shed some light on this. I'm not surprised if a GPU takes longer than 100ms to do device- specific, driver-managed, non-PCI things like detect and wake up monitors. But I *am* surprised if generic PCI bus-level things like config reads take longer than that. I also cc'd Lukas because he knows a lot more about PCI PM than I do. Bjorn ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs 2016-11-16 20:58 ` Bjorn Helgaas @ 2016-11-16 21:30 ` Sinan Kaya 2016-11-17 13:28 ` Alex Deucher 2016-11-17 14:44 ` Lukas Wunner 2 siblings, 0 replies; 41+ messages in thread From: Sinan Kaya @ 2016-11-16 21:30 UTC (permalink / raw) To: Bjorn Helgaas, Daniel Vetter Cc: Emil Velikov, dri-devel, Bjorn Helgaas, linux-pci, Greg KH, Lukas Wunner On 11/16/2016 3:58 PM, Bjorn Helgaas wrote: > [+cc Sinan, Lukas] > > Hi Daniel, > > On Mon, Nov 14, 2016 at 07:40:03PM +0100, Daniel Vetter wrote: >> On Fri, Nov 11, 2016 at 02:37:23PM +0000, Emil Velikov wrote: >>> From: Emil Velikov <emil.velikov@collabora.com> >>> >>> Currently the revision isn't available via sysfs/libudev thus if one >>> wants to know the value they need to read through the config file. >>> >>> This in itself wakes/powers up the device, causing unwanted delay >>> since it can be quite costly. >>> >>> There are at least two userspace components which could make use the new >>> file libpciaccess and libdrm. The former [used in various places] wakes >>> up _every_ PCI device, which can be observed via glxinfo [when using >>> Mesa 10.0+ drivers]. While the latter [in association with Mesa 13.0] >>> can lead to 2-3 second delays while starting firefox, thunderbird or >>> chromium. >>> >>> Expose the revision as a separate file, just like we do for the device, >>> vendor, their subsystem version and class. >>> >>> Cc: Bjorn Helgaas <bhelgaas@google.com> >>> Cc: linux-pci@vger.kernel.org >>> Cc: Greg KH <gregkh@linuxfoundation.org> >>> Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 >>> Tested-by: Mauro Santos <registo.mailling@gmail.com> >>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> >>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com> >> >> Given that waking a gpu can take somewhere between ages and forever, and >> that we read the pci revisions everytime we launch a new gl app I think >> this is the correct approach. Of course we could just patch libdrm and >> everyone to not look at the pci revision, but that just leads to every >> pci-based driver having a driver-private ioctl/getparam thing to expose >> it. Which also doesn't make much sense. > > This re-asserts what has already been said, but doesn't address any of > my questions in the v2 discussion, so I'm still looking to continue > that thread. > > I am curious about this long wakeup issue, though. Are we talking > about a D3cold -> D0 transition? I assume so, since config space is > generally accessible in all power states except D3cold. From the > device's point of view this is basically like a power-on. I think the > gist of PCIe r3.0, sec 6.6.1 is that we need to wait 100ms, e.g., > PCI_PM_D3COLD_WAIT, before doing config accesses. > > We do support Configuration Request Retry Status Software Visibility > (pci_enable_crs()), so a device *can* take longer than 100ms after > power-up to respond to a config read, but I think that only applies to > reads of the Vendor ID. I cc'd Sinan because we do have some issues > with our CRS support, and maybe he can shed some light on this. This applies to all config requests including vendor ID. It is just the vendor ID returns a special code (device id = 0x0001 vendor id =0xffff) so that CRS aware software can understand the difference between CRS and an actual failure. It is recommended to always read the vendor ID following a reset/power on to understand if the device is sending a CRS. As Bjorn mentioned, we do enable CRS visibility in the kernel but only for the root port in pci_enable_crs function. When CRS visibility is disabled, root port needs to retry the request until a good response is received. CRS polling happens behind the scenes in pci_bus_read_dev_vendor_id function. In order for reads to take longer than 100ms, the CRS must be disabled at a lower level in the bus. I wonder if you have a bridge between your device and the root port. Bridge Configuration Retry Enable needs to be programmed if we want kernel to know about retry responses behind a bridge. "Bridge Configuration Retry Enable – When Set, this bit enables PCI Express to PCI/PCI-X bridges to return Configuration Request Retry Status (CRS) in response to Configuration Requests that target devices below the bridge. Refer to the PCI Express to PCI/PCI-X Bridge Specification, Revision 1.0 for further details. Default value of this bit is 0b." Kernel is currently not setting this. > > I'm not surprised if a GPU takes longer than 100ms to do device- > specific, driver-managed, non-PCI things like detect and wake up > monitors. But I *am* surprised if generic PCI bus-level things like > config reads take longer than that. I also cc'd Lukas because he > knows a lot more about PCI PM than I do. > > Bjorn > -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs 2016-11-16 20:58 ` Bjorn Helgaas @ 2016-11-17 13:28 ` Alex Deucher 2016-11-17 13:28 ` Alex Deucher 2016-11-17 14:44 ` Lukas Wunner 2 siblings, 0 replies; 41+ messages in thread From: Alex Deucher @ 2016-11-17 13:28 UTC (permalink / raw) To: Bjorn Helgaas Cc: Daniel Vetter, Linux PCI, Emil Velikov, Maling list - DRI developers, Sinan Kaya, Greg KH, Bjorn Helgaas On Wed, Nov 16, 2016 at 3:58 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > [+cc Sinan, Lukas] > > Hi Daniel, > > On Mon, Nov 14, 2016 at 07:40:03PM +0100, Daniel Vetter wrote: >> On Fri, Nov 11, 2016 at 02:37:23PM +0000, Emil Velikov wrote: >> > From: Emil Velikov <emil.velikov@collabora.com> >> > >> > Currently the revision isn't available via sysfs/libudev thus if one >> > wants to know the value they need to read through the config file. >> > >> > This in itself wakes/powers up the device, causing unwanted delay >> > since it can be quite costly. >> > >> > There are at least two userspace components which could make use the new >> > file libpciaccess and libdrm. The former [used in various places] wakes >> > up _every_ PCI device, which can be observed via glxinfo [when using >> > Mesa 10.0+ drivers]. While the latter [in association with Mesa 13.0] >> > can lead to 2-3 second delays while starting firefox, thunderbird or >> > chromium. >> > >> > Expose the revision as a separate file, just like we do for the device, >> > vendor, their subsystem version and class. >> > >> > Cc: Bjorn Helgaas <bhelgaas@google.com> >> > Cc: linux-pci@vger.kernel.org >> > Cc: Greg KH <gregkh@linuxfoundation.org> >> > Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 >> > Tested-by: Mauro Santos <registo.mailling@gmail.com> >> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> >> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> >> >> Given that waking a gpu can take somewhere between ages and forever, and >> that we read the pci revisions everytime we launch a new gl app I think >> this is the correct approach. Of course we could just patch libdrm and >> everyone to not look at the pci revision, but that just leads to every >> pci-based driver having a driver-private ioctl/getparam thing to expose >> it. Which also doesn't make much sense. > > This re-asserts what has already been said, but doesn't address any of > my questions in the v2 discussion, so I'm still looking to continue > that thread. > > I am curious about this long wakeup issue, though. Are we talking > about a D3cold -> D0 transition? I assume so, since config space is > generally accessible in all power states except D3cold. From the > device's point of view this is basically like a power-on. I think the > gist of PCIe r3.0, sec 6.6.1 is that we need to wait 100ms, e.g., > PCI_PM_D3COLD_WAIT, before doing config accesses. > > We do support Configuration Request Retry Status Software Visibility > (pci_enable_crs()), so a device *can* take longer than 100ms after > power-up to respond to a config read, but I think that only applies to > reads of the Vendor ID. I cc'd Sinan because we do have some issues > with our CRS support, and maybe he can shed some light on this. > > I'm not surprised if a GPU takes longer than 100ms to do device- > specific, driver-managed, non-PCI things like detect and wake up > monitors. But I *am* surprised if generic PCI bus-level things like > config reads take longer than that. I also cc'd Lukas because he > knows a lot more about PCI PM than I do. FWIW, If you run lspci on a GPU that is in the powered off state (either D3 cold if supported or the older vendor specific power controls that pre-dated D3 cold), any fields that were not previously cached return all 1s. So for example the pci revision would be 0xff rather than whatever it's supposed to be. Alex ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs @ 2016-11-17 13:28 ` Alex Deucher 0 siblings, 0 replies; 41+ messages in thread From: Alex Deucher @ 2016-11-17 13:28 UTC (permalink / raw) To: Bjorn Helgaas Cc: Linux PCI, Emil Velikov, Maling list - DRI developers, Sinan Kaya, Greg KH, Bjorn Helgaas On Wed, Nov 16, 2016 at 3:58 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > [+cc Sinan, Lukas] > > Hi Daniel, > > On Mon, Nov 14, 2016 at 07:40:03PM +0100, Daniel Vetter wrote: >> On Fri, Nov 11, 2016 at 02:37:23PM +0000, Emil Velikov wrote: >> > From: Emil Velikov <emil.velikov@collabora.com> >> > >> > Currently the revision isn't available via sysfs/libudev thus if one >> > wants to know the value they need to read through the config file. >> > >> > This in itself wakes/powers up the device, causing unwanted delay >> > since it can be quite costly. >> > >> > There are at least two userspace components which could make use the new >> > file libpciaccess and libdrm. The former [used in various places] wakes >> > up _every_ PCI device, which can be observed via glxinfo [when using >> > Mesa 10.0+ drivers]. While the latter [in association with Mesa 13.0] >> > can lead to 2-3 second delays while starting firefox, thunderbird or >> > chromium. >> > >> > Expose the revision as a separate file, just like we do for the device, >> > vendor, their subsystem version and class. >> > >> > Cc: Bjorn Helgaas <bhelgaas@google.com> >> > Cc: linux-pci@vger.kernel.org >> > Cc: Greg KH <gregkh@linuxfoundation.org> >> > Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 >> > Tested-by: Mauro Santos <registo.mailling@gmail.com> >> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> >> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> >> >> Given that waking a gpu can take somewhere between ages and forever, and >> that we read the pci revisions everytime we launch a new gl app I think >> this is the correct approach. Of course we could just patch libdrm and >> everyone to not look at the pci revision, but that just leads to every >> pci-based driver having a driver-private ioctl/getparam thing to expose >> it. Which also doesn't make much sense. > > This re-asserts what has already been said, but doesn't address any of > my questions in the v2 discussion, so I'm still looking to continue > that thread. > > I am curious about this long wakeup issue, though. Are we talking > about a D3cold -> D0 transition? I assume so, since config space is > generally accessible in all power states except D3cold. From the > device's point of view this is basically like a power-on. I think the > gist of PCIe r3.0, sec 6.6.1 is that we need to wait 100ms, e.g., > PCI_PM_D3COLD_WAIT, before doing config accesses. > > We do support Configuration Request Retry Status Software Visibility > (pci_enable_crs()), so a device *can* take longer than 100ms after > power-up to respond to a config read, but I think that only applies to > reads of the Vendor ID. I cc'd Sinan because we do have some issues > with our CRS support, and maybe he can shed some light on this. > > I'm not surprised if a GPU takes longer than 100ms to do device- > specific, driver-managed, non-PCI things like detect and wake up > monitors. But I *am* surprised if generic PCI bus-level things like > config reads take longer than that. I also cc'd Lukas because he > knows a lot more about PCI PM than I do. FWIW, If you run lspci on a GPU that is in the powered off state (either D3 cold if supported or the older vendor specific power controls that pre-dated D3 cold), any fields that were not previously cached return all 1s. So for example the pci revision would be 0xff rather than whatever it's supposed to be. Alex _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs 2016-11-17 13:28 ` Alex Deucher (?) @ 2016-11-17 14:35 ` Bjorn Helgaas 2016-11-17 14:48 ` Alex Deucher -1 siblings, 1 reply; 41+ messages in thread From: Bjorn Helgaas @ 2016-11-17 14:35 UTC (permalink / raw) To: Alex Deucher Cc: Daniel Vetter, Linux PCI, Emil Velikov, Maling list - DRI developers, Sinan Kaya, Greg KH, Bjorn Helgaas On Thu, Nov 17, 2016 at 08:28:50AM -0500, Alex Deucher wrote: > On Wed, Nov 16, 2016 at 3:58 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Mon, Nov 14, 2016 at 07:40:03PM +0100, Daniel Vetter wrote: > >> Given that waking a gpu can take somewhere between ages and forever, and > >> that we read the pci revisions everytime we launch a new gl app I think > >> this is the correct approach. Of course we could just patch libdrm and > >> everyone to not look at the pci revision, but that just leads to every > >> pci-based driver having a driver-private ioctl/getparam thing to expose > >> it. Which also doesn't make much sense. > > > > I am curious about this long wakeup issue, though. Are we talking > > about a D3cold -> D0 transition? I assume so, since config space is > > generally accessible in all power states except D3cold. From the > > device's point of view this is basically like a power-on. I think the > > gist of PCIe r3.0, sec 6.6.1 is that we need to wait 100ms, e.g., > > PCI_PM_D3COLD_WAIT, before doing config accesses. > > > > We do support Configuration Request Retry Status Software Visibility > > (pci_enable_crs()), so a device *can* take longer than 100ms after > > power-up to respond to a config read, but I think that only applies to > > reads of the Vendor ID. I cc'd Sinan because we do have some issues > > with our CRS support, and maybe he can shed some light on this. > > > > I'm not surprised if a GPU takes longer than 100ms to do device- > > specific, driver-managed, non-PCI things like detect and wake up > > monitors. But I *am* surprised if generic PCI bus-level things like > > config reads take longer than that. I also cc'd Lukas because he > > knows a lot more about PCI PM than I do. > > FWIW, If you run lspci on a GPU that is in the powered off state > (either D3 cold if supported or the older vendor specific power > controls that pre-dated D3 cold), any fields that were not previously > cached return all 1s. So for example the pci revision would be 0xff > rather than whatever it's supposed to be. That doesn't feel like the right behavior to me -- I would have expected lspci to either wake up the device and show valid data or maybe complain "this device is powered off" (this seems hard to do without races). Showing a mix of cached valid data and all 1s data seems like a strange user experience to me. Caching the revision would fix that particular piece, of course, but not the overall experience. Am I expecting too much? Maybe my expectations are out of line. I think in this particular case (reading config space of a powered-off device), CRS doesn't apply because the device is powered off, and there's probably no delay: the read would probably complete immediately because the link to the device is down, and the Root Port or Downstream Port would probably generate an Unsupported Request completion, which the Root Complex might handle by fabricating all 1s data for the CPU. Bjorn ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs 2016-11-17 14:35 ` Bjorn Helgaas @ 2016-11-17 14:48 ` Alex Deucher 0 siblings, 0 replies; 41+ messages in thread From: Alex Deucher @ 2016-11-17 14:48 UTC (permalink / raw) To: Bjorn Helgaas Cc: Daniel Vetter, Linux PCI, Emil Velikov, Maling list - DRI developers, Sinan Kaya, Greg KH, Bjorn Helgaas On Thu, Nov 17, 2016 at 9:35 AM, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Thu, Nov 17, 2016 at 08:28:50AM -0500, Alex Deucher wrote: >> On Wed, Nov 16, 2016 at 3:58 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: >> > On Mon, Nov 14, 2016 at 07:40:03PM +0100, Daniel Vetter wrote: >> >> Given that waking a gpu can take somewhere between ages and forever, and >> >> that we read the pci revisions everytime we launch a new gl app I think >> >> this is the correct approach. Of course we could just patch libdrm and >> >> everyone to not look at the pci revision, but that just leads to every >> >> pci-based driver having a driver-private ioctl/getparam thing to expose >> >> it. Which also doesn't make much sense. >> > >> > I am curious about this long wakeup issue, though. Are we talking >> > about a D3cold -> D0 transition? I assume so, since config space is >> > generally accessible in all power states except D3cold. From the >> > device's point of view this is basically like a power-on. I think the >> > gist of PCIe r3.0, sec 6.6.1 is that we need to wait 100ms, e.g., >> > PCI_PM_D3COLD_WAIT, before doing config accesses. >> > >> > We do support Configuration Request Retry Status Software Visibility >> > (pci_enable_crs()), so a device *can* take longer than 100ms after >> > power-up to respond to a config read, but I think that only applies to >> > reads of the Vendor ID. I cc'd Sinan because we do have some issues >> > with our CRS support, and maybe he can shed some light on this. >> > >> > I'm not surprised if a GPU takes longer than 100ms to do device- >> > specific, driver-managed, non-PCI things like detect and wake up >> > monitors. But I *am* surprised if generic PCI bus-level things like >> > config reads take longer than that. I also cc'd Lukas because he >> > knows a lot more about PCI PM than I do. >> >> FWIW, If you run lspci on a GPU that is in the powered off state >> (either D3 cold if supported or the older vendor specific power >> controls that pre-dated D3 cold), any fields that were not previously >> cached return all 1s. So for example the pci revision would be 0xff >> rather than whatever it's supposed to be. > > That doesn't feel like the right behavior to me -- I would have > expected lspci to either wake up the device and show valid data or > maybe complain "this device is powered off" (this seems hard to do > without races). Showing a mix of cached valid data and all 1s data > seems like a strange user experience to me. > > Caching the revision would fix that particular piece, of course, but > not the overall experience. Am I expecting too much? Maybe my > expectations are out of line. I agree with you, I'm just saying that that is the current behavior. The GPU power is controlled by runtimepm, perhaps there is a corner case we are missing when pci config space is accessed on a powered off device? Alex > > I think in this particular case (reading config space of a powered-off > device), CRS doesn't apply because the device is powered off, and > there's probably no delay: the read would probably complete > immediately because the link to the device is down, and the Root Port > or Downstream Port would probably generate an Unsupported Request > completion, which the Root Complex might handle by fabricating all 1s > data for the CPU. > > Bjorn ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs @ 2016-11-17 14:48 ` Alex Deucher 0 siblings, 0 replies; 41+ messages in thread From: Alex Deucher @ 2016-11-17 14:48 UTC (permalink / raw) To: Bjorn Helgaas Cc: Linux PCI, Emil Velikov, Maling list - DRI developers, Sinan Kaya, Greg KH, Bjorn Helgaas On Thu, Nov 17, 2016 at 9:35 AM, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Thu, Nov 17, 2016 at 08:28:50AM -0500, Alex Deucher wrote: >> On Wed, Nov 16, 2016 at 3:58 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: >> > On Mon, Nov 14, 2016 at 07:40:03PM +0100, Daniel Vetter wrote: >> >> Given that waking a gpu can take somewhere between ages and forever, and >> >> that we read the pci revisions everytime we launch a new gl app I think >> >> this is the correct approach. Of course we could just patch libdrm and >> >> everyone to not look at the pci revision, but that just leads to every >> >> pci-based driver having a driver-private ioctl/getparam thing to expose >> >> it. Which also doesn't make much sense. >> > >> > I am curious about this long wakeup issue, though. Are we talking >> > about a D3cold -> D0 transition? I assume so, since config space is >> > generally accessible in all power states except D3cold. From the >> > device's point of view this is basically like a power-on. I think the >> > gist of PCIe r3.0, sec 6.6.1 is that we need to wait 100ms, e.g., >> > PCI_PM_D3COLD_WAIT, before doing config accesses. >> > >> > We do support Configuration Request Retry Status Software Visibility >> > (pci_enable_crs()), so a device *can* take longer than 100ms after >> > power-up to respond to a config read, but I think that only applies to >> > reads of the Vendor ID. I cc'd Sinan because we do have some issues >> > with our CRS support, and maybe he can shed some light on this. >> > >> > I'm not surprised if a GPU takes longer than 100ms to do device- >> > specific, driver-managed, non-PCI things like detect and wake up >> > monitors. But I *am* surprised if generic PCI bus-level things like >> > config reads take longer than that. I also cc'd Lukas because he >> > knows a lot more about PCI PM than I do. >> >> FWIW, If you run lspci on a GPU that is in the powered off state >> (either D3 cold if supported or the older vendor specific power >> controls that pre-dated D3 cold), any fields that were not previously >> cached return all 1s. So for example the pci revision would be 0xff >> rather than whatever it's supposed to be. > > That doesn't feel like the right behavior to me -- I would have > expected lspci to either wake up the device and show valid data or > maybe complain "this device is powered off" (this seems hard to do > without races). Showing a mix of cached valid data and all 1s data > seems like a strange user experience to me. > > Caching the revision would fix that particular piece, of course, but > not the overall experience. Am I expecting too much? Maybe my > expectations are out of line. I agree with you, I'm just saying that that is the current behavior. The GPU power is controlled by runtimepm, perhaps there is a corner case we are missing when pci config space is accessed on a powered off device? Alex > > I think in this particular case (reading config space of a powered-off > device), CRS doesn't apply because the device is powered off, and > there's probably no delay: the read would probably complete > immediately because the link to the device is down, and the Root Port > or Downstream Port would probably generate an Unsupported Request > completion, which the Root Complex might handle by fabricating all 1s > data for the CPU. > > Bjorn _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs 2016-11-16 20:58 ` Bjorn Helgaas @ 2016-11-17 14:44 ` Lukas Wunner 2016-11-17 13:28 ` Alex Deucher 2016-11-17 14:44 ` Lukas Wunner 2 siblings, 0 replies; 41+ messages in thread From: Lukas Wunner @ 2016-11-17 14:44 UTC (permalink / raw) To: Bjorn Helgaas Cc: Daniel Vetter, Emil Velikov, dri-devel, Bjorn Helgaas, linux-pci, Greg KH, Sinan Kaya On Wed, Nov 16, 2016 at 02:58:11PM -0600, Bjorn Helgaas wrote: > On Mon, Nov 14, 2016 at 07:40:03PM +0100, Daniel Vetter wrote: > > On Fri, Nov 11, 2016 at 02:37:23PM +0000, Emil Velikov wrote: > > > From: Emil Velikov <emil.velikov@collabora.com> > > > > > > Currently the revision isn't available via sysfs/libudev thus if one > > > wants to know the value they need to read through the config file. > > > > > > This in itself wakes/powers up the device, causing unwanted delay > > > since it can be quite costly. > > > > > > There are at least two userspace components which could make use the new > > > file libpciaccess and libdrm. The former [used in various places] wakes > > > up _every_ PCI device, which can be observed via glxinfo [when using > > > Mesa 10.0+ drivers]. While the latter [in association with Mesa 13.0] > > > can lead to 2-3 second delays while starting firefox, thunderbird or > > > chromium. > > > > > > Expose the revision as a separate file, just like we do for the device, > > > vendor, their subsystem version and class. > > > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > Cc: linux-pci@vger.kernel.org > > > Cc: Greg KH <gregkh@linuxfoundation.org> > > > Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 > > > Tested-by: Mauro Santos <registo.mailling@gmail.com> > > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > > > Given that waking a gpu can take somewhere between ages and forever, and > > that we read the pci revisions everytime we launch a new gl app I think > > this is the correct approach. Of course we could just patch libdrm and > > everyone to not look at the pci revision, but that just leads to every > > pci-based driver having a driver-private ioctl/getparam thing to expose > > it. Which also doesn't make much sense. > > This re-asserts what has already been said, but doesn't address any of > my questions in the v2 discussion, so I'm still looking to continue > that thread. > > I am curious about this long wakeup issue, though. Are we talking > about a D3cold -> D0 transition? Yes, this is about a D3cold -> D0 transition, the bugzilla entry Emil linked talks about a dual GPU notebook. E.g. the Nvidia Kepler GPU in my MacBook Pro has 5 different power rails which must be brought up in a specific sequence (3.3V, 1.8V, 5V for the GPU, 1.35V for the video RAM and 1.05V for PCIe), something on the order of half a second to one second is reasonable for that. And the DRM driver needs a lot of time as well, half a second in the case of nouveau: [ 1500.780052] nouveau 0000:01:00.0: DRM: resuming kernel object tree... [ 1501.176616] nouveau 0000:01:00.0: DRM: resuming client object trees... [ 1501.176672] nouveau 0000:01:00.0: DRM: resuming display... [ 1501.298985] nouveau 0000:01:00.0: DRM: resuming console... There are no special PCIe things happening here. And since Sinan asked, these discrete GPUs usually aren't located behind a bridge, just a regular root port. Thanks, Lukas ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs @ 2016-11-17 14:44 ` Lukas Wunner 0 siblings, 0 replies; 41+ messages in thread From: Lukas Wunner @ 2016-11-17 14:44 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, Emil Velikov, dri-devel, Sinan Kaya, Greg KH, Bjorn Helgaas On Wed, Nov 16, 2016 at 02:58:11PM -0600, Bjorn Helgaas wrote: > On Mon, Nov 14, 2016 at 07:40:03PM +0100, Daniel Vetter wrote: > > On Fri, Nov 11, 2016 at 02:37:23PM +0000, Emil Velikov wrote: > > > From: Emil Velikov <emil.velikov@collabora.com> > > > > > > Currently the revision isn't available via sysfs/libudev thus if one > > > wants to know the value they need to read through the config file. > > > > > > This in itself wakes/powers up the device, causing unwanted delay > > > since it can be quite costly. > > > > > > There are at least two userspace components which could make use the new > > > file libpciaccess and libdrm. The former [used in various places] wakes > > > up _every_ PCI device, which can be observed via glxinfo [when using > > > Mesa 10.0+ drivers]. While the latter [in association with Mesa 13.0] > > > can lead to 2-3 second delays while starting firefox, thunderbird or > > > chromium. > > > > > > Expose the revision as a separate file, just like we do for the device, > > > vendor, their subsystem version and class. > > > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > Cc: linux-pci@vger.kernel.org > > > Cc: Greg KH <gregkh@linuxfoundation.org> > > > Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 > > > Tested-by: Mauro Santos <registo.mailling@gmail.com> > > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > > > Given that waking a gpu can take somewhere between ages and forever, and > > that we read the pci revisions everytime we launch a new gl app I think > > this is the correct approach. Of course we could just patch libdrm and > > everyone to not look at the pci revision, but that just leads to every > > pci-based driver having a driver-private ioctl/getparam thing to expose > > it. Which also doesn't make much sense. > > This re-asserts what has already been said, but doesn't address any of > my questions in the v2 discussion, so I'm still looking to continue > that thread. > > I am curious about this long wakeup issue, though. Are we talking > about a D3cold -> D0 transition? Yes, this is about a D3cold -> D0 transition, the bugzilla entry Emil linked talks about a dual GPU notebook. E.g. the Nvidia Kepler GPU in my MacBook Pro has 5 different power rails which must be brought up in a specific sequence (3.3V, 1.8V, 5V for the GPU, 1.35V for the video RAM and 1.05V for PCIe), something on the order of half a second to one second is reasonable for that. And the DRM driver needs a lot of time as well, half a second in the case of nouveau: [ 1500.780052] nouveau 0000:01:00.0: DRM: resuming kernel object tree... [ 1501.176616] nouveau 0000:01:00.0: DRM: resuming client object trees... [ 1501.176672] nouveau 0000:01:00.0: DRM: resuming display... [ 1501.298985] nouveau 0000:01:00.0: DRM: resuming console... There are no special PCIe things happening here. And since Sinan asked, these discrete GPUs usually aren't located behind a bridge, just a regular root port. Thanks, Lukas _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs 2016-11-17 14:44 ` Lukas Wunner (?) @ 2016-11-17 23:48 ` Bjorn Helgaas 2016-11-18 1:42 ` Michel Dänzer -1 siblings, 1 reply; 41+ messages in thread From: Bjorn Helgaas @ 2016-11-17 23:48 UTC (permalink / raw) To: Lukas Wunner Cc: Daniel Vetter, Emil Velikov, dri-devel, Bjorn Helgaas, linux-pci, Greg KH, Sinan Kaya On Thu, Nov 17, 2016 at 03:44:02PM +0100, Lukas Wunner wrote: > On Wed, Nov 16, 2016 at 02:58:11PM -0600, Bjorn Helgaas wrote: > > On Mon, Nov 14, 2016 at 07:40:03PM +0100, Daniel Vetter wrote: > > > On Fri, Nov 11, 2016 at 02:37:23PM +0000, Emil Velikov wrote: > > > > From: Emil Velikov <emil.velikov@collabora.com> > > > > > > > > Currently the revision isn't available via sysfs/libudev thus if one > > > > wants to know the value they need to read through the config file. > > > > > > > > This in itself wakes/powers up the device, causing unwanted delay > > > > since it can be quite costly. > > > > > > > > There are at least two userspace components which could make use the new > > > > file libpciaccess and libdrm. The former [used in various places] wakes > > > > up _every_ PCI device, which can be observed via glxinfo [when using > > > > Mesa 10.0+ drivers]. While the latter [in association with Mesa 13.0] > > > > can lead to 2-3 second delays while starting firefox, thunderbird or > > > > chromium. > > > > > > > > Expose the revision as a separate file, just like we do for the device, > > > > vendor, their subsystem version and class. > > > > > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > > Cc: linux-pci@vger.kernel.org > > > > Cc: Greg KH <gregkh@linuxfoundation.org> > > > > Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 > > > > Tested-by: Mauro Santos <registo.mailling@gmail.com> > > > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > > > > > Given that waking a gpu can take somewhere between ages and forever, and > > > that we read the pci revisions everytime we launch a new gl app I think > > > this is the correct approach. Of course we could just patch libdrm and > > > everyone to not look at the pci revision, but that just leads to every > > > pci-based driver having a driver-private ioctl/getparam thing to expose > > > it. Which also doesn't make much sense. > > > > This re-asserts what has already been said, but doesn't address any of > > my questions in the v2 discussion, so I'm still looking to continue > > that thread. > > > > I am curious about this long wakeup issue, though. Are we talking > > about a D3cold -> D0 transition? > > Yes, this is about a D3cold -> D0 transition, the bugzilla entry > Emil linked talks about a dual GPU notebook. > > E.g. the Nvidia Kepler GPU in my MacBook Pro has 5 different power > rails which must be brought up in a specific sequence (3.3V, 1.8V, 5V > for the GPU, 1.35V for the video RAM and 1.05V for PCIe), something > on the order of half a second to one second is reasonable for that. Ah, OK. That makes a lot more sense. Much of the time is actually for bringing up other power rails, so the PCIe part I've been considering is only the time between applying the 1.05V PCIe power and the config read, which is just a fraction of the whole powerup time. Popping the stack all the way back to Emil's Nov 8 message: When using the Mesa drivers alongside firefox [1] (since Mesa 13.0), glxinfo (Mesa 10.0) and others, all the GPUs* will be awaken, causing unwanted delays and increased power usage. [1] https://bugs.freedesktop.org/show_bug.cgi?id=98502 The bug is about a delay in starting firefox, thunderbird, or chromium. I assume the browser starts on the current, powered-up, GPU. I don't understand why we care about the revision of other, powered-off, GPUs. The fact that it's slow might be telling us "we need to optimize some code that is necessary but slow," or it might be telling us "we're doing some unnecessary work that we should stop doing." > And the DRM driver needs a lot of time as well, half a second in > the case of nouveau: > > [ 1500.780052] nouveau 0000:01:00.0: DRM: resuming kernel object tree... > [ 1501.176616] nouveau 0000:01:00.0: DRM: resuming client object trees... > [ 1501.176672] nouveau 0000:01:00.0: DRM: resuming display... > [ 1501.298985] nouveau 0000:01:00.0: DRM: resuming console... I assume this DRM time is unrelated to the sysfs "revision" file question. Bjorn ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs 2016-11-17 23:48 ` Bjorn Helgaas @ 2016-11-18 1:42 ` Michel Dänzer 0 siblings, 0 replies; 41+ messages in thread From: Michel Dänzer @ 2016-11-18 1:42 UTC (permalink / raw) To: Bjorn Helgaas, Lukas Wunner Cc: linux-pci, Emil Velikov, dri-devel, Sinan Kaya, Greg KH, Bjorn Helgaas On 18/11/16 08:48 AM, Bjorn Helgaas wrote: > > Popping the stack all the way back to Emil's Nov 8 message: > > When using the Mesa drivers alongside firefox [1] (since Mesa 13.0), > glxinfo (Mesa 10.0) and others, all the GPUs* will be awaken, > causing unwanted delays and increased power usage. > > [1] https://bugs.freedesktop.org/show_bug.cgi?id=98502 > > The bug is about a delay in starting firefox, thunderbird, or > chromium. I assume the browser starts on the current, powered-up, > GPU. I don't understand why we care about the revision of other, > powered-off, GPUs. We don't. The problem is that the current libdrm API unconditionally provides the revision. The plan is to address this in two ways: * Add new libdrm API which allows the caller to say "I don't need the revision", and make Mesa use that. Users having those changes will not run into the problem even on older kernels. * Add the separate revision file in sysfs and make libdrm use that for its current API. This means that even callers of the current libdrm API will not run into the problem with newer kernels. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs @ 2016-11-18 1:42 ` Michel Dänzer 0 siblings, 0 replies; 41+ messages in thread From: Michel Dänzer @ 2016-11-18 1:42 UTC (permalink / raw) To: Bjorn Helgaas, Lukas Wunner Cc: linux-pci, Emil Velikov, dri-devel, Sinan Kaya, Greg KH, Bjorn Helgaas On 18/11/16 08:48 AM, Bjorn Helgaas wrote: > > Popping the stack all the way back to Emil's Nov 8 message: > > When using the Mesa drivers alongside firefox [1] (since Mesa 13.0), > glxinfo (Mesa 10.0) and others, all the GPUs* will be awaken, > causing unwanted delays and increased power usage. > > [1] https://bugs.freedesktop.org/show_bug.cgi?id=98502 > > The bug is about a delay in starting firefox, thunderbird, or > chromium. I assume the browser starts on the current, powered-up, > GPU. I don't understand why we care about the revision of other, > powered-off, GPUs. We don't. The problem is that the current libdrm API unconditionally provides the revision. The plan is to address this in two ways: * Add new libdrm API which allows the caller to say "I don't need the revision", and make Mesa use that. Users having those changes will not run into the problem even on older kernels. * Add the separate revision file in sysfs and make libdrm use that for its current API. This means that even callers of the current libdrm API will not run into the problem with newer kernels. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs 2016-11-18 1:42 ` Michel Dänzer (?) @ 2016-11-18 2:40 ` Bjorn Helgaas 2016-11-18 9:42 ` Daniel Vetter -1 siblings, 1 reply; 41+ messages in thread From: Bjorn Helgaas @ 2016-11-18 2:40 UTC (permalink / raw) To: Michel Dänzer Cc: Lukas Wunner, linux-pci, Emil Velikov, dri-devel, Sinan Kaya, Greg KH, Bjorn Helgaas On Fri, Nov 18, 2016 at 10:42:20AM +0900, Michel Dänzer wrote: > On 18/11/16 08:48 AM, Bjorn Helgaas wrote: > > > > Popping the stack all the way back to Emil's Nov 8 message: > > > > When using the Mesa drivers alongside firefox [1] (since Mesa 13.0), > > glxinfo (Mesa 10.0) and others, all the GPUs* will be awaken, > > causing unwanted delays and increased power usage. > > > > [1] https://bugs.freedesktop.org/show_bug.cgi?id=98502 > > > > The bug is about a delay in starting firefox, thunderbird, or > > chromium. I assume the browser starts on the current, powered-up, > > GPU. I don't understand why we care about the revision of other, > > powered-off, GPUs. > > We don't. The problem is that the current libdrm API unconditionally > provides the revision. The plan is to address this in two ways: > > * Add new libdrm API which allows the caller to say "I don't need the > revision", and make Mesa use that. Users having those changes will not > run into the problem even on older kernels. > > * Add the separate revision file in sysfs and make libdrm use that for > its current API. This means that even callers of the current libdrm API > will not run into the problem with newer kernels. Why do we care about *anything* for the other, powered-off, GPUs? Even users of the new libdrm API who say "I don't need the revision" are still getting the vendor/device/etc for those other GPUs. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs 2016-11-18 2:40 ` Bjorn Helgaas @ 2016-11-18 9:42 ` Daniel Vetter 2016-11-18 9:48 ` Daniel Vetter 0 siblings, 1 reply; 41+ messages in thread From: Daniel Vetter @ 2016-11-18 9:42 UTC (permalink / raw) To: Bjorn Helgaas Cc: Michel Dänzer, linux-pci, Emil Velikov, dri-devel, Sinan Kaya, Greg KH, Bjorn Helgaas On Thu, Nov 17, 2016 at 08:40:10PM -0600, Bjorn Helgaas wrote: > On Fri, Nov 18, 2016 at 10:42:20AM +0900, Michel Dänzer wrote: > > On 18/11/16 08:48 AM, Bjorn Helgaas wrote: > > > > > > Popping the stack all the way back to Emil's Nov 8 message: > > > > > > When using the Mesa drivers alongside firefox [1] (since Mesa 13.0), > > > glxinfo (Mesa 10.0) and others, all the GPUs* will be awaken, > > > causing unwanted delays and increased power usage. > > > > > > [1] https://bugs.freedesktop.org/show_bug.cgi?id=98502 > > > > > > The bug is about a delay in starting firefox, thunderbird, or > > > chromium. I assume the browser starts on the current, powered-up, > > > GPU. I don't understand why we care about the revision of other, > > > powered-off, GPUs. > > > > We don't. The problem is that the current libdrm API unconditionally > > provides the revision. The plan is to address this in two ways: > > > > * Add new libdrm API which allows the caller to say "I don't need the > > revision", and make Mesa use that. Users having those changes will not > > run into the problem even on older kernels. > > > > * Add the separate revision file in sysfs and make libdrm use that for > > its current API. This means that even callers of the current libdrm API > > will not run into the problem with newer kernels. > > Why do we care about *anything* for the other, powered-off, GPUs? > Even users of the new libdrm API who say "I don't need the revision" > are still getting the vendor/device/etc for those other GPUs. egl device enumeration, and for that you need to know what gpus you have and load their drivers. Afaik. Yes by default they'll all select the online gpu, but they can opt for the faster/other one on multi-gpu machines. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs 2016-11-18 9:42 ` Daniel Vetter @ 2016-11-18 9:48 ` Daniel Vetter 2016-11-18 14:29 ` Bjorn Helgaas 2016-11-18 15:04 ` Alex Deucher 0 siblings, 2 replies; 41+ messages in thread From: Daniel Vetter @ 2016-11-18 9:48 UTC (permalink / raw) To: Bjorn Helgaas Cc: Michel Dänzer, linux-pci, Emil Velikov, dri-devel, Sinan Kaya, Greg KH, Bjorn Helgaas On Fri, Nov 18, 2016 at 10:42:07AM +0100, Daniel Vetter wrote: > On Thu, Nov 17, 2016 at 08:40:10PM -0600, Bjorn Helgaas wrote: > > On Fri, Nov 18, 2016 at 10:42:20AM +0900, Michel Dänzer wrote: > > > On 18/11/16 08:48 AM, Bjorn Helgaas wrote: > > > > > > > > Popping the stack all the way back to Emil's Nov 8 message: > > > > > > > > When using the Mesa drivers alongside firefox [1] (since Mesa 13.0), > > > > glxinfo (Mesa 10.0) and others, all the GPUs* will be awaken, > > > > causing unwanted delays and increased power usage. > > > > > > > > [1] https://bugs.freedesktop.org/show_bug.cgi?id=98502 > > > > > > > > The bug is about a delay in starting firefox, thunderbird, or > > > > chromium. I assume the browser starts on the current, powered-up, > > > > GPU. I don't understand why we care about the revision of other, > > > > powered-off, GPUs. > > > > > > We don't. The problem is that the current libdrm API unconditionally > > > provides the revision. The plan is to address this in two ways: > > > > > > * Add new libdrm API which allows the caller to say "I don't need the > > > revision", and make Mesa use that. Users having those changes will not > > > run into the problem even on older kernels. > > > > > > * Add the separate revision file in sysfs and make libdrm use that for > > > its current API. This means that even callers of the current libdrm API > > > will not run into the problem with newer kernels. > > > > Why do we care about *anything* for the other, powered-off, GPUs? > > Even users of the new libdrm API who say "I don't need the revision" > > are still getting the vendor/device/etc for those other GPUs. > > egl device enumeration, and for that you need to know what gpus you have > and load their drivers. Afaik. Yes by default they'll all select the > online gpu, but they can opt for the faster/other one on multi-gpu > machines. Now the only reason we can avoid parsing the revision flag is that most drivers don't care, and the one that does (i915) is generally the one that's on and only needs it after driver init. But fundamentally we can't avoid the enumeration, and imo it's better to cache this in the kernel than implement some userspace thingy (require udev or whatever). I guess in the end I don't really get why caching the revision in the kernel is a problem ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs 2016-11-18 9:48 ` Daniel Vetter @ 2016-11-18 14:29 ` Bjorn Helgaas 2016-11-18 15:04 ` Alex Deucher 1 sibling, 0 replies; 41+ messages in thread From: Bjorn Helgaas @ 2016-11-18 14:29 UTC (permalink / raw) To: Daniel Vetter Cc: Michel Dänzer, linux-pci, Emil Velikov, dri-devel, Sinan Kaya, Greg KH, Bjorn Helgaas On Fri, Nov 18, 2016 at 10:48:46AM +0100, Daniel Vetter wrote: > On Fri, Nov 18, 2016 at 10:42:07AM +0100, Daniel Vetter wrote: > > On Thu, Nov 17, 2016 at 08:40:10PM -0600, Bjorn Helgaas wrote: > > > On Fri, Nov 18, 2016 at 10:42:20AM +0900, Michel Dänzer wrote: > > > > On 18/11/16 08:48 AM, Bjorn Helgaas wrote: > > > > > > > > > > Popping the stack all the way back to Emil's Nov 8 message: > > > > > > > > > > When using the Mesa drivers alongside firefox [1] (since Mesa 13.0), > > > > > glxinfo (Mesa 10.0) and others, all the GPUs* will be awaken, > > > > > causing unwanted delays and increased power usage. > > > > > > > > > > [1] https://bugs.freedesktop.org/show_bug.cgi?id=98502 > > > > > > > > > > The bug is about a delay in starting firefox, thunderbird, or > > > > > chromium. I assume the browser starts on the current, powered-up, > > > > > GPU. I don't understand why we care about the revision of other, > > > > > powered-off, GPUs. > > > > > > > > We don't. The problem is that the current libdrm API unconditionally > > > > provides the revision. The plan is to address this in two ways: > > > > > > > > * Add new libdrm API which allows the caller to say "I don't need the > > > > revision", and make Mesa use that. Users having those changes will not > > > > run into the problem even on older kernels. > > > > > > > > * Add the separate revision file in sysfs and make libdrm use that for > > > > its current API. This means that even callers of the current libdrm API > > > > will not run into the problem with newer kernels. > > > > > > Why do we care about *anything* for the other, powered-off, GPUs? > > > Even users of the new libdrm API who say "I don't need the revision" > > > are still getting the vendor/device/etc for those other GPUs. > > > > egl device enumeration, and for that you need to know what gpus you have > > and load their drivers. Afaik. Yes by default they'll all select the > > online gpu, but they can opt for the faster/other one on multi-gpu > > machines. > > Now the only reason we can avoid parsing the revision flag is that most > drivers don't care, and the one that does (i915) is generally the one > that's on and only needs it after driver init. But fundamentally we can't > avoid the enumeration, and imo it's better to cache this in the kernel > than implement some userspace thingy (require udev or whatever). Sigh. AFAIK, this userspace enumeration is really unique to X, and it's been a PITA for years because it puts too much arch knowledge in userspace, e.g., PCI segment/domain knowledge, PCI BAR details, hotplug events, etc. I personally wouldn't object to udev or whatever, since that's the supported mechanism everybody else uses. > I guess in the end I don't really get why caching the revision in the > kernel is a problem ... It's not that it's such a problem, I just object to adding things to enable what seems like a broken design. But I guess I'm trying to boil the ocean here, and that energy would be better spent elsewhere, so I'll apply it. Bjorn ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs 2016-11-18 9:48 ` Daniel Vetter @ 2016-11-18 15:04 ` Alex Deucher 2016-11-18 15:04 ` Alex Deucher 1 sibling, 0 replies; 41+ messages in thread From: Alex Deucher @ 2016-11-18 15:04 UTC (permalink / raw) To: Daniel Vetter Cc: Bjorn Helgaas, linux-pci, Michel Dänzer, Emil Velikov, dri-devel, Sinan Kaya, Greg KH, Bjorn Helgaas On Fri, Nov 18, 2016 at 4:48 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Nov 18, 2016 at 10:42:07AM +0100, Daniel Vetter wrote: >> On Thu, Nov 17, 2016 at 08:40:10PM -0600, Bjorn Helgaas wrote: >> > On Fri, Nov 18, 2016 at 10:42:20AM +0900, Michel D=C3=A4nzer wrote: >> > > On 18/11/16 08:48 AM, Bjorn Helgaas wrote: >> > > > >> > > > Popping the stack all the way back to Emil's Nov 8 message: >> > > > >> > > > When using the Mesa drivers alongside firefox [1] (since Mesa 13= .0), >> > > > glxinfo (Mesa 10.0) and others, all the GPUs* will be awaken, >> > > > causing unwanted delays and increased power usage. >> > > > >> > > > [1] https://bugs.freedesktop.org/show_bug.cgi?id=3D98502 >> > > > >> > > > The bug is about a delay in starting firefox, thunderbird, or >> > > > chromium. I assume the browser starts on the current, powered-up, >> > > > GPU. I don't understand why we care about the revision of other, >> > > > powered-off, GPUs. >> > > >> > > We don't. The problem is that the current libdrm API unconditionally >> > > provides the revision. The plan is to address this in two ways: >> > > >> > > * Add new libdrm API which allows the caller to say "I don't need th= e >> > > revision", and make Mesa use that. Users having those changes will n= ot >> > > run into the problem even on older kernels. >> > > >> > > * Add the separate revision file in sysfs and make libdrm use that f= or >> > > its current API. This means that even callers of the current libdrm = API >> > > will not run into the problem with newer kernels. >> > >> > Why do we care about *anything* for the other, powered-off, GPUs? >> > Even users of the new libdrm API who say "I don't need the revision" >> > are still getting the vendor/device/etc for those other GPUs. >> >> egl device enumeration, and for that you need to know what gpus you have >> and load their drivers. Afaik. Yes by default they'll all select the >> online gpu, but they can opt for the faster/other one on multi-gpu >> machines. > > Now the only reason we can avoid parsing the revision flag is that most > drivers don't care, and the one that does (i915) is generally the one > that's on and only needs it after driver init. But fundamentally we can't > avoid the enumeration, and imo it's better to cache this in the kernel > than implement some userspace thingy (require udev or whatever). > We need the revision id as well. Alex > I guess in the end I don't really get why caching the revision in the > kernel is a problem ... > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs @ 2016-11-18 15:04 ` Alex Deucher 0 siblings, 0 replies; 41+ messages in thread From: Alex Deucher @ 2016-11-18 15:04 UTC (permalink / raw) To: Daniel Vetter Cc: linux-pci, Michel Dänzer, Emil Velikov, dri-devel, Sinan Kaya, Bjorn Helgaas, Greg KH, Bjorn Helgaas On Fri, Nov 18, 2016 at 4:48 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Nov 18, 2016 at 10:42:07AM +0100, Daniel Vetter wrote: >> On Thu, Nov 17, 2016 at 08:40:10PM -0600, Bjorn Helgaas wrote: >> > On Fri, Nov 18, 2016 at 10:42:20AM +0900, Michel Dänzer wrote: >> > > On 18/11/16 08:48 AM, Bjorn Helgaas wrote: >> > > > >> > > > Popping the stack all the way back to Emil's Nov 8 message: >> > > > >> > > > When using the Mesa drivers alongside firefox [1] (since Mesa 13.0), >> > > > glxinfo (Mesa 10.0) and others, all the GPUs* will be awaken, >> > > > causing unwanted delays and increased power usage. >> > > > >> > > > [1] https://bugs.freedesktop.org/show_bug.cgi?id=98502 >> > > > >> > > > The bug is about a delay in starting firefox, thunderbird, or >> > > > chromium. I assume the browser starts on the current, powered-up, >> > > > GPU. I don't understand why we care about the revision of other, >> > > > powered-off, GPUs. >> > > >> > > We don't. The problem is that the current libdrm API unconditionally >> > > provides the revision. The plan is to address this in two ways: >> > > >> > > * Add new libdrm API which allows the caller to say "I don't need the >> > > revision", and make Mesa use that. Users having those changes will not >> > > run into the problem even on older kernels. >> > > >> > > * Add the separate revision file in sysfs and make libdrm use that for >> > > its current API. This means that even callers of the current libdrm API >> > > will not run into the problem with newer kernels. >> > >> > Why do we care about *anything* for the other, powered-off, GPUs? >> > Even users of the new libdrm API who say "I don't need the revision" >> > are still getting the vendor/device/etc for those other GPUs. >> >> egl device enumeration, and for that you need to know what gpus you have >> and load their drivers. Afaik. Yes by default they'll all select the >> online gpu, but they can opt for the faster/other one on multi-gpu >> machines. > > Now the only reason we can avoid parsing the revision flag is that most > drivers don't care, and the one that does (i915) is generally the one > that's on and only needs it after driver init. But fundamentally we can't > avoid the enumeration, and imo it's better to cache this in the kernel > than implement some userspace thingy (require udev or whatever). > We need the revision id as well. Alex > I guess in the end I don't really get why caching the revision in the > kernel is a problem ... > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3] PCI: create revision file in sysfs 2016-11-11 14:37 ` [PATCH v3] " Emil Velikov 2016-11-14 18:40 ` Daniel Vetter @ 2016-11-18 19:06 ` Bjorn Helgaas 1 sibling, 0 replies; 41+ messages in thread From: Bjorn Helgaas @ 2016-11-18 19:06 UTC (permalink / raw) To: Emil Velikov; +Cc: dri-devel, Bjorn Helgaas, linux-pci, Greg KH On Fri, Nov 11, 2016 at 02:37:23PM +0000, Emil Velikov wrote: > From: Emil Velikov <emil.velikov@collabora.com> > > Currently the revision isn't available via sysfs/libudev thus if one > wants to know the value they need to read through the config file. > > This in itself wakes/powers up the device, causing unwanted delay > since it can be quite costly. > > There are at least two userspace components which could make use the new > file libpciaccess and libdrm. The former [used in various places] wakes > up _every_ PCI device, which can be observed via glxinfo [when using > Mesa 10.0+ drivers]. While the latter [in association with Mesa 13.0] > can lead to 2-3 second delays while starting firefox, thunderbird or > chromium. > > Expose the revision as a separate file, just like we do for the device, > vendor, their subsystem version and class. > > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: linux-pci@vger.kernel.org > Cc: Greg KH <gregkh@linuxfoundation.org> > Link: https://bugs.freedesktop.org/show_bug.cgi?id=98502 > Tested-by: Mauro Santos <registo.mailling@gmail.com> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> Applied with Daniel's reviewed-by to pci/misc for v4.10, thanks. > --- > v2: Add r-b/t-b tags, slim down CC list, add note about userspace. > > v3: Add Documentation/ bits (Greg KH) > > Gents, please keep me in the CC list. > > Thanks > Emil > --- > Documentation/ABI/testing/sysfs-bus-pci | 7 +++++++ > Documentation/filesystems/sysfs-pci.txt | 2 ++ > drivers/pci/pci-sysfs.c | 2 ++ > 3 files changed, 11 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci > index b3bc50f..5a1732b 100644 > --- a/Documentation/ABI/testing/sysfs-bus-pci > +++ b/Documentation/ABI/testing/sysfs-bus-pci > @@ -294,3 +294,10 @@ Description: > a firmware bug to the system vendor. Writing to this file > taints the kernel with TAINT_FIRMWARE_WORKAROUND, which > reduces the supportability of your system. > + > +What: /sys/bus/pci/devices/.../revision > +Date: November 2016 > +Contact: Emil Velikov <emil.l.velikov@gmail.com> > +Description: > + This file contains the revision field of the the PCI device. > + The value comes from device config space. The file is read only. > diff --git a/Documentation/filesystems/sysfs-pci.txt b/Documentation/filesystems/sysfs-pci.txt > index 74eaac2..6ea1ced 100644 > --- a/Documentation/filesystems/sysfs-pci.txt > +++ b/Documentation/filesystems/sysfs-pci.txt > @@ -17,6 +17,7 @@ that support it. For example, a given bus might look like this: > | |-- resource0 > | |-- resource1 > | |-- resource2 > + | |-- revision > | |-- rom > | |-- subsystem_device > | |-- subsystem_vendor > @@ -41,6 +42,7 @@ files, each with their own function. > resource PCI resource host addresses (ascii, ro) > resource0..N PCI resource N, if present (binary, mmap, rw[1]) > resource0_wc..N_wc PCI WC map resource N, if prefetchable (binary, mmap) > + revision PCI revision (ascii, ro) > rom PCI ROM resource, if present (binary, ro) > subsystem_device PCI subsystem device (ascii, ro) > subsystem_vendor PCI subsystem vendor (ascii, ro) > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index bcd10c7..0666287 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -50,6 +50,7 @@ pci_config_attr(vendor, "0x%04x\n"); > pci_config_attr(device, "0x%04x\n"); > pci_config_attr(subsystem_vendor, "0x%04x\n"); > pci_config_attr(subsystem_device, "0x%04x\n"); > +pci_config_attr(revision, "0x%02x\n"); > pci_config_attr(class, "0x%06x\n"); > pci_config_attr(irq, "%u\n"); > > @@ -568,6 +569,7 @@ static struct attribute *pci_dev_attrs[] = { > &dev_attr_device.attr, > &dev_attr_subsystem_vendor.attr, > &dev_attr_subsystem_device.attr, > + &dev_attr_revision.attr, > &dev_attr_class.attr, > &dev_attr_irq.attr, > &dev_attr_local_cpus.attr, > -- > 2.9.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2016-11-18 19:06 UTC | newest] Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-01 15:42 [PATCH] PCI: create revision file in sysfs Emil Velikov 2016-11-01 15:47 ` Alex Deucher 2016-11-01 15:47 ` Alex Deucher 2016-11-08 11:27 ` Emil Velikov 2016-11-08 11:27 ` Emil Velikov 2016-11-08 11:27 ` Emil Velikov 2016-11-09 16:56 ` [PATCH v2] " Emil Velikov 2016-11-09 16:56 ` Emil Velikov 2016-11-10 7:13 ` Greg KH 2016-11-10 7:13 ` Greg KH 2016-11-10 13:14 ` Emil Velikov 2016-11-10 23:59 ` Bjorn Helgaas 2016-11-11 0:31 ` Emil Velikov 2016-11-11 14:49 ` Bjorn Helgaas 2016-11-11 18:56 ` Emil Velikov 2016-11-11 18:56 ` Emil Velikov 2016-11-14 17:20 ` Bjorn Helgaas 2016-11-14 3:35 ` Michel Dänzer 2016-11-14 3:35 ` Michel Dänzer 2016-11-11 14:37 ` [PATCH v3] " Emil Velikov 2016-11-14 18:40 ` Daniel Vetter 2016-11-14 18:40 ` Daniel Vetter 2016-11-16 20:58 ` Bjorn Helgaas 2016-11-16 21:30 ` Sinan Kaya 2016-11-17 13:28 ` Alex Deucher 2016-11-17 13:28 ` Alex Deucher 2016-11-17 14:35 ` Bjorn Helgaas 2016-11-17 14:48 ` Alex Deucher 2016-11-17 14:48 ` Alex Deucher 2016-11-17 14:44 ` Lukas Wunner 2016-11-17 14:44 ` Lukas Wunner 2016-11-17 23:48 ` Bjorn Helgaas 2016-11-18 1:42 ` Michel Dänzer 2016-11-18 1:42 ` Michel Dänzer 2016-11-18 2:40 ` Bjorn Helgaas 2016-11-18 9:42 ` Daniel Vetter 2016-11-18 9:48 ` Daniel Vetter 2016-11-18 14:29 ` Bjorn Helgaas 2016-11-18 15:04 ` Alex Deucher 2016-11-18 15:04 ` Alex Deucher 2016-11-18 19:06 ` Bjorn Helgaas
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.