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

[Sebastian Wick]

On Thu, Jan 11, 2024 at 05:17:46PM +, Andri Yngvason wrote:
> mið., 10. jan. 2024 kl. 13:26 skrifaði Daniel Stone :
> > >
> > > 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.

That's a good idea.

Anything else won't work with the new color pipeline API anyways because
user space will be responsible for setting up the color pipeline API in
a way so that the monitor will receive the correctly encoded data.

> Cheers,
> Andri
> 

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 :
> >
> > 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

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

[Werner Sembach]

Hi,

Am 09.01.24 um 19:10 schrieb Andri Yngvason:

From: Werner Sembach 

Add a new general drm property "active color format" which can be used by
graphic drivers to report the used color format back to userspace.

There was no way to check which color format got actually used on a given
monitor. To surely predict this, one must know the exact capabilities of
the monitor, the GPU, and the connection used and what the default
behaviour of the used driver is (e.g. amdgpu prefers YCbCr 4:4:4 while i915
prefers RGB). This property helps eliminating the guessing on this point.

In the future, automatic color calibration for screens might also depend on
this information being available.

Signed-off-by: Werner Sembach 
Signed-off-by: Andri Yngvason 
Tested-by: Andri Yngvason 


One suggestion from back then was, instead picking out singular properties like 
"active color format", to just expose whatever the last HDMI or DP metadata 
block(s)/frame(s) that was sent over the display wire was to userspace and 
accompanying it with a parsing script.


Question: Does the driver really actually know what the GPU is ultimatively 
sending out the wire, or is that decided by a final firmware blob we have no 
info about?


Greetings

Werner


---
  drivers/gpu/drm/drm_connector.c | 63 +
  include/drm/drm_connector.h | 10 ++
  2 files changed, 73 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index c3725086f4132..30d62e505d188 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1061,6 +1061,14 @@ static const struct drm_prop_enum_list 
drm_dp_subconnector_enum_list[] = {
{ DRM_MODE_SUBCONNECTOR_Native,  "Native"}, /* DP */
  };
  
+static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = {

+   { 0, "not applicable" },
+   { DRM_COLOR_FORMAT_RGB444, "rgb" },
+   { DRM_COLOR_FORMAT_YCBCR444, "ycbcr444" },
+   { DRM_COLOR_FORMAT_YCBCR422, "ycbcr422" },
+   { DRM_COLOR_FORMAT_YCBCR420, "ycbcr420" },
+};
+
  DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
 drm_dp_subconnector_enum_list)
  
@@ -1390,6 +1398,15 @@ static const u32 dp_colorspaces =

   *drm_connector_attach_max_bpc_property() to create and attach the
   *property to the connector during initialization.
   *
+ * active color format:
+ * This read-only property tells userspace the color format actually used
+ * by the hardware display engine "on the cable" on a connector. The chosen
+ * value depends on hardware capabilities, both display engine and
+ * connected monitor. Drivers shall use
+ * drm_connector_attach_active_color_format_property() to install this
+ * property. Possible values are "not applicable", "rgb", "ycbcr444",
+ * "ycbcr422", and "ycbcr420".
+ *
   * Connectors also have one standardized atomic property:
   *
   * CRTC_ID:
@@ -2451,6 +2468,52 @@ int drm_connector_attach_max_bpc_property(struct 
drm_connector *connector,
  }
  EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
  
+/**

+ * drm_connector_attach_active_color_format_property - attach "active color 
format" property
+ * @connector: connector to attach active color format property on.
+ *
+ * This is used to check the applied color format on a connector.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_active_color_format_property(struct drm_connector 
*connector)
+{
+   struct drm_device *dev = connector->dev;
+   struct drm_property *prop;
+
+   if (!connector->active_color_format_property) {
+   prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE, 
"active color format",
+   
drm_active_color_format_enum_list,
+   
ARRAY_SIZE(drm_active_color_format_enum_list));
+   if (!prop)
+   return -ENOMEM;
+
+   connector->active_color_format_property = prop;
+   }
+
+   drm_object_attach_property(>base, prop, 0);
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_active_color_format_property);
+
+/**
+ * drm_connector_set_active_color_format_property - sets the active color 
format property for a
+ * connector
+ * @connector: drm connector
+ * @active_color_format: color format for the connector currently active "on the 
cable"
+ *
+ * Should be used by atomic drivers to update the active color format over a 
connector.
+ */
+void drm_connector_set_active_color_format_property(struct drm_connector 
*connector,
+   u32 active_color_format)
+{
+   drm_object_property_set_value(>base, 
connector->active_color_format_property,
+ active_color_format);
+}
+EXPORT_SYMBOL(drm_connector_set_active_color_format_property);
+
  /**
   * 

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

[Daniel Stone]

Hi,

On Wed, 10 Jan 2024 at 10:44, Daniel Vetter  wrote:
> On Tue, Jan 09, 2024 at 11:12:11PM +, Andri Yngvason wrote:
> > ţri., 9. jan. 2024 kl. 22:32 skrifađi Daniel Stone :
> > > How does userspace determine what's happened without polling? Will it
> > > only change after an `ALLOW_MODESET` commit, and be guaranteed to be
> > > updated after the commit has completed and the event being sent?
> > > Should it send a HOTPLUG event? Other?
> > >
> >
> > Userspace does not determine what's happened without polling. The purpose
> > of this property is not for programmatic verification that the preferred
> > property was applied. It is my understanding that it's mostly intended for
> > debugging purposes. It should only change as a consequence of modesetting,
> > although I didn't actually look into what happens if you set the "preferred
> > color format" outside of a modeset.
>
> This feels a bit irky to me, since we don't have any synchronization and
> it kinda breaks how userspace gets to know about stuff.
>
> For context the current immutable properties are all stuff that's derived
> from the sink (like edid, or things like that). Userspace is guaranteed to
> get a hotplug event (minus driver bugs as usual) if any of these change,
> and we've added infrastructure so that the hotplug event even contains the
> specific property so that userspace can avoid re-read (which can cause
> some costly re-probing) them all.

Right.

> [...]
>
> 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.

Cheers,
Daniel

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

[Daniel Vetter]

On Tue, Jan 09, 2024 at 11:12:11PM +, Andri Yngvason wrote:
> Hi Daniel,
> 
> þri., 9. jan. 2024 kl. 22:32 skrifaði Daniel Stone :
> 
> > On Tue, 9 Jan 2024 at 18:12, Andri Yngvason  wrote:
> > > + * active color format:
> > > + * This read-only property tells userspace the color format
> > actually used
> > > + * by the hardware display engine "on the cable" on a connector.
> > The chosen
> > > + * value depends on hardware capabilities, both display engine and
> > > + * connected monitor. Drivers shall use
> > > + * drm_connector_attach_active_color_format_property() to install
> > this
> > > + * property. Possible values are "not applicable", "rgb",
> > "ycbcr444",
> > > + * "ycbcr422", and "ycbcr420".
> >
> > How does userspace determine what's happened without polling? Will it
> > only change after an `ALLOW_MODESET` commit, and be guaranteed to be
> > updated after the commit has completed and the event being sent?
> > Should it send a HOTPLUG event? Other?
> >
> 
> Userspace does not determine what's happened without polling. The purpose
> of this property is not for programmatic verification that the preferred
> property was applied. It is my understanding that it's mostly intended for
> debugging purposes. It should only change as a consequence of modesetting,
> although I didn't actually look into what happens if you set the "preferred
> color format" outside of a modeset.

This feels a bit irky to me, since we don't have any synchronization and
it kinda breaks how userspace gets to know about stuff.

For context the current immutable properties are all stuff that's derived
from the sink (like edid, or things like that). Userspace is guaranteed to
get a hotplug event (minus driver bugs as usual) if any of these change,
and we've added infrastructure so that the hotplug event even contains the
specific property so that userspace can avoid re-read (which can cause
some costly re-probing) them all.

As an example you can look at drm_connector_set_link_status_property,
which drivers follow by a call to drm_kms_helper_connector_hotplug_event
to make sure userspace knows about what's up. Could be optimized I think.

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).

You probably need a bit of work to be able to handle immutable properties
with the atomic state infrastructure, but I think otherwise this should
fit all rather neatly.

Cheers, Sima
> 
> The way I've implemented things in sway, calling the
> "preferred_signal_format" command triggers a modeset with the "preferred
> color format" set and calling "get_outputs", immediately queries the
> "actual color format" and displays it.
> 
> Regards,
> Andri

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Andri Yngvason]

Hi Daniel,

þri., 9. jan. 2024 kl. 22:32 skrifaði Daniel Stone :

> On Tue, 9 Jan 2024 at 18:12, Andri Yngvason  wrote:
> > + * active color format:
> > + * This read-only property tells userspace the color format
> actually used
> > + * by the hardware display engine "on the cable" on a connector.
> The chosen
> > + * value depends on hardware capabilities, both display engine and
> > + * connected monitor. Drivers shall use
> > + * drm_connector_attach_active_color_format_property() to install
> this
> > + * property. Possible values are "not applicable", "rgb",
> "ycbcr444",
> > + * "ycbcr422", and "ycbcr420".
>
> How does userspace determine what's happened without polling? Will it
> only change after an `ALLOW_MODESET` commit, and be guaranteed to be
> updated after the commit has completed and the event being sent?
> Should it send a HOTPLUG event? Other?
>

Userspace does not determine what's happened without polling. The purpose
of this property is not for programmatic verification that the preferred
property was applied. It is my understanding that it's mostly intended for
debugging purposes. It should only change as a consequence of modesetting,
although I didn't actually look into what happens if you set the "preferred
color format" outside of a modeset.

The way I've implemented things in sway, calling the
"preferred_signal_format" command triggers a modeset with the "preferred
color format" set and calling "get_outputs", immediately queries the
"actual color format" and displays it.

Regards,
Andri

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

[Andri Yngvason]

Hi Daniel,

Please excuse my misconfigured email client. HTML was accidentally
enabled in my previous messages, so I'll re-send it for the benefit of
mailing lists.

þri., 9. jan. 2024 kl. 22:32 skrifaði Daniel Stone :
>
> On Tue, 9 Jan 2024 at 18:12, Andri Yngvason  wrote:
> > + * active color format:
> > + * This read-only property tells userspace the color format actually 
> > used
> > + * by the hardware display engine "on the cable" on a connector. The 
> > chosen
> > + * value depends on hardware capabilities, both display engine and
> > + * connected monitor. Drivers shall use
> > + * drm_connector_attach_active_color_format_property() to install this
> > + * property. Possible values are "not applicable", "rgb", "ycbcr444",
> > + * "ycbcr422", and "ycbcr420".
>
> How does userspace determine what's happened without polling? Will it
> only change after an `ALLOW_MODESET` commit, and be guaranteed to be
> updated after the commit has completed and the event being sent?
> Should it send a HOTPLUG event? Other?

Userspace does not determine what's happened without polling. The
purpose of this property is not for programmatic verification that the
preferred property was applied. It is my understanding that it's
mostly intended for debugging purposes. It should only change as a
consequence of modesetting, although I didn't actually look into what
happens if you set the "preferred color format" outside of a modeset.

The way I've implemented things in sway, calling the
"preferred_signal_format" command triggers a modeset with the
"preferred color format" set and calling "get_outputs", immediately
queries the "actual color format" and displays it.

Regards,
Andri

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

[Daniel Stone]

Hi,

On Tue, 9 Jan 2024 at 18:12, Andri Yngvason  wrote:
> + * active color format:
> + * This read-only property tells userspace the color format actually used
> + * by the hardware display engine "on the cable" on a connector. The 
> chosen
> + * value depends on hardware capabilities, both display engine and
> + * connected monitor. Drivers shall use
> + * drm_connector_attach_active_color_format_property() to install this
> + * property. Possible values are "not applicable", "rgb", "ycbcr444",
> + * "ycbcr422", and "ycbcr420".

How does userspace determine what's happened without polling? Will it
only change after an `ALLOW_MODESET` commit, and be guaranteed to be
updated after the commit has completed and the event being sent?
Should it send a HOTPLUG event? Other?

Cheers,
Daniel