[PATCH xf86-video-amdgpu 00/13] Enabling Color Management - Round 2

[PATCH xf86-video-amdgpu 00/13] Enabling Color Management - Round 2

[sunpeng.li-5C7GfCeVMHo]

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


This patchset ended up looking quite different from the first. To address some
fundamental issues, the design had to be reworked.

Things gathered from previous review:

1. User client should not have to handle DRM blob objects. That should be the
   job of the DDX driver.

2. Since legacy gamma sets the same properties within DRM as non-legacy gamma,
   the previous implementation created conflicts when setting both.

    * We should at least support this use-case: Nightlight enabled (uses legacy
      gamma), with monitor correction enabled (non-legacy gamma)

3. Since color management properties are attached to the CRTC, the previous
   revision has the properties hooked onto the CRTC life-cycle, not the output
   life-cycle. This is problematic, since clients expect properties on an
   output to stay consistent throughout its life.

4. Although not mentioned during review, the color properties did not persist
   across certain events, such as DPMS state changes or hotplugs.


To address the above, the following was done:

1. XRandR allows setting of array-like properties. This is used to directly
   pass LUT/CTM data from the client to the DDX driver.

2. Legacy and non-legacy gamma LUTs are now merged via composition. This will
   allow both to be in effect, while only programming one LUT in kernel driver.

3. The three color management properties (Degamma LUT, Color Transform Matrix
   (CTM), and Gamma LUT) are hard-coded into the DDX driver, to be listed (as
   disabled) regardless of whether a CRTC is attached on the output, or whether
   the kernel driver supports it.

    * If kernel driver does not support color management, the properties will
      remain disabled. A `xrandr --set` will then error.

4. Color properties are now *staged* inside the driver-private CRTC object.
   This allows us to *push* the properties into kernel DRM (and consequently
   into hardware) whenever there is a need.

    * Along with staging and pushing, an *update* function is used to notify
      RandR to update the color properties listed on outputs. This can be used
      when `xrandr --auto` enables a CRTC on an output, and the output need to
      reflect the CRTC's color properties.


However, there are some things being done that aren't quite nice, to which I
don't yet have a solution. Any thoughts and suggestions are welcome:

* When using libXrandr to set the CTM property, 16bit format is used. Ideally
  we should use 64bit format, since the CTM consists of 9x64bit fixed-point
  values. However, it isn't recognized by XRRChangeOutputProperty as a valid
  format. 32 bit could work, since the CTM values are S31.32 fixed-point.
  However, using this format corrupts the data once it gets to xserver. On
  first glance, it may be the cast to long within XRRChangeOutputProperty, in
  addition to compiling 64 bit (shouldn't it use int32_t instead?).

* Since LUTs can be quite long, the output of `xrandr --prop` isn't very nice.

* Setting these properties through the xrandr app isn't supported without
  modifications to the app, due to the length of these properties. However,
  setting through libXrandr works.

Support on the xrandr app side can come as a seperate patch set. For now,
testing the new API can be done via libXrandr. I've made a sample application
here: git://people.freedesktop.org/~hwentland/color-demo-app
Clone, make, and it's ready to go.


Leo (Sunpeng) Li (13):
  Add color management properties to driver-private CRTC object
  Push color properties to kernel DRM on CRTC init
  List disabled color properties on RandR outputs without a CRTC
  Use CRTC's color properties if output has a CRTC attached
  Enable setting of color properties though RandR
  Compose non-legacy with legacy gamma LUT before pushing to kernel DRM
  Also compose LUT when setting legacy gamma.
  Set driver-private CRTC's dpms mode on disable
  Move drmmode_do_crtc_dpms
  Push staged color properties when DPMS state toggles On
  Push staged color properties on output detect
  Update color properties on modeset major
  Refactor pushing color management properties into a function

 src/drmmode_display.c | 878 ++++++++++++++++++++++++++++++++++++++++++++++----
 src/drmmode_display.h |   8 +
 2 files changed, 824 insertions(+), 62 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 01/13] Add color management properties to driver-private CRTC object

[sunpeng.li-5C7GfCeVMHo]

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

Non-legacy color management consists of 3 properties on the CRTC:
Degamma LUT, Color Transformation Matrix (CTM), and Gamma LUT.

Add these properties to the driver-private CRTC, and initialize them
when the CRTC is initialized. These values are refered to as "staged"
values. They exist in the DDX driver, but require a "push" to DRM in
order to be realized in hardware.

Also add a destructor for the driver-private CRTC, which cleans up the
non-legacy properties.

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

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 49284c6..0ffc6ad 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -747,6 +747,83 @@ drmmode_crtc_scanout_update(xf86CrtcPtr crtc, DisplayModePtr mode,
 	}
 }
 
+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,
+};
+
+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;
+}
+
+/**
+ * Query DRM for the property value of the specified color management property,
+ * on the specified CRTC.
+ *
+ * @crtc: The CRTC to query DRM properties on.
+ * @prop_id: Color management property enum.
+ * Return: The property value (if property is DRM blob, return blob_id) of the
+ *         specified color property. 0 if property not found.
+ */
+static uint64_t get_drm_cm_prop_value(xf86CrtcPtr crtc,
+				      enum drmmode_cm_prop prop_id)
+{
+	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+	drmModeObjectPropertiesPtr drm_props;
+	int i;
+
+	/* Get list of DRM CRTC properties, and their values */
+	drm_props = drmModeObjectGetProperties(pAMDGPUEnt->fd,
+					       drmmode_crtc->mode_crtc->crtc_id,
+					       DRM_MODE_OBJECT_CRTC);
+	if (!drm_props)
+		goto err_allocs;
+
+	for (i = 0; i < drm_props->count_props; i++) {
+		drmModePropertyPtr drm_prop;
+
+		drm_prop = drmModeGetProperty(pAMDGPUEnt->fd,
+					      drm_props->props[i]);
+		if (!drm_prop)
+			goto err_allocs;
+
+		if (get_cm_enum_from_str(drm_prop->name) == prop_id){
+			drmModeFreeProperty(drm_prop);
+			return drm_props->prop_values[i];
+		}
+
+		drmModeFreeProperty(drm_prop);
+	}
+
+err_allocs:
+	drmModeFreeObjectProperties(drm_props);
+	return 0;
+}
+
 static void
 drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t *red, uint16_t *green,
 			  uint16_t *blue, int size)
@@ -1293,6 +1370,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,
@@ -1309,7 +1402,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,
 };
 
@@ -1353,6 +1446,18 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, drmModeResPtr mode_res
 	crtc->driver_private = drmmode_crtc;
 	drmmode_crtc_hw_id(crtc);
 
+	drmmode_crtc->gamma_lut_size =
+		(uint32_t)get_drm_cm_prop_value(crtc, CM_GAMMA_LUT_SIZE);
+	drmmode_crtc->degamma_lut_size =
+		(uint32_t)get_drm_cm_prop_value(crtc, CM_DEGAMMA_LUT_SIZE);
+	drmmode_crtc->ctm = calloc(1, sizeof(*drmmode_crtc->ctm));
+	if (drmmode_crtc->ctm == NULL)
+		return 0;
+
+	/* Init to identity matrix. Values are in S31.32 fixed-point format */
+	drmmode_crtc->ctm->matrix[0] = drmmode_crtc->ctm->matrix[4] =
+		drmmode_crtc->ctm->matrix[8] = (uint64_t)1 << 32;
+
 	/* 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 25ae9f8..21498e3 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -109,6 +109,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 02/13] Push color properties to kernel DRM on CRTC init

[sunpeng.li-5C7GfCeVMHo]

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

Push staged values on the driver-private CRTC, to kernel DRM when it's
initialized. This is to flush out any previous state that hardware was
in, and set them to their default values.

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

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 0ffc6ad..85de01e 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -824,6 +824,133 @@ err_allocs:
 	return 0;
 }
 
+/**
+ * Query DRM for the property ID - as recognized by DRM - for the specified
+ * color management property, on the specified CRTC.
+ *
+ * @crtc: The CRTC to query DRM properties on.
+ * @prop_id: Color management property enum.
+ *
+ * Return the DRM property ID, if the property exists. 0 otherwise.
+ */
+static uint32_t get_drm_cm_prop_id(xf86CrtcPtr crtc,
+				   enum drmmode_cm_prop prop_id)
+{
+	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+	drmModeObjectPropertiesPtr drm_props;
+	int i;
+
+	drm_props = drmModeObjectGetProperties(pAMDGPUEnt->fd,
+					       drmmode_crtc->mode_crtc->crtc_id,
+					       DRM_MODE_OBJECT_CRTC);
+	if (!drm_props)
+		goto err_allocs;
+
+	for (i = 0; i < drm_props->count_props; i++) {
+		drmModePropertyPtr drm_prop;
+
+		drm_prop = drmModeGetProperty(pAMDGPUEnt->fd,
+					      drm_props->props[i]);
+		if (!drm_prop)
+			goto err_allocs;
+
+		if (get_cm_enum_from_str(drm_prop->name) == prop_id){
+			drmModeFreeProperty(drm_prop);
+			return drm_props->props[i];
+		}
+
+		drmModeFreeProperty(drm_prop);
+	}
+
+err_allocs:
+	drmModeFreeObjectProperties(drm_props);
+	return 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)
+{
+	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+	size_t expected_bytes = 0;
+	uint32_t created_blob_id = 0;
+	void *blob_data = NULL;
+	uint32_t drm_prop_id;
+	Bool free_blob_data = FALSE;
+	int ret;
+
+	drm_prop_id = get_drm_cm_prop_id(crtc, cm_prop_index);
+	/* It's possible that kernel driver does not support color management.
+	 */
+	if (!drm_prop_id)
+		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->degamma_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);
+			if (free_blob_data)
+				free(blob_data);
+			return BadRequest;
+		}
+	}
+
+	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);
+		if (free_blob_data)
+			free(blob_data);
+		return BadRequest;
+	}
+
+	if (free_blob_data)
+		free(blob_data);
+
+	return Success;
+}
+
 static void
 drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t *red, uint16_t *green,
 			  uint16_t *blue, int size)
