From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Hello all,

this series extends the existing displif protocol with new
request and adds additional parameter to the exiting one.
It also provides support for the new parameter in libgnttab
via ioctl. The relevant changes in the backend can be found at [1]
and the frontend at [2].

List of changes:

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. Add the corresponding filed to the
protocol and handle it in libgnttab.
This change is required for bringing virtual display on iMX8
platform which uses offset of 64 bytes for the buffers allocated
on GPU side and then imported into PV DRM frontend. Otherwise the
final picture looks shifted.

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.

Open questions and notes on the changes:

1. gnttab minor version change from 2 to 3
As per my understanding it is required to bump the version when
I add the new functionality, but I would like to hear from the
maintainers on that.

2. gnttab and version 2 of the ioctls
Because we add an additional parameter (data offset) and the structures
used to pass ioctl arguments cannot be extended (there are no enough
reserved fields), so there are to ways to solve that:
- break the existing API and add data_ofs field into the existing
structures
- create a copy of the ioctl (v2) with the additional parameter.

It seems to be easier to go the first way, but this breaks things,
so I decided to introduce v2 of the same ioctl which behaves gracefully
with respect to the existing users, but adds some amount of
copy-paste code (I was trying to minimize copy-paste so it is easier
to maintain, but the result looked ugly to me because of lots of
"if (protocol v1)" constructs.

Please note that struct ioctl_gntdev_dmabuf_imp_to_refs, e.g.
version 1 of the ioctl, has uint32_t reserved field which can be
used for the data offset, but its counterpart (ioctl_gntdev_dmabuf_exp_from_refs)
doesn't have any, so it seems not to be aligned to only introduce
version 2 of the ioctl for the later.

The other question is if to keep that reserved field in
ioctl_gntdev_dmabuf_imp_to_refs_v2 structure or drop it.

3. displif protocol version string to int conversion
The existing protocol defines its version as a string "1"
which is ok, but makes it not so easy to be directly used by
C/C++ preprocessor which would like to see an integer for constructs
like "#if XENDISPL_PROTOCOL_VERSION > 2"

At the same time this change may break the existing users of the protocol
which still expect version as a string. I tried something like

#define STR_HELPER(x) #x
#define STR(x) STR_HELPER(x)

#define XENDISPL_PROTOCOL_VERSION_INT 1
#define XENDISPL_PROTOCOL_VERSION STR(XENDISPL_PROTOCOL_VERSION_INT)

but not sure if this is the right and good way to solve the issue,
so in this series I have deliberately changed the protocol version to
int.
Other possible way could be that every user of the header has its local
copy (this is what we now use in the display backend). This way the
local copy can be changed in a way suitable for the concrete user.
This cannot be done (?) for the Linux Kernel though.

Thank you,
Oleksandr

[1] https://github.com/xen-troops/displ_be
[2] https://github.com/xen-troops/linux/pull/87

Oleksandr Andrushchenko (2):
  xen/displif: Protocol version 2
  libgnttab: Add support for Linux dma-buf offset

 tools/include/xen-sys/Linux/gntdev.h  | 53 +++++++++++++++++
 tools/libs/gnttab/Makefile            |  2 +-
 tools/libs/gnttab/freebsd.c           | 15 +++++
 tools/libs/gnttab/gnttab_core.c       | 17 ++++++
 tools/libs/gnttab/include/xengnttab.h | 13 ++++
 tools/libs/gnttab/libxengnttab.map    |  6 ++
 tools/libs/gnttab/linux.c             | 86 +++++++++++++++++++++++++++
 tools/libs/gnttab/minios.c            | 15 +++++
 tools/libs/gnttab/private.h           |  9 +++
 xen/include/public/io/displif.h       | 83 +++++++++++++++++++++++++-
 10 files changed, 295 insertions(+), 4 deletions(-)

-- 
2.17.1

ping

On 5/20/20 12:04 PM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> Hello all,
>
> this series extends the existing displif protocol with new
> request and adds additional parameter to the exiting one.
> It also provides support for the new parameter in libgnttab
> via ioctl. The relevant changes in the backend can be found at [1]
> and the frontend at [2].
>
> List of changes:
>
> 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. Add the corresponding filed to the
> protocol and handle it in libgnttab.
> This change is required for bringing virtual display on iMX8
> platform which uses offset of 64 bytes for the buffers allocated
> on GPU side and then imported into PV DRM frontend. Otherwise the
> final picture looks shifted.
>
> 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.
>
> Open questions and notes on the changes:
>
> 1. gnttab minor version change from 2 to 3
> As per my understanding it is required to bump the version when
> I add the new functionality, but I would like to hear from the
> maintainers on that.
>
> 2. gnttab and version 2 of the ioctls
> Because we add an additional parameter (data offset) and the structures
> used to pass ioctl arguments cannot be extended (there are no enough
> reserved fields), so there are to ways to solve that:
> - break the existing API and add data_ofs field into the existing
> structures
> - create a copy of the ioctl (v2) with the additional parameter.
>
> It seems to be easier to go the first way, but this breaks things,
> so I decided to introduce v2 of the same ioctl which behaves gracefully
> with respect to the existing users, but adds some amount of
> copy-paste code (I was trying to minimize copy-paste so it is easier
> to maintain, but the result looked ugly to me because of lots of
> "if (protocol v1)" constructs.
>
> Please note that struct ioctl_gntdev_dmabuf_imp_to_refs, e.g.
> version 1 of the ioctl, has uint32_t reserved field which can be
> used for the data offset, but its counterpart (ioctl_gntdev_dmabuf_exp_from_refs)
> doesn't have any, so it seems not to be aligned to only introduce
> version 2 of the ioctl for the later.
>
> The other question is if to keep that reserved field in
> ioctl_gntdev_dmabuf_imp_to_refs_v2 structure or drop it.
>
> 3. displif protocol version string to int conversion
> The existing protocol defines its version as a string "1"
> which is ok, but makes it not so easy to be directly used by
> C/C++ preprocessor which would like to see an integer for constructs
> like "#if XENDISPL_PROTOCOL_VERSION > 2"
>
> At the same time this change may break the existing users of the protocol
> which still expect version as a string. I tried something like
>
> #define STR_HELPER(x) #x
> #define STR(x) STR_HELPER(x)
>
> #define XENDISPL_PROTOCOL_VERSION_INT 1
> #define XENDISPL_PROTOCOL_VERSION STR(XENDISPL_PROTOCOL_VERSION_INT)
>
> but not sure if this is the right and good way to solve the issue,
> so in this series I have deliberately changed the protocol version to
> int.
> Other possible way could be that every user of the header has its local
> copy (this is what we now use in the display backend). This way the
> local copy can be changed in a way suitable for the concrete user.
> This cannot be done (?) for the Linux Kernel though.
>
> Thank you,
> Oleksandr
>
> [1] https://github.com/xen-troops/displ_be
> [2] https://github.com/xen-troops/linux/pull/87
>
> Oleksandr Andrushchenko (2):
>    xen/displif: Protocol version 2
>    libgnttab: Add support for Linux dma-buf offset
>
>   tools/include/xen-sys/Linux/gntdev.h  | 53 +++++++++++++++++
>   tools/libs/gnttab/Makefile            |  2 +-
>   tools/libs/gnttab/freebsd.c           | 15 +++++
>   tools/libs/gnttab/gnttab_core.c       | 17 ++++++
>   tools/libs/gnttab/include/xengnttab.h | 13 ++++
>   tools/libs/gnttab/libxengnttab.map    |  6 ++
>   tools/libs/gnttab/linux.c             | 86 +++++++++++++++++++++++++++
>   tools/libs/gnttab/minios.c            | 15 +++++
>   tools/libs/gnttab/private.h           |  9 +++
>   xen/include/public/io/displif.h       | 83 +++++++++++++++++++++++++-
>   10 files changed, 295 insertions(+), 4 deletions(-)
>