Re: [PATCH 1/2] xen/displif: Protocol version 2

[Oleksandr Andrushchenko <andr2000@gmail.com>]

On 6/30/20 10:03 AM, Jürgen Groß wrote:
> On 30.06.20 08:13, Oleksandr Andrushchenko wrote:
>> On 6/29/20 10:02 AM, Jürgen Groß wrote:
>>> On 20.05.20 11:04, Oleksandr Andrushchenko wrote:
>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>>
>>>> 1. Change protocol version from string to integer
>>>>
>>>> Version string, which is in fact an integer, is hard to handle in the
>>>> code that supports different protocol versions. To simplify that
>>>> make the version an integer.
>>>>
>>>> 2. Pass buffer offset with XENDISPL_OP_DBUF_CREATE
>>>>
>>>> There are cases when display data buffer is created with non-zero
>>>> offset to the data start. Handle such cases and provide that offset
>>>> while creating a display buffer.
>>>>
>>>> 3. Add XENDISPL_OP_GET_EDID command
>>>>
>>>> Add an optional request for reading Extended Display Identification
>>>> Data (EDID) structure which allows better configuration of the
>>>> display connectors over the configuration set in XenStore.
>>>> With this change connectors may have multiple resolutions defined
>>>> with respect to detailed timing definitions and additional properties
>>>> normally provided by displays.
>>>>
>>>> If this request is not supported by the backend then visible area
>>>> is defined by the relevant XenStore's "resolution" property.
>>>>
>>>> If backend provides extended display identification data (EDID) with
>>>> XENDISPL_OP_GET_EDID request then EDID values must take precedence
>>>> over the resolutions defined in XenStore.
>>>>
>>>> 4. Bump protocol version to 2.
>>>>
>>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>> ---
>>>>    xen/include/public/io/displif.h | 83 +++++++++++++++++++++++++++++++--
>>>>    1 file changed, 80 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/xen/include/public/io/displif.h b/xen/include/public/io/displif.h
>>>> index cc5de9cb1f35..4d43ba5078c8 100644
>>>> --- a/xen/include/public/io/displif.h
>>>> +++ b/xen/include/public/io/displif.h
>>>> @@ -38,7 +38,7 @@
>>>>     *                           Protocol version
>>>> ******************************************************************************
>>>>     */
>>>> -#define XENDISPL_PROTOCOL_VERSION     "1"
>>>> +#define XENDISPL_PROTOCOL_VERSION     2
>>>
>>> This is not very nice regarding compatibility.
>>>
>>> Can't you add a new macro for the integer value?
>>
>> We can leave it as is, e.g. define XENDISPL_PROTOCOL_VERSION as "2",
>>
>> so we do not break the existing users. Then if every user of the header has
>>
>> its local copy (this is what we now use in the display backend), then that
>> local copy can be changed in a way suitable for the concrete user, e.g. "2"
>>
>> redefined to 2. This cannot be done (?) for the Linux Kernel though.
>>
>> Or we can have
>>
>> #define XENDISPL_PROTOCOL_VERSION     "2"
>>
>> #define XENDISPL_PROTOCOL_VERSION_INT  2
>>
>> Jurgen, what's your preference here?
>
> I would prefer the latter, as it avoids the need to modify the header
> when copying it to a local project.
>
Ok, I'll have 2 definitions then

Anything else (beside the comments on new functionality) I can add/change

before sending v2 of the patch?

>
> Juergen