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

[Oleksandr Andrushchenko <andr2000@gmail.com>]

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@lists.xen.org
https://lists.xen.org/xen-devel