<html> <head> <meta content="text/html; charset=utf-8" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> Hi Pekka, Many thanks for the review comments, and suggestions. I will address these in the next patch. Please find my clarifications/comments inline below. <div class="moz-cite-prefix">On 11/3/2017 3:52 PM, Pekka Paalanen wrote: </div> <blockquote cite="mid:20171103122222.4fb6e11e@eldfell" type="cite"> <pre wrap="">(I fixed the subject line to contain v3.) On Mon, 16 Oct 2017 20:25:23 +0530 Nautiyal Ankit <a class="moz-txt-link-rfc2396E" href="mailto:ankit.k.nautiyal@intel.com"><ankit.k.nautiyal@intel.com></a> wrote: </pre> <blockquote type="cite"> <pre wrap="">From: Ankit Nautiyal <a class="moz-txt-link-rfc2396E" href="mailto:ankit.k.nautiyal@intel.com"><ankit.k.nautiyal@intel.com></a> DRM layer populates aspect ratio information for CEA video modes, but we lose it while parsing modeline (Aspect ratio patch series in drm layer: <a class="moz-txt-link-freetext" href="https://patchwork.freedesktop.org/series/10850">https://patchwork.freedesktop.org/series/10850</a>). 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@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 <a class="moz-txt-link-rfc2396E" href="mailto:ankit.k.nautiyal@intel.com"><ankit.k.nautiyal@intel.com></a> </pre> </blockquote> <pre wrap=""> Hi Ankit, thank you for this, we are getting closer. I have some review comments and some kernel behaviour questions below. </pre> <blockquote type="cite"> <pre wrap="">--- 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; </pre> </blockquote> <pre wrap=""> 'mode' does not need initialization.</pre> </blockquote> Will fix this in the next patch. <blockquote cite="mid:20171103122222.4fb6e11e@eldfell" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ 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; </pre> </blockquote> <pre wrap=""> The choose_mode logic looks good. </pre> <blockquote type="cite"> <pre wrap=""> 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"); + </pre> </blockquote> <pre wrap=""> Good. </pre> <blockquote type="cite"> <pre wrap=""> 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; </pre> </blockquote> <pre wrap=""> 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. </pre> </blockquote> Will take care of this. <blockquote cite="mid:20171103122222.4fb6e11e@eldfell" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ } + aspect_ratio = (info->flags & DRM_MODE_FLAG_PIC_AR_MASK) >> + DRM_MODE_FLAG_PIC_AR_BITS_POS; </pre> </blockquote> <pre wrap=""> 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) </pre> </blockquote> Agreed. In the previous patch, I was modifying the weston_mode flags, therefore the function, but now as we are just setting the aspect_ratio value, your suggestion makes perfect sense. <blockquote cite="mid:20171103122222.4fb6e11e@eldfell" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ 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; </pre> </blockquote> <pre wrap=""> 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?</pre> </blockquote> Before the aspect-ratio patches in drm: The aspect-ratio modes will be there in the list of modes, but the modes will have aspect-ratio flag bits (19-22) set as 0. If the user-land sends a mode, with the aspect-bits sets, those bits will be ignored. So yes, you are right, we can do away with the check here, and set the aspect ratio according to the bits received. As you already know, there is currently aspect-ratio patch in review, and I am working on the adding the aspect-ratio client cap, patch submission is due. After aspect-ratio patches in drm: <blockquote>If user-land sets drm Client for aspect ratio (supports aspect ratio): 1. The modes will be listed and aspect ratio flag bits will be set. 2. The mode given by user-land to be set will be parsed for the aspect ratio flags. If user-land does not set drm client cap for aspect-ratio: 1. The modes with aspect-ratio, will be pruned from the list of modes for a given connector. 2. If user-land gives any mode to be set, the flags for the aspect-ratio for the usermode will be discarded. </blockquote> <blockquote cite="mid:20171103122222.4fb6e11e@eldfell" type="cite"> <blockquote type="cite"> <pre wrap=""> 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"); </pre> </blockquote> <pre wrap=""> 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. </pre> </blockquote> Noted. Will proceed to check the aspect ratio only for n = 5. In that case, if the aspect ratio is not proper, will return with invalid modeline. <blockquote cite="mid:20171103122222.4fb6e11e@eldfell" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ 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; </pre> </blockquote> <pre wrap=""> Unnecessary variable. </pre> <blockquote type="cite"> <pre wrap=""> + 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; + </pre> </blockquote> <pre wrap=""> I suppose this ok. </pre> <blockquote type="cite"> <pre wrap=""> 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 +}; + </pre> </blockquote> <pre wrap=""> 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. </pre> </blockquote> Will use the literal values, along with documentation about the DRM Macros. <blockquote cite="mid:20171103122222.4fb6e11e@eldfell" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap=""> #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; </pre> </blockquote> <pre wrap=""> Good. </pre> <blockquote type="cite"> <pre wrap=""> 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@RR " "Resolution as above and refresh-rate in Hertz" +.BR "WIDTHxHEIGHT@RR RATIO " "Resolution as above and aspect-ratio as length:breadth e.g :" + 720x576@50 4:3, 1920x1080@60 16:9, 2560x1080@60 64:27, 4096x2160@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 </pre> </blockquote> <pre wrap=""> Good. Thanks, pq </pre> </blockquote> Thanks & Regards, Ankit </body> </html>