Hi Am 05.01.21 um 03:27 schrieb Deepak Rawat:
On Mon, 2021-01-04 at 14:03 +0100, Thomas Zimmermann wrote:Hi, I've been looking forward to this patchset. :) The code is really nice already.Thanks Thomas for the review.+config DRM_HYPERV+ tristate "DRM Support for hyperv synthetic video device" + depends on DRM && PCI && MMU && HYPERV + select DRM_KMS_HELPER + select DRM_GEM_SHMEM_HELPERSHMEM helpers might not be a good choice, because you need this blit code, which has a memcpy. I guess it's easily possible to configure 16 MiB or more for the guest's VRAM? If so, I suggest to use VRAM helpers. Guests will be able to render into VRAM directly with the driver's memcpy. The driver will do page flipping. The bochs driver would be an example. Hyperv doesn't need buffer sharing with other devices, I guess?It's not possible to do page flip with this virtual device. The call to SYNTHVID_VRAM_LOCATION is only honoured once. So unfortunately need to use SHMEM helpers.
I was thinking about using struct video_output_situation.vram_offset; in case you want to tinker with that. There's a comment in the patch that vram_offset should always be 0. But this comment seems incorrect for devices with more than one output.
In any case, SHMEM is good enough for now and this would not be a blocker.
+#define PCI_VENDOR_ID_MICROSOFT 0x1414 +#define PCI_DEVICE_ID_HYPERV_VIDEO 0x5353 + +struct hyperv_device {Could this name lead to conflicts with other hyperv drivers? I suggest to name it hyperv_drm_device.Sure make sense to use hyperv_drm_device.+ +struct synthvid_pointer_shape {Do you have plans for adding cursor support?Yes I have tested with a prototype and cursor works. Will attempt this in future iteration.
Sounds good.
+ + /* Negotiate the protocol version with host */ + switch (vmbus_proto_version) { + case VERSION_WIN10: + case VERSION_WIN10_V5: + ret = synthvid_negotiate_ver(hdev, SYNTHVID_VERSION_WIN10); + if (!ret) + break; + fallthrough; + case VERSION_WIN8: + case VERSION_WIN8_1: + ret = synthvid_negotiate_ver(hdev, SYNTHVID_VERSION_WIN8); + if (!ret) + break; + fallthrough; + case VERSION_WS2008: + case VERSION_WIN7: + ret = synthvid_negotiate_ver(hdev, SYNTHVID_VERSION_WIN7); + break; + default: + ret = synthvid_negotiate_ver(hdev, SYNTHVID_VERSION_WIN10);I don't get the logic of this switch statement. If the host is Win10, I'd expect the graphics device to use Win10's protocol, if the host is Win8, the graphics device uses win8 protocols. So what's the point of the fallthroughs? Can there be newer versions of vmbus_proto_version that only support older devices?This is copied as it is from hyperv_fb driver. I suppose this is just to accomodate newer version.
I see. I would put the default case to the top; right before the Win10 case. So for newer or unknown versions, it tests Win10 and then falls through until something works.
Best regards Thomas
Deepak
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
