Operating KMS UAPI (Re: RFC: Drm-connector properties managed by another driver / privacy screen support)

Mon May 4 09:49:13 UTC 2020

On Thu, 30 Apr 2020 15:53:23 +0200
Daniel Vetter <daniel at ffwll.ch> 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 at ffwll.ch> 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 at ffwll.ch> wrote:
> > > >     
> > > > > On Tue, Apr 21, 2020 at 4:33 PM Pekka Paalanen <ppaalanen at gmail.com> wrote:    
> > > > > >
> > > > > > On Tue, 21 Apr 2020 14:15:52 +0200
> > > > > > Daniel Vetter <daniel at ffwll.ch> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200504/780d08cb/attachment.sig>