[PATCH xf86-video-amdgpu 0/5] Implementing non-legacy color management

[PATCH xf86-video-amdgpu 0/5] Implementing non-legacy color management

[sunpeng.li-5C7GfCeVMHo]

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

These patches will enable modification of non-legacy color management
properties via xrandr.

On top of the current legacy gamma, DRM allows the setting of three color
management tables: the degamma LUT, the color transform matrix (CTM), and the
regamma LUT. To user land, all of them are stored as DRM blobs, and are
referenced by CRTC properties via their blob IDs.

Therefore, in order to allow setting color management via xrandr, we have to:

1. Enable modification of CRTC properties via xrandr
2. Allow configuring and changing DRM blob properties via their IDs
3. Ensure compatability with legacy gamma

The first three patches does the above, while the last two does some
refactoring work to remove repetative code.

A note to reviewers, I'm a little unclear on whether this woks when one CRTC is
connected to multiple outputs. I expect that changing a CRTC property via one
of its outputs will update for that output only, since randr still understands
it as an "output property". In whic case, there needs to be a v2. However, I'm
not sure how I can setup a test for this. Let me know if you have tips.

Leo (Sunpeng) Li (5):
  Add functions for changing CRTC color management properties
  Hook up CRTC color management functions
  Keep CRTC properties consistent
  Enable configure & change helper to handle enums
  Modify output_create_resources to use configure & change helper

 src/drmmode_display.c | 483 +++++++++++++++++++++++++++++++++++++++++---------
 src/drmmode_display.h |  17 +-
 2 files changed, 414 insertions(+), 86 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/5] Add functions for changing CRTC color management properties

[sunpeng.li-5C7GfCeVMHo]

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

This change adds a few functions in preparation of enabling CRTC color
managment via the randr interface.

The driver-private CRTC object now contains a list of properties,
mirroring the driver-private output object. The lifecycle of the CRTC
properties will also mirror the output.

Since color managment properties are all DRM blobs, we'll expose the
ability to change the blob ID. The user can create blobs via libdrm
(which can be done without ownership of DRM master), then set the ID via
xrandr. The user will then have to ensure proper cleanup by subsequently
releasing the blob.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 src/drmmode_display.c | 254 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/drmmode_display.h |  17 ++--
 2 files changed, 264 insertions(+), 7 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 85970d1..23f9ad3 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -1301,6 +1301,25 @@ 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;
+	int i;
+
+	/* Free property list */
+	for (i = 0; i < drmmode_crtc->num_props; i++) {
+		drmModeFreeProperty(drmmode_crtc->props[i].mode_prop);
+		free(drmmode_crtc->props[i].atoms);
+	}
+	free(drmmode_crtc->props);
+
+	drmModeFreeCrtc(drmmode_crtc->mode_crtc);
+
+	free(drmmode_crtc);
+	crtc->driver_private = NULL;
+}
+
+
 static xf86CrtcFuncsRec drmmode_crtc_funcs = {
 	.dpms = drmmode_crtc_dpms,
 	.set_mode_major = drmmode_set_mode_major,
@@ -1604,6 +1623,18 @@ static void drmmode_output_dpms(xf86OutputPtr output, int mode)
 	}
 }
 
+static Bool drmmode_crtc_property_ignore(drmModePropertyPtr prop)
+{
+	if (!prop)
+		return TRUE;
+	/* Ignore CRTC gamma lut sizes */
+	if (!strcmp(prop->name, "GAMMA_LUT_SIZE") ||
+	    !strcmp(prop->name, "DEGAMMA_LUT_SIZE"))
+		return TRUE;
+
+	return FALSE;
+}
+
 static Bool drmmode_property_ignore(drmModePropertyPtr prop)
 {
 	if (!prop)
@@ -1618,6 +1649,163 @@ static Bool drmmode_property_ignore(drmModePropertyPtr prop)
 	return FALSE;
 }
 
+/**
+* Configure and change the given output property through randr. Currently
+* ignores DRM_MODE_PROP_ENU property types. Used as part of create_resources.
+*
+* Return: 0 on success, X-defined error codes on failure.
+*/
+static int __rr_configure_and_change_property(xf86OutputPtr output,
+					      drmmode_prop_ptr pmode_prop)
+{
+	drmModePropertyPtr mode_prop = pmode_prop->mode_prop;
+	Bool is_immutable = mode_prop->flags & DRM_MODE_PROP_IMMUTABLE ?
+				TRUE : FALSE;
+	int err;
+
+	if (mode_prop->flags & DRM_MODE_PROP_BLOB) {
+		INT32 blob_id = pmode_prop->value;
+		INT32 range[2];
+
+		range[0] = 0;
+		range[1] = 0x7fffffff;  /* Max signed 32-bit integer */
+
+		pmode_prop->num_atoms = 1;
+		pmode_prop->atoms = calloc(pmode_prop->num_atoms, sizeof(Atom));
+		if (!pmode_prop->atoms)
+			return -1;
+
+		pmode_prop->atoms[0] = MakeAtom(mode_prop->name,
+						strlen(mode_prop->name),
+						TRUE);
+		err = RRConfigureOutputProperty(output->randr_output,
+						pmode_prop->atoms[0],
+						FALSE, TRUE,
+						is_immutable, 2, range);
+		if (err)
+			goto rrconfig_error;
+
+		err = RRChangeOutputProperty(output->randr_output,
+					     pmode_prop->atoms[0],
+					     XA_INTEGER, 32,
+					     PropModeReplace, 1, &blob_id,
+					     FALSE, TRUE);
+		if (err)
+			goto rrchange_error;
+	}
+	else if (mode_prop->flags & DRM_MODE_PROP_RANGE) {
+		INT32 range[2];
+		INT32 value = pmode_prop->value;
+
+		pmode_prop->num_atoms = 1;
+		pmode_prop->atoms = calloc(pmode_prop->num_atoms, sizeof(Atom));
+		if (!pmode_prop->atoms)
+			return -1;
+		pmode_prop->atoms[0] = MakeAtom(mode_prop->name,
+						strlen(mode_prop->name),
+						TRUE);
+		range[0] = mode_prop->values[0];
+		range[1] = mode_prop->values[1];
+
+		err = RRConfigureOutputProperty(output->randr_output,
+						pmode_prop->atoms[0],
+						FALSE, TRUE,
+						is_immutable, 2, range);
+		if (err)
+			goto rrconfig_error;
+
+		err = RRChangeOutputProperty(output->randr_output,
+					     pmode_prop->atoms[0],
+					     XA_INTEGER, 32,
+					     PropModeReplace, 1, &value,
+					     FALSE, TRUE);
+		if (err)
+			goto rrchange_error;
+	}
+
+	return 0;
+
+rrconfig_error:
+	xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+		   "Configuring property %s failed with %d\n",
+		   mode_prop->name, err);
+	return err;
+
+rrchange_error:
+	xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+		   "Changing property %s failed with %d\n",
+		   mode_prop->name, err);
+	return err;
+
+}
+
+static void drmmode_crtc_create_resources(xf86CrtcPtr crtc,
+					  xf86OutputPtr output)
+{
+	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+	int i, j;
+
+	/* 'p' prefix for driver private objects */
+	drmmode_crtc_private_ptr pmode_crtc = crtc->driver_private;
+	drmModeCrtcPtr mode_crtc = pmode_crtc->mode_crtc;
+
+	drmmode_prop_ptr pmode_prop;
+	drmModePropertyPtr mode_prop;
+
+	/* Get list of DRM CRTC properties, and their values */
+	drmModeObjectPropertiesPtr mode_props;
+	mode_props = drmModeObjectGetProperties(pAMDGPUEnt->fd,
+						mode_crtc->crtc_id,
+						DRM_MODE_OBJECT_CRTC);
+	if (!mode_props)
+		goto err_allocs;
+
+	/* Allocate, then populate the driver-private CRTC property list */
+	pmode_crtc->props = calloc(mode_props->count_props + 1,
+				     sizeof(drmmode_prop_rec));
+	if (!pmode_crtc->props)
+		goto err_allocs;
+
+	pmode_crtc->num_props = 0;
+
+	/* Filter through drm crtc properties for relevant ones, and save
+	 * them */
+	for (i = 0, j = 0; i < mode_props->count_props; i++) {
+		mode_prop = drmModeGetProperty(pAMDGPUEnt->fd,
+					       mode_props->props[i]);
+		if (!mode_prop)
+			goto err_allocs;
+
+		if (drmmode_crtc_property_ignore(mode_prop)) {
+			drmModeFreeProperty(mode_prop);
+			continue;
+		}
+
+		pmode_crtc->num_props++;
+		pmode_prop = &pmode_crtc->props[j];
+		pmode_prop->mode_prop = mode_prop;
+		pmode_prop->value = mode_props->prop_values[i];
+
+		j++;
+	}
+
+	/* Finally, configure and set the properties */
+	for (i = 0; i < pmode_crtc->num_props; i++) {
+		pmode_prop = &pmode_crtc->props[i];
+		__rr_configure_and_change_property(output, pmode_prop);
+	}
+
+	drmModeFreeObjectProperties(mode_props);
+	return;
+
+err_allocs:
+	xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+		   "Memory error creating resources for CRTC %d\n",
+		   mode_crtc->crtc_id);
+	drmModeFreeObjectProperties(mode_props);
+	return;
+}
+
 static void drmmode_output_create_resources(xf86OutputPtr output)
 {
 	AMDGPUInfoPtr info = AMDGPUPTR(output->scrn);
@@ -1747,6 +1935,72 @@ static void drmmode_output_create_resources(xf86OutputPtr output)
 	}
 }
 
