On Thu, 30 Apr 2020 15:53:23 +0200 Daniel Vetter <daniel@xxxxxxxx> wrote: > On Wed, Apr 29, 2020 at 01:07:54PM +0300, Pekka Paalanen wrote: > > On Tue, 28 Apr 2020 16:51:57 +0200 > > Daniel Vetter <daniel@xxxxxxxx> wrote: > > > > > On Fri, Apr 24, 2020 at 11:32:16AM +0300, Pekka Paalanen wrote: > > > > On Thu, 23 Apr 2020 17:01:49 +0200 > > > > Daniel Vetter <daniel@xxxxxxxx> wrote: > > > > > > > > > On Tue, Apr 21, 2020 at 4:33 PM Pekka Paalanen <ppaalanen@xxxxxxxxx> wrote: > > > > > > > > > > > > On Tue, 21 Apr 2020 14:15:52 +0200 > > > > > > Daniel Vetter <daniel@xxxxxxxx> wrote: > > > > > > > > > > > > > > ... > > > > Hi, > > > > please skip to the TL;DR at the bottom, the rest is just how I ended up > > there. > > > > > > > > > > > > > Note that the kernel isn't entire consistent on this. I've looked a bit > > > > > > > more closely at stuff. Ignoring content protection I've found following > > > > > > > approaches things: > > > > > > > > > > > > > > - self refresh helpers, which are entirely transparent. Therefore we do a > > > > > > > hack to set allow_modeset when the self-refresh helpers need to do a > > > > > > > modeset, to avoid total surprise for userspace. I think this is only ok > > > > > > > for these kind of behind-the-scenes helpers like self-refresh. > > > > > > > > > > > > > > - link-status is always reset to "good" when you include any connector, > > > > > > > which might force a modeset. Even when allow_modeset isn't set by > > > > > > > userspace. Maybe we should fix that, but we've discussed forever how to > > > > > > > make sure a bad link isn't ever stuck at "bad" for old userspace, so > > > > > > > we've gone with this. But maybe limiting to only allow_modeset cases > > > > > > > would also work. > > > > > > > > > > > > Wait, what do you mean "include any connector"? > > > > > > > > > > > > What exactly could cause a modeset instead of failure when > > > > > > ALLOW_MODESET is not set? > > > > > > > > > > If you do an atomic commit with the connector included that has the > > > > > bad link status, then it'll reset it to Good to try to get a full > > > > > modeset to restore stuff. If you don't also have ALLOW_MODESET set, > > > > > it'll fail and userspace might be sad and not understand what's going > > > > > on. We can easily fix this by only forcing the link training to be > > > > > fixed if userspace has set ALLOW_MODESET. > > > > > > > > Hi, > > > > > > > > oh, that's ok, the fail case is there like it should. > > > > > > > > It sounded like even if userspace does not set ALLOW_MODESET, the > > > > kernel would still do a modeset in come cases. I'm happy to have > > > > misunderstood. > > > > > > Well currently that can go wrong, if you include a connector and a > > > link-status is bad. But could be fixed fairly easily. > > > > What do you mean by "include a connector"? Which property on which > > object? > > > > Weston always submits more or less full KMS state (known properties on > > intended-active outputs) on all updates, on simple pageflips too, so I > > am worried that the connector is "included", leading to automatic > > papering over link-status failures, and Weston doesn't handle > > link-status yet. > > Include a connector = you have a property for a connector included in your > atomic update. Sounds like you do that, so if you have a real world > link-status failure, then things go a bit boom already. Are you talking about setting a connector's property, like "CRTC_ID", "Content Protection", "HDCP Content Type"? Weston sets all of those on every update if they exist. Or is it about any property on any connector, not just a specific property on the specific connector, or any property on the specific connector? Also CRTC properties "MODE_ID" and "ACTIVE" are set on every update, modeset or not. Weston just trusts that no-op changes in state do not require a modeset. Is it not the opposite? If a link fails, then Weston will produce a glitch when the kernel automatically re-trains the link, and then everything continues happily? > I guess we need a kernel patch to make sure link-status only gets fixed > when userspace is ok with a modeset. That would be nice, but would it not also mean the above Weston case ends up with a permanent black screen instead of a temporary glitch? Or is there a hotplug uevent at play here too? > > Weston does this, because it is the easy thing to do and debug. You > > don't have to track back thousands of frames to figure out what the > > mode or the connectors are, when something doesn't work like it should. > > Or to figure out why some state wasn't emitted when it was supposed to. > > ... > One idea I proposed on irc is that logind would capture the boot-up state, > and you'd do a loginctl reset-screen cmd to reset it if something broke > somewhere. logind already has the code for forced vt switching, that feels > like a reasonable extension of that. Needing to run a command manually to restore the screen instead of "simply" switching to the graphical login manager to re-gain user control seems a bit difficult. > Then you could pick between "smooth transitions" (probably best for users > only using 1 compositor) and "whack it back to boot-up state when > switching to a new compositor" (maybe best default if you run multiple > different compositors, logind could even do that automatically). I think even better would be for logind to serialize and hand out the default state it saved, instead of applying it. Then the compositor who takes over can integrate the default state into its own state and inherited state to apply it all in one commit. Doing so, the compositor could even do a TEST_ONLY commit without ALLOW_MODESET to see if a smooth transition is possible. If not, it can skip some smooth transition animation for instance. > > > The upshot of doing the exact same as a logind request along the lines of > > > "pls reset display to sane state, thx" is that logind can be changed much > > > easier than kernel uapi in case we get this wrong. Which we will. > > > > Sorry, I don't see the difference if the goal is to have fixed > > defaults. The only difference with logind is that there is no angry > > Linus. > > Oh it's the exact same problem, but more focused. E.g. we don't have to > worry about embedded stuff, because that's going to run one compositor > only. So entire class of regression reports goes away. > > If we put the defaults into the kernel, then more people will use it (it's > there), so the wiggle room is substantially reduced once we do have > conflict demands. > > logind with multiple different compositors is going to be used on > desktops/latops only, so some chances that the definition of what default > should be all match up. > > Also no regression pretty much applies across the board, e.g. we don't > have modifiers enabled in many compositors by default yet because it > breaks stuff. I'm not sure I understand, but ok, I'll stop pushing for it. > > TL;DR: > > > > If blind save & restore (but restricted only to cases where KMS state > > may have been lost!) works, it will ensure that a running userspace > > program has what it was started with, but it does not guarantee that > > the state is the same between restarts of the program. > > > > It seems that the default state is always undefined, and cannot be > > defined because whatever one defines might not always result in a > > picture on screen. Bugs in defined default state cannot be fixed, by > > definition as the default state is immutable once defined. > > > > Ensuring consistent state between restarts is the responsibility of > > userspace and each KMS program individually. They might attempt to > > blindly save KMS state in a file to be re-used on the next start. > > I disagree with "each KMS program individually". We already have defined > protocols for drm master switching: Either the cooperative vt-switch > dance, or the unpriviledge switch logind allows. We also have ad-hoc > definitions of smooth transitions, it's "one primary plane, untiled". Some > people broke that, and now we have getfb2 so you can smoothly transition > even with tiled buffers. Except not everyone supports that. Switching is not the point with saving unrecognized KMS state into a compositor-specific file. The point of the file is to re-create the exact same KMS state after the compositor has been restarted, regardless of what KMS program might have ran first. Use case: when you have completed monitor color profiling, the profile may become invalid as soon as anything affecting colors changes, e.g. link bit depth. With all the new properties for color spaces, HDR etc. the likelihood of an unrecognized property affecting colors is quite high. Either the property must be reset to the value it had during profiling, or the profile needs to be rejected. This is more about rebooting the machine than it is about switching between display servers. If the compositor has such a file saved, it might as well use it also for sanitation when switching from another display server. This still does not handle the case where a kernel upgrade introduces a new property, the graphical login manager sets the property altering colors, and the session compositor you profiled does not recognize the new property and also does not have it saved in the file because it didn't exist last time. But maybe that's a corner-case enough, and people who care will notice the profile is invalid when physically measuring its validity before starting to work. But this could be seen as a kernel regression too: you only had to upgrade the kernel. Logind saving state before fbcon takes over could remedy this, if the session compositor integrated the logind saved state on every start. But again we would rely on kernel not changing the hardware state between kernel versions before fbcon takes over. Thanks, pq
Attachment:
pgpsBB4wBZTvc.pgp
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
