Re: [PATCH 2/7] drm/uAPI: Add "active color format" drm property as feedback for userspace

[Andri Yngvason]

mið., 10. jan. 2024 kl. 13:26 skrifaði Daniel Stone <daniel@xxxxxxxxxxxxx>:
> >
> > This thing here works entirely differently, and I think we need somewhat
> > new semantics for this:
> >
> > - I agree it should be read-only for userspace, so immutable sounds right.
> >
> > - But I also agree with Daniel Stone that this should be tied more
> >   directly to the modeset state.
> >
> > So I think the better approach would be to put the output type into
> > drm_connector_state, require that drivers compute it in their
> > ->atomic_check code (which in the future would allow us to report it out
> > for TEST_ONLY commits too), and so guarantee that the value is updated
> > right after the kms ioctl returns (and not somewhen later for non-blocking
> > commits).
>
> That's exactly the point. Whether userspace gets an explicit
> notification or it has to 'know' when to read isn't much of an issue -
> just as long as it's well defined. I think the suggestion of 'do it in
> atomic_check and then it's guaranteed to be readable when the commit
> completes' is a good one.
>
> I do still have some reservations - for instance, why do we have the
> fallback to auto when userspace has explicitly requested a certain
> type? - but they may have been covered previously.
>

We discussed this further on IRC and this is summary of that discussion:

Sima proposed a new type of property that can be used to git feedback to
userspace after atomic ioctl. The user supplies a list of output properties
that they want to query and the kernel fills it in before returning from the
ioctl. This would help to get some information about why things failed
during TEST_ONLY.

Emersion raised the point that you might not know how much memory is needed
beforehand for the returned properties, to which sima replied: blob
property. There was some discussion about how that makes it possible to leak
kernel memory, especially if userspace does not know about a new property
blob. Emersion pointed out that userspace should only request properties
that it understands and pq agreed.

Emersion asked how the user should inform the kernel that it's done with the
blob, to which sima replied: DRM_IOCTL_MODE_DESTROYPROPBLOB. Sima also
mentioned using some sort of weak reference garbage collection scheme for
properties and there was some further discussion, but I'm not sure there was
any conclusion.

I asked if it made sense to add color format capabilities to the mode info
struct, but the conclusion was that it wouldn't really be useful because we
need TEST_ONLY anyway to see if the color format setting is compatible with
other settings.

I asked again if we should drop the "active color format" property as it
seems to be more trouble than it's worth for now. pq mentioned that there
are 2 separate use cases (in his words):
- People playing with setting UI would like to know what "auto" would result
  in, but that's just nice to have
- The other use case is the flicker-free boot into known configuration He
  went on to point out that the main problem with "auto" is that any modeset
  could make the driver decide differently. This means that we cannot fully
  rely on the previously set property.

However, leaving out "active color property" did not put us in a worse
situation than before, so the conclusion was that we should leave it out for
now. For GUI selectors, the current TEST_ONLY should be good enough, and all
the fancy stuff discussed previously isn't needed for now.

To summarise the summary: this means that we will drop the "active
color format" property and rename the "preferred color format"
property to "force color format" or just "color format" and failing to
satisfy that constraint will fail the modeset rather than falling back
to the "auto" behaviour.

Cheers,
Andri