+/**
+* Set a CRTC property.
+*
+* Return 0 on success, -errno on failure.
+* A >0 return value implies the given property was not found on the CRTC.
+*/
+static int drmmode_crtc_set_property(xf86CrtcPtr crtc, Atom property,
+				     RRPropertyValuePtr value)
+{
+	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+	drmmode_crtc_private_ptr pmode_crtc = crtc->driver_private;
+	drmmode_prop_ptr p = NULL;
+	int i;
+
+	for (i = 0; i < pmode_crtc->num_props; i++) {
+		p = &pmode_crtc->props[i];
+
+		if (p->atoms[0] == property)
+			break;
+	}
+	if (i == pmode_crtc->num_props)  /* Property not found in CRTC. */
+		return 1;
+
+	if (p->mode_prop->flags & DRM_MODE_PROP_BLOB) {
+		uint32_t blob_id;
+		int ret;
+
+		if (value->type != XA_INTEGER || value->format != 32 ||
+		    value->size != 1)
+			return -EINVAL;
+
+		blob_id = *(uint32_t *) value->data;
+
+		ret = drmModeObjectSetProperty(pAMDGPUEnt->fd,
+					       pmode_crtc->mode_crtc->crtc_id,
+					       DRM_MODE_OBJECT_CRTC,
+					       p->mode_prop->prop_id,
+					       (uint64_t)blob_id);
+		if (ret)
+			return ret;
+
+		return 0;
+	}
+	if (p->mode_prop->flags & DRM_MODE_PROP_RANGE) {
+		uint32_t val;
+		int ret;
+
+		if (value->type != XA_INTEGER || value->format != 32 ||
+		    value->size != 1)
+			return -EINVAL;
+		val = *(uint32_t *) value->data;
+
+		ret = drmModeObjectSetProperty(pAMDGPUEnt->fd,
+					       pmode_crtc->mode_crtc->crtc_id,
+					       DRM_MODE_OBJECT_CRTC,
+					       p->mode_prop->prop_id,
+					       (uint64_t) val);
+		if (ret)
+			return ret;
+		return 0;
+	}
+
+	/* No property set. */
+	return 1;
+}
+
 static Bool
 drmmode_output_set_property(xf86OutputPtr output, Atom property,
 			    RRPropertyValuePtr value)
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index 2aa5672..fcadce3 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -74,6 +74,13 @@ struct drmmode_scanout {
 };
 
 typedef struct {
+	drmModePropertyPtr mode_prop;
+	uint64_t value;
+	int num_atoms;		/* if range prop, num_atoms == 1; if enum prop, num_atoms == num_enums + 1 */
+	Atom *atoms;
+} drmmode_prop_rec, *drmmode_prop_ptr;
+
+typedef struct {
 	drmmode_ptr drmmode;
 	drmModeCrtcPtr mode_crtc;
 	int hw_id;
@@ -109,14 +116,10 @@ typedef struct {
 	unsigned present_vblank_msc;
 	Bool present_flip_expected;
 #endif
-} drmmode_crtc_private_rec, *drmmode_crtc_private_ptr;
 
-typedef struct {
-	drmModePropertyPtr mode_prop;
-	uint64_t value;
-	int num_atoms;		/* if range prop, num_atoms == 1; if enum prop, num_atoms == num_enums + 1 */
-	Atom *atoms;
-} drmmode_prop_rec, *drmmode_prop_ptr;
+	int num_props;
+	drmmode_prop_ptr props;
+} drmmode_crtc_private_rec, *drmmode_crtc_private_ptr;
 
 typedef struct {
 	drmmode_ptr drmmode;
-- 
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/5] Hook up CRTC color management functions

[sunpeng.li-5C7GfCeVMHo]

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

The functions insert into the output resource creation, and property
change functions. CRTC destroy is also hooked-up for proper cleanup of
the CRTC property list.

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

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 23f9ad3..1966fd2 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -1336,7 +1336,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,
 };
 
@@ -1933,6 +1933,9 @@ static void drmmode_output_create_resources(xf86OutputPtr output)
 			}
 		}
 	}
+
+	if (output->crtc)
+		drmmode_crtc_create_resources(output->crtc, output);
 }
 
 /**
@@ -2009,6 +2012,20 @@ drmmode_output_set_property(xf86OutputPtr output, Atom property,
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(output->scrn);
 	int i;
 
+	if (output->crtc) {
+		int ret;
+		ret = drmmode_crtc_set_property(output->crtc, property, value);
+		if (ret == 0)
+			return TRUE;
+		if (ret < 0) {
+			xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+				   "Set CRTC mode property failed with %d\n",
+				   ret);
+			return FALSE;
+		}
+		/* Else, continue */
+	}
+
 	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 3/5] Keep CRTC properties consistent

[sunpeng.li-5C7GfCeVMHo]

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

In cases where CRTC properties are updated without going through
RRChangeOutputProperty, we don't update the properties in user land.

Consider setting legacy gamma. It doesn't go through
RRChangeOutputProperty, but modifies the CRTC's color management
properties. Unless they are updated, the user properties will remain
stale.

Therefore, add a function to update user CRTC properties by querying DRM,
and call it whenever legacy gamma is changed.

Also modify the configure and change helper to take in a 'pending'
argument, to allow property updates for user only.

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

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 1966fd2..45457c4 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -61,8 +61,13 @@
 
 #define DEFAULT_NOMINAL_FRAME_RATE 60
 
+/* Forward declarations */
+
 static Bool drmmode_xf86crtc_resize(ScrnInfoPtr scrn, int width, int height);
 
+static void drmmode_crtc_update_resources(xf86CrtcPtr crtc);
+
+
 static Bool
 AMDGPUZaphodStringMatches(ScrnInfoPtr pScrn, const char *s, char *output_name)
 {
@@ -768,6 +773,7 @@ drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t *red, uint16_t *green,
 
 	drmModeCrtcSetGamma(pAMDGPUEnt->fd, drmmode_crtc->mode_crtc->crtc_id,
 			    size, red, green, blue);
+	drmmode_crtc_update_resources(crtc);
 }
 
 Bool
@@ -1653,10 +1659,15 @@ static Bool drmmode_property_ignore(drmModePropertyPtr prop)
 * Configure and change the given output property through randr. Currently
 * ignores DRM_MODE_PROP_ENU property types. Used as part of create_resources.
 *
+* @output: The output to configure and change the property on.
+* @pmode_prop: The driver-private property object.
+* @pending: Whether the property changes are pending.
+*
 * Return: 0 on success, X-defined error codes on failure.
 */
 static int __rr_configure_and_change_property(xf86OutputPtr output,
-					      drmmode_prop_ptr pmode_prop)
+					      drmmode_prop_ptr pmode_prop,
+					      Bool pending)
 {
 	drmModePropertyPtr mode_prop = pmode_prop->mode_prop;
 	Bool is_immutable = mode_prop->flags & DRM_MODE_PROP_IMMUTABLE ?
@@ -1689,7 +1700,7 @@ static int __rr_configure_and_change_property(xf86OutputPtr output,
 					     pmode_prop->atoms[0],
 					     XA_INTEGER, 32,
 					     PropModeReplace, 1, &blob_id,
-					     FALSE, TRUE);
+					     FALSE, pending);
 		if (err)
 			goto rrchange_error;
 	}
@@ -1718,7 +1729,7 @@ static int __rr_configure_and_change_property(xf86OutputPtr output,
 					     pmode_prop->atoms[0],
 					     XA_INTEGER, 32,
 					     PropModeReplace, 1, &value,
-					     FALSE, TRUE);
+					     FALSE, pending);
 		if (err)
 			goto rrchange_error;
 	}
@@ -1739,6 +1750,89 @@ rrchange_error:
 
 }
 
