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

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

Hi, Konrad!

On 02/17/2017 07:33 PM, Oleksandr Andrushchenko wrote:
> On 02/17/2017 06:33 PM, 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?
> yes, if Stefano already has it, then I can re-use it
> Actually, going kbdif/fbif way was just because nothing is in ring.h 
> for that
> > .. snip..
> > > I am attaching the diff between v3 and v4 for your convenience
> I am specifically interested in the recovery section - does it look ok?
> So, I can put the same into sndif
Could you please review the recovery section - this is common to
both displif and sndif (so I can send sndif v18 with this in)
> > Thanks.
> Thank you,
> Oleksandr
Thank you


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