| Message ID | 1475649297.25728.5.camel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 05, 2016 at 08:34:57AM +0200, Gerd Hoffmann wrote: > On Di, 2016-10-04 at 09:43 +0200, Gerd Hoffmann wrote: > > 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 ... > > Well, dev_name() returns a string without the "pci:" prefix, we have to > add that to make things actually work, like this: Hm right, I missed that in digging through the piles of layers of struct device naming. > > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > @@ -60,13 +60,22 @@ int drm_virtio_init(struct drm_driver *driver, > struct virtio_device *vdev) > > if (strcmp(vdev->dev.parent->bus->name, "pci") == 0) { > struct pci_dev *pdev = to_pci_dev(vdev->dev.parent); > + const char *pname = dev_name(&pdev->dev); > bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA; > + char unique[20]; Iirc we kfree this on unload. Needs to be a kasprintf I think. > > - DRM_INFO("pci: %s detected\n", > - vga ? "virtio-vga" : "virtio-gpu-pci"); > + DRM_INFO("pci: %s detected at %s\n", > + vga ? "virtio-vga" : "virtio-gpu-pci", > + pname); > dev->pdev = pdev; > if (vga) > virtio_pci_kick_out_firmware_fb(pdev); > + > + snprintf(unique, sizeof(unique), "pci:%s", pname); > + ret = drm_dev_set_unique(dev, unique); > + if (ret) > + goto err_free; > + I think once we can nuke the pci_setbusid stuff we might want a drm_dev_pci_set_unique, which takes the struct pci_device and wraps the above magic. But that can wait, we still have a few years on the uabi clock for that. Strictly speaking this is an issue for virtgpu too, but since virtgpu is newer than the last broken libdrm release I think we'll be fine. With the kasprintf change above: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > } > > ret = drm_dev_register(dev, 0); > > And a partial revert of commit a742946a1ba57e24e8be205ea87224c05b38c380 > to re-export drm_dev_set_unique(). > > Pushed to git://git.kraxel.org/linux drm-qemu > > cheers, > Gerd >
On Mi, 2016-10-05 at 12:30 +0200, Daniel Vetter wrote: > On Wed, Oct 05, 2016 at 08:34:57AM +0200, Gerd Hoffmann wrote: > > On Di, 2016-10-04 at 09:43 +0200, Gerd Hoffmann wrote: > > > 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 ... > > > > Well, dev_name() returns a string without the "pci:" prefix, we have to > > add that to make things actually work, like this: > > Hm right, I missed that in digging through the piles of layers of struct > device naming. > > > > > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > > @@ -60,13 +60,22 @@ int drm_virtio_init(struct drm_driver *driver, > > struct virtio_device *vdev) > > > > if (strcmp(vdev->dev.parent->bus->name, "pci") == 0) { > > struct pci_dev *pdev = to_pci_dev(vdev->dev.parent); > > + const char *pname = dev_name(&pdev->dev); > > bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA; > > + char unique[20]; > > Iirc we kfree this on unload. Needs to be a kasprintf I think. drm_dev_set_unique() does this: dev->unique = kstrdup(name, GFP_KERNEL); So it should not matter at all where the caller stores name. Or do you mean something else? cheers, Gerd
On Wed, Oct 05, 2016 at 02:43:37PM +0200, Gerd Hoffmann wrote: > On Mi, 2016-10-05 at 12:30 +0200, Daniel Vetter wrote: > > On Wed, Oct 05, 2016 at 08:34:57AM +0200, Gerd Hoffmann wrote: > > > On Di, 2016-10-04 at 09:43 +0200, Gerd Hoffmann wrote: > > > > 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 ... > > > > > > Well, dev_name() returns a string without the "pci:" prefix, we have to > > > add that to make things actually work, like this: > > > > Hm right, I missed that in digging through the piles of layers of struct > > device naming. > > > > > > > > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > > > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > > > @@ -60,13 +60,22 @@ int drm_virtio_init(struct drm_driver *driver, > > > struct virtio_device *vdev) > > > > > > if (strcmp(vdev->dev.parent->bus->name, "pci") == 0) { > > > struct pci_dev *pdev = to_pci_dev(vdev->dev.parent); > > > + const char *pname = dev_name(&pdev->dev); > > > bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA; > > > + char unique[20]; > > > > Iirc we kfree this on unload. Needs to be a kasprintf I think. > > drm_dev_set_unique() does this: > > dev->unique = kstrdup(name, GFP_KERNEL); > > So it should not matter at all where the caller stores name. > Or do you mean something else? Oh, missed that. r-b stands as-is. -Daniel
--- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c @@ -60,13 +60,22 @@ int drm_virtio_init(struct drm_driver *driver, struct virtio_device *vdev) if (strcmp(vdev->dev.parent->bus->name, "pci") == 0) { struct pci_dev *pdev = to_pci_dev(vdev->dev.parent); + const char *pname = dev_name(&pdev->dev); bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA; + char unique[20]; - DRM_INFO("pci: %s detected\n", - vga ? "virtio-vga" : "virtio-gpu-pci"); + DRM_INFO("pci: %s detected at %s\n", + vga ? "virtio-vga" : "virtio-gpu-pci", + pname); dev->pdev = pdev; if (vga) virtio_pci_kick_out_firmware_fb(pdev); + + snprintf(unique, sizeof(unique), "pci:%s", pname); + ret = drm_dev_set_unique(dev, unique); + if (ret) + goto err_free; + }