[PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

This ended up being different enough from v2 to warrant a new patchset. Per
Michel's suggestions, there have been various optimizations and cleanups.
Here's what's changed:

* Cache DRM color management property IDs at pre-init,
    * instead of querying DRM each time we need to modify color properties.

* Remove drmmode_update_cm_props().
    * Update color properties in drmmode_output_get_property() instead.
    * This also makes old calls to update_cm_props redundant.

* Get rid of fake CRTCs.
    * Previously, we were allocating a fake CRTC to configure color props on
      outputs that don't have a CRTC.
    * Instead, rr_configure_and_change_cm_property() can be easily modified to
      accept NULL CRTCs.

* Drop patches to persist color properties across DPMS events.
    * Kernel driver should be patched instead:
      https://lists.freedesktop.org/archives/amd-gfx/2018-May/022744.html
    * Color props including legacy gamma now persist across crtc dpms.
    * Non-legacy props now persist across output dpms and hotplug, as long
      as the same CRTC remains attached to that output.

And some smaller improvements:

* Change CTM to be 32-bit format instead of 16-bit.
    * This requires clients to ensure that each 32-bit element is padded to be
      long-sized, since libXrandr parses 32-bit format as long-typed.

* Optimized color management init during CRTC init.
    * Query DRM once for the list of properties, instead of twice.


The test/demo application is also updated to reflect that 32-bit CTM change:
https://cgit.freedesktop.org/~hwentland/color-demo-app

Leo (Sunpeng) Li (7):
  Cache color property IDs during pre-init
  Initialize color properties on CRTC during CRTC init
  Configure color properties when creating output resources
  Update color properties on output_get_property
  Enable setting of color properties via RandR
  Compose non-legacy with legacy regamma LUT
  Also compose LUT when setting legacy gamma

 src/drmmode_display.c | 739 +++++++++++++++++++++++++++++++++++++++++++++++++-
 src/drmmode_display.h |  27 ++
 2 files changed, 763 insertions(+), 3 deletions(-)

-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 1/7] Cache color property IDs during pre-init

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

DRM creates property types with unique IDs during kernel driver init.
Cache the color property IDs on DDX init for use later, when we need
to modify these properties.

Since these IDs are the same regardless of the CRTC, they can be
cached within the private drmmode_rec object. We can also use any color-
management-enabled CRTC to initially fetch these IDs.

Also introduce an enumeration of possible color management properties,
to provide a easy and unified way of referring to them.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 src/drmmode_display.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/drmmode_display.h | 19 +++++++++++
 2 files changed, 109 insertions(+)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 8a1a201..36b22ad 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -746,6 +746,28 @@ drmmode_crtc_scanout_update(xf86CrtcPtr crtc, DisplayModePtr mode,
 	}
 }
 
+static char *cm_prop_names[] = {
+	"GAMMA_LUT_SIZE",
+	"DEGAMMA_LUT_SIZE",
+	"GAMMA_LUT",
+	"CTM",
+	"DEGAMMA_LUT",
+};
+
+/**
+ * Return the enum of the color management property with the given name.
+ */
+static enum drmmode_cm_prop get_cm_enum_from_str(const char *prop_name)
+{
+	enum drmmode_cm_prop ret;
+
+	for (ret = 0; ret < CM_NUM_PROPS; ret++) {
+		if (!strcmp(prop_name, cm_prop_names[ret]))
+			return ret;
+	}
+	return CM_INVALID_PROP;
+}
+
 static void
 drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t *red, uint16_t *green,
 			  uint16_t *blue, int size)
@@ -2413,6 +2435,72 @@ drmmode_page_flip_target_relative(AMDGPUEntPtr pAMDGPUEnt,
 				 drm_queue_seq);
 }
 
+/**
+ * Cache DRM color management property type IDs. This should be done during DDX
+ * initialization, as these IDs do not change. They will be used later to
+ * modify color management via DRM, or to determine if there's any CRTC(s) that
+ * support color management.
+ *
+ * If the cache ID's are all 0 after calling this function, then color
+ * management is not supported by any CRTC. For short, checking if the gamma
+ * LUT size property ID == 0 is sufficient.
+ *
+ * @drm_fd: DRM file descriptor
+ * @drmmode: drmmode object, where the cached IDs are stored
+ * @mode_res: The DRM mode resource containing the CRTC ids
+ */
+static void drmmode_cache_cm_prop_ids(int drm_fd,
+				      drmmode_ptr drmmode,
+				      drmModeResPtr mode_res)
+{
+	drmModeObjectPropertiesPtr drm_props;
+	drmModePropertyPtr drm_prop;
+	enum drmmode_cm_prop cm_prop;
+	uint32_t cm_enabled = 0;
+	uint32_t cm_all_enabled = (1 << CM_NUM_PROPS) - 1;
+	int i, j;
+
+	memset(drmmode->cm_prop_ids, 0, sizeof(drmmode->cm_prop_ids));
+
+	/* If even one CRTC supports cm, then we cache the properties.
+	 * Otherwise, set them all to 0.
+	 */
+	for (i = 0; i < drmmode->count_crtcs; i++) {
+
+		drm_props = drmModeObjectGetProperties(drm_fd,
+						       mode_res->crtcs[i],
+						       DRM_MODE_OBJECT_CRTC);
+		if (!drm_props)
+			continue;
+
+		for (j = 0; j < drm_props->count_props; j++) {
+			drm_prop = drmModeGetProperty(drm_fd,
+						      drm_props->props[j]);
+			if (!drm_prop)
+				/* No guarantee that the property we failed to
+				 * get isn't a cm property. Skip to next CRTC.
+				 */
+				break;
+
+			cm_prop = get_cm_enum_from_str(drm_prop->name);
+			if (cm_prop == CM_INVALID_PROP)
+				continue;
+
+			drmmode->cm_prop_ids[cm_prop] = drm_props->props[j];
+			cm_enabled |= 1 << cm_prop;
+
+			drmModeFreeProperty(drm_prop);
+		}
+		drmModeFreeObjectProperties(drm_props);
+
+		if (cm_enabled == cm_all_enabled)
+			break;
+
+		cm_enabled = 0;
+		memset(drmmode->cm_prop_ids, 0, sizeof(drmmode->cm_prop_ids));
+	}
+}
+
 Bool drmmode_pre_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, int cpp)
 {
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(pScrn);
@@ -2459,6 +2547,8 @@ Bool drmmode_pre_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, int cpp)
 	if (pScrn->depth == 30)
 		info->drmmode_crtc_funcs.gamma_set = NULL;
 
+	drmmode_cache_cm_prop_ids(pAMDGPUEnt->fd, drmmode, mode_res);
+
 	for (i = 0; i < mode_res->count_crtcs; i++)
 		if (!xf86IsEntityShared(pScrn->entityList[0]) ||
 		    (crtcs_needed && !(pAMDGPUEnt->assigned_crtcs & (1 << i))))
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index 25ae9f8..bbb5423 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -36,6 +36,22 @@
 #include "amdgpu_probe.h"
 #include "amdgpu.h"
 
+/*
+ * Enum of non-legacy color management properties, according to DRM. The values
+ * should be incremental, starting with 0, as defined by C99 (with the
+ * exception of the INVALID member). There are logics that depend on this fact
+ * (i.e. bitmasks).
+ */
+enum drmmode_cm_prop {
+	CM_GAMMA_LUT_SIZE,
+	CM_DEGAMMA_LUT_SIZE,
+	CM_GAMMA_LUT,
+	CM_CTM,
+	CM_DEGAMMA_LUT,
+	CM_NUM_PROPS,
+	CM_INVALID_PROP = -1,
+};
+
 typedef struct {
 	ScrnInfoPtr scrn;
 #ifdef HAVE_LIBUDEV
@@ -49,6 +65,9 @@ typedef struct {
 
 	Bool dri2_flipping;
 	Bool present_flipping;
+
+	/* Cache for DRM property type IDs for CRTC color management */
+	uint32_t cm_prop_ids[CM_NUM_PROPS];
 } drmmode_rec, *drmmode_ptr;
 
 typedef struct {
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 2/7] Initialize color properties on CRTC during CRTC init

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

And destroy them on the CRTC destroy hook.

When initializing color management properties on the private
drmmode_crtc, we want to:

1. Obtain its degamma and regamma LUT sizes
2. Default its color transform matrix (CTM) to identity
3. Program hardware with default color management values (SRGB for
   de/regamma, identity for CTM)

It's possible that cm initialization fails due to memory error or DRM
error. In which case, DDX support for color management will be disabled
on this CRTC.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 src/drmmode_display.c | 189 +++++++++++++++++++++++++++++++++++++++++++++++++-
 src/drmmode_display.h |   8 +++
 2 files changed, 196 insertions(+), 1 deletion(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 36b22ad..de09361 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -768,6 +768,88 @@ static enum drmmode_cm_prop get_cm_enum_from_str(const char *prop_name)
 	return CM_INVALID_PROP;
 }
 
+/**
+ * Return TRUE if the given CRTC supports non-legacy color management. False
+ * otherwise.
+ */
+static Bool drmmode_crtc_cm_enabled(drmmode_crtc_private_ptr drmmode_crtc)
+{
+	return drmmode_crtc->gamma_lut_size > 0 &&
+	       drmmode_crtc->degamma_lut_size > 0;
+}
+
+/**
+ * Push staged color management properties on the CRTC to DRM.
+ *
+ * @crtc: The CRTC containing staged properties
+ * @cm_prop_index: The color property to push
+ *
+ * Return 0 on success, X-defined error codes on failure.
+ */
+static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
+				     enum drmmode_cm_prop cm_prop_index)
+{
+	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+	uint32_t created_blob_id = 0;
+	uint32_t drm_prop_id;
+	size_t expected_bytes = 0;
+	void *blob_data = NULL;
+	int ret;
+
+	if (!drmmode_crtc_cm_enabled(drmmode_crtc))
+		return BadName;
+
+	if (cm_prop_index == CM_GAMMA_LUT) {
+		/* Calculate the expected size of value in bytes */
+		expected_bytes = sizeof(struct drm_color_lut) *
+					drmmode_crtc->gamma_lut_size;
+		blob_data = drmmode_crtc->gamma_lut;
+	} else if (cm_prop_index == CM_DEGAMMA_LUT) {
+		expected_bytes = sizeof(struct drm_color_lut) *
+					drmmode_crtc->degamma_lut_size;
+		blob_data = drmmode_crtc->degamma_lut;
+	} else if (cm_prop_index == CM_CTM) {
+		expected_bytes = sizeof(struct drm_color_ctm);
+		blob_data = drmmode_crtc->ctm;
+	} else
+		return BadName;
+
+	if (blob_data) {
+		ret = drmModeCreatePropertyBlob(pAMDGPUEnt->fd,
+						blob_data, expected_bytes,
+						&created_blob_id);
+		if (ret) {
+			xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
+				   "Creating DRM blob failed with errno %d\n",
+				   ret);
+			return BadRequest;
+		}
+	}
+
+	drm_prop_id = drmmode_crtc->drmmode->cm_prop_ids[cm_prop_index];
+	ret = drmModeObjectSetProperty(pAMDGPUEnt->fd,
+				       drmmode_crtc->mode_crtc->crtc_id,
+				       DRM_MODE_OBJECT_CRTC,
+				       drm_prop_id,
+				       (uint64_t)created_blob_id);
+
+	/* If successful, kernel will have a reference already. Safe to destroy
+	 * the blob either way.
+	 */
+	if (blob_data)
+		drmModeDestroyPropertyBlob(pAMDGPUEnt->fd, created_blob_id);
+
+	if (ret) {
+		xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
+			   "Setting DRM property blob failed with errno %d\n",
+			   ret);
+		return BadRequest;
+	}
+
+	return Success;
+}
+
 static void
 drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t *red, uint16_t *green,
 			  uint16_t *blue, int size)
@@ -1314,6 +1396,22 @@ static Bool drmmode_set_scanout_pixmap(xf86CrtcPtr crtc, PixmapPtr ppix)
 	return TRUE;
 }
 
+static void drmmode_crtc_destroy(xf86CrtcPtr crtc)
+{
+	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+
+	drmModeFreeCrtc(drmmode_crtc->mode_crtc);
+
+	/* Free LUTs and CTM */
+	free(drmmode_crtc->gamma_lut);
+	free(drmmode_crtc->degamma_lut);
+	free(drmmode_crtc->ctm);
+
+	free(drmmode_crtc);
+	crtc->driver_private = NULL;
+}
+
+
 static xf86CrtcFuncsRec drmmode_crtc_funcs = {
 	.dpms = drmmode_crtc_dpms,
 	.set_mode_major = drmmode_set_mode_major,
@@ -1330,7 +1428,7 @@ static xf86CrtcFuncsRec drmmode_crtc_funcs = {
 	.shadow_create = drmmode_crtc_shadow_create,
 	.shadow_allocate = drmmode_crtc_shadow_allocate,
 	.shadow_destroy = drmmode_crtc_shadow_destroy,
-	.destroy = NULL,	/* XXX */
+	.destroy = drmmode_crtc_destroy,
 	.set_scanout_pixmap = drmmode_set_scanout_pixmap,
 };
 
@@ -1354,6 +1452,93 @@ void drmmode_crtc_hw_id(xf86CrtcPtr crtc)
 		drmmode_crtc->hw_id = -1;
 }
 
+/**
+ * Initialize color management properties for the given CRTC by fetching its
+ * gamma/degamma LUT sizes, then programming default gamma/degamma LUTs and
+ * CTM.
+ *
+ * If the CRTC does not support color management, or if errors occur during
+ * initialization, all color properties on the driver-private CRTC will left
+ * as NULL.
+ *
+ * @drm_fd: DRM file descriptor
+ * @crtc: CRTC to initialize color management on.
+ */
+static void drmmode_crtc_cm_init(int drm_fd, xf86CrtcPtr crtc)
+{
+	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+	drmModeObjectPropertiesPtr drm_props;
+	drmModePropertyPtr drm_prop;
+	enum drmmode_cm_prop prop_id;
+	int i;
+
+	drm_props = drmModeObjectGetProperties(drm_fd,
+					       drmmode_crtc->mode_crtc->crtc_id,
+					       DRM_MODE_OBJECT_CRTC);
+	if (!drm_props)
+		goto err_allocs;
+
+	/* Fetch degamma/gamma LUT sizes */
+	for (i = 0; i < drm_props->count_props; i++) {
+		drm_prop = drmModeGetProperty(drm_fd, drm_props->props[i]);
+		if (!drm_prop){
+			drmModeFreeObjectProperties(drm_props);
+			goto err_allocs;
+		}
+
+		prop_id = get_cm_enum_from_str(drm_prop->name);
+
+		if (prop_id == CM_GAMMA_LUT_SIZE)
+			drmmode_crtc->gamma_lut_size = drm_props->prop_values[i];
+		else if (prop_id == CM_DEGAMMA_LUT_SIZE)
+			drmmode_crtc->degamma_lut_size = drm_props->prop_values[i];
+
+		drmModeFreeProperty(drm_prop);
+	}
+	drmModeFreeObjectProperties(drm_props);
+
+	if (!drmmode_crtc_cm_enabled(drmmode_crtc)) {
+		xf86DrvMsg(crtc->scrn->scrnIndex, X_INFO,
+			   "CRTC%d does not support non-legacy color management.\n",
+			   drmmode_get_crtc_id(crtc));
+		drmmode_crtc->degamma_lut_size = 0;
+		drmmode_crtc->gamma_lut_size = 0;
+		return;
+	}
+	xf86DrvMsg(crtc->scrn->scrnIndex, X_INFO,
+		   "CRTC%d supports non-legacy color management.\n",
+		   drmmode_get_crtc_id(crtc));
+
+	/* Init CTM to identity. Values are in S31.32 fixed-point format */
+	drmmode_crtc->ctm = calloc(1, sizeof(*drmmode_crtc->ctm));
+	if (drmmode_crtc->ctm == NULL)
+		goto err_allocs;
+
+	drmmode_crtc->ctm->matrix[0] = drmmode_crtc->ctm->matrix[4] =
+		drmmode_crtc->ctm->matrix[8] = (uint64_t)1 << 32;
+
+	/* Push properties to reset properties currently in hardware */
+	for (i = 0; i < CM_NUM_PROPS; i++) {
+		if (i == CM_DEGAMMA_LUT_SIZE || i == CM_GAMMA_LUT_SIZE)
+			continue;
+
+		if (drmmode_crtc_push_cm_prop(crtc, i))
+			xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
+				   "Failed to initialize color management "
+				   "property %s on CRTC%d. Property value may "
+				   "not reflect actual hardware state.\n",
+				   cm_prop_names[i],
+				   drmmode_get_crtc_id(crtc));
+	}
+
+	return;
+
+err_allocs:
+	xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
+		   "Memory error initializing cm properties for CRTC%d",
+		   drmmode_get_crtc_id(crtc));
+}
+
 static unsigned int
 drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, drmModeResPtr mode_res, int num)
 {
@@ -1374,6 +1559,8 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, drmModeResPtr mode_res
 	crtc->driver_private = drmmode_crtc;
 	drmmode_crtc_hw_id(crtc);
 
+	drmmode_crtc_cm_init(pAMDGPUEnt->fd, crtc);
+
 	/* Mark num'th crtc as in use on this device. */
 	pAMDGPUEnt->assigned_crtcs |= (1 << num);
 	xf86DrvMsgVerb(pScrn->scrnIndex, X_INFO, AMDGPU_LOGLEVEL_DEBUG,
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index bbb5423..1293249 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -128,6 +128,14 @@ typedef struct {
 	unsigned present_vblank_msc;
 	Bool present_flip_expected;
 #endif
+
+	uint32_t gamma_lut_size;
+	struct drm_color_lut *gamma_lut;
+
+	uint32_t degamma_lut_size;
+	struct drm_color_lut *degamma_lut;
+
+	struct drm_color_ctm *ctm;
 } drmmode_crtc_private_rec, *drmmode_crtc_private_ptr;
 
 typedef struct {
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 3/7] Configure color properties when creating output resources

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

List color management properties on outputs if there's at least one
CRTC that supports color management. Otherwise, don't list them at
all.

If there's no CRTC attached to the output, and there exists a CRTC
that supports color management, then list "disabled" properties
(immutable and NULL-valued).

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 src/drmmode_display.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 148 insertions(+)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index de09361..32ac441 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -769,6 +769,15 @@ static enum drmmode_cm_prop get_cm_enum_from_str(const char *prop_name)
 }
 
 /**
+ * Return TRUE if there's at least one CRTC that supports non-legacy color
+ * management. False otherwise.
+ */
+static Bool drmmode_cm_enabled(drmmode_ptr drmmode)
+{
+	return drmmode->cm_prop_ids[CM_GAMMA_LUT_SIZE] != 0;
+}
+
+/**
  * Return TRUE if the given CRTC supports non-legacy color management. False
  * otherwise.
  */
@@ -779,6 +788,123 @@ static Bool drmmode_crtc_cm_enabled(drmmode_crtc_private_ptr drmmode_crtc)
 }
 
 /**
+ * Configure and change a color property on a CRTC, through RandR. Only the
+ * specified output will be affected, even if the CRTC is attached to multiple
+ * outputs. Note that changes will be non-pending: the changes won't be pushed
+ * to kernel driver.
+ *
+ * @output: RandR output to set the property on.
+ * @crtc: The driver-private CRTC object containing the color properties.
+ *        If this is NULL, "disabled" values of 0 will be used.
+ * @cm_prop_index: Color management property to configure and change.
+ *
+ * Return 0 on success, X-defined error code otherwise.
+ */
+static int rr_configure_and_change_cm_property(xf86OutputPtr output,
+					       drmmode_crtc_private_ptr crtc,
+					       enum drmmode_cm_prop cm_prop_index)
+{
+	Bool need_configure = TRUE;
+	unsigned long length = 0;
+	void *data = NULL;
+	int format = 0;
+	uint32_t zero = 0;
+	INT32 range[2];
+	Atom atom;
+	int err;
+
+	if (cm_prop_index == CM_INVALID_PROP)
+		return BadName;
+
+	atom = MakeAtom(cm_prop_names[cm_prop_index],
+			strlen(cm_prop_names[cm_prop_index]),
+			TRUE);
+	if (!atom)
+		return BadAlloc;
+
+	if (cm_prop_index == CM_GAMMA_LUT_SIZE) {
+		format = 32;
+		length = 1;
+		data = crtc ? &crtc->gamma_lut_size : &zero;
+		range[0] = 0;
+		range[1] = -1;
+
+	} else if (cm_prop_index == CM_DEGAMMA_LUT_SIZE) {
+		format = 32;
+		length = 1;
+		data = crtc ? &crtc->degamma_lut_size : &zero;
+		range[0] = 0;
+		range[1] = -1;
+
+	} else if (cm_prop_index == CM_GAMMA_LUT) {
+		format = 16;
+		range[0] = 0;
+		range[1] = (1 << 16) - 1; // Max 16 bit unsigned int.
+		if (crtc && crtc->gamma_lut) {
+			/* Convert from 8bit size to 16bit size */
+			length = sizeof(*crtc->gamma_lut) >> 1;
+			length *= crtc->gamma_lut_size;
+			data = crtc->gamma_lut;
+		} else {
+			length = 1;
+			data = &zero;
+		}
+	} else if (cm_prop_index == CM_DEGAMMA_LUT) {
+		format = 16;
+		range[0] = 0;
+		range[1] = (1 << 16) - 1; // Max 16 bit unsigned int.
+		if (crtc && crtc->degamma_lut) {
+			/* Convert from 8bit size to 16bit size */
+			length = sizeof(*crtc->degamma_lut) >> 1;
+			length *= crtc->degamma_lut_size;
+			data = crtc->degamma_lut;
+		} else {
+			length = 1;
+			data = &zero;
+		}
+	} else {
+		/* CTM is fixed-point S31.32 format. */
+		format = 32;
+		need_configure = FALSE;
+		if (crtc && crtc->ctm) {
+			/* Convert from 8bit size to 32bit size */
+			length = sizeof(*crtc->ctm) >> 2;
+			data = crtc->ctm;
+		} else {
+			length = 1;
+			data = &zero;
+		}
+	}
+
+	if (need_configure) {
+		err = RRConfigureOutputProperty(output->randr_output, atom,
+						FALSE, TRUE, FALSE, 2, range);
+		if (err) {
+			xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+				   "Configuring color management property %s failed with %d\n",
+				   cm_prop_names[cm_prop_index], err);
+			return err;
+		}
+	}
+
+	/* Always issue a non-pending change. We'll push cm properties
+	 * ourselves.
+	 */
+	err = RRChangeOutputProperty(output->randr_output, atom,
+				     XA_INTEGER, format,
+				     PropModeReplace,
+				     length, data, FALSE, FALSE);
+	if (err) {
+		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+			   "Changing color management property %s failed with %d\n",
+			   cm_prop_names[cm_prop_index], err);
+		return err;
+	}
+
+	return 0;
+}
+
+/**
  * Push staged color management properties on the CRTC to DRM.
  *
  * @crtc: The CRTC containing staged properties
@@ -1822,6 +1948,7 @@ static void drmmode_output_create_resources(xf86OutputPtr output)
 {
 	AMDGPUInfoPtr info = AMDGPUPTR(output->scrn);
 	drmmode_output_private_ptr drmmode_output = output->driver_private;
+	drmmode_crtc_private_ptr drmmode_crtc;
 	drmModeConnectorPtr mode_output = drmmode_output->mode_output;
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(output->scrn);
 	drmModePropertyPtr drmmode_prop, tearfree_prop;
@@ -1945,6 +2072,27 @@ static void drmmode_output_create_resources(xf86OutputPtr output)
 			}
 		}
 	}
+
+	/*
+	 * Note that we usually check for cm support per CRTC. However, since
+	 * one may not be attached to the output at the moment, we will check
+	 * if there's at least one CRTC that supports cm.
+	 *
+	 * This means that if no CRTCs have cm support, then we won't list the
+	 * cm properties on any of the outputs.
+	 *
+	 * But if even one CRTC has cm support, then we will list the
+	 * properties on all outputs. Outputs with no CRTC, or with one that
+	 * doesn't support cm, will then have "disabled" cm properties (null-
+	 * valued, and cannot be modified).
+	 */
+	if (!drmmode_cm_enabled(drmmode_output->drmmode))
+		return;
+
+	drmmode_crtc = output->crtc ? output->crtc->driver_private : NULL;
+
+	for (i = 0; i < CM_NUM_PROPS; i++)
+		rr_configure_and_change_cm_property(output, drmmode_crtc, i);
 }
 
 static void
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 4/7] Update color properties on output_get_property

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

Notify RandR of any updated color properties on the output's CRTC when
its get_property() hook is called.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 src/drmmode_display.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 32ac441..350dc3d 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -2184,6 +2184,30 @@ drmmode_output_set_property(xf86OutputPtr output, Atom property,
 
 static Bool drmmode_output_get_property(xf86OutputPtr output, Atom property)
 {
+	drmmode_crtc_private_ptr drmmode_crtc;
+	enum drmmode_cm_prop cm_prop_id;
+	int ret;
+
+	/* First, see if it's a cm property */
+	cm_prop_id = get_cm_enum_from_str(NameForAtom(property));
+	if (output->crtc && cm_prop_id != CM_INVALID_PROP) {
+		drmmode_crtc = output->crtc->driver_private;
+
+		if (!drmmode_crtc_cm_enabled(drmmode_crtc))
+			return TRUE;
+
+		ret = rr_configure_and_change_cm_property(output, drmmode_crtc,
+							  cm_prop_id);
+		if (ret) {
+			xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+				   "Error getting color property: %d\n",
+				   ret);
+			return FALSE;
+		}
+		return TRUE;
+	}
+
+	/* Otherwise, must be an output property. */
 	return TRUE;
 }
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 5/7] Enable setting of color properties via RandR

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

Setting a color property involves:
1. Staging the property onto the driver-private CRTC object
2. Pushing the staged property into kernel DRM, for HW programming

