[PATCH v4] compositor-drm: Add the aspect ratio parsing support

[Pekka Paalanen]

On Tue, 14 Nov 2017 09:32:43 +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.
> 
> 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 <ankit.k.nautiyal at intel.com>
> ---
>  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

Hi Ankit,

the logic looks good to me now. With the kernel UABI in mind, this gets

Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

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.

> @@ -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 {

Style nitpick: empty line too much; if one branch has braces, all
branches should have braces.

> +
> +				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

Style nitpick: braces unnecessary, just one simple statement per branch.

> +				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,

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?


> +};
> +
>  #define WESTON_DRM_OUTPUT_API_NAME "weston_drm_output_api_v1"
>  

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/20171218/7bb5a856/attachment.sig>