[PATCH v2] compositor-drm: Add the aspect ratio parsing support
Pekka Paalanen
ppaalanen at gmail.com
Wed Oct 4 08:43:25 UTC 2017
On Wed, 20 Sep 2017 17:33:00 +0530
Nautiyal Ankit <ankit.k.nautiyal at intel.com> wrote:
> From: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>
> DRM layer populates aspect ratio information for CEA video modes,
> but we lose it while parsing modeline (Aspect ratio patch series
> in drm layer: https://patchwork.freedesktop.org/series/10850).
> The flag bits 19-22 of the connector modes, provide the aspect ratio info.
> This information can be stored in flags bits of the weston mode structure,
> so that it can used for setting a mode with a particular aspect ratio.
>
> This patch:
> - preserves aspect ratio flags from kernel video modes and accommodates it
> in wayland mode flags (bits 02-05).
> - Uses these aspect ratio flags to pick the appropriate mode during
> modeset.
> - changes the mode format in configuration file weston.ini to accommodate
> aspect ratio information as:
> <width>x<height>@<RefreshRate> <aspect-ratio>.
> The aspect ratio should be given as <length:breadth>.
>
> v2: As per recommendation from Pekka Paalanen, Quentin Glidic,
> and Daniel Stone, dropped the aspect-ratio info from wayland protocol,
> thereby avoiding exposure of aspect-ratio to the client.
>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
> ---
> libweston/compositor-drm.c | 98 +++++++++++++++++++++++++++++++++++++++-------
> libweston/compositor-drm.h | 23 +++++++++++
> man/weston.ini.man | 10 +++--
> 3 files changed, 113 insertions(+), 18 deletions(-)
Hi Ankit,
this patch has an issue with API organization.
'struct weston_mode' is defined in libweston/compositor.h and is common
to all backends and all frontends (compositor/main.c). Therefore, if
you add more flags to weston_mode::flags, they also need to be defined
in compositor.h.
In compositor.c, bind_output() needs to be patched to not relay the new
flags to Wayland clients, because the Wayland protocol spec does not
define these flags.
However, I would recommend to just add a new field 'uint32_t
aspect_ratio' to 'struct weston_mode' and not re-use
weston_mode::flags. Document the new field to use values from 'enum
weston_mode_aspect_ratio' but also allow unlisted values in case the
kernel has added new entries and the weston list is outdated.
That way there is no need to patch the existing uses of
weston_mode::flags, and we do not mix protocol flags with other flags
in the same variable.
There are more comments below.
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 1a96138..fb84b13 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -1962,24 +1962,41 @@ drm_assign_planes(struct weston_output *output_base, void *repaint_data)
> static struct drm_mode *
> choose_mode (struct drm_output *output, struct weston_mode *target_mode)
> {
> - struct drm_mode *tmp_mode = NULL, *mode;
> + struct drm_mode *tmp_mode = NULL, *mode_fall_back = NULL, *mode;
> + uint8_t src_aspect = 0;
> + uint8_t target_aspect = 0;
>
> + target_aspect =
> + target_mode->flags & WESTON_MODE_FLAG_PIC_AR_MASK;
> if (output->base.current_mode->width == target_mode->width &&
> output->base.current_mode->height == target_mode->height &&
> (output->base.current_mode->refresh == target_mode->refresh ||
> - target_mode->refresh == 0))
> - return (struct drm_mode *)output->base.current_mode;
> + target_mode->refresh == 0)) {
> + src_aspect = mode->mode_info.flags &
> + WESTON_MODE_FLAG_PIC_AR_MASK;
'mode' is being used uninitialized.
> + if (target_aspect && src_aspect && src_aspect == target_aspect)
> + return (struct drm_mode *)output->base.current_mode;
Why must target_aspect and src_aspect be non-zero in addition to being
equal? Zero aspect ratio value is explicitly defined to be NONE, not
unknown. We have always assumed NONE means square pixels, because we
have assumed square pixels for unset and NONE is identical to unset.
> + }
>
> wl_list_for_each(mode, &output->base.mode_list, base.link) {
> if (mode->mode_info.hdisplay == target_mode->width &&
> mode->mode_info.vdisplay == target_mode->height) {
> if (mode->base.refresh == target_mode->refresh ||
> - target_mode->refresh == 0) {
> - return mode;
> - } else if (!tmp_mode)
> + target_mode->refresh == 0) {
Unlike the kernel alignment rules, we usually align to the beginning
parenthesis the way it was in the old code.
> + src_aspect = mode->mode_info.flags &
> + WESTON_MODE_FLAG_PIC_AR_MASK;
> + if (target_aspect && src_aspect &&
> + src_aspect == target_aspect)
The same question: why non-zero aspect flags required for finding a
match, is equality not enough?
> + return mode;
> + else if (!mode_fall_back)
> + mode_fall_back = mode;
> + } else if (!tmp_mode) {
> tmp_mode = mode;
> + }
> }
> }
> + if (mode_fall_back)
> + return mode_fall_back;
>
> return tmp_mode;
I understand the matching idea is, in order:
- If the current mode matches exactly, return that.
- If there is a mode in the mode list that matches exactly, return that.
- If there is a mode in the mode list that matches except for the
aspect ratio, return that.
- If there is a mode in the mode list that matches for resolution,
return that.
- Return NULL.
Sounds fine.
> }
> @@ -2982,21 +3021,38 @@ drm_output_choose_initial_mode(struct drm_backend *backend,
> struct drm_mode *preferred = NULL;
> struct drm_mode *current = NULL;
> struct drm_mode *configured = NULL;
> + struct drm_mode *config_fall_back = NULL;
> struct drm_mode *best = NULL;
> struct drm_mode *drm_mode;
> drmModeModeInfo drm_modeline;
> int32_t width = 0;
> int32_t height = 0;
> uint32_t refresh = 0;
> + uint32_t aspect_width = 0;
> + uint32_t aspect_height = 0;
> + uint8_t aspect_ratio = 0;
> int n;
>
> if (mode == WESTON_DRM_BACKEND_OUTPUT_PREFERRED && modeline) {
> - n = sscanf(modeline, "%dx%d@%d", &width, &height, &refresh);
> - if (n != 2 && n != 3) {
> + n = sscanf(modeline, "%dx%d@%d %u:%u", &width, &height,
> + &refresh, &aspect_width, &aspect_height);
> + if (aspect_width == 4 && aspect_height == 3)
> + aspect_ratio = WESTON_MODE_PIC_AR_4_3;
> + else if (aspect_width == 16 && aspect_height == 9)
> + aspect_ratio = WESTON_MODE_PIC_AR_16_9;
> + else if (aspect_width == 64 && aspect_height == 27)
> + aspect_ratio = WESTON_MODE_PIC_AR_64_27;
> + else if (aspect_width == 256 && aspect_height == 135)
> + aspect_ratio = WESTON_MODE_PIC_AR_256_135;
> + else
> + aspect_ratio = WESTON_MODE_PIC_AR_NONE;
If aspect ratio is given but is not any ratio we recognize, this should
at least log an error.
> +
> + if (n != 2 && n != 3 && n != 5) {
> width = -1;
>
> if (parse_modeline(modeline, &drm_modeline) == 0) {
> - configured = drm_output_add_mode(output, &drm_modeline);
> + configured = drm_output_add_mode(output,
> + &drm_modeline);
> if (!configured)
> return NULL;
> } else {
> @@ -3009,9 +3065,20 @@ drm_output_choose_initial_mode(struct drm_backend *backend,
> wl_list_for_each_reverse(drm_mode, &output->base.mode_list, base.link) {
> if (width == drm_mode->base.width &&
> height == drm_mode->base.height &&
> - (refresh == 0 || refresh == drm_mode->mode_info.vrefresh))
> - configured = drm_mode;
> -
> + (refresh == 0 || refresh == drm_mode->mode_info.vrefresh)) {
> + uint8_t src_ar;
> +
> + src_ar = (drm_mode->base.flags &
> + WESTON_MODE_FLAG_PIC_AR_MASK) >>
> + WESTON_MODE_FLAG_PIC_AR_BITS_POS;
> +
> + if (aspect_ratio != WESTON_MODE_PIC_AR_NONE
> + && src_ar != WESTON_MODE_PIC_AR_NONE
> + && aspect_ratio == src_ar)
Again the same question about why equality is not enough.
> + configured = drm_mode;
> + else
> + config_fall_back = drm_mode;
> + }
> if (memcmp(current_mode, &drm_mode->mode_info,
> sizeof *current_mode) == 0)
> current = drm_mode;
> @@ -3034,6 +3101,9 @@ drm_output_choose_initial_mode(struct drm_backend *backend,
> if (configured)
> return configured;
>
> + if (config_fall_back)
> + return config_fall_back;
> +
> if (preferred)
> return preferred;
>
> diff --git a/libweston/compositor-drm.h b/libweston/compositor-drm.h
> index 8181492..258d467 100644
> --- a/libweston/compositor-drm.h
> +++ b/libweston/compositor-drm.h
> @@ -52,6 +52,29 @@ enum weston_drm_backend_output_mode {
> WESTON_DRM_BACKEND_OUTPUT_PREFERRED,
> };
>
> +/**
> + * aspect ratio info taken from the drmModeModeInfo flag bits 19-22,
> + * which is set in the weston_mode flag bits 2 to 5.
> + */
> +#define DRM_MODE_FLAG_PIC_AR_BITS_POS 19
> +#define WESTON_MODE_FLAG_PIC_AR_BITS_POS 2
> +#ifndef DRM_MODE_FLAG_PIC_AR_MASK
> +#define DRM_MODE_FLAG_PIC_AR_MASK (0xF << DRM_MODE_FLAG_PIC_AR_BITS_POS)
> +#endif
> +#define WESTON_MODE_FLAG_PIC_AR_MASK (0xF << WESTON_MODE_FLAG_PIC_AR_BITS_POS)
> +
> +
> +enum weston_mode_aspect_ratio {
> + /** The picture aspect ratio values for the aspect ratio bits 2 to 5 of
> + * weston_mode flag.
> + */
For this enum, it would be good to document that the values here are
taken from DRM_MODE_PICTURE_ASPECT_* in drm_mode.h and they must match
the DRM definitions. That will allow you to avoid a conversion table.
> + WESTON_MODE_PIC_AR_NONE = 0,
> + WESTON_MODE_PIC_AR_4_3 = 1,
> + WESTON_MODE_PIC_AR_16_9 = 2,
> + WESTON_MODE_PIC_AR_64_27 = 3,
> + WESTON_MODE_PIC_AR_256_135 = 4
> +};
> +
> #define WESTON_DRM_OUTPUT_API_NAME "weston_drm_output_api_v1"
>
> struct weston_drm_output_api {
> diff --git a/man/weston.ini.man b/man/weston.ini.man
> index 4cfefc9..16965aa 100644
> --- a/man/weston.ini.man
> +++ b/man/weston.ini.man
> @@ -363,10 +363,12 @@ The DRM backend accepts different modes:
> .PP
> .RS 10
> .nf
> -.BR "WIDTHxHEIGHT " "Resolution size width and height in pixels"
> -.BR "preferred " "Uses the preferred mode"
> -.BR "current " "Uses the current crt controller mode"
> -.BR "off " "Disables the output"
> +.BR "WIDTHxHEIGHT " "Resolution size width and height in pixels"
> +.BR "WIDTHxHEIGHT at RR " "Resolution as above and refresh-rate in Hertz"
> +.BR "WIDTHxHEIGHT at RR L:B " "Resolution as above and aspect-ratio as length:breadth"
Is length and breadth really the official terminology? Intuitively I
would be confused by what they mean.
Since we can only parse a small fixed list of possible aspect-ratio
strings, it would be nice to list the valid strings.
> +.BR "preferred " "Uses the preferred mode"
> +.BR "current " "Uses the current crt controller mode"
> +.BR "off " "Disables the output"
> .fi
> .RE
> .RS
Man page update is good.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20171004/cacee3b7/attachment.sig>
More information about the wayland-devel
mailing list