Add a function to do the staging, and execute the above steps in
output_property_set.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 src/drmmode_display.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 350dc3d..6e5ae74 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -905,6 +905,89 @@ static int rr_configure_and_change_cm_property(xf86OutputPtr output,
 }
 
 /**
+* Stage a color management property. This parses the property value, according
+* to the cm property type, then stores it within the driver-private CRTC
+* object.
+*
+* @crtc: The CRTC to stage the new color management properties in
+* @cm_prop_index: The color property to stage
+* @value: The RandR property value to stage
+*
+* Return 0 on success, X-defined error code on failure.
+*/
+static int drmmode_crtc_stage_cm_prop(xf86CrtcPtr crtc,
+				      enum drmmode_cm_prop cm_prop_index,
+				      RRPropertyValuePtr value)
+{
+	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+	size_t expected_bytes = 0;
+	void **blob_data = NULL;
+	Bool use_default = FALSE;
+
+	/* Early return if CRTC has no cm support */
+	if (!drmmode_crtc_cm_enabled(drmmode_crtc))
+		return BadName;
+
+	/* Update properties on the driver-private CRTC */
+	if (cm_prop_index == CM_GAMMA_LUT) {
+		/* Calculate the expected size of value in bytes */
+		expected_bytes = sizeof(struct drm_color_lut) *
+					drmmode_crtc->gamma_lut_size;
+
+		/* For gamma and degamma, we allow a default SRGB curve to be
+		 * set via setting a single element
+		 *
+		 * Otherwise, value size is in terms of the value format.
+		 * Ensure it's also in bytes (<< 1) before comparing with the
+		 * expected bytes.
+		 */
+		if (value->size == 1)
+			use_default = TRUE;
+		else if (value->type != XA_INTEGER || value->format != 16 ||
+			 (size_t)(value->size << 1) != expected_bytes)
+			return BadLength;
+
+		blob_data = (void**)&drmmode_crtc->gamma_lut;
+
+	} else if (cm_prop_index == CM_DEGAMMA_LUT) {
+		expected_bytes = sizeof(struct drm_color_lut) *
+					drmmode_crtc->degamma_lut_size;
+
+		if (value->size == 1)
+			use_default = TRUE;
+		else if (value->type != XA_INTEGER || value->format != 16 ||
+			 (size_t)(value->size << 1) != expected_bytes)
+			return BadLength;
+
+		blob_data = (void**)&drmmode_crtc->degamma_lut;
+
+	} else if (cm_prop_index == CM_CTM) {
+		expected_bytes = sizeof(struct drm_color_ctm);
+
+		if (value->size == 1)
+			use_default = TRUE;
+		if (value->type != XA_INTEGER || value->format != 32 ||
+		    (size_t)(value->size << 2) != expected_bytes)
+			return BadLength;
+
+		blob_data = (void**)&drmmode_crtc->ctm;
+
+	} else
+		return BadName;
+
+	free(*blob_data);
+	if (!use_default) {
+		*blob_data = malloc(expected_bytes);
+		if (!*blob_data)
+			return BadAlloc;
+		memcpy(*blob_data, value->data, expected_bytes);
+	} else
+		*blob_data = NULL;
+
+	return Success;
+}
+
+/**
  * Push staged color management properties on the CRTC to DRM.
  *
  * @crtc: The CRTC containing staged properties
@@ -2126,8 +2209,22 @@ drmmode_output_set_property(xf86OutputPtr output, Atom property,
 {
 	drmmode_output_private_ptr drmmode_output = output->driver_private;
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(output->scrn);
+	enum drmmode_cm_prop cm_prop_index;
 	int i;
 
+	cm_prop_index = get_cm_enum_from_str(NameForAtom(property));
+	if (cm_prop_index >= 0 && cm_prop_index != CM_GAMMA_LUT_SIZE &&
+	    cm_prop_index != CM_DEGAMMA_LUT_SIZE) {
+		if (!output->crtc)
+			return FALSE;
+		if (drmmode_crtc_stage_cm_prop(output->crtc, cm_prop_index,
+					       value))
+			return FALSE;
+		if (drmmode_crtc_push_cm_prop(output->crtc, cm_prop_index))
+			return FALSE;
+		return TRUE;
+	}
+
 	for (i = 0; i < drmmode_output->num_props; i++) {
 		drmmode_prop_ptr p = &drmmode_output->props[i];
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 6/7] Compose non-legacy with legacy regamma LUT

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

Frequently, a user may have non-legacy gamma enabled for monitor
correction, while using legacy gamma for things like
redshift/nightlight.

To do so, we compose the two LUTs. Legacy gamma will be applied first,
then non-legacy. i.e. non-legacy_LUT(legacy_LUT(in_color)).

Note that the staged gamma LUT within the driver-private CRTC will
always contain the non-legacy LUT. This is to ensure that we have a
cached copy for future compositions.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 src/drmmode_display.c | 177 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 176 insertions(+), 1 deletion(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 6e5ae74..b4e1d57 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -788,6 +788,150 @@ static Bool drmmode_crtc_cm_enabled(drmmode_crtc_private_ptr drmmode_crtc)
 }
 
 /**
+ * If legacy LUT is a, and non-legacy LUT is b, then the result of b(a(x)) is
+ * returned in out_lut. out_lut's length is expected to be the same as the
+ * non-legacy LUT b.
+ *
+ * @a_(red|green|blue): The red, green, and blue components of the legacy LUT.
+ * @b_lut: The non-legacy LUT, in DRM's color LUT format.
+ * @out_lut: The composed LUT, in DRM's color LUT format.
+ * @len_a: Length of legacy lut.
+ * @len_b: Length of non-legacy lut.
+ */
+static void drmmode_lut_compose(uint16_t *a_red,
+				uint16_t *a_green,
+				uint16_t *a_blue,
+				struct drm_color_lut *b_lut,
+				struct drm_color_lut *out_lut,
+				uint32_t len_a, uint32_t len_b)
+{
+	uint32_t i_l, i_r, i;
+	uint32_t i_amax, i_bmax;
+	uint32_t coeff_ibmax;
+	uint32_t j;
+	uint64_t a_out_ibmax;
+	int color;
+	size_t struct_size = sizeof(struct drm_color_lut);
+
+	uint32_t max_lut = (1 << 16) - 1;
+
+	i_amax = len_a - 1;
+	i_bmax = len_b - 1;
+
+	/* A linear interpolation is done on the legacy LUT before it is
+	 * composed, to bring it up-to-size with the non-legacy LUT. The
+	 * interpolation uses integers by keeping things multiplied until the
+	 * last moment.
+	 */
+	for (color = 0; color < 3; color++) {
+		uint16_t *a, *b, *out;
+
+		/* Set the initial pointers to the right color components. The
+		 * inner for-loop will then maintain the correct offset from
+		 * the initial element.
+		 */
+		if (color == 0) {
+			a = a_red;
+			b = &b_lut[0].red;
+			out = &out_lut[0].red;
+		} else if (color == 1) {
+			a = a_green;
+			b = &b_lut[0].green;
+			out = &out_lut[0].green;
+		} else {
+			a = a_blue;
+			b = &b_lut[0].blue;
+			out = &out_lut[0].blue;
+		}
+
+		for (i = 0; i < len_b; i++) {
+			/* i_l and i_r tracks the left and right elements in
+			 * a_lut, to the sample point i. Also handle last
+			 * element edge case, when i_l = i_amax.
+			 */
+			i_l = i * i_amax / i_bmax;
+			i_r = i_l + !!(i_amax - i_l);
+
+			/* coeff is intended to be in [0, 1), depending on
+			 * where sample i is between i_l and i_r. We keep it
+			 * multiplied with i_bmax throughout to maintain
+			 * precision */
+			coeff_ibmax = (i * i_amax) - (i_l * i_bmax);
+			a_out_ibmax = i_bmax * a[i_l] +
+				      coeff_ibmax * (a[i_r] - a[i_l]);
+
+			/* j = floor((a_out/max_lut)*i_bmax).
+			 * i.e. the element in LUT b that a_out maps to. We
+			 * have to divide by max_lut to normalize a_out, since
+			 * values in the LUTs are [0, 1<<16)
+			 */
+			j = a_out_ibmax / max_lut;
+			*(uint16_t*)((void*)out + (i*struct_size)) =
+				*(uint16_t*)((void*)b + (j*struct_size));
+		}
+	}
+
+	for (i = 0; i < len_b; i++)
+		out_lut[i].reserved = 0;
+}
+
+/**
+ * Resize a LUT, using linear interpolation.
+ *
+ * @in_(red|green|blue): Legacy LUT components
+ * @out_lut: The resized LUT is returned here, in DRM color LUT format.
+ * @len_in: Length of legacy LUT.
+ * @len_out: Length of out_lut, i.e. the target size.
+ */
+static void drmmode_lut_interpolate(uint16_t *in_red,
+				    uint16_t *in_green,
+				    uint16_t *in_blue,
+				    struct drm_color_lut *out_lut,
+				    uint32_t len_in, uint32_t len_out)
+{
+	uint32_t i_l, i_r, i;
+	uint32_t i_amax, i_bmax;
+	uint32_t coeff_ibmax;
+	uint64_t out_ibmax;
+	int color;
+	size_t struct_size = sizeof(struct drm_color_lut);
+
+	i_amax = len_in - 1;
+	i_bmax = len_out - 1;
+
+	/* See @drmmode_lut_compose for details */
+	for (color = 0; color < 3; color++) {
+		uint16_t *in, *out;
+
+		if (color == 0) {
+			in = in_red;
+			out = &out_lut[0].red;
+		} else if (color == 1) {
+			in = in_green;
+			out = &out_lut[0].green;
+		} else {
+			in = in_blue;
+			out = &out_lut[0].blue;
+		}
+
+		for (i = 0; i < len_out; i++) {
+			i_l = i * i_amax / i_bmax;
+			i_r = i_l + !!(i_amax - i_l);
+
+			coeff_ibmax = (i * i_amax) - (i_l * i_bmax);
+			out_ibmax = i_bmax * in[i_l] +
+				      coeff_ibmax * (in[i_r] - in[i_l]);
+
+			*(uint16_t*)((void*)out + (i*struct_size)) =
+				out_ibmax / i_bmax;
+		}
+	}
+
+	for (i = 0; i < len_out; i++)
+		out_lut[i].reserved = 0;
+}
+
+/**
  * Configure and change a color property on a CRTC, through RandR. Only the
  * specified output will be affected, even if the CRTC is attached to multiple
  * outputs. Note that changes will be non-pending: the changes won't be pushed
@@ -1000,6 +1144,7 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
 {
 	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+	Bool free_blob_data = FALSE;
 	uint32_t created_blob_id = 0;
 	uint32_t drm_prop_id;
 	size_t expected_bytes = 0;
@@ -1013,7 +1158,30 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
 		/* Calculate the expected size of value in bytes */
 		expected_bytes = sizeof(struct drm_color_lut) *
 					drmmode_crtc->gamma_lut_size;
-		blob_data = drmmode_crtc->gamma_lut;
+
+		blob_data = malloc(expected_bytes);
+		if (!blob_data)
+			return BadAlloc;
+		free_blob_data = TRUE;
+		/*
+		 * Compose legacy and non-legacy LUT if non-legacy was set.
+		 * Otherwise, interpolate legacy LUT to non-legacy size.
+		 */
+		if (drmmode_crtc->gamma_lut) {
+			drmmode_lut_compose(crtc->gamma_red,
+					    crtc->gamma_green,
+					    crtc->gamma_blue,
+					    drmmode_crtc->gamma_lut,
+					    blob_data, crtc->gamma_size,
+					    drmmode_crtc->gamma_lut_size);
+		} else {
+			drmmode_lut_interpolate(crtc->gamma_red,
+						crtc->gamma_green,
+						crtc->gamma_blue,
+						blob_data,
+						crtc->gamma_size,
+						drmmode_crtc->gamma_lut_size);
+		}
 	} else if (cm_prop_index == CM_DEGAMMA_LUT) {
 		expected_bytes = sizeof(struct drm_color_lut) *
 					drmmode_crtc->degamma_lut_size;
@@ -1032,6 +1200,8 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
 			xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
 				   "Creating DRM blob failed with errno %d\n",
 				   ret);
+			if (free_blob_data)
+				free(blob_data);
 			return BadRequest;
 		}
 	}
@@ -1053,9 +1223,14 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
 		xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
 			   "Setting DRM property blob failed with errno %d\n",
 			   ret);
+		if (free_blob_data)
+			free(blob_data);
 		return BadRequest;
 	}
 
+	if (free_blob_data)
+		free(blob_data);
+
 	return Success;
 }
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 7/7] Also compose LUT when setting legacy gamma

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

We compose the two LUTs when pushing non-legacy gamma changes, and the
same needs to be done when setting legacy gamma.

To do so, we just call push_cm_prop() on the gamma LUT. It will compose
the LUTs for us, and fall back to using legacy LUT (upscaled to non-
legacy size) if non-legacy is unavailable.

It's also possible that the CRTC has no support support for non-legacy
color. In which case, we fall back to legacy gamma.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 src/drmmode_display.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index b4e1d57..d31f975 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -1240,9 +1240,21 @@ drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t *red, uint16_t *green,
 {
 	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+	int ret;
+
+	/* Use legacy if the CRTC does not support non-legacy gamma */
+	if (!drmmode_crtc_cm_enabled(drmmode_crtc)) {
+		drmModeCrtcSetGamma(pAMDGPUEnt->fd,
+				    drmmode_crtc->mode_crtc->crtc_id,
+				    size, red, green, blue);
+		return;
+	}
 
-	drmModeCrtcSetGamma(pAMDGPUEnt->fd, drmmode_crtc->mode_crtc->crtc_id,
-			    size, red, green, blue);
+	ret = drmmode_crtc_push_cm_prop(crtc, CM_GAMMA_LUT);
+	if (ret)
+		xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
+			   "Setting Gamma LUT failed with errno %d\n",
+			   ret);
 }
 
 Bool
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Michel Dänzer]

Hi Leo,


On 2018-06-01 06:03 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> This ended up being different enough from v2 to warrant a new patchset. Per
> Michel's suggestions, there have been various optimizations and cleanups.
> Here's what's changed:
> 
> * Cache DRM color management property IDs at pre-init,
>     * instead of querying DRM each time we need to modify color properties.
> 
> * Remove drmmode_update_cm_props().
>     * Update color properties in drmmode_output_get_property() instead.
>     * This also makes old calls to update_cm_props redundant.
> 
> * Get rid of fake CRTCs.
>     * Previously, we were allocating a fake CRTC to configure color props on
>       outputs that don't have a CRTC.
>     * Instead, rr_configure_and_change_cm_property() can be easily modified to
>       accept NULL CRTCs.
> 
> * Drop patches to persist color properties across DPMS events.
>     * Kernel driver should be patched instead:
>       https://lists.freedesktop.org/archives/amd-gfx/2018-May/022744.html
>     * Color props including legacy gamma now persist across crtc dpms.
>     * Non-legacy props now persist across output dpms and hotplug, as long
>       as the same CRTC remains attached to that output.
> 
> And some smaller improvements:
> 
> * Change CTM to be 32-bit format instead of 16-bit.
>     * This requires clients to ensure that each 32-bit element is padded to be
>       long-sized, since libXrandr parses 32-bit format as long-typed.
> 
> * Optimized color management init during CRTC init.
>     * Query DRM once for the list of properties, instead of twice.

Sounds good. I'll be going through the patches in detail from now on,
but I don't know yet when I'll be able to finish the review.


Meanwhile, heads up on two issues I discovered while smoke-testing the
series (which are sort of related, but occur even without this series):


Running Xorg in depth 30[0] results in completely wrong colours
(everything has a red tint) with current kernels. I think this is
because DC now preserves the gamma LUT values, but xf86-video-amdgpu
never sets them at depth 30, so the hardware is still using values for
24-bit RGB.

Can you look into making xf86-video-amdgpu set the LUT values at depth
30 as well? Ideally in a way which doesn't require all patches in this
series, so that it could be backported for a 18.0.2 point release if
necessary. (Similarly for skipping drmmode_cursor_gamma when the kernel
supports the new colour management properties, to fix
https://bugs.freedesktop.org/106578)


Trying to run Xorg in depth 16 or 8[0] results in failure to set any mode:

[    56.138] (EE) AMDGPU(0): failed to set mode: Invalid argument
[    56.138] (WW) AMDGPU(0): Failed to set mode on CRTC 0
[    56.172] (EE) AMDGPU(0): Failed to enable any CRTC

Works fine with amdgpu.dc=0. This has been broken at least since the
4.16 development cycle.


[0] You can change Xorg's colour depth either via -depth on its command
line, or via the xorg.conf screen section:

Section "Screen"
        Identifier "Screen0"
        DefaultDepth 30 # or 16 or 8
EndSection


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Michel Dänzer]

On 2018-06-06 06:01 PM, Michel Dänzer wrote:
> On 2018-06-01 06:03 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>
>> This ended up being different enough from v2 to warrant a new patchset. Per
>> Michel's suggestions, there have been various optimizations and cleanups.
>> Here's what's changed:
>>
>> * Cache DRM color management property IDs at pre-init,
>>     * instead of querying DRM each time we need to modify color properties.
>>
>> * Remove drmmode_update_cm_props().
>>     * Update color properties in drmmode_output_get_property() instead.
>>     * This also makes old calls to update_cm_props redundant.
>>
>> * Get rid of fake CRTCs.
>>     * Previously, we were allocating a fake CRTC to configure color props on
>>       outputs that don't have a CRTC.
>>     * Instead, rr_configure_and_change_cm_property() can be easily modified to
>>       accept NULL CRTCs.
>>
>> * Drop patches to persist color properties across DPMS events.
>>     * Kernel driver should be patched instead:
>>       https://lists.freedesktop.org/archives/amd-gfx/2018-May/022744.html
>>     * Color props including legacy gamma now persist across crtc dpms.
>>     * Non-legacy props now persist across output dpms and hotplug, as long
>>       as the same CRTC remains attached to that output.
>>
>> And some smaller improvements:
>>
>> * Change CTM to be 32-bit format instead of 16-bit.
>>     * This requires clients to ensure that each 32-bit element is padded to be
>>       long-sized, since libXrandr parses 32-bit format as long-typed.
>>
>> * Optimized color management init during CRTC init.
>>     * Query DRM once for the list of properties, instead of twice.
> 
> Sounds good. I'll be going through the patches in detail from now on,
> but I don't know yet when I'll be able to finish the review.
> 
> 
> Meanwhile, heads up on two issues I discovered while smoke-testing the
> series (which are sort of related, but occur even without this series):
> 
> 
> Running Xorg in depth 30[0] results in completely wrong colours
> (everything has a red tint) with current kernels. I think this is
> because DC now preserves the gamma LUT values, but xf86-video-amdgpu
> never sets them at depth 30, so the hardware is still using values for
> 24-bit RGB.

Actually, looks like I made a mistake in my testing before; this issue
only occurs as of patch 6 of this series.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Leo Li]

On 2018-06-06 01:03 PM, Michel Dänzer wrote:
> On 2018-06-06 06:01 PM, Michel Dänzer wrote:
>> On 2018-06-01 06:03 PM, sunpeng.li@amd.com wrote:
>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>
>>> This ended up being different enough from v2 to warrant a new patchset. Per
>>> Michel's suggestions, there have been various optimizations and cleanups.
>>> Here's what's changed:
>>>
>>> * Cache DRM color management property IDs at pre-init,
>>>      * instead of querying DRM each time we need to modify color properties.
>>>
>>> * Remove drmmode_update_cm_props().
>>>      * Update color properties in drmmode_output_get_property() instead.
>>>      * This also makes old calls to update_cm_props redundant.
>>>
>>> * Get rid of fake CRTCs.
>>>      * Previously, we were allocating a fake CRTC to configure color props on
>>>        outputs that don't have a CRTC.
>>>      * Instead, rr_configure_and_change_cm_property() can be easily modified to
>>>        accept NULL CRTCs.
>>>
>>> * Drop patches to persist color properties across DPMS events.
>>>      * Kernel driver should be patched instead:
>>>        https://lists.freedesktop.org/archives/amd-gfx/2018-May/022744.html
>>>      * Color props including legacy gamma now persist across crtc dpms.
>>>      * Non-legacy props now persist across output dpms and hotplug, as long
>>>        as the same CRTC remains attached to that output.
>>>
>>> And some smaller improvements:
>>>
>>> * Change CTM to be 32-bit format instead of 16-bit.
>>>      * This requires clients to ensure that each 32-bit element is padded to be
>>>        long-sized, since libXrandr parses 32-bit format as long-typed.
>>>
>>> * Optimized color management init during CRTC init.
>>>      * Query DRM once for the list of properties, instead of twice.
>>
>> Sounds good. I'll be going through the patches in detail from now on,
>> but I don't know yet when I'll be able to finish the review.
>>
>>
>> Meanwhile, heads up on two issues I discovered while smoke-testing the
>> series (which are sort of related, but occur even without this series):
>>
>>
>> Running Xorg in depth 30[0] results in completely wrong colours
>> (everything has a red tint) with current kernels. I think this is
>> because DC now preserves the gamma LUT values, but xf86-video-amdgpu
>> never sets them at depth 30, so the hardware is still using values for
>> 24-bit RGB.
> 
> Actually, looks like I made a mistake in my testing before; this issue
> only occurs as of patch 6 of this series.
> 

