Re: [PATCH] drm/hyperv: Don't rely on screen_info.lfb_base for Gen1 VMs

From: Thomas Zimmermann
Date: Tue Sep 13 2022 - 12:42:33 EST


Hi

Am 13.09.22 um 17:15 schrieb Saurabh Singh Sengar:
On Mon, Sep 12, 2022 at 09:03:53AM +0200, Thomas Zimmermann wrote:
Hi

Am 11.09.22 um 18:21 schrieb Saurabh Singh Sengar:
On Sat, Sep 10, 2022 at 08:11:24PM +0200, Thomas Zimmermann wrote:
Hi

Am 09.09.22 um 16:43 schrieb Saurabh Sengar:
hyperv_setup_vram tries to remove conflicting framebuffer based on
'screen_info'. As observed in past due to some bug or wrong setting
in grub, the 'screen_info' fields may not be set for Gen1, and in such
cases drm_aperture_remove_conflicting_framebuffers will not do anything
useful.
For Gen1 VMs, it should always be possible to get framebuffer
conflict removed using PCI device instead.

Fixes: a0ab5abced55 ("drm/hyperv : Removing the restruction of VRAM allocation with PCI bar size")
Signed-off-by: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/hyperv/hyperv_drm_drv.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
index 6d11e7938c83..b0cc974efa45 100644
--- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
+++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
@@ -73,12 +73,28 @@ static int hyperv_setup_vram(struct hyperv_drm_device *hv,
struct hv_device *hdev)
{
struct drm_device *dev = &hv->dev;
+ struct pci_dev *pdev;
int ret;
- drm_aperture_remove_conflicting_framebuffers(screen_info.lfb_base,
- screen_info.lfb_size,
- false,
- &hyperv_driver);
+ if (efi_enabled(EFI_BOOT)) {
+ drm_aperture_remove_conflicting_framebuffers(screen_info.lfb_base,
+ screen_info.lfb_size,

Using screen_info here seems wrong in any case. You want to remove
the framebuffer devices that conflict with your driver, which might
be unrelated to screen_info. AFAICT the correct solution would
always retrieve the PCI device for removal (i.e., always do the else
branch).

In a Gen2 VM, the Hyper-V frame buffer device is presented only as a VMbus device.
It's not presented as a PCI device like it is in a Gen1 VM. This would have worked
if we had the frame buffer device available as PCI device in Gen2 but unfortunately
thats not the case here.

Thanks for explaining. There is an instance of struct hv_device
passed to the probe function. I suspect you cannot get the
framebuffer range from this instance (e.g., via the device's
platform_data)?

If you absolutely can't get the actual memory region from the
device, it's better to remove all framebuffers via
drm_aperture_remove_framebuffers() than to use screen_info.

Best regards
Thomas

Thanks for your suggestion, and I thought of using drm_aperture_remove_framebuffers
here, but this driver will be used in many different systems with many other graphics
devices (GPU etc). Removing all the framebuffer is a bit blunt approach which may disturb
the devices we are not intended to and which are even outside of the HyperV MMIO region.
I feel this API use will be risky, and I would like to stick to the earlier method which
is proven to be working for many years and we are sure it won't disturb anyone outside
MMIO region.

But the problem with the current code is that you also remove the driver that covers the boot-up graphics, even if it's not even a hyperv device. That's why I said that you should try to detect if the hyperv device you're creating actually uses the screen_info when you create the device.

Set that information as device data and look it up in the hyperv-drm driver. If no such screen_info data is attached to the device, you don't have to remove conflicting framebuffers at all.

Best regards
Thomas


Regards,
Saurabh


Regards,
Saurabh


Best regard
Thomas

+ false,
+ &hyperv_driver);
+ } else {
+ 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_aperture_remove_conflicting_pci_framebuffers(pdev, &hyperv_driver);
+ pci_dev_put(pdev);
+ if (ret) {
+ drm_err(dev, "Not able to remove boot fb\n");
+ return ret;
+ }
+ }
hv->fb_size = (unsigned long)hv->mmio_megabytes * 1024 * 1024;

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev




--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev




--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature