| Message ID | CAKMK7uFVk=iVQ=cN=dQjGtf_y2FhZJsdzdQ+puGVPyZ1bex_8w@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/03/16 21:12, Daniel Vetter wrote: > On Mon, Oct 3, 2016 at 4:15 PM, Laszlo Ersek <lersek@redhat.com> wrote: >> With kernel commit a325725633c2 applied, the drmGetBusid() call in >> get_drm_info() [hw/xfree86/os-support/linux/lnx_platform.c] returns >> "virtio0". >> >> Without kernel commit a325725633c2 in place, the same function call >> produces "pci:0000:00:02.0". >> >> I think that the idea of kernel commit a325725633c2: >> >> We already have a fallback in place to fill out the unique from >> dev->unique, which is set to something reasonable in drm_dev_alloc. >> >> is inappropriate for virtio devices. >> >> While it is true that "virtioN" is unique across the guest system, those >> identifiers are not stable. >> >> # ls -1d /sys/devices/pci*/*/virtio* >> /sys/devices/pci0000:00/0000:00:02.0/virtio0 >> /sys/devices/pci0000:00/0000:00:03.0/virtio1 >> /sys/devices/pci0000:00/0000:00:05.0/virtio2 >> /sys/devices/pci0000:00/0000:00:06.0/virtio3 >> /sys/devices/pci0000:00/0000:00:09.0/virtio4 >> >> If you tweak the PCI addresses of other virtio devices on the QEMU >> command line, while keeping the PCI address of the virtio-vga device >> intact, the "virtioN" identifier of the virtio-vga device may change in >> an unspecified / unexpected manner. >> >> From the above listing, it seems like higher PCI Device numbers happen >> to end up with higher "virtioN" identifiers. While this is unspecified / >> non-guaranteed behavior, in practice it means the following: >> >> Moving the "virtio1" device from PCI address 00:03.0 to 00:01.0, while >> preserving the PCI address of virtio-vga as 00:02.0, will bump >> virtio-vga to "virtio1" from "virtio0" (and sink that other device from >> "virtio1" to "virtio0"). >> >> I think that unstable identifiers don't qualify for BusID use, >> regardless of the actual format of the IDs in question. >> >> Searching the patch quickly, drm_virtio_set_busid() is the only >> implementation of the "drm_driver.set_busid" callback that delegates the >> task to drm_pci_set_busid(). All other implementations of this callback >> were provided by drm_platform_set_busid(). >> >> drm_platform_set_busid() would ultimately format >> "dev->platformdev->name" and "dev->platformdev->id" into the bus >> identifier. Replacing that common logic with the drm_dev_alloc() >> fallback that is already in place is probably justified: judging from >> "virtio0", the fallback likely captures the exact same information, just >> with a different format. >> >> However, drm_virtio_set_busid() used to implement a different logic >> (encoding different information). It intentionally used stable PCI >> addresses rather than (platformdev->name, platformdev->id). >> >> So, I think that removing drm_virtio_set_busid() was an actual >> regression. I propose that the related hunks of a325725633c2 be >> reverted. For virtio-vga and virtio-gpu-pci, "virtioN" is an unstable >> and inappropraite BusID pattern. > > Jumping in a bit late here, and maybe I'm not coherent (flu and all > that), but I'd still like to nuke the set_busid callback for virtio. > The trouble here seems to be that virtio has a bit a confusion about > what it considers to be the parent device it wants to expose to > userspace. And userspace is less than clueful about walking up the > device hierarchy to figure this out on its own. > > Anyway looking at drm_virtio_init in virtgpu_drm_bus.c we already have > a special case for when we're on a PCI bus. I think the better way to > fix this issue would be to: > - again export drm_drv_set_unique to drivers > - overwrite drm's choice of unique identifier with a call to > > drm_dev_set_unique(dev, dev->pdev); > > in that if block. With a very big comment that this only exists for > backwards compat with userspace's expectations. > > Since virtio is newer than drm1.1 we don't need to care about the > backwards compat cruft in the pci set_busid function, and it would be > great to restrict that as much as possible imo. Something like the > below essentially (entirely untested): > > diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > index a59d0e309bfc..1fcf739bf509 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver, > struct virtio_device *vdev) > dev->pdev = pdev; > if (vga) > virtio_pci_kick_out_firmware_fb(pdev); > + > + ret = drm_dev_set_unique(dev, dev_name(dev->pdev)); > + if (ret) > + goto err_free; > } > > ret = drm_dev_register(dev, 0); > > Cheers, Daniel On the flu front: thank you for taking the time to think about this and to respond despite being ill. Get better! On the patch front: normally I wouldn't oppose experimentation and shooting untested patches to users for testing -- that's okay in my opinion. However, in this case the original patch regressed userspace *first*; we know the culprit hunks, and those hunks can be very easily reverted without dropping the entire patch. Considering Emil's feedback, he wouldn't have let the patch (a325725633c2) through review if he had noticed that hunk -- similarly to the amdgpu hunk, which he did notice, in v3. I think that experimentation, and asking users to test on (virtual) hardware that is not easily available to the patch submitter, are all fine -- as long as the public tree is in working shape. Currently, it is not; with the above in mind, I strongly prefer the (partial) revert. If you'd like to slay the remaining drm_pci_set_busid() calls, which affect both virtio-gpu and amdgpu, that could be an entirely justified change, a 100% improvement, and I'll be more than happy to assist with testing that (the virtio-gpu case at least) -- *after* the tree is returned to working shape (including stable), now that we're able to do that. Personally, I'm not a DRM "expert" by any means -- this is actually the first thread on dri-devel that I've even read --, and this is about the time I can squeeze out right now for this issue. I should help with the testing as stated above, but we also should have a more convenient schedule for that testing. Thanks Laszlo
On Mon, Oct 3, 2016 at 9:36 PM, Laszlo Ersek <lersek@redhat.com> wrote: > On 10/03/16 21:12, Daniel Vetter wrote: >> On Mon, Oct 3, 2016 at 4:15 PM, Laszlo Ersek <lersek@redhat.com> wrote: >>> With kernel commit a325725633c2 applied, the drmGetBusid() call in >>> get_drm_info() [hw/xfree86/os-support/linux/lnx_platform.c] returns >>> "virtio0". >>> >>> Without kernel commit a325725633c2 in place, the same function call >>> produces "pci:0000:00:02.0". >>> >>> I think that the idea of kernel commit a325725633c2: >>> >>> We already have a fallback in place to fill out the unique from >>> dev->unique, which is set to something reasonable in drm_dev_alloc. >>> >>> is inappropriate for virtio devices. >>> >>> While it is true that "virtioN" is unique across the guest system, those >>> identifiers are not stable. >>> >>> # ls -1d /sys/devices/pci*/*/virtio* >>> /sys/devices/pci0000:00/0000:00:02.0/virtio0 >>> /sys/devices/pci0000:00/0000:00:03.0/virtio1 >>> /sys/devices/pci0000:00/0000:00:05.0/virtio2 >>> /sys/devices/pci0000:00/0000:00:06.0/virtio3 >>> /sys/devices/pci0000:00/0000:00:09.0/virtio4 >>> >>> If you tweak the PCI addresses of other virtio devices on the QEMU >>> command line, while keeping the PCI address of the virtio-vga device >>> intact, the "virtioN" identifier of the virtio-vga device may change in >>> an unspecified / unexpected manner. >>> >>> From the above listing, it seems like higher PCI Device numbers happen >>> to end up with higher "virtioN" identifiers. While this is unspecified / >>> non-guaranteed behavior, in practice it means the following: >>> >>> Moving the "virtio1" device from PCI address 00:03.0 to 00:01.0, while >>> preserving the PCI address of virtio-vga as 00:02.0, will bump >>> virtio-vga to "virtio1" from "virtio0" (and sink that other device from >>> "virtio1" to "virtio0"). >>> >>> I think that unstable identifiers don't qualify for BusID use, >>> regardless of the actual format of the IDs in question. >>> >>> Searching the patch quickly, drm_virtio_set_busid() is the only >>> implementation of the "drm_driver.set_busid" callback that delegates the >>> task to drm_pci_set_busid(). All other implementations of this callback >>> were provided by drm_platform_set_busid(). >>> >>> drm_platform_set_busid() would ultimately format >>> "dev->platformdev->name" and "dev->platformdev->id" into the bus >>> identifier. Replacing that common logic with the drm_dev_alloc() >>> fallback that is already in place is probably justified: judging from >>> "virtio0", the fallback likely captures the exact same information, just >>> with a different format. >>> >>> However, drm_virtio_set_busid() used to implement a different logic >>> (encoding different information). It intentionally used stable PCI >>> addresses rather than (platformdev->name, platformdev->id). >>> >>> So, I think that removing drm_virtio_set_busid() was an actual >>> regression. I propose that the related hunks of a325725633c2 be >>> reverted. For virtio-vga and virtio-gpu-pci, "virtioN" is an unstable >>> and inappropraite BusID pattern. >> >> Jumping in a bit late here, and maybe I'm not coherent (flu and all >> that), but I'd still like to nuke the set_busid callback for virtio. >> The trouble here seems to be that virtio has a bit a confusion about >> what it considers to be the parent device it wants to expose to >> userspace. And userspace is less than clueful about walking up the >> device hierarchy to figure this out on its own. >> >> Anyway looking at drm_virtio_init in virtgpu_drm_bus.c we already have >> a special case for when we're on a PCI bus. I think the better way to >> fix this issue would be to: >> - again export drm_drv_set_unique to drivers >> - overwrite drm's choice of unique identifier with a call to >> >> drm_dev_set_unique(dev, dev->pdev); >> >> in that if block. With a very big comment that this only exists for >> backwards compat with userspace's expectations. >> >> Since virtio is newer than drm1.1 we don't need to care about the >> backwards compat cruft in the pci set_busid function, and it would be >> great to restrict that as much as possible imo. Something like the >> below essentially (entirely untested): >> >> diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >> b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >> index a59d0e309bfc..1fcf739bf509 100644 >> --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >> +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >> @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver, >> struct virtio_device *vdev) >> dev->pdev = pdev; >> if (vga) >> virtio_pci_kick_out_firmware_fb(pdev); >> + >> + ret = drm_dev_set_unique(dev, dev_name(dev->pdev)); >> + if (ret) >> + goto err_free; >> } >> >> ret = drm_dev_register(dev, 0); >> >> Cheers, Daniel > > > On the flu front: thank you for taking the time to think about this and > to respond despite being ill. Get better! > > On the patch front: normally I wouldn't oppose experimentation and > shooting untested patches to users for testing -- that's okay in my > opinion. However, in this case the original patch regressed userspace > *first*; we know the culprit hunks, and those hunks can be very easily > reverted without dropping the entire patch. > > Considering Emil's feedback, he wouldn't have let the patch > (a325725633c2) through review if he had noticed that hunk -- similarly > to the amdgpu hunk, which he did notice, in v3. > > I think that experimentation, and asking users to test on (virtual) > hardware that is not easily available to the patch submitter, are all > fine -- as long as the public tree is in working shape. Currently, it is > not; with the above in mind, I strongly prefer the (partial) revert. > > If you'd like to slay the remaining drm_pci_set_busid() calls, which > affect both virtio-gpu and amdgpu, that could be an entirely justified > change, a 100% improvement, and I'll be more than happy to assist with > testing that (the virtio-gpu case at least) -- *after* the tree is > returned to working shape (including stable), now that we're able to do > that. > > Personally, I'm not a DRM "expert" by any means -- this is actually the > first thread on dri-devel that I've even read --, and this is about the > time I can squeeze out right now for this issue. I should help with the > testing as stated above, but we also should have a more convenient > schedule for that testing. I didn't say I'm blocking your fix for this, nor did I want to imply that. It's just been my experience that bug reporters tend to disappear fast. So if you have time, please test the above snippet (without your patch of course). If you don't, no harm done, since I've been trying to untangle the busid mess since years, so a few more won't matter ;-) -Daniel
On 10/03/16 22:34, Daniel Vetter wrote: > On Mon, Oct 3, 2016 at 9:36 PM, Laszlo Ersek <lersek@redhat.com> wrote: >> On 10/03/16 21:12, Daniel Vetter wrote: >>> On Mon, Oct 3, 2016 at 4:15 PM, Laszlo Ersek <lersek@redhat.com> wrote: >>>> With kernel commit a325725633c2 applied, the drmGetBusid() call in >>>> get_drm_info() [hw/xfree86/os-support/linux/lnx_platform.c] returns >>>> "virtio0". >>>> >>>> Without kernel commit a325725633c2 in place, the same function call >>>> produces "pci:0000:00:02.0". >>>> >>>> I think that the idea of kernel commit a325725633c2: >>>> >>>> We already have a fallback in place to fill out the unique from >>>> dev->unique, which is set to something reasonable in drm_dev_alloc. >>>> >>>> is inappropriate for virtio devices. >>>> >>>> While it is true that "virtioN" is unique across the guest system, those >>>> identifiers are not stable. >>>> >>>> # ls -1d /sys/devices/pci*/*/virtio* >>>> /sys/devices/pci0000:00/0000:00:02.0/virtio0 >>>> /sys/devices/pci0000:00/0000:00:03.0/virtio1 >>>> /sys/devices/pci0000:00/0000:00:05.0/virtio2 >>>> /sys/devices/pci0000:00/0000:00:06.0/virtio3 >>>> /sys/devices/pci0000:00/0000:00:09.0/virtio4 >>>> >>>> If you tweak the PCI addresses of other virtio devices on the QEMU >>>> command line, while keeping the PCI address of the virtio-vga device >>>> intact, the "virtioN" identifier of the virtio-vga device may change in >>>> an unspecified / unexpected manner. >>>> >>>> From the above listing, it seems like higher PCI Device numbers happen >>>> to end up with higher "virtioN" identifiers. While this is unspecified / >>>> non-guaranteed behavior, in practice it means the following: >>>> >>>> Moving the "virtio1" device from PCI address 00:03.0 to 00:01.0, while >>>> preserving the PCI address of virtio-vga as 00:02.0, will bump >>>> virtio-vga to "virtio1" from "virtio0" (and sink that other device from >>>> "virtio1" to "virtio0"). >>>> >>>> I think that unstable identifiers don't qualify for BusID use, >>>> regardless of the actual format of the IDs in question. >>>> >>>> Searching the patch quickly, drm_virtio_set_busid() is the only >>>> implementation of the "drm_driver.set_busid" callback that delegates the >>>> task to drm_pci_set_busid(). All other implementations of this callback >>>> were provided by drm_platform_set_busid(). >>>> >>>> drm_platform_set_busid() would ultimately format >>>> "dev->platformdev->name" and "dev->platformdev->id" into the bus >>>> identifier. Replacing that common logic with the drm_dev_alloc() >>>> fallback that is already in place is probably justified: judging from >>>> "virtio0", the fallback likely captures the exact same information, just >>>> with a different format. >>>> >>>> However, drm_virtio_set_busid() used to implement a different logic >>>> (encoding different information). It intentionally used stable PCI >>>> addresses rather than (platformdev->name, platformdev->id). >>>> >>>> So, I think that removing drm_virtio_set_busid() was an actual >>>> regression. I propose that the related hunks of a325725633c2 be >>>> reverted. For virtio-vga and virtio-gpu-pci, "virtioN" is an unstable >>>> and inappropraite BusID pattern. >>> >>> Jumping in a bit late here, and maybe I'm not coherent (flu and all >>> that), but I'd still like to nuke the set_busid callback for virtio. >>> The trouble here seems to be that virtio has a bit a confusion about >>> what it considers to be the parent device it wants to expose to >>> userspace. And userspace is less than clueful about walking up the >>> device hierarchy to figure this out on its own. >>> >>> Anyway looking at drm_virtio_init in virtgpu_drm_bus.c we already have >>> a special case for when we're on a PCI bus. I think the better way to >>> fix this issue would be to: >>> - again export drm_drv_set_unique to drivers >>> - overwrite drm's choice of unique identifier with a call to >>> >>> drm_dev_set_unique(dev, dev->pdev); >>> >>> in that if block. With a very big comment that this only exists for >>> backwards compat with userspace's expectations. >>> >>> Since virtio is newer than drm1.1 we don't need to care about the >>> backwards compat cruft in the pci set_busid function, and it would be >>> great to restrict that as much as possible imo. Something like the >>> below essentially (entirely untested): >>> >>> diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >>> b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >>> index a59d0e309bfc..1fcf739bf509 100644 >>> --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >>> +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >>> @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver, >>> struct virtio_device *vdev) >>> dev->pdev = pdev; >>> if (vga) >>> virtio_pci_kick_out_firmware_fb(pdev); >>> + >>> + ret = drm_dev_set_unique(dev, dev_name(dev->pdev)); >>> + if (ret) >>> + goto err_free; >>> } >>> >>> ret = drm_dev_register(dev, 0); >>> >>> Cheers, Daniel >> >> >> On the flu front: thank you for taking the time to think about this and >> to respond despite being ill. Get better! >> >> On the patch front: normally I wouldn't oppose experimentation and >> shooting untested patches to users for testing -- that's okay in my >> opinion. However, in this case the original patch regressed userspace >> *first*; we know the culprit hunks, and those hunks can be very easily >> reverted without dropping the entire patch. >> >> Considering Emil's feedback, he wouldn't have let the patch >> (a325725633c2) through review if he had noticed that hunk -- similarly >> to the amdgpu hunk, which he did notice, in v3. >> >> I think that experimentation, and asking users to test on (virtual) >> hardware that is not easily available to the patch submitter, are all >> fine -- as long as the public tree is in working shape. Currently, it is >> not; with the above in mind, I strongly prefer the (partial) revert. >> >> If you'd like to slay the remaining drm_pci_set_busid() calls, which >> affect both virtio-gpu and amdgpu, that could be an entirely justified >> change, a 100% improvement, and I'll be more than happy to assist with >> testing that (the virtio-gpu case at least) -- *after* the tree is >> returned to working shape (including stable), now that we're able to do >> that. >> >> Personally, I'm not a DRM "expert" by any means -- this is actually the >> first thread on dri-devel that I've even read --, and this is about the >> time I can squeeze out right now for this issue. I should help with the >> testing as stated above, but we also should have a more convenient >> schedule for that testing. > > I didn't say I'm blocking your fix for this, nor did I want to imply > that. It's just been my experience that bug reporters tend to > disappear fast. Valid experience. Doesn't apply to me. > So if you have time, please test the above snippet > (without your patch of course). If you don't, no harm done, since I've > been trying to untangle the busid mess since years, so a few more > won't matter ;-) Can you please send the patch in a form that I can apply with git-am, and ensure that it will build? (I assume "entirely untested" implies "not build tested".) I think a patch attachment on-list, or an inline patch sent to me privately should be fine (so as not to confuse subscribers and/or any patch bot / patchwork instance scraping the list). I guess a threaded message with a subject-prefix different from [PATCH] might work as well. (It's not that I'm too lazy to re-code your line-wrapped patch manually -- instead, you want me to interfere with your patch before testing it as little as possible. Git-am or even a fetchable remote are best for that.) Thanks! Laszlo
Hi, > diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > index a59d0e309bfc..1fcf739bf509 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver, > struct virtio_device *vdev) > dev->pdev = pdev; > if (vga) > virtio_pci_kick_out_firmware_fb(pdev); > + > + ret = drm_dev_set_unique(dev, dev_name(dev->pdev)); > + if (ret) > + goto err_free; > } Approach looks sensible to me, I'll give it a try ... cheers, Gerd
diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c index a59d0e309bfc..1fcf739bf509 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c @@ -68,6 +68,10 @@ int drm_virtio_init(struct drm_driver *driver, struct virtio_device *vdev) dev->pdev = pdev; if (vga) virtio_pci_kick_out_firmware_fb(pdev); + + ret = drm_dev_set_unique(dev, dev_name(dev->pdev)); + if (ret) + goto err_free; }