[v4,12/17] drm/uAPI: Add "preferred color format" drm property as setting for userspace

Commit Message

Werner Sembach June 18, 2021, 9:11 a.m. UTC

Add a new general drm property "preferred color format" which can be used
by userspace to tell the graphic drivers to which color format to use.

Possible options are:
    - auto (default/current behaviour)
    - rgb
    - ycbcr444
    - ycbcr422 (not supported by both amdgpu and i915)
    - ycbcr420

In theory the auto option should choose the best available option for the
current setup, but because of bad internal conversion some monitors look
better with rgb and some with ycbcr444.

Also, because of bad shielded connectors and/or cables, it might be
preferable to use the less bandwidth heavy ycbcr422 and ycbcr420 formats
for a signal that is less deceptible to interference.

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

Signed-off-by: Werner Sembach <[e-mail]>
---
 drivers/gpu/drm/drm_atomic_helper.c |  4 +++
 drivers/gpu/drm/drm_atomic_uapi.c   |  4 +++
 drivers/gpu/drm/drm_connector.c     | 48 ++++++++++++++++++++++++++++-
 include/drm/drm_connector.h         | 17 ++++++++++
 4 files changed, 72 insertions(+), 1 deletion(-)

Comments

Pekka Paalanen June 22, 2021, 7:15 a.m. UTC | #1

On Fri, 18 Jun 2021 11:11:11 +0200
Werner Sembach <[e-mail]> wrote:

