Re: [PATCH 1/2] drm/hyperv: Add DRM driver for hyperv synthetic video device

From: Daniel Vetter <daniel@ffwll.ch>
To: Deepak Rawat <drawat.floss@gmail.com>
Cc: linux-hyperv@vger.kernel.org, dri-devel@lists.freedesktop.org,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Gerd Hoffmann <kraxel@redhat.com>,
	Sam Ravnborg <sam@ravnborg.org>, Dexuan Cui <decui@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Wei Hu <weh@microsoft.com>,
	Tang Shaofeng <shaofeng.tang@intel.com>,
	Michael Kelley <mikelley@microsoft.com>
Subject: Re: [PATCH 1/2] drm/hyperv: Add DRM driver for hyperv synthetic video device
Date: Tue, 4 Apr 2023 15:20:55 +0200	[thread overview]
Message-ID: <CAKMK7uHRTcdZxreD5ymc2TsV9LNePeR=JgvJbnO-q2_wN99kEA@mail.gmail.com> (raw)
In-Reply-To: <20210102060336.832866-1-drawat.floss@gmail.com>

Yeah way late reply, but I just stumbled over this and got a bit puzzled ...

On Sat, 2 Jan 2021 at 07:03, Deepak Rawat <drawat.floss@gmail.com> wrote:
> +/*
> + * PCI/vmbus interface
> + */
> +
> +static int hyperv_pci_probe(struct pci_dev *pdev,
> +                           const struct pci_device_id *ent)
> +{
> +       return 0;
> +}

Why do you have this dummy driver when it does nothing? Can it just be
deleted? If it's just to have a driver, then we really don't need that
on linux, there's no requirement to have a device driver for every
device in a system.

If you actually need to make sure that this pci device isn't passed to
a guest vm or something like that, then the main driver must ensure
that the pci driver is bound (ideally with component.c because
otherwise you'll get the unbind/rebind dance wrong in one of the
bazillion of subtle ways). Just having a driver doesn't stop anyone
from unbinding it and then wreaking havoc.
-Daniel

