Re: [PATCH] drm/virtio: populate .set_busid callback

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/03/16 19:28, Emil Velikov wrote:
> On 3 October 2016 at 18:08, Laszlo Ersek <lersek@xxxxxxxxxx> 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@xxxxxxxx>
>>> Cc: Laszlo Ersek <lersek@xxxxxxxxxx>
>>> Cc: stable@xxxxxxxxxxxxxxx
>>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1366842
>>> Reported-by: Laszlo Ersek <lersek@xxxxxxxxxx>
>>> Signed-off-by: Emil Velikov <emil.l.velikov@xxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxx>
>> 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@xxxxxxxx>
>>     Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>>     Cc: David Airlie <airlied@xxxxxxxxxx>
>>     Cc: Emil Velikov <emil.l.velikov@xxxxxxxxx>
>>     Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
>>     Cc: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
>>     Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
>>     Cc: Joachim Frieben <jfrieben@xxxxxxxxxxx>
>>     Cc: stable@xxxxxxxxxxxxxxx
>>     Reported-by: Joachim Frieben <jfrieben@xxxxxxxxxxx>
>>     Fixes: a325725633c26aa66ab940f762a6b0778edf76c0
>>     Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1366842
>>     Signed-off-by: Laszlo Ersek <lersek@xxxxxxxxxx>
>>
>>  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@xxxxxxxxx>

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
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]