[PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"
Sharma, Shashank
shashank.sharma at intel.com
Mon Nov 14 16:56:08 UTC 2016
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.
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
More information about the dri-devel
mailing list