On 10/23/18 18:35, Emil Velikov wrote: > From: Emil Velikov <emil.velikov@xxxxxxxxxxxxx> > > A while back we removed it, yet that lead to regressions. At some later > point, I've attempted to remove it again without fully grasping the > unique (pun intended) situation that virtio is in. pun appreciated :) > > Add a bulky comment to document any the call should stay as-is, for the > next person who's around. s/any/why/? > > As a Tl;Dr: virtio sits on top of struct virtio_device, which confuses > dev_is_pci(), wrong info gets sent to userspace and X doesn't start. > Driver needs to explicitly call drm_dev_set_unique() to keep it working. > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> > Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx> > Cc: Hans de Goede <hdegoede@xxxxxxxxxx> > Cc: Laszlo Ersek <lersek@xxxxxxxxxx> > Signed-off-by: Emil Velikov <emil.velikov@xxxxxxxxxxxxx> > --- > drivers/gpu/drm/virtio/virtgpu_drm_bus.c | 31 ++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > index 757ca28ab93e..54f12b862231 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > +++ b/drivers/gpu/drm/virtio/virtgpu_drm_bus.c > @@ -53,6 +53,37 @@ int drm_virtio_init(struct drm_driver *driver, struct virtio_device *vdev) > 0, > "virtiodrmfb"); > > + /* > + * Normally the drm_dev_set_unique() call is done by core DRM. > + * The following comment covers, why virtio cannot rely on it. > + * > + * Unlike the other virtual GPU drivers, virtio abstracts the > + * underlying bus type by using struct virtio_device. > + * > + * Hence the dev_is_pci() check, used in core DRM, will fail > + * and the unique returned will be the virtio_device "virtio0", > + * while a "pci:..." one is required. > + * > + * A few other ideas were considered: > + * - Extend dev_is_pci() check [in drm_set_busid] to consider > + * virtio. I suggest "Extend [the] dev_is_pci() check" for a more "fluent" reading experience. (I should note however that I'm not a native speaker.) In addition, the word "virtio" doesn't appear correctly indented, within the comment. > + * Seems like a bigger hack than what we have already. > + * > + * - Point drm_device::dev to the parent of the virtio_device > + * Semantic changes: > + * * Using the wrong device for i2c, framebuffer_alloc and > + * prime import. > + * Visual changes: > + * * Helpers such as DRM_DEV_ERROR, dev_info, drm_printer, > + * will print the wrong information. > + * > + * We could address the latter issues, by introducing > + * drm_device::bus_dev, ... which will be used solely for this. s/will/would/? > + * > + * So for the moment keep things as-is, with a bulky comment > + * for the next person who feels like removing this > + * drm_dev_set_unique() quirk. > + */ > snprintf(unique, sizeof(unique), "pci:%s", pname); > ret = drm_dev_set_unique(dev, unique); > if (ret) > Semantically: I've by now totally forgotten about these details, so if you wanted to, you could totally sell me on some "underhanded inaccuracies" in this comment. That said, I feel the comment (and the commit message) are great. The comment makes me recall the details on the subject, and I think it'll be pretty effective as a warning, effective at preventing further regressions in the area. With the typos fixed: Reviewed-by: Laszlo Ersek <lersek@xxxxxxxxxx> Thanks! Laszlo _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