Hi Michel,

I'll look into this. Thanks for testing :)

Leo

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Mike Lothian]

[-- Attachment #1.1: Type: text/plain, Size: 3990 bytes --]

Hi

Is there a way to set depth using a xorg.conf.d snippet rather than
creating an xorg.conf?

Cheers

Mike

On Wed, 6 Jun 2018 at 17:01 Michel Dänzer <michel-otUistvHUpPR7s880joybQ@public.gmane.org> wrote:

>
> Hi Leo,
>
>
> On 2018-06-01 06:03 PM, sunpeng.li-5C7GfCeVMHo@public.gmane.org wrote:
> > From: "Leo (Sunpeng) Li" <sunpeng.li-5C7GfCeVMHo@public.gmane.org>
> >
> > This ended up being different enough from v2 to warrant a new patchset.
> Per
> > Michel's suggestions, there have been various optimizations and cleanups.
> > Here's what's changed:
> >
> > * Cache DRM color management property IDs at pre-init,
> >     * instead of querying DRM each time we need to modify color
> properties.
> >
> > * Remove drmmode_update_cm_props().
> >     * Update color properties in drmmode_output_get_property() instead.
> >     * This also makes old calls to update_cm_props redundant.
> >
> > * Get rid of fake CRTCs.
> >     * Previously, we were allocating a fake CRTC to configure color
> props on
> >       outputs that don't have a CRTC.
> >     * Instead, rr_configure_and_change_cm_property() can be easily
> modified to
> >       accept NULL CRTCs.
> >
> > * Drop patches to persist color properties across DPMS events.
> >     * Kernel driver should be patched instead:
> >
> https://lists.freedesktop.org/archives/amd-gfx/2018-May/022744.html
> >     * Color props including legacy gamma now persist across crtc dpms.
> >     * Non-legacy props now persist across output dpms and hotplug, as
> long
> >       as the same CRTC remains attached to that output.
> >
> > And some smaller improvements:
> >
> > * Change CTM to be 32-bit format instead of 16-bit.
> >     * This requires clients to ensure that each 32-bit element is padded
> to be
> >       long-sized, since libXrandr parses 32-bit format as long-typed.
> >
> > * Optimized color management init during CRTC init.
> >     * Query DRM once for the list of properties, instead of twice.
>
> Sounds good. I'll be going through the patches in detail from now on,
> but I don't know yet when I'll be able to finish the review.
>
>
> Meanwhile, heads up on two issues I discovered while smoke-testing the
> series (which are sort of related, but occur even without this series):
>
>
> Running Xorg in depth 30[0] results in completely wrong colours
> (everything has a red tint) with current kernels. I think this is
> because DC now preserves the gamma LUT values, but xf86-video-amdgpu
> never sets them at depth 30, so the hardware is still using values for
> 24-bit RGB.
>
> Can you look into making xf86-video-amdgpu set the LUT values at depth
> 30 as well? Ideally in a way which doesn't require all patches in this
> series, so that it could be backported for a 18.0.2 point release if
> necessary. (Similarly for skipping drmmode_cursor_gamma when the kernel
> supports the new colour management properties, to fix
> https://bugs.freedesktop.org/106578)
>
>
> Trying to run Xorg in depth 16 or 8[0] results in failure to set any mode:
>
> [    56.138] (EE) AMDGPU(0): failed to set mode: Invalid argument
> [    56.138] (WW) AMDGPU(0): Failed to set mode on CRTC 0
> [    56.172] (EE) AMDGPU(0): Failed to enable any CRTC
>
> Works fine with amdgpu.dc=0. This has been broken at least since the
> 4.16 development cycle.
>
>
> [0] You can change Xorg's colour depth either via -depth on its command
> line, or via the xorg.conf screen section:
>
> Section "Screen"
>         Identifier "Screen0"
>         DefaultDepth 30 # or 16 or 8
> EndSection
>
>
> --
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 5342 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Michel Dänzer]

On 2018-06-07 09:22 AM, Mike Lothian wrote:
> Hi
> 
> Is there a way to set depth using a xorg.conf.d snippet rather than
> creating an xorg.conf?

Sure (the xorg.conf.d snippets are simply concatenated together with
xorg.conf, so anything can go in either of them), but it's the wrong way
to do it. :)

The xorg.conf.d mechanism is for shipping snippets required for a driver
or other Xorg module to work out of the box. xorg.conf is still the
place for actual configuration.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Michel Dänzer]

On 2018-06-06 07:03 PM, Michel Dänzer wrote:
> On 2018-06-06 06:01 PM, Michel Dänzer wrote:
>> On 2018-06-01 06:03 PM, sunpeng.li@amd.com wrote:
>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>
>>> This ended up being different enough from v2 to warrant a new patchset. Per
>>> Michel's suggestions, there have been various optimizations and cleanups.
>>> Here's what's changed:
>>>
>>> * Cache DRM color management property IDs at pre-init,
>>>     * instead of querying DRM each time we need to modify color properties.
>>>
>>> * Remove drmmode_update_cm_props().
>>>     * Update color properties in drmmode_output_get_property() instead.
>>>     * This also makes old calls to update_cm_props redundant.
>>>
>>> * Get rid of fake CRTCs.
>>>     * Previously, we were allocating a fake CRTC to configure color props on
>>>       outputs that don't have a CRTC.
>>>     * Instead, rr_configure_and_change_cm_property() can be easily modified to
>>>       accept NULL CRTCs.
>>>
>>> * Drop patches to persist color properties across DPMS events.
>>>     * Kernel driver should be patched instead:
>>>       https://lists.freedesktop.org/archives/amd-gfx/2018-May/022744.html
>>>     * Color props including legacy gamma now persist across crtc dpms.
>>>     * Non-legacy props now persist across output dpms and hotplug, as long
>>>       as the same CRTC remains attached to that output.
>>>
>>> And some smaller improvements:
>>>
>>> * Change CTM to be 32-bit format instead of 16-bit.
>>>     * This requires clients to ensure that each 32-bit element is padded to be
>>>       long-sized, since libXrandr parses 32-bit format as long-typed.
>>>
>>> * Optimized color management init during CRTC init.
>>>     * Query DRM once for the list of properties, instead of twice.
>>
>> Sounds good. I'll be going through the patches in detail from now on,
>> but I don't know yet when I'll be able to finish the review.
>>
>>
>> Meanwhile, heads up on two issues I discovered while smoke-testing the
>> series (which are sort of related, but occur even without this series):
>>
>>
>> Running Xorg in depth 30[0] results in completely wrong colours
>> (everything has a red tint) with current kernels. I think this is
>> because DC now preserves the gamma LUT values, but xf86-video-amdgpu
>> never sets them at depth 30, so the hardware is still using values for
>> 24-bit RGB.
> 
> Actually, looks like I made a mistake in my testing before; this issue
> only occurs as of patch 6 of this series.

For this series, the simplest solution for now is probably to continue
not setting any gamma values at depth 30, and probably not expose the
colour management properties either. We can worry about making this all
work at depth 30 later.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Mike Lothian]

[-- Attachment #1.1: Type: text/plain, Size: 859 bytes --]

It seems messy having to create a whole xorg.conf just for one parameter

On Thu, 7 Jun 2018 at 08:31 Michel Dänzer <michel-otUistvHUpPR7s880joybQ@public.gmane.org> wrote:

> On 2018-06-07 09:22 AM, Mike Lothian wrote:
> > Hi
> >
> > Is there a way to set depth using a xorg.conf.d snippet rather than
> > creating an xorg.conf?
>
> Sure (the xorg.conf.d snippets are simply concatenated together with
> xorg.conf, so anything can go in either of them), but it's the wrong way
> to do it. :)
>
> The xorg.conf.d mechanism is for shipping snippets required for a driver
> or other Xorg module to work out of the box. xorg.conf is still the
> place for actual configuration.
>
>
> --
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer
>

[-- Attachment #1.2: Type: text/html, Size: 1252 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Michel Dänzer]

On 2018-06-07 03:26 PM, Mike Lothian wrote:
> It seems messy having to create a whole xorg.conf just for one parameter

Not sure I can agree, anyway it's the same whether the file is called
/etc/X11/xorf.conf or /etc/X11/xorg.conf.d/foobar . :)


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Leo Li]

On 2018-06-06 01:03 PM, Michel Dänzer wrote:
> On 2018-06-06 06:01 PM, Michel Dänzer wrote:
>> On 2018-06-01 06:03 PM, sunpeng.li@amd.com wrote:
>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>
>>> This ended up being different enough from v2 to warrant a new patchset. Per
>>> Michel's suggestions, there have been various optimizations and cleanups.
>>> Here's what's changed:
>>>
>>> * Cache DRM color management property IDs at pre-init,
>>>      * instead of querying DRM each time we need to modify color properties.
>>>
>>> * Remove drmmode_update_cm_props().
>>>      * Update color properties in drmmode_output_get_property() instead.
>>>      * This also makes old calls to update_cm_props redundant.
>>>
>>> * Get rid of fake CRTCs.
>>>      * Previously, we were allocating a fake CRTC to configure color props on
>>>        outputs that don't have a CRTC.
>>>      * Instead, rr_configure_and_change_cm_property() can be easily modified to
>>>        accept NULL CRTCs.
>>>
>>> * Drop patches to persist color properties across DPMS events.
>>>      * Kernel driver should be patched instead:
>>>        https://lists.freedesktop.org/archives/amd-gfx/2018-May/022744.html
>>>      * Color props including legacy gamma now persist across crtc dpms.
>>>      * Non-legacy props now persist across output dpms and hotplug, as long
>>>        as the same CRTC remains attached to that output.
>>>
>>> And some smaller improvements:
>>>
>>> * Change CTM to be 32-bit format instead of 16-bit.
>>>      * This requires clients to ensure that each 32-bit element is padded to be
>>>        long-sized, since libXrandr parses 32-bit format as long-typed.
>>>
>>> * Optimized color management init during CRTC init.
>>>      * Query DRM once for the list of properties, instead of twice.
>>
>> Sounds good. I'll be going through the patches in detail from now on,
>> but I don't know yet when I'll be able to finish the review.
>>
>>
>> Meanwhile, heads up on two issues I discovered while smoke-testing the
>> series (which are sort of related, but occur even without this series):
>>
>>
>> Running Xorg in depth 30[0] results in completely wrong colours
>> (everything has a red tint) with current kernels. I think this is
>> because DC now preserves the gamma LUT values, but xf86-video-amdgpu
>> never sets them at depth 30, so the hardware is still using values for
>> 24-bit RGB.
> 
> Actually, looks like I made a mistake in my testing before; this issue
> only occurs as of patch 6 of this series.
> 

It seems to be caused by legacy gamma being disabled on depth 30. When
that's the case, the gamma_set() hook is set to null. RandR won't
compute the legacy LUT, causing LUT interpolation/composition to spit
out an invalid LUT. I verified this by commenting out the `pScrn->depth
== 30` conditionals guarding the legacy gamma features (in pre_init, and
mode_major).

I'm not certain of the best way to fix this. Would it make sense to
enable legacy gamma on 30bpp if non-legacy is supported? We can do that
since legacy gamma gets interpolated/composed to non-legacy.

However, it doesn't make sense when you look at the LUT size, since
legacy gamma supports only 256 elements (should be 1024 on depth 30?)
In which case we can skip interpolation/composition altogether.

Leo
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Michel Dänzer]

On 2018-06-08 12:21 AM, Leo Li wrote:
> 
> 
> On 2018-06-06 01:03 PM, Michel Dänzer wrote:
>> On 2018-06-06 06:01 PM, Michel Dänzer wrote:
>>> On 2018-06-01 06:03 PM, sunpeng.li@amd.com wrote:
>>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>>
>>>> This ended up being different enough from v2 to warrant a new
>>>> patchset. Per
>>>> Michel's suggestions, there have been various optimizations and
>>>> cleanups.
>>>> Here's what's changed:
>>>>
>>>> * Cache DRM color management property IDs at pre-init,
>>>>      * instead of querying DRM each time we need to modify color
>>>> properties.
>>>>
>>>> * Remove drmmode_update_cm_props().
>>>>      * Update color properties in drmmode_output_get_property()
>>>> instead.
>>>>      * This also makes old calls to update_cm_props redundant.
>>>>
>>>> * Get rid of fake CRTCs.
>>>>      * Previously, we were allocating a fake CRTC to configure color
>>>> props on
>>>>        outputs that don't have a CRTC.
>>>>      * Instead, rr_configure_and_change_cm_property() can be easily
>>>> modified to
>>>>        accept NULL CRTCs.
>>>>
>>>> * Drop patches to persist color properties across DPMS events.
>>>>      * Kernel driver should be patched instead:
>>>>       
>>>> https://lists.freedesktop.org/archives/amd-gfx/2018-May/022744.html
>>>>      * Color props including legacy gamma now persist across crtc dpms.
>>>>      * Non-legacy props now persist across output dpms and hotplug,
>>>> as long
>>>>        as the same CRTC remains attached to that output.
>>>>
>>>> And some smaller improvements:
>>>>
>>>> * Change CTM to be 32-bit format instead of 16-bit.
>>>>      * This requires clients to ensure that each 32-bit element is
>>>> padded to be
>>>>        long-sized, since libXrandr parses 32-bit format as long-typed.
>>>>
>>>> * Optimized color management init during CRTC init.
>>>>      * Query DRM once for the list of properties, instead of twice.
>>>
>>> Sounds good. I'll be going through the patches in detail from now on,
>>> but I don't know yet when I'll be able to finish the review.
>>>
>>>
>>> Meanwhile, heads up on two issues I discovered while smoke-testing the
>>> series (which are sort of related, but occur even without this series):
>>>
>>>
>>> Running Xorg in depth 30[0] results in completely wrong colours
>>> (everything has a red tint) with current kernels. I think this is
>>> because DC now preserves the gamma LUT values, but xf86-video-amdgpu
>>> never sets them at depth 30, so the hardware is still using values for
>>> 24-bit RGB.
>>
>> Actually, looks like I made a mistake in my testing before; this issue
>> only occurs as of patch 6 of this series.
>>
> 
> It seems to be caused by legacy gamma being disabled on depth 30. When
> that's the case, the gamma_set() hook is set to null. RandR won't
> compute the legacy LUT, causing LUT interpolation/composition to spit
> out an invalid LUT. I verified this by commenting out the `pScrn->depth
> == 30` conditionals guarding the legacy gamma features (in pre_init, and
> mode_major).
> 
> I'm not certain of the best way to fix this. Would it make sense to
> enable legacy gamma on 30bpp if non-legacy is supported? We can do that
> since legacy gamma gets interpolated/composed to non-legacy.
> 
> However, it doesn't make sense when you look at the LUT size, since
> legacy gamma supports only 256 elements (should be 1024 on depth 30?)
> In which case we can skip interpolation/composition altogether.

