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

[Deepak Rawat <drawat.floss@gmail.com>]

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