> Add a new general drm property "preferred color format" which can be used
> by userspace to tell the graphic drivers to which color format to use.
> 
> Possible options are:
>     - auto (default/current behaviour)
>     - rgb
>     - ycbcr444
>     - ycbcr422 (not supported by both amdgpu and i915)
>     - ycbcr420
> 
> In theory the auto option should choose the best available option for the
> current setup, but because of bad internal conversion some monitors look
> better with rgb and some with ycbcr444.
> 
> Also, because of bad shielded connectors and/or cables, it might be
> preferable to use the less bandwidth heavy ycbcr422 and ycbcr420 formats
> for a signal that is less deceptible to interference.
> 
> In the future, automatic color calibration for screens might also depend on
> this option being available.
> 
> Signed-off-by: Werner Sembach <[e-mail]>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c |  4 +++
>  drivers/gpu/drm/drm_atomic_uapi.c   |  4 +++
>  drivers/gpu/drm/drm_connector.c     | 48 ++++++++++++++++++++++++++++-
>  include/drm/drm_connector.h         | 17 ++++++++++
>  4 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index bc3487964fb5..90d62f305257 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -687,6 +687,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  			if (old_connector_state->max_requested_bpc !=
>  			    new_connector_state->max_requested_bpc)
>  				new_crtc_state->connectors_changed = true;
> +
> +			if (old_connector_state->preferred_color_format !=
> +			    new_connector_state->preferred_color_format)
> +				new_crtc_state->connectors_changed = true;
>  		}
>  
>  		if (funcs->atomic_check)
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 438e9585b225..c536f5e22016 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -796,6 +796,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  						   fence_ptr);
>  	} else if (property == connector->max_bpc_property) {
>  		state->max_requested_bpc = val;
> +	} else if (property == connector->preferred_color_format_property) {
> +		state->preferred_color_format = val;
>  	} else if (connector->funcs->atomic_set_property) {
>  		return connector->funcs->atomic_set_property(connector,
>  				state, property, val);
> @@ -873,6 +875,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = 0;
>  	} else if (property == connector->max_bpc_property) {
>  		*val = state->max_requested_bpc;
> +	} else if (property == connector->preferred_color_format_property) {
> +		*val = state->preferred_color_format;
>  	} else if (connector->funcs->atomic_get_property) {
>  		return connector->funcs->atomic_get_property(connector,
>  				state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 818de58d972f..aea03dd02e33 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -889,6 +889,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_preferred_color_format_enum_list[] = {
> +	{ 0, "auto" },
> +	{ DRM_COLOR_FORMAT_RGB444, "rgb" },
> +	{ DRM_COLOR_FORMAT_YCRCB444, "ycbcr444" },
> +	{ DRM_COLOR_FORMAT_YCRCB422, "ycbcr422" },
> +	{ DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" },
> +};
> +
>  static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = {
>  	{ 0, "unknown" },
>  	{ DRM_COLOR_FORMAT_RGB444, "rgb" },
> @@ -1219,11 +1227,19 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>   *	Drivers shall use drm_connector_attach_active_bpc_property() to install
>   *	this property.
>   *
> + * preferred color format:
> + *	This property is used by userspace to change the used color format. When
> + *	used the driver will use the selected format if valid for the hardware,
> + *	sink, and current resolution and refresh rate combination. Drivers to
> + *	use the function drm_connector_attach_preferred_color_format_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
> + *	connected monitor, and the "preferred color format". Drivers shall use
>   *	drm_connector_attach_active_color_format_property() to install this
>   *	property.
>   *
> @@ -2233,6 +2249,36 @@ void drm_connector_set_active_bpc_property(struct drm_connector *connector, int
>  }
>  EXPORT_SYMBOL(drm_connector_set_active_bpc_property);
>  
> +/**
> + * drm_connector_attach_preferred_color_format_property - attach "preferred color format" property
> + * @connector: connector to attach active color format property on.
> + *
> + * This is used to add support for selecting a color format on a connector.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_preferred_color_format_property(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	if (!connector->preferred_color_format_property) {
> +		prop = drm_property_create_enum(dev, 0, "preferred color format",
> +						drm_preferred_color_format_enum_list,
> +						ARRAY_SIZE(drm_preferred_color_format_enum_list));
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->preferred_color_format_property = prop;
> +		drm_object_attach_property(&connector->base, prop, 0);
> +		connector->state->preferred_color_format = 0;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_preferred_color_format_property);
> +
>  /**
>   * drm_connector_attach_active_color_format_property - attach "active color format" property
>   * @connector: connector to attach active color format property on.
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 9fb7119b7a02..7b85407ba45c 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -799,6 +799,16 @@ struct drm_connector_state {
>  	 */
>  	u8 max_bpc;
>  
> +	/**
> +	 * preferred_color_format: Property set by userspace to tell the GPU
> +	 * driver which color format to use. It only gets applied if hardware,
> +	 * meaning both the computer and the monitor, and the driver support the
> +	 * given format at the current resolution and refresh rate. Userspace
> +	 * can check for (un-)successful application via the active_color_format
> +	 * property.
> +	 */
> +	u32 preferred_color_format;

Hi,

yes, I think this makes sense, even if it is a property that one can't
tell for sure what it does before hand.

Using a pair of properties, preference and active, to ask for something
and then check what actually worked is good for reducing the
combinatorial explosion caused by needing to "atomic TEST_ONLY commit"
test different KMS configurations. Userspace has a better chance of
finding a configuration that is possible.

OTOH, this has the problem than in UI one cannot tell the user in
advance which options are truly possible. Given that KMS properties are
rarely completely independent, and in this case known to depend on
several other KMS properties, I think it is good enough to know after
the fact.

If a driver does not use what userspace prefers, there is no way to
understand why, or what else to change to make it happen. That problem
exists anyway, because TEST_ONLY commits do not give useful feedback
but only a yes/no.

Acked-by: Pekka Paalanen <[e-mail]>


Thanks,
pq


> +
>  	/**
>  	 * @hdr_output_metadata:
>  	 * DRM blob property for HDR output metadata
> @@ -1404,6 +1414,12 @@ struct drm_connector {
>  	 */
>  	struct drm_property *active_bpc_property;
>  
> +	/**
> +	 * @preferred_color_format_property: Default connector property for the
> +	 * preferred color format to be driven out of the connector.
> +	 */
> +	struct drm_property *preferred_color_format_property;
> +
>  	/**
>  	 * @active_color_format_property: Default connector property for the
>  	 * active color format to be driven out of the connector.
> @@ -1740,6 +1756,7 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  					  int min, int max);
>  int drm_connector_attach_active_bpc_property(struct drm_connector *connector, int min, int max);
>  void drm_connector_set_active_bpc_property(struct drm_connector *connector, int active_bpc);
> +int drm_connector_attach_preferred_color_format_property(struct drm_connector *connector);
>  int drm_connector_attach_active_color_format_property(struct drm_connector *connector);
>  void drm_connector_set_active_color_format_property(struct drm_connector *connector,
>  						    u32 active_color_format);

Simon Ser June 29, 2021, 8:12 a.m. UTC | #2

On Tuesday, June 22nd, 2021 at 09:15, Pekka Paalanen <[e-mail]> wrote:

> yes, I think this makes sense, even if it is a property that one can't
> tell for sure what it does before hand.
>
> Using a pair of properties, preference and active, to ask for something
> and then check what actually worked is good for reducing the
> combinatorial explosion caused by needing to "atomic TEST_ONLY commit"
> test different KMS configurations. Userspace has a better chance of
> finding a configuration that is possible.
>
> OTOH, this has the problem than in UI one cannot tell the user in
> advance which options are truly possible. Given that KMS properties are
> rarely completely independent, and in this case known to depend on
> several other KMS properties, I think it is good enough to know after
> the fact.
>
> If a driver does not use what userspace prefers, there is no way to
> understand why, or what else to change to make it happen. That problem
> exists anyway, because TEST_ONLY commits do not give useful feedback
> but only a yes/no.

By submitting incremental atomic reqs with TEST_ONLY (i.e. only changing one
property at a time), user-space can discover which property makes the atomic
commit fail.

I'm not a fan of this "preference" property approach. The only way to find out
whether it's possible to change the color format is to perform a user-visible
change (with a regular atomic commit) and check whether it worked
after-the-fact. This is unlike all other existing KMS properties.

I'd much rather see a more general approach to fix this combinatorial explosion
than to add special-cases like this.

Pekka Paalanen June 29, 2021, 11:17 a.m. UTC | #3

On Tue, 29 Jun 2021 08:12:54 +0000
Simon Ser <[e-mail]> wrote:

> On Tuesday, June 22nd, 2021 at 09:15, Pekka Paalanen <[e-mail]> wrote:
> 
> > yes, I think this makes sense, even if it is a property that one can't
> > tell for sure what it does before hand.
> >
> > Using a pair of properties, preference and active, to ask for something
> > and then check what actually worked is good for reducing the
> > combinatorial explosion caused by needing to "atomic TEST_ONLY commit"
> > test different KMS configurations. Userspace has a better chance of
> > finding a configuration that is possible.
> >
> > OTOH, this has the problem than in UI one cannot tell the user in
> > advance which options are truly possible. Given that KMS properties are
> > rarely completely independent, and in this case known to depend on
> > several other KMS properties, I think it is good enough to know after
> > the fact.
> >
> > If a driver does not use what userspace prefers, there is no way to
> > understand why, or what else to change to make it happen. That problem
> > exists anyway, because TEST_ONLY commits do not give useful feedback
> > but only a yes/no.  
> 
> By submitting incremental atomic reqs with TEST_ONLY (i.e. only changing one
> property at a time), user-space can discover which property makes the atomic
> commit fail.

That works if the properties are independent of each other. Color
range, color format, bpc and more may all be interconnected,
allowing only certain combinations to work.

If all these properties have "auto" setting too, then it would be
possible to probe each property individually, but that still does not
tell which combinations are valid.

If you probe towards a certain configuration by setting the properties
one by one, then depending on the order you pick the properties, you
may come to a different conclusion on which property breaks the
configuration.

> I'm not a fan of this "preference" property approach. The only way to find out
> whether it's possible to change the color format is to perform a user-visible
> change (with a regular atomic commit) and check whether it worked
> after-the-fact. This is unlike all other existing KMS properties.

I agree. FWIW, "max bpc" exists already.

> I'd much rather see a more general approach to fix this combinatorial explosion
> than to add special-cases like this.

What would you suggest?

Maybe all properties should have an "auto" value in addition to the
explicit no-negotiation values where at all possible?

That might help probing each property individually with TEST_ONLY
commits, but it says nothing about combinations.

A feedback list perhaps? TEST_ONLY commit somehow returning a list of
property/value tuples indicating what value the "auto" valued
properties actually get?

What should a kernel driver optimize for when determining "auto" values?


Thanks,
pq

Werner Sembach June 29, 2021, 11:37 a.m. UTC | #4

Am 29.06.21 um 13:17 schrieb Pekka Paalanen:
> On Tue, 29 Jun 2021 08:12:54 +0000
> Simon Ser <[e-mail]> wrote:
>
>> On Tuesday, June 22nd, 2021 at 09:15, Pekka Paalanen <[e-mail]> wrote:
>>
>>> yes, I think this makes sense, even if it is a property that one can't
>>> tell for sure what it does before hand.
>>>
>>> Using a pair of properties, preference and active, to ask for something
>>> and then check what actually worked is good for reducing the
>>> combinatorial explosion caused by needing to "atomic TEST_ONLY commit"
>>> test different KMS configurations. Userspace has a better chance of
>>> finding a configuration that is possible.
>>>
>>> OTOH, this has the problem than in UI one cannot tell the user in
>>> advance which options are truly possible. Given that KMS properties are
>>> rarely completely independent, and in this case known to depend on
>>> several other KMS properties, I think it is good enough to know after
>>> the fact.
>>>
>>> If a driver does not use what userspace prefers, there is no way to
>>> understand why, or what else to change to make it happen. That problem
>>> exists anyway, because TEST_ONLY commits do not give useful feedback
>>> but only a yes/no.  
>> By submitting incremental atomic reqs with TEST_ONLY (i.e. only changing one
>> property at a time), user-space can discover which property makes the atomic
>> commit fail.
> That works if the properties are independent of each other. Color
> range, color format, bpc and more may all be interconnected,
> allowing only certain combinations to work.
>
> If all these properties have "auto" setting too, then it would be
> possible to probe each property individually, but that still does not
> tell which combinations are valid.
>
> If you probe towards a certain configuration by setting the properties
> one by one, then depending on the order you pick the properties, you
> may come to a different conclusion on which property breaks the
> configuration.

My mind crossed another point that must be considered: When plugin in a Monitor a list of possible Resolutions+Framerate
combinations is created for xrandr and other userspace (I guess by atomic checks? but I don't know). During this drm
properties are already considered, which is no problem atm because as far as i can tell there is currently no drm
property that would make a certain Resolutions+Framerate combination unreachable that would be possible with everything
on default.

However for example forcing YCbCr420 encoding would limit the available resolutions (my screen for example only supports
YCbCr420 on 4k@60 and @50Hz and on no other resolution or frequency (native is 2560x1440@144Hz).

So would a "force color format" that does not get resetted on repluging/reenabling a monitor break the output, for
example, of an not updated xrandr, unaware of this new property?

>
>> I'm not a fan of this "preference" property approach. The only way to find out
>> whether it's possible to change the color format is to perform a user-visible
>> change (with a regular atomic commit) and check whether it worked
>> after-the-fact. This is unlike all other existing KMS properties.
> I agree. FWIW, "max bpc" exists already.
>
>> I'd much rather see a more general approach to fix this combinatorial explosion
>> than to add special-cases like this.
> What would you suggest?
>
> Maybe all properties should have an "auto" value in addition to the
> explicit no-negotiation values where at all possible?
>
> That might help probing each property individually with TEST_ONLY
> commits, but it says nothing about combinations.
>
> A feedback list perhaps? TEST_ONLY commit somehow returning a list of
> property/value tuples indicating what value the "auto" valued
> properties actually get?
>
> What should a kernel driver optimize for when determining "auto" values?
>
>
> Thanks,
> pq
>
> _______________________________________________
> amd-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Werner Sembach June 29, 2021, 11:39 a.m. UTC | #5

Am 29.06.21 um 13:17 schrieb Pekka Paalanen:
> On Tue, 29 Jun 2021 08:12:54 +0000
> Simon Ser <[e-mail]> wrote:
>
>> On Tuesday, June 22nd, 2021 at 09:15, Pekka Paalanen <[e-mail]> wrote:
>>
>>> yes, I think this makes sense, even if it is a property that one can't
>>> tell for sure what it does before hand.
>>>
>>> Using a pair of properties, preference and active, to ask for something
>>> and then check what actually worked is good for reducing the
>>> combinatorial explosion caused by needing to "atomic TEST_ONLY commit"
>>> test different KMS configurations. Userspace has a better chance of
>>> finding a configuration that is possible.
>>>
>>> OTOH, this has the problem than in UI one cannot tell the user in
>>> advance which options are truly possible. Given that KMS properties are
>>> rarely completely independent, and in this case known to depend on
>>> several other KMS properties, I think it is good enough to know after
>>> the fact.
>>>
>>> If a driver does not use what userspace prefers, there is no way to
>>> understand why, or what else to change to make it happen. That problem
>>> exists anyway, because TEST_ONLY commits do not give useful feedback
>>> but only a yes/no.  
>> By submitting incremental atomic reqs with TEST_ONLY (i.e. only changing one
>> property at a time), user-space can discover which property makes the atomic
>> commit fail.
> That works if the properties are independent of each other. Color
> range, color format, bpc and more may all be interconnected,
> allowing only certain combinations to work.
>
> If all these properties have "auto" setting too, then it would be
> possible to probe each property individually, but that still does not
> tell which combinations are valid.
>
> If you probe towards a certain configuration by setting the properties
> one by one, then depending on the order you pick the properties, you
> may come to a different conclusion on which property breaks the
> configuration.

My mind crossed another point that must be considered: When plugin in a Monitor a list of possible Resolutions+Framerate
combinations is created for xrandr and other userspace (I guess by atomic checks? but I don't know). During this drm
properties are already considered, which is no problem atm because as far as i can tell there is currently no drm
property that would make a certain Resolutions+Framerate combination unreachable that would be possible with everything
on default.

However for example forcing YCbCr420 encoding would limit the available resolutions (my screen for example only supports
YCbCr420 on 4k@60 and @50Hz and on no other resolution or frequency (native is 2560x1440@144Hz).

So would a "force color format" that does not get resetted on repluging/reenabling a monitor break the output, for
example, of an not updated xrandr, unaware of this new property?

>> I'm not a fan of this "preference" property approach. The only way to find out
>> whether it's possible to change the color format is to perform a user-visible
>> change (with a regular atomic commit) and check whether it worked
>> after-the-fact. This is unlike all other existing KMS properties.
> I agree. FWIW, "max bpc" exists already.
>
>> I'd much rather see a more general approach to fix this combinatorial explosion
>> than to add special-cases like this.
> What would you suggest?
>
> Maybe all properties should have an "auto" value in addition to the
> explicit no-negotiation values where at all possible?
>
> That might help probing each property individually with TEST_ONLY
> commits, but it says nothing about combinations.
>
> A feedback list perhaps? TEST_ONLY commit somehow returning a list of
> property/value tuples indicating what value the "auto" valued
> properties actually get?
>
> What should a kernel driver optimize for when determining "auto" values?
>
>
> Thanks,
> pq
> _______________________________________________
> amd-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Pekka Paalanen June 30, 2021, 8:41 a.m. UTC | #6

On Tue, 29 Jun 2021 13:39:18 +0200
Werner Sembach <[e-mail]> wrote:

> Am 29.06.21 um 13:17 schrieb Pekka Paalanen:
> > On Tue, 29 Jun 2021 08:12:54 +0000
> > Simon Ser <[e-mail]> wrote:
> >  
> >> On Tuesday, June 22nd, 2021 at 09:15, Pekka Paalanen <[e-mail]> wrote:
> >>  
> >>> yes, I think this makes sense, even if it is a property that one can't
> >>> tell for sure what it does before hand.
> >>>
> >>> Using a pair of properties, preference and active, to ask for something
> >>> and then check what actually worked is good for reducing the
> >>> combinatorial explosion caused by needing to "atomic TEST_ONLY commit"
> >>> test different KMS configurations. Userspace has a better chance of
> >>> finding a configuration that is possible.
> >>>
> >>> OTOH, this has the problem than in UI one cannot tell the user in
> >>> advance which options are truly possible. Given that KMS properties are
> >>> rarely completely independent, and in this case known to depend on
> >>> several other KMS properties, I think it is good enough to know after
> >>> the fact.
> >>>
> >>> If a driver does not use what userspace prefers, there is no way to
> >>> understand why, or what else to change to make it happen. That problem
> >>> exists anyway, because TEST_ONLY commits do not give useful feedback
> >>> but only a yes/no.    
> >> By submitting incremental atomic reqs with TEST_ONLY (i.e. only changing one
> >> property at a time), user-space can discover which property makes the atomic
> >> commit fail.  
> > That works if the properties are independent of each other. Color
> > range, color format, bpc and more may all be interconnected,
> > allowing only certain combinations to work.
> >
> > If all these properties have "auto" setting too, then it would be
> > possible to probe each property individually, but that still does not
> > tell which combinations are valid.
> >
> > If you probe towards a certain configuration by setting the properties
> > one by one, then depending on the order you pick the properties, you
> > may come to a different conclusion on which property breaks the
> > configuration.  
> 
> My mind crossed another point that must be considered: When plugin in
> a Monitor a list of possible Resolutions+Framerate combinations is
> created for xrandr and other userspace (I guess by atomic checks? but
> I don't know).

Hi,

I would not think so, but I hope to be corrected if I'm wrong.

My belief is that the driver collects a list of modes from EDID, some
standard modes, and maybe some other hardcoded modes, and then
validates each entry against all the known limitations like vertical
and horizontal frequency limits, discarding modes that do not fit.

Not all limitations are known during that phase, which is why KMS
property "link-status" exists. When userspace actually programs a mode
(not a TEST_ONLY commit), the link training may fail. The kernel prunes
the mode from the list and sets the link status property to signal
failure, and sends a hotplug uevent. Userspace needs to re-check the
mode list and try again.

That is a generic escape hatch for when TEST_ONLY commit succeeds, but
in reality the hardware cannot do it, you just cannot know until you
actually try for real. It causes end user visible flicker if it happens
on an already running connector, but since it usually happens when
turning a connector on to begin with, there is no flicker to be seen,
just a small delay in finding a mode that works.

> During this drm
> properties are already considered, which is no problem atm because as
> far as i can tell there is currently no drm property that would make
> a certain Resolutions+Framerate combination unreachable that would be
> possible with everything on default.

I would not expect KMS properties to be considered at all. It would
reject modes that are actually possible if the some KMS properties were
changed. So at least going forward, current KMS property values cannot
factor in.

> However for example forcing YCbCr420 encoding would limit the
> available resolutions (my screen for example only supports YCbCr420
> on 4k@60 and @50Hz and on no other resolution or frequency (native is
> 2560x1440@144Hz).
> 
> So would a "force color format" that does not get resetted on
> repluging/reenabling a monitor break the output, for example, of an
> not updated xrandr, unaware of this new property?

Yes, not because the mode list would be missing the mode, but because
actually setting the mode would fail.

RandR in particular is problematic, because it does not actually
understand any KMS properties, it is merely a relay. So anything
that *uses* RandR protocol or xrandr command would also need to be
patched to understand the new properties.

The kernel automatically resetting *some* properties in *some*
occasions seems really fragile and complicated to me, which is why I'm
a lot more keen to see a "reset everything to sensible defaults"
generic mechanism added to KMS.


Thanks,
pq

Werner Sembach June 30, 2021, 9:20 a.m. UTC | #7

Am 30.06.21 um 10:41 schrieb Pekka Paalanen:

> On Tue, 29 Jun 2021 13:39:18 +0200
> Werner Sembach <[e-mail]> wrote:
>
>> Am 29.06.21 um 13:17 schrieb Pekka Paalanen:
>>> On Tue, 29 Jun 2021 08:12:54 +0000
>>> Simon Ser <[e-mail]> wrote:
>>>   
>>>> On Tuesday, June 22nd, 2021 at 09:15, Pekka Paalanen <[e-mail]> wrote:
>>>>   
>>>>> yes, I think this makes sense, even if it is a property that one can't
>>>>> tell for sure what it does before hand.
>>>>>
>>>>> Using a pair of properties, preference and active, to ask for something
>>>>> and then check what actually worked is good for reducing the
>>>>> combinatorial explosion caused by needing to "atomic TEST_ONLY commit"
>>>>> test different KMS configurations. Userspace has a better chance of
>>>>> finding a configuration that is possible.
>>>>>
>>>>> OTOH, this has the problem than in UI one cannot tell the user in
>>>>> advance which options are truly possible. Given that KMS properties are
>>>>> rarely completely independent, and in this case known to depend on
>>>>> several other KMS properties, I think it is good enough to know after
>>>>> the fact.
>>>>>
>>>>> If a driver does not use what userspace prefers, there is no way to
>>>>> understand why, or what else to change to make it happen. That problem
>>>>> exists anyway, because TEST_ONLY commits do not give useful feedback
>>>>> but only a yes/no.
>>>> By submitting incremental atomic reqs with TEST_ONLY (i.e. only changing one
>>>> property at a time), user-space can discover which property makes the atomic
>>>> commit fail.
>>> That works if the properties are independent of each other. Color
>>> range, color format, bpc and more may all be interconnected,
>>> allowing only certain combinations to work.
>>>
>>> If all these properties have "auto" setting too, then it would be
>>> possible to probe each property individually, but that still does not
>>> tell which combinations are valid.
>>>
>>> If you probe towards a certain configuration by setting the properties
>>> one by one, then depending on the order you pick the properties, you
>>> may come to a different conclusion on which property breaks the
>>> configuration.
>> My mind crossed another point that must be considered: When plugin in
>> a Monitor a list of possible Resolutions+Framerate combinations is
>> created for xrandr and other userspace (I guess by atomic checks? but
>> I don't know).
> Hi,
>
> I would not think so, but I hope to be corrected if I'm wrong.
>
> My belief is that the driver collects a list of modes from EDID, some
> standard modes, and maybe some other hardcoded modes, and then
> validates each entry against all the known limitations like vertical
> and horizontal frequency limits, discarding modes that do not fit.
>
> Not all limitations are known during that phase, which is why KMS
> property "link-status" exists. When userspace actually programs a mode
> (not a TEST_ONLY commit), the link training may fail. The kernel prunes
> the mode from the list and sets the link status property to signal
> failure, and sends a hotplug uevent. Userspace needs to re-check the
> mode list and try again.
>
> That is a generic escape hatch for when TEST_ONLY commit succeeds, but
> in reality the hardware cannot do it, you just cannot know until you
> actually try for real. It causes end user visible flicker if it happens
> on an already running connector, but since it usually happens when
> turning a connector on to begin with, there is no flicker to be seen,
> just a small delay in finding a mode that works.
>
>> During this drm
>> properties are already considered, which is no problem atm because as
>> far as i can tell there is currently no drm property that would make
>> a certain Resolutions+Framerate combination unreachable that would be
>> possible with everything on default.
> I would not expect KMS properties to be considered at all. It would
> reject modes that are actually possible if the some KMS properties were
> changed. So at least going forward, current KMS property values cannot
> factor in.

At least the debugfs variable "force_yuv420_output" did change the 
available modes here: 
https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L5165 
before my patch 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68eb3ae3c63708f823aeeb63bb15197c727bd9bf

Forcing a color format via a DRM property in this function would 
reintroduce the problem.

And I think i915 driver works similar in this regard.

>
>> However for example forcing YCbCr420 encoding would limit the
>> available resolutions (my screen for example only supports YCbCr420
>> on 4k@60 and @50Hz and on no other resolution or frequency (native is
>> 2560x1440@144Hz).
>>
>> So would a "force color format" that does not get resetted on
>> repluging/reenabling a monitor break the output, for example, of an
>> not updated xrandr, unaware of this new property?
> Yes, not because the mode list would be missing the mode, but because
> actually setting the mode would fail.
Well, like described above, I think the mode would actually be missing, 
which is also an unexpected behavior from a user perspective.
>
> RandR in particular is problematic, because it does not actually
> understand any KMS properties, it is merely a relay. So anything
> that *uses* RandR protocol or xrandr command would also need to be
> patched to understand the new properties.
>
> The kernel automatically resetting *some* properties in *some*
> occasions seems really fragile and complicated to me, which is why I'm
> a lot more keen to see a "reset everything to sensible defaults"
> generic mechanism added to KMS.
Would you see that mechanism not (yet) existing a blocker for this 
patchset/the "force-" properties?
>
>
> Thanks,
> pq
> _______________________________________________
> amd-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Pekka Paalanen July 1, 2021, 8:07 a.m. UTC | #8

On Wed, 30 Jun 2021 11:20:18 +0200
Werner Sembach <[e-mail]> wrote:

> Am 30.06.21 um 10:41 schrieb Pekka Paalanen:
> 
> > On Tue, 29 Jun 2021 13:39:18 +0200
> > Werner Sembach <[e-mail]> wrote:
> >  
> >> Am 29.06.21 um 13:17 schrieb Pekka Paalanen:  
> >>> On Tue, 29 Jun 2021 08:12:54 +0000
> >>> Simon Ser <[e-mail]> wrote:
> >>>     
> >>>> On Tuesday, June 22nd, 2021 at 09:15, Pekka Paalanen <[e-mail]> wrote:
> >>>>     
> >>>>> yes, I think this makes sense, even if it is a property that one can't
> >>>>> tell for sure what it does before hand.
> >>>>>
> >>>>> Using a pair of properties, preference and active, to ask for something
> >>>>> and then check what actually worked is good for reducing the
> >>>>> combinatorial explosion caused by needing to "atomic TEST_ONLY commit"
> >>>>> test different KMS configurations. Userspace has a better chance of
> >>>>> finding a configuration that is possible.
> >>>>>
> >>>>> OTOH, this has the problem than in UI one cannot tell the user in
> >>>>> advance which options are truly possible. Given that KMS properties are
> >>>>> rarely completely independent, and in this case known to depend on
> >>>>> several other KMS properties, I think it is good enough to know after
> >>>>> the fact.
> >>>>>
> >>>>> If a driver does not use what userspace prefers, there is no way to
> >>>>> understand why, or what else to change to make it happen. That problem
> >>>>> exists anyway, because TEST_ONLY commits do not give useful feedback
> >>>>> but only a yes/no.  
> >>>> By submitting incremental atomic reqs with TEST_ONLY (i.e. only changing one
> >>>> property at a time), user-space can discover which property makes the atomic
> >>>> commit fail.  
> >>> That works if the properties are independent of each other. Color
> >>> range, color format, bpc and more may all be interconnected,
> >>> allowing only certain combinations to work.
> >>>
> >>> If all these properties have "auto" setting too, then it would be
> >>> possible to probe each property individually, but that still does not
> >>> tell which combinations are valid.
> >>>
> >>> If you probe towards a certain configuration by setting the properties
> >>> one by one, then depending on the order you pick the properties, you
> >>> may come to a different conclusion on which property breaks the
> >>> configuration.  
> >> My mind crossed another point that must be considered: When plugin in
> >> a Monitor a list of possible Resolutions+Framerate combinations is
> >> created for xrandr and other userspace (I guess by atomic checks? but
> >> I don't know).  
> > Hi,
> >
> > I would not think so, but I hope to be corrected if I'm wrong.
> >
> > My belief is that the driver collects a list of modes from EDID, some
> > standard modes, and maybe some other hardcoded modes, and then
> > validates each entry against all the known limitations like vertical
> > and horizontal frequency limits, discarding modes that do not fit.
> >
> > Not all limitations are known during that phase, which is why KMS
> > property "link-status" exists. When userspace actually programs a mode
> > (not a TEST_ONLY commit), the link training may fail. The kernel prunes
> > the mode from the list and sets the link status property to signal
> > failure, and sends a hotplug uevent. Userspace needs to re-check the
> > mode list and try again.
> >
> > That is a generic escape hatch for when TEST_ONLY commit succeeds, but
> > in reality the hardware cannot do it, you just cannot know until you
> > actually try for real. It causes end user visible flicker if it happens
> > on an already running connector, but since it usually happens when
> > turning a connector on to begin with, there is no flicker to be seen,
> > just a small delay in finding a mode that works.
> >  
> >> During this drm
> >> properties are already considered, which is no problem atm because as
> >> far as i can tell there is currently no drm property that would make
> >> a certain Resolutions+Framerate combination unreachable that would be
> >> possible with everything on default.  
> > I would not expect KMS properties to be considered at all. It would
> > reject modes that are actually possible if the some KMS properties were
> > changed. So at least going forward, current KMS property values cannot
> > factor in.  
> 
> At least the debugfs variable "force_yuv420_output" did change the 
> available modes here: 
> https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L5165 
> before my patch 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68eb3ae3c63708f823aeeb63bb15197c727bd9bf

Hi,

debugfs is not proper UAPI, so we can just ignore it. Display servers
cannot be expected to poke in debugfs. Debugfs is not even supposed to
exist in production systems, but I'm sure people use it for hacking
stuff. But that's all it is for: developer testing and hacking.

> Forcing a color format via a DRM property in this function would 
> reintroduce the problem.

The property would need to be defined differently because its presence
could otherwise break existing userspace. Well, I suppose it could
break existing userspace no matter what, so we would need the generic
"reset to sane defaults" mechanism first IMO.

DRM has client caps for exposing video modes that old userspace might
not expect, to avoid breaking old userspace. Care needs to be taken
with all new UAPI, because if a kernel upgrade makes something wrong,
it's the kernel's fault no matter what userspace is doing, in principle.

Debugfs has no problem breaking userspace AFAIU, since it's not proper
UAPI.

> And I think i915 driver works similar in this regard.
> 
> >  
> >> However for example forcing YCbCr420 encoding would limit the
> >> available resolutions (my screen for example only supports YCbCr420
> >> on 4k@60 and @50Hz and on no other resolution or frequency (native is
> >> 2560x1440@144Hz).
> >>
> >> So would a "force color format" that does not get resetted on
> >> repluging/reenabling a monitor break the output, for example, of an
> >> not updated xrandr, unaware of this new property?  
> > Yes, not because the mode list would be missing the mode, but because
> > actually setting the mode would fail.  
> Well, like described above, I think the mode would actually be missing, 
> which is also an unexpected behavior from a user perspective.

I think that is not how the property should work.

If KMS properties would affect the list of modes, then userspace would
need to set the properties for real (TEST_ONLY cannot change anything)
and re-fetch the mode lists (maybe there is a hotplug event, maybe
not). That workflow just doesn't work.

A very interesting question is when should link-status failure not drop
the current mode from the mode list, if other KMS properties affect the
bandwidth etc. requirements of the mode. That mechanism is engineered
for old userspace that doesn't actually handle link-status but does
handle hotplug, so the hotplug triggers re-fetching the mode list and
userspace maybe trying again with a better luck since the offending
mode is gone. How to keep that working when introducing KMS properties
forcing the cable format, I don't know.

As long as the other affecting KMS properties are all "auto", the
driver will probably exhaust all possibilities to make the mode work
before signalling link-status failure and pruning the mode.
Theoretically, as I have no idea what drivers actually do.

> >
> > RandR in particular is problematic, because it does not actually
> > understand any KMS properties, it is merely a relay. So anything
> > that *uses* RandR protocol or xrandr command would also need to be
> > patched to understand the new properties.
> >
> > The kernel automatically resetting *some* properties in *some*
> > occasions seems really fragile and complicated to me, which is why I'm
> > a lot more keen to see a "reset everything to sensible defaults"
> > generic mechanism added to KMS.  
> Would you see that mechanism not (yet) existing a blocker for this 
> patchset/the "force-" properties?

For the active properties, no.

For the force properties, that is a very good question and I am
somewhat concerned. I can very much see how the force properties would
break userspace, but since it would require other userspace to mess up
the KMS configuration first, I'm not sure if kernel developers would
see that as a kernel regression as it is an existing problem. The force
properties just make it more pronounced.


Thanks,
pq

Werner Sembach July 1, 2021, 12:50 p.m. UTC | #9

Am 01.07.21 um 10:07 schrieb Pekka Paalanen:

> On Wed, 30 Jun 2021 11:20:18 +0200
> Werner Sembach <[e-mail]> wrote:
>
>> Am 30.06.21 um 10:41 schrieb Pekka Paalanen:
>>
>>> On Tue, 29 Jun 2021 13:39:18 +0200
>>> Werner Sembach <[e-mail]> wrote:
>>>  
>>>> Am 29.06.21 um 13:17 schrieb Pekka Paalanen:  
>>>>> On Tue, 29 Jun 2021 08:12:54 +0000
>>>>> Simon Ser <[e-mail]> wrote:
>>>>>     
>>>>>> On Tuesday, June 22nd, 2021 at 09:15, Pekka Paalanen <[e-mail]> wrote:
>>>>>>     
>>>>>>> yes, I think this makes sense, even if it is a property that one can't
>>>>>>> tell for sure what it does before hand.
>>>>>>>
>>>>>>> Using a pair of properties, preference and active, to ask for something
>>>>>>> and then check what actually worked is good for reducing the
>>>>>>> combinatorial explosion caused by needing to "atomic TEST_ONLY commit"
>>>>>>> test different KMS configurations. Userspace has a better chance of
>>>>>>> finding a configuration that is possible.
>>>>>>>
>>>>>>> OTOH, this has the problem than in UI one cannot tell the user in
>>>>>>> advance which options are truly possible. Given that KMS properties are
>>>>>>> rarely completely independent, and in this case known to depend on
>>>>>>> several other KMS properties, I think it is good enough to know after
>>>>>>> the fact.
>>>>>>>
>>>>>>> If a driver does not use what userspace prefers, there is no way to
>>>>>>> understand why, or what else to change to make it happen. That problem
>>>>>>> exists anyway, because TEST_ONLY commits do not give useful feedback
>>>>>>> but only a yes/no.  
>>>>>> By submitting incremental atomic reqs with TEST_ONLY (i.e. only changing one
>>>>>> property at a time), user-space can discover which property makes the atomic
>>>>>> commit fail.  
>>>>> That works if the properties are independent of each other. Color
>>>>> range, color format, bpc and more may all be interconnected,
>>>>> allowing only certain combinations to work.
>>>>>
>>>>> If all these properties have "auto" setting too, then it would be
>>>>> possible to probe each property individually, but that still does not
>>>>> tell which combinations are valid.
>>>>>
>>>>> If you probe towards a certain configuration by setting the properties
>>>>> one by one, then depending on the order you pick the properties, you
>>>>> may come to a different conclusion on which property breaks the
>>>>> configuration.  
>>>> My mind crossed another point that must be considered: When plugin in
>>>> a Monitor a list of possible Resolutions+Framerate combinations is
>>>> created for xrandr and other userspace (I guess by atomic checks? but
>>>> I don't know).  
>>> Hi,
>>>
>>> I would not think so, but I hope to be corrected if I'm wrong.
>>>
>>> My belief is that the driver collects a list of modes from EDID, some
>>> standard modes, and maybe some other hardcoded modes, and then
>>> validates each entry against all the known limitations like vertical
>>> and horizontal frequency limits, discarding modes that do not fit.
>>>
>>> Not all limitations are known during that phase, which is why KMS
>>> property "link-status" exists. When userspace actually programs a mode
>>> (not a TEST_ONLY commit), the link training may fail. The kernel prunes
>>> the mode from the list and sets the link status property to signal
>>> failure, and sends a hotplug uevent. Userspace needs to re-check the
>>> mode list and try again.
>>>
>>> That is a generic escape hatch for when TEST_ONLY commit succeeds, but
>>> in reality the hardware cannot do it, you just cannot know until you
>>> actually try for real. It causes end user visible flicker if it happens
>>> on an already running connector, but since it usually happens when
>>> turning a connector on to begin with, there is no flicker to be seen,
>>> just a small delay in finding a mode that works.
>>>  
>>>> During this drm
>>>> properties are already considered, which is no problem atm because as
>>>> far as i can tell there is currently no drm property that would make
>>>> a certain Resolutions+Framerate combination unreachable that would be
>>>> possible with everything on default.  
>>> I would not expect KMS properties to be considered at all. It would
>>> reject modes that are actually possible if the some KMS properties were
>>> changed. So at least going forward, current KMS property values cannot
>>> factor in.  
>> At least the debugfs variable "force_yuv420_output" did change the 
>> available modes here: 
>> https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L5165 
>> before my patch 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68eb3ae3c63708f823aeeb63bb15197c727bd9bf
> Hi,
>
> debugfs is not proper UAPI, so we can just ignore it. Display servers
> cannot be expected to poke in debugfs. Debugfs is not even supposed to
> exist in production systems, but I'm sure people use it for hacking
> stuff. But that's all it is for: developer testing and hacking.
e.g. Ubuntu has it active by default, but only read (and writable) by root.
>
>> Forcing a color format via a DRM property in this function would 
>> reintroduce the problem.
> The property would need to be defined differently because its presence
> could otherwise break existing userspace. Well, I suppose it could
> break existing userspace no matter what, so we would need the generic
> "reset to sane defaults" mechanism first IMO.
>
> DRM has client caps for exposing video modes that old userspace might
> not expect, to avoid breaking old userspace. Care needs to be taken
> with all new UAPI, because if a kernel upgrade makes something wrong,
> it's the kernel's fault no matter what userspace is doing, in principle.
Can you give me a link describing how I define this caps?
>
> Debugfs has no problem breaking userspace AFAIU, since it's not proper
> UAPI.
>
>> And I think i915 driver works similar in this regard.
>>
>>>  
>>>> However for example forcing YCbCr420 encoding would limit the
>>>> available resolutions (my screen for example only supports YCbCr420
>>>> on 4k@60 and @50Hz and on no other resolution or frequency (native is
>>>> 2560x1440@144Hz).
>>>>
>>>> So would a "force color format" that does not get resetted on
>>>> repluging/reenabling a monitor break the output, for example, of an
>>>> not updated xrandr, unaware of this new property?  
>>> Yes, not because the mode list would be missing the mode, but because
>>> actually setting the mode would fail.  
>> Well, like described above, I think the mode would actually be missing, 
>> which is also an unexpected behavior from a user perspective.
> I think that is not how the property should work.
>
> If KMS properties would affect the list of modes, then userspace would
> need to set the properties for real (TEST_ONLY cannot change anything)
> and re-fetch the mode lists (maybe there is a hotplug event, maybe
> not). That workflow just doesn't work.
The properties are set before the list is created in the first place. Because, in my example, the properties get set
before the monitor is plugged in and the list can only be created as soon as the monitor is plugged in.
>
> A very interesting question is when should link-status failure not drop
> the current mode from the mode list, if other KMS properties affect the
> bandwidth etc. requirements of the mode. That mechanism is engineered
> for old userspace that doesn't actually handle link-status but does
> handle hotplug, so the hotplug triggers re-fetching the mode list and
> userspace maybe trying again with a better luck since the offending
> mode is gone. How to keep that working when introducing KMS properties
> forcing the cable format, I don't know.
>
> As long as the other affecting KMS properties are all "auto", the
> driver will probably exhaust all possibilities to make the mode work
> before signalling link-status failure and pruning the mode.
> Theoretically, as I have no idea what drivers actually do.
Isn't that exactly how the "preferred color format" property works in my patchset now?
>
>>> RandR in particular is problematic, because it does not actually
>>> understand any KMS properties, it is merely a relay. So anything
>>> that *uses* RandR protocol or xrandr command would also need to be
>>> patched to understand the new properties.
>>>
>>> The kernel automatically resetting *some* properties in *some*
>>> occasions seems really fragile and complicated to me, which is why I'm
>>> a lot more keen to see a "reset everything to sensible defaults"
>>> generic mechanism added to KMS.  
>> Would you see that mechanism not (yet) existing a blocker for this 
>> patchset/the "force-" properties?
> For the active properties, no.
>
> For the force properties, that is a very good question and I am
> somewhat concerned. I can very much see how the force properties would
> break userspace, but since it would require other userspace to mess up
> the KMS configuration first, I'm not sure if kernel developers would
> see that as a kernel regression as it is an existing problem. The force
> properties just make it more pronounced.
>
>
> Thanks,
> pq

Pekka Paalanen July 1, 2021, 1:24 p.m. UTC | #10

On Thu, 1 Jul 2021 14:50:13 +0200
Werner Sembach <[e-mail]> wrote:

> Am 01.07.21 um 10:07 schrieb Pekka Paalanen:
> 
> > On Wed, 30 Jun 2021 11:20:18 +0200
> > Werner Sembach <[e-mail]> wrote:
> >  
> >> Am 30.06.21 um 10:41 schrieb Pekka Paalanen:
> >>  
> >>> On Tue, 29 Jun 2021 13:39:18 +0200
> >>> Werner Sembach <[e-mail]> wrote:
> >>>    
> >>>> Am 29.06.21 um 13:17 schrieb Pekka Paalanen:    
> >>>>> On Tue, 29 Jun 2021 08:12:54 +0000
> >>>>> Simon Ser <[e-mail]> wrote:
> >>>>>       
> >>>>>> On Tuesday, June 22nd, 2021 at 09:15, Pekka Paalanen <[e-mail]> wrote:
> >>>>>>       
> >>>>>>> yes, I think this makes sense, even if it is a property that one can't
> >>>>>>> tell for sure what it does before hand.
> >>>>>>>
> >>>>>>> Using a pair of properties, preference and active, to ask for something
> >>>>>>> and then check what actually worked is good for reducing the
> >>>>>>> combinatorial explosion caused by needing to "atomic TEST_ONLY commit"
> >>>>>>> test different KMS configurations. Userspace has a better chance of
> >>>>>>> finding a configuration that is possible.
> >>>>>>>
> >>>>>>> OTOH, this has the problem than in UI one cannot tell the user in
> >>>>>>> advance which options are truly possible. Given that KMS properties are
> >>>>>>> rarely completely independent, and in this case known to depend on
> >>>>>>> several other KMS properties, I think it is good enough to know after
> >>>>>>> the fact.
> >>>>>>>
> >>>>>>> If a driver does not use what userspace prefers, there is no way to
> >>>>>>> understand why, or what else to change to make it happen. That problem
> >>>>>>> exists anyway, because TEST_ONLY commits do not give useful feedback
> >>>>>>> but only a yes/no.    
> >>>>>> By submitting incremental atomic reqs with TEST_ONLY (i.e. only changing one
> >>>>>> property at a time), user-space can discover which property makes the atomic
> >>>>>> commit fail.    
> >>>>> That works if the properties are independent of each other. Color
> >>>>> range, color format, bpc and more may all be interconnected,
> >>>>> allowing only certain combinations to work.
> >>>>>
> >>>>> If all these properties have "auto" setting too, then it would be
> >>>>> possible to probe each property individually, but that still does not
> >>>>> tell which combinations are valid.
> >>>>>
> >>>>> If you probe towards a certain configuration by setting the properties
> >>>>> one by one, then depending on the order you pick the properties, you
> >>>>> may come to a different conclusion on which property breaks the
> >>>>> configuration.    
> >>>> My mind crossed another point that must be considered: When plugin in
> >>>> a Monitor a list of possible Resolutions+Framerate combinations is
> >>>> created for xrandr and other userspace (I guess by atomic checks? but
> >>>> I don't know).    
> >>> Hi,
> >>>
> >>> I would not think so, but I hope to be corrected if I'm wrong.
> >>>
> >>> My belief is that the driver collects a list of modes from EDID, some
> >>> standard modes, and maybe some other hardcoded modes, and then
> >>> validates each entry against all the known limitations like vertical
> >>> and horizontal frequency limits, discarding modes that do not fit.
> >>>
> >>> Not all limitations are known during that phase, which is why KMS
> >>> property "link-status" exists. When userspace actually programs a mode
> >>> (not a TEST_ONLY commit), the link training may fail. The kernel prunes
> >>> the mode from the list and sets the link status property to signal
> >>> failure, and sends a hotplug uevent. Userspace needs to re-check the
> >>> mode list and try again.
> >>>
> >>> That is a generic escape hatch for when TEST_ONLY commit succeeds, but
> >>> in reality the hardware cannot do it, you just cannot know until you
> >>> actually try for real. It causes end user visible flicker if it happens
> >>> on an already running connector, but since it usually happens when
> >>> turning a connector on to begin with, there is no flicker to be seen,
> >>> just a small delay in finding a mode that works.
> >>>    
> >>>> During this drm
> >>>> properties are already considered, which is no problem atm because as
> >>>> far as i can tell there is currently no drm property that would make
> >>>> a certain Resolutions+Framerate combination unreachable that would be
> >>>> possible with everything on default.    
> >>> I would not expect KMS properties to be considered at all. It would
> >>> reject modes that are actually possible if the some KMS properties were
> >>> changed. So at least going forward, current KMS property values cannot
> >>> factor in.    
> >> At least the debugfs variable "force_yuv420_output" did change the 
> >> available modes here: 
> >> https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L5165 
> >> before my patch 
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68eb3ae3c63708f823aeeb63bb15197c727bd9bf  
> > Hi,
> >
> > debugfs is not proper UAPI, so we can just ignore it. Display servers
> > cannot be expected to poke in debugfs. Debugfs is not even supposed to
> > exist in production systems, but I'm sure people use it for hacking
> > stuff. But that's all it is for: developer testing and hacking.  
> e.g. Ubuntu has it active by default, but only read (and writable) by root.

Hi,

that's normal, yes. Root can do damage anyway, and it's useful for
debugging. KMS clients OTOH often do not run as root.

> >  
> >> Forcing a color format via a DRM property in this function would 
> >> reintroduce the problem.  
> > The property would need to be defined differently because its presence
> > could otherwise break existing userspace. Well, I suppose it could
> > break existing userspace no matter what, so we would need the generic
> > "reset to sane defaults" mechanism first IMO.
> >
> > DRM has client caps for exposing video modes that old userspace might
> > not expect, to avoid breaking old userspace. Care needs to be taken
> > with all new UAPI, because if a kernel upgrade makes something wrong,
> > it's the kernel's fault no matter what userspace is doing, in principle.  
> Can you give me a link describing how I define this caps?

I don't have any, but you can find all the existing ones by grepping
for DRM_CLIENT_CAP_.

I'm not saying that we need it, but mentioning them as a possible
workaround if userspace breakage seems imminent or is proven.

> >
> > Debugfs has no problem breaking userspace AFAIU, since it's not proper
> > UAPI.
> >  
> >> And I think i915 driver works similar in this regard.
> >>  
> >>>    
> >>>> However for example forcing YCbCr420 encoding would limit the
> >>>> available resolutions (my screen for example only supports YCbCr420
> >>>> on 4k@60 and @50Hz and on no other resolution or frequency (native is
> >>>> 2560x1440@144Hz).
> >>>>
> >>>> So would a "force color format" that does not get resetted on
> >>>> repluging/reenabling a monitor break the output, for example, of an
> >>>> not updated xrandr, unaware of this new property?    
> >>> Yes, not because the mode list would be missing the mode, but because
> >>> actually setting the mode would fail.    
> >> Well, like described above, I think the mode would actually be missing, 
> >> which is also an unexpected behavior from a user perspective.  
> > I think that is not how the property should work.
> >
> > If KMS properties would affect the list of modes, then userspace would
> > need to set the properties for real (TEST_ONLY cannot change anything)
> > and re-fetch the mode lists (maybe there is a hotplug event, maybe
> > not). That workflow just doesn't work.  

> The properties are set before the list is created in the first place.
> Because, in my example, the properties get set before the monitor is
> plugged in and the list can only be created as soon as the monitor is
> plugged in.

That's just an accident, it's not what I mean.

What I mean is, we cannot have the KMS properties affect the list of
modes, because then userspace that want to use specific values on those
properties would have to program those properties first, and then get
the list of modes. KMS UAPI does not work that way AFAIK.

If the initial mode list is created on hotplug like you say, then the
initial list could already be missing some modes that would be valid if
some KMS properties had different values.

> >
> > A very interesting question is when should link-status failure not drop
> > the current mode from the mode list, if other KMS properties affect the
> > bandwidth etc. requirements of the mode. That mechanism is engineered
> > for old userspace that doesn't actually handle link-status but does
> > handle hotplug, so the hotplug triggers re-fetching the mode list and
> > userspace maybe trying again with a better luck since the offending
> > mode is gone. How to keep that working when introducing KMS properties
> > forcing the cable format, I don't know.
> >
> > As long as the other affecting KMS properties are all "auto", the
> > driver will probably exhaust all possibilities to make the mode work
> > before signalling link-status failure and pruning the mode.
> > Theoretically, as I have no idea what drivers actually do.  

> Isn't that exactly how the "preferred color format" property works in
> my patchset now?

There was an argument that "preferred" with no guarantees is not
useful enough. So I'm considering the force property instead.
The problem is, "auto" is not the only possible value.

When the value is not "auto", should link failure drop the mode or not?
Userspace might change the value back to "auto" next time. If you
dropped the mode, it would be gone. If you didn't drop the mode,
userspace might be stuck picking the same non-working mode again and
again if it doesn't know about the force mode property.

You could argue that changing the value back to "auto" needs to reset
the mode list, but that only gets us back to the "need to set
properties before getting mode list".

Maybe there needs to be an assumption that if "force color format" is
not "auto", then link failure does not drop modes and userspace knows
to handle this. Messy.

I'm afraid I just don't know to give any clear answer. It's also
possible that, as I'm not a kernel dev, I have some false assumptions
here.


Thanks,
pq

Werner Sembach July 5, 2021, 3:49 p.m. UTC | #11

Am 01.07.21 um 15:24 schrieb Pekka Paalanen:
> On Thu, 1 Jul 2021 14:50:13 +0200
> Werner Sembach <[e-mail]> wrote:
>
>> Am 01.07.21 um 10:07 schrieb Pekka Paalanen:
>>
>>> On Wed, 30 Jun 2021 11:20:18 +0200
>>> Werner Sembach <[e-mail]> wrote:
>>>  
>>>> Am 30.06.21 um 10:41 schrieb Pekka Paalanen:
>>>>  
>>>>> On Tue, 29 Jun 2021 13:39:18 +0200
>>>>> Werner Sembach <[e-mail]> wrote:
>>>>>    
>>>>>> Am 29.06.21 um 13:17 schrieb Pekka Paalanen:    
>>>>>>> On Tue, 29 Jun 2021 08:12:54 +0000
>>>>>>> Simon Ser <[e-mail]> wrote:
>>>>>>>       
>>>>>>>> On Tuesday, June 22nd, 2021 at 09:15, Pekka Paalanen <[e-mail]> wrote:
>>>>>>>>       
>>>>>>>>> yes, I think this makes sense, even if it is a property that one can't
>>>>>>>>> tell for sure what it does before hand.
>>>>>>>>>
>>>>>>>>> Using a pair of properties, preference and active, to ask for something
>>>>>>>>> and then check what actually worked is good for reducing the
>>>>>>>>> combinatorial explosion caused by needing to "atomic TEST_ONLY commit"
>>>>>>>>> test different KMS configurations. Userspace has a better chance of
>>>>>>>>> finding a configuration that is possible.
>>>>>>>>>
>>>>>>>>> OTOH, this has the problem than in UI one cannot tell the user in
>>>>>>>>> advance which options are truly possible. Given that KMS properties are
>>>>>>>>> rarely completely independent, and in this case known to depend on
>>>>>>>>> several other KMS properties, I think it is good enough to know after
>>>>>>>>> the fact.
>>>>>>>>>
>>>>>>>>> If a driver does not use what userspace prefers, there is no way to
>>>>>>>>> understand why, or what else to change to make it happen. That problem
>>>>>>>>> exists anyway, because TEST_ONLY commits do not give useful feedback
>>>>>>>>> but only a yes/no.    
>>>>>>>> By submitting incremental atomic reqs with TEST_ONLY (i.e. only changing one
>>>>>>>> property at a time), user-space can discover which property makes the atomic
>>>>>>>> commit fail.    
>>>>>>> That works if the properties are independent of each other. Color
>>>>>>> range, color format, bpc and more may all be interconnected,
>>>>>>> allowing only certain combinations to work.
>>>>>>>
>>>>>>> If all these properties have "auto" setting too, then it would be
>>>>>>> possible to probe each property individually, but that still does not
>>>>>>> tell which combinations are valid.
>>>>>>>
>>>>>>> If you probe towards a certain configuration by setting the properties
>>>>>>> one by one, then depending on the order you pick the properties, you
>>>>>>> may come to a different conclusion on which property breaks the
>>>>>>> configuration.    
>>>>>> My mind crossed another point that must be considered: When plugin in
>>>>>> a Monitor a list of possible Resolutions+Framerate combinations is
>>>>>> created for xrandr and other userspace (I guess by atomic checks? but
>>>>>> I don't know).    
>>>>> Hi,
>>>>>
>>>>> I would not think so, but I hope to be corrected if I'm wrong.
>>>>>
>>>>> My belief is that the driver collects a list of modes from EDID, some
>>>>> standard modes, and maybe some other hardcoded modes, and then
>>>>> validates each entry against all the known limitations like vertical
>>>>> and horizontal frequency limits, discarding modes that do not fit.
>>>>>
>>>>> Not all limitations are known during that phase, which is why KMS
>>>>> property "link-status" exists. When userspace actually programs a mode
>>>>> (not a TEST_ONLY commit), the link training may fail. The kernel prunes
>>>>> the mode from the list and sets the link status property to signal
>>>>> failure, and sends a hotplug uevent. Userspace needs to re-check the
>>>>> mode list and try again.
>>>>>
>>>>> That is a generic escape hatch for when TEST_ONLY commit succeeds, but
>>>>> in reality the hardware cannot do it, you just cannot know until you
>>>>> actually try for real. It causes end user visible flicker if it happens
>>>>> on an already running connector, but since it usually happens when
>>>>> turning a connector on to begin with, there is no flicker to be seen,
>>>>> just a small delay in finding a mode that works.
>>>>>    
>>>>>> During this drm
>>>>>> properties are already considered, which is no problem atm because as
>>>>>> far as i can tell there is currently no drm property that would make
>>>>>> a certain Resolutions+Framerate combination unreachable that would be
>>>>>> possible with everything on default.    
>>>>> I would not expect KMS properties to be considered at all. It would
>>>>> reject modes that are actually possible if the some KMS properties were
>>>>> changed. So at least going forward, current KMS property values cannot
>>>>> factor in.    
>>>> At least the debugfs variable "force_yuv420_output" did change the 
>>>> available modes here: 
>>>> https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L5165 
>>>> before my patch 
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68eb3ae3c63708f823aeeb63bb15197c727bd9bf  
>>> Hi,
>>>
>>> debugfs is not proper UAPI, so we can just ignore it. Display servers
>>> cannot be expected to poke in debugfs. Debugfs is not even supposed to
>>> exist in production systems, but I'm sure people use it for hacking
>>> stuff. But that's all it is for: developer testing and hacking.  
>> e.g. Ubuntu has it active by default, but only read (and writable) by root.
> Hi,
>
> that's normal, yes. Root can do damage anyway, and it's useful for
> debugging. KMS clients OTOH often do not run as root.
>
>>>  
>>>> Forcing a color format via a DRM property in this function would 
>>>> reintroduce the problem.  
>>> The property would need to be defined differently because its presence
>>> could otherwise break existing userspace. Well, I suppose it could
>>> break existing userspace no matter what, so we would need the generic
>>> "reset to sane defaults" mechanism first IMO.
>>>
>>> DRM has client caps for exposing video modes that old userspace might
>>> not expect, to avoid breaking old userspace. Care needs to be taken
>>> with all new UAPI, because if a kernel upgrade makes something wrong,
>>> it's the kernel's fault no matter what userspace is doing, in principle.  
>> Can you give me a link describing how I define this caps?
> I don't have any, but you can find all the existing ones by grepping
> for DRM_CLIENT_CAP_.
>
> I'm not saying that we need it, but mentioning them as a possible
> workaround if userspace breakage seems imminent or is proven.
>
>>> Debugfs has no problem breaking userspace AFAIU, since it's not proper
>>> UAPI.
>>>  
>>>> And I think i915 driver works similar in this regard.
>>>>  
>>>>>    
>>>>>> However for example forcing YCbCr420 encoding would limit the
>>>>>> available resolutions (my screen for example only supports YCbCr420
>>>>>> on 4k@60 and @50Hz and on no other resolution or frequency (native is
>>>>>> 2560x1440@144Hz).
>>>>>>
>>>>>> So would a "force color format" that does not get resetted on
>>>>>> repluging/reenabling a monitor break the output, for example, of an
>>>>>> not updated xrandr, unaware of this new property?    
>>>>> Yes, not because the mode list would be missing the mode, but because
>>>>> actually setting the mode would fail.    
>>>> Well, like described above, I think the mode would actually be missing, 
>>>> which is also an unexpected behavior from a user perspective.  
>>> I think that is not how the property should work.
>>>
>>> If KMS properties would affect the list of modes, then userspace would
>>> need to set the properties for real (TEST_ONLY cannot change anything)
>>> and re-fetch the mode lists (maybe there is a hotplug event, maybe
>>> not). That workflow just doesn't work.  
>> The properties are set before the list is created in the first place.
>> Because, in my example, the properties get set before the monitor is
>> plugged in and the list can only be created as soon as the monitor is
>> plugged in.
> That's just an accident, it's not what I mean.
>
> What I mean is, we cannot have the KMS properties affect the list of
> modes, because then userspace that want to use specific values on those
> properties would have to program those properties first, and then get
> the list of modes. KMS UAPI does not work that way AFAIK.
>
> If the initial mode list is created on hotplug like you say, then the
> initial list could already be missing some modes that would be valid if
> some KMS properties had different values.

Depends if the mode list is created by TEST_ONLY:

- The force properties should return false on TEST_ONLY

- The force properties should not prevent the mode from showing up in the list

If the list is created by TEST_ONLY both things can't be fulfilled at the same time obviously.

I hope some can give more insights or has an idea how the properties could work best.

>
>>> A very interesting question is when should link-status failure not drop
>>> the current mode from the mode list, if other KMS properties affect the
>>> bandwidth etc. requirements of the mode. That mechanism is engineered
>>> for old userspace that doesn't actually handle link-status but does
>>> handle hotplug, so the hotplug triggers re-fetching the mode list and
>>> userspace maybe trying again with a better luck since the offending
>>> mode is gone. How to keep that working when introducing KMS properties
>>> forcing the cable format, I don't know.
>>>
>>> As long as the other affecting KMS properties are all "auto", the
>>> driver will probably exhaust all possibilities to make the mode work
>>> before signalling link-status failure and pruning the mode.
>>> Theoretically, as I have no idea what drivers actually do.  
>> Isn't that exactly how the "preferred color format" property works in
>> my patchset now?
> There was an argument that "preferred" with no guarantees is not
> useful enough. So I'm considering the force property instead.
> The problem is, "auto" is not the only possible value.
>
> When the value is not "auto", should link failure drop the mode or not?
> Userspace might change the value back to "auto" next time. If you
> dropped the mode, it would be gone. If you didn't drop the mode,
> userspace might be stuck picking the same non-working mode again and
> again if it doesn't know about the force mode property.
>
> You could argue that changing the value back to "auto" needs to reset
> the mode list, but that only gets us back to the "need to set
> properties before getting mode list".
>
> Maybe there needs to be an assumption that if "force color format" is
> not "auto", then link failure does not drop modes and userspace knows
> to handle this. Messy.
>
> I'm afraid I just don't know to give any clear answer. It's also
> possible that, as I'm not a kernel dev, I have some false assumptions
> here.
>
>
> Thanks,
> pq
> _______________________________________________
> amd-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Pekka Paalanen July 6, 2021, 7:09 a.m. UTC | #12

On Mon, 5 Jul 2021 17:49:42 +0200
Werner Sembach <[e-mail]> wrote:

> Am 01.07.21 um 15:24 schrieb Pekka Paalanen:
> > On Thu, 1 Jul 2021 14:50:13 +0200
> > Werner Sembach <[e-mail]> wrote:
> >  
> >> Am 01.07.21 um 10:07 schrieb Pekka Paalanen:
> >>  
> >>> On Wed, 30 Jun 2021 11:20:18 +0200
> >>> Werner Sembach <[e-mail]> wrote:
> >>>    
> >>>> Am 30.06.21 um 10:41 schrieb Pekka Paalanen:
> >>>>    
> >>>>> On Tue, 29 Jun 2021 13:39:18 +0200
> >>>>> Werner Sembach <[e-mail]> wrote:
> >>>>>      
> >>>>>> Am 29.06.21 um 13:17 schrieb Pekka Paalanen:      
> >>>>>>> On Tue, 29 Jun 2021 08:12:54 +0000
> >>>>>>> Simon Ser <[e-mail]> wrote:
> >>>>>>>         
> >>>>>>>> On Tuesday, June 22nd, 2021 at 09:15, Pekka Paalanen <[e-mail]> wrote:
> >>>>>>>>         
> >>>>>>>>> yes, I think this makes sense, even if it is a property that one can't
> >>>>>>>>> tell for sure what it does before hand.
> >>>>>>>>>
> >>>>>>>>> Using a pair of properties, preference and active, to ask for something
> >>>>>>>>> and then check what actually worked is good for reducing the
> >>>>>>>>> combinatorial explosion caused by needing to "atomic TEST_ONLY commit"
> >>>>>>>>> test different KMS configurations. Userspace has a better chance of
> >>>>>>>>> finding a configuration that is possible.
> >>>>>>>>>
> >>>>>>>>> OTOH, this has the problem than in UI one cannot tell the user in
> >>>>>>>>> advance which options are truly possible. Given that KMS properties are
> >>>>>>>>> rarely completely independent, and in this case known to depend on
> >>>>>>>>> several other KMS properties, I think it is good enough to know after
> >>>>>>>>> the fact.
> >>>>>>>>>
> >>>>>>>>> If a driver does not use what userspace prefers, there is no way to
> >>>>>>>>> understand why, or what else to change to make it happen. That problem
> >>>>>>>>> exists anyway, because TEST_ONLY commits do not give useful feedback
> >>>>>>>>> but only a yes/no.      
> >>>>>>>> By submitting incremental atomic reqs with TEST_ONLY (i.e. only changing one
> >>>>>>>> property at a time), user-space can discover which property makes the atomic
> >>>>>>>> commit fail.      
> >>>>>>> That works if the properties are independent of each other. Color
> >>>>>>> range, color format, bpc and more may all be interconnected,
> >>>>>>> allowing only certain combinations to work.
> >>>>>>>
> >>>>>>> If all these properties have "auto" setting too, then it would be
> >>>>>>> possible to probe each property individually, but that still does not
> >>>>>>> tell which combinations are valid.
> >>>>>>>
> >>>>>>> If you probe towards a certain configuration by setting the properties
> >>>>>>> one by one, then depending on the order you pick the properties, you
> >>>>>>> may come to a different conclusion on which property breaks the
> >>>>>>> configuration.      
> >>>>>> My mind crossed another point that must be considered: When plugin in
> >>>>>> a Monitor a list of possible Resolutions+Framerate combinations is
> >>>>>> created for xrandr and other userspace (I guess by atomic checks? but
> >>>>>> I don't know).      
> >>>>> Hi,
> >>>>>
> >>>>> I would not think so, but I hope to be corrected if I'm wrong.
> >>>>>
> >>>>> My belief is that the driver collects a list of modes from EDID, some
> >>>>> standard modes, and maybe some other hardcoded modes, and then
> >>>>> validates each entry against all the known limitations like vertical
> >>>>> and horizontal frequency limits, discarding modes that do not fit.
> >>>>>
> >>>>> Not all limitations are known during that phase, which is why KMS
> >>>>> property "link-status" exists. When userspace actually programs a mode
> >>>>> (not a TEST_ONLY commit), the link training may fail. The kernel prunes
> >>>>> the mode from the list and sets the link status property to signal
> >>>>> failure, and sends a hotplug uevent. Userspace needs to re-check the
> >>>>> mode list and try again.
> >>>>>
> >>>>> That is a generic escape hatch for when TEST_ONLY commit succeeds, but
> >>>>> in reality the hardware cannot do it, you just cannot know until you
> >>>>> actually try for real. It causes end user visible flicker if it happens
> >>>>> on an already running connector, but since it usually happens when
> >>>>> turning a connector on to begin with, there is no flicker to be seen,
> >>>>> just a small delay in finding a mode that works.
> >>>>>      
> >>>>>> During this drm
> >>>>>> properties are already considered, which is no problem atm because as
> >>>>>> far as i can tell there is currently no drm property that would make
> >>>>>> a certain Resolutions+Framerate combination unreachable that would be
> >>>>>> possible with everything on default.      
> >>>>> I would not expect KMS properties to be considered at all. It would
> >>>>> reject modes that are actually possible if the some KMS properties were
> >>>>> changed. So at least going forward, current KMS property values cannot
> >>>>> factor in.      
> >>>> At least the debugfs variable "force_yuv420_output" did change the 
> >>>> available modes here: 
> >>>> https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L5165 
> >>>> before my patch 
> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68eb3ae3c63708f823aeeb63bb15197c727bd9bf    
> >>> Hi,
> >>>
> >>> debugfs is not proper UAPI, so we can just ignore it. Display servers
> >>> cannot be expected to poke in debugfs. Debugfs is not even supposed to
> >>> exist in production systems, but I'm sure people use it for hacking
> >>> stuff. But that's all it is for: developer testing and hacking.    
> >> e.g. Ubuntu has it active by default, but only read (and writable) by root.  
> > Hi,
> >
> > that's normal, yes. Root can do damage anyway, and it's useful for
> > debugging. KMS clients OTOH often do not run as root.
> >  
> >>>    
> >>>> Forcing a color format via a DRM property in this function would 
> >>>> reintroduce the problem.    
> >>> The property would need to be defined differently because its presence
> >>> could otherwise break existing userspace. Well, I suppose it could
> >>> break existing userspace no matter what, so we would need the generic
> >>> "reset to sane defaults" mechanism first IMO.
> >>>
> >>> DRM has client caps for exposing video modes that old userspace might
> >>> not expect, to avoid breaking old userspace. Care needs to be taken
> >>> with all new UAPI, because if a kernel upgrade makes something wrong,
> >>> it's the kernel's fault no matter what userspace is doing, in principle.    
> >> Can you give me a link describing how I define this caps?  
> > I don't have any, but you can find all the existing ones by grepping
> > for DRM_CLIENT_CAP_.
> >
> > I'm not saying that we need it, but mentioning them as a possible
> > workaround if userspace breakage seems imminent or is proven.
> >  
> >>> Debugfs has no problem breaking userspace AFAIU, since it's not proper
> >>> UAPI.
> >>>    
> >>>> And I think i915 driver works similar in this regard.
> >>>>    
> >>>>>      
> >>>>>> However for example forcing YCbCr420 encoding would limit the
> >>>>>> available resolutions (my screen for example only supports YCbCr420
> >>>>>> on 4k@60 and @50Hz and on no other resolution or frequency (native is
> >>>>>> 2560x1440@144Hz).
> >>>>>>
> >>>>>> So would a "force color format" that does not get resetted on
> >>>>>> repluging/reenabling a monitor break the output, for example, of an
> >>>>>> not updated xrandr, unaware of this new property?      
> >>>>> Yes, not because the mode list would be missing the mode, but because
> >>>>> actually setting the mode would fail.      
> >>>> Well, like described above, I think the mode would actually be missing, 
> >>>> which is also an unexpected behavior from a user perspective.    
> >>> I think that is not how the property should work.
> >>>
> >>> If KMS properties would affect the list of modes, then userspace would
> >>> need to set the properties for real (TEST_ONLY cannot change anything)
> >>> and re-fetch the mode lists (maybe there is a hotplug event, maybe
> >>> not). That workflow just doesn't work.    
> >> The properties are set before the list is created in the first place.
> >> Because, in my example, the properties get set before the monitor is
> >> plugged in and the list can only be created as soon as the monitor is
> >> plugged in.  
> > That's just an accident, it's not what I mean.
> >
> > What I mean is, we cannot have the KMS properties affect the list of
> > modes, because then userspace that want to use specific values on those
> > properties would have to program those properties first, and then get
> > the list of modes. KMS UAPI does not work that way AFAIK.
> >
> > If the initial mode list is created on hotplug like you say, then the
> > initial list could already be missing some modes that would be valid if
> > some KMS properties had different values.  
> 
> Depends if the mode list is created by TEST_ONLY:

Hi,

I'm pretty sure it's not created by any kind of atomic test probing,
exactly because some properties might affect the result. Also because
of legacy: the mode lists predate atomic by far. It just doesn't make
sense to prune the mode list based on current arbitrary property values.

The function drm_helper_probe_single_connector_modes() looks relevant
to me. It has a big comment that seems to point towards more things to
look at.


Thanks,
pq

> - The force properties should return false on TEST_ONLY
> 
> - The force properties should not prevent the mode from showing up in the list
> 
> If the list is created by TEST_ONLY both things can't be fulfilled at the same time obviously.
> 
> I hope some can give more insights or has an idea how the properties could work best.
> 
> >  
> >>> A very interesting question is when should link-status failure not drop
> >>> the current mode from the mode list, if other KMS properties affect the
> >>> bandwidth etc. requirements of the mode. That mechanism is engineered
> >>> for old userspace that doesn't actually handle link-status but does
> >>> handle hotplug, so the hotplug triggers re-fetching the mode list and
> >>> userspace maybe trying again with a better luck since the offending
> >>> mode is gone. How to keep that working when introducing KMS properties
> >>> forcing the cable format, I don't know.
> >>>
> >>> As long as the other affecting KMS properties are all "auto", the
> >>> driver will probably exhaust all possibilities to make the mode work
> >>> before signalling link-status failure and pruning the mode.
> >>> Theoretically, as I have no idea what drivers actually do.    
> >> Isn't that exactly how the "preferred color format" property works in
> >> my patchset now?  
> > There was an argument that "preferred" with no guarantees is not
> > useful enough. So I'm considering the force property instead.
> > The problem is, "auto" is not the only possible value.
> >
> > When the value is not "auto", should link failure drop the mode or not?
> > Userspace might change the value back to "auto" next time. If you
> > dropped the mode, it would be gone. If you didn't drop the mode,
> > userspace might be stuck picking the same non-working mode again and
> > again if it doesn't know about the force mode property.
> >
> > You could argue that changing the value back to "auto" needs to reset
> > the mode list, but that only gets us back to the "need to set
> > properties before getting mode list".
> >
> > Maybe there needs to be an assumption that if "force color format" is
> > not "auto", then link failure does not drop modes and userspace knows
> > to handle this. Messy.
> >
> > I'm afraid I just don't know to give any clear answer. It's also
> > possible that, as I'm not a kernel dev, I have some false assumptions
> > here.
> >
> >
> > Thanks,
> > pq
> > _______________________________________________
> > amd-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Werner Sembach July 14, 2021, 5:59 p.m. UTC | #13

Am 06.07.21 um 09:09 schrieb Pekka Paalanen:
> On Mon, 5 Jul 2021 17:49:42 +0200
> Werner Sembach <[e-mail]> wrote:
>
>> Am 01.07.21 um 15:24 schrieb Pekka Paalanen:
>>> On Thu, 1 Jul 2021 14:50:13 +0200
>>> Werner Sembach <[e-mail]> wrote:
>>>   
>>>> Am 01.07.21 um 10:07 schrieb Pekka Paalanen:
>>>>   
>>>>> On Wed, 30 Jun 2021 11:20:18 +0200
>>>>> Werner Sembach <[e-mail]> wrote:
>>>>>     
>>>>>> Am 30.06.21 um 10:41 schrieb Pekka Paalanen:
>>>>>>     
>>>>>>> On Tue, 29 Jun 2021 13:39:18 +0200
>>>>>>> Werner Sembach <[e-mail]> wrote:
>>>>>>>       
>>>>>>>> Am 29.06.21 um 13:17 schrieb Pekka Paalanen:
>>>>>>>>> On Tue, 29 Jun 2021 08:12:54 +0000
>>>>>>>>> Simon Ser <[e-mail]> wrote:
>>>>>>>>>          
>>>>>>>>>> On Tuesday, June 22nd, 2021 at 09:15, Pekka Paalanen <[e-mail]> wrote:
>>>>>>>>>>          
>>>>>>>>>>> yes, I think this makes sense, even if it is a property that one can't
>>>>>>>>>>> tell for sure what it does before hand.
>>>>>>>>>>>
>>>>>>>>>>> Using a pair of properties, preference and active, to ask for something
>>>>>>>>>>> and then check what actually worked is good for reducing the
>>>>>>>>>>> combinatorial explosion caused by needing to "atomic TEST_ONLY commit"
>>>>>>>>>>> test different KMS configurations. Userspace has a better chance of
>>>>>>>>>>> finding a configuration that is possible.
>>>>>>>>>>>
>>>>>>>>>>> OTOH, this has the problem than in UI one cannot tell the user in
>>>>>>>>>>> advance which options are truly possible. Given that KMS properties are
>>>>>>>>>>> rarely completely independent, and in this case known to depend on
>>>>>>>>>>> several other KMS properties, I think it is good enough to know after
>>>>>>>>>>> the fact.
>>>>>>>>>>>
>>>>>>>>>>> If a driver does not use what userspace prefers, there is no way to
>>>>>>>>>>> understand why, or what else to change to make it happen. That problem
>>>>>>>>>>> exists anyway, because TEST_ONLY commits do not give useful feedback
>>>>>>>>>>> but only a yes/no.
>>>>>>>>>> By submitting incremental atomic reqs with TEST_ONLY (i.e. only changing one
>>>>>>>>>> property at a time), user-space can discover which property makes the atomic
>>>>>>>>>> commit fail.
>>>>>>>>> That works if the properties are independent of each other. Color
>>>>>>>>> range, color format, bpc and more may all be interconnected,
>>>>>>>>> allowing only certain combinations to work.
>>>>>>>>>
>>>>>>>>> If all these properties have "auto" setting too, then it would be
>>>>>>>>> possible to probe each property individually, but that still does not
>>>>>>>>> tell which combinations are valid.
>>>>>>>>>
>>>>>>>>> If you probe towards a certain configuration by setting the properties
>>>>>>>>> one by one, then depending on the order you pick the properties, you
>>>>>>>>> may come to a different conclusion on which property breaks the
>>>>>>>>> configuration.
>>>>>>>> My mind crossed another point that must be considered: When plugin in
>>>>>>>> a Monitor a list of possible Resolutions+Framerate combinations is
>>>>>>>> created for xrandr and other userspace (I guess by atomic checks? but
>>>>>>>> I don't know).
>>>>>>> Hi,
>>>>>>>
>>>>>>> I would not think so, but I hope to be corrected if I'm wrong.
>>>>>>>
>>>>>>> My belief is that the driver collects a list of modes from EDID, some
>>>>>>> standard modes, and maybe some other hardcoded modes, and then
>>>>>>> validates each entry against all the known limitations like vertical
>>>>>>> and horizontal frequency limits, discarding modes that do not fit.
>>>>>>>
>>>>>>> Not all limitations are known during that phase, which is why KMS
>>>>>>> property "link-status" exists. When userspace actually programs a mode
>>>>>>> (not a TEST_ONLY commit), the link training may fail. The kernel prunes
>>>>>>> the mode from the list and sets the link status property to signal
>>>>>>> failure, and sends a hotplug uevent. Userspace needs to re-check the
>>>>>>> mode list and try again.
>>>>>>>
>>>>>>> That is a generic escape hatch for when TEST_ONLY commit succeeds, but
>>>>>>> in reality the hardware cannot do it, you just cannot know until you
>>>>>>> actually try for real. It causes end user visible flicker if it happens
>>>>>>> on an already running connector, but since it usually happens when
>>>>>>> turning a connector on to begin with, there is no flicker to be seen,
>>>>>>> just a small delay in finding a mode that works.
>>>>>>>       
>>>>>>>> During this drm
>>>>>>>> properties are already considered, which is no problem atm because as
>>>>>>>> far as i can tell there is currently no drm property that would make
>>>>>>>> a certain Resolutions+Framerate combination unreachable that would be
>>>>>>>> possible with everything on default.
>>>>>>> I would not expect KMS properties to be considered at all. It would
>>>>>>> reject modes that are actually possible if the some KMS properties were
>>>>>>> changed. So at least going forward, current KMS property values cannot
>>>>>>> factor in.
>>>>>> At least the debugfs variable "force_yuv420_output" did change the
>>>>>> available modes here:
>>>>>> https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L5165
>>>>>> before my patch
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68eb3ae3c63708f823aeeb63bb15197c727bd9bf
>>>>> Hi,
>>>>>
>>>>> debugfs is not proper UAPI, so we can just ignore it. Display servers
>>>>> cannot be expected to poke in debugfs. Debugfs is not even supposed to
>>>>> exist in production systems, but I'm sure people use it for hacking
>>>>> stuff. But that's all it is for: developer testing and hacking.
>>>> e.g. Ubuntu has it active by default, but only read (and writable) by root.
>>> Hi,
>>>
>>> that's normal, yes. Root can do damage anyway, and it's useful for
>>> debugging. KMS clients OTOH often do not run as root.
>>>   
>>>>>     
>>>>>> Forcing a color format via a DRM property in this function would
>>>>>> reintroduce the problem.
>>>>> The property would need to be defined differently because its presence
>>>>> could otherwise break existing userspace. Well, I suppose it could
>>>>> break existing userspace no matter what, so we would need the generic
>>>>> "reset to sane defaults" mechanism first IMO.
>>>>>
>>>>> DRM has client caps for exposing video modes that old userspace might
>>>>> not expect, to avoid breaking old userspace. Care needs to be taken
>>>>> with all new UAPI, because if a kernel upgrade makes something wrong,
>>>>> it's the kernel's fault no matter what userspace is doing, in principle.
>>>> Can you give me a link describing how I define this caps?
>>> I don't have any, but you can find all the existing ones by grepping
>>> for DRM_CLIENT_CAP_.
>>>
>>> I'm not saying that we need it, but mentioning them as a possible
>>> workaround if userspace breakage seems imminent or is proven.
>>>   
>>>>> Debugfs has no problem breaking userspace AFAIU, since it's not proper
>>>>> UAPI.
>>>>>     
>>>>>> And I think i915 driver works similar in this regard.
>>>>>>     
>>>>>>>       
>>>>>>>> However for example forcing YCbCr420 encoding would limit the
>>>>>>>> available resolutions (my screen for example only supports YCbCr420
>>>>>>>> on 4k@60 and @50Hz and on no other resolution or frequency (native is
>>>>>>>> 2560x1440@144Hz).
>>>>>>>>
>>>>>>>> So would a "force color format" that does not get resetted on
>>>>>>>> repluging/reenabling a monitor break the output, for example, of an
>>>>>>>> not updated xrandr, unaware of this new property?
>>>>>>> Yes, not because the mode list would be missing the mode, but because
>>>>>>> actually setting the mode would fail.
>>>>>> Well, like described above, I think the mode would actually be missing,
>>>>>> which is also an unexpected behavior from a user perspective.
>>>>> I think that is not how the property should work.
>>>>>
>>>>> If KMS properties would affect the list of modes, then userspace would
>>>>> need to set the properties for real (TEST_ONLY cannot change anything)
>>>>> and re-fetch the mode lists (maybe there is a hotplug event, maybe
>>>>> not). That workflow just doesn't work.
>>>> The properties are set before the list is created in the first place.
>>>> Because, in my example, the properties get set before the monitor is
>>>> plugged in and the list can only be created as soon as the monitor is
>>>> plugged in.
>>> That's just an accident, it's not what I mean.
>>>
>>> What I mean is, we cannot have the KMS properties affect the list of
>>> modes, because then userspace that want to use specific values on those
>>> properties would have to program those properties first, and then get
>>> the list of modes. KMS UAPI does not work that way AFAIK.
>>>
>>> If the initial mode list is created on hotplug like you say, then the
>>> initial list could already be missing some modes that would be valid if
>>> some KMS properties had different values.
>> Depends if the mode list is created by TEST_ONLY:
> Hi,
>
> I'm pretty sure it's not created by any kind of atomic test probing,
> exactly because some properties might affect the result. Also because
> of legacy: the mode lists predate atomic by far. It just doesn't make
> sense to prune the mode list based on current arbitrary property values.

I implemented a buggy prototype for "force color format" and it doesn't 
make modes disappear as I feared.

It does successfully prevent setting a color format that is unsupported 
by the currently connected monitor.

However, if no monitor is connected and the property is set or the 
monitor is switched to another one that doesn't support currently the 
selected mode, the screen might stay black.

I don't think this should be the intended behavior, but the only 2 
sollutions i come up with violate the principle of not having a 
decentralized reset:

1. On monitor connect always reset the property to auto

     - alternatively: set on disconnect and don't allow to change 
without a connected monitor

2. On monitor connect, try the current setting and reset to auto if it 
fails (basically a one time "preferred color format" until the monitor 
capabilities are known).

>
> The function drm_helper_probe_single_connector_modes() looks relevant
> to me. It has a big comment that seems to point towards more things to
> look at.
>
>
> Thanks,
> pq
>
>> - The force properties should return false on TEST_ONLY
>>
>> - The force properties should not prevent the mode from showing up in the list
>>
>> If the list is created by TEST_ONLY both things can't be fulfilled at the same time obviously.
>>
>> I hope some can give more insights or has an idea how the properties could work best.
>>
>>>   
>>>>> A very interesting question is when should link-status failure not drop
>>>>> the current mode from the mode list, if other KMS properties affect the
>>>>> bandwidth etc. requirements of the mode. That mechanism is engineered
>>>>> for old userspace that doesn't actually handle link-status but does
>>>>> handle hotplug, so the hotplug triggers re-fetching the mode list and
>>>>> userspace maybe trying again with a better luck since the offending
>>>>> mode is gone. How to keep that working when introducing KMS properties
>>>>> forcing the cable format, I don't know.
>>>>>
>>>>> As long as the other affecting KMS properties are all "auto", the
>>>>> driver will probably exhaust all possibilities to make the mode work
>>>>> before signalling link-status failure and pruning the mode.
>>>>> Theoretically, as I have no idea what drivers actually do.
>>>> Isn't that exactly how the "preferred color format" property works in
>>>> my patchset now?
>>> There was an argument that "preferred" with no guarantees is not
>>> useful enough. So I'm considering the force property instead.
>>> The problem is, "auto" is not the only possible value.
>>>
>>> When the value is not "auto", should link failure drop the mode or not?
>>> Userspace might change the value back to "auto" next time. If you
>>> dropped the mode, it would be gone. If you didn't drop the mode,
>>> userspace might be stuck picking the same non-working mode again and
>>> again if it doesn't know about the force mode property.
>>>
>>> You could argue that changing the value back to "auto" needs to reset
>>> the mode list, but that only gets us back to the "need to set
>>> properties before getting mode list".
>>>
>>> Maybe there needs to be an assumption that if "force color format" is
>>> not "auto", then link failure does not drop modes and userspace knows
>>> to handle this. Messy.
>>>
>>> I'm afraid I just don't know to give any clear answer. It's also
>>> possible that, as I'm not a kernel dev, I have some false assumptions
>>> here.
>>>
>>>
>>> Thanks,
>>> pq
>>> _______________________________________________
>>> amd-gfx mailing list
>>> [e-mail]
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Patch

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index bc3487964fb5..90d62f305257 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -687,6 +687,10 @@  drm_atomic_helper_check_modeset(struct drm_device *dev,
 			if (old_connector_state->max_requested_bpc !=
 			    new_connector_state->max_requested_bpc)
 				new_crtc_state->connectors_changed = true;
+
+			if (old_connector_state->preferred_color_format !=
+			    new_connector_state->preferred_color_format)
+				new_crtc_state->connectors_changed = true;
 		}
 
 		if (funcs->atomic_check)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 438e9585b225..c536f5e22016 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -796,6 +796,8 @@  static int drm_atomic_connector_set_property(struct drm_connector *connector,
 						   fence_ptr);
 	} else if (property == connector->max_bpc_property) {
 		state->max_requested_bpc = val;
+	} else if (property == connector->preferred_color_format_property) {
+		state->preferred_color_format = val;
 	} else if (connector->funcs->atomic_set_property) {
 		return connector->funcs->atomic_set_property(connector,
 				state, property, val);
@@ -873,6 +875,8 @@  drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = 0;
 	} else if (property == connector->max_bpc_property) {
 		*val = state->max_requested_bpc;
+	} else if (property == connector->preferred_color_format_property) {
+		*val = state->preferred_color_format;
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
 				state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 818de58d972f..aea03dd02e33 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -889,6 +889,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_preferred_color_format_enum_list[] = {
+	{ 0, "auto" },
+	{ DRM_COLOR_FORMAT_RGB444, "rgb" },
+	{ DRM_COLOR_FORMAT_YCRCB444, "ycbcr444" },
+	{ DRM_COLOR_FORMAT_YCRCB422, "ycbcr422" },
+	{ DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" },
+};
+
 static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = {
 	{ 0, "unknown" },
 	{ DRM_COLOR_FORMAT_RGB444, "rgb" },
@@ -1219,11 +1227,19 @@  static const struct drm_prop_enum_list dp_colorspaces[] = {
  *	Drivers shall use drm_connector_attach_active_bpc_property() to install
  *	this property.
  *
+ * preferred color format:
+ *	This property is used by userspace to change the used color format. When
+ *	used the driver will use the selected format if valid for the hardware,
+ *	sink, and current resolution and refresh rate combination. Drivers to
+ *	use the function drm_connector_attach_preferred_color_format_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
+ *	connected monitor, and the "preferred color format". Drivers shall use
  *	drm_connector_attach_active_color_format_property() to install this
  *	property.
  *
@@ -2233,6 +2249,36 @@  void drm_connector_set_active_bpc_property(struct drm_connector *connector, int
 }
 EXPORT_SYMBOL(drm_connector_set_active_bpc_property);
 
+/**
+ * drm_connector_attach_preferred_color_format_property - attach "preferred color format" property
+ * @connector: connector to attach active color format property on.
+ *
+ * This is used to add support for selecting a color format on a connector.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_preferred_color_format_property(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_property *prop;
+
+	if (!connector->preferred_color_format_property) {
+		prop = drm_property_create_enum(dev, 0, "preferred color format",
+						drm_preferred_color_format_enum_list,
+						ARRAY_SIZE(drm_preferred_color_format_enum_list));
+		if (!prop)
+			return -ENOMEM;
+
+		connector->preferred_color_format_property = prop;
+		drm_object_attach_property(&connector->base, prop, 0);
+		connector->state->preferred_color_format = 0;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_preferred_color_format_property);
+
 /**
  * drm_connector_attach_active_color_format_property - attach "active color format" property
  * @connector: connector to attach active color format property on.
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 9fb7119b7a02..7b85407ba45c 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -799,6 +799,16 @@  struct drm_connector_state {
 	 */
 	u8 max_bpc;
 
+	/**
+	 * preferred_color_format: Property set by userspace to tell the GPU
+	 * driver which color format to use. It only gets applied if hardware,
+	 * meaning both the computer and the monitor, and the driver support the
+	 * given format at the current resolution and refresh rate. Userspace
+	 * can check for (un-)successful application via the active_color_format
+	 * property.
+	 */
+	u32 preferred_color_format;
+
 	/**
 	 * @hdr_output_metadata:
 	 * DRM blob property for HDR output metadata
@@ -1404,6 +1414,12 @@  struct drm_connector {
 	 */
 	struct drm_property *active_bpc_property;
 
+	/**
+	 * @preferred_color_format_property: Default connector property for the
+	 * preferred color format to be driven out of the connector.
+	 */
+	struct drm_property *preferred_color_format_property;
+
 	/**
 	 * @active_color_format_property: Default connector property for the
 	 * active color format to be driven out of the connector.
@@ -1740,6 +1756,7 @@  int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 					  int min, int max);
 int drm_connector_attach_active_bpc_property(struct drm_connector *connector, int min, int max);
 void drm_connector_set_active_bpc_property(struct drm_connector *connector, int active_bpc);
+int drm_connector_attach_preferred_color_format_property(struct drm_connector *connector);
 int drm_connector_attach_active_color_format_property(struct drm_connector *connector);
 void drm_connector_set_active_color_format_property(struct drm_connector *connector,
 						    u32 active_color_format);