[PATCH v4] compositor-drm: Add the aspect ratio parsing support
ankit.k.nautiyal at intel.com
ankit.k.nautiyal at intel.com
Thu Jan 18 11:07:04 UTC 2018
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:
On Monday 18 December 2017 07:12 PM, Pekka Paalanen wrote:
> 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.
Will take care of this in the next patch-set.
>
>> +
>> + 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.
Right. Will fix this in the next patch-set.
>
>> + 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?
>
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.
>> +};
>> +
>> #define WESTON_DRM_OUTPUT_API_NAME "weston_drm_output_api_v1"
>>
> Thanks,
> pq
Thanks & Regards,
Ankit
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180118/1abd5143/attachment-0001.html>
More information about the wayland-devel
mailing list