[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:42:04 UTC 2016
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.
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