[Intel-gfx] [RFC 2/7] drm: Add Plane CTM property

[Shankar, Uma]

>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of
>Brian Starkey
>Sent: Tuesday, November 7, 2017 11:10 PM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: intel-gfx at lists.freedesktop.org; Syrjala, Ville <ville.syrjala at intel.com>;
>Lankhorst, Maarten <maarten.lankhorst at intel.com>; dri-
>devel at lists.freedesktop.org
>Subject: Re: [RFC 2/7] drm: Add Plane CTM property
>
>Hi Uma,
>
>On Tue, Nov 07, 2017 at 05:36:26PM +0530, Uma Shankar wrote:
>>Add a blob property for plane CSC usage.
>>
>>v2: Rebase
>>
>>Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>>---
>> drivers/gpu/drm/drm_atomic.c        |   10 ++++++++++
>> drivers/gpu/drm/drm_atomic_helper.c |    3 +++
>> drivers/gpu/drm/drm_mode_config.c   |    7 +++++++
>> include/drm/drm_mode_config.h       |    6 ++++++
>> include/drm/drm_plane.h             |    8 ++++++++
>> 5 files changed, 34 insertions(+)
>>
>>diff --git a/drivers/gpu/drm/drm_atomic.c
>>b/drivers/gpu/drm/drm_atomic.c index 30853c7..45aede5 100644
>>--- a/drivers/gpu/drm/drm_atomic.c
>>+++ b/drivers/gpu/drm/drm_atomic.c
>>@@ -766,6 +766,14 @@ static int drm_atomic_plane_set_property(struct
>drm_plane *plane,
>> 					val, -1, &replaced);
>> 		state->color_mgmt_changed |= replaced;
>> 		return ret;
>>+	} else if (property == config->plane_ctm_property) {
>>+		ret = drm_atomic_replace_property_blob_from_id(dev,
>>+					&state->ctm,
>>+					val,
>>+					sizeof(struct drm_color_ctm),
>>+					&replaced);
>>+		state->color_mgmt_changed |= replaced;
>>+		return ret;
>> 	} else {
>> 		return -EINVAL;
>> 	}
>>@@ -827,6 +835,8 @@ static int drm_atomic_plane_set_property(struct
>drm_plane *plane,
>> 	} else if (property == config->plane_degamma_lut_property) {
>> 		*val = (state->degamma_lut) ?
>> 			state->degamma_lut->base.id : 0;
>>+	} else if (property == config->plane_ctm_property) {
>>+		*val = (state->ctm) ? state->ctm->base.id : 0;
>> 	} else {
>> 		return -EINVAL;
>> 	}
>>diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>>b/drivers/gpu/drm/drm_atomic_helper.c
>>index ba924cf..d3154e0 100644
>>--- a/drivers/gpu/drm/drm_atomic_helper.c
>>+++ b/drivers/gpu/drm/drm_atomic_helper.c
>>@@ -3398,6 +3398,8 @@ void
>>__drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>>
>> 	if (state->degamma_lut)
>> 		drm_property_blob_get(state->degamma_lut);
>>+	if (state->ctm)
>>+		drm_property_blob_get(state->ctm);
>> 	state->color_mgmt_changed = false;
>> }
>> EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
>>@@ -3445,6 +3447,7 @@ void
>__drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>> 		drm_crtc_commit_put(state->commit);
>>
>> 	drm_property_blob_put(state->degamma_lut);
>>+	drm_property_blob_put(state->ctm);
>> }
>> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>>
>>diff --git a/drivers/gpu/drm/drm_mode_config.c
>>b/drivers/gpu/drm/drm_mode_config.c
>>index 118f6ac..bccc70e 100644
>>--- a/drivers/gpu/drm/drm_mode_config.c
>>+++ b/drivers/gpu/drm/drm_mode_config.c
>>@@ -362,6 +362,13 @@ static int
>drm_mode_create_standard_properties(struct drm_device *dev)
>> 		return -ENOMEM;
>> 	dev->mode_config.plane_degamma_lut_size_property = prop;
>>
>>+	prop = drm_property_create(dev,
>>+			DRM_MODE_PROP_BLOB,
>>+			"PLANE_CTM", 0);
>
>I do wonder if "PLANE_" is really needed here, as the property will only ever be
>found on a plane (same would apply to all three property names).

This is just to explicitly separate out the names from the crtc (pipe) properties. 
(Similar properties exist for pipe already). It will create confusion, hence explicitly called
them out appending with a  "PLANE" prefix.

>
>>+	if (!prop)
>>+		return -ENOMEM;
>>+	dev->mode_config.plane_ctm_property = prop;
>>+
>> 	return 0;
>> }
>>
>>diff --git a/include/drm/drm_mode_config.h
>>b/include/drm/drm_mode_config.h index 6ee2df6..3bf7fc6 100644
>>--- a/include/drm/drm_mode_config.h
>>+++ b/include/drm/drm_mode_config.h
>>@@ -727,6 +727,12 @@ struct drm_mode_config {
>> 	 * size of the degamma LUT as supported by the driver (read-only).
>> 	 */
>> 	struct drm_property *plane_degamma_lut_size_property;
>>+	/**
>>+	 * @plane_ctm_property: Optional CRTC property to set the
>>+	 * matrix used to convert colors after the lookup in the
>>+	 * degamma LUT.
>>+	 */
>
>Copy-paste error - should be "Optional plane property"

Yeah indeed, thanks for spotting it. Will fix in next set.

Regards,
Uma Shankar

>
>Thanks,
>-Brian
>
>>+	struct drm_property *plane_ctm_property;
>>
>> 	/**
>> 	 * @suggested_x_property: Optional connector property with a hint for
>>diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index
>>d112c50..7fcc51e 100644
>>--- a/include/drm/drm_plane.h
>>+++ b/include/drm/drm_plane.h
>>@@ -132,6 +132,14 @@ struct drm_plane_state {
>> 	struct drm_property_blob *degamma_lut;
>>
>> 	/**
>>+	 * @ctm:
>>+	 *
>>+	 * Color transformation matrix. See drm_plane_enable_color_mgmt().
>The
>>+	 * blob (if not NULL) is a &struct drm_color_ctm.
>>+	 */
>>+	struct drm_property_blob *ctm;
>>+
>>+	/**
>> 	 * @commit: Tracks the pending commit to prevent use-after-free
>conditions,
>> 	 * and for async plane updates.
>> 	 *
>>--
>>1.7.9.5
>>
>>_______________________________________________
>>dri-devel mailing list
>>dri-devel at lists.freedesktop.org
>>https://lists.freedesktop.org/mailman/listinfo/dri-devel
>_______________________________________________
>dri-devel mailing list
>dri-devel at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel