Re: [PATCH v3] displif: add ABI for para-virtual display

From: Stefano Stabellini <sstabellini@kernel.org>
To: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: olekstysh@gmail.com, sstabellini@kernel.org,
	Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>,
	Oleksandr Andrushchenko <andr2000@gmail.com>,
	andrii.anisov@gmail.com, stefano@aporeto.com,
	vlad.babchuk@gmail.com, al1img@gmail.com,
	Oleksandr Grytsov <oleksandr_grytsov@epam.com>,
	xen-devel@lists.xenproject.org, joculator@gmail.com
Subject: Re: [PATCH v3] displif: add ABI for para-virtual display
Date: Fri, 17 Feb 2017 11:12:02 -0800 (PST)	[thread overview]
Message-ID: <alpine.DEB.2.10.1702171050080.9566@sstabellini-ThinkPad-X260> (raw)
In-Reply-To: <20170217163338.GB30631@char.us.ORACLE.com>

On Fri, 17 Feb 2017, Konrad Rzeszutek Wilk wrote:
> On Thu, Feb 16, 2017 at 10:36:01AM +0200, Oleksandr Andrushchenko wrote:
> > On 02/15/2017 11:33 PM, Konrad Rzeszutek Wilk wrote:
> > > .snip..
> > > > > > > > I will define 2 sections:
> > > > > > > >     *------------------ Connector Request Transport Parameters
> > > > > > > > -------------------
> > > > > > > >     *
> > > > > > > >     * ctrl-event-channel
> > > > > > > >     * ctrl-ring-ref
> > > > > > > >     *
> > > > > > > >     *------------------- Connector Event Transport Parameters
> > > > > > > > --------------------
> > > > > > > >     *
> > > > > > > >     * event-channel
> > > > > > > >     * event-ring-ref
> > > > > > > > 
> > > > > > > > > Or is the other ring buffer the one that is created via 'gref_directory' ?
> > > > > > > > no
> > > > > > > > At the bottom:
> > > > > > > >     * In order to deliver asynchronous events from back to front a shared page
> > > > > > > > is
> > > > > > > >     * allocated by front and its gref propagated to back via XenStore entries
> > > > > > > >     * (event-XXX).
> > > > > > > AAnd you may want to say this is guarded by REQ_ALLOC feature right?
> > > > > > Not sure I understood you. Event path is totally independent
> > > > > > from any feature, e.g. REQ_ALLOC.
> > > > > > It just provides means to send async events
> > > > > > from back to front, "page flip done" in my case.
> > > > > <scratche his head> Why do you need a seperate ring to send
> > > > > responses back? Why not use the same ring on which requests
> > > > > were sent
> > > > Ok, it seems we are not on the same page for rings/channels usage.
> > > > Let me describe how those are used:
> > > > 
> > > > 1. Command/control event channel and its corresponding ring are used
> > > > to pass requests from front to back (XENDISPL_OP_XXX) and get responses
> > > > from the back. These are synchronous, use macros from ring.h:
> > > > ctrl-event-channel + ctrl-ring-ref
> > > > I call them "ctrl-" because this way front controls back, or sends commands
> > > > if you will. Maybe "cmd-" would fit better here?
> > > > 
> > > > 2. Event channel - asynchronous path for the backend to signal activity
> > > > to the frontend, currently used for "page flip done" event which is sent
> > > > at some point of time after back has actually completed the page flip
> > > > requested
> > > > (so, before that the corresponding request was sent and response received,
> > > > but
> > > > operation didn't complete yet, instead it was scheduled)
> > > > No macros exist for this use-case in ring.h (kbdif+fbif implement
> > > > this on their own, so do I)
> > > > These are:  event-channel + event-ring-ref
> > > > Probably here is the point from where confusion comes, naming.
> > > > We can have something like "be-to-fe-event-channel" or anything else
> > > > more cute and descriptive.
> > > > 
> > > > Hope this explains the need for 2 paths
> > > Aha!
> > > 
> > > So this is like the network where there is an 'rx' and 'tx'!
> > kind of
> > > 
> > > Now I get it.
> > sorry, I was probably not clear
> > > 
> > > In that case why not just prefix it with 'in' and 'out'? Such as:
> > > 
> > > 'out-ring-ref' and 'out-event-channel' and 'in-ring-ref' along
> > > with 'in-event-channel'.
> > hmmmm, it may confuse, because you must know "out"
> > from which POV, e.g. frontend's or backend's.
> > What is more, these "out-" and "in-" are... nameless?
> 
> Yes :-)
> 
> > Can we still have something like "ctrl-"/"cmd-"/"req-"
> > for the req/resp path and probably "evt-" for
> > events from back to front?
> 
> I like the 'req' and 'evt-' part. That makes it more
> clear I think.  But see below.
> 
> > > Or perhaps better - borrow the same idea that Stefano came up for
> > > 9pfs and PV calls - where his ring does both.
> > > 
> > > Then you just need 'ring-ref', 'event-channel', 'max-page-ring-order'
> > > (which must be 1 or larger).
> > > 
> > > And you split the ring-ref in two - one for 'in' events and the other
> > > part for 'out' events?
> > yes, I saw current implementations (kbdif, fbif) and
> > what Stefano did, but would rather stick to what is currently
> > defined (I believe it is optimal as is)
> 
> Right, but with two protocols with that way ... perhaps you
> could re-use some of what Stefano wrote? See below.
> 
> > And hope, that maybe someone will put new functionality into ring.h
> > to serve async events one day :)
> 
> That would be fantastic. And knowing Stefano I would think
> he has alreadhy done so? (Or at least have an prototype patch
> for this?)
> 
> Re-using his mechanism in your driver means easier maintaince
> as things are kind of using the same thing.
> 
> Stefano, you wouldn't have an patch for the ring.h somewhere
> would you?

I do have all the headers and macros for both pvcalls and 9pfs (see
below the 9pfs header), but not yet something generic used by both.
However, it shouldn't be difficult to generalize it. I'll submit a patch
to ring.h in the next couple of days.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel