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

Tue Jul 3 10:35:23 UTC 2018

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?
>>>   