+
+/**
+ * Update user properties on the given CRTC with values from DRM.
+ *
+ * This should be called when a CRTC property is set, without going through
+ * RRChangeOutputProperty.
+ *
+ * For example, setting legacy gamma will modify color management CRTC
+ * properties, but it does not go through RRChangeOutputProperty. This leaves
+ * the user property values in a stale state. Calling this function will grab
+ * updated values from DRM, and update the user properties.
+ */
+static void drmmode_crtc_update_resources(xf86CrtcPtr crtc)
+{
+	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+	xf86CrtcConfigPtr xf86_crtc_config = XF86_CRTC_CONFIG_PTR(crtc->scrn);
+	int i;
+
+	drmmode_crtc_private_ptr user_crtc = crtc->driver_private;
+
+	/* Get list of DRM CRTC properties, and their values */
+	drmModeObjectPropertiesPtr drm_props;
+	drm_props = drmModeObjectGetProperties(pAMDGPUEnt->fd,
+					       user_crtc->mode_crtc->crtc_id,
+					       DRM_MODE_OBJECT_CRTC);
+	if (!drm_props)
+		goto err_allocs;
+
+	for (i = 0; i < drm_props->count_props; i++) {
+		drmmode_prop_ptr user_prop = NULL;
+		drmModePropertyPtr drm_prop;
+		int j;
+
+		drm_prop = drmModeGetProperty(pAMDGPUEnt->fd,
+					       drm_props->props[i]);
+		if (!drm_prop)
+			goto err_allocs;
+
+		/* Check if property is currently on user CRTC */
+		for (j = 0; j < user_crtc->num_props; j++) {
+			user_prop = &user_crtc->props[j];
+
+			if (!strcmp(drm_prop->name,
+				    user_prop->mode_prop->name))
+				/* Found */
+				break;
+		}
+		if (j == user_crtc->num_props) {
+			/* Not found */
+			drmModeFreeProperty(drm_prop);
+			continue;
+		}
+
+		/* If found, update it with value from kernel */
+		drmModeFreeProperty(user_prop->mode_prop);
+		user_prop->mode_prop = drm_prop;
+		user_prop->value = drm_props->prop_values[i];
+
+		for (j = 0; j < xf86_crtc_config->num_output; j++) {
+			xf86OutputPtr output = xf86_crtc_config->output[j];
+			if (output->crtc != crtc)
+				continue;
+
+			/* Non-pending configure and change: Just updating
+			 * values on user-side */
+			__rr_configure_and_change_property(output,
+							   user_prop,
+							   FALSE);
+		}
+	}
+
+	drmModeFreeObjectProperties(drm_props);
+	return;
+
+err_allocs:
+	xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
+		   "Memory error updating resources for CRTC %d\n",
+		   user_crtc->mode_crtc->crtc_id);
+	drmModeFreeObjectProperties(drm_props);
+	return;
+
+}
+
 static void drmmode_crtc_create_resources(xf86CrtcPtr crtc,
 					  xf86OutputPtr output)
 {
@@ -1792,7 +1886,7 @@ static void drmmode_crtc_create_resources(xf86CrtcPtr crtc,
 	/* Finally, configure and set the properties */
 	for (i = 0; i < pmode_crtc->num_props; i++) {
 		pmode_prop = &pmode_crtc->props[i];
-		__rr_configure_and_change_property(output, pmode_prop);
+		__rr_configure_and_change_property(output, pmode_prop, TRUE);
 	}
 
 	drmModeFreeObjectProperties(mode_props);
-- 
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/5] Enable configure & change helper to handle enums

[sunpeng.li-5C7GfCeVMHo]

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

Copy code from output_create_resources, insert into configure &
change helper, and rename variables. Some formatting changes as well.
Modify doc-string to reflect change.

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

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 45457c4..9c95abc 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -1656,8 +1656,7 @@ static Bool drmmode_property_ignore(drmModePropertyPtr prop)
 }
 
 /**
-* Configure and change the given output property through randr. Currently
-* ignores DRM_MODE_PROP_ENU property types. Used as part of create_resources.
+* Configure and change the given output/CRTC property through randr.
 *
 * @output: The output to configure and change the property on.
 * @pmode_prop: The driver-private property object.
@@ -1733,6 +1732,45 @@ static int __rr_configure_and_change_property(xf86OutputPtr output,
 		if (err)
 			goto rrchange_error;
 	}
+	else if (mode_prop->flags & DRM_MODE_PROP_ENUM) {
+		int j;
+
+		pmode_prop->num_atoms = mode_prop->count_enums + 1;
+		pmode_prop->atoms = calloc(pmode_prop->num_atoms, sizeof(Atom));
+		if (!pmode_prop->atoms)
+			return 0;
+
+		pmode_prop->atoms[0] = MakeAtom(mode_prop->name,
+						strlen(mode_prop->name), TRUE);
+
+		for (j = 1; j <= mode_prop->count_enums; j++) {
+			struct drm_mode_property_enum *e =
+						&mode_prop->enums[j - 1];
+			pmode_prop->atoms[j] = MakeAtom(e->name,
+							strlen(e->name),
+							TRUE);
+		}
+		err = RRConfigureOutputProperty(output->randr_output,
+						pmode_prop->atoms[0],
+						FALSE, FALSE,
+						is_immutable,
+						pmode_prop->num_atoms - 1,
+						(INT32 *) &pmode_prop->atoms[1]);
+		if (err != 0)
+			goto rrconfig_error;
+
+		for (j = 0; j < mode_prop->count_enums; j++)
+			if (mode_prop->enums[j].value == pmode_prop->value)
+				break;
+		/* there's always a matching value */
+		err = RRChangeOutputProperty(output->randr_output,
+					     pmode_prop->atoms[0], XA_ATOM, 32,
+					     PropModeReplace, 1,
+					     &pmode_prop->atoms[j + 1], FALSE,
+					     pending);
+		if (err != 0)
+			goto rrchange_error;
+	}
 
 	return 0;
 
-- 
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/5] Modify output_create_resources to use configure & change helper

[sunpeng.li-5C7GfCeVMHo]

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

The conditionals within the for-loop do the same thing, since they're
essentially copied to the configure&change helper.

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

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 9c95abc..f55ba4d 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -1945,7 +1945,7 @@ static void drmmode_output_create_resources(xf86OutputPtr output)
 	drmModeConnectorPtr mode_output = drmmode_output->mode_output;
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(output->scrn);
 	drmModePropertyPtr drmmode_prop, tearfree_prop;
-	int i, j, err;
+	int i, j;
 
 	drmmode_output->props =
 		calloc(mode_output->count_props + 1, sizeof(drmmode_prop_rec));
@@ -1985,85 +1985,7 @@ static void drmmode_output_create_resources(xf86OutputPtr output)
 
 	for (i = 0; i < drmmode_output->num_props; i++) {
 		drmmode_prop_ptr p = &drmmode_output->props[i];
-		drmmode_prop = p->mode_prop;
-
-		if (drmmode_prop->flags & DRM_MODE_PROP_RANGE) {
-			INT32 range[2];
-			INT32 value = p->value;
-
-			p->num_atoms = 1;
-			p->atoms = calloc(p->num_atoms, sizeof(Atom));
-			if (!p->atoms)
-				continue;
-			p->atoms[0] =
-			    MakeAtom(drmmode_prop->name,
-				     strlen(drmmode_prop->name), TRUE);
-			range[0] = drmmode_prop->values[0];
-			range[1] = drmmode_prop->values[1];
-			err =
-			    RRConfigureOutputProperty(output->randr_output,
-						      p->atoms[0], FALSE, TRUE,
-						      drmmode_prop->flags &
-						      DRM_MODE_PROP_IMMUTABLE ?
-						      TRUE : FALSE, 2, range);
-			if (err != 0) {
-				xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
-					   "RRConfigureOutputProperty error, %d\n",
-					   err);
-			}
-			err =
-			    RRChangeOutputProperty(output->randr_output,
-						   p->atoms[0], XA_INTEGER, 32,
-						   PropModeReplace, 1, &value,
-						   FALSE, TRUE);
-			if (err != 0) {
-				xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
-					   "RRChangeOutputProperty error, %d\n",
-					   err);
-			}
-		} else if (drmmode_prop->flags & DRM_MODE_PROP_ENUM) {
-			p->num_atoms = drmmode_prop->count_enums + 1;
-			p->atoms = calloc(p->num_atoms, sizeof(Atom));
-			if (!p->atoms)
-				continue;
-			p->atoms[0] =
-			    MakeAtom(drmmode_prop->name,
-				     strlen(drmmode_prop->name), TRUE);
-			for (j = 1; j <= drmmode_prop->count_enums; j++) {
-				struct drm_mode_property_enum *e =
-				    &drmmode_prop->enums[j - 1];
-				p->atoms[j] =
-				    MakeAtom(e->name, strlen(e->name), TRUE);
-			}
-			err =
-			    RRConfigureOutputProperty(output->randr_output,
-						      p->atoms[0], FALSE, FALSE,
-						      drmmode_prop->flags &
-						      DRM_MODE_PROP_IMMUTABLE ?
-						      TRUE : FALSE,
-						      p->num_atoms - 1,
-						      (INT32 *) & p->atoms[1]);
-			if (err != 0) {
-				xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
-					   "RRConfigureOutputProperty error, %d\n",
-					   err);
-			}
-			for (j = 0; j < drmmode_prop->count_enums; j++)
-				if (drmmode_prop->enums[j].value == p->value)
-					break;
-			/* there's always a matching value */
-			err =
-			    RRChangeOutputProperty(output->randr_output,
-						   p->atoms[0], XA_ATOM, 32,
-						   PropModeReplace, 1,
-						   &p->atoms[j + 1], FALSE,
-						   TRUE);
-			if (err != 0) {
-				xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
-					   "RRChangeOutputProperty error, %d\n",
-					   err);
-			}
-		}
+		__rr_configure_and_change_property (output, p, TRUE);
 	}
 
 	if (output->crtc)
-- 
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/5] Implementing non-legacy color management

[Michel Dänzer]

Hi Leo,


apologies for the late follow-up; I was on vacation and then backlogged.


On 2018-03-26 10:00 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> These patches will enable modification of non-legacy color management
> properties via xrandr.
> 
> On top of the current legacy gamma, DRM allows the setting of three color
> management tables: the degamma LUT, the color transform matrix (CTM), and the
> regamma LUT. To user land, all of them are stored as DRM blobs, and are
> referenced by CRTC properties via their blob IDs.
> 
> Therefore, in order to allow setting color management via xrandr, we have to:
> 
> 1. Enable modification of CRTC properties via xrandr
> 2. Allow configuring and changing DRM blob properties via their IDs
> 3. Ensure compatability with legacy gamma
> 
> The first three patches does the above, while the last two does some
> refactoring work to remove repetative code.
> 
> A note to reviewers, I'm a little unclear on whether this woks when one CRTC is
> connected to multiple outputs. I expect that changing a CRTC property via one
> of its outputs will update for that output only, since randr still understands
> it as an "output property". In whic case, there needs to be a v2.

Yes, I suspect so.

> However, I'm not sure how I can setup a test for this. Let me know if you have tips.

Something like

xrandr --output DVI-D-1 --off --output DVI-D-2 --off
xrandr --output DVI-D-1 --crtc 0 --mode 1920x1080 --output DVI-D-2
 --crtc 0 --mode 1920x1080

and then verify with xrandr --verbose that both outputs are actually
using the same CRTC. Note that I'm getting an error on the second step
when trying this right now, so there may be something preventing using
the same CRTC for multiple outputs. But AFAIK at least in theory it's
possible.


I will follow up with some comments on individual 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 1/5] Add functions for changing CRTC color management properties

[Michel Dänzer]

