[PATCH v2] compositor-drm: Add the aspect ratio parsing support
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Thu Oct 5 10:58:37 UTC 2017
Hi Pekka,
Thanks a lot for your review comments and the suggestions, and pointing
out the things I had missed.
I agree to most of the changes, please find my response inline.
I will shortly send the next version of the patch in a couple of days.
On 10/4/2017 2:13 PM, Pekka Paalanen wrote:
> On Wed, 20 Sep 2017 17:33:00 +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 flags (bits 02-05).
>> - Uses these aspect ratio flags to pick the appropriate mode during
>> modeset.
>> - changes the mode format in configuration file weston.ini to accommodate
>> aspect ratio information as:
>> <width>x<height>@<RefreshRate> <aspect-ratio>.
>> The aspect ratio should be given as <length:breadth>.
>>
>> v2: As per recommendation from Pekka Paalanen, Quentin Glidic,
>> and Daniel Stone, dropped the aspect-ratio info from wayland protocol,
>> thereby avoiding exposure of aspect-ratio to the client.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>> ---
>> libweston/compositor-drm.c | 98 +++++++++++++++++++++++++++++++++++++++-------
>> libweston/compositor-drm.h | 23 +++++++++++
>> man/weston.ini.man | 10 +++--
>> 3 files changed, 113 insertions(+), 18 deletions(-)
> Hi Ankit,
>
> this patch has an issue with API organization.
>
> 'struct weston_mode' is defined in libweston/compositor.h and is common
> to all backends and all frontends (compositor/main.c). Therefore, if
> you add more flags to weston_mode::flags, they also need to be defined
> in compositor.h.
>
> In compositor.c, bind_output() needs to be patched to not relay the new
> flags to Wayland clients, because the Wayland protocol spec does not
> define these flags.
>
> However, I would recommend to just add a new field 'uint32_t
> aspect_ratio' to 'struct weston_mode' and not re-use
> weston_mode::flags. Document the new field to use values from 'enum
> weston_mode_aspect_ratio' but also allow unlisted values in case the
> kernel has added new entries and the weston list is outdated.
>
> That way there is no need to patch the existing uses of
> weston_mode::flags, and we do not mix protocol flags with other flags
> in the same variable.
>
>
> There are more comments below.
That makes perfect sense. I will add new field to the struct weston_mode
instead of
reusing the weston_mode::flags, to avoid the need to mask this info for
the client.
Also, will make provision for the unlisted values for the new entries.
>
>> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
>> index 1a96138..fb84b13 100644
>> --- a/libweston/compositor-drm.c
>> +++ b/libweston/compositor-drm.c
>> @@ -1962,24 +1962,41 @@ 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;
>>
>> + target_aspect =
>> + target_mode->flags & WESTON_MODE_FLAG_PIC_AR_MASK;
>> 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 = mode->mode_info.flags &
>> + WESTON_MODE_FLAG_PIC_AR_MASK;
> 'mode' is being used uninitialized.
Thanks for pointing it out. Will take care in next patchset.
>
>> + if (target_aspect && src_aspect && src_aspect == target_aspect)
>> + return (struct drm_mode *)output->base.current_mode;
> Why must target_aspect and src_aspect be non-zero in addition to being
> equal? Zero aspect ratio value is explicitly defined to be NONE, not
> unknown. We have always assumed NONE means square pixels, because we
> have assumed square pixels for unset and NONE is identical to unset.
You are right, we can do away with checking for aspect ratio none, when
we already have defined it.
The idea here was to avoid the matching if aspect ratio from DRM layer
is DRM_ASPECT_NONE.
DRM_ASPECT_NONE here means the aspect ratio is not given in the EDID for
this mode. NON-CEA modes
do not have this information (we get 0 for the aspect ratio bits), that
would mean 1:1 pixel aspect ratio.
so in those case, we can just skip the match and proceed with the next
mode in the list.
I was not taking into account that, user might also want to set
DRM_MODE_NONE as the aspect ratio,
and was considering it as the case where no aspect ratio is given, which
is indeed flawed.
>> + }
>>
>> 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)
>> + target_mode->refresh == 0) {
> Unlike the kernel alignment rules, we usually align to the beginning
> parenthesis the way it was in the old code.
Missed that. Will take care of the alignment rules.
>
>> + src_aspect = mode->mode_info.flags &
>> + WESTON_MODE_FLAG_PIC_AR_MASK;
>> + if (target_aspect && src_aspect &&
>> + src_aspect == target_aspect)
> The same question: why non-zero aspect flags required for finding a
> match, is equality not enough?
As mentioned above equality would be sufficient.
>
>> + 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;
> I understand the matching idea is, in order:
> - If the current mode matches exactly, return that.
> - If there is a mode in the mode list that matches exactly, return that.
> - If there is a mode in the mode list that matches except for the
> aspect ratio, return that.
> - If there is a mode in the mode list that matches for resolution,
> return that.
> - Return NULL.
>
> Sounds fine.
>
>> }
>
>> @@ -2982,21 +3021,38 @@ 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 = 0;
>> 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
>> + aspect_ratio = WESTON_MODE_PIC_AR_NONE;
> If aspect ratio is given but is not any ratio we recognize, this should
> at least log an error.
Agreed, an error message is required here.
>
>> +
>> + if (n != 2 && n != 3 && n != 5) {
>> width = -1;
>>
>> if (parse_modeline(modeline, &drm_modeline) == 0) {
>> - configured = drm_output_add_mode(output, &drm_modeline);
>> + configured = drm_output_add_mode(output,
>> + &drm_modeline);
>> if (!configured)
>> return NULL;
>> } else {
>> @@ -3009,9 +3065,20 @@ 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;
>> +
>> + src_ar = (drm_mode->base.flags &
>> + WESTON_MODE_FLAG_PIC_AR_MASK) >>
>> + WESTON_MODE_FLAG_PIC_AR_BITS_POS;
>> +
>> + if (aspect_ratio != WESTON_MODE_PIC_AR_NONE
>> + && src_ar != WESTON_MODE_PIC_AR_NONE
>> + && aspect_ratio == src_ar)
> Again the same question about why equality is not enough.
As discussed above, equality is sufficient.
>
>> + configured = drm_mode;
>> + else
>> + config_fall_back = drm_mode;
>> + }
>> if (memcmp(current_mode, &drm_mode->mode_info,
>> sizeof *current_mode) == 0)
>> current = drm_mode;
>> @@ -3034,6 +3101,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..258d467 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 is set in the weston_mode flag bits 2 to 5.
>> + */
>> +#define DRM_MODE_FLAG_PIC_AR_BITS_POS 19
>> +#define WESTON_MODE_FLAG_PIC_AR_BITS_POS 2
>> +#ifndef DRM_MODE_FLAG_PIC_AR_MASK
>> +#define DRM_MODE_FLAG_PIC_AR_MASK (0xF << DRM_MODE_FLAG_PIC_AR_BITS_POS)
>> +#endif
>> +#define WESTON_MODE_FLAG_PIC_AR_MASK (0xF << WESTON_MODE_FLAG_PIC_AR_BITS_POS)
>> +
>> +
>> +enum weston_mode_aspect_ratio {
>> + /** The picture aspect ratio values for the aspect ratio bits 2 to 5 of
>> + * weston_mode flag.
>> + */
> For this enum, it would be good to document that the values here are
> taken from DRM_MODE_PICTURE_ASPECT_* in drm_mode.h and they must match
> the DRM definitions. That will allow you to avoid a conversion table.
Will elaborate more about the Aspect ratio MACROS being taken from
drm_mode.h,
and use of the same DRM definitions.
>
>> + WESTON_MODE_PIC_AR_NONE = 0,
>> + WESTON_MODE_PIC_AR_4_3 = 1,
>> + WESTON_MODE_PIC_AR_16_9 = 2,
>> + WESTON_MODE_PIC_AR_64_27 = 3,
>> + WESTON_MODE_PIC_AR_256_135 = 4
>> +};
>> +
>> #define WESTON_DRM_OUTPUT_API_NAME "weston_drm_output_api_v1"
>>
>> struct weston_drm_output_api {
>> diff --git a/man/weston.ini.man b/man/weston.ini.man
>> index 4cfefc9..16965aa 100644
>> --- a/man/weston.ini.man
>> +++ b/man/weston.ini.man
>> @@ -363,10 +363,12 @@ 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 L:B " "Resolution as above and aspect-ratio as length:breadth"
> Is length and breadth really the official terminology? Intuitively I
> would be confused by what they mean.
>
> Since we can only parse a small fixed list of possible aspect-ratio
> strings, it would be nice to list the valid strings.
Length Breadth is not official terminology, I just wanted to distinguish
from width and height of the mode.
Since we have just a few valid aspect-ratio, it makes sense to add that
as an example.
>> +.BR "preferred " "Uses the preferred mode"
>> +.BR "current " "Uses the current crt controller mode"
>> +.BR "off " "Disables the output"
>> .fi
>> .RE
>> .RS
> Man page update is good.
>
>
> Thanks,
> pq
Thanks & Regards,
Ankit
More information about the wayland-devel
mailing list