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

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

 



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_HELPER

SHMEM 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


[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux