<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> </head> <body bgcolor="#FFFFFF" text="#000000"> Hi Pekka, Thanks for the review and the comments. I agree with all of the suggestions, will send a patch in a day or two. Discussion about the CEA with aspect ratio and and Non- CEA modes is currently in progress in another thread, lets close it in the other thread itself. Please find my responses inline for the remaining comments: <div class="moz-cite-prefix">On 6/28/2018 7:09 PM, Pekka Paalanen wrote: </div> <blockquote cite="mid:20180628163939.4cb80bfd@eldfell" type="cite"> <pre wrap="">On Sun, 20 May 2018 18:33:01 +0530 "Nautiyal, Ankit K" <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> The flag bits 19-22 of the connector modes, provide the aspect-ratio information. 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. Currently, DRM layer supports aspect-ratio with atomic-modesetting by default. For legacy modeset path, the user-space needs to set the drm client cap for aspect-ratio, if it wants aspect-ratio information in modes. 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>. - modifies the man pages to explain the usage of different mode format configurations in weston.ini. 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. v4: Minor modifications and fixes as suggested by Pekka Paalanen. v5: Rebased, fixed some styling issues, and added aspect-ratio information while printing weston_modes. Signed-off-by: Ankit Nautiyal <a class="moz-txt-link-rfc2396E" href="mailto:ankit.k.nautiyal@intel.com"><ankit.k.nautiyal@intel.com></a> Acked-by: Pekka Paalanen <a class="moz-txt-link-rfc2396E" href="mailto:pekka.paalanen@collabora.co.uk"><pekka.paalanen@collabora.co.uk></a> (v4) --- libweston/compositor-drm.c | 115 +++++++++++++++++++++++++++++++++---- libweston/compositor-drm.h | 21 +++++++ libweston/compositor.h | 1 + man/weston.ini.man | 13 +++-- 4 files changed, 136 insertions(+), 14 deletions(-) </pre> </blockquote> <pre wrap="">Hi Ankit, it's been a long while since I last looked at this, so forgive me if I go in circles with my review comments. I see the aspect ratio bits are in Linus' RC kernel. I would like to get this merged for the next release which basically means during the next week if you can. I didn't see the aspect ratio bits in libdrm master yet, I guess it should be ok to re-import the headers from kernel to libdrm by now. </pre> <blockquote type="cite"> <pre wrap="">diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c index 287431eb..e1d79e49 100644 --- a/libweston/compositor-drm.c +++ b/libweston/compositor-drm.c @@ -73,6 +73,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 @@ -272,6 +276,8 @@ struct drm_backend { uint32_t pageflip_timeout; bool shutting_down; + + bool aspect_ratio_supported; }; struct drm_mode { @@ -3049,6 +3055,19 @@ err: drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0); } +/* + * Get the aspect-ratio from drmModeModeInfo mode flags. + * + * @param drm_mode_flags- flags from drmModeModeInfo structure. + * @returns aspect-ratio as encoded in enum 'weston_mode_aspect_ratio'. + */ +static uint8_t +drm_to_weston_mode_aspect_ratio(uint32_t drm_mode_flags) +{ + return (drm_mode_flags & DRM_MODE_FLAG_PIC_AR_MASK) >> + DRM_MODE_FLAG_PIC_AR_BITS_POS; +} + static void drm_assign_planes(struct weston_output *output_base, void *repaint_data) { @@ -3179,25 +3198,44 @@ 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; + struct drm_backend *b; + b = to_drm_backend(output->base.compositor); + target_aspect = target_mode->aspect_ratio; + src_aspect = output->base.current_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 to_drm_mode(output->base.current_mode); + target_mode->refresh == 0)) { + if (!b->aspect_ratio_supported || src_aspect == target_aspect) + return (struct drm_mode *)output->base.current_mode; </pre> </blockquote> <pre wrap="">Please keep to_drm_mode() instead of converting it back to a cast.</pre> </blockquote> Got it. Will change this in next patch set. <blockquote cite="mid:20180628163939.4cb80bfd@eldfell" type="cite"> <blockquote type="cite"> <pre wrap="">+ } wl_list_for_each(mode, &output->base.mode_list, base.link) { + uint32_t flags = mode->mode_info.flags; + + src_aspect = drm_to_weston_mode_aspect_ratio(flags); </pre> </blockquote> <pre wrap="">This could be simply: src_aspect = mode->base.aspect_ratio; </pre> </blockquote> Yeah right, we have already filled aspect_ratio, so it makes sense to use it here. <blockquote cite="mid:20180628163939.4cb80bfd@eldfell" type="cite"> <blockquote type="cite"> <pre wrap=""> 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) + if (!b->aspect_ratio_supported || + 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; + return tmp_mode; } @@ -3335,6 +3373,11 @@ init_kms_caps(struct drm_backend *b) weston_log("DRM: %s atomic modesetting\n", b->atomic_modeset ? "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"); + return 0; } @@ -3739,6 +3782,8 @@ 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; + mode->base.aspect_ratio = drm_to_weston_mode_aspect_ratio(info->flags); + wl_list_insert(output->base.mode_list.prev, &mode->base.link); return mode; @@ -4605,17 +4650,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 (backend->aspect_ratio_supported && n == 5) { + 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("Invalid modeline \"%s\" for output %s\n", + modeline, output->base.name); + } + if (n != 2 && n != 3 && n != 5) { width = -1; if (parse_modeline(modeline, &drm_modeline) == 0) { @@ -4632,8 +4695,13 @@ 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)) { + if (!backend->aspect_ratio_supported || + aspect_ratio == drm_mode->base.aspect_ratio) + configured = drm_mode; + else + config_fall_back = drm_mode; + } if (memcmp(current_mode, &drm_mode->mode_info, sizeof *current_mode) == 0) @@ -4657,6 +4725,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; @@ -5039,17 +5110,41 @@ drm_output_print_modes(struct drm_output *output) { struct weston_mode *m; struct drm_mode *dm; + char *aspect_ratio; + int ret; wl_list_for_each(m, &output->base.mode_list, link) { dm = to_drm_mode(m); - weston_log_continue(STAMP_SPACE <a class="moz-txt-link-rfc2396E" href="mailto:%dx%d@%.1f%s%s,%.1fMHz%5Cn">"%dx%d@%.1f%s%s, %.1f MHz\n"</a>, + switch (m->aspect_ratio) { + case WESTON_MODE_PIC_AR_4_3: + ret = asprintf(&aspect_ratio, " 4:3"); + break; + case WESTON_MODE_PIC_AR_16_9: + ret = asprintf(&aspect_ratio, " 16:9"); + break; + case WESTON_MODE_PIC_AR_64_27: + ret = asprintf(&aspect_ratio, " 64:27"); + break; + case WESTON_MODE_PIC_AR_256_135: + ret = asprintf(&aspect_ratio, " 256:135"); + break; + default: + ret = asprintf(&aspect_ratio, "%s", ""); + } </pre> </blockquote> <pre wrap="">What's the idea with asprintf here? Why not: static const char * const aspect_ratio_as_string[] = { [WESTON_MODE_PIC_AR_NONE] = "", [WESTON_MODE_PIC_AR_4_3] = " 4:3", ... }; static const char * aspect_ratio_to_string(enum weston_mode_aspect_ratio ratio) { if (ratio < 0 || ratio >= ARRAY_SIZE(aspect_ratio_as_string) || !aspect_ratio_as_string[ratio]) return " (unknown aspect ratio)" return aspect_ratio_as_string[ratio]; } </pre> </blockquote> I think I just focused on printing the aspect-ratio names in the modes info. Thanks for your suggestion. I will use the global constant array. <blockquote cite="mid:20180628163939.4cb80bfd@eldfell" type="cite"> <blockquote type="cite"> <pre wrap="">+ if (ret < 0) { + weston_log("Error in asprintf, can't print modes.\n"); + return; + } + weston_log_continue(STAMP_SPACE <a class="moz-txt-link-rfc2396E" href="mailto:%dx%d@%.1f%s%s%s,%.1fMHz%5Cn">"%dx%d@%.1f%s%s%s, %.1f MHz\n"</a>, m->width, m->height, m->refresh / 1000.0, + aspect_ratio, m->flags & WL_OUTPUT_MODE_PREFERRED ? ", preferred" : "", m->flags & WL_OUTPUT_MODE_CURRENT ? ", current" : "", dm->mode_info.clock / 1000.0); + free(aspect_ratio); } } diff --git a/libweston/compositor-drm.h b/libweston/compositor-drm.h index 68f93eab..61ad44dc 100644 --- a/libweston/compositor-drm.h +++ b/libweston/compositor-drm.h @@ -52,6 +52,27 @@ 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 + </pre> </blockquote> <pre wrap="">The above is ok in compositor-drm.h, but it could be in compositor-drm.c instead, since it's not used anywhere else so far. </pre> </blockquote> Noted. Will do in next patch-set. <blockquote cite="mid:20180628163939.4cb80bfd@eldfell" type="cite"> <pre wrap="">The below could be in compositor.h instead so that... </pre> <blockquote type="cite"> <pre wrap="">+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 = 0, /* DRM_MODE_PICTURE_ASPECT_NONE */ + WESTON_MODE_PIC_AR_4_3 = 1, /* DRM_MODE_PICTURE_ASPECT_4_3 */ + WESTON_MODE_PIC_AR_16_9 = 2, /* DRM_MODE_PICTURE_ASPECT_16_9 */ + WESTON_MODE_PIC_AR_64_27 = 3, /* DRM_MODE_PICTURE_ASPECT_64_27 */ + WESTON_MODE_PIC_AR_256_135 = 4, /* DRM_MODE_PICTURE_ASPECT_256_135*/ +}; + #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 47337d8a..3dedbbfe 100644 --- a/libweston/compositor.h +++ b/libweston/compositor.h @@ -95,6 +95,7 @@ enum weston_led { struct weston_mode { uint32_t flags; + uint8_t aspect_ratio; </pre> </blockquote> <pre wrap="">...the type of this could be enum weston_mode_aspect_ratio. None of it actually depends on libdrm, so compositor.h would be fine. The docs are good. First I thought that aspect_ratio would belong in drm_mode rather than weston_mode, but thinking ahead, we should get the mode choosing out of libweston and into the compositor, so it makes sense to have aspect_ratio in weston_mode for the future.</pre> </blockquote> Yes we can do that. Is there plan to move the choose_mode...( ) in compositor ? What about other non-drm compositors. will they be using the weston_mode::aspect_ratio? <blockquote cite="mid:20180628163939.4cb80bfd@eldfell" type="cite"> <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 f237fd60..e982cac9 100644 --- a/man/weston.ini.man +++ b/man/weston.ini.man @@ -371,10 +371,15 @@ 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" </pre> </blockquote> <pre wrap="">I think the length:breadth could be confusing. H:V? hor:vert? Or simply A:B, assuming that aspect ratio is general knowledge. It would also be worth to list all the acceptable values, because there are only few. One cannot give e.g. 8:6 and assume Weston figures out it means 4:3. If they are explicitly listed, we could just use RATIO and not split the value further.</pre> </blockquote> Alright, we can just list the valid values for aspect-ratio. Instead of length:breadth we can just give RATIO. <blockquote cite="mid:20180628163939.4cb80bfd@eldfell" type="cite"> <pre wrap="">Since these are specific to the DRM-backend, they would be more logical in weston-drm.man.</pre> </blockquote> Do you mean, to have the same information in weston-drm.man (just like mode, modeline etc) , along with the weston.ini.man? <blockquote cite="mid:20180628163939.4cb80bfd@eldfell" type="cite"> <blockquote type="cite"> <pre wrap="">+ +e.g. 720x576@50 4:3, 1920x1080@60 16:9, 2560x1080@60 64:27, 4096x2160@60 256:135 etc. </pre> </blockquote> <pre wrap="">What is the point of giving the aspect ratio explicitly if the resolution directly results in the same aspect ratio? Shouldn't the examples show cases where it actually matters, i.e. when the display shows non-square pixels? E.g. 1024x768 16:9 </pre> </blockquote> These were just for the example. we can have better examples :) User can give 1920x1080@ 64:27, it will be applied provided if its in the list of connector modes. <blockquote cite="mid:20180628163939.4cb80bfd@eldfell" type="cite"> <pre wrap="">The implementation also makes a difference between implicit aspect ratio (that is, NONE) and explicit aspect ratio even if the two are equal. Is that intentional? Is there an actual difference? If the kernel or the user defines an aspect ratio that is equal to the resolution aspect ratio, should we convert the aspect ratio to WESTON_MODE_PIC_AR_NONE?</pre> </blockquote> Discussion continuing with Shashank in another thread. <blockquote cite="mid:20180628163939.4cb80bfd@eldfell" type="cite"> <blockquote type="cite"> <pre wrap="">+ +.BR "preferred " "Uses the preferred mode" +.BR "current " "Uses the current crt controller mode" +.BR "off " "Disables the output" </pre> </blockquote> <pre wrap="">These are actually quite specific to the DRM-backend and would belong more in weston-drm.man. Would be nice to move them in a separate patch before the aspect ratio support if you wish.</pre> </blockquote> So shall we have two patches? first for adding aspect ratio info through weston.ini in weston-drm.man, and second with the actual changes? <blockquote cite="mid:20180628163939.4cb80bfd@eldfell" type="cite"> <blockquote type="cite"> <pre wrap=""> .fi .RE .RS </pre> </blockquote> <pre wrap="">Thanks, pq </pre> </blockquote> </body> </html>