Right, we'll probably need to increase the RandR LUT size as well at
depth 30.


Anyway, let's not hold up this patch series over this. Simply keep all
this code disabled (i.e. same as if the kernel didn't expose the
properties) at depth 30 for now.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Michel Dänzer]

On 2018-06-08 04:33 PM, Michel Dänzer wrote:
> On 2018-06-08 12:21 AM, Leo Li wrote:
>> On 2018-06-06 01:03 PM, Michel Dänzer wrote:
>>> On 2018-06-06 06:01 PM, Michel Dänzer wrote:
>>>>
>>>> Running Xorg in depth 30[0] results in completely wrong colours
>>>> (everything has a red tint) with current kernels. I think this is
>>>> because DC now preserves the gamma LUT values, but xf86-video-amdgpu
>>>> never sets them at depth 30, so the hardware is still using values for
>>>> 24-bit RGB.
>>>
>>> Actually, looks like I made a mistake in my testing before; this issue
>>> only occurs as of patch 6 of this series.
>>
>> It seems to be caused by legacy gamma being disabled on depth 30. When
>> that's the case, the gamma_set() hook is set to null. RandR won't
>> compute the legacy LUT, causing LUT interpolation/composition to spit
>> out an invalid LUT. I verified this by commenting out the `pScrn->depth
>> == 30` conditionals guarding the legacy gamma features (in pre_init, and
>> mode_major).
>>
>> I'm not certain of the best way to fix this. Would it make sense to
>> enable legacy gamma on 30bpp if non-legacy is supported? We can do that
>> since legacy gamma gets interpolated/composed to non-legacy.
>>
>> However, it doesn't make sense when you look at the LUT size, since
>> legacy gamma supports only 256 elements (should be 1024 on depth 30?)
>> In which case we can skip interpolation/composition altogether.
> 
> Right, we'll probably need to increase the RandR LUT size as well at
> depth 30.
> 
> 
> Anyway, let's not hold up this patch series over this. Simply keep all
> this code disabled (i.e. same as if the kernel didn't expose the
> properties) at depth 30 for now.

Or, if there's a straightforward way to expose the properties while
ignoring the RandR LUT at depth 30, that's another option for this series.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 6/7 v2] Compose non-legacy with legacy regamma LUT

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

Frequently, a user may have non-legacy gamma enabled for monitor
correction, while using legacy gamma for things like
redshift/nightlight.

To do so, we compose the two LUTs. Legacy gamma will be applied first,
then non-legacy. i.e. non-legacy_LUT(legacy_LUT(in_color)).

Note that the staged gamma LUT within the driver-private CRTC will
always contain the non-legacy LUT. This is to ensure that we have a
cached copy for future compositions.

v2: Don't compose LUTs if legacy gamma is disabled (which is the case
    for deep 30bpp color). The legacy LUT won't be computed here,
    causing composition to spit out something invalid.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 src/drmmode_display.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 186 insertions(+), 1 deletion(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 6e5ae74..77a136e 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -788,6 +788,150 @@ static Bool drmmode_crtc_cm_enabled(drmmode_crtc_private_ptr drmmode_crtc)
 }
 
 /**
+ * If legacy LUT is a, and non-legacy LUT is b, then the result of b(a(x)) is
+ * returned in out_lut. out_lut's length is expected to be the same as the
+ * non-legacy LUT b.
+ *
+ * @a_(red|green|blue): The red, green, and blue components of the legacy LUT.
+ * @b_lut: The non-legacy LUT, in DRM's color LUT format.
+ * @out_lut: The composed LUT, in DRM's color LUT format.
+ * @len_a: Length of legacy lut.
+ * @len_b: Length of non-legacy lut.
+ */
+static void drmmode_lut_compose(uint16_t *a_red,
+				uint16_t *a_green,
+				uint16_t *a_blue,
+				struct drm_color_lut *b_lut,
+				struct drm_color_lut *out_lut,
+				uint32_t len_a, uint32_t len_b)
+{
+	uint32_t i_l, i_r, i;
+	uint32_t i_amax, i_bmax;
+	uint32_t coeff_ibmax;
+	uint32_t j;
+	uint64_t a_out_ibmax;
+	int color;
+	size_t struct_size = sizeof(struct drm_color_lut);
+
+	uint32_t max_lut = (1 << 16) - 1;
+
+	i_amax = len_a - 1;
+	i_bmax = len_b - 1;
+
+	/* A linear interpolation is done on the legacy LUT before it is
+	 * composed, to bring it up-to-size with the non-legacy LUT. The
+	 * interpolation uses integers by keeping things multiplied until the
+	 * last moment.
+	 */
+	for (color = 0; color < 3; color++) {
+		uint16_t *a, *b, *out;
+
+		/* Set the initial pointers to the right color components. The
+		 * inner for-loop will then maintain the correct offset from
+		 * the initial element.
+		 */
+		if (color == 0) {
+			a = a_red;
+			b = &b_lut[0].red;
+			out = &out_lut[0].red;
+		} else if (color == 1) {
+			a = a_green;
+			b = &b_lut[0].green;
+			out = &out_lut[0].green;
+		} else {
+			a = a_blue;
+			b = &b_lut[0].blue;
+			out = &out_lut[0].blue;
+		}
+
+		for (i = 0; i < len_b; i++) {
+			/* i_l and i_r tracks the left and right elements in
+			 * a_lut, to the sample point i. Also handle last
+			 * element edge case, when i_l = i_amax.
+			 */
+			i_l = i * i_amax / i_bmax;
+			i_r = i_l + !!(i_amax - i_l);
+
+			/* coeff is intended to be in [0, 1), depending on
+			 * where sample i is between i_l and i_r. We keep it
+			 * multiplied with i_bmax throughout to maintain
+			 * precision */
+			coeff_ibmax = (i * i_amax) - (i_l * i_bmax);
+			a_out_ibmax = i_bmax * a[i_l] +
+				      coeff_ibmax * (a[i_r] - a[i_l]);
+
+			/* j = floor((a_out/max_lut)*i_bmax).
+			 * i.e. the element in LUT b that a_out maps to. We
+			 * have to divide by max_lut to normalize a_out, since
+			 * values in the LUTs are [0, 1<<16)
+			 */
+			j = a_out_ibmax / max_lut;
+			*(uint16_t*)((void*)out + (i*struct_size)) =
+				*(uint16_t*)((void*)b + (j*struct_size));
+		}
+	}
+
+	for (i = 0; i < len_b; i++)
+		out_lut[i].reserved = 0;
+}
+
+/**
+ * Resize a LUT, using linear interpolation.
+ *
+ * @in_(red|green|blue): Legacy LUT components
+ * @out_lut: The resized LUT is returned here, in DRM color LUT format.
+ * @len_in: Length of legacy LUT.
+ * @len_out: Length of out_lut, i.e. the target size.
+ */
+static void drmmode_lut_interpolate(uint16_t *in_red,
+				    uint16_t *in_green,
+				    uint16_t *in_blue,
+				    struct drm_color_lut *out_lut,
+				    uint32_t len_in, uint32_t len_out)
+{
+	uint32_t i_l, i_r, i;
+	uint32_t i_amax, i_bmax;
+	uint32_t coeff_ibmax;
+	uint64_t out_ibmax;
+	int color;
+	size_t struct_size = sizeof(struct drm_color_lut);
+
+	i_amax = len_in - 1;
+	i_bmax = len_out - 1;
+
+	/* See @drmmode_lut_compose for details */
+	for (color = 0; color < 3; color++) {
+		uint16_t *in, *out;
+
+		if (color == 0) {
+			in = in_red;
+			out = &out_lut[0].red;
+		} else if (color == 1) {
+			in = in_green;
+			out = &out_lut[0].green;
+		} else {
+			in = in_blue;
+			out = &out_lut[0].blue;
+		}
+
+		for (i = 0; i < len_out; i++) {
+			i_l = i * i_amax / i_bmax;
+			i_r = i_l + !!(i_amax - i_l);
+
+			coeff_ibmax = (i * i_amax) - (i_l * i_bmax);
+			out_ibmax = i_bmax * in[i_l] +
+				      coeff_ibmax * (in[i_r] - in[i_l]);
+
+			*(uint16_t*)((void*)out + (i*struct_size)) =
+				out_ibmax / i_bmax;
+		}
+	}
+
+	for (i = 0; i < len_out; i++)
+		out_lut[i].reserved = 0;
+}
+
+/**
  * Configure and change a color property on a CRTC, through RandR. Only the
  * specified output will be affected, even if the CRTC is attached to multiple
  * outputs. Note that changes will be non-pending: the changes won't be pushed
@@ -1000,6 +1144,7 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
 {
 	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+	Bool free_blob_data = FALSE;
 	uint32_t created_blob_id = 0;
 	uint32_t drm_prop_id;
 	size_t expected_bytes = 0;
@@ -1013,7 +1158,39 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
 		/* Calculate the expected size of value in bytes */
 		expected_bytes = sizeof(struct drm_color_lut) *
 					drmmode_crtc->gamma_lut_size;
-		blob_data = drmmode_crtc->gamma_lut;
+
+		/* Legacy gamma LUT is disabled on deep 30bpp color. In which
+		 * case, directly use non-legacy LUT.
+		 */
+		if (crtc->funcs->gamma_set == NULL) {
+			blob_data = drmmode_crtc->gamma_lut;
+			goto do_push;
+		}
+
+		blob_data = malloc(expected_bytes);
+		if (!blob_data)
+			return BadAlloc;
+
+		free_blob_data = TRUE;
+		/*
+		 * Compose legacy and non-legacy LUT if non-legacy was set.
+		 * Otherwise, interpolate legacy LUT to non-legacy size.
+		 */
+		if (drmmode_crtc->gamma_lut) {
+			drmmode_lut_compose(crtc->gamma_red,
+					    crtc->gamma_green,
+					    crtc->gamma_blue,
+					    drmmode_crtc->gamma_lut,
+					    blob_data, crtc->gamma_size,
+					    drmmode_crtc->gamma_lut_size);
+		} else {
+			drmmode_lut_interpolate(crtc->gamma_red,
+						crtc->gamma_green,
+						crtc->gamma_blue,
+						blob_data,
+						crtc->gamma_size,
+						drmmode_crtc->gamma_lut_size);
+		}
 	} else if (cm_prop_index == CM_DEGAMMA_LUT) {
 		expected_bytes = sizeof(struct drm_color_lut) *
 					drmmode_crtc->degamma_lut_size;
@@ -1024,6 +1201,7 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
 	} else
 		return BadName;
 
+do_push:
 	if (blob_data) {
 		ret = drmModeCreatePropertyBlob(pAMDGPUEnt->fd,
 						blob_data, expected_bytes,
@@ -1032,6 +1210,8 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
 			xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
 				   "Creating DRM blob failed with errno %d\n",
 				   ret);
+			if (free_blob_data)
+				free(blob_data);
 			return BadRequest;
 		}
 	}
@@ -1053,9 +1233,14 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
 		xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
 			   "Setting DRM property blob failed with errno %d\n",
 			   ret);
+		if (free_blob_data)
+			free(blob_data);
 		return BadRequest;
 	}
 
+	if (free_blob_data)
+		free(blob_data);
+
 	return Success;
 }
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Michel Dänzer]

Hi Leo,


sorry for the delay.


On 2018-06-01 06:03 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> This ended up being different enough from v2 to warrant a new patchset. Per
> Michel's suggestions, there have been various optimizations and cleanups.
> Here's what's changed:
> 
> * Cache DRM color management property IDs at pre-init,
>     * instead of querying DRM each time we need to modify color properties.
> 
> * Remove drmmode_update_cm_props().
>     * Update color properties in drmmode_output_get_property() instead.
>     * This also makes old calls to update_cm_props redundant.
> 
> * Get rid of fake CRTCs.
>     * Previously, we were allocating a fake CRTC to configure color props on
>       outputs that don't have a CRTC.
>     * Instead, rr_configure_and_change_cm_property() can be easily modified to
>       accept NULL CRTCs.

Is it currently ever the case in the hardware / kernel, or expected to
ever be, that colour management is supported for some but not all CRTCs
of a GPU?

If not, the LUT sizes could be tracked once instead of per CRTC, and at
least the (DE)GAMMA_LUT_SIZE properties could always return the proper
values, even if the output currently isn't associated with a CRTC.


Other than that, I'm going to send some minor feedback on patches 2 & 3.
If you prefer, I could fix up those and other cosmetic issues before
pushing the patches.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 2/7] Initialize color properties on CRTC during CRTC init

[Michel Dänzer]

On 2018-06-01 06:03 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> And destroy them on the CRTC destroy hook.
> 
> When initializing color management properties on the private
> drmmode_crtc, we want to:
> 
> 1. Obtain its degamma and regamma LUT sizes
> 2. Default its color transform matrix (CTM) to identity
> 3. Program hardware with default color management values (SRGB for
>    de/regamma, identity for CTM)
> 
> It's possible that cm initialization fails due to memory error or DRM
> error. In which case, DDX support for color management will be disabled
> on this CRTC.
> 
> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
> 
> [...]
> 
> +	drmModeFreeObjectProperties(drm_props);
> +
> +	if (!drmmode_crtc_cm_enabled(drmmode_crtc)) {
> +		xf86DrvMsg(crtc->scrn->scrnIndex, X_INFO,
> +			   "CRTC%d does not support non-legacy color management.\n",
> +			   drmmode_get_crtc_id(crtc));
> +		drmmode_crtc->degamma_lut_size = 0;
> +		drmmode_crtc->gamma_lut_size = 0;

drmmode_crtc_cm_enabled returning FALSE means these are already both 0.


> +	xf86DrvMsg(crtc->scrn->scrnIndex, X_INFO,
> +		   "CRTC%d supports non-legacy color management.\n",
> +		   drmmode_get_crtc_id(crtc));

Is this log message really needed? For every CRTC? :)

This can easily be determined by checking for the existence of the
properties with xrandr.


> +	/* Push properties to reset properties currently in hardware */
> +	for (i = 0; i < CM_NUM_PROPS; i++) {
> +		if (i == CM_DEGAMMA_LUT_SIZE || i == CM_GAMMA_LUT_SIZE)
> +			continue;

This loop and the drmmode_output_set_property change in patch 5 could be
simplified by putting CM_(DE)GAMMA_LUT_SIZE at the end of the enum
definition and using the first of them as the sentinel instead of
CM_NUM_PROPS.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 3/7] Configure color properties when creating output resources

[Michel Dänzer]

On 2018-06-01 06:03 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> List color management properties on outputs if there's at least one
> CRTC that supports color management. Otherwise, don't list them at
> all.
> 
> If there's no CRTC attached to the output, and there exists a CRTC
> that supports color management, then list "disabled" properties
> (immutable and NULL-valued).
> 
> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
> 
> [...]
>  
> +	if (cm_prop_index == CM_GAMMA_LUT_SIZE) {
> +		format = 32;
> +		length = 1;
> +		data = crtc ? &crtc->gamma_lut_size : &zero;
> +		range[0] = 0;
> +		range[1] = -1;
> +
> +	} else if (cm_prop_index == CM_DEGAMMA_LUT_SIZE) {
> +	[...]

Better use a switch statement here instead of if with multiple else if.
(Same in patch 5)


> +	/* Always issue a non-pending change. We'll push cm properties
> +	 * ourselves.
> +	 */
> +	err = RRChangeOutputProperty(output->randr_output, atom,
> +				     XA_INTEGER, format,
> +				     PropModeReplace,
> +				     length, data, FALSE, FALSE);
> +	if (err) {
> +		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
> +			   "Changing color management property %s failed with %d\n",
> +			   cm_prop_names[cm_prop_index], err);
> +		return err;
> +	}
> +
> +	return 0;
> +}

