On Mon, 5 Jul 2021 17:49:42 +0200 Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx> wrote: > Am 01.07.21 um 15:24 schrieb Pekka Paalanen: > > On Thu, 1 Jul 2021 14:50:13 +0200 > > Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx> wrote: > > > >> Am 01.07.21 um 10:07 schrieb Pekka Paalanen: > >> > >>> On Wed, 30 Jun 2021 11:20:18 +0200 > >>> Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx> wrote: > >>> > >>>> Am 30.06.21 um 10:41 schrieb Pekka Paalanen: > >>>> > >>>>> On Tue, 29 Jun 2021 13:39:18 +0200 > >>>>> Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx> wrote: > >>>>> > >>>>>> Am 29.06.21 um 13:17 schrieb Pekka Paalanen: > >>>>>>> On Tue, 29 Jun 2021 08:12:54 +0000 > >>>>>>> Simon Ser <contact@xxxxxxxxxxx> wrote: > >>>>>>> > >>>>>>>> On Tuesday, June 22nd, 2021 at 09:15, Pekka Paalanen <ppaalanen@xxxxxxxxx> wrote: > >>>>>>>> > >>>>>>>>> yes, I think this makes sense, even if it is a property that one can't > >>>>>>>>> tell for sure what it does before hand. > >>>>>>>>> > >>>>>>>>> Using a pair of properties, preference and active, to ask for something > >>>>>>>>> and then check what actually worked is good for reducing the > >>>>>>>>> combinatorial explosion caused by needing to "atomic TEST_ONLY commit" > >>>>>>>>> test different KMS configurations. Userspace has a better chance of > >>>>>>>>> finding a configuration that is possible. > >>>>>>>>> > >>>>>>>>> OTOH, this has the problem than in UI one cannot tell the user in > >>>>>>>>> advance which options are truly possible. Given that KMS properties are > >>>>>>>>> rarely completely independent, and in this case known to depend on > >>>>>>>>> several other KMS properties, I think it is good enough to know after > >>>>>>>>> the fact. > >>>>>>>>> > >>>>>>>>> If a driver does not use what userspace prefers, there is no way to > >>>>>>>>> understand why, or what else to change to make it happen. That problem > >>>>>>>>> exists anyway, because TEST_ONLY commits do not give useful feedback > >>>>>>>>> but only a yes/no. > >>>>>>>> By submitting incremental atomic reqs with TEST_ONLY (i.e. only changing one > >>>>>>>> property at a time), user-space can discover which property makes the atomic > >>>>>>>> commit fail. > >>>>>>> That works if the properties are independent of each other. Color > >>>>>>> range, color format, bpc and more may all be interconnected, > >>>>>>> allowing only certain combinations to work. > >>>>>>> > >>>>>>> If all these properties have "auto" setting too, then it would be > >>>>>>> possible to probe each property individually, but that still does not > >>>>>>> tell which combinations are valid. > >>>>>>> > >>>>>>> If you probe towards a certain configuration by setting the properties > >>>>>>> one by one, then depending on the order you pick the properties, you > >>>>>>> may come to a different conclusion on which property breaks the > >>>>>>> configuration. > >>>>>> My mind crossed another point that must be considered: When plugin in > >>>>>> a Monitor a list of possible Resolutions+Framerate combinations is > >>>>>> created for xrandr and other userspace (I guess by atomic checks? but > >>>>>> I don't know). > >>>>> Hi, > >>>>> > >>>>> I would not think so, but I hope to be corrected if I'm wrong. > >>>>> > >>>>> My belief is that the driver collects a list of modes from EDID, some > >>>>> standard modes, and maybe some other hardcoded modes, and then > >>>>> validates each entry against all the known limitations like vertical > >>>>> and horizontal frequency limits, discarding modes that do not fit. > >>>>> > >>>>> Not all limitations are known during that phase, which is why KMS > >>>>> property "link-status" exists. When userspace actually programs a mode > >>>>> (not a TEST_ONLY commit), the link training may fail. The kernel prunes > >>>>> the mode from the list and sets the link status property to signal > >>>>> failure, and sends a hotplug uevent. Userspace needs to re-check the > >>>>> mode list and try again. > >>>>> > >>>>> That is a generic escape hatch for when TEST_ONLY commit succeeds, but > >>>>> in reality the hardware cannot do it, you just cannot know until you > >>>>> actually try for real. It causes end user visible flicker if it happens > >>>>> on an already running connector, but since it usually happens when > >>>>> turning a connector on to begin with, there is no flicker to be seen, > >>>>> just a small delay in finding a mode that works. > >>>>> > >>>>>> During this drm > >>>>>> properties are already considered, which is no problem atm because as > >>>>>> far as i can tell there is currently no drm property that would make > >>>>>> a certain Resolutions+Framerate combination unreachable that would be > >>>>>> possible with everything on default. > >>>>> I would not expect KMS properties to be considered at all. It would > >>>>> reject modes that are actually possible if the some KMS properties were > >>>>> changed. So at least going forward, current KMS property values cannot > >>>>> factor in. > >>>> At least the debugfs variable "force_yuv420_output" did change the > >>>> available modes here: > >>>> https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L5165 > >>>> before my patch > >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68eb3ae3c63708f823aeeb63bb15197c727bd9bf > >>> Hi, > >>> > >>> debugfs is not proper UAPI, so we can just ignore it. Display servers > >>> cannot be expected to poke in debugfs. Debugfs is not even supposed to > >>> exist in production systems, but I'm sure people use it for hacking > >>> stuff. But that's all it is for: developer testing and hacking. > >> e.g. Ubuntu has it active by default, but only read (and writable) by root. > > Hi, > > > > that's normal, yes. Root can do damage anyway, and it's useful for > > debugging. KMS clients OTOH often do not run as root. > > > >>> > >>>> Forcing a color format via a DRM property in this function would > >>>> reintroduce the problem. > >>> The property would need to be defined differently because its presence > >>> could otherwise break existing userspace. Well, I suppose it could > >>> break existing userspace no matter what, so we would need the generic > >>> "reset to sane defaults" mechanism first IMO. > >>> > >>> DRM has client caps for exposing video modes that old userspace might > >>> not expect, to avoid breaking old userspace. Care needs to be taken > >>> with all new UAPI, because if a kernel upgrade makes something wrong, > >>> it's the kernel's fault no matter what userspace is doing, in principle. > >> Can you give me a link describing how I define this caps? > > I don't have any, but you can find all the existing ones by grepping > > for DRM_CLIENT_CAP_. > > > > I'm not saying that we need it, but mentioning them as a possible > > workaround if userspace breakage seems imminent or is proven. > > > >>> Debugfs has no problem breaking userspace AFAIU, since it's not proper > >>> UAPI. > >>> > >>>> And I think i915 driver works similar in this regard. > >>>> > >>>>> > >>>>>> However for example forcing YCbCr420 encoding would limit the > >>>>>> available resolutions (my screen for example only supports YCbCr420 > >>>>>> on 4k@60 and @50Hz and on no other resolution or frequency (native is > >>>>>> 2560x1440@144Hz). > >>>>>> > >>>>>> So would a "force color format" that does not get resetted on > >>>>>> repluging/reenabling a monitor break the output, for example, of an > >>>>>> not updated xrandr, unaware of this new property? > >>>>> Yes, not because the mode list would be missing the mode, but because > >>>>> actually setting the mode would fail. > >>>> Well, like described above, I think the mode would actually be missing, > >>>> which is also an unexpected behavior from a user perspective. > >>> I think that is not how the property should work. > >>> > >>> If KMS properties would affect the list of modes, then userspace would > >>> need to set the properties for real (TEST_ONLY cannot change anything) > >>> and re-fetch the mode lists (maybe there is a hotplug event, maybe > >>> not). That workflow just doesn't work. > >> The properties are set before the list is created in the first place. > >> Because, in my example, the properties get set before the monitor is > >> plugged in and the list can only be created as soon as the monitor is > >> plugged in. > > That's just an accident, it's not what I mean. > > > > What I mean is, we cannot have the KMS properties affect the list of > > modes, because then userspace that want to use specific values on those > > properties would have to program those properties first, and then get > > the list of modes. KMS UAPI does not work that way AFAIK. > > > > If the initial mode list is created on hotplug like you say, then the > > initial list could already be missing some modes that would be valid if > > some KMS properties had different values. > > Depends if the mode list is created by TEST_ONLY: Hi, I'm pretty sure it's not created by any kind of atomic test probing, exactly because some properties might affect the result. Also because of legacy: the mode lists predate atomic by far. It just doesn't make sense to prune the mode list based on current arbitrary property values. The function drm_helper_probe_single_connector_modes() looks relevant to me. It has a big comment that seems to point towards more things to look at. Thanks, pq > - The force properties should return false on TEST_ONLY > > - The force properties should not prevent the mode from showing up in the list > > If the list is created by TEST_ONLY both things can't be fulfilled at the same time obviously. > > I hope some can give more insights or has an idea how the properties could work best. > > > > >>> A very interesting question is when should link-status failure not drop > >>> the current mode from the mode list, if other KMS properties affect the > >>> bandwidth etc. requirements of the mode. That mechanism is engineered > >>> for old userspace that doesn't actually handle link-status but does > >>> handle hotplug, so the hotplug triggers re-fetching the mode list and > >>> userspace maybe trying again with a better luck since the offending > >>> mode is gone. How to keep that working when introducing KMS properties > >>> forcing the cable format, I don't know. > >>> > >>> As long as the other affecting KMS properties are all "auto", the > >>> driver will probably exhaust all possibilities to make the mode work > >>> before signalling link-status failure and pruning the mode. > >>> Theoretically, as I have no idea what drivers actually do. > >> Isn't that exactly how the "preferred color format" property works in > >> my patchset now? > > There was an argument that "preferred" with no guarantees is not > > useful enough. So I'm considering the force property instead. > > The problem is, "auto" is not the only possible value. > > > > When the value is not "auto", should link failure drop the mode or not? > > Userspace might change the value back to "auto" next time. If you > > dropped the mode, it would be gone. If you didn't drop the mode, > > userspace might be stuck picking the same non-working mode again and > > again if it doesn't know about the force mode property. > > > > You could argue that changing the value back to "auto" needs to reset > > the mode list, but that only gets us back to the "need to set > > properties before getting mode list". > > > > Maybe there needs to be an assumption that if "force color format" is > > not "auto", then link failure does not drop modes and userspace knows > > to handle this. Messy. > > > > I'm afraid I just don't know to give any clear answer. It's also > > possible that, as I'm not a kernel dev, I have some false assumptions > > here. > > > > > > Thanks, > > pq > > _______________________________________________ > > amd-gfx mailing list > > amd-gfx@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Attachment:
pgpnOjlDbfYQC.pgp
Description: OpenPGP digital signature