On 2018-03-26 10:00 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> This change adds a few functions in preparation of enabling CRTC color
> managment via the randr interface.
> 
> The driver-private CRTC object now contains a list of properties,
> mirroring the driver-private output object. The lifecycle of the CRTC
> properties will also mirror the output.
> 
> Since color managment properties are all DRM blobs, we'll expose the
> ability to change the blob ID. The user can create blobs via libdrm
> (which can be done without ownership of DRM master), then set the ID via
> xrandr. The user will then have to ensure proper cleanup by subsequently
> releasing the blob.

That sounds a bit clunky. :)

When changing a blob ID, the change only takes effect on the next atomic
commit, doesn't it? How does the client trigger the atomic commit?


> @@ -1604,6 +1623,18 @@ static void drmmode_output_dpms(xf86OutputPtr output, int mode)
>  	}
>  }
>  
> +static Bool drmmode_crtc_property_ignore(drmModePropertyPtr prop)
> +{
> +	if (!prop)
> +		return TRUE;
> +	/* Ignore CRTC gamma lut sizes */
> +	if (!strcmp(prop->name, "GAMMA_LUT_SIZE") ||
> +	    !strcmp(prop->name, "DEGAMMA_LUT_SIZE"))
> +		return TRUE;

Without these properties, how can a client know the LUT sizes?


> @@ -1618,6 +1649,163 @@ static Bool drmmode_property_ignore(drmModePropertyPtr prop)
>  	return FALSE;
>  }
>  
> +/**
> +* Configure and change the given output property through randr. Currently

"RandR"

> +* ignores DRM_MODE_PROP_ENU property types. Used as part of create_resources.

DRM_MODE_PROP_ENUM is missing the final M.

> +*
> +* Return: 0 on success, X-defined error codes on failure.
> +*/
> +static int __rr_configure_and_change_property(xf86OutputPtr output,
> +					      drmmode_prop_ptr pmode_prop)

No leading underscores in function names please.


> +	}
> +	else if (mode_prop->flags & DRM_MODE_PROP_RANGE) {

The else should be on the same line as }.


> +static void drmmode_crtc_create_resources(xf86CrtcPtr crtc,
> +					  xf86OutputPtr output)
> +{
> +	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
> +	int i, j;
> +
> +	/* 'p' prefix for driver private objects */
> +	drmmode_crtc_private_ptr pmode_crtc = crtc->driver_private;

Existing code refers to this as drmmode_crtc, please stick to that.


> +	drmModeCrtcPtr mode_crtc = pmode_crtc->mode_crtc;
> +
> +	drmmode_prop_ptr pmode_prop;
> +	drmModePropertyPtr mode_prop;
> +
> +	/* Get list of DRM CRTC properties, and their values */
> +	drmModeObjectPropertiesPtr mode_props;

All local variable declarations should be in a single block, with no
blank lines between them, and generally sorted from longer lines to
shorter ones.


> +	mode_props = drmModeObjectGetProperties(pAMDGPUEnt->fd,
> +						mode_crtc->crtc_id,
> +						DRM_MODE_OBJECT_CRTC);
> +	if (!mode_props)
> +		goto err_allocs;
> +
> +	/* Allocate, then populate the driver-private CRTC property list */
> +	pmode_crtc->props = calloc(mode_props->count_props + 1,
> +				     sizeof(drmmode_prop_rec));

Continuation lines should be aligned to opening parens. Any editor which
supports EditorConfig should do this automagically.


> +	if (!pmode_crtc->props)
> +		goto err_allocs;
> +
> +	pmode_crtc->num_props = 0;
> +
> +	/* Filter through drm crtc properties for relevant ones, and save
> +	 * them */

	/* Multi-line comments should be formatted like this.
	 * Comment end marker on a separate line:
	 */


> +err_allocs:
> +	xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
> +		   "Memory error creating resources for CRTC %d\n",
> +		   mode_crtc->crtc_id);
> +	drmModeFreeObjectProperties(mode_props);
> +	return;
> +}

Remove the superfluous return statement at the end of a function
returning void.


-- 
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/5] Hook up CRTC color management functions

[Michel Dänzer]

On 2018-03-26 10:00 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> The functions insert into the output resource creation, and property
> change functions. CRTC destroy is also hooked-up for proper cleanup of
> the CRTC property list.
> 
> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>

[...]

> @@ -1933,6 +1933,9 @@ static void drmmode_output_create_resources(xf86OutputPtr output)
>  			}
>  		}
>  	}
> +
> +	if (output->crtc)
> +		drmmode_crtc_create_resources(output->crtc, output);

output->crtc is only non-NULL here for outputs which are enabled at Xorg
startup; other outputs won't have the new properties.


-- 
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/5] Keep CRTC properties consistent

[Michel Dänzer]

On 2018-03-26 10:00 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> In cases where CRTC properties are updated without going through
> RRChangeOutputProperty, we don't update the properties in user land.
> 
> Consider setting legacy gamma. It doesn't go through
> RRChangeOutputProperty, but modifies the CRTC's color management
> properties. Unless they are updated, the user properties will remain
> stale.

Can you describe a bit more how the legacy gamma and the new properties
interact?


> Therefore, add a function to update user CRTC properties by querying DRM,
> and call it whenever legacy gamma is changed.

Note that drmmode_crtc_gamma_do_set is called from
drmmode_set_mode_major, i.e. on every modeset or under some
circumstances when a DRI3 client stops page flipping.


> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index 1966fd2..45457c4 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -61,8 +61,13 @@
>  
>  #define DEFAULT_NOMINAL_FRAME_RATE 60
>  
> +/* Forward declarations */
> +
>  static Bool drmmode_xf86crtc_resize(ScrnInfoPtr scrn, int width, int height);
>  
> +static void drmmode_crtc_update_resources(xf86CrtcPtr crtc);

Can you move the drmmode_crtc_update_resources such that the forward
declaration isn't necessary?


>  static Bool
>  AMDGPUZaphodStringMatches(ScrnInfoPtr pScrn, const char *s, char *output_name)
>  {
> @@ -768,6 +773,7 @@ drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t *red, uint16_t *green,
>  
>  	drmModeCrtcSetGamma(pAMDGPUEnt->fd, drmmode_crtc->mode_crtc->crtc_id,
>  			    size, red, green, blue);
> +	drmmode_crtc_update_resources(crtc);
>  }
>  
>  Bool
> @@ -1653,10 +1659,15 @@ static Bool drmmode_property_ignore(drmModePropertyPtr prop)
>  * Configure and change the given output property through randr. Currently
>  * ignores DRM_MODE_PROP_ENU property types. Used as part of create_resources.
>  *
> +* @output: The output to configure and change the property on.
> +* @pmode_prop: The driver-private property object.

These two should have been added in patch 1.


-- 
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 4/5] Enable configure & change helper to handle enums

[Michel Dänzer]

On 2018-03-26 10:00 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> Copy code from output_create_resources, insert into configure &
> change helper, and rename variables. Some formatting changes as well.
> Modify doc-string to reflect change.
> 
> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>

Please squash this patch and patch 5 to a single patch.


-- 
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 1/5] Add functions for changing CRTC color management properties

[Leo Li]

On 2018-04-09 11:03 AM, Michel Dänzer wrote:
> On 2018-03-26 10:00 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>
>> This change adds a few functions in preparation of enabling CRTC color
>> managment via the randr interface.
>>
>> The driver-private CRTC object now contains a list of properties,
>> mirroring the driver-private output object. The lifecycle of the CRTC
>> properties will also mirror the output.
>>
>> Since color managment properties are all DRM blobs, we'll expose the
>> ability to change the blob ID. The user can create blobs via libdrm
>> (which can be done without ownership of DRM master), then set the ID via
>> xrandr. The user will then have to ensure proper cleanup by subsequently
>> releasing the blob.
> 
> That sounds a bit clunky. :)
> 
> When changing a blob ID, the change only takes effect on the next atomic
> commit, doesn't it? How does the client trigger the atomic commit?
> 

 From the perspective of a client that wishes to change a property, the
process between regular properties and blob properties should be
essentially the same. Both will trigger an atomic commit when the DRM
set property ioctl is called from our DDX driver.

The only difference is that DRM property blobs can be arbitrary in size,
and needs to be passed by reference through its DRM-defined blob ID.
Because of this, the client has to create the blob, save it's id, call
libXrandr to change it, then destroy the blob after it's been committed.

The client has to call libXrandr due to DRM permissions. IIRC, there can
be only one DRM master. And since xserver is DRM master, an external
application cannot set DRM properties unless it goes through X. However,
creating and destroying DRM property blobs and can be done by anyone.

Was this the source of the clunkiness? I've thought about having DDX
create and destroy the blob instead, but that needs an interface for the
client to get arbitrarily sized data to DDX. I'm not aware of any good
ways to do so. Don't think the kernel can do this for us either. It does
create the blob for legacy gamma, but that's because there's a dedicated
ioctl for it.

> 
>> @@ -1604,6 +1623,18 @@ static void drmmode_output_dpms(xf86OutputPtr output, int mode)
>>   	}
>>   }
>>   
>> +static Bool drmmode_crtc_property_ignore(drmModePropertyPtr prop)
>> +{
>> +	if (!prop)
>> +		return TRUE;
>> +	/* Ignore CRTC gamma lut sizes */
>> +	if (!strcmp(prop->name, "GAMMA_LUT_SIZE") ||
>> +	    !strcmp(prop->name, "DEGAMMA_LUT_SIZE"))
>> +		return TRUE;
> 
> Without these properties, how can a client know the LUT sizes?

Good point, I originally thought the sizes are fixed and did not need
exposing. But who knows if they may change, or even be different per asic.

> 
> 
>> @@ -1618,6 +1649,163 @@ static Bool drmmode_property_ignore(drmModePropertyPtr prop)
>>   	return FALSE;
>>   }
>>   
>> +/**
>> +* Configure and change the given output property through randr. Currently
> 
> "RandR"
> 
>> +* ignores DRM_MODE_PROP_ENU property types. Used as part of create_resources.
> 
> DRM_MODE_PROP_ENUM is missing the final M.
> 
>> +*
>> +* Return: 0 on success, X-defined error codes on failure.
>> +*/
>> +static int __rr_configure_and_change_property(xf86OutputPtr output,
>> +					      drmmode_prop_ptr pmode_prop)
> 
> No leading underscores in function names please. >
> 
>> +	}
>> +	else if (mode_prop->flags & DRM_MODE_PROP_RANGE) {
> 
> The else should be on the same line as }.
> 
> 
>> +static void drmmode_crtc_create_resources(xf86CrtcPtr crtc,
>> +					  xf86OutputPtr output)
>> +{
>> +	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
>> +	int i, j;
>> +
>> +	/* 'p' prefix for driver private objects */
>> +	drmmode_crtc_private_ptr pmode_crtc = crtc->driver_private;
> 
> Existing code refers to this as drmmode_crtc, please stick to that.
> 
> 
>> +	drmModeCrtcPtr mode_crtc = pmode_crtc->mode_crtc;
>> +
>> +	drmmode_prop_ptr pmode_prop;
>> +	drmModePropertyPtr mode_prop;
>> +
>> +	/* Get list of DRM CRTC properties, and their values */
>> +	drmModeObjectPropertiesPtr mode_props;
> 
> All local variable declarations should be in a single block, with no
> blank lines between them, and generally sorted from longer lines to
> shorter ones.
> 
> 
>> +	mode_props = drmModeObjectGetProperties(pAMDGPUEnt->fd,
>> +						mode_crtc->crtc_id,
>> +						DRM_MODE_OBJECT_CRTC);
>> +	if (!mode_props)
>> +		goto err_allocs;
>> +
>> +	/* Allocate, then populate the driver-private CRTC property list */
>> +	pmode_crtc->props = calloc(mode_props->count_props + 1,
>> +				     sizeof(drmmode_prop_rec));
> 
> Continuation lines should be aligned to opening parens. Any editor which
> supports EditorConfig should do this automagically.
> 
> 
>> +	if (!pmode_crtc->props)
>> +		goto err_allocs;
>> +
>> +	pmode_crtc->num_props = 0;
>> +
>> +	/* Filter through drm crtc properties for relevant ones, and save
>> +	 * them */
> 
> 	/* Multi-line comments should be formatted like this.
> 	 * Comment end marker on a separate line:
> 	 */
> 
> 
>> +err_allocs:
>> +	xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
>> +		   "Memory error creating resources for CRTC %d\n",
>> +		   mode_crtc->crtc_id);
>> +	drmModeFreeObjectProperties(mode_props);
>> +	return;
>> +}
> 
> Remove the superfluous return statement at the end of a function
> returning void.

Will fix all style problems.

Leo

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

Re: [PATCH xf86-video-amdgpu 2/5] Hook up CRTC color management functions

[Leo Li]

On 2018-04-09 11:03 AM, Michel Dänzer wrote:
> On 2018-03-26 10:00 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>
>> The functions insert into the output resource creation, and property
>> change functions. CRTC destroy is also hooked-up for proper cleanup of
>> the CRTC property list.
>>
>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
> 
> [...]
> 
>> @@ -1933,6 +1933,9 @@ static void drmmode_output_create_resources(xf86OutputPtr output)
>>   			}
>>   		}
>>   	}
>> +
>> +	if (output->crtc)
>> +		drmmode_crtc_create_resources(output->crtc, output);
> 
> output->crtc is only non-NULL here for outputs which are enabled at Xorg
> startup; other outputs won't have the new properties.

Is it necessary to have the CRTC properties on a output if the CRTC is
disabled for that output?

I've tested hot-plugging with this, and the properties do initialize on
hot-plug. Though they stay on the output on hot-unplug... Haven't dug
into this just yet.

Leo

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

Re: [PATCH xf86-video-amdgpu 3/5] Keep CRTC properties consistent

[Leo Li]

On 2018-04-09 11:03 AM, Michel Dänzer wrote:
> On 2018-03-26 10:00 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>
>> In cases where CRTC properties are updated without going through
>> RRChangeOutputProperty, we don't update the properties in user land.
>>
>> Consider setting legacy gamma. It doesn't go through
>> RRChangeOutputProperty, but modifies the CRTC's color management
>> properties. Unless they are updated, the user properties will remain
>> stale.
> 
> Can you describe a bit more how the legacy gamma and the new properties
> interact?
> 

Sure thing, I'll include this in the message for v2:

In kernel, the legacy set gamma interface is essentially an adapter to
the non-legacy set properties interface. In the end, they both set the
same property to a DRM property blob, which contains the gamma lookup
table. The key difference between them is how this blob is created.

For legacy gamma, the kernel takes 3 arrays from user-land, and creates
the blob using them. Note that a blob is identified by it's blob_id.

For non-legacy gamma, the kernel takes a blob_id from user-land that
references the blob. This means user-land is responsible for creating
the blob.

 From the perspective of RandR, this presents some problems. Since both
paths modify the same property, RandR must keep the reported property
value up-to-date with which ever path is used:

1. Legacy gamma via
xrandr --output <output_here> --gamma x:x:x
2. Non-legacy color properties via
xrandr --output <output_here> --set GAMMA_LUT <blob_id>

Keeping the value up-to-date isn't a problem for 2, since RandR updates
it for us as part of changing output properties.

But if 1 is used, the property blob is created within kernel, and RandR
is unaware of the new blob_id. To update it, we need to ask kernel about it.

--- continue with rest of message ---
> 
>> Therefore, add a function to update user CRTC properties by querying DRM,
>> and call it whenever legacy gamma is changed.
> 
> Note that drmmode_crtc_gamma_do_set is called from
> drmmode_set_mode_major, i.e. on every modeset or under some
> circumstances when a DRI3 client stops page flipping.
> 

The property will have to be updated each time the legacy set gamma
ioctl is called, since a new blob (with a new blob_id) is created each time.

Not sure if this is a good idea, but perhaps we can have a flag that
explicitly enable one or the other, depending on user preference? A
user-only property with something like:

0: Use legacy gamma, calls to change non-legacy properties are ignored.
1: Use non-legacy, calls to legacy gamma will be ignored.

On 0, we can remove/disable all non-legacy properties from the property
list, and avoid having to update them. On 1, we'll enable the
properties, and won't have to update them either since legacy gamma is
"disabled". It has the added benefit of avoiding unexpected legacy gamma
sets when using non-legacy, and vice versa.

> 
>> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
>> index 1966fd2..45457c4 100644
>> --- a/src/drmmode_display.c
>> +++ b/src/drmmode_display.c
>> @@ -61,8 +61,13 @@
>>   
>>   #define DEFAULT_NOMINAL_FRAME_RATE 60
>>   
>> +/* Forward declarations */
>> +
>>   static Bool drmmode_xf86crtc_resize(ScrnInfoPtr scrn, int width, int height);
>>   
>> +static void drmmode_crtc_update_resources(xf86CrtcPtr crtc);
> 
> Can you move the drmmode_crtc_update_resources such that the forward
> declaration isn't necessary?
> 

Seems possible. It uses the rr_configure_and_change helper, so I'll pull
both of them up.

> 
>>   static Bool
>>   AMDGPUZaphodStringMatches(ScrnInfoPtr pScrn, const char *s, char *output_name)
>>   {
>> @@ -768,6 +773,7 @@ drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t *red, uint16_t *green,
>>   
>>   	drmModeCrtcSetGamma(pAMDGPUEnt->fd, drmmode_crtc->mode_crtc->crtc_id,
>>   			    size, red, green, blue);
>> +	drmmode_crtc_update_resources(crtc);
>>   }
>>   
>>   Bool
>> @@ -1653,10 +1659,15 @@ static Bool drmmode_property_ignore(drmModePropertyPtr prop)
>>   * Configure and change the given output property through randr. Currently
>>   * ignores DRM_MODE_PROP_ENU property types. Used as part of create_resources.
>>   *
>> +* @output: The output to configure and change the property on.
>> +* @pmode_prop: The driver-private property object.
> 
> These two should have been added in patch 1.

Yep, will move.

Leo

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

Re: [PATCH xf86-video-amdgpu 0/5] Implementing non-legacy color management

[Leo Li]

On 2018-04-09 10:10 AM, Michel Dänzer wrote:
> 
> Hi Leo,
> 
> 
> apologies for the late follow-up; I was on vacation and then backlogged.

No worries, thanks for the review :)

> 
> 
> On 2018-03-26 10:00 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>
>> These patches will enable modification of non-legacy color management
>> properties via xrandr.
>>
>> On top of the current legacy gamma, DRM allows the setting of three color
>> management tables: the degamma LUT, the color transform matrix (CTM), and the
>> regamma LUT. To user land, all of them are stored as DRM blobs, and are
>> referenced by CRTC properties via their blob IDs.
>>
>> Therefore, in order to allow setting color management via xrandr, we have to:
>>
>> 1. Enable modification of CRTC properties via xrandr
>> 2. Allow configuring and changing DRM blob properties via their IDs
>> 3. Ensure compatability with legacy gamma
>>
>> The first three patches does the above, while the last two does some
>> refactoring work to remove repetative code.
>>
>> A note to reviewers, I'm a little unclear on whether this woks when one CRTC is
>> connected to multiple outputs. I expect that changing a CRTC property via one
>> of its outputs will update for that output only, since randr still understands
>> it as an "output property". In whic case, there needs to be a v2.
> 
> Yes, I suspect so.
> 
>> However, I'm not sure how I can setup a test for this. Let me know if you have tips.
> 
> Something like
> 
> xrandr --output DVI-D-1 --off --output DVI-D-2 --off
> xrandr --output DVI-D-1 --crtc 0 --mode 1920x1080 --output DVI-D-2
>   --crtc 0 --mode 1920x1080
> 
> and then verify with xrandr --verbose that both outputs are actually
> using the same CRTC. Note that I'm getting an error on the second step
> when trying this right now, so there may be something preventing using
> the same CRTC for multiple outputs. But AFAIK at least in theory it's
> possible.

I'll give this a shot.

Leo

> 
> 
> I will follow up with some comments on individual patches.
> 
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 3/5] Keep CRTC properties consistent

[Michel Dänzer]

On 2018-04-10 08:02 PM, Leo Li wrote:
> On 2018-04-09 11:03 AM, Michel Dänzer wrote:
>> On 2018-03-26 10:00 PM, sunpeng.li@amd.com wrote:
>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>
>>> In cases where CRTC properties are updated without going through
>>> RRChangeOutputProperty, we don't update the properties in user land.
>>>
>>> Consider setting legacy gamma. It doesn't go through
>>> RRChangeOutputProperty, but modifies the CRTC's color management
>>> properties. Unless they are updated, the user properties will remain
>>> stale.
>>
>> Can you describe a bit more how the legacy gamma and the new properties
>> interact?
>>
> 
> Sure thing, I'll include this in the message for v2:
> 
> In kernel, the legacy set gamma interface is essentially an adapter to
> the non-legacy set properties interface. In the end, they both set the
> same property to a DRM property blob, which contains the gamma lookup
> table. The key difference between them is how this blob is created.
> 
> For legacy gamma, the kernel takes 3 arrays from user-land, and creates
> the blob using them. Note that a blob is identified by it's blob_id.
> 
> For non-legacy gamma, the kernel takes a blob_id from user-land that
> references the blob. This means user-land is responsible for creating
> the blob.
> 
> From the perspective of RandR, this presents some problems. Since both
> paths modify the same property, RandR must keep the reported property
> value up-to-date with which ever path is used:
> 
> 1. Legacy gamma via
> xrandr --output <output_here> --gamma x:x:x
> 2. Non-legacy color properties via
> xrandr --output <output_here> --set GAMMA_LUT <blob_id>
> 
> Keeping the value up-to-date isn't a problem for 2, since RandR updates
> it for us as part of changing output properties.
> 
> But if 1 is used, the property blob is created within kernel, and RandR
> is unaware of the new blob_id. To update it, we need to ask kernel about
> it.
> 
> --- continue with rest of message ---
>>
>>> Therefore, add a function to update user CRTC properties by querying
>>> DRM,
>>> and call it whenever legacy gamma is changed.
>>
>> Note that drmmode_crtc_gamma_do_set is called from
>> drmmode_set_mode_major, i.e. on every modeset or under some
>> circumstances when a DRI3 client stops page flipping.
>>
> 
> The property will have to be updated each time the legacy set gamma
> ioctl is called, since a new blob (with a new blob_id) is created each
> time.
> 
> Not sure if this is a good idea, but perhaps we can have a flag that
> explicitly enable one or the other, depending on user preference? A
> user-only property with something like:
> 
> 0: Use legacy gamma, calls to change non-legacy properties are ignored.
> 1: Use non-legacy, calls to legacy gamma will be ignored.
> 
> On 0, we can remove/disable all non-legacy properties from the property
> list, and avoid having to update them. On 1, we'll enable the
> properties, and won't have to update them either since legacy gamma is
> "disabled". It has the added benefit of avoiding unexpected legacy gamma
> sets when using non-legacy, and vice versa.

Hmm. So either legacy or non-legacy clients won't work at all, or
they'll step on each other's toes, clobbering the HW gamma LUT from each
other.

I'm afraid neither of those alternatives sound like a good user
experience to me.

Consider on the one hand something like Night Light / redshift, using
legacy APIs to adjust colour temperature to the time of day. On the
other hand, another client using the non-legacy API for say fine-tuning
of a display's advanced gamma capabilities.

Ideally, in this case the gamma LUTs from the legacy and non-legacy APIs
should be combined, such that the hardware LUT reflects both the colour
temperature set by Night Light / refshift and the fine-tuning set by the
non-legacy client. Is that feasible? If not, can you explain a little why?


-- 
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 1/5] Add functions for changing CRTC color management properties

[Michel Dänzer]

On 2018-04-10 08:02 PM, Leo Li wrote:
> On 2018-04-09 11:03 AM, Michel Dänzer wrote:
>> On 2018-03-26 10:00 PM, sunpeng.li@amd.com wrote:
>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>
>>> This change adds a few functions in preparation of enabling CRTC color
>>> managment via the randr interface.
>>>
>>> The driver-private CRTC object now contains a list of properties,
>>> mirroring the driver-private output object. The lifecycle of the CRTC
>>> properties will also mirror the output.
>>>
>>> Since color managment properties are all DRM blobs, we'll expose the
>>> ability to change the blob ID. The user can create blobs via libdrm
>>> (which can be done without ownership of DRM master), then set the ID via
>>> xrandr. The user will then have to ensure proper cleanup by subsequently
>>> releasing the blob.
>>
>> That sounds a bit clunky. :)
>>
>> When changing a blob ID, the change only takes effect on the next atomic
>> commit, doesn't it? How does the client trigger the atomic commit?
> 
> From the perspective of a client that wishes to change a property, the
> process between regular properties and blob properties should be
> essentially the same. Both will trigger an atomic commit when the DRM
> set property ioctl is called from our DDX driver.

That doesn't sound right — if every set property ioctl call implicitly
triggered an atomic commit, the KMS atomic API wouldn't be "atomic" at all.

Do you mean that the DDX driver triggers an atomic commit on each
property change? How is that done?


> The only difference is that DRM property blobs can be arbitrary in size,
> and needs to be passed by reference through its DRM-defined blob ID.
> Because of this, the client has to create the blob, save it's id, call
> libXrandr to change it, then destroy the blob after it's been committed.
> 
> The client has to call libXrandr due to DRM permissions. IIRC, there can
> be only one DRM master. And since xserver is DRM master, an external
> application cannot set DRM properties unless it goes through X. However,
> creating and destroying DRM property blobs and can be done by anyone.
> 
> Was this the source of the clunkiness? I've thought about having DDX
> create and destroy the blob instead, but that needs an interface for the
> client to get arbitrarily sized data to DDX. I'm not aware of any good
> ways to do so. Don't think the kernel can do this for us either. It does
> create the blob for legacy gamma, but that's because there's a dedicated
> ioctl for it.

Maybe there are better approaches than exposing these properties to
clients directly. See also my latest follow-up on patch 3.


>>> @@ -1604,6 +1623,18 @@ static void drmmode_output_dpms(xf86OutputPtr
>>> output, int mode)
>>>       }
>>>   }
>>>   +static Bool drmmode_crtc_property_ignore(drmModePropertyPtr prop)
>>> +{
>>> +    if (!prop)
>>> +        return TRUE;
>>> +    /* Ignore CRTC gamma lut sizes */
>>> +    if (!strcmp(prop->name, "GAMMA_LUT_SIZE") ||
>>> +        !strcmp(prop->name, "DEGAMMA_LUT_SIZE"))
>>> +        return TRUE;
>>
>> Without these properties, how can a client know the LUT sizes?
> 
> Good point, I originally thought the sizes are fixed and did not need
> exposing. But who knows if they may change, or even be different per asic.

AFAIK at least Intel hardware already has different sizes in different
hardware generations. We should avoid creating an API which couldn't
also work for the modesetting driver and generic clients.


-- 
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/5] Hook up CRTC color management functions

[Michel Dänzer]

On 2018-04-10 08:02 PM, Leo Li wrote:
> On 2018-04-09 11:03 AM, Michel Dänzer wrote:
>> On 2018-03-26 10:00 PM, sunpeng.li@amd.com wrote:
>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>
>>> The functions insert into the output resource creation, and property
>>> change functions. CRTC destroy is also hooked-up for proper cleanup of
>>> the CRTC property list.
>>>
>>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
>>
>> [...]
>>
>>> @@ -1933,6 +1933,9 @@ static void
>>> drmmode_output_create_resources(xf86OutputPtr output)
>>>               }
>>>           }
>>>       }
>>> +
>>> +    if (output->crtc)
>>> +        drmmode_crtc_create_resources(output->crtc, output);
>>
>> output->crtc is only non-NULL here for outputs which are enabled at Xorg
>> startup; other outputs won't have the new properties.
> 
> Is it necessary to have the CRTC properties on a output if the CRTC is
> disabled for that output?

The set of properties exposed by an output should be constant throughout
its lifetime.


> I've tested hot-plugging with this, and the properties do initialize on
> hot-plug.

I suspect you were testing something like DP MST, where the output is
created dynamically on hot-plug. For "static" outputs such as
DVI/HDMI/VGA/LVDS (and "normal" (e)DP), drmmode_output_create_resources
is only called once during Xorg initialization, and output->crtc is NULL
at that point unless the output is enabled from the beginning (which is
only the case if either the output is connected at that point, or is
explicitly configured in xorg.conf to be on).


-- 
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 1/5] Add functions for changing CRTC color management properties

[Leo Li]

On 2018-04-11 04:39 AM, Michel Dänzer wrote:
> On 2018-04-10 08:02 PM, Leo Li wrote:
>> On 2018-04-09 11:03 AM, Michel Dänzer wrote:
>>> On 2018-03-26 10:00 PM, sunpeng.li@amd.com wrote:
>>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>>
>>>> This change adds a few functions in preparation of enabling CRTC color
>>>> managment via the randr interface.
>>>>
>>>> The driver-private CRTC object now contains a list of properties,
>>>> mirroring the driver-private output object. The lifecycle of the CRTC
>>>> properties will also mirror the output.
>>>>
>>>> Since color managment properties are all DRM blobs, we'll expose the
>>>> ability to change the blob ID. The user can create blobs via libdrm
>>>> (which can be done without ownership of DRM master), then set the ID via
>>>> xrandr. The user will then have to ensure proper cleanup by subsequently
>>>> releasing the blob.
>>>
>>> That sounds a bit clunky. :)
>>>
>>> When changing a blob ID, the change only takes effect on the next atomic
>>> commit, doesn't it? How does the client trigger the atomic commit?
>>
>>  From the perspective of a client that wishes to change a property, the
>> process between regular properties and blob properties should be
>> essentially the same. Both will trigger an atomic commit when the DRM
>> set property ioctl is called from our DDX driver.
> 
> That doesn't sound right — if every set property ioctl call implicitly
> triggered an atomic commit, the KMS atomic API wouldn't be "atomic" at all.
> 
> Do you mean that the DDX driver triggers an atomic commit on each
> property change? How is that done?
> 

Yes,

In drmmode_display.c: drmmode_output_set_property(), a call is made to
drmModeConnectorSetProperty() [libdrm].

And if drmmode_crtc_set_property() is considered, then a call can be
made to drmModeObjectSetProperty() [libdrm].

Both of these functions trigger an ioctl that eventually calls
drm_mode_obj_set_property_ioctl() within the kernel. It will trigger an
atomic commit, if the kernel driver supports it.

To be "truly atomic", the set of properties that DDX wish to change must
be compiled together using drmModeAtomicAddProperty(), then committed
all at once via drmModeAtomicCommit(). Intel's IGT test suite has a good
example of this, within igt_atomic_commit() here:
https://cgit.freedesktop.org/drm/igt-gpu-tools/tree/lib/igt_kms.c#n2997
The for-loops will add the properties, which are the committed at the end.

> 
>> The only difference is that DRM property blobs can be arbitrary in size,
>> and needs to be passed by reference through its DRM-defined blob ID.
>> Because of this, the client has to create the blob, save it's id, call
>> libXrandr to change it, then destroy the blob after it's been committed.
>>
>> The client has to call libXrandr due to DRM permissions. IIRC, there can
>> be only one DRM master. And since xserver is DRM master, an external
>> application cannot set DRM properties unless it goes through X. However,
>> creating and destroying DRM property blobs and can be done by anyone.
>>
>> Was this the source of the clunkiness? I've thought about having DDX
>> create and destroy the blob instead, but that needs an interface for the
>> client to get arbitrarily sized data to DDX. I'm not aware of any good
>> ways to do so. Don't think the kernel can do this for us either. It does
>> create the blob for legacy gamma, but that's because there's a dedicated
>> ioctl for it.
> 
> Maybe there are better approaches than exposing these properties to
> clients directly. See also my latest follow-up on patch 3.
> 

I'll reply to this in patch 3 as well.

Leo

> 
>>>> @@ -1604,6 +1623,18 @@ static void drmmode_output_dpms(xf86OutputPtr
>>>> output, int mode)
>>>>        }
>>>>    }
>>>>    +static Bool drmmode_crtc_property_ignore(drmModePropertyPtr prop)
>>>> +{
>>>> +    if (!prop)
>>>> +        return TRUE;
>>>> +    /* Ignore CRTC gamma lut sizes */
>>>> +    if (!strcmp(prop->name, "GAMMA_LUT_SIZE") ||
>>>> +        !strcmp(prop->name, "DEGAMMA_LUT_SIZE"))
>>>> +        return TRUE;
>>>
>>> Without these properties, how can a client know the LUT sizes?
>>
>> Good point, I originally thought the sizes are fixed and did not need
>> exposing. But who knows if they may change, or even be different per asic.
> 
> AFAIK at least Intel hardware already has different sizes in different
> hardware generations. We should avoid creating an API which couldn't
> also work for the modesetting driver and generic clients.
> 
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 2/5] Hook up CRTC color management functions

[Leo Li]

On 2018-04-11 04:48 AM, Michel Dänzer wrote:
> On 2018-04-10 08:02 PM, Leo Li wrote:
>> On 2018-04-09 11:03 AM, Michel Dänzer wrote:
>>> On 2018-03-26 10:00 PM, sunpeng.li@amd.com wrote:
>>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>>
>>>> The functions insert into the output resource creation, and property
>>>> change functions. CRTC destroy is also hooked-up for proper cleanup of
>>>> the CRTC property list.
>>>>
>>>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
>>>
>>> [...]
>>>
>>>> @@ -1933,6 +1933,9 @@ static void
>>>> drmmode_output_create_resources(xf86OutputPtr output)
>>>>                }
>>>>            }
>>>>        }
>>>> +
>>>> +    if (output->crtc)
>>>> +        drmmode_crtc_create_resources(output->crtc, output);
>>>
>>> output->crtc is only non-NULL here for outputs which are enabled at Xorg
>>> startup; other outputs won't have the new properties.
>>
>> Is it necessary to have the CRTC properties on a output if the CRTC is
>> disabled for that output?
> 
> The set of properties exposed by an output should be constant throughout
> its lifetime.
> 

I see.

I'm not sure what the standard is for listing 'disabled' properties like
this. What do clients expect? Should the properties be configured as 0
and immutable until a valid CRTC is attached?

This would mean that DDX has to know the list of available CRTC
properties before a CRTC is available. I guess we can hard-code a list
of expected properties from kernel DRM, if that's not of concern. And if
they don't exist because of version differences, we can just leave them
disabled.

> 
>> I've tested hot-plugging with this, and the properties do initialize on
>> hot-plug.
> 
> I suspect you were testing something like DP MST, where the output is
> created dynamically on hot-plug. For "static" outputs such as
> DVI/HDMI/VGA/LVDS (and "normal" (e)DP), drmmode_output_create_resources
> is only called once during Xorg initialization, and output->crtc is NULL
> at that point unless the output is enabled from the beginning (which is
> only the case if either the output is connected at that point, or is
> explicitly configured in xorg.conf to be on).
> 

Yep, I was using a DP... This definitely should be fixed.

Leo

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

Re: [PATCH xf86-video-amdgpu 3/5] Keep CRTC properties consistent

[Leo Li]

On 2018-04-11 04:39 AM, Michel Dänzer wrote:
> On 2018-04-10 08:02 PM, Leo Li wrote:
>> On 2018-04-09 11:03 AM, Michel Dänzer wrote:
>>> On 2018-03-26 10:00 PM, sunpeng.li@amd.com wrote:
>>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>>
>>>> In cases where CRTC properties are updated without going through
>>>> RRChangeOutputProperty, we don't update the properties in user land.
>>>>
>>>> Consider setting legacy gamma. It doesn't go through
>>>> RRChangeOutputProperty, but modifies the CRTC's color management
>>>> properties. Unless they are updated, the user properties will remain
>>>> stale.
>>>
>>> Can you describe a bit more how the legacy gamma and the new properties
>>> interact?
>>>
>>
>> Sure thing, I'll include this in the message for v2:
>>
>> In kernel, the legacy set gamma interface is essentially an adapter to
>> the non-legacy set properties interface. In the end, they both set the
>> same property to a DRM property blob, which contains the gamma lookup
>> table. The key difference between them is how this blob is created.
>>
>> For legacy gamma, the kernel takes 3 arrays from user-land, and creates
>> the blob using them. Note that a blob is identified by it's blob_id.
>>
>> For non-legacy gamma, the kernel takes a blob_id from user-land that
>> references the blob. This means user-land is responsible for creating
>> the blob.
>>
>>  From the perspective of RandR, this presents some problems. Since both
>> paths modify the same property, RandR must keep the reported property
>> value up-to-date with which ever path is used:
>>
>> 1. Legacy gamma via
>> xrandr --output <output_here> --gamma x:x:x
>> 2. Non-legacy color properties via
>> xrandr --output <output_here> --set GAMMA_LUT <blob_id>
>>
>> Keeping the value up-to-date isn't a problem for 2, since RandR updates
>> it for us as part of changing output properties.
>>
>> But if 1 is used, the property blob is created within kernel, and RandR
>> is unaware of the new blob_id. To update it, we need to ask kernel about
>> it.
>>
>> --- continue with rest of message ---
>>>
>>>> Therefore, add a function to update user CRTC properties by querying
>>>> DRM,
>>>> and call it whenever legacy gamma is changed.
>>>
>>> Note that drmmode_crtc_gamma_do_set is called from
>>> drmmode_set_mode_major, i.e. on every modeset or under some
>>> circumstances when a DRI3 client stops page flipping.
>>>
>>
>> The property will have to be updated each time the legacy set gamma
>> ioctl is called, since a new blob (with a new blob_id) is created each
>> time.
>>
>> Not sure if this is a good idea, but perhaps we can have a flag that
>> explicitly enable one or the other, depending on user preference? A
>> user-only property with something like:
>>
>> 0: Use legacy gamma, calls to change non-legacy properties are ignored.
>> 1: Use non-legacy, calls to legacy gamma will be ignored.
>>
>> On 0, we can remove/disable all non-legacy properties from the property
>> list, and avoid having to update them. On 1, we'll enable the
>> properties, and won't have to update them either since legacy gamma is
>> "disabled". It has the added benefit of avoiding unexpected legacy gamma
>> sets when using non-legacy, and vice versa.
> 
> Hmm. So either legacy or non-legacy clients won't work at all, or
> they'll step on each other's toes, clobbering the HW gamma LUT from each
> other.
> 
> I'm afraid neither of those alternatives sound like a good user
> experience to me.
> 
> Consider on the one hand something like Night Light / redshift, using
> legacy APIs to adjust colour temperature to the time of day. On the
> other hand, another client using the non-legacy API for say fine-tuning
> of a display's advanced gamma capabilities.
> 
> Ideally, in this case the gamma LUTs from the legacy and non-legacy APIs
> should be combined, such that the hardware LUT reflects both the colour
> temperature set by Night Light / refshift and the fine-tuning set by the
> non-legacy client. Is that feasible? If not, can you explain a little why?
> 

