<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> Hi Pekka, Apologize for the late reply, as I was on vacation. Thanks for reviewing the patch and ACK by tag. Trying to get the necessary changes in DRM layer, patches are under review. I'll keep the style comments in mind while sending the next patch-set. Please find my comments in-line: <div class="moz-cite-prefix">On Monday 18 December 2017 07:12 PM, Pekka Paalanen wrote: </div> <blockquote type="cite" cite="mid:20171218154221.52a39a71@eldfell"> <pre wrap="">On Tue, 14 Nov 2017 09:32:43 +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. v4: Added a general function to get aspect-ratio info from drmModeModeInfo flags, as suggested by Pekka Paalanen. Took care of minor things suggested in last review. Signed-off-by: Ankit Nautiyal <a class="moz-txt-link-rfc2396E" href="mailto:ankit.k.nautiyal@intel.com"><ankit.k.nautiyal@intel.com></a> --- libweston/compositor-drm.c | 91 +++++++++++++++++++++++++++++++++++++++++----- libweston/compositor-drm.h | 23 ++++++++++++ libweston/compositor.h | 1 + man/weston.ini.man | 11 ++++-- 4 files changed, 112 insertions(+), 14 deletions(-) diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c index b641d61..db1be5b 100644 --- a/libweston/compositor-drm.c +++ b/libweston/compositor-drm.c </pre> </blockquote> <pre wrap=""> Hi Ankit, the logic looks good to me now. With the kernel UABI in mind, this gets Acked-by: Pekka Paalanen <a class="moz-txt-link-rfc2396E" href="mailto:pekka.paalanen@collabora.co.uk"><pekka.paalanen@collabora.co.uk></a> I have style comments below, but nothing that would affect the UABI. I will be expecting a v5 of this patch when the kernel bits have been merged. </pre> <blockquote type="cite"> <pre wrap="">@@ -3010,17 +3054,37 @@ 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 { </pre> </blockquote> <pre wrap=""> Style nitpick: empty line too much; if one branch has braces, all branches should have braces.</pre> </blockquote> Will take care of this in the next patch-set. <blockquote type="cite" cite="mid:20171218154221.52a39a71@eldfell"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ + 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) { @@ -3037,9 +3101,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 </pre> </blockquote> <pre wrap=""> Style nitpick: braces unnecessary, just one simple statement per branch.</pre> </blockquote> Right. Will fix this in the next patch-set. <blockquote type="cite" cite="mid:20171218154221.52a39a71@eldfell"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ config_fall_back = drm_mode; + } if (memcmp(current_mode, &drm_mode->mode_info, sizeof *current_mode) == 0) current = drm_mode; @@ -3062,6 +3130,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..e759f11 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 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 +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 */ + /* Reserved for future */ + WESTON_MODE_PIC_AR_RESERVED_1 = 5, + WESTON_MODE_PIC_AR_RESERVED_2 = 6, </pre> </blockquote> <pre wrap=""> While these may be reserved in the kernel as well, is there a reason to name these unused values like this? If the values become used in the future, they will surely have other names, and by definition these names here should never be used by anyone as far as I understand. Or, since the value is four bits wide in the flags, there are 16 possible values. What would be the difference between reserved and unnamed values? </pre> </blockquote> <pre wrap="">These values are just to allow unlisted values in case the kernel has added new entries and the weston list is outdated. Currently kernel doesn't have any such reserved value. So we can do away with them if required. </pre> <blockquote type="cite" cite="mid:20171218154221.52a39a71@eldfell"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+}; + #define WESTON_DRM_OUTPUT_API_NAME "weston_drm_output_api_v1" </pre> </blockquote> <pre wrap=""> Thanks, pq </pre> </blockquote> Thanks & Regards, Ankit </body> </html>