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

From: Deepak Rawat <drawat.floss@gmail.com>
To: Thomas Zimmermann <tzimmermann@suse.de>,
	linux-hyperv@vger.kernel.org, dri-devel@lists.freedesktop.org
Cc: Daniel Vetter <daniel@ffwll.ch>,
	Gerd Hoffmann <kraxel@redhat.com>,
	Sam Ravnborg <sam@ravnborg.org>, Dexuan Cui <decui@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Wei Hu <weh@microsoft.com>,
	Tang Shaofeng <shaofeng.tang@intel.com>,
	Michael Kelley <mikelley@microsoft.com>
Subject: Re: [PATCH 1/2] drm/hyperv: Add DRM driver for hyperv synthetic video device
Date: Mon, 04 Jan 2021 18:27:24 -0800	[thread overview]
Message-ID: <4f7818f99734c0912325e1f3b6b80cb2a04df3ef.camel@gmail.com> (raw)
In-Reply-To: <2b49fcd2-38f7-dae5-2992-721a8bd142a2@suse.de>

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