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.
> > +#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.
> > +
> > + /* 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.
>
Deepak
