[PATCH v3] compositor-drm: Add the aspect ratio parsing support
Pekka Paalanen
ppaalanen at gmail.com
Fri Nov 3 10:22:22 UTC 2017
(I fixed the subject line to contain v3.)
On Mon, 16 Oct 2017 20:25:23 +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.
> - Uses aspect ratio to pick the appropriate mode during modeset.
> - changes the mode format in configuration file weston.ini to accommodate
> aspect ratio information as:
> WIDTHxHEIGHT at REFRESH-RATE ASPECT-RATIO
> The aspect ratio should be given as <length:breadth>.
>
> v2: As per recommendation from Pekka Paalanen, Quentin Glidic,
> Daniel Stone, dropped the aspect-ratio info from wayland protocol,
> thereby avoiding exposure of aspect-ratio to the client.
>
> v3: As suggested by Pekka Paalanen, added aspect_ratio field to store
> aspect-ratio information from the drm. Also added drm client capability
> for aspect ratio, as recommended by Daniel Vetter.
>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
Hi Ankit,
thank you for this, we are getting closer. I have some review comments
and some kernel behaviour questions below.
> ---
> libweston/compositor-drm.c | 102 ++++++++++++++++++++++++++++++++++++++++-----
> libweston/compositor-drm.h | 45 ++++++++++++++++++++
> libweston/compositor.h | 1 +
> man/weston.ini.man | 11 +++--
> 4 files changed, 145 insertions(+), 14 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index b641d61..f016c08 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -71,6 +71,10 @@
> #define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2
> #endif
>
> +#ifndef DRM_CLIENT_CAP_ASPECT_RATIO
> +#define DRM_CLIENT_CAP_ASPECT_RATIO 4
> +#endif
> +
> #ifndef DRM_CAP_CURSOR_WIDTH
> #define DRM_CAP_CURSOR_WIDTH 0x8
> #endif
> @@ -184,6 +188,7 @@ struct drm_backend {
> int32_t cursor_height;
>
> uint32_t pageflip_timeout;
> + bool aspect_ratio_supported;
> };
>
> struct drm_mode {
> @@ -1962,24 +1967,40 @@ 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 = NULL;
'mode' does not need initialization.
> + uint8_t src_aspect = 0;
> + uint8_t target_aspect = 0;
>
> + target_aspect = target_mode->aspect_ratio;
> 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 = output->base.current_mode->aspect_ratio;
> + if (src_aspect == target_aspect)
> +
> + return (struct drm_mode *)output->base.current_mode;
> + }
>
> 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)
> + src_aspect = (mode->mode_info.flags &
> + DRM_MODE_FLAG_PIC_AR_MASK) >>
> + DRM_MODE_FLAG_PIC_AR_BITS_POS;
> + if (src_aspect == target_aspect)
> + 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;
The choose_mode logic looks good.
>
> return tmp_mode;
> }
> @@ -2112,6 +2133,11 @@ init_kms_caps(struct drm_backend *b)
> weston_log("DRM: %s universal planes\n",
> b->universal_planes ? "supports" : "does not support");
>
> + ret = drmSetClientCap(b->drm.fd, DRM_CLIENT_CAP_ASPECT_RATIO, 1);
> + b->aspect_ratio_supported = (ret == 0);
> + weston_log("DRM: %s picture aspect ratio\n",
> + b->aspect_ratio_supported ? "supports" : "does not support");
> +
Good.
> return 0;
> }
>
> @@ -2362,6 +2388,28 @@ destroy_sprites(struct drm_backend *b)
> drm_plane_destroy(plane);
> }
>
> +
> +/**
> + * Get the aspect ratio bits from flag of drmModeModeInfo mode and set the
> + * aspect_ratio in weston_mode structure.
> + *
> + * @param w_mode the weston mode to which aspect ratio is to be added.
> + * @param info the drmModeModeInfo mode from which has the aspect ratio info.
> + */
> +static void
> +drm_set_aspect_ratio(struct weston_mode *w_mode, const drmModeModeInfo *info)
> +{
> + uint8_t aspect_ratio;
> +
> + if (w_mode == NULL || info == NULL) {
> + weston_log("NULL value received, unable to set aspect-ratio\n");
> + return;
We don't do this kind of checks in Weston, this is not a library API
function. You could at most have assert()s, but even those seem
superfluous here.
If this was a function callable from outside of libweston, it would be
more ok.
> + }
> + aspect_ratio = (info->flags & DRM_MODE_FLAG_PIC_AR_MASK) >>
> + DRM_MODE_FLAG_PIC_AR_BITS_POS;
I suppose a more useful function would be one that only converts from
DRM flags into weston_mode_aspect_ratio values, e.g.:
static uint8_t
drm_to_weston_mode_aspect_ratio(drm flags)
> + w_mode->aspect_ratio = aspect_ratio;
> +}
> +
> /**
> * Add a mode to output's mode list
> *
> @@ -2376,6 +2424,7 @@ static struct drm_mode *
> drm_output_add_mode(struct drm_output *output, const drmModeModeInfo *info)
> {
> struct drm_mode *mode;
> + struct drm_backend *b;
> uint64_t refresh;
>
> mode = malloc(sizeof *mode);
> @@ -2402,7 +2451,12 @@ drm_output_add_mode(struct drm_output *output, const drmModeModeInfo *info)
>
> if (info->type & DRM_MODE_TYPE_PREFERRED)
> mode->base.flags |= WL_OUTPUT_MODE_PREFERRED;
> -
> + /* if drm supports aspect ratio, set aspect ratio in weston_mode */
> + b = to_drm_backend(output->base.compositor);
> + if (b->aspect_ratio_supported)
> + drm_set_aspect_ratio(&mode->base, info);
> + else
> + mode->base.aspect_ratio = WESTON_MODE_PIC_AR_NONE;
This looks a little odd.
If the kernel supports aspect ratio, then we can assume that the kernel
sets the flags correctly and we can just copy them.
But, if the kernel does not support aspect ratio, does it guarantee
that the bits used for aspect ratio are zero? I would assume it does,
in which case we can just copy them, we are guaranteed to get NONE.
I would not expect the kernel to return garbage in those bits, so
please correct me if I'm wrong.
Furthermore, if the kernel does not support the aspect ratio flags,
will it also never expose aspect ratio modes to user space? Or is it so
that old kernels would expose aspect ratio modes without knowing they
are aspect ratio modes? In that case, when the kernel does not support
aspect ratio modes, it would be wrong to mark the modes as PIC_AR_NONE,
because they could be anything, and we would need a PIC_AR_UNKNOWN
value for them.
Which way do old kernels work?
> wl_list_insert(output->base.mode_list.prev, &mode->base.link);
>
> return mode;
> @@ -3010,17 +3064,35 @@ 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 = WESTON_MODE_PIC_AR_NONE;
> 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 {
> + weston_log("Unknown aspect ratio given, retaining WESTON_MODE_PIC_AR_NONE\n");
This prints a warning every time a modeline does not contain an aspect
ratio, that's not good. It should handle cases n < 5 without warnings.
Rather than reverting to something the user did not intend and
continue, I'd return failure here. It's an invalid modeline.
> + aspect_ratio = WESTON_MODE_PIC_AR_NONE;
> + }
> +
> + if (n != 2 && n != 3 && n != 5) {
> width = -1;
>
> if (parse_modeline(modeline, &drm_modeline) == 0) {
> @@ -3037,9 +3109,16 @@ 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;
Unnecessary variable.
>
> + src_ar = drm_mode->base.aspect_ratio;
> +
> + if (aspect_ratio == src_ar)
> + configured = drm_mode;
> + else
> + config_fall_back = drm_mode;
> + }
> if (memcmp(current_mode, &drm_mode->mode_info,
> sizeof *current_mode) == 0)
> current = drm_mode;
> @@ -3062,6 +3141,9 @@ drm_output_choose_initial_mode(struct drm_backend *backend,
> if (configured)
> return configured;
>
> + if (config_fall_back)
> + return config_fall_back;
> +
I suppose this ok.
> if (preferred)
> return preferred;
>
> diff --git a/libweston/compositor-drm.h b/libweston/compositor-drm.h
> index 8181492..1c291ac 100644
> --- a/libweston/compositor-drm.h
> +++ b/libweston/compositor-drm.h
> @@ -52,6 +52,51 @@ enum weston_drm_backend_output_mode {
> WESTON_DRM_BACKEND_OUTPUT_PREFERRED,
> };
>
> +/**
> + * aspect ratio info taken from the drmModeModeInfo flag bits 19-22,
> + * which should be used to fill the aspect ratio field in weston_mode.
> + */
> +#define DRM_MODE_FLAG_PIC_AR_BITS_POS 19
> +#ifndef DRM_MODE_FLAG_PIC_AR_MASK
> +#define DRM_MODE_FLAG_PIC_AR_MASK (0xF << DRM_MODE_FLAG_PIC_AR_BITS_POS)
> +#endif
> +
> +#ifndef DRM_MODE_PICTURE_ASPECT_NONE
> +#define DRM_MODE_PICTURE_ASPECT_NONE 0
> +#endif
> +
> +#ifndef DRM_MODE_PICTURE_ASPECT_4_3
> +#define DRM_MODE_PICTURE_ASPECT_4_3 1
> +#endif
> +
> +#ifndef DRM_MODE_PICTURE_ASPECT_16_9
> +#define DRM_MODE_PICTURE_ASPECT_16_9 2
> +#endif
> +
> +#ifndef DRM_MODE_PICTURE_ASPECT_64_27
> +#define DRM_MODE_PICTURE_ASPECT_64_27 3
> +#endif
> +
> +#ifndef DRM_MODE_PICTURE_ASPECT_256_135
> +#define DRM_MODE_PICTURE_ASPECT_256_135 4
> +#endif
> +/* For future aspect ratios supported in kernel */
> +#define DRM_MODE_PICTURE_ASPECT_RESERVED_1 5
> +#define DRM_MODE_PICTURE_ASPECT_RESERVED_2 6
> +enum weston_mode_aspect_ratio {
> + /** The picture aspect ratio values, for the aspect_ratio field of
> + * weston_mode. The values here, are taken from
> + * DRM_MODE_PICTURE_ASPECT_* from drm_mode.h.
> + */
> + WESTON_MODE_PIC_AR_NONE = DRM_MODE_PICTURE_ASPECT_NONE,
> + WESTON_MODE_PIC_AR_4_3 = DRM_MODE_PICTURE_ASPECT_4_3,
> + WESTON_MODE_PIC_AR_16_9 = DRM_MODE_PICTURE_ASPECT_16_9,
> + WESTON_MODE_PIC_AR_64_27 = DRM_MODE_PICTURE_ASPECT_64_27,
> + WESTON_MODE_PIC_AR_256_135 = DRM_MODE_PICTURE_ASPECT_256_135,
> + WESTON_MODE_PIC_AR_RESERVED_1 = DRM_MODE_PICTURE_ASPECT_RESERVED_1,
> + WESTON_MODE_PIC_AR_RESERVED_2 = DRM_MODE_PICTURE_ASPECT_RESERVED_2
> +};
> +
The problem here is that while weston_mode::aspect_ratio field is
defined in compositor.h, the values for the field are defined in
compositor-drm.h which is a backend-specific header. If there was a
compositor based on libweston using any other backend, it would not
have the value definitions as it should not need to include
compositor-drm.h which may pull in DRM-specific additional
dependencies.
It would also be weird to use DRM macros in compositor.h which should
be agnostic of any backend.
I think enum weston_mode_aspect_ratio is good, but the values should be
given as literals, not with the DRM macros, and it should be in
compositor.h. You should still document that the enum specifically
copies the values from DRM UAPI.
> #define WESTON_DRM_OUTPUT_API_NAME "weston_drm_output_api_v1"
>
> struct weston_drm_output_api {
> diff --git a/libweston/compositor.h b/libweston/compositor.h
> index 8b2d2b0..530b9dd 100644
> --- a/libweston/compositor.h
> +++ b/libweston/compositor.h
> @@ -94,6 +94,7 @@ enum weston_led {
>
> struct weston_mode {
> uint32_t flags;
> + uint8_t aspect_ratio;
Good.
> int32_t width, height;
> uint32_t refresh;
> struct wl_list link;
> diff --git a/man/weston.ini.man b/man/weston.ini.man
> index 4cfefc9..d1105ea 100644
> --- a/man/weston.ini.man
> +++ b/man/weston.ini.man
> @@ -363,10 +363,13 @@ 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 RATIO " "Resolution as above and aspect-ratio as length:breadth e.g :"
> + 720x576 at 50 4:3, 1920x1080 at 60 16:9, 2560x1080 at 60 64:27, 4096x2160 at 60 256:156 etc.
> +.BR "preferred " "Uses the preferred mode"
> +.BR "current " "Uses the current crt controller mode"
> +.BR "off " "Disables the output"
> .fi
> .RE
> .RS
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/20171103/ce4e98dd/attachment-0001.sig>
More information about the wayland-devel
mailing list