[PATCH weston v5 09/36] cms-colord: find a good head

[Pekka Paalanen]

On Fri, 2 Feb 2018 13:40:49 -0600
Derek Foreman <derekf at osg.samsung.com> wrote:

> On 2018-02-02 03:45 AM, Pekka Paalanen wrote:
> > On Thu, 1 Feb 2018 22:10:33 +0000
> > Daniel Stone <daniel at fooishbar.org> wrote:
> >   
> >> Hi,
> >>
> >> On 1 February 2018 at 22:00, Derek Foreman <derekf at osg.samsung.com> wrote:  
> >>> On 2017-12-14 05:40 AM, Pekka Paalanen wrote:  
> >>>> The 'head' member of 'struct weston_output' is going to go unused and
> >>>> then disappear, so stop using it and find a head from the proper list.
> >>>>
> >>>> However, this leaves a problem in cms-colord: if you have multiple
> >>>> monitors driver with the same CRTC, what do you say to the color
> >>>> management system? The monitors could be different, but all the color
> >>>> LUTs etc. are in the CRTC and are shared, as is the framebuffer.
> >>>>
> >>>> Do the simple hack here and just use whatever head happens to be the
> >>>> first in the list.  
> >>>
> >>> I am a complete non-expert in this area, so if I'm making no sense feel free
> >>> to tell me to shut up...
> >>>
> >>> I think if someone's going through the effort to properly setup color
> >>> management, then we can't use cloned heads off a single CRTC?
> >>>
> >>> We should probably disallow a CRTCs to drive multiple heads if two or more
> >>> of those heads have differing color profiles?
> >>>
> >>> If I went to the trouble of calibrating two displays, I would probably be
> >>> extremely surprised to see them looking different with clone mode enabled.  
> >>
> >> Yeah, what Derek said. I think non-homogeneous colour properties when
> >> colour management is enabled, should be cause to unclone. That being
> >> said, it's a niche enough usecase that I'd be comfortable landing it
> >> with documentation that it was a known shortcoming. I'd be super
> >> comfortable landing it if we also had an output configuration tweak we
> >> could use to force non-clone mode, or really even just a simple
> >> out-of-tree patch (or environment variable, a la atomic?) that allows
> >> people to work around it easily enough by disabling clones.  
> > 
> > Hi,
> > 
> > I agree with Derek's suggestion that we should check color properties
> > and fail the shared-CRTC cloning, and with Daniel that we could just
> > document this. I'm not sure what the "output configuration tweak"
> > refers to. Do you mean an option that would just disable shared-CRTC
> > cloning?  
> 
> I can get behind just documenting it.
> 
> > We have Weston that handles all option parsing, and Weston also does
> > the decision to use or not use shared-CRTC cloning by how it attaches
> > heads to outputs. Libweston will not try shared-CRTC cloning unless
> > explicitly told to, and it will not transparently fall back to
> > something else. After the complete clone mode series, which will not
> > include independent-CRTC cloning support, the option to disable
> > shared-CRTC cloning would be equivalent to a weston.ini that does not
> > configure cloning at all.  
> 
> QED. :)
> 
> For myself, I've been struggling with the ramifications of shared-CRTC 
> vs independent-CRTC cloning, and didn't realize until you pointed out to 
> me out-of-band that independent-CRTC cloning isn't just fallback 
> operation (and in fact won't work at all without additional work).
> 
> > However, the weston.ini syntax as is does not differentiate between
> > shared-CRTC and independent-CRTC cloning. My intention is that Weston
> > (not libweston) attemps shared-CRTC cloning and falls back to
> > independent-CRTC cloning.
> > 
> > For the cms-colord plugin to be able to veto shared-CRTC cloning we
> > would need libweston infrastructure work to either offer plugins a veto
> > API or make libweston core aware of color profiles. An alternative
> > would be to have Weston query the color management plugin directly if
> > heads are compatible for shared-CRTC mode.  
> 
> Yes, it had occurred to me that this could be particularly painful. :(
> 
> The plug-in veto idea, seems like it would be a fairly heavy amount of 
> overkill if cms-colord would forever be the only plug-in to actually use it.
> 
> If I'm understanding all the pieces in play at this point...  We'd 
> really just be giving cms-colord the ability to stop weston from 
> launching at all, since independent-CRTC cloning doesn't actually work, 
> and is (quite reasonably!) out of scope for this series and its follow up?

Pretty much, yes... FWIW, the Weston configuration logic is here:
https://gitlab.collabora.com/pq/weston/commit/99d5cf59685c3f98eaf4d31445d29317ebd1087a

The logic starts with drm_heads_changed() which will be called on start
after all KMS resources have been found.

It quits if the output configuration at launch with currently connected
heads does not work out, and it bails out on where it should fall back
to independent-CRTC cloning.

> > One more idea coming to mind is that the "same-as" directive in
> > weston.ini could refer to either exclusively shared-CRTC clone mode or
> > shared-CRTC with fallback to independent-CRTC mode, and we could use a
> > hypothetical future output layout configuration directives to force
> > independent-CRTC mode by defining two outputs to show the same area of
> > the desktop.
> > 
> > Given that, what would you recommend for now?  
> 
> Can we generate a log message when cms-colord configuration results in 
> suboptimal display for cloned heads (possibly including some strings to 
> identify the display/displays that look wrong)?

I suppose... I'd have to look deeper into the CMS API to find out what
to compare. To me this would be a good solution if not even a long-term
solution.

> Or even a warning when cms-colord and clones are both enabled at the 
> same time?

I think the CMS plugins are loaded via a generic plugin loading
mechanism, neither Weston nor libweston actually recognizes them
currently.

> At this point, I think whoever the first person to need clone+cms-colord 
> simultaneously is going to have to step up and do some heavy lifting.
> 
> As long as we're not silently doing something surprising with color 
> profiles, I think it should be ok?  Since clone mode is all new, this 
> isn't going to cause a regression for anyone.

Yeah.

So, would we be good with initially just documenting in weston-drm man
page for "same-as" directive the issue with CMS? Would you demand the
warning message in the log as well on runtime when there actually is a
problem?

Realistically, whatever you want me to do on this matter has to be a
blocker on the cloning feature. Otherwise I can't promise to do it. :-)


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/wayland-devel/attachments/20180205/38221003/attachment.sig>