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 > > > 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
