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

From: Daniel Vetter <daniel@ffwll.ch>
To: Deepak Rawat <drawat.floss@gmail.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>,
	linux-hyperv@vger.kernel.org,
	Stephen Hemminger <sthemmin@microsoft.com>,
	David Airlie <airlied@linux.ie>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Wei Liu <wei.liu@kernel.org>,
	dri-devel@lists.freedesktop.org,
	Michael Kelley <mikelley@microsoft.com>,
	Jork Loeser <jloeser@microsoft.com>, Wei Hu <weh@microsoft.com>,
	K Y Srinivasan <kys@microsoft.com>
Subject: Re: [RFC PATCH 1/2] drm/hyperv: Add DRM driver for hyperv synthetic video device
Date: Tue, 23 Jun 2020 11:42:25 +0200	[thread overview]
Message-ID: <20200623094225.GJ20149@phenom.ffwll.local> (raw)
In-Reply-To: <f6923296368dc676df10e75593ebc18575efc476.camel@gmail.com>

On Mon, Jun 22, 2020 at 03:20:34PM -0700, Deepak Rawat wrote:
> On Mon, 2020-06-22 at 14:46 +0200, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > +/* Should be done only once during init and resume */
> > > +static int synthvid_update_vram_location(struct hv_device *hdev,
> > > +					  phys_addr_t vram_pp)
> > > +{
> > > +	struct hyperv_device *hv = hv_get_drvdata(hdev);
> > > +	struct synthvid_msg *msg = (struct synthvid_msg *)hv->init_buf;
> > > +	unsigned long t;
> > > +	int ret = 0;
> > > +
> > > +	memset(msg, 0, sizeof(struct synthvid_msg));
> > > +	msg->vid_hdr.type = SYNTHVID_VRAM_LOCATION;
> > > +	msg->vid_hdr.size = sizeof(struct synthvid_msg_hdr) +
> > > +		sizeof(struct synthvid_vram_location);
> > > +	msg->vram.user_ctx = msg->vram.vram_gpa = vram_pp;
> > > +	msg->vram.is_vram_gpa_specified = 1;
> > > +	synthvid_send(hdev, msg);
> > 
> > That suggests it is possible to define multiple framebuffers in vram,
> > then pageflip by setting vram.vram_gpa.  If that is the case I'm
> > wondering whenever vram helpers are a better fit maybe?  You don't
> > have
> > to blit each and every display update then.
> 
> Thanks for the review. Unfortunately only the first vmbus message take
> effect and subsequent calls are ignored. I originally implemented using
> vram helpers but I figured out calling this vmbus message again won't
> change the vram location.

I think that needs a very big comment. Maybe even enforce that with a
"vram_gpa_set" boolean in the device structure, and complain if we try to
do that twice.

Also I guess congrats to microsoft for defining things like that :-/
-Daniel

> 
> > 
> > FYI: cirrus goes the blit route because (a) the amount of vram is
> > very
> > small so trying to store multiple framebuffers there is out of
> > question,
> > and (b) cirrus converts formats on the fly to hide some hardware
> > oddities.
> > 
> > take care,
> >   Gerd
> > 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch