Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

["Sharma, Shashank" <shashank.sharma@intel.com>]

Regards

Shashank


On 11/15/2016 2:21 PM, Daniel Vetter wrote:
> On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
>> On 11/14/2016 10:15 PM, Ville Syrjälä wrote:
>>> On Mon, Nov 14, 2016 at 10:12:04PM +0530, Sharma, Shashank wrote:
>>>> Regards
>>>>
>>>> Shashank
>>>>
>>>>
>>>> On 11/14/2016 9:50 PM, Ville Syrjälä wrote:
>>>>> On Mon, Nov 14, 2016 at 09:37:18PM +0530, Sharma, Shashank wrote:
>>>>>> Regards
>>>>>>
>>>>>> Shashank
>>>>>>
>>>>>>
>>>>>> On 11/14/2016 9:19 PM, Ville Syrjälä wrote:
>>>>>>> On Mon, Nov 14, 2016 at 08:14:34PM +0530, Sharma, Shashank wrote:
>>>>>>>> Regards
>>>>>>>> Shashank
>>>>>>>>> the revert:
>>>>>>>>>
>>>>>>>>>       HDMI2 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 700mm x 390mm
>>>>>>>>> -   1920x1080     60.00*+
>>>>>>>>> -   1920x1080i    60.00    50.00
>>>>>>>>> +   1920x1080     60.00*+  50.00    59.94    30.00    25.00    24.00    29.97    23.98
>>>>>>>>> +   1920x1080i    60.00    50.00    59.94
>>>>>>>>>          1600x1200     60.00
>>>>>>>>>          1680x1050     59.88
>>>>>>>>>          1280x1024     75.02    60.02
>>>>>>>>> @@ -13,30 +13,29 @@
>>>>>>>>>          1360x768      60.02
>>>>>>>>>          1280x800      59.91
>>>>>>>>>          1152x864      75.00
>>>>>>>>> -   1280x720      60.00    50.00
>>>>>>>>> +   1280x720      60.00    50.00    59.94
>>>>>>>>>          1024x768      75.03    70.07    60.00
>>>>>>>>>          832x624       74.55
>>>>>>>>>          800x600       72.19    75.00    60.32
>>>>>>>>> -   640x480       75.00    72.81    66.67    59.94
>>>>>>>>> +   720x576       50.00
>>>>>>>>> +   720x480       60.00    59.94
>>>>>>>>> +   640x480       75.00    72.81    66.67    60.00    59.94
>>>>>>>>>          720x400       70.08
>>>>>>>> None of these aspect ratios are new modes / new aspect ratios from HDMI
>>>>>>>> 2.0/CEA-861-F
>>>>>>>> These are the existing modes, and should be independent of reverted
>>>>>>>> patches.
>>>>>>> They're affected because your patches changed them by adding the aspect
>>>>>>> ratio flags to them.
>>>>>> Yes, But they are independent of reverted patch, which adds aspect ratio
>>>>>> for HDMI 2.0 ratios (64:27 and 256:135)
>>>>> The second patch had to be reverted so that the first patch would revert
>>>>> cleanly.
>>>>>
>>>>>>>>> This was with sna, which does this:
>>>>>>>>>       #define KNOWN_MODE_FLAGS ((1<<14)-1)
>>>>>>>>>       if (mode->status == MODE_OK && kmode->flags & ~KNOWN_MODE_FLAGS)
>>>>>>>>>       	mode->status = MODE_BAD; /* unknown flags => unhandled */
>>>>>>>>> so all the modes with an aspect ratio just vanished.
>>>>>>>>>
>>>>>>>>> -modesetting and -ati on the other hand just copy over the unknown
>>>>>>>>> bits into the xrandr mode structure, which sounds dubious at best:
>>>>>>>>>       mode->Flags = kmode->flags; //& FLAG_BITS;
>>>>>>>>> I've not checked what damage it can actually cause.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> It looks like a few modes disappeared from the kernel's mode list
>>>>>>>>> as well, presumably because some cea modes in the list originated from
>>>>>>>>> DTDs and whanot so they don't have an aspect ratio and that causes
>>>>>>>>> add_alternate_cea_modes() to ignore them. So not populating an aspect
>>>>>>>>> ratio for cea modes originating from a source other than
>>>>>>>>> edid_cea_modes[] looks like another bug to me as well.
>>>>>>>> I am writing a patch series to cap the aspect ratio implementation under
>>>>>>>> a drm_cap_hdmi2_aspect_ratios
>>>>>>>> This is how its going to work (inspired from the 2D/stereo series from
>>>>>>>> damien L)
>>>>>>>>
>>>>>>>> - Add a new capability hdmi2_ar
>>>>>>> It should be just a generic "expose aspect ratio flags to userspace?"
>>>>>> Makes sense, in this way we can even revert the aspect_ratio property
>>>>>> for HDMI connector, as discussed during
>>>>>> the code review sessions of this patch series. In this way, when kernel
>>>>>> will expose the aspect ratios, it will either
>>>>>> do the aspect ratios as per EDID, or wont.
>>>>>>>> - by default parsing the new hdmi 2.0 aspect ratio will be disabled
>>>>>>>> under check of this cap
>>>>>>>> - during bootup time, while initializing the display, a userspace can
>>>>>>>> get_cap on the hdmi2_aspect_ratio
>>>>>>>> - If it wants HDMI 2.0 aspect ratio support, it will set the cap, and
>>>>>>>> kernel will expose these aspect ratios
>>>>>>>>> Another bug I think might be the ordering of the modes with aspect ratio
>>>>>>>>> specified. IIRC the spec says that the preferred aspect ratio should be
>>>>>>>>> listed first in the EDID, but I don't think we preserve that ordering
>>>>>>>>> in the final mode list. I guess we could fix that by somehow noting
>>>>>>>>> which aspect ratio is preferred and sort based on that, or we try to
>>>>>>>>> preserve the order from the EDID until we're ready to sort, and then do
>>>>>>>>> the sorting with a stable algorithm.
>>>>>>>> AFAIK The mode order and priority is decided and arranged in userspace,
>>>>>>>> based on various factors like
>>>>>>>> - preferred mode.
>>>>>>>> - previously applied mode in previous sessions (like for android tvs)
>>>>>>>> - Bigger h/w vs better refresh rate ?
>>>>>>>> - Xserver applies its own algorithms to decide which mode should be
>>>>>>>> shown first.
>>>>>>> Xorg does sort on its own. But since it doesn't know anything about
>>>>>>> aspect ratios and whatnot I wouldn't rely on that for anything. I
>>>>>>> also wouldn't expect eg. wayland compositors to do their own sorting.
>>>>>>> And yeah, looks like weston at least doesn't do any sorting whatsoever.
>>>>>>>
>>>>>>>> I dont think kernel needs to bother about it.
>>>>>>> So I'm going to say that we in fact do need to bother.
>>>>>>>
>>>>>> IMHO, making policies for UI is not a part of kernel design, a UI
>>>>>> manager (Hardware composed, X or Wayland) should take care of it, as
>>>>>> they have access to much information (Like previously applied mode, user
>>>>>> preference etc). When it comes to sorting of modes, the only general rule
>>>>>> across drivers like FB, V4L2, I have seen is the first mode in the list
>>>>>> should be preferred mode, which we are still keeping. And after that our
>>>>>> probed_modes were
>>>>>> anyways not sorted now, so it doesn't matter further.
>>>>> Having userspace be responsible for sorting the aspect ratios would
>>>>> perhaps require that userspace parses the EDID, which is pretty crazy.
>>>> Why ?
>>>> userspace has to just set cap for aspect ratio, and kernel can read
>>>> EDID, parse the CEA block, populate the aspect ratios flags
>>>> and add the modes (Just what this patch was doing, except the cap part)
>>>> Once userspace has the getResources/getConnector call filled, it can
>>>> access all the modes (with and without aspect) and do the sorting
>>>> in any way it wants.
>>>>> I guess it could try to deduce something from the physical aspect ratio
>>>>> of the display, but I'm not sure that's quite what we want either.
>>>>>
>>>>> Also we already sort the modes in the kernel anyway, so it's not like
>>>>> we'd be doing something new by also considering the aspect ratios.
>>>>> I would at the very least want to avoid a totally random order between
>>>>> modes that differ only by the aspect ratio.
>>>> Path: get_connector -> probe_single_connector_mode -> drm_add_edid_modes
>>>> Again, IMHO, we don't sort the modes in kernel, we populate modes in a
>>>> particular order, which is:
>>>> (From drm_edid.c::drm_add_edid_modes)
>>>> ##############################################################
>>>> /*
>>>>         * EDID spec says modes should be preferred in this order:
>>>>         * - preferred detailed mode
>>>>         * - other detailed modes from base block
>>>>         * - detailed modes from extension blocks
>>>>         * - CVT 3-byte code modes
>>>>         * - standard timing codes
>>>>         * - established timing codes
>>>>         * - modes inferred from GTF or CVT range information
>>>>         *
>>>>         * We get this pretty much right.
>>>>         *
>>>>         * XXX order for additional mode types in extension blocks?
>>>>         */
>>>>        num_modes += add_detailed_modes(connector, edid, quirks);
>>>>        num_modes += add_cvt_modes(connector, edid);
>>>>        num_modes += add_standard_modes(connector, edid);
>>>>        num_modes += add_established_modes(connector, edid);
>>>>        num_modes += add_cea_modes(connector, edid);
>>>>        num_modes += add_alternate_cea_modes(connector, edid);
>>>>        num_modes += add_displayid_detailed_modes(connector, edid);
>>>> ###############################################################
>>>>
>>>> Here the modes are added in the connector, in the same order they are
>>>> arranged into their respective blocks in EDID.
>>>> But the order to read the block is a preferred order (no sorting).
>>>>
>>>> Now, in this patch series, we are adding aspect ratio information in
>>>> edid_cea_modes db, which is going to affect only
>>>> add_cea/alternate_cea_modes() call, and the modes accordingly.
>>>> Please let me know if I misunderstood something here.
>>> We explicitly sort the modes after this.
>> In any case, I guess addition of a cap for aspect ratio should fix the
>> current objections for this implementation.
>>
>> And I will keep it 0 by default, so that no aspect ratio information is
>> added until userspace sets the cap to 1 on its own.
> Note that cap = needs new userspace.
> -Daniel
I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
Is that what you mean ?

>>
>> Regards
>>
>> Shashank
>>
>>>> Regards
>>>> Shashank
>>>>>> If X server doesn't know what to do with aspect ratio flags, it can
>>>>>> chose not to set the cap, and if HWC knows, it can chose to set. This is
>>>>>> the same situation as 2D stereo modes
>>>>>> which are existing already.
>>>>>>
>>>>>> Regards
>>>>>> Shashank

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel