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

Nautiyal, Ankit K ankit.k.nautiyal at intel.com
Tue Jul 3 10:26:10 UTC 2018


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:


On 6/28/2018 7:09 PM, Pekka Paalanen wrote:
> On Sun, 20 May 2018 18:33:01 +0530
> "Nautiyal, Ankit K"<ankit.k.nautiyal at intel.com>  wrote:
>
>> From: Ankit Nautiyal<ankit.k.nautiyal at intel.com>
>>
>> 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<ankit.k.nautiyal at intel.com>
>>
>> Acked-by: Pekka Paalanen<pekka.paalanen at collabora.co.uk>  (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(-)
> 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.
>
>> 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;
> Please keep to_drm_mode() instead of converting it back to a cast.
Got it. Will change this in next patch set.
>> +	}
>>   
>>   	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);
> This could be simply:
> 	src_aspect = mode->base.aspect_ratio;

Yeah right, we have already filled aspect_ratio, so it makes sense to 
use it here.

>>   		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"%dx%d@%.1f%s%s, %.1f MHz\n",
>> +		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", "");
>> +		}
> 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];
> }
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.

>> +		if (ret < 0) {
>> +			weston_log("Error in asprintf, can't print modes.\n");
>> +			return;
>> +		}
>> +		weston_log_continue(STAMP_SPACE"%dx%d@%.1f%s%s%s, %.1f MHz\n",
>>   				    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
>> +
> 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.

Noted. Will do in next patch-set.

> The below could be in compositor.h instead so that...
>
>> +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;
> ...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.

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?

>>   	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 at RR            " "Resolution as above and refresh-rate in Hertz"
>> +.BR "WIDTHxHEIGHT at RR RATIO      " "Resolution as above and aspect-ratio as length:breadth"
> 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.
Alright, we can just list the valid values for aspect-ratio.
Instead of length:breadth we can just give RATIO.

> Since these are specific to the DRM-backend, they would be more logical
> in weston-drm.man.

Do you mean, to have the same information in weston-drm.man (just like 
mode, modeline etc) , along with the weston.ini.man?


>> +
>> +e.g. 720x576 at 50 4:3, 1920x1080 at 60 16:9, 2560x1080 at 60 64:27, 4096x2160 at 60 256:135 etc.
> 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
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.

> 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?
Discussion continuing with Shashank in another thread.

>> +
>> +.BR "preferred                  " "Uses the preferred mode"
>> +.BR "current                    " "Uses the current crt controller mode"
>> +.BR "off                        " "Disables the output"
> 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.

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?

>>   .fi
>>   .RE
>>   .RS
> Thanks,
> pq

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180703/b4ae0e20/attachment-0001.html>


More information about the wayland-devel mailing list