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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

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