Re: [PATCH 1/2] xen/displif: Protocol version 2
On 30.06.20 09:39, Oleksandr Andrushchenko wrote: On 6/30/20 10:30 AM, Jürgen Groß wrote: On 30.06.20 09:09, Oleksandr Andrushchenko wrote: 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 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 --- 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? I would have said so. :-) Thank you, the series also has a patch for libgnttab. Do you want me to send the protocol patch separately or should we wait for Ian and Wei to review? These changes are related, thus I sent then togheter This is something to ask the tools maintainers IMO. Juergen
Re: [PATCH 1/2] xen/displif: Protocol version 2
On 6/30/20 10:30 AM, Jürgen Groß wrote: > On 30.06.20 09:09, Oleksandr Andrushchenko wrote: >> 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 >> >> 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 >> --- >> 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? > > I would have said so. :-) Thank you, the series also has a patch for libgnttab. Do you want me to send the protocol patch separately or should we wait for Ian and Wei to review? These changes are related, thus I sent then togheter > > > Juergen Thank you, Oleksandr
Re: [PATCH 1/2] xen/displif: Protocol version 2
On 30.06.20 09:09, Oleksandr Andrushchenko wrote: 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 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 --- 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? I would have said so. :-) Juergen
Re: [PATCH 1/2] xen/displif: Protocol version 2
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 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 --- 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
Re: [PATCH 1/2] xen/displif: Protocol version 2
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 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 --- 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. Juergen
Re: [PATCH 1/2] xen/displif: Protocol version 2
On 6/29/20 10:02 AM, Jürgen Groß wrote: > On 20.05.20 11:04, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko >> >> 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 >> --- >> 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? > > And please add comments further down which additions are related to > the new version. I will after the review is done and other comments are fixed > > > Juergen Thank you, Oleksandr
Re: [PATCH 1/2] xen/displif: Protocol version 2
On 20.05.20 11:04, Oleksandr Andrushchenko wrote: From: Oleksandr Andrushchenko 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 --- 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? And please add comments further down which additions are related to the new version. Juergen
