Re: The state of Quantization Range handling

2022-11-16 Thread Pekka Paalanen
On Tue, 15 Nov 2022 00:11:56 +0100
Sebastian Wick  wrote:

> There are still regular bug reports about monitors (sinks) and sources
> disagreeing about the quantization range of the pixel data. In
> particular sources sending full range data when the sink expects
> limited range. From a user space perspective, this is all hidden in
> the kernel. We send full range data to the kernel and then hope it
> does the right thing but as the bug reports show: some combinations of
> displays and drivers result in problems.
> 
> In general the whole handling of the quantization range on linux is
> not defined or documented at all. User space sends full range data
> because that's what seems to work most of the time but technically
> this is all undefined and user space can not fix those issues. Some
> compositors have resorted to giving users the option to choose the
> quantization range but this really should only be necessary for
> straight up broken hardware.
> 
> Quantization Range can be explicitly controlled by AVI InfoFrame or
> HDMI General Control Packets. This is the ideal case and when the
> source uses them there is not a lot that can go wrong. Not all
> displays support those explicit controls in which case the chosen
> video format (IT, CE, SD; details in CTA-861-H 5.1) influences which
> quantization range the sink expects.
> 
> This means that we have to expect that sometimes we have to send
> limited and sometimes full range content. The big question however
> that is not answered in the docs: who is responsible for making sure
> the data is in the correct range? Is it the kernel or user space?
> 
> If it's the kernel: does user space supply full range or limited range
> content? Each of those has a disadvantage. If we send full range
> content and the driver scales it down to limited range, we can't use
> the out-of-range bits to transfer information. If we send limited
> range content and the driver scales it up we lose information.
> 
> Either way, this must be documented. My suggestion is to say that the
> kernel always expects full range data as input and is responsible for
> scaling it to limited range data if the sink expects limited range
> data.

Hi Sebastian,

you are proposing the that driver/hardware will do either no range
conversion, or full-to-limited range conversion. Limited-to-full range
conversion would never be supported.

I still wonder if limited-to-full range conversion could be useful with
video content.

> Another problem is that some displays do not behave correctly. It must
> be possible to override the kernel when the user detects such a
> situation. This override then controls if the driver converts the full
> range data coming from the client or not (Default, Force Limited,
> Force Full). It does not try to control what range the sink expects.
> Let's call this the Quantization Range Override property which should
> be implemented by all drivers.

In other words, a CRTC "quantization range conversion" property with
values:
- auto, with the assumption that color pipeline always produces full-range
- identity
- full-to-limited
(- limited-to-full)

If this property was truly independent of the metadata being sent to
the sink, and of the framebuffer format, it would allow us to do four
ways: both full/limited framebuffer on both full/limited sink. It would
allow us to send sub-blacks and super-whites as well.

More precisely, framebuffers would always have *undefined* quantization
range. The configuration of the color pipeline then determines how that
data is manipulated into a video signal.

So I am advocating the same design as with color spaces: do not tell
KMS what your colorspaces are. Instead tell KMS what operations it
needs to do with the pixel data, and what metadata to send to the sink.

> All drivers should make sure their behavior is correct:
> 
> * check that drivers choose the correct default quantization range for
> the selected mode

Mode implying a quantization range is awkward, but maybe the kernel
established modes should just have a flag for it. Then userspace would
know. Unless the video mode system is extended to communicate
IT/CE/SD/VIC and whatnot to userspace, making the modes better defined.
Then userspace would know too.

> * whenever explicit control is available, use it and set the
> quantization range to full
> * make sure that the hardware converts from full range to limited
> range whenever the sink expects limited range
> * implement the Quantization Range Override property
> 
> I'm volunteering for the documentation, UAPI and maybe even the drm
> core parts if there is willingness to tackle the issue.

Is it a good idea to put even more automation/magic into configuring
the color pipeline and metadata for a sink, making them even more
intertwined?

I would prefer the opposite direction, making thing more explicit and
orthogonal.


Thanks,
pq

> Appendix A: Broadcast RGB property
> 
> A few drivers already implement the Broadcast RGB property

[PATCH] drm/atomic: add quirks for blind save/restore

2022-11-16 Thread Simon Ser
Two quirks to make blind atomic save/restore [1] work correctly:

- Mark the DPMS property as immutable for atomic clients, since
  atomic clients cannot change it.
- Allow user-space to set content protection to "enabled", interpret
  it as "desired".

[1]: https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3794

Signed-off-by: Simon Ser 
---

I don't have the motivation to write IGT tests for this.

 drivers/gpu/drm/drm_atomic_uapi.c | 5 +++--
 drivers/gpu/drm/drm_property.c| 7 +++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
b/drivers/gpu/drm/drm_atomic_uapi.c
index c06d0639d552..95363aac7f69 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -741,8 +741,9 @@ static int drm_atomic_connector_set_property(struct 
drm_connector *connector,
state->scaling_mode = val;
} else if (property == config->content_protection_property) {
if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
-   drm_dbg_kms(dev, "only drivers can set CP Enabled\n");
-   return -EINVAL;
+   /* Degrade ENABLED to DESIRED so that blind atomic
+* save/restore works as intended. */
+   val = DRM_MODE_CONTENT_PROTECTION_DESIRED;
}
state->content_protection = val;
} else if (property == config->hdcp_content_type_property) {
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index dfec479830e4..dde42986f8cb 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -474,7 +474,14 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
return -ENOENT;
 
strscpy_pad(out_resp->name, property->name, DRM_PROP_NAME_LEN);
+
out_resp->flags = property->flags;
+   if (file_priv->atomic && property == dev->mode_config.dpms_property) {
+   /* Quirk: indicate that the legacy DPMS property is not
+* writable from atomic user-space, so that blind atomic
+* save/restore works as intended. */
+   out_resp->flags |= DRM_MODE_PROP_IMMUTABLE;
+   }
 
value_count = property->num_values;
values_ptr = u64_to_user_ptr(out_resp->values_ptr);
-- 
2.38.1