@@ -1433,6 +1560,7 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, drmModeResPtr mode_res
 	drmmode_crtc_private_ptr drmmode_crtc;
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(pScrn);
 	AMDGPUInfoPtr info = AMDGPUPTR(pScrn);
+	int i;
 
 	crtc = xf86CrtcCreate(pScrn, &info->drmmode_crtc_funcs);
 	if (crtc == NULL)
@@ -1458,6 +1586,14 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, drmModeResPtr mode_res
 	drmmode_crtc->ctm->matrix[0] = drmmode_crtc->ctm->matrix[4] =
 		drmmode_crtc->ctm->matrix[8] = (uint64_t)1 << 32;
 
+	/* Push properties to initialize them */
+	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))
+			return 0;
+	}
+
 	/* Mark num'th crtc as in use on this device. */
 	pAMDGPUEnt->assigned_crtcs |= (1 << num);
 	xf86DrvMsgVerb(pScrn->scrnIndex, X_INFO, AMDGPU_LOGLEVEL_DEBUG,
-- 
2.7.4

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

[PATCH xf86-video-amdgpu 03/13] List disabled color properties on RandR outputs without a CRTC

[sunpeng.li-5C7GfCeVMHo]

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

The properties on an RandR output needs to stay consistent throughout
it's lifecycle. However, we cannot list color properties on an output if
there is no CRTC attached.

Therefore, create a fake CRTC, and initialize "disabled" color
properites on outputs without a CRTC.

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

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 85de01e..c28796c 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -869,6 +869,124 @@ err_allocs:
 }
 
 /**
+ * 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. If the request is pending, then the change will make it's way into
+ * the kernel DRM driver. Otherwise, the request will be a user-land only
+ * update.
+ *
+ * @output: RandR output to set the property on.
+ * @crtc: The driver-private CRTC object containing the color properties.
+ * @cm_prop_index: Color management property to configure and change.
+ * @pending: Whether this request is pending.
+ *
+ * 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 pending)
+{
+	Bool need_configure = TRUE;
+	unsigned long length = 0;
+	void *data = NULL;
+	int format = 0;
+	uint16_t value;
+	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->gamma_lut_size;
+		range[0] = 0;
+		range[1] = -1;
+
+	} else if (cm_prop_index == CM_DEGAMMA_LUT_SIZE) {
+		format = 32;
+		length = 1;
+		data = &crtc->degamma_lut_size;
+		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->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;
+			value = 0;
+			data = &value;
+		}
+	} 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->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;
+			value = 0;
+			data = &value;
+		}
+	} else {
+		/* CTM is fixed-point S31.32 format. */
+		format = 16;
+		need_configure = FALSE;
+		if (crtc->ctm) {
+			length = sizeof(*crtc->ctm) >> 1;
+			data = crtc->ctm;
+		} else {
+			length = 1;
+			value = 0;
+			data = &value;
+		}
+	}
+
+	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;
+		}
+	}
+
+	err = RRChangeOutputProperty(output->randr_output, atom,
+				     XA_INTEGER, format,
+				     PropModeReplace,
+				     length, data, FALSE, pending);
+	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
@@ -1851,6 +1969,40 @@ static Bool drmmode_property_ignore(drmModePropertyPtr prop)
 	return FALSE;
 }
 
+/**
+ * Using a fake CRTC, configure and change color properties for the output,
+ * using the default (0) color properties on the fake CRTC. Use to initialize
+ * color management properties on an output where a CRTC has not yet been
+ * attached.
+ */
+static void drmmode_disabled_crtc_create_resources(xf86OutputPtr output)
+{
+	drmmode_crtc_private_ptr fake_crtc;
+	int i, ret;
+
+	/* Create a fake crtc */
+	fake_crtc = calloc(1, sizeof(drmmode_crtc_private_rec));
+	if (!fake_crtc) {
+		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+			   "Memory error creating resources for fake CRTC\n");
+		return;
+	}
+
+	for (i = 0; i < CM_NUM_PROPS; i++) {
+		ret = rr_configure_and_change_cm_property(output, fake_crtc, i,
+							  FALSE);
+		if (ret) {
+			xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+				   "Configur and change cm property error %d\n",
+				   ret);
+			break;
+		}
+	}
+
+	free(fake_crtc);
+	return;
+}
+
 static void drmmode_output_create_resources(xf86OutputPtr output)
 {
 	AMDGPUInfoPtr info = AMDGPUPTR(output->scrn);
@@ -1978,6 +2130,8 @@ static void drmmode_output_create_resources(xf86OutputPtr output)
 			}
 		}
 	}
+
+	drmmode_disabled_crtc_create_resources(output);
 }
 
 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 04/13] Use CRTC's color properties if output has a CRTC attached

[sunpeng.li-5C7GfCeVMHo]

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

Previously, all outputs will have disabled color properties initialized.
However, an output with a CRTC attached should use the properties on the
attached CRTC.

To do so, an color properties "update" function is added. It takes
staged properties within the driver-private CRTC, and executes a
non-pending RandR configure + change on all outputs it's attached on.
This effectively updates the output properties listed by RandR, without
unnecessarily pushing changes to kernel DRM.

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

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index c28796c..ac50e19 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -987,6 +987,42 @@ static int rr_configure_and_change_cm_property(xf86OutputPtr output,
 }
 
 /**
+ * Take the staged color properties on the CRTC, and updated the values within
+ * RandR. All outputs using the given CRTC will have their color properties
+ * updated in user-land only. i.e. changes will not be pushed to DRM.
+ *
+ * @crtc: The CRTC containing staged properties.
+ */
+static void drmmode_crtc_update_cm_props(xf86CrtcPtr crtc)
+{
+	xf86CrtcConfigPtr xf86_crtc_config = XF86_CRTC_CONFIG_PTR(crtc->scrn);
+	int i, j, ret;
+
+	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+
+	for (i = 0; i < xf86_crtc_config->num_output; i++) {
+		xf86OutputPtr output = xf86_crtc_config->output[i];
+		if (output->crtc != crtc)
+			continue;
+
+		for (j = 0; j < CM_NUM_PROPS; j++) {
+			/* Non-pending configure and change: Just updating
+			 * values on user-side */
+			ret = rr_configure_and_change_cm_property(output,
+								  drmmode_crtc,
+								  j, FALSE);
+			if (ret) {
+				xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+					   "Error updating color properties: %d\n",
+					   ret);
+				break;
+			}
+		}
+	}
+
+}
+
+/**
  * Push staged color management properties on the CRTC to DRM.
  *
  * @crtc: The CRTC containing staged properties
@@ -2131,7 +2167,10 @@ static void drmmode_output_create_resources(xf86OutputPtr output)
 		}
 	}
 
-	drmmode_disabled_crtc_create_resources(output);
+	if (output->crtc)
+		drmmode_crtc_update_cm_props(output->crtc);
+	else
+		drmmode_disabled_crtc_create_resources(output);
 }
 
 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 05/13] Enable setting of color properties though 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
3. Update all RandR outputs using this CRTC to list the new property

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 | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index ac50e19..7223b93 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -1023,6 +1023,84 @@ static void drmmode_crtc_update_cm_props(xf86CrtcPtr crtc)
 }
 
 /**
+* Stage a color management property onto 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;
+
+	/* 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 != 16 ||
+		    (size_t)(value->size << 1) != 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
@@ -2204,8 +2282,23 @@ 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;
+		drmmode_crtc_update_cm_props(output->crtc);
+		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 06/13] Compose non-legacy with legacy gamma LUT before pushing to kernel DRM

[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 | 106 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 1 deletion(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 7223b93..9c8c344 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -869,6 +869,95 @@ err_allocs:
 }
 
 /**
+ * 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;
+}
+
+/**
  * 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. If the request is pending, then the change will make it's way into
@@ -1130,7 +1219,22 @@ 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->degamma_lut;
+
+		/* Compose legacy and non-legacy LUT */
+		if (drmmode_crtc->gamma_lut) {
+			blob_data = malloc(expected_bytes);
+			if (!blob_data)
+				return BadAlloc;
+			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);
+			free_blob_data = TRUE;
+		} else
+			blob_data = NULL;
+
 	} else if (cm_prop_index == CM_DEGAMMA_LUT) {
 		expected_bytes = sizeof(struct drm_color_lut) *
 					drmmode_crtc->degamma_lut_size;
-- 
2.7.4

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

[PATCH xf86-video-amdgpu 07/13] 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.

If a non-legacy LUT has not been set (i.e. using the default value),
the legacy LUT will act independantly. However, instead of using DRM's
legacy gamma code-path, we adapt to the non-legacy path by upscaling the
LUT to non-legacy size.

It's also possible that the kernel driver doesn't support non-legacy
color management. In which case, we fall back to legacy gamma.

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

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 9c8c344..2b38a71 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -958,6 +958,63 @@ static void drmmode_lut_compose(uint16_t *a_red,
 }
 
 /**
+ * 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. If the request is pending, then the change will make it's way into
@@ -1293,9 +1350,59 @@ 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);
+	struct drm_color_lut *composed;
+	uint32_t drm_prop_id, created_blob_id;
+	size_t bytes;
+	int ret;
+
+	drm_prop_id = get_drm_cm_prop_id(crtc, CM_GAMMA_LUT);
+	/* Use legacy if kernel does not support non-legacy gamma */
+	if (!drm_prop_id) {
+		drmModeCrtcSetGamma(pAMDGPUEnt->fd,
+				    drmmode_crtc->mode_crtc->crtc_id,
+				    size, red, green, blue);
+		return;
+	}
+
+	bytes = sizeof(*composed) * drmmode_crtc->gamma_lut_size;
+
+	composed = malloc(bytes);
+	if (!composed) {
+		xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
+			   "Memory error allocating legacy LUT.");
+		return;
+	}
+
+	if (drmmode_crtc->gamma_lut)
+		drmmode_lut_compose(red, green, blue, drmmode_crtc->gamma_lut,
+				    composed, size,
+				    drmmode_crtc->gamma_lut_size);
+	else
+		drmmode_lut_interpolate(red, green, blue, composed,
+					size, drmmode_crtc->gamma_lut_size);
+
+	ret = drmModeCreatePropertyBlob(pAMDGPUEnt->fd, composed, bytes,
+					&created_blob_id);
+	if (ret) {
+		xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
+			   "Creating Gamma LUT failed with errno %d\n",
+			   ret);
+		free(composed);
+		return;
+	}
+
+	ret = drmModeObjectSetProperty(pAMDGPUEnt->fd,
+				       drmmode_crtc->mode_crtc->crtc_id,
+				       DRM_MODE_OBJECT_CRTC,
+				       drm_prop_id,
+				       (uint64_t)created_blob_id);
+
+	if (ret)
+		xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
+			   "Setting Gamma LUT failed with errno %d\n",
+			   ret);
 
