Re: [v3 1/7] drm: Add gamma mode property

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Apr 12, 2019 at 03:50:57PM +0530, Uma Shankar wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 
> Add a gamma mode property to enable various kind of
> gamma modes supported by platforms like: Interpolated, Split,
> Multi Segmented etc. Userspace can get this property and
> should be able to get the platform capabilties wrt various
> gamma modes possible and the possible ranges.
> 
> It can select one of the modes exposed as blob_id as an
> enum and set the respective mode.
> 
> It can then create the LUT and send it to driver using
> already available GAMMA_LUT property as blob.
> 
> v2: Addressed Sam Ravnborg's review comments. Implemented
> gamma mode with just one property and renamed the current
> one to GAMMA_MODE property as recommended by Ville.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>

Please also extend the CTM property docs, see

https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#color-management-properties

And especially how GAMMA_MODE interacts with everything else we have
already. I think the current comments don't really explain well how this
is supposed to be used.

Also, since this is quite a complicated data structure, can't we do at
least some basic validation in the core code?
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  5 +++
>  drivers/gpu/drm/drm_color_mgmt.c  | 77 +++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_color_mgmt.h      |  8 ++++
>  include/drm/drm_crtc.h            |  7 ++++
>  include/drm/drm_mode_config.h     |  6 +++
>  include/uapi/drm/drm_mode.h       | 38 +++++++++++++++++++
>  6 files changed, 141 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index ea797d4..d85e0c9 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -459,6 +459,9 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  					&replaced);
>  		state->color_mgmt_changed |= replaced;
>  		return ret;
> +	} else if (property == config->gamma_mode_property) {
> +		state->gamma_mode = val;
> +		state->color_mgmt_changed |= replaced;
>  	} else if (property == config->prop_out_fence_ptr) {
>  		s32 __user *fence_ptr = u64_to_user_ptr(val);
>  
> @@ -495,6 +498,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  		*val = (state->mode_blob) ? state->mode_blob->base.id : 0;
>  	else if (property == config->prop_vrr_enabled)
>  		*val = state->vrr_enabled;
> +	else if (property == config->gamma_mode_property)
> +		*val = state->gamma_mode;
>  	else if (property == config->degamma_lut_property)
>  		*val = (state->degamma_lut) ? state->degamma_lut->base.id : 0;
>  	else if (property == config->ctm_property)
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index d5d34d0..4d6792d 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -176,6 +176,83 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>  }
>  EXPORT_SYMBOL(drm_crtc_enable_color_mgmt);
>  
> +void drm_crtc_attach_gamma_mode_property(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (!config->gamma_mode_property)
> +		return;
> +
> +	drm_object_attach_property(&crtc->base,
> +				   config->gamma_mode_property, 0);
> +}
> +EXPORT_SYMBOL(drm_crtc_attach_gamma_mode_property);
> +
> +int drm_color_create_gamma_mode_property(struct drm_device *dev,
> +					 int num_values)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create(dev,
> +				   DRM_MODE_PROP_ENUM,
> +				   "GAMMA_MODE", num_values);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	config->gamma_mode_property = prop;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_color_create_gamma_mode_property);
> +
> +int drm_color_add_gamma_mode_range(struct drm_device *dev,
> +				   const char *name,
> +				   const struct drm_color_lut_range *ranges,
> +				   size_t length)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_property_blob *blob;
> +	struct drm_property *prop;
> +	int num_ranges = length / sizeof(ranges[0]);
> +	int i, ret, num_types_0;
> +
> +	if (WARN_ON(length == 0 || length % sizeof(ranges[0]) != 0))
> +		return -EINVAL;
> +
> +	num_types_0 = hweight8(ranges[0].flags & (DRM_MODE_LUT_GAMMA |
> +						  DRM_MODE_LUT_DEGAMMA));
> +	if (num_types_0 == 0)
> +		return -EINVAL;
> +
> +	for (i = 1; i < num_ranges; i++) {
> +		int num_types = hweight8(ranges[i].flags & (DRM_MODE_LUT_GAMMA |
> +							    DRM_MODE_LUT_DEGAMMA));
> +
> +		/* either all ranges have DEGAMMA|GAMMA or none have it */
> +		if (num_types_0 != num_types)
> +			return -EINVAL;
> +	}
> +
> +	prop = config->gamma_mode_property;
> +	if (!prop)
> +		return -EINVAL;
> +
> +	blob = drm_property_create_blob(dev, length, ranges);
> +	if (IS_ERR(blob))
> +		return PTR_ERR(blob);
> +
> +	ret = drm_property_add_enum(prop, blob->base.id, name);
> +	if (ret) {
> +		drm_property_blob_put(blob);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_color_add_gamma_mode_range);
> +
>  /**
>   * drm_mode_crtc_set_gamma_size - set the gamma table size
>   * @crtc: CRTC to set the gamma table size for
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index d1c662d..f18e9b8 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -51,6 +51,14 @@ static inline int drm_color_lut_size(const struct drm_property_blob *blob)
>  	return blob->length / sizeof(struct drm_color_lut);
>  }
>  
> +int drm_color_create_gamma_mode_property(struct drm_device *dev,
> +					 int num_values);
> +void drm_crtc_attach_gamma_mode_property(struct drm_crtc *crtc);
> +int drm_color_add_gamma_mode_range(struct drm_device *dev,
> +				   const char *name,
> +				   const struct drm_color_lut_range *ranges,
> +				   size_t length);
> +
>  enum drm_color_encoding {
>  	DRM_COLOR_YCBCR_BT601,
>  	DRM_COLOR_YCBCR_BT709,
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 58ad983..f2e60bd 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -249,6 +249,13 @@ struct drm_crtc_state {
>  	struct drm_property_blob *mode_blob;
>  
>  	/**
> +	 * @gamma_mode: This is a blob_id and exposes the platform capabilties
> +	 * wrt to various gamma modes and the respective lut ranges. This also
> +	 * helps user select a gamma mode amongst the supported ones.
> +	 */
> +	u32 gamma_mode;
> +
> +	/**
>  	 * @degamma_lut:
>  	 *
>  	 * Lookup table for converting framebuffer pixel data before apply the
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 7f60e8e..8f961c5b 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -761,6 +761,12 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *content_type_property;
>  	/**
> +	 * @gamma_mode_property: Optional CRTC property to enumerate and
> +	 * select the mode of the crtc gamma/degmama LUTs. This also exposes
> +	 * the lut ranges of the various supported gamma modes to userspace.
> +	 */
> +	struct drm_property *gamma_mode_property;
> +	/**
>  	 * @degamma_lut_property: Optional CRTC property to set the LUT used to
>  	 * convert the framebuffer's colors to linear gamma.
>  	 */
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 83cd163..e70b7f8 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -630,6 +630,44 @@ struct drm_color_lut {
>  	__u16 reserved;
>  };
>  
> +/*
> + * DRM_MODE_LUT_GAMMA|DRM_MODE_LUT_DEGAMMA is legal and means the LUT
> + * can be used for either purpose, but not simultaneously. To expose
> + * modes that support gamma and degamma simultaneously the gamma mode
> + * must declare distinct DRM_MODE_LUT_GAMMA and DRM_MODE_LUT_DEGAMMA
> + * ranges.
> + */
> +/* LUT is for gamma (after CTM) */
> +#define DRM_MODE_LUT_GAMMA BIT(0)
> +/* LUT is for degamma (before CTM) */
> +#define DRM_MODE_LUT_DEGAMMA BIT(1)
> +/* linearly interpolate between the points */
> +#define DRM_MODE_LUT_INTERPOLATE BIT(2)
> +/*
> + * the last value of the previous range is the
> + * first value of the current range.
> + */
> +#define DRM_MODE_LUT_REUSE_LAST BIT(3)
> +/* the curve must be non-decreasing */
> +#define DRM_MODE_LUT_NON_DECREASING BIT(4)
> +/* the curve is reflected across origin for negative inputs */
> +#define DRM_MODE_LUT_REFLECT_NEGATIVE BIT(5)
> +/* the same curve (red) is used for blue and green channels as well */
> +#define DRM_MODE_LUT_SINGLE_CHANNEL BIT(6)
> +
> +struct drm_color_lut_range {
> +	/* DRM_MODE_LUT_* */
> +	__u32 flags;
> +	/* number of points on the curve */
> +	__u16 count;
> +	/* input/output bits per component */
> +	__u8 input_bpc, output_bpc;
> +	/* input start/end values */
> +	__s32 start, end;
> +	/* output min/max values */
> +	__s32 min, max;
> +};
> +
>  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
>  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
>  #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux