Re: [RFC PATCH v3 17/23] drm/colorop: Introduce DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5283 bytes --]

On Wed, 8 Nov 2023 11:36:36 -0500
Harry Wentland <harry.wentland@amd.com> wrote:

> With the introduction of the pre-blending color pipeline we
> can no longer have color operations that don't have a clear
> position in the color pipeline. We deprecate all existing
> plane properties. For upstream drivers those are:
>  - COLOR_ENCODING
>  - COLOR_RANGE
> 
> Userspace that registers with the
> DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE will be permitted to set
> the COLOR_PIPELINE plane property and drm_colorop properties.
> But it won't be allowed to set COLOR_ENCODING and
> COLOR_RANGE. Userspace that does not set this client cap
> will not be permitted to touch the color pipeline.
> 
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c | 23 ++++++++++++++++++++++-
>  drivers/gpu/drm/drm_ioctl.c       |  7 +++++++
>  include/drm/drm_file.h            |  7 +++++++
>  include/uapi/drm/drm.h            | 15 +++++++++++++++
>  4 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index c6629fdaa114..69c56982e2d0 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -610,10 +610,26 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>  	} else if (property == plane->zpos_property) {
>  		state->zpos = val;
>  	} else if (property == plane->color_encoding_property) {
> +		if (file_priv->plane_color_pipeline) {
> +			drm_dbg_atomic(dev,
> +				       "Setting COLOR_PIPELINE plane property not permitted when DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE is set\n");
> +			return -EINVAL;
> +		}
>  		state->color_encoding = val;
>  	} else if (property == plane->color_range_property) {
> +		if (file_priv->plane_color_pipeline) {
> +			drm_dbg_atomic(dev,
> +				       "Setting COLOR_PIPELINE plane property not permitted when DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE is set\n");
> +			return -EINVAL;
> +		}
>  		state->color_range = val;

The above two cases mention COLOR_PIPELINE when they should probably
refer to something else.


Thanks,
pq

>  	} else if (property == plane->color_pipeline_property) {
> +		if (!file_priv->plane_color_pipeline) {
> +			drm_dbg_atomic(dev,
> +				       "Setting COLOR_PIPELINE plane property not permitted unless DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE is set\n");
> +			return -EINVAL;
> +		}
> +
>  		/* find DRM colorop object */
>  		struct drm_colorop *colorop = NULL;
>  		colorop = drm_colorop_find(dev, file_priv, val);
> @@ -1158,6 +1174,12 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>  		break;
>  	}
>  	case DRM_MODE_OBJECT_COLOROP: {
> +		if (!file_priv->plane_color_pipeline) {
> +			drm_dbg_atomic(prop->dev,
> +				       "[OBJECT:%d] is a colorop but DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE not set\n",
> +				       obj->id);
> +			ret = -EINVAL;
> +		}
>  		struct drm_colorop *colorop = obj_to_colorop(obj);
>  		struct drm_colorop_state *colorop_state;
>  
> @@ -1170,7 +1192,6 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>  		ret = drm_atomic_colorop_set_property(colorop,
>  				colorop_state, file_priv,
>  				prop, prop_value);
> -
>  		break;
>  	}
>  	default:
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 44fda68c28ae..0d869658e13e 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -361,6 +361,13 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  			return -EINVAL;
>  		file_priv->writeback_connectors = req->value;
>  		break;
> +	case DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE:
> +		if (!file_priv->atomic)
> +			return -EINVAL;
> +		if (req->value > 1)
> +			return -EINVAL;
> +		file_priv->plane_color_pipeline = req->value;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index e1b5b4282f75..bf11b646c898 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -204,6 +204,13 @@ struct drm_file {
>  	 */
>  	bool writeback_connectors;
>  
> +	/**
> +	 * @plane_color_pipeline:
> +	 *
> +	 * True if client understands plane color pipelines
> +	 */
> +	bool plane_color_pipeline;
> +
>  	/**
>  	 * @was_master:
>  	 *
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 841d393fb84e..2576b170e8d0 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -842,6 +842,21 @@ struct drm_get_cap {
>   */
>  #define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	5
>  
> +/**
> + * DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE
> + *
> + * If set to 1, the DRM core will
> + * - expose plane COLOR_PIPELINE properties for pre-blending color management.
> + * - reject setting of these plane properties:
> + *   - COLOR_ENCODING
> + *   - COLOR_RANGE
> + *
> + * The client must enable &DRM_CLIENT_CAP_ATOMIC first.
> + *
> + * This capability is currently in development.
> + */
> +#define DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE	7
> +
>  /* DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
>  struct drm_set_client_cap {
>  	__u64 capability;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]