-	drmModeCrtcSetGamma(pAMDGPUEnt->fd, drmmode_crtc->mode_crtc->crtc_id,
-			    size, red, green, blue);
+	free(composed);
 }
 
 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 08/13] Set driver-private CRTC's dpms mode on disable

[sunpeng.li-5C7GfCeVMHo]

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

The dpms_mode flag on the driver-private CRTC was not being set when
it's DPMS state is set to off. This causes some problems when toggling
it back on, as some conditionals check this flag.

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

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 2b38a71..f86f99a 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -347,6 +347,7 @@ drmmode_crtc_dpms(xf86CrtcPtr crtc, int mode)
 		drmModeSetCrtc(pAMDGPUEnt->fd, drmmode_crtc->mode_crtc->crtc_id,
 			       0, 0, 0, NULL, 0, NULL);
 		drmmode_fb_reference(pAMDGPUEnt->fd, &drmmode_crtc->fb, NULL);
+		drmmode_crtc->dpms_mode = mode;
 	} else if (drmmode_crtc->dpms_mode != DPMSModeOn)
 		crtc->funcs->set_mode_major(crtc, &crtc->mode, crtc->rotation,
 					    crtc->x, crtc->y);
-- 
2.7.4

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

[PATCH xf86-video-amdgpu 09/13] Move drmmode_do_crtc_dpms

[sunpeng.li-5C7GfCeVMHo]

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

To avoid forward declarations in upcomming changes

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

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index f86f99a..45c582c 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -276,65 +276,6 @@ int drmmode_crtc_get_ust_msc(xf86CrtcPtr crtc, CARD64 *ust, CARD64 *msc)
 }
 
 static void
-drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
-{
-	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
-	ScrnInfoPtr scrn = crtc->scrn;
-	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(scrn);
-	CARD64 ust;
-	int ret;
-
-	if (drmmode_crtc->dpms_mode == DPMSModeOn && mode != DPMSModeOn) {
-		uint32_t seq;
-
-		drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
-						drmmode_crtc->flip_pending);
-
-		/*
-		 * On->Off transition: record the last vblank time,
-		 * sequence number and frame period.
-		 */
-		if (!drmmode_wait_vblank(crtc, DRM_VBLANK_RELATIVE, 0, 0, &ust,
-					 &seq))
-			xf86DrvMsg(scrn->scrnIndex, X_ERROR,
-				   "%s cannot get last vblank counter\n",
-				   __func__);
-		else {
-			CARD64 nominal_frame_rate, pix_in_frame;
-
-			drmmode_crtc->dpms_last_ust = ust;
-			drmmode_crtc->dpms_last_seq = seq;
-			nominal_frame_rate = crtc->mode.Clock;
-			nominal_frame_rate *= 1000;
-			pix_in_frame = crtc->mode.HTotal * crtc->mode.VTotal;
-			if (nominal_frame_rate == 0 || pix_in_frame == 0)
-				nominal_frame_rate = DEFAULT_NOMINAL_FRAME_RATE;
-			else
-				nominal_frame_rate /= pix_in_frame;
-			drmmode_crtc->dpms_last_fps = nominal_frame_rate;
-		}
-	} else if (drmmode_crtc->dpms_mode != DPMSModeOn && mode == DPMSModeOn) {
-		/*
-		 * Off->On transition: calculate and accumulate the
-		 * number of interpolated vblanks while we were in Off state
-		 */
-		ret = drmmode_get_current_ust(pAMDGPUEnt->fd, &ust);
-		if (ret)
-			xf86DrvMsg(scrn->scrnIndex, X_ERROR,
-				   "%s cannot get current time\n", __func__);
-		else if (drmmode_crtc->dpms_last_ust) {
-			CARD64 time_elapsed, delta_seq;
-			time_elapsed = ust - drmmode_crtc->dpms_last_ust;
-			delta_seq = time_elapsed * drmmode_crtc->dpms_last_fps;
-			delta_seq /= 1000000;
-			drmmode_crtc->interpolated_vblanks += delta_seq;
-
-		}
-	}
-	drmmode_crtc->dpms_mode = mode;
-}
-
-static void
 drmmode_crtc_dpms(xf86CrtcPtr crtc, int mode)
 {
 	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
@@ -1346,6 +1287,65 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
 }
 
 static void
+drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
+{
+	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+	ScrnInfoPtr scrn = crtc->scrn;
+	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(scrn);
+	CARD64 ust;
+	int ret;
+
+	if (drmmode_crtc->dpms_mode == DPMSModeOn && mode != DPMSModeOn) {
+		uint32_t seq;
+
+		drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
+						drmmode_crtc->flip_pending);
+
+		/*
+		 * On->Off transition: record the last vblank time,
+		 * sequence number and frame period.
+		 */
+		if (!drmmode_wait_vblank(crtc, DRM_VBLANK_RELATIVE, 0, 0, &ust,
+					 &seq))
+			xf86DrvMsg(scrn->scrnIndex, X_ERROR,
+				   "%s cannot get last vblank counter\n",
+				   __func__);
+		else {
+			CARD64 nominal_frame_rate, pix_in_frame;
+
+			drmmode_crtc->dpms_last_ust = ust;
+			drmmode_crtc->dpms_last_seq = seq;
+			nominal_frame_rate = crtc->mode.Clock;
+			nominal_frame_rate *= 1000;
+			pix_in_frame = crtc->mode.HTotal * crtc->mode.VTotal;
+			if (nominal_frame_rate == 0 || pix_in_frame == 0)
+				nominal_frame_rate = DEFAULT_NOMINAL_FRAME_RATE;
+			else
+				nominal_frame_rate /= pix_in_frame;
+			drmmode_crtc->dpms_last_fps = nominal_frame_rate;
+		}
+	} else if (drmmode_crtc->dpms_mode != DPMSModeOn && mode == DPMSModeOn) {
+		/*
+		 * Off->On transition: calculate and accumulate the
+		 * number of interpolated vblanks while we were in Off state
+		 */
+		ret = drmmode_get_current_ust(pAMDGPUEnt->fd, &ust);
+		if (ret)
+			xf86DrvMsg(scrn->scrnIndex, X_ERROR,
+				   "%s cannot get current time\n", __func__);
+		else if (drmmode_crtc->dpms_last_ust) {
+			CARD64 time_elapsed, delta_seq;
+			time_elapsed = ust - drmmode_crtc->dpms_last_ust;
+			delta_seq = time_elapsed * drmmode_crtc->dpms_last_fps;
+			delta_seq /= 1000000;
+			drmmode_crtc->interpolated_vblanks += delta_seq;
+		}
+
+	}
+	drmmode_crtc->dpms_mode = mode;
+}
+
+static void
 drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t *red, uint16_t *green,
 			  uint16_t *blue, int size)
 {
-- 
2.7.4

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

[PATCH xf86-video-amdgpu 10/13] Push staged color properties when DPMS state toggles On

[sunpeng.li-5C7GfCeVMHo]

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

This will persist color management properties on a CRTC across DPMS
state changes.

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

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 45c582c..06ae902 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -1294,6 +1294,7 @@ drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(scrn);
 	CARD64 ust;
 	int ret;
+	int i;
 
 	if (drmmode_crtc->dpms_mode == DPMSModeOn && mode != DPMSModeOn) {
 		uint32_t seq;
@@ -1341,6 +1342,11 @@ drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
 			drmmode_crtc->interpolated_vblanks += delta_seq;
 		}
 
+		for (i = 0; i < CM_NUM_PROPS; i++) {
+			if (i == CM_GAMMA_LUT_SIZE || i == CM_DEGAMMA_LUT_SIZE)
+				continue;
+			drmmode_crtc_push_cm_prop(crtc, i);
+		}
 	}
 	drmmode_crtc->dpms_mode = mode;
 }
-- 
2.7.4

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

[PATCH xf86-video-amdgpu 11/13] Push staged color properties on output detect

[sunpeng.li-5C7GfCeVMHo]

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

This will persist color management properties on a CRTC across
hot-plugs.

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

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 06ae902..f7fad34 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -2125,6 +2125,7 @@ static xf86OutputStatus drmmode_output_detect(xf86OutputPtr output)
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(output->scrn);
 	xf86OutputStatus status;
 	drmModeFreeConnector(drmmode_output->mode_output);
+	int i;
 
 	drmmode_output->mode_output =
 	    drmModeGetConnector(pAMDGPUEnt->fd, drmmode_output->output_id);
@@ -2134,6 +2135,13 @@ static xf86OutputStatus drmmode_output_detect(xf86OutputPtr output)
 	}
 
 	drmmode_output_update_properties(output);
+	if (output->crtc) {
+		for (i = 0; i < CM_NUM_PROPS; i++) {
+			if (i == CM_GAMMA_LUT_SIZE || i == CM_DEGAMMA_LUT_SIZE)
+				continue;
+			drmmode_crtc_push_cm_prop(output->crtc, i);
+		}
+	}
 
 	switch (drmmode_output->mode_output->connection) {
 	case DRM_MODE_CONNECTED:
-- 
2.7.4

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

[PATCH xf86-video-amdgpu 12/13] Update color properties on modeset major

[sunpeng.li-5C7GfCeVMHo]

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

This is needed for cases such as enabling a CRTC on an output that did
not previously have a CRTC attached. Doing so will update the output
properties listed with the correct color properties.

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

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index f7fad34..ba8be14 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -1498,6 +1498,8 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
 			goto done;
 
 		drmmode_crtc_update_tear_free(crtc);
+		drmmode_crtc_update_cm_props(crtc);
+
 		if (drmmode_crtc->tear_free)
 			scanout_id = drmmode_crtc->scanout_id;
 		else
-- 
2.7.4

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

[PATCH xf86-video-amdgpu 13/13] Refactor pushing color management properties into a function

[sunpeng.li-5C7GfCeVMHo]

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

Pushing all color properties to kernel DRM is done in a few places.
Reduce repetition by refactoring into a function.

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

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index ba8be14..6dc1a93 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -1286,6 +1286,21 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
 	return Success;
 }
 
+static int drmmode_crtc_push_cm_all(xf86CrtcPtr crtc) {
+	int i, ret;
+
+	for (i = 0; i < CM_NUM_PROPS; i++) {
+		if (i == CM_GAMMA_LUT_SIZE || i == CM_DEGAMMA_LUT_SIZE)
+			continue;
+
+		ret = drmmode_crtc_push_cm_prop(crtc, i);
+
+		if (ret)
+			return ret;
+	}
+	return Success;
+}
+
 static void
 drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
 {
@@ -1294,7 +1309,6 @@ drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(scrn);
 	CARD64 ust;
 	int ret;
-	int i;
 
 	if (drmmode_crtc->dpms_mode == DPMSModeOn && mode != DPMSModeOn) {
 		uint32_t seq;
@@ -1342,11 +1356,7 @@ drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
 			drmmode_crtc->interpolated_vblanks += delta_seq;
 		}
 
-		for (i = 0; i < CM_NUM_PROPS; i++) {
-			if (i == CM_GAMMA_LUT_SIZE || i == CM_DEGAMMA_LUT_SIZE)
-				continue;
-			drmmode_crtc_push_cm_prop(crtc, i);
-		}
+		drmmode_crtc_push_cm_all(crtc);
 	}
 	drmmode_crtc->dpms_mode = mode;
 }
@@ -2012,7 +2022,6 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, drmModeResPtr mode_res
 	drmmode_crtc_private_ptr drmmode_crtc;
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(pScrn);
 	AMDGPUInfoPtr info = AMDGPUPTR(pScrn);
-	int i;
 
 	crtc = xf86CrtcCreate(pScrn, &info->drmmode_crtc_funcs);
 	if (crtc == NULL)
@@ -2039,12 +2048,8 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, drmModeResPtr mode_res
 		drmmode_crtc->ctm->matrix[8] = (uint64_t)1 << 32;
 
 	/* Push properties to initialize them */
-	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))
-			return 0;
-	}
+	if (drmmode_crtc_push_cm_all(crtc))
+		return 0;
 
 	/* Mark num'th crtc as in use on this device. */
 	pAMDGPUEnt->assigned_crtcs |= (1 << num);
@@ -2127,7 +2132,6 @@ static xf86OutputStatus drmmode_output_detect(xf86OutputPtr output)
 	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(output->scrn);
 	xf86OutputStatus status;
 	drmModeFreeConnector(drmmode_output->mode_output);
-	int i;
 
 	drmmode_output->mode_output =
 	    drmModeGetConnector(pAMDGPUEnt->fd, drmmode_output->output_id);
@@ -2137,13 +2141,8 @@ static xf86OutputStatus drmmode_output_detect(xf86OutputPtr output)
 	}
 
 	drmmode_output_update_properties(output);
-	if (output->crtc) {
-		for (i = 0; i < CM_NUM_PROPS; i++) {
-			if (i == CM_GAMMA_LUT_SIZE || i == CM_DEGAMMA_LUT_SIZE)
-				continue;
-			drmmode_crtc_push_cm_prop(output->crtc, i);
-		}
-	}
+	if (output->crtc)
+		drmmode_crtc_push_cm_all(output->crtc);
 
 	switch (drmmode_output->mode_output->connection) {
 	case DRM_MODE_CONNECTED:
-- 
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 00/13] Enabling Color Management - Round 2

[Leo Li]

Ping :)

Leo

On 2018-05-03 02:31 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> 
> This patchset ended up looking quite different from the first. To address some
> fundamental issues, the design had to be reworked.
> 
> Things gathered from previous review:
> 
> 1. User client should not have to handle DRM blob objects. That should be the
>     job of the DDX driver.
> 
> 2. Since legacy gamma sets the same properties within DRM as non-legacy gamma,
>     the previous implementation created conflicts when setting both.
> 
>      * We should at least support this use-case: Nightlight enabled (uses legacy
>        gamma), with monitor correction enabled (non-legacy gamma)
> 
> 3. Since color management properties are attached to the CRTC, the previous
>     revision has the properties hooked onto the CRTC life-cycle, not the output
>     life-cycle. This is problematic, since clients expect properties on an
>     output to stay consistent throughout its life.
> 
> 4. Although not mentioned during review, the color properties did not persist
>     across certain events, such as DPMS state changes or hotplugs.
> 
> 
> To address the above, the following was done:
> 
> 1. XRandR allows setting of array-like properties. This is used to directly
>     pass LUT/CTM data from the client to the DDX driver.
> 
> 2. Legacy and non-legacy gamma LUTs are now merged via composition. This will
>     allow both to be in effect, while only programming one LUT in kernel driver.
> 
> 3. The three color management properties (Degamma LUT, Color Transform Matrix
>     (CTM), and Gamma LUT) are hard-coded into the DDX driver, to be listed (as
>     disabled) regardless of whether a CRTC is attached on the output, or whether
>     the kernel driver supports it.
> 
>      * If kernel driver does not support color management, the properties will
>        remain disabled. A `xrandr --set` will then error.
> 
> 4. Color properties are now *staged* inside the driver-private CRTC object.
>     This allows us to *push* the properties into kernel DRM (and consequently
>     into hardware) whenever there is a need.
> 
>      * Along with staging and pushing, an *update* function is used to notify
>        RandR to update the color properties listed on outputs. This can be used
>        when `xrandr --auto` enables a CRTC on an output, and the output need to
>        reflect the CRTC's color properties.
> 
> 
> However, there are some things being done that aren't quite nice, to which I
> don't yet have a solution. Any thoughts and suggestions are welcome:
> 
> * When using libXrandr to set the CTM property, 16bit format is used. Ideally
>    we should use 64bit format, since the CTM consists of 9x64bit fixed-point
>    values. However, it isn't recognized by XRRChangeOutputProperty as a valid
>    format. 32 bit could work, since the CTM values are S31.32 fixed-point.
>    However, using this format corrupts the data once it gets to xserver. On
>    first glance, it may be the cast to long within XRRChangeOutputProperty, in
>    addition to compiling 64 bit (shouldn't it use int32_t instead?).
> 
> * Since LUTs can be quite long, the output of `xrandr --prop` isn't very nice.
> 
> * Setting these properties through the xrandr app isn't supported without
>    modifications to the app, due to the length of these properties. However,
>    setting through libXrandr works.
> 
> Support on the xrandr app side can come as a seperate patch set. For now,
> testing the new API can be done via libXrandr. I've made a sample application
> here: git://people.freedesktop.org/~hwentland/color-demo-app
> Clone, make, and it's ready to go.
> 
> 
> Leo (Sunpeng) Li (13):
>    Add color management properties to driver-private CRTC object
>    Push color properties to kernel DRM on CRTC init
>    List disabled color properties on RandR outputs without a CRTC
>    Use CRTC's color properties if output has a CRTC attached
>    Enable setting of color properties though RandR
>    Compose non-legacy with legacy gamma LUT before pushing to kernel DRM
>    Also compose LUT when setting legacy gamma.
>    Set driver-private CRTC's dpms mode on disable
>    Move drmmode_do_crtc_dpms
>    Push staged color properties when DPMS state toggles On
>    Push staged color properties on output detect
>    Update color properties on modeset major
>    Refactor pushing color management properties into a function
> 
>   src/drmmode_display.c | 878 ++++++++++++++++++++++++++++++++++++++++++++++----
>   src/drmmode_display.h |   8 +
>   2 files changed, 824 insertions(+), 62 deletions(-)
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 00/13] Enabling Color Management - Round 2

[Michel Dänzer]

Hi Leo,


On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> 
> This patchset ended up looking quite different from the first. To address some
> fundamental issues, the design had to be reworked.

Your summary sounds really good. I haven't got a chance to look at the
patches in detail, and I don't know yet when I will, but this is to let
you know that it's still on my list. :)


-- 
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 00/13] Enabling Color Management - Round 2

[Michel Dänzer]

On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
> 
> 3. The three color management properties (Degamma LUT, Color Transform Matrix
>    (CTM), and Gamma LUT) are hard-coded into the DDX driver, to be listed (as
>    disabled) regardless of whether a CRTC is attached on the output, or whether
>    the kernel driver supports it.
> 
>     * If kernel driver does not support color management, the properties will
>       remain disabled. A `xrandr --set` will then error.

Is it really useful to expose these properties to clients if the kernel
doesn't support them?


> 4. Color properties are now *staged* inside the driver-private CRTC object.
>    This allows us to *push* the properties into kernel DRM (and consequently
>    into hardware) whenever there is a need.
> 
>     * Along with staging and pushing, an *update* function is used to notify
>       RandR to update the color properties listed on outputs. This can be used
>       when `xrandr --auto` enables a CRTC on an output, and the output need to
>       reflect the CRTC's color properties.

I feel like some of this is a bit more complicated than necessary. This
is how I'd envision it working:

In drmmode_crtc_init, query the properties from the kernel, more or less
as done in this series.

In drmmode_output_create_resources, create the output properties (or
not, per above) based on output->crtc, or if that is NULL, based on
xf86_config->crtc[0].

In drmmode_output_get_property, if output->crtc != NULL, update the
RandR property value from it, otherwise set it to a dummy value.

In drmmode_output_set_property, if output->crtc != NULL, update its
property value, otherwise do nothing.

Push the CRTC's property values to the kernel in
drmmode_crtc_gamma_do_set and drmmode_output_set_property (or maybe just
call the former from the latter when one of these properties are changed).

Is that feasible?


If patch 13 still exists after this, can you squash it into the earlier
patches adding the code?


> * When using libXrandr to set the CTM property, 16bit format is used. Ideally
>   we should use 64bit format, since the CTM consists of 9x64bit fixed-point
>   values. However, it isn't recognized by XRRChangeOutputProperty as a valid
>   format. 32 bit could work, since the CTM values are S31.32 fixed-point.
>   However, using this format corrupts the data once it gets to xserver. On
>   first glance, it may be the cast to long within XRRChangeOutputProperty, in
>   addition to compiling 64 bit (shouldn't it use int32_t instead?).

For historical reasons, Xlib uses long for 32-bit values, so you have to
pad each 32-bit value to a long. XCB shouldn't be affected by this.


-- 
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 01/13] Add color management properties to driver-private CRTC object

[Michel Dänzer]

On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> Non-legacy color management consists of 3 properties on the CRTC:
> Degamma LUT, Color Transformation Matrix (CTM), and Gamma LUT.
> 
> Add these properties to the driver-private CRTC, and initialize them
> when the CRTC is initialized. These values are refered to as "staged"
> values. They exist in the DDX driver, but require a "push" to DRM in
> order to be realized in hardware.
> 
> Also add a destructor for the driver-private CRTC, which cleans up the
> non-legacy properties.
> 
> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>

Note that while I have some cosmetic feedback on this patch (some of
which may also apply to others), in general the code in this series is
cleanly formatted and well documented, thanks!


> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index 49284c6..0ffc6ad 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -747,6 +747,83 @@ drmmode_crtc_scanout_update(xf86CrtcPtr crtc, DisplayModePtr mode,
> [...]
> +char *CM_PROP_NAMES[] = {

This identifier should be lowercase, since it's not a macro.


> +		if (get_cm_enum_from_str(drm_prop->name) == prop_id){

Missing space between ) and {.

Also, no empty lines after { or before } please.


> @@ -1353,6 +1446,18 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, drmModeResPtr mode_res
>  	crtc->driver_private = drmmode_crtc;
>  	drmmode_crtc_hw_id(crtc);
>  
> +	drmmode_crtc->gamma_lut_size =
> +		(uint32_t)get_drm_cm_prop_value(crtc, CM_GAMMA_LUT_SIZE);
> +	drmmode_crtc->degamma_lut_size =
> +		(uint32_t)get_drm_cm_prop_value(crtc, CM_DEGAMMA_LUT_SIZE);

Calling drmModeObjectGetProperties and iterating over the properties
twice seems a bit inefficient. Can you combine this to one
drmModeObjectGetProperties call, then iterating over the properties
once, until drmmode_crtc->(de)gamma_lut_size are both non-0?


> +	drmmode_crtc->ctm = calloc(1, sizeof(*drmmode_crtc->ctm));
> +	if (drmmode_crtc->ctm == NULL)

	if (!drmmode_crtc->ctm)


-- 
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 02/13] Push color properties to kernel DRM on CRTC init

[Michel Dänzer]

On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> Push staged values on the driver-private CRTC, to kernel DRM when it's
> initialized. This is to flush out any previous state that hardware was
> in, and set them to their default values.
> 
> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
> ---
>  src/drmmode_display.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 136 insertions(+)
> 
> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index 0ffc6ad..85de01e 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -824,6 +824,133 @@ err_allocs:
>  	return 0;
>  }
>  
> +/**
> + * Query DRM for the property ID - as recognized by DRM - for the specified
> + * color management property, on the specified CRTC.
> + *
> + * @crtc: The CRTC to query DRM properties on.
> + * @prop_id: Color management property enum.
> + *
> + * Return the DRM property ID, if the property exists. 0 otherwise.
> + */
> +static uint32_t get_drm_cm_prop_id(xf86CrtcPtr crtc,
> +				   enum drmmode_cm_prop prop_id)
> +{
> +	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
> +	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
> +	drmModeObjectPropertiesPtr drm_props;
> +	int i;
> +
> +	drm_props = drmModeObjectGetProperties(pAMDGPUEnt->fd,
> +					       drmmode_crtc->mode_crtc->crtc_id,
> +					       DRM_MODE_OBJECT_CRTC);
> +	if (!drm_props)
> +		goto err_allocs;
> +
> +	for (i = 0; i < drm_props->count_props; i++) {
> +		drmModePropertyPtr drm_prop;
> +
> +		drm_prop = drmModeGetProperty(pAMDGPUEnt->fd,
> +					      drm_props->props[i]);
> +		if (!drm_prop)
> +			goto err_allocs;
> +
> +		if (get_cm_enum_from_str(drm_prop->name) == prop_id){
> +			drmModeFreeProperty(drm_prop);
> +			return drm_props->props[i];
> +		}
> +
> +		drmModeFreeProperty(drm_prop);
> +	}
> +
> +err_allocs:
> +	drmModeFreeObjectProperties(drm_props);
> +	return 0;
> +}

It seems a bit heavy to call drmModeObjectGetProperties and
drmModeGetProperty (which both in turn call into the kernel) every time
here. Assuming the property IDs don't change at runtime, they could be
cached.


> +/**
> + * 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)
> +{
> +	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
> +	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
> +	size_t expected_bytes = 0;
> +	uint32_t created_blob_id = 0;
> +	void *blob_data = NULL;
> +	uint32_t drm_prop_id;
> +	Bool free_blob_data = FALSE;

free_blob_data is always FALSE. If that's intended, it shouldn't be
added (in this patch yet).


> @@ -1458,6 +1586,14 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, drmModeResPtr mode_res
>  	drmmode_crtc->ctm->matrix[0] = drmmode_crtc->ctm->matrix[4] =
>  		drmmode_crtc->ctm->matrix[8] = (uint64_t)1 << 32;
>  
> +	/* Push properties to initialize them */
> +	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))
> +			return 0;

Per my follow-up to the cover letter, I don't think this should be
necessary here anyway, but FWIW:

Returning 0 early here breaks the pAMDGPUEnt->assigned_crtcs related
logic in this function and drmmode_pre_init.


-- 
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 03/13] List disabled color properties on RandR outputs without a CRTC

[Michel Dänzer]

On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> The properties on an RandR output needs to stay consistent throughout
> it's lifecycle. However, we cannot list color properties on an output if
> there is no CRTC attached.
> 
> Therefore, create a fake CRTC, and initialize "disabled" color
> properites on outputs without a CRTC.
> 
> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>

If this patch remains: I don't get the point of the fake CRTC. AFAICT
rr_configure_and_change_cm_property could simply handle a NULL crtc more
or less the same way it does the fake one?


-- 
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 08/13] Set driver-private CRTC's dpms mode on disable

[Michel Dänzer]

On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> The dpms_mode flag on the driver-private CRTC was not being set when
> it's DPMS state is set to off. This causes some problems when toggling
> it back on, as some conditionals check this flag.
> 
> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
> ---
>  src/drmmode_display.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index 2b38a71..f86f99a 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -347,6 +347,7 @@ drmmode_crtc_dpms(xf86CrtcPtr crtc, int mode)
>  		drmModeSetCrtc(pAMDGPUEnt->fd, drmmode_crtc->mode_crtc->crtc_id,
>  			       0, 0, 0, NULL, 0, NULL);
>  		drmmode_fb_reference(pAMDGPUEnt->fd, &drmmode_crtc->fb, NULL);
> +		drmmode_crtc->dpms_mode = mode;
>  	} else if (drmmode_crtc->dpms_mode != DPMSModeOn)
>  		crtc->funcs->set_mode_major(crtc, &crtc->mode, crtc->rotation,
>  					    crtc->x, crtc->y);
> 

drmmode_crtc->dpms_mode is updated in drmmode_do_crtc_dpms. I'm a bit
worried that doing it here as well might cause subtle breakage. Is this
related to patches 10 & 11, or can you describe the scenario that
prompted you to make this change?


-- 
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 10/13] Push staged color properties when DPMS state toggles On

[Michel Dänzer]

On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> This will persist color management properties on a CRTC across DPMS
> state changes.
> 
> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
> ---
>  src/drmmode_display.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index 45c582c..06ae902 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -1294,6 +1294,7 @@ drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
>  	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(scrn);
>  	CARD64 ust;
>  	int ret;
> +	int i;
>  
>  	if (drmmode_crtc->dpms_mode == DPMSModeOn && mode != DPMSModeOn) {
>  		uint32_t seq;
> @@ -1341,6 +1342,11 @@ drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
>  			drmmode_crtc->interpolated_vblanks += delta_seq;
>  		}
>  
> +		for (i = 0; i < CM_NUM_PROPS; i++) {
> +			if (i == CM_GAMMA_LUT_SIZE || i == CM_DEGAMMA_LUT_SIZE)
> +				continue;
> +			drmmode_crtc_push_cm_prop(crtc, i);
> +		}
>  	}
>  	drmmode_crtc->dpms_mode = mode;
>  }
> 

This and patch 11 smell like workarounds for a kernel issue. The kernel
should preserve the property values regardless of DPMS state.

This probably explains something I just discovered: the legacy gamma LUT
becomes ineffective after turning a CRTC off and on again with DC,
whereas it's preserved without DC.


-- 
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 01/13] Add color management properties to driver-private CRTC object

[Leo Li]

On 2018-05-16 01:07 PM, Michel Dänzer wrote:
> On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>
>> Non-legacy color management consists of 3 properties on the CRTC:
>> Degamma LUT, Color Transformation Matrix (CTM), and Gamma LUT.
>>
>> Add these properties to the driver-private CRTC, and initialize them
>> when the CRTC is initialized. These values are refered to as "staged"
>> values. They exist in the DDX driver, but require a "push" to DRM in
>> order to be realized in hardware.
>>
>> Also add a destructor for the driver-private CRTC, which cleans up the
>> non-legacy properties.
>>
>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
> 
> Note that while I have some cosmetic feedback on this patch (some of
> which may also apply to others), in general the code in this series is
> cleanly formatted and well documented, thanks!
> 

