On Wed, Nov 28, 2018 at 02:29:53PM +0000, Shankar, Uma wrote:
-----Original Message----- From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Brian Starkey Sent: Wednesday, November 28, 2018 5:27 PM To: Shankar, Uma uma.shankar@intel.com Cc: Syrjala, Ville ville.syrjala@intel.com; jonas@kwiboo.se; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; hansverk@cisco.com; nd nd@arm.com; Lankhorst, Maarten maarten.lankhorst@intel.com Subject: Re: [v4 0/3] Add Colorspace connector property interface
Hi,
On Tue, Nov 27, 2018 at 10:10:40PM +0530, Uma Shankar wrote:
This patch series creates a new connector property to program colorspace to sink devices. Modern sink devices support more than 1 type of colorspace like 601, 709, BT2020 etc. This helps to switch based on content type which is to be displayed. The decision lies with compositors as to in which scenarios, a particular colorspace will be picked.
This will be helpful mostly to switch to higher gamut colorspaces like BT2020 when the media content is encoded as BT2020. Thereby giving a good visual experience to users.
The expectation from userspace is that it should parse the EDID and get supported colorspaces. Use this property and switch to the one supported. Kernel will not give the supported colorspaces since this is panel dependent and our current property infrastructure is not supporting it.
So is the problem here that we've no way to change the supported enum values at runtime? Conceptually, do you think there's a problem with the kernel only exposing the colorspaces that the sink supports (if that were possible)? I'm wondering if changing the current property infrastructure is better than punting the job to userspace to decode the EDID.
Only problem which I see is that all the connector properties are created at connector initialization including the connector->edid property. The respective sync devices may not be attached also at that moment. There is an option to change the blob id's for case of edid when new sink is plugged in, but the fundamental structure remains same. I mean the enum which is used at creating the colorspace enum property can't be changed at runtime. It stays the same throughout the drivers life (this is of course based on my understanding :), correct me I am wrong). Hence changing it based on sink is not easy with the current design.
My understanding is the same. Did you look at what would be needed in order to change the enum property code to support changing the values at runtime? We do already have things which change (e.g. link status), just not enums.
Have tested this using xrandr by using below command: xrandr --output HDMI2 --set "Colorspace" "BT2020_rgb"
It would also be really great to get some more comprehensive documentation about how this property is meant to be used. Is the expectation that userspace does everything? i.e.:
- Userspace sets up CRTC DEGAMMA/CTM/GAMMA to convert to some sink colorspace
- Userspace sets this new property to let the sink know what it converted the CRTC output to.
Or in other words, I think this new property has zero impact on any pixel processing in the pipeline - it only sets the colorspace in the infoframe. That seems very valuable to write down explicitly.
Yes, this is what this property does. Userspace decides the blending and colorspace target for blending output from pipe. This helps to pass that info to sink device which was missing till now. I will update it and add documentation on the same to be clear wrt property expectation and purpose.
BTW, is there already a standard property for converting CRTC output to YCbCr? And does that interact with picking the YCC colorimetries with this property?
AFAIK, there is no dedicated property but the YUV modes are added as part of HDMI 2.0 development work from Shashank. User can set the mode and driver will do the conversion internally if the hardware supports it, else it may choose to drop the mode.
Thanks, I'll go take a look at that.
-Brian
Regards, Uma Shankar
Cheers, -Brian
v2: Addressed Ville and Maarten's review comments. Merged the 2nd and 3rd patch into one common logical patch.
v3: Removed Adobe references from enum definitions as per Ville, Hans Verkuil and Jonas Karlman suggestions. Changed default to an unset state where driver will assign the colorspace when not chosen by user, suggested by Ville and Maarten. Addressed other misc review comments from Maarten. Split the changes to have separate colorspace property for DP and HDMI.
v4: Addressed Chris and Ville's review comments, and created a common colorspace property for DP and HDMI, filtered the list based on the colorspaces supported by the respective protocol standard. Handled the default case more efficiently.
Uma Shankar (3): drm: Add HDMI colorspace property drm: Add DP colorspace property drm/i915: Attach colorspace property and enable modeset
drivers/gpu/drm/drm_atomic_uapi.c | 4 ++ drivers/gpu/drm/drm_connector.c | 92
++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_atomic.c | 1 + drivers/gpu/drm/i915/intel_connector.c | 8 +++ drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_hdmi.c | 18 +++++++ include/drm/drm_connector.h | 14 ++++++ include/uapi/drm/drm_mode.h | 33 ++++++++++++ 8 files changed, 171 insertions(+)
-- 1.9.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel