Re: [PATCH 0/2] displif: Protocol version 2

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

ping

On 5/20/20 12:04 PM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>
> 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(-)
>