Thanks for the review! Will fix all the cosmetic issues.

> 
>> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
>> index 49284c6..0ffc6ad 100644
>> --- a/src/drmmode_display.c
>> +++ b/src/drmmode_display.c
>> @@ -747,6 +747,83 @@ drmmode_crtc_scanout_update(xf86CrtcPtr crtc, DisplayModePtr mode,
>> [...]
>> +char *CM_PROP_NAMES[] = {
> 
> This identifier should be lowercase, since it's not a macro.
> 
> 
>> +		if (get_cm_enum_from_str(drm_prop->name) == prop_id){
> 
> Missing space between ) and {.
> 
> Also, no empty lines after { or before } please.
> 
> 
>> @@ -1353,6 +1446,18 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, drmModeResPtr mode_res
>>   	crtc->driver_private = drmmode_crtc;
>>   	drmmode_crtc_hw_id(crtc);
>>   
>> +	drmmode_crtc->gamma_lut_size =
>> +		(uint32_t)get_drm_cm_prop_value(crtc, CM_GAMMA_LUT_SIZE);
>> +	drmmode_crtc->degamma_lut_size =
>> +		(uint32_t)get_drm_cm_prop_value(crtc, CM_DEGAMMA_LUT_SIZE);
> 
> Calling drmModeObjectGetProperties and iterating over the properties
> twice seems a bit inefficient. Can you combine this to one
> drmModeObjectGetProperties call, then iterating over the properties
> once, until drmmode_crtc->(de)gamma_lut_size are both non-0?
> 

Yep, it seems this is the only function that's calling it anyways.

Leo

> 
>> +	drmmode_crtc->ctm = calloc(1, sizeof(*drmmode_crtc->ctm));
>> +	if (drmmode_crtc->ctm == NULL)
> 
> 	if (!drmmode_crtc->ctm)
> 
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 02/13] Push color properties to kernel DRM on CRTC init

[Leo Li]

On 2018-05-16 01:08 PM, Michel Dänzer wrote:
> On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>
>> Push staged values on the driver-private CRTC, to kernel DRM when it's
>> initialized. This is to flush out any previous state that hardware was
>> in, and set them to their default values.
>>
>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
>> ---
>>   src/drmmode_display.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 136 insertions(+)
>>
>> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
>> index 0ffc6ad..85de01e 100644
>> --- a/src/drmmode_display.c
>> +++ b/src/drmmode_display.c
>> @@ -824,6 +824,133 @@ err_allocs:
>>   	return 0;
>>   }
>>   
>> +/**
>> + * Query DRM for the property ID - as recognized by DRM - for the specified
>> + * color management property, on the specified CRTC.
>> + *
>> + * @crtc: The CRTC to query DRM properties on.
>> + * @prop_id: Color management property enum.
>> + *
>> + * Return the DRM property ID, if the property exists. 0 otherwise.
>> + */
>> +static uint32_t get_drm_cm_prop_id(xf86CrtcPtr crtc,
>> +				   enum drmmode_cm_prop prop_id)
>> +{
>> +	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
>> +	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
>> +	drmModeObjectPropertiesPtr drm_props;
>> +	int i;
>> +
>> +	drm_props = drmModeObjectGetProperties(pAMDGPUEnt->fd,
>> +					       drmmode_crtc->mode_crtc->crtc_id,
>> +					       DRM_MODE_OBJECT_CRTC);
>> +	if (!drm_props)
>> +		goto err_allocs;
>> +
>> +	for (i = 0; i < drm_props->count_props; i++) {
>> +		drmModePropertyPtr drm_prop;
>> +
>> +		drm_prop = drmModeGetProperty(pAMDGPUEnt->fd,
>> +					      drm_props->props[i]);
>> +		if (!drm_prop)
>> +			goto err_allocs;
>> +
>> +		if (get_cm_enum_from_str(drm_prop->name) == prop_id){
>> +			drmModeFreeProperty(drm_prop);
>> +			return drm_props->props[i];
>> +		}
>> +
>> +		drmModeFreeProperty(drm_prop);
>> +	}
>> +
>> +err_allocs:
>> +	drmModeFreeObjectProperties(drm_props);
>> +	return 0;
>> +}
> 
> It seems a bit heavy to call drmModeObjectGetProperties and
> drmModeGetProperty (which both in turn call into the kernel) every time
> here. Assuming the property IDs don't change at runtime, they could be
> cached.
> 

Hmm, good point. I took a look at the DRM property code, and indeed the
ID's don't change.

> 
>> +/**
>> + * 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)
>> +{
>> +	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
>> +	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
>> +	size_t expected_bytes = 0;
>> +	uint32_t created_blob_id = 0;
>> +	void *blob_data = NULL;
>> +	uint32_t drm_prop_id;
>> +	Bool free_blob_data = FALSE;
> 
> free_blob_data is always FALSE. If that's intended, it shouldn't be
> added (in this patch yet).
> 

Must have missed this while preparing the patches, will move to patch 6.

> 
>> @@ -1458,6 +1586,14 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, drmModeResPtr mode_res
>>   	drmmode_crtc->ctm->matrix[0] = drmmode_crtc->ctm->matrix[4] =
>>   		drmmode_crtc->ctm->matrix[8] = (uint64_t)1 << 32;
>>   
>> +	/* Push properties to initialize them */
>> +	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))
>> +			return 0;
> 
> Per my follow-up to the cover letter, I don't think this should be
> necessary here anyway, but FWIW:
> 
> Returning 0 early here breaks the pAMDGPUEnt->assigned_crtcs related
> logic in this function and drmmode_pre_init.
> 

I originally thought it'd make sense if DDX pushes the properties on
init, to maintain consistency between what DDX initially reports, and
what DRM is using. It would also reset color properties if X was restarted.

The other way around could also work, which I assume is what you're
suggesting? i.e. pull all color properties from DRM into the
driver-private crtc on crtc init, instead of pushing it to kernel?

Leo

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

Re: [PATCH xf86-video-amdgpu 03/13] List disabled color properties on RandR outputs without a CRTC

[Leo Li]

On 2018-05-16 01:09 PM, Michel Dänzer wrote:
> On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>
>> The properties on an RandR output needs to stay consistent throughout
>> it's lifecycle. However, we cannot list color properties on an output if
>> there is no CRTC attached.
>>
>> Therefore, create a fake CRTC, and initialize "disabled" color
>> properites on outputs without a CRTC.
>>
>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
> 
> If this patch remains: I don't get the point of the fake CRTC. AFAICT
> rr_configure_and_change_cm_property could simply handle a NULL crtc more
> or less the same way it does the fake one?
> 
> 

It does look like it can. Modifying the conditionals from
`if (crtc->gamma_lut)` to `if (crtc && crtc->gamma_lut)` should do the
trick. Thanks for catching this.

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

Re: [PATCH xf86-video-amdgpu 08/13] Set driver-private CRTC's dpms mode on disable

[Leo Li]

On 2018-05-16 01:09 PM, Michel Dänzer wrote:
> On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>
>> The dpms_mode flag on the driver-private CRTC was not being set when
>> it's DPMS state is set to off. This causes some problems when toggling
>> it back on, as some conditionals check this flag.
>>
>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
>> ---
>>   src/drmmode_display.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
>> index 2b38a71..f86f99a 100644
>> --- a/src/drmmode_display.c
>> +++ b/src/drmmode_display.c
>> @@ -347,6 +347,7 @@ drmmode_crtc_dpms(xf86CrtcPtr crtc, int mode)
>>   		drmModeSetCrtc(pAMDGPUEnt->fd, drmmode_crtc->mode_crtc->crtc_id,
>>   			       0, 0, 0, NULL, 0, NULL);
>>   		drmmode_fb_reference(pAMDGPUEnt->fd, &drmmode_crtc->fb, NULL);
>> +		drmmode_crtc->dpms_mode = mode;
>>   	} else if (drmmode_crtc->dpms_mode != DPMSModeOn)
>>   		crtc->funcs->set_mode_major(crtc, &crtc->mode, crtc->rotation,
>>   					    crtc->x, crtc->y);
>>
> 
> drmmode_crtc->dpms_mode is updated in drmmode_do_crtc_dpms. I'm a bit
> worried that doing it here as well might cause subtle breakage. Is this
> related to patches 10 & 11, or can you describe the scenario that
> prompted you to make this change?
> 

After reading your response to patch 10, and the cover letter, I think
this patch could be dropped. See my reply to patch 10 for details.

I originally had this change for the `xrandr --output --off` case.
Unlike xset dpms off, it wasn't triggering drmmode_do_crtc_dpms, and
therefore not setting the dpms_mode. This causes drmmode_do_crtc_dpms to
skip over a section of the code when output is turned back on, where
patch 10 resides in.

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

Re: [PATCH xf86-video-amdgpu 00/13] Enabling Color Management - Round 2

[Leo Li]

On 2018-05-16 01:06 PM, Michel Dänzer wrote:
> On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
>>
>> 3. The three color management properties (Degamma LUT, Color Transform Matrix
>>     (CTM), and Gamma LUT) are hard-coded into the DDX driver, to be listed (as
>>     disabled) regardless of whether a CRTC is attached on the output, or whether
>>     the kernel driver supports it.
>>
>>      * If kernel driver does not support color management, the properties will
>>        remain disabled. A `xrandr --set` will then error.
> 
> Is it really useful to expose these properties to clients if the kernel
> doesn't support them?
> 

I left them exposed mainly for simplicity. I can see how it would
confuse a client.

It should be simpler to hide these properties once the color property
IDs are cached somewhere (maybe on the AMDGPUInfo struct?) during
pre_init. It'll provide a easy way to determine if there's kernel
support within other functions.

> 
>> 4. Color properties are now *staged* inside the driver-private CRTC object.
>>     This allows us to *push* the properties into kernel DRM (and consequently
>>     into hardware) whenever there is a need.
>>
>>      * Along with staging and pushing, an *update* function is used to notify
>>        RandR to update the color properties listed on outputs. This can be used
>>        when `xrandr --auto` enables a CRTC on an output, and the output need to
>>        reflect the CRTC's color properties.
> 
> I feel like some of this is a bit more complicated than necessary. This
> is how I'd envision it working:
> 
> In drmmode_crtc_init, query the properties from the kernel, more or less
> as done in this series.
> 
> In drmmode_output_create_resources, create the output properties (or
> not, per above) based on output->crtc, or if that is NULL, based on
> xf86_config->crtc[0].

