| Message ID | 20161003164317.15546-1-emil.l.velikov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/03/16 18:43, Emil Velikov wrote: > Earlier commit was removing all the users of drm_platform_set_busid and > removed the virtio hunk (which uses the PCI version of the function) by > mistake. > > This in itself resulted in user space receiving an incorrect value for > the busid, which in the case below lead to the failure to detect > the (correct) device and ultimately the X server failing to start. > > Fixes: a325725633c ("drm: Lobotomize set_busid nonsense for !pci > drivers") > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: stable@vger.kernel.org > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1366842 > Reported-by: Laszlo Ersek <lersek@redhat.com> > Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> > --- > Since I'm not 100% sure if we can get into .set_busid() as pci_dev is > NULL (for the virtio-vga 'vs' virgio-gpu-pci case) I've left the local > wrapper. > > Note: patch is against mainline (refs/tags/v4.8) but should apply for > drm-next/others. > --- > drivers/gpu/drm/virtio/virtgpu_drm_bus.c | 10 ++++++++++ > drivers/gpu/drm/virtio/virtgpu_drv.c | 1 + > drivers/gpu/drm/virtio/virtgpu_drv.h | 1 + > 3 files changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > index 7f0e93f87..88a3916 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > @@ -27,6 +27,16 @@ > > #include "virtgpu_drv.h" > > +int drm_virtio_set_busid(struct drm_device *dev, struct drm_master *master) > +{ > + struct pci_dev *pdev = dev->pdev; > + > + if (pdev) { > + return drm_pci_set_busid(dev, master); > + } > + return 0; > +} > + > static void virtio_pci_kick_out_firmware_fb(struct pci_dev *pci_dev) > { > struct apertures_struct *ap; > diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c > index c13f70c..5820b702 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_drv.c > +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c > @@ -117,6 +117,7 @@ static const struct file_operations virtio_gpu_driver_fops = { > > static struct drm_driver driver = { > .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME | DRIVER_RENDER | DRIVER_ATOMIC, > + .set_busid = drm_virtio_set_busid, > .load = virtio_gpu_driver_load, > .unload = virtio_gpu_driver_unload, > .open = virtio_gpu_driver_open, > diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h > index b18ef31..acf556a 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_drv.h > +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h > @@ -49,6 +49,7 @@ > #define DRIVER_PATCHLEVEL 1 > > /* virtgpu_drm_bus.c */ > +int drm_virtio_set_busid(struct drm_device *dev, struct drm_master *master); > int drm_virtio_init(struct drm_driver *driver, struct virtio_device *vdev); > > struct virtio_gpu_object { > I've come up with the identical patch (code-wise). I haven't posted it yet because: - I'd like to reproduce the original issue with the fresh 4.8 kernel, using the F24 config (so the build is taking forever :( :( :(), - I'd like to test the patch too. Obviously, testing your patch is just as good as testing my (identical) patch, it's just that I'll have to respond with a Tested-by. However, my commit message is different; what do you think of it? -------- commit a005c99587ce52b60cfae897071f124cc5867347 Author: Laszlo Ersek <lersek@redhat.com> Date: Mon Oct 3 17:59:14 2016 +0200 drm: virtio: reinstate drm_virtio_set_busid() Before commit a325725633c2 ("drm: Lobotomize set_busid nonsense for !pci drivers"), several DRM drivers for platform devices used to expose an explicit "drm_driver.set_busid" callback, invariably backed by drm_platform_set_busid(). Commit a325725633c2 removed drm_platform_set_busid(), along with the referring .set_busid field initializations. This was justified because interchangeable functionality had been implemented in drm_dev_alloc() / drm_dev_init(), which DRM_IOCTL_SET_VERSION would rely on going forward. However, commit a325725633c2 also removed drm_virtio_set_busid(), for which the same consolidation was not appropriate: this .set_busid callback had been implemented with drm_pci_set_busid(), and not drm_platform_set_busid(). The error regressed Xorg/xserver on QEMU's "virtio-vga" card; the drmGetBusid() function from libdrm would no longer return stable PCI identifiers like "pci:0000:00:02.0", but rather unstable platform ones like "virtio0". Reinstate drm_virtio_set_busid() with judicious use of git checkout -p a325725633c2^ -- drivers/gpu/drm/virtio Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: David Airlie <airlied@redhat.com> Cc: Emil Velikov <emil.l.velikov@gmail.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk> Cc: Hans de Goede <hdegoede@redhat.com> Cc: Joachim Frieben <jfrieben@hotmail.com> Cc: stable@vger.kernel.org Reported-by: Joachim Frieben <jfrieben@hotmail.com> Fixes: a325725633c26aa66ab940f762a6b0778edf76c0 Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1366842 Signed-off-by: Laszlo Ersek <lersek@redhat.com> drivers/gpu/drm/virtio/virtgpu_drm_bus.c | 10 ++++++++++ drivers/gpu/drm/virtio/virtgpu_drv.c | 1 + drivers/gpu/drm/virtio/virtgpu_drv.h | 1 + 3 files changed, 12 insertions(+) -------- I obviously don't "insist" that you take my commit message :), but you might want to scavenge it for some bits. You might not as well. :) Regarding the local wrapper around drm_pci_set_busid(): that's not because of virtio-vga vs. virtio-gpu-pci. Instead, it's due to the virtio-mmio vs. virtio-pci transports that are possible for virtio devices. Theoretically at least, a virtio-gpu device could be exposed by QEMU as a memory mapped device in arm/arm64 guests. In that case the "platform bus ID" would simply be the base address of the virtio-mmio register block. In practice, virtio-gpu over virtio-mmio would be an extremely outlandish configuration (noone does / should do that, plain and simple). Which is why implementing actual "platform bus ID" logic for the virtio-mmio transport case is unnecessary. It's just that the kernel shouldn't crash even if someone tries virtio-gpu over virtio-mmio. So, the wrapper function (with the pdev nullity check) should be preserved, even if not entirely for the reason you suspected. Anyway, my laptop seems to have ceased emitting sounds resembling a gas turbine, which I'll take as the completion of the 4.8 kernel build. Will be back soon with test results... Thanks! Laszlo
On 3 October 2016 at 18:08, Laszlo Ersek <lersek@redhat.com> wrote: > On 10/03/16 18:43, Emil Velikov wrote: >> Earlier commit was removing all the users of drm_platform_set_busid and >> removed the virtio hunk (which uses the PCI version of the function) by >> mistake. >> >> This in itself resulted in user space receiving an incorrect value for >> the busid, which in the case below lead to the failure to detect >> the (correct) device and ultimately the X server failing to start. >> >> Fixes: a325725633c ("drm: Lobotomize set_busid nonsense for !pci >> drivers") >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Cc: stable@vger.kernel.org >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1366842 >> Reported-by: Laszlo Ersek <lersek@redhat.com> >> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> >> --- >> Since I'm not 100% sure if we can get into .set_busid() as pci_dev is >> NULL (for the virtio-vga 'vs' virgio-gpu-pci case) I've left the local >> wrapper. >> >> Note: patch is against mainline (refs/tags/v4.8) but should apply for >> drm-next/others. >> --- >> drivers/gpu/drm/virtio/virtgpu_drm_bus.c | 10 ++++++++++ >> drivers/gpu/drm/virtio/virtgpu_drv.c | 1 + >> drivers/gpu/drm/virtio/virtgpu_drv.h | 1 + >> 3 files changed, 12 insertions(+) >> >> diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >> index 7f0e93f87..88a3916 100644 >> --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >> +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >> @@ -27,6 +27,16 @@ >> >> #include "virtgpu_drv.h" >> >> +int drm_virtio_set_busid(struct drm_device *dev, struct drm_master *master) >> +{ >> + struct pci_dev *pdev = dev->pdev; >> + >> + if (pdev) { >> + return drm_pci_set_busid(dev, master); >> + } >> + return 0; >> +} >> + >> static void virtio_pci_kick_out_firmware_fb(struct pci_dev *pci_dev) >> { >> struct apertures_struct *ap; >> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c >> index c13f70c..5820b702 100644 >> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c >> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c >> @@ -117,6 +117,7 @@ static const struct file_operations virtio_gpu_driver_fops = { >> >> static struct drm_driver driver = { >> .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME | DRIVER_RENDER | DRIVER_ATOMIC, >> + .set_busid = drm_virtio_set_busid, >> .load = virtio_gpu_driver_load, >> .unload = virtio_gpu_driver_unload, >> .open = virtio_gpu_driver_open, >> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h >> index b18ef31..acf556a 100644 >> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h >> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h >> @@ -49,6 +49,7 @@ >> #define DRIVER_PATCHLEVEL 1 >> >> /* virtgpu_drm_bus.c */ >> +int drm_virtio_set_busid(struct drm_device *dev, struct drm_master *master); >> int drm_virtio_init(struct drm_driver *driver, struct virtio_device *vdev); >> >> struct virtio_gpu_object { >> > > I've come up with the identical patch (code-wise). I haven't posted it yet because: > - I'd like to reproduce the original issue with the fresh 4.8 kernel, using the F24 config (so the build is taking forever :( :( :(), > - I'd like to test the patch too. > > Obviously, testing your patch is just as good as testing my (identical) patch, it's just that I'll have to respond with a Tested-by. > > However, my commit message is different; what do you think of it? > > -------- > commit a005c99587ce52b60cfae897071f124cc5867347 > Author: Laszlo Ersek <lersek@redhat.com> > Date: Mon Oct 3 17:59:14 2016 +0200 > > drm: virtio: reinstate drm_virtio_set_busid() > > Before commit a325725633c2 ("drm: Lobotomize set_busid nonsense for !pci > drivers"), several DRM drivers for platform devices used to expose an > explicit "drm_driver.set_busid" callback, invariably backed by > drm_platform_set_busid(). > > Commit a325725633c2 removed drm_platform_set_busid(), along with the > referring .set_busid field initializations. This was justified because > interchangeable functionality had been implemented in drm_dev_alloc() / > drm_dev_init(), which DRM_IOCTL_SET_VERSION would rely on going forward. > > However, commit a325725633c2 also removed drm_virtio_set_busid(), for > which the same consolidation was not appropriate: this .set_busid callback > had been implemented with drm_pci_set_busid(), and not > drm_platform_set_busid(). The error regressed Xorg/xserver on QEMU's > "virtio-vga" card; the drmGetBusid() function from libdrm would no longer > return stable PCI identifiers like "pci:0000:00:02.0", but rather unstable > platform ones like "virtio0". > > Reinstate drm_virtio_set_busid() with judicious use of > > git checkout -p a325725633c2^ -- drivers/gpu/drm/virtio > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: David Airlie <airlied@redhat.com> > Cc: Emil Velikov <emil.l.velikov@gmail.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Joachim Frieben <jfrieben@hotmail.com> > Cc: stable@vger.kernel.org > Reported-by: Joachim Frieben <jfrieben@hotmail.com> > Fixes: a325725633c26aa66ab940f762a6b0778edf76c0 > Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1366842 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > drivers/gpu/drm/virtio/virtgpu_drm_bus.c | 10 ++++++++++ > drivers/gpu/drm/virtio/virtgpu_drv.c | 1 + > drivers/gpu/drm/virtio/virtgpu_drv.h | 1 + > 3 files changed, 12 insertions(+) > -------- > > I obviously don't "insist" that you take my commit message :), but you might want to scavenge it for some bits. You might not as well. :) > Your commit message sounds a lot better, so I'd go with it (it even provides the exact same steps I used to revert the virtio-gpu hunks). Feel free to add: Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> > Regarding the local wrapper around drm_pci_set_busid(): that's not because of virtio-vga vs. virtio-gpu-pci. Instead, it's due to the virtio-mmio vs. virtio-pci transports that are possible for virtio devices. Theoretically at least, a virtio-gpu device could be exposed by QEMU as a memory mapped device in arm/arm64 guests. In that case the "platform bus ID" would simply be the base address of the virtio-mmio register block. > > In practice, virtio-gpu over virtio-mmio would be an extremely outlandish configuration (noone does / should do that, plain and simple). Which is why implementing actual "platform bus ID" logic for the virtio-mmio transport case is unnecessary. It's just that the kernel shouldn't crash even if someone tries virtio-gpu over virtio-mmio. > > So, the wrapper function (with the pdev nullity check) should be preserved, even if not entirely for the reason you suspected. > > Anyway, my laptop seems to have ceased emitting sounds resembling a gas turbine, which I'll take as the completion of the 4.8 kernel build. Will be back soon with test results... > Ack, my virtio terminology is a bit sub par ;-) Fwiw you could do a quick verification and hack a local libdrm.so LD_PRELOAD-ing it ;-) Either way, thanks for the report and for the well spotted thinko (/me still cannot believe he did not see it) Emil
On 10/03/16 19:08, Laszlo Ersek wrote: > On 10/03/16 18:43, Emil Velikov wrote: >> Earlier commit was removing all the users of drm_platform_set_busid and >> removed the virtio hunk (which uses the PCI version of the function) by >> mistake. >> >> This in itself resulted in user space receiving an incorrect value for >> the busid, which in the case below lead to the failure to detect >> the (correct) device and ultimately the X server failing to start. >> >> Fixes: a325725633c ("drm: Lobotomize set_busid nonsense for !pci >> drivers") >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Cc: stable@vger.kernel.org >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1366842 >> Reported-by: Laszlo Ersek <lersek@redhat.com> >> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> >> --- >> Since I'm not 100% sure if we can get into .set_busid() as pci_dev is >> NULL (for the virtio-vga 'vs' virgio-gpu-pci case) I've left the local >> wrapper. >> >> Note: patch is against mainline (refs/tags/v4.8) but should apply for >> drm-next/others. >> --- >> drivers/gpu/drm/virtio/virtgpu_drm_bus.c | 10 ++++++++++ >> drivers/gpu/drm/virtio/virtgpu_drv.c | 1 + >> drivers/gpu/drm/virtio/virtgpu_drv.h | 1 + >> 3 files changed, 12 insertions(+) >> >> diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >> index 7f0e93f87..88a3916 100644 >> --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >> +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >> @@ -27,6 +27,16 @@ >> >> #include "virtgpu_drv.h" >> >> +int drm_virtio_set_busid(struct drm_device *dev, struct drm_master *master) >> +{ >> + struct pci_dev *pdev = dev->pdev; >> + >> + if (pdev) { >> + return drm_pci_set_busid(dev, master); >> + } >> + return 0; >> +} >> + >> static void virtio_pci_kick_out_firmware_fb(struct pci_dev *pci_dev) >> { >> struct apertures_struct *ap; >> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c >> index c13f70c..5820b702 100644 >> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c >> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c >> @@ -117,6 +117,7 @@ static const struct file_operations virtio_gpu_driver_fops = { >> >> static struct drm_driver driver = { >> .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME | DRIVER_RENDER | DRIVER_ATOMIC, >> + .set_busid = drm_virtio_set_busid, >> .load = virtio_gpu_driver_load, >> .unload = virtio_gpu_driver_unload, >> .open = virtio_gpu_driver_open, >> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h >> index b18ef31..acf556a 100644 >> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h >> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h >> @@ -49,6 +49,7 @@ >> #define DRIVER_PATCHLEVEL 1 >> >> /* virtgpu_drm_bus.c */ >> +int drm_virtio_set_busid(struct drm_device *dev, struct drm_master *master); >> int drm_virtio_init(struct drm_driver *driver, struct virtio_device *vdev); >> >> struct virtio_gpu_object { >> > > I've come up with the identical patch (code-wise). I haven't posted it yet because: > - I'd like to reproduce the original issue with the fresh 4.8 kernel, using the F24 config (so the build is taking forever :( :( :(), > - I'd like to test the patch too. > > Obviously, testing your patch is just as good as testing my (identical) patch, it's just that I'll have to respond with a Tested-by. The patch works fine. Tested-by: Laszlo Ersek <lersek@redhat.com> Thanks! Laszlo > > However, my commit message is different; what do you think of it? > > -------- > commit a005c99587ce52b60cfae897071f124cc5867347 > Author: Laszlo Ersek <lersek@redhat.com> > Date: Mon Oct 3 17:59:14 2016 +0200 > > drm: virtio: reinstate drm_virtio_set_busid() > > Before commit a325725633c2 ("drm: Lobotomize set_busid nonsense for !pci > drivers"), several DRM drivers for platform devices used to expose an > explicit "drm_driver.set_busid" callback, invariably backed by > drm_platform_set_busid(). > > Commit a325725633c2 removed drm_platform_set_busid(), along with the > referring .set_busid field initializations. This was justified because > interchangeable functionality had been implemented in drm_dev_alloc() / > drm_dev_init(), which DRM_IOCTL_SET_VERSION would rely on going forward. > > However, commit a325725633c2 also removed drm_virtio_set_busid(), for > which the same consolidation was not appropriate: this .set_busid callback > had been implemented with drm_pci_set_busid(), and not > drm_platform_set_busid(). The error regressed Xorg/xserver on QEMU's > "virtio-vga" card; the drmGetBusid() function from libdrm would no longer > return stable PCI identifiers like "pci:0000:00:02.0", but rather unstable > platform ones like "virtio0". > > Reinstate drm_virtio_set_busid() with judicious use of > > git checkout -p a325725633c2^ -- drivers/gpu/drm/virtio > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: David Airlie <airlied@redhat.com> > Cc: Emil Velikov <emil.l.velikov@gmail.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Joachim Frieben <jfrieben@hotmail.com> > Cc: stable@vger.kernel.org > Reported-by: Joachim Frieben <jfrieben@hotmail.com> > Fixes: a325725633c26aa66ab940f762a6b0778edf76c0 > Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1366842 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > drivers/gpu/drm/virtio/virtgpu_drm_bus.c | 10 ++++++++++ > drivers/gpu/drm/virtio/virtgpu_drv.c | 1 + > drivers/gpu/drm/virtio/virtgpu_drv.h | 1 + > 3 files changed, 12 insertions(+) > -------- > > I obviously don't "insist" that you take my commit message :), but you might want to scavenge it for some bits. You might not as well. :) > > Regarding the local wrapper around drm_pci_set_busid(): that's not because of virtio-vga vs. virtio-gpu-pci. Instead, it's due to the virtio-mmio vs. virtio-pci transports that are possible for virtio devices. Theoretically at least, a virtio-gpu device could be exposed by QEMU as a memory mapped device in arm/arm64 guests. In that case the "platform bus ID" would simply be the base address of the virtio-mmio register block. > > In practice, virtio-gpu over virtio-mmio would be an extremely outlandish configuration (noone does / should do that, plain and simple). Which is why implementing actual "platform bus ID" logic for the virtio-mmio transport case is unnecessary. It's just that the kernel shouldn't crash even if someone tries virtio-gpu over virtio-mmio. > > So, the wrapper function (with the pdev nullity check) should be preserved, even if not entirely for the reason you suspected. > > Anyway, my laptop seems to have ceased emitting sounds resembling a gas turbine, which I'll take as the completion of the 4.8 kernel build. Will be back soon with test results... > > Thanks! > Laszlo >
On 10/03/16 19:28, Emil Velikov wrote: > On 3 October 2016 at 18:08, Laszlo Ersek <lersek@redhat.com> wrote: >> On 10/03/16 18:43, Emil Velikov wrote: >>> Earlier commit was removing all the users of drm_platform_set_busid and >>> removed the virtio hunk (which uses the PCI version of the function) by >>> mistake. >>> >>> This in itself resulted in user space receiving an incorrect value for >>> the busid, which in the case below lead to the failure to detect >>> the (correct) device and ultimately the X server failing to start. >>> >>> Fixes: a325725633c ("drm: Lobotomize set_busid nonsense for !pci >>> drivers") >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Cc: Laszlo Ersek <lersek@redhat.com> >>> Cc: stable@vger.kernel.org >>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1366842 >>> Reported-by: Laszlo Ersek <lersek@redhat.com> >>> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> >>> --- >>> Since I'm not 100% sure if we can get into .set_busid() as pci_dev is >>> NULL (for the virtio-vga 'vs' virgio-gpu-pci case) I've left the local >>> wrapper. >>> >>> Note: patch is against mainline (refs/tags/v4.8) but should apply for >>> drm-next/others. >>> --- >>> drivers/gpu/drm/virtio/virtgpu_drm_bus.c | 10 ++++++++++ >>> drivers/gpu/drm/virtio/virtgpu_drv.c | 1 + >>> drivers/gpu/drm/virtio/virtgpu_drv.h | 1 + >>> 3 files changed, 12 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >>> index 7f0e93f87..88a3916 100644 >>> --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >>> +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c >>> @@ -27,6 +27,16 @@ >>> >>> #include "virtgpu_drv.h" >>> >>> +int drm_virtio_set_busid(struct drm_device *dev, struct drm_master *master) >>> +{ >>> + struct pci_dev *pdev = dev->pdev; >>> + >>> + if (pdev) { >>> + return drm_pci_set_busid(dev, master); >>> + } >>> + return 0; >>> +} >>> + >>> static void virtio_pci_kick_out_firmware_fb(struct pci_dev *pci_dev) >>> { >>> struct apertures_struct *ap; >>> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c >>> index c13f70c..5820b702 100644 >>> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c >>> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c >>> @@ -117,6 +117,7 @@ static const struct file_operations virtio_gpu_driver_fops = { >>> >>> static struct drm_driver driver = { >>> .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME | DRIVER_RENDER | DRIVER_ATOMIC, >>> + .set_busid = drm_virtio_set_busid, >>> .load = virtio_gpu_driver_load, >>> .unload = virtio_gpu_driver_unload, >>> .open = virtio_gpu_driver_open, >>> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h >>> index b18ef31..acf556a 100644 >>> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h >>> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h >>> @@ -49,6 +49,7 @@ >>> #define DRIVER_PATCHLEVEL 1 >>> >>> /* virtgpu_drm_bus.c */ >>> +int drm_virtio_set_busid(struct drm_device *dev, struct drm_master *master); >>> int drm_virtio_init(struct drm_driver *driver, struct virtio_device *vdev); >>> >>> struct virtio_gpu_object { >>> >> >> I've come up with the identical patch (code-wise). I haven't posted it yet because: >> - I'd like to reproduce the original issue with the fresh 4.8 kernel, using the F24 config (so the build is taking forever :( :( :(), >> - I'd like to test the patch too. >> >> Obviously, testing your patch is just as good as testing my (identical) patch, it's just that I'll have to respond with a Tested-by. >> >> However, my commit message is different; what do you think of it? >> >> -------- >> commit a005c99587ce52b60cfae897071f124cc5867347 >> Author: Laszlo Ersek <lersek@redhat.com> >> Date: Mon Oct 3 17:59:14 2016 +0200 >> >> drm: virtio: reinstate drm_virtio_set_busid() >> >> Before commit a325725633c2 ("drm: Lobotomize set_busid nonsense for !pci >> drivers"), several DRM drivers for platform devices used to expose an >> explicit "drm_driver.set_busid" callback, invariably backed by >> drm_platform_set_busid(). >> >> Commit a325725633c2 removed drm_platform_set_busid(), along with the >> referring .set_busid field initializations. This was justified because >> interchangeable functionality had been implemented in drm_dev_alloc() / >> drm_dev_init(), which DRM_IOCTL_SET_VERSION would rely on going forward. >> >> However, commit a325725633c2 also removed drm_virtio_set_busid(), for >> which the same consolidation was not appropriate: this .set_busid callback >> had been implemented with drm_pci_set_busid(), and not >> drm_platform_set_busid(). The error regressed Xorg/xserver on QEMU's >> "virtio-vga" card; the drmGetBusid() function from libdrm would no longer >> return stable PCI identifiers like "pci:0000:00:02.0", but rather unstable >> platform ones like "virtio0". >> >> Reinstate drm_virtio_set_busid() with judicious use of >> >> git checkout -p a325725633c2^ -- drivers/gpu/drm/virtio >> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: Daniel Vetter <daniel.vetter@intel.com> >> Cc: David Airlie <airlied@redhat.com> >> Cc: Emil Velikov <emil.l.velikov@gmail.com> >> Cc: Gerd Hoffmann <kraxel@redhat.com> >> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >> Cc: Hans de Goede <hdegoede@redhat.com> >> Cc: Joachim Frieben <jfrieben@hotmail.com> >> Cc: stable@vger.kernel.org >> Reported-by: Joachim Frieben <jfrieben@hotmail.com> >> Fixes: a325725633c26aa66ab940f762a6b0778edf76c0 >> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1366842 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> >> drivers/gpu/drm/virtio/virtgpu_drm_bus.c | 10 ++++++++++ >> drivers/gpu/drm/virtio/virtgpu_drv.c | 1 + >> drivers/gpu/drm/virtio/virtgpu_drv.h | 1 + >> 3 files changed, 12 insertions(+) >> -------- >> >> I obviously don't "insist" that you take my commit message :), but you might want to scavenge it for some bits. You might not as well. :) >> > Your commit message sounds a lot better, so I'd go with it (it even > provides the exact same steps I used to revert the virtio-gpu hunks). > Feel free to add: > Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Thank you, I will post the patch in a minute! > >> Regarding the local wrapper around drm_pci_set_busid(): that's not because of virtio-vga vs. virtio-gpu-pci. Instead, it's due to the virtio-mmio vs. virtio-pci transports that are possible for virtio devices. Theoretically at least, a virtio-gpu device could be exposed by QEMU as a memory mapped device in arm/arm64 guests. In that case the "platform bus ID" would simply be the base address of the virtio-mmio register block. >> >> In practice, virtio-gpu over virtio-mmio would be an extremely outlandish configuration (noone does / should do that, plain and simple). Which is why implementing actual "platform bus ID" logic for the virtio-mmio transport case is unnecessary. It's just that the kernel shouldn't crash even if someone tries virtio-gpu over virtio-mmio. >> >> So, the wrapper function (with the pdev nullity check) should be preserved, even if not entirely for the reason you suspected. >> >> Anyway, my laptop seems to have ceased emitting sounds resembling a gas turbine, which I'll take as the completion of the 4.8 kernel build. Will be back soon with test results... >> > Ack, my virtio terminology is a bit sub par ;-) > Fwiw you could do a quick verification and hack a local libdrm.so > LD_PRELOAD-ing it ;-) > > Either way, thanks for the report and for the well spotted thinko (/me > still cannot believe he did not see it) Could be an argument against long and verbose identifiers in C! ;) Cheers, Laszlo
diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c index 7f0e93f87..88a3916 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c @@ -27,6 +27,16 @@ #include "virtgpu_drv.h" +int drm_virtio_set_busid(struct drm_device *dev, struct drm_master *master) +{ + struct pci_dev *pdev = dev->pdev; + + if (pdev) { + return drm_pci_set_busid(dev, master); + } + return 0; +} + static void virtio_pci_kick_out_firmware_fb(struct pci_dev *pci_dev) { struct apertures_struct *ap; diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c index c13f70c..5820b702 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.c +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c @@ -117,6 +117,7 @@ static const struct file_operations virtio_gpu_driver_fops = { static struct drm_driver driver = { .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME | DRIVER_RENDER | DRIVER_ATOMIC, + .set_busid = drm_virtio_set_busid, .load = virtio_gpu_driver_load, .unload = virtio_gpu_driver_unload, .open = virtio_gpu_driver_open, diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h index b18ef31..acf556a 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.h +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h @@ -49,6 +49,7 @@ #define DRIVER_PATCHLEVEL 1 /* virtgpu_drm_bus.c */ +int drm_virtio_set_busid(struct drm_device *dev, struct drm_master *master); int drm_virtio_init(struct drm_driver *driver, struct virtio_device *vdev); struct virtio_gpu_object {
Earlier commit was removing all the users of drm_platform_set_busid and removed the virtio hunk (which uses the PCI version of the function) by mistake. This in itself resulted in user space receiving an incorrect value for the busid, which in the case below lead to the failure to detect the (correct) device and ultimately the X server failing to start. Fixes: a325725633c ("drm: Lobotomize set_busid nonsense for !pci drivers") Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Laszlo Ersek <lersek@redhat.com> Cc: stable@vger.kernel.org Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1366842 Reported-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> --- Since I'm not 100% sure if we can get into .set_busid() as pci_dev is NULL (for the virtio-vga 'vs' virgio-gpu-pci case) I've left the local wrapper. Note: patch is against mainline (refs/tags/v4.8) but should apply for drm-next/others. --- drivers/gpu/drm/virtio/virtgpu_drm_bus.c | 10 ++++++++++ drivers/gpu/drm/virtio/virtgpu_drv.c | 1 + drivers/gpu/drm/virtio/virtgpu_drv.h | 1 + 3 files changed, 12 insertions(+)