Hmm, combining LUTs could be feasible, but I don't think that's the
right approach.

LUTs can be combined through composition h(x) = f(g(x)), with some
interpolation involved. Once combined, it can be set using the
non-legacy API, since that'll likely have a larger LUT size.

But I think what you've mentioned raises a bigger issue of color
management conflicts in general. It doesn't have to be redshift vs
monitor correction, what if there more than 2 applications contending to
set gamma? xrandr's brightness already conflicts with redshift, and so
does some apps running on WINE. Ultimately, any (legacy or not) gamma
set requests are unified into one CRTC property in DRM, and they will
all conflict if not managed. I don't think combining two LUTs will help
here.

For the small example at hand, the ideal solution is to have redshift
use the color transformation matrix (CTM), which will be exposed as part
of the non-legacy color API. It'll need modification of redshift, but at
least it won't conflict with anything gamma related.

----

Jumping back on some patch 1 topics:

Are there ways to get arbitrarily (no more than 4096 elements) sized
arrays from a client, to the DDX driver? Otherwise, I'm not sure if
there's another way to expose these properties, short of modifying the
RandR interface. I agree, it would definitely be nicer for clients to
not worry about DRM blobs at all.

Leo

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

Re: [PATCH xf86-video-amdgpu 3/5] Keep CRTC properties consistent

[Michel Dänzer]

On 2018-04-11 11:26 PM, Leo Li wrote:
> On 2018-04-11 04:39 AM, Michel Dänzer wrote:
>> 
>> Hmm. So either legacy or non-legacy clients won't work at all, or 
>> they'll step on each other's toes, clobbering the HW gamma LUT from
>> each other.
>> 
>> I'm afraid neither of those alternatives sound like a good user 
>> experience to me.
>> 
>> Consider on the one hand something like Night Light / redshift,
>> using legacy APIs to adjust colour temperature to the time of day.
>> On the other hand, another client using the non-legacy API for say
>> fine-tuning of a display's advanced gamma capabilities.
>> 
>> Ideally, in this case the gamma LUTs from the legacy and non-legacy
>> APIs should be combined, such that the hardware LUT reflects both
>> the colour temperature set by Night Light / refshift and the
>> fine-tuning set by the non-legacy client. Is that feasible? If not,
>> can you explain a little why?
> 
> Hmm, combining LUTs could be feasible, but I don't think that's the 
> right approach.
> 
> LUTs can be combined through composition h(x) = f(g(x)), with some 
> interpolation involved. Once combined, it can be set using the 
> non-legacy API, since that'll likely have a larger LUT size.
> 
> But I think what you've mentioned raises a bigger issue of color 
> management conflicts in general. It doesn't have to be redshift vs 
> monitor correction, what if there more than 2 applications contending
> to set gamma? xrandr's brightness already conflicts with redshift,
> and so does some apps running on WINE.

What you're describing is an X11 design flaw, which we cannot do
anything about in the DDX driver.

What I want to avoid is repeating a similar situation as we had before
xserver 1.19, where one could have either working per-window colormaps
and global gamma, or per-CRTC gamma via RandR, not all at the same
time. (Before xserver 1.7, they would clobber the HW LUT from each
other) I fixed this in
https://cgit.freedesktop.org/xorg/xserver/commit/?id=b4e46c0444bb09f4af59d9d13acc939a0fbbc6d6
by composing the LUTs.


> For the small example at hand, the ideal solution is to have
> redshift use the color transformation matrix (CTM), which will be
> exposed as part of the non-legacy color API. It'll need modification
> of redshift, but at least it won't conflict with anything gamma
> related.

From past experience, it can take a long time (up to forever) for some
clients to be updated like this. E.g. it's unlikely at this point that
libraries such as SDL1 will ever be updated for the new API, so I'm
afraid users would run into things like
https://bugs.freedesktop.org/show_bug.cgi?id=27222 again.

(Besides, it would conflict with anything else using the same API, as
you described above, so it's not even obviously the ideal solution)


> Jumping back on some patch 1 topics:
> 
> Are there ways to get arbitrarily (no more than 4096 elements) sized 
> arrays from a client, to the DDX driver?

Seems like the RRChangeOutputProperty request could work.


> I agree, it would definitely be nicer for clients to not worry about
> DRM blobs at all.

Indeed. E.g. as a bonus, it would allow this to work even with a remote
display connection.


I'm holding off on the more detailed discussion about the other patches
until there is a plan for this fundamental issue.


-- 
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/5] Keep CRTC properties consistent

[Leo Li]

On 2018-04-12 06:30 AM, Michel Dänzer wrote:
> On 2018-04-11 11:26 PM, Leo Li wrote:
>> On 2018-04-11 04:39 AM, Michel Dänzer wrote:
>>>
>>> Hmm. So either legacy or non-legacy clients won't work at all, or
>>> they'll step on each other's toes, clobbering the HW gamma LUT from
>>> each other.
>>>
>>> I'm afraid neither of those alternatives sound like a good user
>>> experience to me.
>>>
>>> Consider on the one hand something like Night Light / redshift,
>>> using legacy APIs to adjust colour temperature to the time of day.
>>> On the other hand, another client using the non-legacy API for say
>>> fine-tuning of a display's advanced gamma capabilities.
>>>
>>> Ideally, in this case the gamma LUTs from the legacy and non-legacy
>>> APIs should be combined, such that the hardware LUT reflects both
>>> the colour temperature set by Night Light / refshift and the
>>> fine-tuning set by the non-legacy client. Is that feasible? If not,
>>> can you explain a little why?
>>
>> Hmm, combining LUTs could be feasible, but I don't think that's the
>> right approach.
>>
>> LUTs can be combined through composition h(x) = f(g(x)), with some
>> interpolation involved. Once combined, it can be set using the
>> non-legacy API, since that'll likely have a larger LUT size.
>>
>> But I think what you've mentioned raises a bigger issue of color
>> management conflicts in general. It doesn't have to be redshift vs
>> monitor correction, what if there more than 2 applications contending
>> to set gamma? xrandr's brightness already conflicts with redshift,
>> and so does some apps running on WINE.
> 
> What you're describing is an X11 design flaw, which we cannot do
> anything about in the DDX driver.
> 
> What I want to avoid is repeating a similar situation as we had before
> xserver 1.19, where one could have either working per-window colormaps
> and global gamma, or per-CRTC gamma via RandR, not all at the same
> time. (Before xserver 1.7, they would clobber the HW LUT from each
> other) I fixed this in
> https://cgit.freedesktop.org/xorg/xserver/commit/?id=b4e46c0444bb09f4af59d9d13acc939a0fbbc6d6
> by composing the LUTs.
> 
> 
>> For the small example at hand, the ideal solution is to have
>> redshift use the color transformation matrix (CTM), which will be
>> exposed as part of the non-legacy color API. It'll need modification
>> of redshift, but at least it won't conflict with anything gamma
>> related.
> 
>  From past experience, it can take a long time (up to forever) for some
> clients to be updated like this. E.g. it's unlikely at this point that
> libraries such as SDL1 will ever be updated for the new API, so I'm
> afraid users would run into things like
> https://bugs.freedesktop.org/show_bug.cgi?id=27222 again.
> 
> (Besides, it would conflict with anything else using the same API, as
> you described above, so it's not even obviously the ideal solution)
> 

Fair enough, it's better to have the frequent redshift+monitor adjust
use-case working now instead of pushing an entirely new API.

> 
>> Jumping back on some patch 1 topics:
>>
>> Are there ways to get arbitrarily (no more than 4096 elements) sized
>> arrays from a client, to the DDX driver?
> 
> Seems like the RRChangeOutputProperty request could work.
> 
> 
>> I agree, it would definitely be nicer for clients to not worry about
>> DRM blobs at all.
> 
> Indeed. E.g. as a bonus, it would allow this to work even with a remote
> display connection.
> 
> 
> I'm holding off on the more detailed discussion about the other patches
> until there is a plan for this fundamental issue.
> 
> 

I'll take another look at RRChangeOutputProperty. Seems I missed the
'length' argument when I first went through it.

Once the blobs are gone, the other issues should be much simpler to
solve. I'll see if I can come up with some v2's.

A side question: How does xrandr display lengthy properties, like LUTs?
Can users set it via --set still?

Leo


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

Re: [PATCH xf86-video-amdgpu 3/5] Keep CRTC properties consistent

[Michel Dänzer]

On 2018-04-12 06:48 PM, Leo Li wrote:
>
> A side question: How does xrandr display lengthy properties, like LUTs?

Presumably it would be displayed the same way as the EDID property, e.g.:

	EDID:
		00ffffffffffff0022f06f3201010101
		181b0103803420782a4ca5a7554da226
		105054210800b30095008100d1c0a9c0
		81c0a9408180283c80a070b023403020
		360006442100001a000000fd00323c1e
		5011000a202020202020000000fc0048
		5020453234320a2020202020000000ff
		00434e43373234305a37470a20200111
		020318b148101f04130302121167030c
		0010000022e2002b023a801871382d40
		582c450006442100001e023a80d07238
		2d40102c458006442100001e011d0072
		51d01e206e28550006442100001e011d
		00bc52d01e20b828554006442100001e
		8c0ad08a20e02d10103e960006442100
		0018000000000000000000000000002f


> Can users set it via --set still?

Not sure, maybe not. If not, maybe you can write a simple
proof-of-concept client, that should also be useful for review and
testing of these 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