> +
> +static void hyperv_pci_remove(struct pci_dev *pdev)
> +{
> +}
> +
> +static const struct pci_device_id hyperv_pci_tbl[] = {
> +       {
> +               .vendor = PCI_VENDOR_ID_MICROSOFT,
> +               .device = PCI_DEVICE_ID_HYPERV_VIDEO,
> +       },
> +       { /* end of list */ }
> +};
> +
> +static struct pci_driver hyperv_pci_driver = {
> +       .name =         KBUILD_MODNAME,
> +       .id_table =     hyperv_pci_tbl,
> +       .probe =        hyperv_pci_probe,
> +       .remove =       hyperv_pci_remove,
> +};
> +
> +static int hyperv_get_vram_gen1(struct hyperv_device *hv)
> +{
> +       struct drm_device *dev = &hv->dev;
> +       struct pci_dev *pdev;
> +       int ret;
> +
> +       pdev = pci_get_device(PCI_VENDOR_ID_MICROSOFT,
> +                             PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
> +       if (!pdev) {
> +               drm_err(dev, "Unable to find PCI Hyper-V video\n");
> +               return -ENODEV;
> +       }
> +
> +       ret = drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, "hypervdrmfb");
> +       if (ret) {
> +               drm_err(dev, "Not able to remove boot fb\n");
> +               return ret;
> +       }
> +
> +       if (pci_request_region(pdev, 0, DRIVER_NAME) != 0)
> +               drm_warn(dev, "Cannot request framebuffer, boot fb still active?\n");
> +
> +       if ((pdev->resource[0].flags & IORESOURCE_MEM) == 0) {
> +               drm_err(dev, "Resource at bar 0 is not IORESOURCE_MEM\n");
> +               ret = -ENODEV;
> +               goto error;
> +       }
> +
> +       hv->fb_base = pci_resource_start(pdev, 0);
> +       hv->fb_size = pci_resource_len(pdev, 0);
> +       if (hv->fb_base == 0) {
> +               drm_err(dev, "Resource not available\n");
> +               ret = -ENODEV;
> +               goto error;
> +       }
> +
> +       hv->fb_size = min(hv->fb_size,
> +                         (unsigned long)(hv->mmio_megabytes * 1024 * 1024));
> +       hv->vram = devm_ioremap(&pdev->dev, hv->fb_base, hv->fb_size);
> +       if (!hv->vram) {
> +               drm_err(dev, "Failed to map vram\n");
> +               ret = -ENOMEM;
> +       }
> +
> +error:
> +       pci_dev_put(pdev);
> +       return ret;
> +}
> +
> +static int hyperv_get_vram_gen2(struct hyperv_device *hv,
> +                               struct hv_device *hdev)
> +{
> +       struct drm_device *dev = &hv->dev;
> +       struct apertures_struct *ap;
> +       int ret;
> +
> +       hv->fb_size = (unsigned long)(hv->mmio_megabytes * 1024 * 1024);
> +
> +       ret = vmbus_allocate_mmio(&hv->mem, hdev, 0, -1, hv->fb_size, 0x100000,
> +                                 true);
> +       if (ret) {
> +               drm_err(dev, "Failed to allocate mmio\n");
> +               return -ENOMEM;
> +       }
> +
> +       hv->vram = ioremap(hv->mem->start, hv->fb_size);
> +       if (!hv->vram) {
> +               drm_err(dev, "Failed to map vram\n");
> +               ret = -ENOMEM;
> +               goto error;
> +       }
> +
> +       hv->fb_base = hv->mem->start;
> +
> +       ap = alloc_apertures(1);
> +       if (!ap) {
> +               drm_err(dev, "Failed to get apertures\n");
> +               ret = -ENOMEM;
> +               goto error;
> +       }
> +
> +       ap->ranges[0].base = screen_info.lfb_base;
> +       ap->ranges[0].size = screen_info.lfb_size;
> +       remove_conflicting_framebuffers(ap, KBUILD_MODNAME, false);
> +       kfree(ap);
> +
> +       return 0;
> +
> +error:
> +       vmbus_free_mmio(hv->mem->start, hv->fb_size);
> +       return ret;
> +}
> +
> +static int hyperv_vmbus_probe(struct hv_device *hdev,
> +                             const struct hv_vmbus_device_id *dev_id)
> +{
> +       struct hyperv_device *hv;
> +       struct drm_device *dev;
> +       int ret;
> +
> +       hv = devm_drm_dev_alloc(&hdev->device, &hyperv_driver,
> +                               struct hyperv_device, dev);
> +       if (IS_ERR(hv))
> +               return PTR_ERR(hv);
> +
> +       dev = &hv->dev;
> +       init_completion(&hv->wait);
> +       hv_set_drvdata(hdev, hv);
> +       hv->hdev = hdev;
> +
> +       /* Get the actual VRAM size from the device */
> +       ret = synthvid_connect_vsp(hdev);
> +       if (ret) {
> +               drm_err(dev, "Failed to connect to vmbus.\n");
> +               goto err_hv_set_drv_data;
> +       }
> +
> +       if (efi_enabled(EFI_BOOT))
> +               ret = hyperv_get_vram_gen2(hv, hdev);
> +       else
> +               ret = hyperv_get_vram_gen1(hv);
> +
> +       if (ret)
> +               goto err_vmbus_close;
> +
> +       /*
> +        * Should be done only once during init and resume. Failing to update
> +        * vram location is not fatal. Device will update dirty area till
> +        * preferred resolution only.
> +        */
> +       ret = synthvid_update_vram_location(hdev, hv->fb_base);
> +       if (ret)
> +               drm_warn(dev, "Failed to update vram location.\n");
> +
> +       ret = hyperv_mode_config_init(hv);
> +       if (ret)
> +               goto err_vmbus_close;
> +
> +       ret = drm_dev_register(dev, 0);
> +       if (ret) {
> +               drm_err(dev, "Failed to register drm driver.\n");
> +               goto err_vmbus_close;
> +       }
> +
> +       drm_fbdev_generic_setup(dev, 0);
> +
> +       return 0;
> +
> +err_vmbus_close:
> +       vmbus_close(hdev->channel);
> +err_hv_set_drv_data:
> +       hv_set_drvdata(hdev, NULL);
> +       return ret;
> +}
> +
> +static int hyperv_vmbus_remove(struct hv_device *hdev)
> +{
> +       struct drm_device *dev = hv_get_drvdata(hdev);
> +       struct hyperv_device *hv = to_hv(dev);
> +
> +       drm_dev_unplug(dev);
> +       drm_atomic_helper_shutdown(dev);
> +       vmbus_close(hdev->channel);
> +       hv_set_drvdata(hdev, NULL);
> +       vmbus_free_mmio(hv->mem->start, hv->fb_size);
> +
> +       return 0;
> +}
> +
> +static int hyperv_vmbus_suspend(struct hv_device *hdev)
> +{
> +       struct drm_device *dev = hv_get_drvdata(hdev);
> +       int ret;
> +
> +       ret = drm_mode_config_helper_suspend(dev);
> +
> +       vmbus_close(hdev->channel);
> +
> +       return ret;
> +}
> +
> +static int hyperv_vmbus_resume(struct hv_device *hdev)
> +{
> +       struct drm_device *dev = hv_get_drvdata(hdev);
> +       int ret;
> +
> +       ret = synthvid_connect_vsp(hdev);
> +       if (ret)
> +               return ret;
> +
> +       return drm_mode_config_helper_resume(dev);
> +}
> +
> +static const struct hv_vmbus_device_id hyperv_vmbus_tbl[] = {
> +       /* Synthetic Video Device GUID */
> +       {HV_SYNTHVID_GUID},
> +       {}
> +};
> +
> +static struct hv_driver hyperv_hv_driver = {
> +       .name = KBUILD_MODNAME,
> +       .id_table = hyperv_vmbus_tbl,
> +       .probe = hyperv_vmbus_probe,
> +       .remove = hyperv_vmbus_remove,
> +       .suspend = hyperv_vmbus_suspend,
> +       .resume = hyperv_vmbus_resume,
> +       .driver = {
> +               .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +       },
> +};
> +
> +/* ---------------------------------------------------------------------- */
> +/* module init/exit                                                       */
> +
> +static int __init hyperv_init(void)
> +{
> +       int ret;
> +
> +       ret = pci_register_driver(&hyperv_pci_driver);
> +       if (ret != 0)
> +               return ret;
> +
> +       return vmbus_driver_register(&hyperv_hv_driver);
> +}
> +
> +static void __exit hyperv_exit(void)
> +{
> +       vmbus_driver_unregister(&hyperv_hv_driver);
> +       pci_unregister_driver(&hyperv_pci_driver);
> +}
> +
> +module_init(hyperv_init);
> +module_exit(hyperv_exit);
> +
> +MODULE_DEVICE_TABLE(pci, hyperv_pci_tbl);
> +MODULE_DEVICE_TABLE(vmbus, hyperv_vmbus_tbl);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Deepak Rawat <drawat.floss@gmail.com>");
> +MODULE_DESCRIPTION("DRM driver for hyperv synthetic video device");
> --
> 2.29.2
>

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch