[PATCH v5] compositor-drm: Add aspect-ratio parsing support
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Tue Jul 3 11:33:36 UTC 2018
Hi Shashank,
Thanks for the detailed explanation about the relationship between the
aspect-ratio, CEA modes and VIC codes.
I will capture these and try to add this as part of man pages for a
better understanding.
Regards,
Ankit
On 7/3/2018 4:05 PM, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 7/3/2018 3:26 PM, Pekka Paalanen wrote:
>> On Tue, 3 Jul 2018 14:42:06 +0530
>> "Sharma, Shashank" <shashank.sharma at intel.com> wrote:
>>
>>> Hello Pekka,
>>>
>>> Thanks for your attention and code review of the patch series.
>>> Ankit will respond on rest of the review comments, I would take one
>>> related to mode with implicit aspect ratio information.
>>> Please find my comment inline.
>>>
>>> Regards
>>> Shashank
>>> 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.
>>
>>>>> 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.
>>>>
>>>> Since these are specific to the DRM-backend, they would be more
>>>> logical
>>>> in weston-drm.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
>>>>
>>>> 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?
>>> Yes there is an actual difference, let me try to explain this.
>>> For this, we should first understand difference between CEA-modes Vs
>>> non-CEA-modes. As you might already know, CEA defines timings of a
>>> video
>>> mode, which is considered as standard for HDMI certification and
>>> compliance testing. CEA defines each and every parameter of a video
>>> mode, like h/vactive, h/vfront, h/vback including aspect ratio
>>> information. This unique videmode is given a Video Identification Code
>>> (VIC) which is unique. For example, VIC=4 is 1280x720 at 60 aspect 16:9.
>> Hi,
>>
>> this was new to me, thank you for explaining.
>>
>>> Now, the videomode would be considered a CEA mode, only if it contains
>>> aspect ratio information. In other words:
>>> - 1280x720 at 60 0:0 is not a CEA mode
>>> - 1280x720 at 60 16:9 is a CEA mode with VIC=4
>> But the latter might not be a CEA mode either, if the detailed
>> parameters are computed with reduced blanking?
>>
>> Userspace is free to use e.g. CVT to compute a detailed modeline and
>> also set aspect ratio flags. Will the kernel compare the full detailed
>> modeline from userspace with the VIC database to find a VIC, or does it
>> perhaps ignore some bits of the detailed modeline?
> Yes, you are right. as kernel compares each video timing parameter, if
> Weston picks a VESA reduced blanking mode over a CEA mode, that would
> cause a mismatch.
>>> Now, when we apply videomode 1280x720 at 60 0:0, as this mode timings
>>> doesn't exactly match the CEA information for VIC=4, so would be called
>>> a non-CEA mode. So while setting the HDMI AVI infoframe filed, VIC
>>> parameter will be kept as '0' . But when we apply videomode 1280x720 at 60
>>> 16:9, this matches CEA timings for the mode with VIC=4, so the VIC
>>> information in AVI infoframes would be filled with value 4.
>>>
>>> When certification testing is done, in tests like 7-27, the HDMI
>>> analyzer applies a particular CEA mode, and expects the applied mode to
>>> be with exactly same timings, including the aspect ratio and VIC field.
>>> So its important to differentiate between mode with no aspect (NONE)
>>> and
>>> mode with explicit aspect ratio.
>> I think this is nuanced enough that there should be some documentation
>> about the difference of modes between with and without aspect ratio in
>> the man pages for Weston. It was certainly all new to me.
> I agree, we can work on adding this documentation about aspect ratios
> and their significance to differentiate between videomodes. Ankit, can
> you please help here ?
>>
>> I do wonder if the current modeline behaviour in Weston is good enough.
>> If user defines a "vague" mode like 1280x720 at 60 16:9 instead of a
>> detailed modeline, libweston will search for a compatible mode in the
>> EDID provided list of video modes. If one is found, it is used, but who
>> knows if the detailed mode by EDID is actually the VIC mode the user is
>> wishing for. The only sure way would be to define a detailed modeline
>> to override EDID. But, I digress. We can talk about choosing a video
>> modes in another thread if it is an issue.
> One good thing is that, in this particular case, while running the
> test cases, most of the HDMI analyzers provide one of the following
> method:
> - They generate EDID on the fly with only 1 mode in it, which is the
> CEA test mode. In this way, kernel will register only one mode in the
> connector's->modes and userspace would be forced to pick that mode for
> modeset.
> - They allow a device under test, to run their own utility which can
> generate a modeset of a choice, bypassing the UI manager.
> I guess in both the cases kernel will parse the test mode, and if
> userspace picks a mode from connector;s->modes, it should be good.
> But, as you mentioned, in general it's a good idea to refine how to
> translate a vague mode of choice from user, into something in kernel's
> modedb or monitor's EDID, for example
> - If mode is with non-zero aspect ratio, Weston should understand that
> its a CEA mode, and search from videomodes in EDID's CEA section
> first, than detailed modes.
> - If mode is with zero aspect ratio, then we will have to first start
> searching from preferred mode/detailed modes and then look for CEA
> modes and standard modes.
>
> Probably a separate thread to discus this topic would be a more suitable.
>
> - Shashank
>>
>> Thanks,
>> pq
>>
>>> - Shashank
>>>
>>>> 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?
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list