What are the contents of xf86_config->crtc[0] initialized to? I'm not
too familiar with how that's setup. Does it stay the same throughout?

> 
> In drmmode_output_get_property, if output->crtc != NULL, update the
> RandR property value from it, otherwise set it to a dummy value.
> 

Feeling kind of stupid that I skipped over this :) I assumed it wasn't
useful since it's currently a no-op. This should eliminate the need for
the cm_update function, assuming it's called every time a client
requests for the properties.

This should work even if one CRTC is attached to multiple outputs,
correct? Since it would first update the RandR property with the
attached CRTC before returning it.

> In drmmode_output_set_property, if output->crtc != NULL, update its
> property value, otherwise do nothing.
> 
> Push the CRTC's property values to the kernel in
> drmmode_crtc_gamma_do_set and drmmode_output_set_property (or maybe just
> call the former from the latter when one of these properties are changed).
> 
> Is that feasible?
> 

Sounds feasible, and much better actually. Assuming it also works well
with 1 crtc on >1 outputs, and I don't see why it wouldn't. Thanks for
the pointers.

> 
> If patch 13 still exists after this, can you squash it into the earlier
> patches adding the code?
> 
> 
>> * When using libXrandr to set the CTM property, 16bit format is used. Ideally
>>    we should use 64bit format, since the CTM consists of 9x64bit fixed-point
>>    values. However, it isn't recognized by XRRChangeOutputProperty as a valid
>>    format. 32 bit could work, since the CTM values are S31.32 fixed-point.
>>    However, using this format corrupts the data once it gets to xserver. On
>>    first glance, it may be the cast to long within XRRChangeOutputProperty, in
>>    addition to compiling 64 bit (shouldn't it use int32_t instead?).
> 
> For historical reasons, Xlib uses long for 32-bit values, so you have to
> pad each 32-bit value to a long. XCB shouldn't be affected by this.
> 

Noted.

Leo

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

Re: [PATCH xf86-video-amdgpu 10/13] Push staged color properties when DPMS state toggles On

[Leo Li]

On 2018-05-16 01:10 PM, Michel Dänzer wrote:
> On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>
>> This will persist color management properties on a CRTC across DPMS
>> state changes.
>>
>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
>> ---
>>   src/drmmode_display.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
>> index 45c582c..06ae902 100644
>> --- a/src/drmmode_display.c
>> +++ b/src/drmmode_display.c
>> @@ -1294,6 +1294,7 @@ drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
>>   	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(scrn);
>>   	CARD64 ust;
>>   	int ret;
>> +	int i;
>>   
>>   	if (drmmode_crtc->dpms_mode == DPMSModeOn && mode != DPMSModeOn) {
>>   		uint32_t seq;
>> @@ -1341,6 +1342,11 @@ drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
>>   			drmmode_crtc->interpolated_vblanks += delta_seq;
>>   		}
>>   
>> +		for (i = 0; i < CM_NUM_PROPS; i++) {
>> +			if (i == CM_GAMMA_LUT_SIZE || i == CM_DEGAMMA_LUT_SIZE)
>> +				continue;
>> +			drmmode_crtc_push_cm_prop(crtc, i);
>> +		}
>>   	}
>>   	drmmode_crtc->dpms_mode = mode;
>>   }
>>
> 
> This and patch 11 smell like workarounds for a kernel issue. The kernel
> should preserve the property values regardless of DPMS state.
> 
> This probably explains something I just discovered: the legacy gamma LUT
> becomes ineffective after turning a CRTC off and on again with DC,
> whereas it's preserved without DC.
> 

That's indeed a kernel issue, will look into it. This patch can be
dropped once the kernel persists the properties across dpms.

In terms of Patch 11, which persists the properties across hotplugs, is
it even a valid use-case? I tested with i915 and amdgpu non-dc drivers.
Both don't seem to persist legacy gamma across hotplugs, or xrandr
--output --off/--auto. If not, patches 9-11 can all be dropped.

Leo

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

Re: [PATCH xf86-video-amdgpu 02/13] Push color properties to kernel DRM on CRTC init

[Michel Dänzer]

On 2018-05-17 11:43 PM, Leo Li wrote:
> On 2018-05-16 01:08 PM, Michel Dänzer wrote:
>> On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
>>>
>>> @@ -1458,6 +1586,14 @@ drmmode_crtc_init(ScrnInfoPtr pScrn,
>>> drmmode_ptr drmmode, drmModeResPtr mode_res
>>>       drmmode_crtc->ctm->matrix[0] = drmmode_crtc->ctm->matrix[4] =
>>>           drmmode_crtc->ctm->matrix[8] = (uint64_t)1 << 32;
>>>   +    /* Push properties to initialize them */
>>> +    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))
>>> +            return 0;
>>
>> Per my follow-up to the cover letter, I don't think this should be
>> necessary here anyway, but FWIW:
>>
>> Returning 0 early here breaks the pAMDGPUEnt->assigned_crtcs related
>> logic in this function and drmmode_pre_init.
> 
> I originally thought it'd make sense if DDX pushes the properties on
> init, to maintain consistency between what DDX initially reports, and
> what DRM is using. It would also reset color properties if X was restarted.

My point was that this will happen from drmmode_set_mode_major anyway,
therefore no need to do it here.

OTOH if the properties are preserved by the kernel, it might make sense
to only do it here and from drmmode_crtc_gamma_set, not from
drmmode_set_mode_major.


> The other way around could also work, which I assume is what you're
> suggesting? i.e. pull all color properties from DRM into the
> driver-private crtc on crtc init, instead of pushing it to kernel?

That's not what I meant, and I suspect that would be tricky. E.g. how
could we separate the pulled values into the legacy and advanced LUTs?
And what if console (or whatever was displaying before) was using a
different colour depth (could even be 8 bpp pseudo colour)?


-- 
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 10/13] Push staged color properties when DPMS state toggles On

[Michel Dänzer]

On 2018-05-17 11:44 PM, Leo Li wrote:
> On 2018-05-16 01:10 PM, Michel Dänzer wrote:
>> On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>
>>> This will persist color management properties on a CRTC across DPMS
>>> state changes.
>>>
>>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
>>> ---
>>>   src/drmmode_display.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
>>> index 45c582c..06ae902 100644
>>> --- a/src/drmmode_display.c
>>> +++ b/src/drmmode_display.c
>>> @@ -1294,6 +1294,7 @@ drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
>>>       AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(scrn);
>>>       CARD64 ust;
>>>       int ret;
>>> +    int i;
>>>         if (drmmode_crtc->dpms_mode == DPMSModeOn && mode !=
>>> DPMSModeOn) {
>>>           uint32_t seq;
>>> @@ -1341,6 +1342,11 @@ drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
>>>               drmmode_crtc->interpolated_vblanks += delta_seq;
>>>           }
>>>   +        for (i = 0; i < CM_NUM_PROPS; i++) {
>>> +            if (i == CM_GAMMA_LUT_SIZE || i == CM_DEGAMMA_LUT_SIZE)
>>> +                continue;
>>> +            drmmode_crtc_push_cm_prop(crtc, i);
>>> +        }
>>>       }
>>>       drmmode_crtc->dpms_mode = mode;
>>>   }
>>>
>>
>> This and patch 11 smell like workarounds for a kernel issue. The kernel
>> should preserve the property values regardless of DPMS state.
>>
>> This probably explains something I just discovered: the legacy gamma LUT
>> becomes ineffective after turning a CRTC off and on again with DC,
>> whereas it's preserved without DC.
> 
> That's indeed a kernel issue, will look into it. This patch can be
> dropped once the kernel persists the properties across dpms.
> 
> In terms of Patch 11, which persists the properties across hotplugs, is
> it even a valid use-case? I tested with i915 and amdgpu non-dc drivers.
> Both don't seem to persist legacy gamma across hotplugs, or xrandr
> --output --off/--auto.

It persists for me (without this series) with amdgpu without DC on
xrandr --output --off/--auto, or explicitly moving an output between
different CRTCs. Those are definitely expected.

Not sure about hotplug; it's possible that something explicitly
re-initializes the legacy gamma in that case. But other than that, each
CRTC should preserve its values unless they are explicitly modified.


-- 
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 00/13] Enabling Color Management - Round 2

[Michel Dänzer]

On 2018-05-17 11:43 PM, Leo Li wrote:
> On 2018-05-16 01:06 PM, Michel Dänzer wrote:
>> On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
>>>
>>> 3. The three color management properties (Degamma LUT, Color
>>> Transform Matrix
>>>     (CTM), and Gamma LUT) are hard-coded into the DDX driver, to be
>>> listed (as
>>>     disabled) regardless of whether a CRTC is attached on the output,
>>> or whether
>>>     the kernel driver supports it.
>>>
>>>      * If kernel driver does not support color management, the
>>> properties will
>>>        remain disabled. A `xrandr --set` will then error.
>>
>> Is it really useful to expose these properties to clients if the kernel
>> doesn't support them?
>>
> 
> I left them exposed mainly for simplicity. I can see how it would
> confuse a client.
> 
> It should be simpler to hide these properties once the color property
> IDs are cached somewhere (maybe on the AMDGPUInfo struct?)

drmmode_crtc_private_rec seems better.


>>> 4. Color properties are now *staged* inside the driver-private CRTC
>>> object.
>>>     This allows us to *push* the properties into kernel DRM (and
>>> consequently
>>>     into hardware) whenever there is a need.
>>>
>>>      * Along with staging and pushing, an *update* function is used
>>> to notify
>>>        RandR to update the color properties listed on outputs. This
>>> can be used
>>>        when `xrandr --auto` enables a CRTC on an output, and the
>>> output need to
>>>        reflect the CRTC's color properties.
>>
>> I feel like some of this is a bit more complicated than necessary. This
>> is how I'd envision it working:
>>
>> In drmmode_crtc_init, query the properties from the kernel, more or less
>> as done in this series.
>>
>> In drmmode_output_create_resources, create the output properties (or
>> not, per above) based on output->crtc, or if that is NULL, based on
>> xf86_config->crtc[0].
> 
> What are the contents of xf86_config->crtc[0] initialized to? I'm not
> too familiar with how that's setup. Does it stay the same throughout?

