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
