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

[Oleksandr Andrushchenko <andr2000@gmail.com>]

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
> Thanks.
Thank you,
Oleksandr

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