I think so, but my point here is mainly that whether or not the
properties exist, and the size of the LUTs should be the same regardless
of which particular CRTC you look at. So for simplicity,
drmmode_output_create_resources could even always look at
xf86_config->crtc[0] (i.e. the first CRTC).


>> In drmmode_output_get_property, if output->crtc != NULL, update the
>> RandR property value from it, otherwise set it to a dummy value.
> 
> Feeling kind of stupid that I skipped over this :) I assumed it wasn't
> useful since it's currently a no-op. This should eliminate the need for
> the cm_update function, assuming it's called every time a client
> requests for the properties.
> 
> This should work even if one CRTC is attached to multiple outputs,
> correct? Since it would first update the RandR property with the
> attached CRTC before returning it.

Yes, I think so.


-- 
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 08/13] Set driver-private CRTC's dpms mode on disable

[Michel Dänzer]

On 2018-05-17 11:43 PM, Leo Li wrote:
> On 2018-05-16 01:09 PM, Michel Dänzer wrote:
>> On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>
>>> The dpms_mode flag on the driver-private CRTC was not being set when
>>> it's DPMS state is set to off. This causes some problems when toggling
>>> it back on, as some conditionals check this flag.
>>>
>>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
>>> ---
>>>   src/drmmode_display.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
>>> index 2b38a71..f86f99a 100644
>>> --- a/src/drmmode_display.c
>>> +++ b/src/drmmode_display.c
>>> @@ -347,6 +347,7 @@ drmmode_crtc_dpms(xf86CrtcPtr crtc, int mode)
>>>           drmModeSetCrtc(pAMDGPUEnt->fd,
>>> drmmode_crtc->mode_crtc->crtc_id,
>>>                      0, 0, 0, NULL, 0, NULL);
>>>           drmmode_fb_reference(pAMDGPUEnt->fd, &drmmode_crtc->fb, NULL);
>>> +        drmmode_crtc->dpms_mode = mode;
>>>       } else if (drmmode_crtc->dpms_mode != DPMSModeOn)
>>>           crtc->funcs->set_mode_major(crtc, &crtc->mode, crtc->rotation,
>>>                           crtc->x, crtc->y);
>>>
>>
>> drmmode_crtc->dpms_mode is updated in drmmode_do_crtc_dpms. I'm a bit
>> worried that doing it here as well might cause subtle breakage. Is this
>> related to patches 10 & 11, or can you describe the scenario that
>> prompted you to make this change?
>>
> 
> After reading your response to patch 10, and the cover letter, I think
> this patch could be dropped. See my reply to patch 10 for details.
> 
> I originally had this change for the `xrandr --output --off` case.
> Unlike xset dpms off, it wasn't triggering drmmode_do_crtc_dpms, and
> therefore not setting the dpms_mode. This causes drmmode_do_crtc_dpms to
> skip over a section of the code when output is turned back on, where
> patch 10 resides in.

Indeed, that's a good catch, thanks. However, it's better to make sure
drmmode_do_crtc_dpms is called in this case as well, please take a look
at https://patchwork.freedesktop.org/patch/224016/ .


-- 
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 10/13] Push staged color properties when DPMS state toggles On

[Leo Li]

On 2018-05-18 04:01 AM, Michel Dänzer wrote:
> On 2018-05-17 11:44 PM, Leo Li wrote:
>> On 2018-05-16 01:10 PM, Michel Dänzer wrote:
>>> On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
>>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>>
>>>> This will persist color management properties on a CRTC across DPMS
>>>> state changes.
>>>>
>>>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
>>>> ---
>>>>    src/drmmode_display.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
>>>> index 45c582c..06ae902 100644
>>>> --- a/src/drmmode_display.c
>>>> +++ b/src/drmmode_display.c
>>>> @@ -1294,6 +1294,7 @@ drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
>>>>        AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(scrn);
>>>>        CARD64 ust;
>>>>        int ret;
>>>> +    int i;
>>>>          if (drmmode_crtc->dpms_mode == DPMSModeOn && mode !=
>>>> DPMSModeOn) {
>>>>            uint32_t seq;
>>>> @@ -1341,6 +1342,11 @@ drmmode_do_crtc_dpms(xf86CrtcPtr crtc, int mode)
>>>>                drmmode_crtc->interpolated_vblanks += delta_seq;
>>>>            }
>>>>    +        for (i = 0; i < CM_NUM_PROPS; i++) {
>>>> +            if (i == CM_GAMMA_LUT_SIZE || i == CM_DEGAMMA_LUT_SIZE)
>>>> +                continue;
>>>> +            drmmode_crtc_push_cm_prop(crtc, i);
>>>> +        }
>>>>        }
>>>>        drmmode_crtc->dpms_mode = mode;
>>>>    }
>>>>
>>>
>>> This and patch 11 smell like workarounds for a kernel issue. The kernel
>>> should preserve the property values regardless of DPMS state.
>>>
>>> This probably explains something I just discovered: the legacy gamma LUT
>>> becomes ineffective after turning a CRTC off and on again with DC,
>>> whereas it's preserved without DC.
>>
>> That's indeed a kernel issue, will look into it. This patch can be
>> dropped once the kernel persists the properties across dpms.
>>
>> In terms of Patch 11, which persists the properties across hotplugs, is
>> it even a valid use-case? I tested with i915 and amdgpu non-dc drivers.
>> Both don't seem to persist legacy gamma across hotplugs, or xrandr
>> --output --off/--auto.
> 
> It persists for me (without this series) with amdgpu without DC on
> xrandr --output --off/--auto, or explicitly moving an output between
> different CRTCs. Those are definitely expected.
> 
> Not sure about hotplug; it's possible that something explicitly
> re-initializes the legacy gamma in that case. But other than that, each
> CRTC should preserve its values unless they are explicitly modified.
> 

Just patched up the kernel to fix this, and the properties indeed
persist like you've described (hotplug as well). Makes sense, since the
properties don't change on the DRM states. We just have to update DC on
these events.

Thanks for your responses, moving on to v3 now.

Leo

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

Re: [PATCH xf86-video-amdgpu 00/13] Enabling Color Management - Round 2

[Leo Li]

On 2018-05-18 04:10 AM, Michel Dänzer wrote:
> On 2018-05-17 11:43 PM, Leo Li wrote:
>> On 2018-05-16 01:06 PM, Michel Dänzer wrote:
>>> On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
>>>>
>>>> 3. The three color management properties (Degamma LUT, Color
>>>> Transform Matrix
>>>>      (CTM), and Gamma LUT) are hard-coded into the DDX driver, to be
>>>> listed (as
>>>>      disabled) regardless of whether a CRTC is attached on the output,
>>>> or whether
>>>>      the kernel driver supports it.
>>>>
>>>>       * If kernel driver does not support color management, the
>>>> properties will
>>>>         remain disabled. A `xrandr --set` will then error.
>>>
>>> Is it really useful to expose these properties to clients if the kernel
>>> doesn't support them?
>>>
>>
>> I left them exposed mainly for simplicity. I can see how it would
>> confuse a client.
>>
>> It should be simpler to hide these properties once the color property
>> IDs are cached somewhere (maybe on the AMDGPUInfo struct?)
> 
> drmmode_crtc_private_rec seems better.
> 

Doesn't that mean we're caching duplicate DRM property IDs on each CRTC
object? I think we only need to cache one copy.

Looking at DRM code, the IDs identify DRM property "types", not the 
actual property data, and are created during kernel driver load. Storing 
one copy is enough, since the types are the same regardless of CRTC.

I was thinking we can fetch these id's in drmmode_pre_init because of 
that, but I'm not sure of the implications. Wouldn't that be better?

Leo

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

Re: [PATCH xf86-video-amdgpu 00/13] Enabling Color Management - Round 2

[Michel Dänzer]

On 2018-05-24 10:29 PM, Leo Li wrote:
> On 2018-05-18 04:10 AM, Michel Dänzer wrote:
>> On 2018-05-17 11:43 PM, Leo Li wrote:
>>> On 2018-05-16 01:06 PM, Michel Dänzer wrote:
>>>> On 2018-05-03 08:31 PM, sunpeng.li@amd.com wrote:
>>>>>
>>>>> 3. The three color management properties (Degamma LUT, Color
>>>>> Transform Matrix
>>>>>      (CTM), and Gamma LUT) are hard-coded into the DDX driver, to be
>>>>> listed (as
>>>>>      disabled) regardless of whether a CRTC is attached on the output,
>>>>> or whether
>>>>>      the kernel driver supports it.
>>>>>
>>>>>       * If kernel driver does not support color management, the
>>>>> properties will
>>>>>         remain disabled. A `xrandr --set` will then error.
>>>>
>>>> Is it really useful to expose these properties to clients if the kernel
>>>> doesn't support them?
>>>>
>>>
>>> I left them exposed mainly for simplicity. I can see how it would
>>> confuse a client.
>>>
>>> It should be simpler to hide these properties once the color property
>>> IDs are cached somewhere (maybe on the AMDGPUInfo struct?)
>>
>> drmmode_crtc_private_rec seems better.
>>
> 
> Doesn't that mean we're caching duplicate DRM property IDs on each CRTC
> object? I think we only need to cache one copy.
> 
> Looking at DRM code, the IDs identify DRM property "types", not the
> actual property data, and are created during kernel driver load. Storing
> one copy is enough, since the types are the same regardless of CRTC.
> 
> I was thinking we can fetch these id's in drmmode_pre_init because of
> that, but I'm not sure of the implications. Wouldn't that be better?

If the IDs are the same for all CRTCs, they should be stored in struct
drmmode_rec.


-- 
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