Could simply always return err here.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Leo Li]

On 2018-06-14 12:57 PM, Michel Dänzer wrote:
> 
> Hi Leo,
> 
> 
> sorry for the delay.
> 

Appreciate the review, it's not a small change by any means :)

> 
> On 2018-06-01 06:03 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>
>> This ended up being different enough from v2 to warrant a new patchset. Per
>> Michel's suggestions, there have been various optimizations and cleanups.
>> Here's what's changed:
>>
>> * Cache DRM color management property IDs at pre-init,
>>      * instead of querying DRM each time we need to modify color properties.
>>
>> * Remove drmmode_update_cm_props().
>>      * Update color properties in drmmode_output_get_property() instead.
>>      * This also makes old calls to update_cm_props redundant.
>>
>> * Get rid of fake CRTCs.
>>      * Previously, we were allocating a fake CRTC to configure color props on
>>        outputs that don't have a CRTC.
>>      * Instead, rr_configure_and_change_cm_property() can be easily modified to
>>        accept NULL CRTCs.
> 
> Is it currently ever the case in the hardware / kernel, or expected to
> ever be, that colour management is supported for some but not all CRTCs
> of a GPU?
> 

This was more of an effort to align with what DRM allows, which is
per-CRTC cm support and LUT sizes. I'm not aware of any current or
future hardware where this is the case though. It was a relatively
simple thing to implement, so I thought, might as well.

> If not, the LUT sizes could be tracked once instead of per CRTC, and at
> least the (DE)GAMMA_LUT_SIZE properties could always return the proper
> values, even if the output currently isn't associated with a CRTC.
>

My original take was that it's best to support what the framework
allows. But it does sound like this would have more utility to the
client, and simplify the code as well.

> 
> Other than that, I'm going to send some minor feedback on patches 2 & 3.
> If you prefer, I could fix up those and other cosmetic issues before
> pushing the patches.
> 

I can incorporate those along with the edit above.

Thanks!
Leo

> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Michel Dänzer]

On 2018-06-14 09:49 PM, Leo Li wrote:
> On 2018-06-14 12:57 PM, Michel Dänzer wrote:
>> On 2018-06-01 06:03 PM, sunpeng.li@amd.com wrote:
>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>
>>> This ended up being different enough from v2 to warrant a new
>>> patchset. Per
>>> Michel's suggestions, there have been various optimizations and
>>> cleanups.
>>> Here's what's changed:
>>>
>>> * Cache DRM color management property IDs at pre-init,
>>>      * instead of querying DRM each time we need to modify color
>>> properties.
>>>
>>> * Remove drmmode_update_cm_props().
>>>      * Update color properties in drmmode_output_get_property() instead.
>>>      * This also makes old calls to update_cm_props redundant.
>>>
>>> * Get rid of fake CRTCs.
>>>      * Previously, we were allocating a fake CRTC to configure color
>>> props on
>>>        outputs that don't have a CRTC.
>>>      * Instead, rr_configure_and_change_cm_property() can be easily
>>> modified to
>>>        accept NULL CRTCs.
>>
>> Is it currently ever the case in the hardware / kernel, or expected to
>> ever be, that colour management is supported for some but not all CRTCs
>> of a GPU?
>>
> 
> This was more of an effort to align with what DRM allows, which is
> per-CRTC cm support and LUT sizes. I'm not aware of any current or
> future hardware where this is the case though. It was a relatively
> simple thing to implement, so I thought, might as well.
> 
>> If not, the LUT sizes could be tracked once instead of per CRTC, and at
>> least the (DE)GAMMA_LUT_SIZE properties could always return the proper
>> values, even if the output currently isn't associated with a CRTC.
>>
> 
> My original take was that it's best to support what the framework
> allows. But it does sound like this would have more utility to the
> client, and simplify the code as well.

Right. In contrast to the generic modesetting driver, we can take
advantage of properties of our GPUs in this driver.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu v2 1/7] Cache color property IDs and LUT sizes during pre-init

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

DRM creates property types with unique IDs during kernel driver init.
Cache the color property IDs on DDX init for use later, when we need
to modify these properties. Also cache the (de)gamma LUT sizes, since
they are the same for all CRTCs on AMD hardware.

Since these values are the same regardless of the CRTC, they can be
cached within the private drmmode_rec object. We can also use any color-
management-enabled CRTC to initially fetch them.

Also introduce an enumeration of possible color management properties,
to provide a easy and unified way of referring to them.

v2:
- Reorder cm property enum so that LUT sizes are at the end. This allows
  us to use DEGAMMA_LUT_SIZE as an anchor for iterating over mutable cm
  properties.
- Cache (de)gamma LUT sizes within drmmode, since it's the same for all
  CRTCs on AMD hardware. Update commit message to reflect this.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 src/drmmode_display.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/drmmode_display.h | 22 ++++++++++++
 2 files changed, 120 insertions(+)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 8a1a201..994f130 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -746,6 +746,28 @@ drmmode_crtc_scanout_update(xf86CrtcPtr crtc, DisplayModePtr mode,
 	}
 }
 
+static char *cm_prop_names[] = {
+	"DEGAMMA_LUT",
+	"CTM",
+	"GAMMA_LUT",
+	"DEGAMMA_LUT_SIZE",
+	"GAMMA_LUT_SIZE",
+};
+
+/**
+ * Return the enum of the color management property with the given name.
+ */
+static enum drmmode_cm_prop get_cm_enum_from_str(const char *prop_name)
+{
+	enum drmmode_cm_prop ret;
+
+	for (ret = 0; ret < CM_NUM_PROPS; ret++) {
+		if (!strcmp(prop_name, cm_prop_names[ret]))
+			return ret;
+	}
+	return CM_INVALID_PROP;
+}
+
 static void
 drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t *red, uint16_t *green,
 			  uint16_t *blue, int size)
@@ -2413,6 +2435,80 @@ drmmode_page_flip_target_relative(AMDGPUEntPtr pAMDGPUEnt,
 				 drm_queue_seq);
 }
 
+/**
+ * Initialize DDX color management support. It does two things:
+ *
+ * 1. Cache DRM color management property type IDs, as they do not change. They
+ *    will be used later to modify color management via DRM, or to determine if
+ *    there's kernel support for color management.
+ *
+ * 2. Cache degamma/gamma LUT sizes, since all CRTCs have the same LUT sizes on
+ *    AMD hardware.
+ *
+ * If the cached ID's are all 0 after calling this function, then color
+ * management is not supported. For short, checking if the gamma LUT size
+ * property ID == 0 is sufficient.
+ *
+ * This should be called before CRTCs are initialized within pre_init, as the
+ * cached values will be used there.
+ *
+ * @drm_fd: DRM file descriptor
+ * @drmmode: drmmode object, where the cached IDs are stored
+ * @mode_res: The DRM mode resource containing the CRTC ids
+ */
+static void drmmode_cm_init(int drm_fd, drmmode_ptr drmmode,
+			    drmModeResPtr mode_res)
+{
+	drmModeObjectPropertiesPtr drm_props;
+	drmModePropertyPtr drm_prop;
+	enum drmmode_cm_prop cm_prop;
+	uint32_t cm_enabled = 0;
+	uint32_t cm_all_enabled = (1 << CM_NUM_PROPS) - 1;
+	int i;
+
+	memset(drmmode->cm_prop_ids, 0, sizeof(drmmode->cm_prop_ids));
+	drmmode->gamma_lut_size = drmmode->degamma_lut_size = 0;
+
+	/* AMD hardware has color management support on all pipes. It is
+	 * therefore sufficient to only check the first CRTC.
+	 */
+	drm_props = drmModeObjectGetProperties(drm_fd,
+					       mode_res->crtcs[0],
+					       DRM_MODE_OBJECT_CRTC);
+	if (!drm_props)
+		return;
+
+	for (i = 0; i < drm_props->count_props; i++) {
+		drm_prop = drmModeGetProperty(drm_fd,
+					      drm_props->props[i]);
+		if (!drm_prop)
+			continue;
+
+		cm_prop = get_cm_enum_from_str(drm_prop->name);
+		if (cm_prop == CM_INVALID_PROP)
+			continue;
+
+		if (cm_prop == CM_DEGAMMA_LUT_SIZE)
+			drmmode->degamma_lut_size = drm_props->prop_values[i];
+		else if (cm_prop == CM_GAMMA_LUT_SIZE)
+			drmmode->gamma_lut_size = drm_props->prop_values[i];
+
+		drmmode->cm_prop_ids[cm_prop] = drm_props->props[i];
+		cm_enabled |= 1 << cm_prop;
+
+		drmModeFreeProperty(drm_prop);
+	}
+	drmModeFreeObjectProperties(drm_props);
+
+	/* cm is enabled only if all prop ids are found */
+	if (cm_enabled == cm_all_enabled)
+		return;
+
+	/* Otherwise, disable DDX cm support */
+	memset(drmmode->cm_prop_ids, 0, sizeof(drmmode->cm_prop_ids));
+	drmmode->gamma_lut_size = drmmode->degamma_lut_size = 0;
+}
+
 Bool drmmode_pre_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, int cpp)
 {
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(pScrn);
@@ -2459,6 +2555,8 @@ Bool drmmode_pre_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, int cpp)
 	if (pScrn->depth == 30)
 		info->drmmode_crtc_funcs.gamma_set = NULL;
 
+	drmmode_cm_init(pAMDGPUEnt->fd, drmmode, mode_res);
+
 	for (i = 0; i < mode_res->count_crtcs; i++)
 		if (!xf86IsEntityShared(pScrn->entityList[0]) ||
 		    (crtcs_needed && !(pAMDGPUEnt->assigned_crtcs & (1 << i))))
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index 25ae9f8..4aa8e88 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -36,6 +36,22 @@
 #include "amdgpu_probe.h"
 #include "amdgpu.h"
 
+/*
+ * Enum of non-legacy color management properties, according to DRM. Note that
+ * the values should be incremental (with the exception of the INVALID member),
+ * as defined by C99. The ordering also matters. Some logics (such as iterators
+ * and bitmasks) depend on these facts.
+ */
+enum drmmode_cm_prop {
+	CM_DEGAMMA_LUT,
+	CM_CTM,
+	CM_GAMMA_LUT,
+	CM_DEGAMMA_LUT_SIZE,
+	CM_GAMMA_LUT_SIZE,
+	CM_NUM_PROPS,
+	CM_INVALID_PROP = -1,
+};
+
 typedef struct {
 	ScrnInfoPtr scrn;
 #ifdef HAVE_LIBUDEV
@@ -49,6 +65,12 @@ typedef struct {
 
 	Bool dri2_flipping;
 	Bool present_flipping;
+
+	/* Cache for DRM property type IDs for CRTC color management */
+	uint32_t cm_prop_ids[CM_NUM_PROPS];
+	/* Lookup table sizes */
+	uint32_t degamma_lut_size;
+	uint32_t gamma_lut_size;
 } drmmode_rec, *drmmode_ptr;
 
 typedef struct {
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu v2 2/7] Initialize color properties on CRTC during CRTC init

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

And destroy them on the CRTC destroy hook.

When initializing color management properties on the private
drmmode_crtc, we want to:

1. Default its color transform matrix (CTM) to identity
2. Program hardware with default color management values (SRGB for
   de/regamma, identity for CTM)

It's possible that cm initialization fails due to memory error or DRM
error. In which case, the RandR state may not reflect the actual
hardware state.

v2:
- Use switch statement in push_cm_prop
- Get rid of per-CRTC cm support checks. Keep it simple and only check
  the first CRTC, since kernel will always report all or nothing for AMD
  hardware.
- Remove per-CRTC LUT size caching, drmmode now holds that. Update
  commit message to reflect this.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 src/drmmode_display.c | 147 +++++++++++++++++++++++++++++++++++++++++++++++++-
 src/drmmode_display.h |   3 ++
 2 files changed, 149 insertions(+), 1 deletion(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 994f130..d750410 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -768,6 +768,89 @@ static enum drmmode_cm_prop get_cm_enum_from_str(const char *prop_name)
 	return CM_INVALID_PROP;
 }
 
+/**
+ * Return TRUE if kernel supports non-legacy color management.
+ */
+static Bool drmmode_cm_enabled(drmmode_ptr drmmode)
+{
+	return drmmode->cm_prop_ids[CM_GAMMA_LUT_SIZE] != 0;
+}
+
+/**
+ * Push staged color management properties on the CRTC to DRM.
+ *
+ * @crtc: The CRTC containing staged properties
+ * @cm_prop_index: The color property to push
+ *
+ * Return 0 on success, X-defined error codes on failure.
+ */
+static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
+				     enum drmmode_cm_prop cm_prop_index)
+{
+	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+	drmmode_ptr drmmode = drmmode_crtc->drmmode;
+	uint32_t created_blob_id = 0;
+	uint32_t drm_prop_id;
+	size_t expected_bytes = 0;
+	void *blob_data = NULL;
+	int ret;
+
+	switch (cm_prop_index) {
+	case CM_GAMMA_LUT:
+		/* Calculate the expected size of value in bytes */
+		expected_bytes = sizeof(struct drm_color_lut) *
+					drmmode->gamma_lut_size;
+		blob_data = drmmode_crtc->gamma_lut;
+		break;
+	case CM_DEGAMMA_LUT:
+		expected_bytes = sizeof(struct drm_color_lut) *
+					drmmode->degamma_lut_size;
+		blob_data = drmmode_crtc->degamma_lut;
+		break;
+	case CM_CTM:
+		expected_bytes = sizeof(struct drm_color_ctm);
+		blob_data = drmmode_crtc->ctm;
+		break;
+	default:
+		return BadName;
+	}
+
+	if (blob_data) {
+		ret = drmModeCreatePropertyBlob(pAMDGPUEnt->fd,
+						blob_data, expected_bytes,
+						&created_blob_id);
+		if (ret) {
+			xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
+				   "Creating DRM blob failed with errno %d\n",
+				   ret);
+			return BadRequest;
+		}
+	}
+
+	drm_prop_id = drmmode_crtc->drmmode->cm_prop_ids[cm_prop_index];
+	ret = drmModeObjectSetProperty(pAMDGPUEnt->fd,
+				       drmmode_crtc->mode_crtc->crtc_id,
+				       DRM_MODE_OBJECT_CRTC,
+				       drm_prop_id,
+				       (uint64_t)created_blob_id);
+
+	/* If successful, kernel will have a reference already. Safe to destroy
+	 * the blob either way.
+	 */
+	if (blob_data)
+		drmModeDestroyPropertyBlob(pAMDGPUEnt->fd, created_blob_id);
+
+	if (ret) {
+		xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
+			   "Setting DRM property blob failed with errno %d\n",
+			   ret);
+		return BadRequest;
+	}
+
+	return Success;
+}
+
 static void
 drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t *red, uint16_t *green,
 			  uint16_t *blue, int size)
@@ -1314,6 +1397,22 @@ static Bool drmmode_set_scanout_pixmap(xf86CrtcPtr crtc, PixmapPtr ppix)
 	return TRUE;
 }
 
