[PATCH v3] compositor-drm: Add the aspect ratio parsing support
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Thu Nov 9 13:54:00 UTC 2017
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.
On 11/3/2017 3:52 PM, Pekka Paalanen wrote:
> (I fixed the subject line to contain v3.)
>
> On Mon, 16 Oct 2017 20:25:23 +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.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
> Hi Ankit,
>
> thank you for this, we are getting closer. I have some review comments
> and some kernel behaviour questions below.
>
>> ---
>> 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;
> 'mode' does not need initialization.
Will fix this in the next patch.
>
>> + 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;
> The choose_mode logic looks good.
>
>>
>> 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");
>> +
> Good.
>
>> 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;
> 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.
Will take care of this.
>> + }
>> + aspect_ratio = (info->flags & DRM_MODE_FLAG_PIC_AR_MASK) >>
>> + DRM_MODE_FLAG_PIC_AR_BITS_POS;
> 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)
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.
>> + 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;
> 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?
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:
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.
>> 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");
> 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.
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.
>> + 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;
> Unnecessary variable.
>
>>
>> + 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;
>> +
> I suppose this ok.
>
>> 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
>> +};
>> +
> 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.
>
Will use the literal values, along with documentation about the DRM Macros.
>
>> #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;
> Good.
>
>> 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 at RR " "Resolution as above and refresh-rate in Hertz"
>> +.BR "WIDTHxHEIGHT at RR RATIO " "Resolution as above and aspect-ratio as length:breadth e.g :"
>> + 720x576 at 50 4:3, 1920x1080 at 60 16:9, 2560x1080 at 60 64:27, 4096x2160 at 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
> Good.
>
>
> Thanks,
> pq
Thanks & Regards,
Ankit
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20171109/3d2ad38f/attachment-0001.html>
More information about the wayland-devel
mailing list