+static void drmmode_crtc_destroy(xf86CrtcPtr crtc)
+{
+	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+
+	drmModeFreeCrtc(drmmode_crtc->mode_crtc);
+
+	/* Free LUTs and CTM */
+	free(drmmode_crtc->gamma_lut);
+	free(drmmode_crtc->degamma_lut);
+	free(drmmode_crtc->ctm);
+
+	free(drmmode_crtc);
+	crtc->driver_private = NULL;
+}
+
+
 static xf86CrtcFuncsRec drmmode_crtc_funcs = {
 	.dpms = drmmode_crtc_dpms,
 	.set_mode_major = drmmode_set_mode_major,
@@ -1330,7 +1429,7 @@ static xf86CrtcFuncsRec drmmode_crtc_funcs = {
 	.shadow_create = drmmode_crtc_shadow_create,
 	.shadow_allocate = drmmode_crtc_shadow_allocate,
 	.shadow_destroy = drmmode_crtc_shadow_destroy,
-	.destroy = NULL,	/* XXX */
+	.destroy = drmmode_crtc_destroy,
 	.set_scanout_pixmap = drmmode_set_scanout_pixmap,
 };
 
@@ -1354,6 +1453,50 @@ void drmmode_crtc_hw_id(xf86CrtcPtr crtc)
 		drmmode_crtc->hw_id = -1;
 }
 
+/**
+ * Initialize color management properties for the given CRTC by programming
+ * the default gamma/degamma LUTs and CTM.
+ *
+ * If the CRTC does not support color management, or if errors occur during
+ * initialization, all color properties on the driver-private CRTC will left
+ * as NULL.
+ *
+ * @drm_fd: DRM file descriptor
+ * @crtc: CRTC to initialize color management on.
+ */
+static void drmmode_crtc_cm_init(int drm_fd, xf86CrtcPtr crtc)
+{
+	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+	drmmode_ptr drmmode = drmmode_crtc->drmmode;
+	int i;
+
+	if (!drmmode_cm_enabled(drmmode))
+		return;
+
+	/* Init CTM to identity. Values are in S31.32 fixed-point format */
+	drmmode_crtc->ctm = calloc(1, sizeof(*drmmode_crtc->ctm));
+	if (drmmode_crtc->ctm == NULL) {
+		xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
+			   "Memory error initializing CTM for CRTC%d",
+			   drmmode_get_crtc_id(crtc));
+		return;
+	}
+
+	drmmode_crtc->ctm->matrix[0] = drmmode_crtc->ctm->matrix[4] =
+		drmmode_crtc->ctm->matrix[8] = (uint64_t)1 << 32;
+
+	/* Push properties to reset properties currently in hardware */
+	for (i = 0; i < CM_DEGAMMA_LUT_SIZE; i++) {
+		if (drmmode_crtc_push_cm_prop(crtc, i))
+			xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
+				   "Failed to initialize color management "
+				   "property %s on CRTC%d. Property value may "
+				   "not reflect actual hardware state.\n",
+				   cm_prop_names[i],
+				   drmmode_get_crtc_id(crtc));
+	}
+}
+
 static unsigned int
 drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, drmModeResPtr mode_res, int num)
 {
@@ -1374,6 +1517,8 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, drmModeResPtr mode_res
 	crtc->driver_private = drmmode_crtc;
 	drmmode_crtc_hw_id(crtc);
 
+	drmmode_crtc_cm_init(pAMDGPUEnt->fd, crtc);
+
 	/* Mark num'th crtc as in use on this device. */
 	pAMDGPUEnt->assigned_crtcs |= (1 << num);
 	xf86DrvMsgVerb(pScrn->scrnIndex, X_INFO, AMDGPU_LOGLEVEL_DEBUG,
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index 4aa8e88..0f0227c 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -131,6 +131,9 @@ typedef struct {
 	unsigned present_vblank_msc;
 	Bool present_flip_expected;
 #endif
+	struct drm_color_lut *degamma_lut;
+	struct drm_color_ctm *ctm;
+	struct drm_color_lut *gamma_lut;
 } drmmode_crtc_private_rec, *drmmode_crtc_private_ptr;
 
 typedef struct {
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu v2 3/7] Configure color properties when creating output resources

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

List color management properties on outputs if there is kernel support.
Otherwise, don't list them at all.

v2:
- Use switch statement in configure_and_change
- Also configure LUT sizes for outputs that don't have an attached CRTC.
  We can do this since LUT sizes are now cached on the drmmode object.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 src/drmmode_display.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 131 insertions(+)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index d750410..42e8ba3 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -777,6 +777,127 @@ static Bool drmmode_cm_enabled(drmmode_ptr drmmode)
 }
 
 /**
+ * Configure and change a color property on a CRTC, through RandR. Only the
+ * specified output will be affected, even if the CRTC is attached to multiple
+ * outputs. Note that changes will be non-pending: the changes won't be pushed
+ * to kernel driver.
+ *
+ * @output: RandR output to set the property on.
+ * @crtc: The driver-private CRTC object containing the color properties.
+ *        If this is NULL, "disabled" values of 0 will be used.
+ * @cm_prop_index: Color management property to configure and change.
+ *
+ * Return 0 on success, X-defined error code otherwise.
+ */
+static int rr_configure_and_change_cm_property(xf86OutputPtr output,
+					       drmmode_crtc_private_ptr crtc,
+					       enum drmmode_cm_prop cm_prop_index)
+{
+	drmmode_output_private_ptr drmmode_output = output->driver_private;
+	drmmode_ptr drmmode = drmmode_output->drmmode;
+	Bool need_configure = TRUE;
+	unsigned long length = 0;
+	const void *data = NULL;
+	int format = 0;
+	uint32_t zero = 0;
+	INT32 range[2];
+	Atom atom;
+	int err;
+
+	if (cm_prop_index == CM_INVALID_PROP)
+		return BadName;
+
+	switch(cm_prop_index) {
+	case CM_GAMMA_LUT_SIZE:
+		format = 32;
+		length = 1;
+		data = &drmmode->gamma_lut_size;
+		range[0] = 0;
+		range[1] = -1;
+		break;
+	case CM_DEGAMMA_LUT_SIZE:
+		format = 32;
+		length = 1;
+		data = &drmmode->degamma_lut_size;
+		range[0] = 0;
+		range[1] = -1;
+		break;
+	case CM_GAMMA_LUT:
+		format = 16;
+		range[0] = 0;
+		range[1] = (1 << 16) - 1; // Max 16 bit unsigned int.
+		if (crtc && crtc->gamma_lut) {
+			/* Convert from 8bit size to 16bit size */
+			length = sizeof(*crtc->gamma_lut) >> 1;
+			length *= drmmode->gamma_lut_size;
+			data = crtc->gamma_lut;
+		} else {
+			length = 1;
+			data = &zero;
+		}
+		break;
+	case CM_DEGAMMA_LUT:
+		format = 16;
+		range[0] = 0;
+		range[1] = (1 << 16) - 1;
+		if (crtc && crtc->degamma_lut) {
+			length = sizeof(*crtc->degamma_lut) >> 1;
+			length *= drmmode->degamma_lut_size;
+			data = crtc->degamma_lut;
+		} else {
+			length = 1;
+			data = &zero;
+		}
+		break;
+	case CM_CTM:
+		/* CTM is fixed-point S31.32 format. */
+		format = 32;
+		need_configure = FALSE;
+		if (crtc && crtc->ctm) {
+			/* Convert from 8bit size to 32bit size */
+			length = sizeof(*crtc->ctm) >> 2;
+			data = crtc->ctm;
+		} else {
+			length = 1;
+			data = &zero;
+		}
+		break;
+	default:
+		return BadName;
+	}
+
+	atom = MakeAtom(cm_prop_names[cm_prop_index],
+			strlen(cm_prop_names[cm_prop_index]),
+			TRUE);
+	if (!atom)
+		return BadAlloc;
+
+	if (need_configure) {
+		err = RRConfigureOutputProperty(output->randr_output, atom,
+						FALSE, TRUE, FALSE, 2, range);
+		if (err) {
+			xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+				   "Configuring color management property %s failed with %d\n",
+				   cm_prop_names[cm_prop_index], err);
+			return err;
+		}
+	}
+
+	/* Always issue a non-pending change. We'll push cm properties
+	 * ourselves.
+	 */
+	err = RRChangeOutputProperty(output->randr_output, atom,
+				     XA_INTEGER, format,
+				     PropModeReplace,
+				     length, data, FALSE, FALSE);
+	if (err)
+		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+			   "Changing color management property %s failed with %d\n",
+			   cm_prop_names[cm_prop_index], err);
+	return err;
+}
+
+/**
  * Push staged color management properties on the CRTC to DRM.
  *
  * @crtc: The CRTC containing staged properties
@@ -1780,6 +1901,7 @@ static void drmmode_output_create_resources(xf86OutputPtr output)
 {
 	AMDGPUInfoPtr info = AMDGPUPTR(output->scrn);
 	drmmode_output_private_ptr drmmode_output = output->driver_private;
+	drmmode_crtc_private_ptr drmmode_crtc;
 	drmModeConnectorPtr mode_output = drmmode_output->mode_output;
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(output->scrn);
 	drmModePropertyPtr drmmode_prop, tearfree_prop;
@@ -1903,6 +2025,15 @@ static void drmmode_output_create_resources(xf86OutputPtr output)
 			}
 		}
 	}
+
+	/* Do not configure cm properties on output if there's no support. */
+	if (!drmmode_cm_enabled(drmmode_output->drmmode))
+		return;
+
+	drmmode_crtc = output->crtc ? output->crtc->driver_private : NULL;
+
+	for (i = 0; i < CM_NUM_PROPS; i++)
+		rr_configure_and_change_cm_property(output, drmmode_crtc, i);
 }
 
 static void
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu v2 4/7] Update color properties on output_get_property

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

Notify RandR of any updated color properties on the output's CRTC when
its get_property() hook is called.

v2: Remove per-CRTC check for color management support.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 src/drmmode_display.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 42e8ba3..a7d532c 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -2125,6 +2125,27 @@ drmmode_output_set_property(xf86OutputPtr output, Atom property,
 
 static Bool drmmode_output_get_property(xf86OutputPtr output, Atom property)
 {
+	drmmode_crtc_private_ptr drmmode_crtc;
+	enum drmmode_cm_prop cm_prop_id;
+	int ret;
+
+	/* First, see if it's a cm property */
+	cm_prop_id = get_cm_enum_from_str(NameForAtom(property));
+	if (output->crtc && cm_prop_id != CM_INVALID_PROP) {
+		drmmode_crtc = output->crtc->driver_private;
+
+		ret = rr_configure_and_change_cm_property(output, drmmode_crtc,
+							  cm_prop_id);
+		if (ret) {
+			xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+				   "Error getting color property: %d\n",
+				   ret);
+			return FALSE;
+		}
+		return TRUE;
+	}
+
+	/* Otherwise, must be an output property. */
 	return TRUE;
 }
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu v2 5/7] Enable setting of color properties via RandR

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

Setting a color property involves:
1. Staging the property onto the driver-private CRTC object
2. Pushing the staged property into kernel DRM, for HW programming

Add a function to do the staging, and execute the above steps in
output_property_set.

v2:
- Remove per-CRTC check for color management support in stage_cm_prop.
- Use switch statement instead of if statements.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 src/drmmode_display.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index a7d532c..d156398 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -898,6 +898,88 @@ static int rr_configure_and_change_cm_property(xf86OutputPtr output,
 }
 
 /**
+* Stage a color management property. This parses the property value, according
+* to the cm property type, then stores it within the driver-private CRTC
+* object.
+*
+* @crtc: The CRTC to stage the new color management properties in
+* @cm_prop_index: The color property to stage
+* @value: The RandR property value to stage
+*
+* Return 0 on success, X-defined error code on failure.
+*/
+static int drmmode_crtc_stage_cm_prop(xf86CrtcPtr crtc,
+				      enum drmmode_cm_prop cm_prop_index,
+				      RRPropertyValuePtr value)
+{
+	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+	drmmode_ptr drmmode = drmmode_crtc->drmmode;
+	size_t expected_bytes = 0;
+	void **blob_data = NULL;
+	Bool use_default = FALSE;
+
+	/* Update properties on the driver-private CRTC */
+	switch (cm_prop_index) {
+	case CM_GAMMA_LUT:
+		/* Calculate the expected size of value in bytes */
+		expected_bytes = sizeof(struct drm_color_lut) *
+					drmmode->gamma_lut_size;
+
+		/* For gamma and degamma, we allow a default SRGB curve to be
+		 * set via setting a single element
+		 *
+		 * Otherwise, value size is in terms of the value format.
+		 * Ensure it's also in bytes (<< 1) before comparing with the
+		 * expected bytes.
+		 */
+		if (value->size == 1)
+			use_default = TRUE;
+		else if (value->type != XA_INTEGER || value->format != 16 ||
+			 (size_t)(value->size << 1) != expected_bytes)
+			return BadLength;
+
+		blob_data = (void**)&drmmode_crtc->gamma_lut;
+		break;
+	case CM_DEGAMMA_LUT:
+		expected_bytes = sizeof(struct drm_color_lut) *
+					drmmode->degamma_lut_size;
+
+		if (value->size == 1)
+			use_default = TRUE;
+		else if (value->type != XA_INTEGER || value->format != 16 ||
+			 (size_t)(value->size << 1) != expected_bytes)
+			return BadLength;
+
+		blob_data = (void**)&drmmode_crtc->degamma_lut;
+		break;
+	case CM_CTM:
+		expected_bytes = sizeof(struct drm_color_ctm);
+
+		if (value->size == 1)
+			use_default = TRUE;
+		if (value->type != XA_INTEGER || value->format != 32 ||
+		    (size_t)(value->size << 2) != expected_bytes)
+			return BadLength;
+
+		blob_data = (void**)&drmmode_crtc->ctm;
+		break;
+	default:
+		return BadName;
+	}
+
+	free(*blob_data);
+	if (!use_default) {
+		*blob_data = malloc(expected_bytes);
+		if (!*blob_data)
+			return BadAlloc;
+		memcpy(*blob_data, value->data, expected_bytes);
+	} else
+		*blob_data = NULL;
+
+	return Success;
+}
+
+/**
  * Push staged color management properties on the CRTC to DRM.
  *
  * @crtc: The CRTC containing staged properties
@@ -2067,8 +2149,21 @@ drmmode_output_set_property(xf86OutputPtr output, Atom property,
 {
 	drmmode_output_private_ptr drmmode_output = output->driver_private;
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(output->scrn);
+	enum drmmode_cm_prop cm_prop_index;
 	int i;
 
+	cm_prop_index = get_cm_enum_from_str(NameForAtom(property));
+	if (cm_prop_index >= 0 && cm_prop_index < CM_DEGAMMA_LUT_SIZE) {
+		if (!output->crtc)
+			return FALSE;
+		if (drmmode_crtc_stage_cm_prop(output->crtc, cm_prop_index,
+					       value))
+			return FALSE;
+		if (drmmode_crtc_push_cm_prop(output->crtc, cm_prop_index))
+			return FALSE;
+		return TRUE;
+	}
+
 	for (i = 0; i < drmmode_output->num_props; i++) {
 		drmmode_prop_ptr p = &drmmode_output->props[i];
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu v2 7/7] Also compose LUT when setting legacy gamma

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

We compose the two LUTs when pushing non-legacy gamma changes, and the
same needs to be done when setting legacy gamma.

To do so, we just call push_cm_prop() on the gamma LUT. It will compose
the LUTs for us, and fall back to using legacy LUT (upscaled to non-
legacy size) if non-legacy is unavailable.

It's also possible that the Kernel has no support support for non-
legacy color. In which case, we fall back to legacy gamma.

v2: Remove per-CRTC check for color management support.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 src/drmmode_display.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 8f3f3bc..17b5dbc 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -1245,9 +1245,21 @@ drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t *red, uint16_t *green,
 {
 	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+	int ret;
+
+	/* Use legacy if no support for non-legacy gamma */
+	if (!drmmode_cm_enabled(drmmode_crtc->drmmode)) {
+		drmModeCrtcSetGamma(pAMDGPUEnt->fd,
+				    drmmode_crtc->mode_crtc->crtc_id,
+				    size, red, green, blue);
+		return;
+	}
 
-	drmModeCrtcSetGamma(pAMDGPUEnt->fd, drmmode_crtc->mode_crtc->crtc_id,
-			    size, red, green, blue);
+	ret = drmmode_crtc_push_cm_prop(crtc, CM_GAMMA_LUT);
+	if (ret)
+		xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
+			   "Setting Gamma LUT failed with errno %d\n",
+			   ret);
 }
 
 Bool
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu v3 6/7] Compose non-legacy with legacy regamma LUT

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

Frequently, a user may have non-legacy gamma enabled for monitor
correction, while using legacy gamma for things like
redshift/nightlight.

To do so, we compose the two LUTs. Legacy gamma will be applied first,
then non-legacy. i.e. non-legacy_LUT(legacy_LUT(in_color)).

Note that the staged gamma LUT within the driver-private CRTC will
always contain the non-legacy LUT. This is to ensure that we have a
cached copy for future compositions.

v2: Don't compose LUTs if legacy gamma is disabled (which is the case
    for deep 30bpp color). The legacy LUT won't be computed here,
    causing composition to spit out something invalid.

v3: Use LUT sizes that are now cached in drmmode.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 src/drmmode_display.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 186 insertions(+), 1 deletion(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index d156398..8f3f3bc 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -777,6 +777,150 @@ static Bool drmmode_cm_enabled(drmmode_ptr drmmode)
 }
 
 /**
+ * If legacy LUT is a, and non-legacy LUT is b, then the result of b(a(x)) is
+ * returned in out_lut. out_lut's length is expected to be the same as the
+ * non-legacy LUT b.
+ *
+ * @a_(red|green|blue): The red, green, and blue components of the legacy LUT.
+ * @b_lut: The non-legacy LUT, in DRM's color LUT format.
+ * @out_lut: The composed LUT, in DRM's color LUT format.
+ * @len_a: Length of legacy lut.
+ * @len_b: Length of non-legacy lut.
+ */
+static void drmmode_lut_compose(uint16_t *a_red,
+				uint16_t *a_green,
+				uint16_t *a_blue,
+				struct drm_color_lut *b_lut,
+				struct drm_color_lut *out_lut,
+				uint32_t len_a, uint32_t len_b)
+{
+	uint32_t i_l, i_r, i;
+	uint32_t i_amax, i_bmax;
+	uint32_t coeff_ibmax;
+	uint32_t j;
+	uint64_t a_out_ibmax;
+	int color;
+	size_t struct_size = sizeof(struct drm_color_lut);
+
+	uint32_t max_lut = (1 << 16) - 1;
+
+	i_amax = len_a - 1;
+	i_bmax = len_b - 1;
+
+	/* A linear interpolation is done on the legacy LUT before it is
+	 * composed, to bring it up-to-size with the non-legacy LUT. The
+	 * interpolation uses integers by keeping things multiplied until the
+	 * last moment.
+	 */
+	for (color = 0; color < 3; color++) {
+		uint16_t *a, *b, *out;
+
+		/* Set the initial pointers to the right color components. The
+		 * inner for-loop will then maintain the correct offset from
+		 * the initial element.
+		 */
+		if (color == 0) {
+			a = a_red;
+			b = &b_lut[0].red;
+			out = &out_lut[0].red;
+		} else if (color == 1) {
+			a = a_green;
+			b = &b_lut[0].green;
+			out = &out_lut[0].green;
+		} else {
+			a = a_blue;
+			b = &b_lut[0].blue;
+			out = &out_lut[0].blue;
+		}
+
+		for (i = 0; i < len_b; i++) {
+			/* i_l and i_r tracks the left and right elements in
+			 * a_lut, to the sample point i. Also handle last
+			 * element edge case, when i_l = i_amax.
+			 */
+			i_l = i * i_amax / i_bmax;
+			i_r = i_l + !!(i_amax - i_l);
+
+			/* coeff is intended to be in [0, 1), depending on
+			 * where sample i is between i_l and i_r. We keep it
+			 * multiplied with i_bmax throughout to maintain
+			 * precision */
+			coeff_ibmax = (i * i_amax) - (i_l * i_bmax);
+			a_out_ibmax = i_bmax * a[i_l] +
+				      coeff_ibmax * (a[i_r] - a[i_l]);
+
+			/* j = floor((a_out/max_lut)*i_bmax).
+			 * i.e. the element in LUT b that a_out maps to. We
+			 * have to divide by max_lut to normalize a_out, since
+			 * values in the LUTs are [0, 1<<16)
+			 */
+			j = a_out_ibmax / max_lut;
+			*(uint16_t*)((void*)out + (i*struct_size)) =
+				*(uint16_t*)((void*)b + (j*struct_size));
+		}
+	}
+
+	for (i = 0; i < len_b; i++)
+		out_lut[i].reserved = 0;
+}
+
+/**
+ * Resize a LUT, using linear interpolation.
+ *
+ * @in_(red|green|blue): Legacy LUT components
+ * @out_lut: The resized LUT is returned here, in DRM color LUT format.
+ * @len_in: Length of legacy LUT.
+ * @len_out: Length of out_lut, i.e. the target size.
+ */
+static void drmmode_lut_interpolate(uint16_t *in_red,
+				    uint16_t *in_green,
+				    uint16_t *in_blue,
+				    struct drm_color_lut *out_lut,
+				    uint32_t len_in, uint32_t len_out)
+{
+	uint32_t i_l, i_r, i;
+	uint32_t i_amax, i_bmax;
+	uint32_t coeff_ibmax;
+	uint64_t out_ibmax;
+	int color;
+	size_t struct_size = sizeof(struct drm_color_lut);
+
+	i_amax = len_in - 1;
+	i_bmax = len_out - 1;
+
+	/* See @drmmode_lut_compose for details */
+	for (color = 0; color < 3; color++) {
+		uint16_t *in, *out;
+
+		if (color == 0) {
+			in = in_red;
+			out = &out_lut[0].red;
+		} else if (color == 1) {
+			in = in_green;
+			out = &out_lut[0].green;
+		} else {
+			in = in_blue;
+			out = &out_lut[0].blue;
+		}
+
+		for (i = 0; i < len_out; i++) {
+			i_l = i * i_amax / i_bmax;
+			i_r = i_l + !!(i_amax - i_l);
+
+			coeff_ibmax = (i * i_amax) - (i_l * i_bmax);
+			out_ibmax = i_bmax * in[i_l] +
+				      coeff_ibmax * (in[i_r] - in[i_l]);
+
+			*(uint16_t*)((void*)out + (i*struct_size)) =
+				out_ibmax / i_bmax;
+		}
+	}
+
+	for (i = 0; i < len_out; i++)
+		out_lut[i].reserved = 0;
+}
+
+/**
  * Configure and change a color property on a CRTC, through RandR. Only the
  * specified output will be affected, even if the CRTC is attached to multiple
  * outputs. Note that changes will be non-pending: the changes won't be pushed
@@ -993,6 +1137,7 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
 	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
 	drmmode_ptr drmmode = drmmode_crtc->drmmode;
+	Bool free_blob_data = FALSE;
 	uint32_t created_blob_id = 0;
 	uint32_t drm_prop_id;
 	size_t expected_bytes = 0;
@@ -1004,7 +1149,39 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
 		/* Calculate the expected size of value in bytes */
 		expected_bytes = sizeof(struct drm_color_lut) *
 					drmmode->gamma_lut_size;
-		blob_data = drmmode_crtc->gamma_lut;
+
+		/* Legacy gamma LUT is disabled on deep 30bpp color. In which
+		 * case, directly use non-legacy LUT.
+		 */
+		if (crtc->funcs->gamma_set == NULL) {
+			blob_data = drmmode_crtc->gamma_lut;
+			goto do_push;
+		}
+
+		blob_data = malloc(expected_bytes);
+		if (!blob_data)
+			return BadAlloc;
+
+		free_blob_data = TRUE;
+		/*
+		 * Compose legacy and non-legacy LUT if non-legacy was set.
+		 * Otherwise, interpolate legacy LUT to non-legacy size.
+		 */
+		if (drmmode_crtc->gamma_lut) {
+			drmmode_lut_compose(crtc->gamma_red,
+					    crtc->gamma_green,
+					    crtc->gamma_blue,
+					    drmmode_crtc->gamma_lut,
+					    blob_data, crtc->gamma_size,
+					    drmmode->gamma_lut_size);
+		} else {
+			drmmode_lut_interpolate(crtc->gamma_red,
+						crtc->gamma_green,
+						crtc->gamma_blue,
+						blob_data,
+						crtc->gamma_size,
+						drmmode->gamma_lut_size);
+		}
 		break;
 	case CM_DEGAMMA_LUT:
 		expected_bytes = sizeof(struct drm_color_lut) *
@@ -1019,6 +1196,7 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
 		return BadName;
 	}
 
+do_push:
 	if (blob_data) {
 		ret = drmModeCreatePropertyBlob(pAMDGPUEnt->fd,
 						blob_data, expected_bytes,
@@ -1027,6 +1205,8 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
 			xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
 				   "Creating DRM blob failed with errno %d\n",
 				   ret);
+			if (free_blob_data)
+				free(blob_data);
 			return BadRequest;
 		}
 	}
@@ -1048,9 +1228,14 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
 		xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
 			   "Setting DRM property blob failed with errno %d\n",
 			   ret);
+		if (free_blob_data)
+			free(blob_data);
 		return BadRequest;
 	}
 
+	if (free_blob_data)
+		free(blob_data);
+
 	return Success;
 }
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Leo Li]

On 2018-06-15 02:57 AM, Michel Dänzer wrote:
> On 2018-06-14 09:49 PM, Leo Li wrote:
>> On 2018-06-14 12:57 PM, Michel Dänzer wrote:
>>> On 2018-06-01 06:03 PM, sunpeng.li@amd.com wrote:
>>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>>
>>>> This ended up being different enough from v2 to warrant a new
>>>> patchset. Per
>>>> Michel's suggestions, there have been various optimizations and
>>>> cleanups.
>>>> Here's what's changed:
>>>>
>>>> * Cache DRM color management property IDs at pre-init,
>>>>       * instead of querying DRM each time we need to modify color
>>>> properties.
>>>>
>>>> * Remove drmmode_update_cm_props().
>>>>       * Update color properties in drmmode_output_get_property() instead.
>>>>       * This also makes old calls to update_cm_props redundant.
>>>>
>>>> * Get rid of fake CRTCs.
>>>>       * Previously, we were allocating a fake CRTC to configure color
>>>> props on
>>>>         outputs that don't have a CRTC.
>>>>       * Instead, rr_configure_and_change_cm_property() can be easily
>>>> modified to
>>>>         accept NULL CRTCs.
>>>
>>> Is it currently ever the case in the hardware / kernel, or expected to
>>> ever be, that colour management is supported for some but not all CRTCs
>>> of a GPU?
>>>
>>
>> This was more of an effort to align with what DRM allows, which is
>> per-CRTC cm support and LUT sizes. I'm not aware of any current or
>> future hardware where this is the case though. It was a relatively
>> simple thing to implement, so I thought, might as well.
>>
>>> If not, the LUT sizes could be tracked once instead of per CRTC, and at
>>> least the (DE)GAMMA_LUT_SIZE properties could always return the proper
>>> values, even if the output currently isn't associated with a CRTC.
>>>
>>
>> My original take was that it's best to support what the framework
>> allows. But it does sound like this would have more utility to the
>> client, and simplify the code as well.
> 
> Right. In contrast to the generic modesetting driver, we can take
> advantage of properties of our GPUs in this driver.
> 

Ping!

FYI, all the new patches are v2, with the exception of 6/7, which is a
v3. (On second thought, should have started a new thread :) )

Leo

> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Michel Dänzer]

On 2018-06-22 03:41 PM, Leo Li wrote:
> On 2018-06-15 02:57 AM, Michel Dänzer wrote:
>> On 2018-06-14 09:49 PM, Leo Li wrote:
>>> On 2018-06-14 12:57 PM, Michel Dänzer wrote:
>>>> On 2018-06-01 06:03 PM, sunpeng.li@amd.com wrote:
>>>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>>>
>>>>> This ended up being different enough from v2 to warrant a new
>>>>> patchset. Per
>>>>> Michel's suggestions, there have been various optimizations and
>>>>> cleanups.
>>>>> Here's what's changed:
>>>>>
>>>>> * Cache DRM color management property IDs at pre-init,
>>>>>       * instead of querying DRM each time we need to modify color
>>>>> properties.
>>>>>
>>>>> * Remove drmmode_update_cm_props().
>>>>>       * Update color properties in drmmode_output_get_property()
>>>>> instead.
>>>>>       * This also makes old calls to update_cm_props redundant.
>>>>>
>>>>> * Get rid of fake CRTCs.
>>>>>       * Previously, we were allocating a fake CRTC to configure color
>>>>> props on
>>>>>         outputs that don't have a CRTC.
>>>>>       * Instead, rr_configure_and_change_cm_property() can be easily
>>>>> modified to
>>>>>         accept NULL CRTCs.
>>>>
>>>> Is it currently ever the case in the hardware / kernel, or expected to
>>>> ever be, that colour management is supported for some but not all CRTCs
>>>> of a GPU?
>>>>
>>>
>>> This was more of an effort to align with what DRM allows, which is
>>> per-CRTC cm support and LUT sizes. I'm not aware of any current or
>>> future hardware where this is the case though. It was a relatively
>>> simple thing to implement, so I thought, might as well.
>>>
>>>> If not, the LUT sizes could be tracked once instead of per CRTC, and at
>>>> least the (DE)GAMMA_LUT_SIZE properties could always return the proper
>>>> values, even if the output currently isn't associated with a CRTC.
>>>>
>>>
>>> My original take was that it's best to support what the framework
>>> allows. But it does sound like this would have more utility to the
>>> client, and simplify the code as well.
>>
>> Right. In contrast to the generic modesetting driver, we can take
>> advantage of properties of our GPUs in this driver.
>>
> 
> Ping!
> 
> FYI, all the new patches are v2, with the exception of 6/7, which is a
> v3. (On second thought, should have started a new thread :) )

Don't worry, I won't forget about this. :) Just been busy with other
stuff, sorry.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Michel Dänzer]

On 2018-06-22 03:41 PM, Leo Li wrote:
> 
> Ping!
> 
> FYI, all the new patches are v2, with the exception of 6/7, which is a
> v3. (On second thought, should have started a new thread :) )

I've pushed the changes (with some minor fix-ups), thanks and sorry it
took so long!


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

[Harry Wentland]

On 2018-06-26 11:46 AM, Michel Dänzer wrote:
> On 2018-06-22 03:41 PM, Leo Li wrote:
>>
>> Ping!
>>
>> FYI, all the new patches are v2, with the exception of 6/7, which is a
>> v3. (On second thought, should have started a new thread :) )
> 
> I've pushed the changes (with some minor fix-ups), thanks and sorry it
> took so long!
> 

Thanks Michel and thanks for your time to review this and make sure we get it right.

Harry

> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx